[Qemu-devel] [PATCH v2 0/6] serial: flow control fixes

[Qemu-devel] [PATCH v2 0/6] serial: flow control fixes

[Paolo Bonzini]

v1->v2:
- use guint for GSource tags
- move migration patch last because it set watch_tag but never read it
- add more details in errors
- don't drop tsr_retry sanity check in migration patch

Paolo Bonzini (6):
  serial: make tsr_retry unsigned
  serial: simplify tsr_retry reset
  serial: separate serial_xmit and serial_watch_cb
  char: change qemu_chr_fe_add_watch to return unsigned
  serial: remove watch on reset
  serial: reinstate watch after migration

 hw/char/cadence_uart.c   |  9 ++++---
 hw/char/serial.c         | 67 ++++++++++++++++++++++++++++++++++++------------
 include/hw/char/serial.h |  3 ++-
 include/sysemu/char.h    | 16 ++++++++++--
 net/vhost-user.c         |  2 +-
 qemu-char.c              |  8 +++---
 6 files changed, 78 insertions(+), 27 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v2 1/6] serial: make tsr_retry unsigned

[Paolo Bonzini]

It can never become negative; reflect this in the type of the field
and simplify the conditions.

Tested-by: Bret Ketchum <bcketchum@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c         | 12 ++++++++----
 include/hw/char/serial.h |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 6d815b5..e65e9e0 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -229,7 +229,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
 
     do {
         assert(!(s->lsr & UART_LSR_TEMT));
-        if (s->tsr_retry <= 0) {
+        if (s->tsr_retry == 0) {
             assert(!(s->lsr & UART_LSR_THRE));
 
             if (s->fcr & UART_FCR_FE) {
@@ -252,7 +252,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
             /* in loopback mode, say that we just received a char */
             serial_receive1(s, &s->tsr, 1);
         } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
-            if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY &&
+            if (s->tsr_retry < MAX_XMIT_RETRY &&
                 qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
                                       serial_xmit, s) > 0) {
                 s->tsr_retry++;
@@ -330,7 +330,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
             s->lsr &= ~UART_LSR_THRE;
             s->lsr &= ~UART_LSR_TEMT;
             serial_update_irq(s);
-            if (s->tsr_retry <= 0) {
+            if (s->tsr_retry == 0) {
                 serial_xmit(NULL, G_IO_OUT, s);
             }
         }
@@ -639,6 +639,10 @@ static int serial_post_load(void *opaque, int version_id)
     if (s->thr_ipending == -1) {
         s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
     }
+    if (s->tsr_retry > MAX_XMIT_RETRY) {
+        s->tsr_retry = MAX_XMIT_RETRY;
+    }
+
     s->last_break_enable = (s->lcr >> 6) & 1;
     /* Initialize fcr via setter to perform essential side-effects */
     serial_write_fcr(s, s->fcr_vmstate);
@@ -685,7 +689,7 @@ static const VMStateDescription vmstate_serial_tsr = {
     .minimum_version_id = 1,
     .needed = serial_tsr_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_INT32(tsr_retry, SerialState),
+        VMSTATE_UINT32(tsr_retry, SerialState),
         VMSTATE_UINT8(thr, SerialState),
         VMSTATE_UINT8(tsr, SerialState),
         VMSTATE_END_OF_LIST()
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 15beb6b..6a322eb 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -55,7 +55,7 @@ struct SerialState {
     int last_break_enable;
     int it_shift;
     int baudbase;
-    int tsr_retry;
+    uint32_t tsr_retry;
     uint32_t wakeup;
 
     /* Time when the last byte was successfully sent out of the tsr */
-- 
2.5.5

[Qemu-devel] [PATCH v2 2/6] serial: simplify tsr_retry reset

[Paolo Bonzini]

Move common code outside the if, and reset tsr_retry even in loopback mode.
Right now it cannot become non-zero, but it will be possible as soon as
we start respecting the baud rate.

Tested-by: Bret Ketchum <bcketchum@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index e65e9e0..904b218 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -258,10 +258,8 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
                 s->tsr_retry++;
                 return FALSE;
             }
-            s->tsr_retry = 0;
-        } else {
-            s->tsr_retry = 0;
         }
+        s->tsr_retry = 0;
 
         /* Transmit another byte if it is already available. It is only
            possible when FIFO is enabled and not empty. */
-- 
2.5.5

[Qemu-devel] [PATCH v2 3/6] serial: separate serial_xmit and serial_watch_cb

[Paolo Bonzini]

serial_xmit starts transmission of whatever is in the transmitter
register, THR or FIFO; serial_watch_cb is a wrapper around it and is
only used as a qemu_chr_fe_add_watch callback.

Tested-by: Bret Ketchum <bcketchum@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 904b218..0b09094 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -106,6 +106,7 @@ do {} while (0)
 #endif
 
 static void serial_receive1(void *opaque, const uint8_t *buf, int size);
+static void serial_xmit(SerialState *s);
 
 static inline void recv_fifo_put(SerialState *s, uint8_t chr)
 {
@@ -223,10 +224,16 @@ static void serial_update_msl(SerialState *s)
     }
 }
 
-static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
+                                void *opaque)
 {
     SerialState *s = opaque;
+    serial_xmit(s);
+    return FALSE;
+}
 
+static void serial_xmit(SerialState *s)
+{
     do {
         assert(!(s->lsr & UART_LSR_TEMT));
         if (s->tsr_retry == 0) {
@@ -254,9 +261,9 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
         } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
             if (s->tsr_retry < MAX_XMIT_RETRY &&
                 qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
-                                      serial_xmit, s) > 0) {
+                                      serial_watch_cb, s) > 0) {
                 s->tsr_retry++;
-                return FALSE;
+                return;
             }
         }
         s->tsr_retry = 0;
@@ -267,11 +274,8 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
 
     s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     s->lsr |= UART_LSR_TEMT;
-
-    return FALSE;
 }
 
-
 /* Setter for FCR.
    is_load flag means, that value is set while loading VM state
    and interrupt should not be invoked */
@@ -329,7 +333,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
             s->lsr &= ~UART_LSR_TEMT;
             serial_update_irq(s);
             if (s->tsr_retry == 0) {
-                serial_xmit(NULL, G_IO_OUT, s);
+                serial_xmit(s);
             }
         }
         break;
-- 
2.5.5

[Qemu-devel] [PATCH v2 4/6] char: change qemu_chr_fe_add_watch to return unsigned

[Paolo Bonzini]

g_source_attach can return any value between 1 and UINT_MAX if you let
QEMU run long enough.  However, qemu_chr_fe_add_watch can also return
a negative errno value when the device is disconnected or does not
support chr_add_watch.  Change it to return zero to avoid overloading
these values.

Fix the cadence_uart which asserts in this case (easily obtained with
"-serial pty").

Tested-by: Bret Ketchum <bcketchum@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/cadence_uart.c |  9 ++++++---
 include/sysemu/char.h  | 16 ++++++++++++++--
 net/vhost-user.c       |  2 +-
 qemu-char.c            |  8 ++++----
 4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index c856fc3..65179fa 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -292,9 +292,12 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
     memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count);
 
     if (s->tx_count) {
-        int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
-                                      cadence_uart_xmit, s);
-        assert(r);
+        guint r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
+                                        cadence_uart_xmit, s);
+        if (!r) {
+            s->tx_count = 0;
+            return FALSE;
+        }
     }
 
     uart_update_status(s);
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 1eb2d0f..57df10a 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -221,8 +221,20 @@ void qemu_chr_fe_event(CharDriverState *s, int event);
 void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 
-int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
-                          GIOFunc func, void *user_data);
+/**
+ * @qemu_chr_fe_add_watch:
+ *
+ * If the backend is connected, create and add a #GSource that fires
+ * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP)
+ * is active; return the #GSource's tag.  If it is disconnected,
+ * return 0.
+ *
+ * @cond the condition to poll for
+ * @func the function to call when the condition happens
+ * @user_data the opaque pointer to pass to @func
+ */
+guint qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
+                            GIOFunc func, void *user_data);
 
 /**
  * @qemu_chr_fe_write:
diff --git a/net/vhost-user.c b/net/vhost-user.c
index d72ce9b..636899a 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -22,7 +22,7 @@ typedef struct VhostUserState {
     NetClientState nc;
     CharDriverState *chr;
     VHostNetState *vhost_net;
-    int watch;
+    guint watch;
     uint64_t acked_features;
 } VhostUserState;
 
diff --git a/qemu-char.c b/qemu-char.c
index 016badb..4aeafe8 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3966,19 +3966,19 @@ void qemu_chr_fe_event(struct CharDriverState *chr, int event)
     }
 }
 
-int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
-                          GIOFunc func, void *user_data)
+guint qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
+                            GIOFunc func, void *user_data)
 {
     GSource *src;
     guint tag;
 
     if (s->chr_add_watch == NULL) {
-        return -ENOSYS;
+        return 0;
     }
 
     src = s->chr_add_watch(s, cond);
     if (!src) {
-        return -EINVAL;
+        return 0;
     }
 
     g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
-- 
2.5.5

[Qemu-devel] [PATCH v2 5/6] serial: remove watch on reset

[Paolo Bonzini]

Otherwise, this can cause serial_xmit to be entered with LSR.TEMT=0,
which is invalid and causes an assertion failure.

Reported-by: Bret Ketchum <bcketchum@gmail.com>
Tested-by: Bret Ketchum <bcketchum@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c         | 16 ++++++++++++----
 include/hw/char/serial.h |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 0b09094..af39e8f 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -228,6 +228,7 @@ static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
                                 void *opaque)
 {
     SerialState *s = opaque;
+    s->watch_tag = 0;
     serial_xmit(s);
     return FALSE;
 }
@@ -258,10 +259,12 @@ static void serial_xmit(SerialState *s)
         if (s->mcr & UART_MCR_LOOP) {
             /* in loopback mode, say that we just received a char */
             serial_receive1(s, &s->tsr, 1);
-        } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
-            if (s->tsr_retry < MAX_XMIT_RETRY &&
-                qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
-                                      serial_watch_cb, s) > 0) {
+        } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1 &&
+                   s->tsr_retry < MAX_XMIT_RETRY) {
+            assert(s->watch_tag == 0);
+            s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
+                                                 serial_watch_cb, s);
+            if (s->watch_tag > 0) {
                 s->tsr_retry++;
                 return;
             }
@@ -821,6 +824,11 @@ static void serial_reset(void *opaque)
 {
     SerialState *s = opaque;
 
+    if (s->watch_tag > 0) {
+        g_source_remove(s->watch_tag);
+        s->watch_tag = 0;
+    }
+
     s->rbr = 0;
     s->ier = 0;
     s->iir = UART_IIR_NO_INT;
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 6a322eb..9feddc6 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -56,6 +56,7 @@ struct SerialState {
     int it_shift;
     int baudbase;
     uint32_t tsr_retry;
+    guint watch_tag;
     uint32_t wakeup;
 
     /* Time when the last byte was successfully sent out of the tsr */
-- 
2.5.5

[Qemu-devel] [PATCH v2 6/6] serial: reinstate watch after migration

[Paolo Bonzini]

Otherwise, a serial port can get stuck if it is migrated while flow control
is in effect.

Tested-by: Bret Ketchum <bcketchum@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index af39e8f..3442f47 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -644,8 +644,29 @@ static int serial_post_load(void *opaque, int version_id)
     if (s->thr_ipending == -1) {
         s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
     }
-    if (s->tsr_retry > MAX_XMIT_RETRY) {
-        s->tsr_retry = MAX_XMIT_RETRY;
+
+    if (s->tsr_retry > 0) {
+        /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty).  */
+        if (s->lsr & UART_LSR_TEMT) {
+            error_report("inconsistent state in serial device "
+                         "(tsr empty, tsr_retry=%d", s->tsr_retry);
+            return -1;
+        }
+
+        if (s->tsr_retry > MAX_XMIT_RETRY) {
+            s->tsr_retry = MAX_XMIT_RETRY;
+        }
+
+        assert(s->watch_tag == 0);
+        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
+                                             serial_watch_cb, s);
+    } else {
+        /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty).  */
+        if (!(s->lsr & UART_LSR_TEMT)) {
+            error_report("inconsistent state in serial device "
+                         "(tsr not empty, tsr_retry=0");
+            return -1;
+        }
     }
 
     s->last_break_enable = (s->lcr >> 6) & 1;
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 0/6] serial: flow control fixes

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> v1->v2:
> - use guint for GSource tags
> - move migration patch last because it set watch_tag but never read it
> - add more details in errors
> - don't drop tsr_retry sanity check in migration patch

Thanks,
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Note you have a 1 character typo in both of your errors, 
+                         "(tsr empty, tsr_retry=%d", s->tsr_retry);
+                         "(tsr not empty, tsr_retry=0");

I think you want to add the close brackets, i.e.:
+                         "(tsr empty, tsr_retry=%d)", s->tsr_retry);
+                         "(tsr not empty, tsr_retry=0)");

Dave

> 
> Paolo Bonzini (6):
>   serial: make tsr_retry unsigned
>   serial: simplify tsr_retry reset
>   serial: separate serial_xmit and serial_watch_cb
>   char: change qemu_chr_fe_add_watch to return unsigned
>   serial: remove watch on reset
>   serial: reinstate watch after migration
> 
>  hw/char/cadence_uart.c   |  9 ++++---
>  hw/char/serial.c         | 67 ++++++++++++++++++++++++++++++++++++------------
>  include/hw/char/serial.h |  3 ++-
>  include/sysemu/char.h    | 16 ++++++++++--
>  net/vhost-user.c         |  2 +-
>  qemu-char.c              |  8 +++---
>  6 files changed, 78 insertions(+), 27 deletions(-)
> 
> -- 
> 2.5.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK