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

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

[Paolo Bonzini]

The main fixes here are in patch 2 and patch 6: watches are lost
after migration, and not removed on reset.

The rest are cleanups; patch 5 fixes the qemu_chr_fe_add_watch
API, which botches its return value pretty badly.

Paolo

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

 hw/char/cadence_uart.c   |  5 +++-
 hw/char/serial.c         | 59 +++++++++++++++++++++++++++++++++++-------------
 include/hw/char/serial.h |  3 ++-
 include/sysemu/char.h    | 16 +++++++++++--
 qemu-char.c              |  8 +++----
 5 files changed, 67 insertions(+), 24 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 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.

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..ef90615 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;
+    unsigned tsr_retry;
     uint32_t wakeup;
 
     /* Time when the last byte was successfully sent out of the tsr */
-- 
2.5.5

[Qemu-devel] [PATCH 2/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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c         | 16 ++++++++++++++--
 include/hw/char/serial.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index e65e9e0..6f0a49e 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -639,8 +639,20 @@ 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) {
+            return -1;
+        }
+
+        assert(s->watch_tag == 0);
+        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
+    } else {
+        /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty).  */
+        if (!(s->lsr & UART_LSR_TEMT)) {
+            return -1;
+        }
     }
 
     s->last_break_enable = (s->lcr >> 6) & 1;
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index ef90615..2ab835c 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -56,6 +56,7 @@ struct SerialState {
     int it_shift;
     int baudbase;
     unsigned tsr_retry;
+    unsigned watch_tag;
     uint32_t wakeup;
 
     /* Time when the last byte was successfully sent out of the tsr */
-- 
2.5.5

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

[Paolo Bonzini]

serial_xmit starts transmission of whatever is in the FIFO or THR;
serial_watch_cb is a wrapper around it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 6f0a49e..4196a2e 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;
         } else {
@@ -269,11 +276,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 */
@@ -331,7 +335,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;
@@ -647,7 +651,8 @@ static int serial_post_load(void *opaque, int version_id)
         }
 
         assert(s->watch_tag == 0);
-        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
+        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)) {
-- 
2.5.5

[Qemu-devel] [PATCH 4/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.

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 4196a2e..d232473 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -265,10 +265,8 @@ static void serial_xmit(SerialState *s)
                 s->tsr_retry++;
                 return;
             }
-            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 5/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").

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/cadence_uart.c |  5 ++++-
 include/sysemu/char.h  | 16 ++++++++++++++--
 qemu-char.c            |  8 ++++----
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index c856fc3..488a570 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -294,7 +294,10 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
     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);
+        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..07434a0 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
+ */
+unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
+                               GIOFunc func, void *user_data);
 
 /**
  * @qemu_chr_fe_write:
diff --git a/qemu-char.c b/qemu-char.c
index 84f49ac..39b2ccd 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)
+unsigned 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 6/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>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index d232473..7c196e2 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;
             }
@@ -834,6 +837,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;
-- 
2.5.5

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

[Bret Ketchum]

Tested-by: Bret Ketchum <bcketchum@gmail.com>

On Mon, Jun 20, 2016 at 9:29 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index d232473..7c196e2 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;
>              }
> @@ -834,6 +837,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;
> --
> 2.5.5
>

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> It can never become negative; reflect this in the type of the field
> and simplify the conditions.

OK, yes, the one case that could make it go -ve was removed in fcfb4d6
somewhere after 1.4.0.


> 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;
> +    }

Good that makes it safe even after receiving the case that could be -1.

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


>      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..ef90615 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;
> +    unsigned tsr_retry;
>      uint32_t wakeup;
>  
>      /* Time when the last byte was successfully sent out of the tsr */
> -- 
> 2.5.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Otherwise, a serial port can get stuck if it is migrated while flow control
> is in effect.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c         | 16 ++++++++++++++--
>  include/hw/char/serial.h |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index e65e9e0..6f0a49e 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -639,8 +639,20 @@ 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;

Why remove the check you just added?

> +
> +    if (s->tsr_retry > 0) {
> +        /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty).  */
> +        if (s->lsr & UART_LSR_TEMT) {
> +            return -1;
> +        }
> +
> +        assert(s->watch_tag == 0);
> +        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
> +    } else {
> +        /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty).  */
> +        if (!(s->lsr & UART_LSR_TEMT)) {
> +            return -1;

If you're failing migration (-1) then please error_report to say why
so when someone hits it I can immediately see why.

Dave

> +        }
>      }
>  
>      s->last_break_enable = (s->lcr >> 6) & 1;
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index ef90615..2ab835c 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -56,6 +56,7 @@ struct SerialState {
>      int it_shift;
>      int baudbase;
>      unsigned tsr_retry;
> +    unsigned watch_tag;
>      uint32_t wakeup;
>  
>      /* Time when the last byte was successfully sent out of the tsr */
> -- 
> 2.5.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> serial_xmit starts transmission of whatever is in the FIFO or THR;
> serial_watch_cb is a wrapper around it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 6f0a49e..4196a2e 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;
> +}

Yes, with a side question.
We register this with G_IO_OUT|G_IO_HUP  but don't check the cond, what's this
code supposed to do if it gets a HUP?

Since we didn't do anything with cond before either:

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


>  
> +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;
>          } else {
> @@ -269,11 +276,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 */
> @@ -331,7 +335,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;
> @@ -647,7 +651,8 @@ static int serial_post_load(void *opaque, int version_id)
>          }
>  
>          assert(s->watch_tag == 0);
> -        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
> +        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)) {
> -- 
> 2.5.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Paolo Bonzini]

On 22/06/2016 17:05, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> Otherwise, a serial port can get stuck if it is migrated while flow control
>> is in effect.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/char/serial.c         | 16 ++++++++++++++--
>>  include/hw/char/serial.h |  1 +
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>> index e65e9e0..6f0a49e 100644
>> --- a/hw/char/serial.c
>> +++ b/hw/char/serial.c
>> @@ -639,8 +639,20 @@ 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;
> 
> Why remove the check you just added?

Because I'm dumb. :)

>> +
>> +    if (s->tsr_retry > 0) {
>> +        /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty).  */
>> +        if (s->lsr & UART_LSR_TEMT) {
>> +            return -1;
>> +        }
>> +
>> +        assert(s->watch_tag == 0);
>> +        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
>> +    } else {
>> +        /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty).  */
>> +        if (!(s->lsr & UART_LSR_TEMT)) {
>> +            return -1;
> 
> If you're failing migration (-1) then please error_report to say why
> so when someone hits it I can immediately see why.

Ok, something like

    error_report("inconsistent state in serial device");

?

Paolo

Re: [Qemu-devel] [PATCH 4/6] serial: simplify tsr_retry reset

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@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 4196a2e..d232473 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -265,10 +265,8 @@ static void serial_xmit(SerialState *s)
>                  s->tsr_retry++;
>                  return;
>              }
> -            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
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Paolo Bonzini]

On 22/06/2016 17:09, Dr. David Alan Gilbert wrote:
> > -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;
> > +}
> 
> Yes, with a side question.
> We register this with G_IO_OUT|G_IO_HUP  but don't check the cond, what's this
> code supposed to do if it gets a HUP?

It will attempt again to write, which will fail (see pty_chr_write);
then attempt to add a watch again, which this time should return 0 (see
pty_chr_add_watch).  The outcome is that tsr_retry is reset to 0.

Thanks,

Paolo

> Since we didn't do anything with cond before either:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 

Re: [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 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").
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/cadence_uart.c |  5 ++++-
>  include/sysemu/char.h  | 16 ++++++++++++++--
>  qemu-char.c            |  8 ++++----
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index c856fc3..488a570 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -294,7 +294,10 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
>      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);
> +        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..07434a0 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
> + */
> +unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> +                               GIOFunc func, void *user_data);

can we make this a guint?  hw/char/virtio-console.c and monitor.c already
use guint as the type for a watch and that matches the glib definition?
(Although not net/vhost-user.c which still has int, but that should probably
be fixed anyway).

But other than that; 

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

Dave

>  
>  /**
>   * @qemu_chr_fe_write:
> diff --git a/qemu-char.c b/qemu-char.c
> index 84f49ac..39b2ccd 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)
> +unsigned 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
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Otherwise, this can cause serial_xmit to be entered with LSR.TEMT=0,
> which is invalid and causes an assertion failure.

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

> Reported-by: Bret Ketchum <bcketchum@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index d232473..7c196e2 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;
>              }
> @@ -834,6 +837,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;
> -- 
> 2.5.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 22/06/2016 17:05, Dr. David Alan Gilbert wrote:
> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
> >> Otherwise, a serial port can get stuck if it is migrated while flow control
> >> is in effect.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  hw/char/serial.c         | 16 ++++++++++++++--
> >>  include/hw/char/serial.h |  1 +
> >>  2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/char/serial.c b/hw/char/serial.c
> >> index e65e9e0..6f0a49e 100644
> >> --- a/hw/char/serial.c
> >> +++ b/hw/char/serial.c
> >> @@ -639,8 +639,20 @@ 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;
> > 
> > Why remove the check you just added?
> 
> Because I'm dumb. :)
> 
> >> +
> >> +    if (s->tsr_retry > 0) {
> >> +        /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty).  */
> >> +        if (s->lsr & UART_LSR_TEMT) {
> >> +            return -1;
> >> +        }
> >> +
> >> +        assert(s->watch_tag == 0);
> >> +        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
> >> +    } else {
> >> +        /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty).  */
> >> +        if (!(s->lsr & UART_LSR_TEMT)) {
> >> +            return -1;
> > 
> > If you're failing migration (-1) then please error_report to say why
> > so when someone hits it I can immediately see why.
> 
> Ok, something like
> 
>     error_report("inconsistent state in serial device");
> 
> ?

Yeh, feel free to add tsr_retry and lsr to the message as well so that if you're
looking at it you can figure out what happened.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK