yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-serial-change-retry-logic-to-avoid-concurrency.patch

9ae3a8
From 19651bdbf15a4ce03d6fc6e3a6be514a3f46a118 Mon Sep 17 00:00:00 2001
9ae3a8
From: Fam Zheng <famz@redhat.com>
9ae3a8
Date: Thu, 18 May 2017 09:21:21 +0200
9ae3a8
Subject: [PATCH 08/18] serial: change retry logic to avoid concurrency
9ae3a8
9ae3a8
RH-Author: Fam Zheng <famz@redhat.com>
9ae3a8
Message-id: <20170518092131.16571-9-famz@redhat.com>
9ae3a8
Patchwork-id: 75300
9ae3a8
O-Subject: [RHEL-7.4 qemu-kvm PATCH v3 08/18] serial: change retry logic to avoid concurrency
9ae3a8
Bugzilla: 1451470
9ae3a8
RH-Acked-by: Paolo Bonzini <pbonzini@redhat.com>
9ae3a8
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
9ae3a8
RH-Acked-by: Eduardo Habkost <ehabkost@redhat.com>
9ae3a8
9ae3a8
From: Kirill Batuzov <batuzovk@ispras.ru>
9ae3a8
9ae3a8
Whenever serial_xmit fails to transmit a byte it adds a watch that would
9ae3a8
call it again when the "line" becomes ready. This results in a retry
9ae3a8
chain:
9ae3a8
  serial_xmit -> add_watch -> serial_xmit
9ae3a8
Each chain is able to transmit one character, and for every character
9ae3a8
passed to serial by the guest driver a new chain is spawned.
9ae3a8
9ae3a8
The problem lays with the fact that a new chain is spawned even when
9ae3a8
there is one already waiting on the watch. So there can be several retry
9ae3a8
chains waiting concurrently on one "line". Every chain tries to transmit
9ae3a8
current character, so character order is not messed up. But also every
9ae3a8
chain increases retry counter (tsr_retry). If there are enough
9ae3a8
concurrent chains this counter will hit MAX_XMIT_RETRY value and
9ae3a8
the character will be dropped.
9ae3a8
9ae3a8
To reproduce this bug you need to feed serial output to some program
9ae3a8
consuming it slowly enough. A python script from bug #1335444
9ae3a8
description is an example of such program.
9ae3a8
9ae3a8
This commit changes retry logic in the following way to avoid
9ae3a8
concurrency: instead of spawning a new chain for each character being
9ae3a8
transmitted spawn only one and make it transmit characters until FIFO is
9ae3a8
empty.
9ae3a8
9ae3a8
The change consists of two parts:
9ae3a8
 - add a do {} while () loop in serial_xmit (diff is a bit erratic
9ae3a8
   for this part, diff -w will show actual change),
9ae3a8
 - do not call serial_xmit from serial_ioport_write if there is one
9ae3a8
   waiting on the watch already.
9ae3a8
9ae3a8
This should fix another issue causing bug #1335444.
9ae3a8
9ae3a8
Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru>
9ae3a8
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
9ae3a8
(cherry picked from commit f702e62a193e9ddb41cef95068717e5582b39a64)
9ae3a8
Signed-off-by: Fam Zheng <famz@redhat.com>
9ae3a8
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
9ae3a8
---
9ae3a8
 hw/char/serial.c | 59 +++++++++++++++++++++++++++++++-------------------------
9ae3a8
 1 file changed, 33 insertions(+), 26 deletions(-)
9ae3a8
9ae3a8
diff --git a/hw/char/serial.c b/hw/char/serial.c
9ae3a8
index add4738..33e06fb 100644
9ae3a8
--- a/hw/char/serial.c
9ae3a8
+++ b/hw/char/serial.c
9ae3a8
@@ -223,37 +223,42 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
9ae3a8
 {
9ae3a8
     SerialState *s = opaque;
9ae3a8
 
9ae3a8
-    if (s->tsr_retry <= 0) {
9ae3a8
-        if (s->fcr & UART_FCR_FE) {
9ae3a8
-            if (fifo8_is_empty(&s->xmit_fifo)) {
9ae3a8
+    do {
9ae3a8
+        if (s->tsr_retry <= 0) {
9ae3a8
+            if (s->fcr & UART_FCR_FE) {
9ae3a8
+                if (fifo8_is_empty(&s->xmit_fifo)) {
9ae3a8
+                    return FALSE;
9ae3a8
+                }
9ae3a8
+                s->tsr = fifo8_pop(&s->xmit_fifo);
9ae3a8
+                if (!s->xmit_fifo.num) {
9ae3a8
+                    s->lsr |= UART_LSR_THRE;
9ae3a8
+                }
9ae3a8
+            } else if ((s->lsr & UART_LSR_THRE)) {
9ae3a8
                 return FALSE;
9ae3a8
-            }
9ae3a8
-            s->tsr = fifo8_pop(&s->xmit_fifo);
9ae3a8
-            if (!s->xmit_fifo.num) {
9ae3a8
+            } else {
9ae3a8
+                s->tsr = s->thr;
9ae3a8
                 s->lsr |= UART_LSR_THRE;
9ae3a8
+                s->lsr &= ~UART_LSR_TEMT;
9ae3a8
             }
9ae3a8
-        } else if ((s->lsr & UART_LSR_THRE)) {
9ae3a8
-            return FALSE;
9ae3a8
-        } else {
9ae3a8
-            s->tsr = s->thr;
9ae3a8
-            s->lsr |= UART_LSR_THRE;
9ae3a8
-            s->lsr &= ~UART_LSR_TEMT;
9ae3a8
         }
9ae3a8
-    }
9ae3a8
 
9ae3a8
-    if (s->mcr & UART_MCR_LOOP) {
9ae3a8
-        /* in loopback mode, say that we just received a char */
9ae3a8
-        serial_receive1(s, &s->tsr, 1);
9ae3a8
-    } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
9ae3a8
-        if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY &&
9ae3a8
-            qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s) > 0) {
9ae3a8
-            s->tsr_retry++;
9ae3a8
-            return FALSE;
9ae3a8
+        if (s->mcr & UART_MCR_LOOP) {
9ae3a8
+            /* in loopback mode, say that we just received a char */
9ae3a8
+            serial_receive1(s, &s->tsr, 1);
9ae3a8
+        } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
9ae3a8
+            if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY &&
9ae3a8
+                qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
9ae3a8
+                                      serial_xmit, s) > 0) {
9ae3a8
+                s->tsr_retry++;
9ae3a8
+                return FALSE;
9ae3a8
+            }
9ae3a8
+            s->tsr_retry = 0;
9ae3a8
+        } else {
9ae3a8
+            s->tsr_retry = 0;
9ae3a8
         }
9ae3a8
-        s->tsr_retry = 0;
9ae3a8
-    } else {
9ae3a8
-        s->tsr_retry = 0;
9ae3a8
-    }
9ae3a8
+        /* Transmit another byte if it is already available. It is only
9ae3a8
+           possible when FIFO is enabled and not empty. */
9ae3a8
+    } while ((s->fcr & UART_FCR_FE) && !fifo8_is_empty(&s->xmit_fifo));
9ae3a8
 
9ae3a8
     s->last_xmit_ts = qemu_get_clock_ns(vm_clock);
9ae3a8
 
9ae3a8
@@ -293,7 +298,9 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
9ae3a8
             s->thr_ipending = 0;
9ae3a8
             s->lsr &= ~UART_LSR_THRE;
9ae3a8
             serial_update_irq(s);
9ae3a8
-            serial_xmit(NULL, G_IO_OUT, s);
9ae3a8
+            if (s->tsr_retry <= 0) {
9ae3a8
+                serial_xmit(NULL, G_IO_OUT, s);
9ae3a8
+            }
9ae3a8
         }
9ae3a8
         break;
9ae3a8
     case 1:
9ae3a8
-- 
9ae3a8
1.8.3.1
9ae3a8