[Qemu-devel] [PATCH 0/3] serial: fixes for migration

[Qemu-devel] [PATCH 0/3] serial: fixes for migration

[Paolo Bonzini]

While adding migration support for thr_ipending, I and Pavel chose
the default value of thr_ipending (absent the subsection) according
to what should be the common behavior of the serial device and its
drivers.

However, the condition did not actually match the common state of the
serial device because of emulation inaccuracies.  Thus the subsection
is almost always transmitted, which breaks migration from 2.2 to 2.1.

Patch 3 fixes these inaccuracies, but unmasks a missed interrupt bug
that upsets Windows.  In order to keep bisectability, that missed
interrupt is first fixed in patch 2.  And to close the circle, patch 1
cleans up more code that didn't match the datasheet too well (and was
also unnecessarily complicated); that was the first, unfruitful attempt
to fix the bug.

Datasheet available at http://www.ti.com/lit/ds/symlink/pc16550d.pdf

Paolo Bonzini (3):
  serial: fix THRE/TEMT handling
  serial: update LSR on enabling/disabling FIFOs
  serial: do not trigger THR interrupt after writing to IER

 hw/char/serial.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] serial: clean up THRE/TEMT handling

[Paolo Bonzini]

- assert THRE cleared and FIFO not empty (if enabled) before
sending a character.  Also assert TEMT cleared, since it is
the combination of THRE && transmitter shift register empty.

- raise THRI immediately after setting THRE

- check THRE to see if another character has to be sent,
which makes the assertions more obvious and also means TEMT
has to be set as soon as the loop ends

- clear TEMT together with THRE even in the non-FIFO case

There are certainly a couple bugfixes in here, but nothing that
squashes known bugs.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index ebcacdc..4cd139f 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -224,21 +224,23 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
     SerialState *s = opaque;
 
     do {
+        assert(!(s->lsr & UART_LSR_TEMT));
+        assert(!(s->lsr & UART_LSR_THRE));
+
         if (s->tsr_retry <= 0) {
             if (s->fcr & UART_FCR_FE) {
-                if (fifo8_is_empty(&s->xmit_fifo)) {
-                    return FALSE;
-                }
+                assert(!fifo8_is_empty(&s->xmit_fifo));
                 s->tsr = fifo8_pop(&s->xmit_fifo);
                 if (!s->xmit_fifo.num) {
                     s->lsr |= UART_LSR_THRE;
                 }
-            } else if ((s->lsr & UART_LSR_THRE)) {
-                return FALSE;
             } else {
                 s->tsr = s->thr;
                 s->lsr |= UART_LSR_THRE;
-                s->lsr &= ~UART_LSR_TEMT;
+            }
+            if ((s->lsr & UART_LSR_THRE) && !s->thr_ipending) {
+                s->thr_ipending = 1;
+                serial_update_irq(s);
             }
         }
 
@@ -256,17 +258,13 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
         } else {
             s->tsr_retry = 0;
         }
+
         /* Transmit another byte if it is already available. It is only
            possible when FIFO is enabled and not empty. */
-    } while ((s->fcr & UART_FCR_FE) && !fifo8_is_empty(&s->xmit_fifo));
+    } while (!(s->lsr & UART_LSR_THRE));
 
     s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
-    if (s->lsr & UART_LSR_THRE) {
-        s->lsr |= UART_LSR_TEMT;
-        s->thr_ipending = 1;
-        serial_update_irq(s);
-    }
+    s->lsr |= UART_LSR_TEMT;
 
     return FALSE;
 }
@@ -323,10 +321,10 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                     fifo8_pop(&s->xmit_fifo);
                 }
                 fifo8_push(&s->xmit_fifo, s->thr);
-                s->lsr &= ~UART_LSR_TEMT;
             }
             s->thr_ipending = 0;
             s->lsr &= ~UART_LSR_THRE;
+            s->lsr &= ~UART_LSR_TEMT;
             serial_update_irq(s);
             if (s->tsr_retry <= 0) {
                 serial_xmit(NULL, G_IO_OUT, s);
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] serial: update LSR on enabling/disabling FIFOs

[Paolo Bonzini]

When the transmit FIFOs is enabled, it is emptied and thus the transmitter
hold register is empty.  When it is disabled, the previous contents
of the transmitter hold register are discarded.  In either case, the
THRE bit in LSR must be set and THRI raised.

When the receive FIFO is enable, it is emptied and thus the data ready
and break bits must be cleared in LSR.  Likewise when the receive FIFO
is disabled.

The transmit changes fix avoid the need for thr_ipending bandaids in the
routines that handle writes to IER.  Without this patch, the next patch
would cause a missed THRI interrupt and break the Windows COM driver.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 4cd139f..f35fa42 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -363,12 +363,15 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
         /* FIFO clear */
 
         if (val & UART_FCR_RFR) {
+            s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
             timer_del(s->fifo_timeout_timer);
             s->timeout_ipending = 0;
             fifo8_reset(&s->recv_fifo);
         }
 
         if (val & UART_FCR_XFR) {
+            s->lsr |= UART_LSR_THRE;
+            s->thr_ipending = 1;
             fifo8_reset(&s->xmit_fifo);
         }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] serial: do not trigger THR interrupt after writing to IER

[Paolo Bonzini]

This is responsible for failure of migration from 2.2 to 2.1, because
thr_ipending is always one in practice.  Calling serial_update_irq is
the right thing to do indeed, because writing to IER could cause an
interrupt to appear.  However, there is no reason to set thr_ipending
again.

This was already reported in 2010.  See this quote from
https://lists.gnu.org/archive/html/qemu-devel/2010-03/msg01914.html:

> The commit in r1049 (serial interrupt fix (Hampa Hug)) prevents
> booting Digital Research DOSPlus. Following patch partially reverts
> that commit and makes DOSPlus booting in QEMU again.

Bochs does not check LSR_THRE in IER, and the log message in r1049 doesn't
explain why the change was made in the first place.

This does not change the migration format, so 2.2.0 -> 2.1 will remain
broken but we can fix 2.2.1 -> 2.1 without breaking 2.2.1 <-> 2.2.0.

Cc: qemu-stable@nongnu.org
Reported-by: Igor Mammedov <imammedo@redhat.com>
Reported-by: Roy Tam <roytam@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index f35fa42..2b7f4f5 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -348,10 +348,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                      s->poll_msl = 0;
                 }
             }
-            if (s->lsr & UART_LSR_THRE) {
-                s->thr_ipending = 1;
-                serial_update_irq(s);
-            }
+            serial_update_irq(s);
         }
         break;
     case 2:
-- 
1.8.3.1