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