Fix UART serial implementation

RFC PATCH In review Calvin Lee 3 participants 6 messages May 12, 2018 – Jul 15, 2018

lore

Share & tools

lore

Calvin Lee

author

2018-05-12 00:05 UTC #0 lore raw

v2 cover Fix UART serial implementation

Hello,

While developing a serial implementation for my OS, I found several bugs in QEMU's serial device. I confirmed (by testing on my x64 laptop) that there are several inconsistancies between QEMU and hardware in this regard. For both patches, I used "http://www.sci.muni.cz/docs/pc/serport.txt" for reference.

First, QEMU has several errors for setting the UART divisor that are fixed in my first patch. Second, (and more importantly) QEMU does not transmit serial bytes at the correct rate, and this is fixed in my second patch.

I have neither contributed to QEMU nor sent patches to a mailing list before, so I marked this patch-series as RFC. I would appreciate any comments you have. Also, I am not very confident in VM migration between versions, so I would appreciate if someone could help me make sure this is correct in my patches.

v2:
fix build

Calvin Lee (2):
PC Chipset: Improve serial divisor calculation
PC Chipset: Send serial bytes at correct rate

 hw/char/serial.c         | 74 +++++++++++++++++++++++++++++++---------
 include/hw/char/serial.h |  2 ++
 2 files changed, 60 insertions(+), 16 deletions(-)

Signature · 1 line

2.17.0

Calvin Lee

author

2018-05-12 00:05 UTC #1 lore raw

RFC PATCH v2 1/2 PC Chipset: Improve serial divisor calculation

This fixes several problems I found in the UART serial implementation. Now all divisor values are allowed, while before divisor values of zero and below the base baud rate were rejected. All changes are in reference to http://www.sci.muni.cz/docs/pc/serport.txt

Signed-off-by Calvin Lee <[email protected]>

---
I included a slight code-style change in this commit because it seemed close enough to the code I was editing to be relevant. If not, I can change the commit to not include this change.

 hw/char/serial.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

hw/char/serial.c +13 ~~-10~~

index 2c080c9862..4159a46a2f 100644
@@ -150,13 +150,10 @@ static void serial_update_irq(SerialState *s)
 
 static void serial_update_parameters(SerialState *s)
 {
-    int speed, parity, data_bits, stop_bits, frame_size;
+    float speed;
+    int parity, data_bits, stop_bits, frame_size;
     QEMUSerialSetParams ssp;
 
-    if (s->divider == 0 || s->divider > s->baudbase) {
-        return;
-    }
-
     /* Start bit. */
     frame_size = 1;
     if (s->lcr & 0x08) {

@@ -169,14 +166,16 @@ static void serial_update_parameters(SerialState *s)
     } else {
             parity = 'N';
     }
-    if (s->lcr & 0x04)
+    if (s->lcr & 0x04) {
         stop_bits = 2;
-    else
+    } else {
         stop_bits = 1;
+    }
 
     data_bits = (s->lcr & 0x03) + 5;
     frame_size += data_bits + stop_bits;
-    speed = s->baudbase / s->divider;
+    /* Zero divisor should give about 3500 baud */
+    speed = (s->divider == 0) ? 3500 : (float) s->baudbase / s->divider;
     ssp.speed = speed;
     ssp.parity = parity;
     ssp.data_bits = data_bits;

@@ -184,7 +183,7 @@ static void serial_update_parameters(SerialState *s)
     s->char_transmit_time =  (NANOSECONDS_PER_SECOND / speed) * frame_size;
     qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
 
-    DPRINTF("speed=%d parity=%c data=%d stop=%d\n",
+    DPRINTF("speed=%.2f parity=%c data=%d stop=%d\n",
            speed, parity, data_bits, stop_bits);
 }

@@ -341,7 +340,11 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
     default:
     case 0:
         if (s->lcr & UART_LCR_DLAB) {
-            s->divider = (s->divider & 0xff00) | val;
+            if (size == 2) {
+                s->divider = (s->divider & 0xff00) | val;
+            } else if (size == 4) {
+                s->divider = val;
+            }
             serial_update_parameters(s);
         } else {
             s->thr = (uint8_t) val;

Signature · 1 line

2.17.0

Calvin Lee

author

2018-05-12 00:05 UTC #2 lore raw

RFC PATCH v2 2/2 PC Chipset: Send serial bytes at correct rate

In reply to Calvin Lee

This fixes bug in QEMU such that UART bytes would be sent immediatly after being put in the THR regardless of the UART frequency (and divisor). Now they will be sent at the appropriate rate.

Signed-off-by Calvin Lee <[email protected]>

---
I am not sure about VM migration here. I want to move a struct field from one VMStateDescription to a new one. I did this by bumping the version number on the old VMStateDescription, and kept the field as _TEST. If this is incorrect please let me know.

 hw/char/serial.c         | 51 +++++++++++++++++++++++++++++++++++-----
 include/hw/char/serial.h |  2 ++
 2 files changed, 47 insertions(+), 6 deletions(-)

hw/char/serial.c +45 -6

index 4159a46a2f..542d949ccd 100644
@@ -359,9 +359,8 @@ 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) {
-                serial_xmit(s);
-            }
+            timer_mod(s->xmit_timeout_timer,
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->char_transmit_time);
         }
         break;
     case 1:

@@ -586,6 +585,15 @@ static void serial_receive_break(SerialState *s)
     serial_update_irq(s);
 }
 
+/* There is data to be sent in xmit_fifo or the thr */
+static void xmit_timeout_int(void *opaque)
+{
+    SerialState *s = opaque;
+    if (s->tsr_retry == 0) {
+        serial_xmit(s);
+    }
+}
+
 /* There's data in recv_fifo and s->rbr has not been read for 4 char transmit times */
 static void fifo_timeout_int (void *opaque) {
     SerialState *s = opaque;

@@ -723,15 +731,20 @@ static bool serial_tsr_needed(void *opaque)
     SerialState *s = (SerialState *)opaque;
     return s->tsr_retry != 0;
 }
+static bool serial_tsr_thr_exists(void *opaque, int version_id)
+{
+    return version_id < 2;
+}
 
 static const VMStateDescription vmstate_serial_tsr = {
     .name = "serial/tsr",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .needed = serial_tsr_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(tsr_retry, SerialState),
-        VMSTATE_UINT8(thr, SerialState),
+        /* Moved to `xmit_timeout_timer` */
+        VMSTATE_UINT8_TEST(thr, SerialState, serial_tsr_thr_exists),
         VMSTATE_UINT8(tsr, SerialState),
         VMSTATE_END_OF_LIST()
     }

@@ -772,6 +785,24 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
     }
 };
 
+static bool serial_xmit_timeout_timer_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return timer_pending(s->xmit_timeout_timer);
+}
+
+static const VMStateDescription vmstate_serial_xmit_timeout_timer = {
+    .name = "serial/xmit_timeout_timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = serial_xmit_timeout_timer_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(thr, SerialState),
+        VMSTATE_TIMER_PTR(xmit_timeout_timer, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool serial_fifo_timeout_timer_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;

@@ -849,6 +880,7 @@ const VMStateDescription vmstate_serial = {
         &vmstate_serial_tsr,
         &vmstate_serial_recv_fifo,
         &vmstate_serial_xmit_fifo,
+        &vmstate_serial_xmit_timeout_timer,
         &vmstate_serial_fifo_timeout_timer,
         &vmstate_serial_timeout_ipending,
         &vmstate_serial_poll,

@@ -880,6 +912,7 @@ static void serial_reset(void *opaque)
     s->poll_msl = 0;
 
     s->timeout_ipending = 0;
+    timer_del(s->xmit_timeout_timer);
     timer_del(s->fifo_timeout_timer);
     timer_del(s->modem_status_poll);

@@ -928,7 +961,10 @@ void serial_realize_core(SerialState *s, Error **errp)
 {
     s->modem_status_poll = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) serial_update_msl, s);
 
-    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
+    s->xmit_timeout_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) xmit_timeout_int, s);
+    s->fifo_timeout_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
     qemu_register_reset(serial_reset, s);
 
     qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1, serial_receive1,

@@ -945,6 +981,9 @@ void serial_exit_core(SerialState *s)
     timer_del(s->modem_status_poll);
     timer_free(s->modem_status_poll);
 
+    timer_del(s->xmit_timeout_timer);
+    timer_free(s->xmit_timeout_timer);
+
     timer_del(s->fifo_timeout_timer);
     timer_free(s->fifo_timeout_timer);

include/hw/char/serial.h +2 -0

index 0acfbbc382..09aece90fb 100644
@@ -65,6 +65,8 @@ struct SerialState {
     /* Time when the last byte was successfully sent out of the tsr */
     uint64_t last_xmit_ts;
     Fifo8 recv_fifo;
+    /* Time to read the next byte from the thr */
+    QEMUTimer *xmit_timeout_timer;
     Fifo8 xmit_fifo;
     /* Interrupt trigger level for recv_fifo */
     uint8_t recv_fifo_itl;

Signature · 1 line

2.17.0

Dr. David Alan Gilbert

2018-05-14 19:13 UTC #3 lore raw

Re: [Qemu-devel] [PATCH RFC v2 2/2] PC Chipset: Send serial bytes at correct rate

Quoted 5 lines · * Calvin Lee ([email protected]) wrote:

This fixes bug in QEMU such that UART bytes would be sent immediatly after being put in the THR regardless of the UART frequency (and divisor). Now they will be sent at the appropriate rate.
Signed-off-by Calvin Lee <[email protected]>

Hi Calvin,
I'll leave the details of the serial to Paolo, but for the migration
some comments below.

Quoted 52 lines

 hw/char/serial.c         | 51 +++++++++++++++++++++++++++++++++++-----
 include/hw/char/serial.h |  2 ++
 2 files changed, 47 insertions(+), 6 deletions(-)

hw/char/serial.c +15 -3

index 4159a46a2f..542d949ccd 100644
@@ -359,9 +359,8 @@ 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) {
-                serial_xmit(s);
-            }
+            timer_mod(s->xmit_timeout_timer,
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->char_transmit_time);
         }
         break;
     case 1:

@@ -586,6 +585,15 @@ static void serial_receive_break(SerialState *s)
     serial_update_irq(s);
 }
 
+/* There is data to be sent in xmit_fifo or the thr */
+static void xmit_timeout_int(void *opaque)
+{
+    SerialState *s = opaque;
+    if (s->tsr_retry == 0) {
+        serial_xmit(s);
+    }
+}
+
 /* There's data in recv_fifo and s->rbr has not been read for 4 char transmit times */
 static void fifo_timeout_int (void *opaque) {
     SerialState *s = opaque;

@@ -723,15 +731,20 @@ static bool serial_tsr_needed(void *opaque)
     SerialState *s = (SerialState *)opaque;
     return s->tsr_retry != 0;
 }
+static bool serial_tsr_thr_exists(void *opaque, int version_id)
+{
+    return version_id < 2;
+}
 
 static const VMStateDescription vmstate_serial_tsr = {

hw/char/serial.c static bool serial_tsr_needed(void *opaque) · line 747

Quoted 10 lines

hw/char/serial.c +3 -2

     .name = "serial/tsr",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .needed = serial_tsr_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(tsr_retry, SerialState),
-        VMSTATE_UINT8(thr, SerialState),
+        /* Moved to `xmit_timeout_timer` */
+        VMSTATE_UINT8_TEST(thr, SerialState, serial_tsr_thr_exists),

So the question is, why move it? If I understand what you've got, then it's the same flag with the same semantics - but moving it you break migration compatibility - so unless you need to, just leave it where it is.

I think it's probably better if you leave the version_id = 1, and actually keep serial_tsr_thr_exists just for compatibility. You just need to fill in a sane value so that if you migrate to an older version it doesn't confuse the old one.

hw/char/serial.c static const VMStateDescription vmstate_serial_xmit_fifo = { · line 793

Quoted 13 lines

hw/char/serial.c +6 -0

         VMSTATE_UINT8(tsr, SerialState),
         VMSTATE_END_OF_LIST()
     }

@@ -772,6 +785,24 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
     }
 };
 
+static bool serial_xmit_timeout_timer_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return timer_pending(s->xmit_timeout_timer);
+}
+

If you add a property/flag on the device, and check the property in that _needed function, then we can make it so that we don't save that section for older machine types.

Dave

Quoted 69 lines

hw/char/serial.c +21 -1

+static const VMStateDescription vmstate_serial_xmit_timeout_timer = {
+    .name = "serial/xmit_timeout_timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = serial_xmit_timeout_timer_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(thr, SerialState),
+        VMSTATE_TIMER_PTR(xmit_timeout_timer, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool serial_fifo_timeout_timer_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;

@@ -849,6 +880,7 @@ const VMStateDescription vmstate_serial = {
         &vmstate_serial_tsr,
         &vmstate_serial_recv_fifo,
         &vmstate_serial_xmit_fifo,
+        &vmstate_serial_xmit_timeout_timer,
         &vmstate_serial_fifo_timeout_timer,
         &vmstate_serial_timeout_ipending,
         &vmstate_serial_poll,

@@ -880,6 +912,7 @@ static void serial_reset(void *opaque)
     s->poll_msl = 0;
 
     s->timeout_ipending = 0;
+    timer_del(s->xmit_timeout_timer);
     timer_del(s->fifo_timeout_timer);
     timer_del(s->modem_status_poll);

@@ -928,7 +961,10 @@ void serial_realize_core(SerialState *s, Error **errp)
 {
     s->modem_status_poll = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) serial_update_msl, s);
 
-    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
+    s->xmit_timeout_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) xmit_timeout_int, s);
+    s->fifo_timeout_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
     qemu_register_reset(serial_reset, s);
 
     qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1, serial_receive1,

@@ -945,6 +981,9 @@ void serial_exit_core(SerialState *s)
     timer_del(s->modem_status_poll);
     timer_free(s->modem_status_poll);
 
+    timer_del(s->xmit_timeout_timer);
+    timer_free(s->xmit_timeout_timer);
+
     timer_del(s->fifo_timeout_timer);
     timer_free(s->fifo_timeout_timer);

include/hw/char/serial.h +2 -0

index 0acfbbc382..09aece90fb 100644
@@ -65,6 +65,8 @@ struct SerialState {
     /* Time when the last byte was successfully sent out of the tsr */
     uint64_t last_xmit_ts;
     Fifo8 recv_fifo;
+    /* Time to read the next byte from the thr */
+    QEMUTimer *xmit_timeout_timer;
     Fifo8 xmit_fifo;
     /* Interrupt trigger level for recv_fifo */
     uint8_t recv_fifo_itl;

Signature · 1 line

2.17.0

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Calvin Lee

author

2018-05-14 19:39 UTC #4 lore raw

Re: [Qemu-devel] [PATCH RFC v2 2/2] PC Chipset: Send serial bytes at correct rate

Quoted 4 lines

So the question is, why move it? If I understand what you've got, then it's the same flag with the same semantics - but moving it you break migration compatibility - so unless you need to, just leave it where it is.

Sorry if I did not make this clear.

This commit will delay flushing the THR until after the new timer has expired. Before, it would be immediatly flushed into the TSR, and was saved by vmstate_serial_tsr. However, now if the xmit_timeout_timer is pending it needs to be saved, that's why I moved it into the new VMStateDescription.

Quoted 3 lines

If you add a property/flag on the device, and check the property in that _needed function, then we can make it so that we don't save that section for older machine types.

I don't really understand. How do I make sure not to save this for older machines?

Thanks,
Calvin

Paolo Bonzini

2018-07-15 15:57 UTC #5 lore raw

In reply to Calvin Lee

Quoted 10 lines · Calvin Lee

While developing a serial implementation for my OS, I found several bugs in QEMU's serial device. I confirmed (by testing on my x64 laptop) that there are several inconsistancies between QEMU and hardware in this regard. For both patches, I used "http://www.sci.muni.cz/docs/pc/serport.txt" for reference.
First, QEMU has several errors for setting the UART divisor that are fixed in my first patch. Second, (and more importantly) QEMU does not transmit serial bytes at the correct rate, and this is fixed in my second patch.

Hi Calvin,

I am queuing the first patch. Regarding the second, it's been debated quite a few times over the years whether QEMU should respect the transmit rate or not. The main advantage of not doing so is that some OSes use the serial port pretty heavily (e.g. Linux for boot messages, or Windows for debugging), and throttling the serial port to just e.g. 115200 bps makes them unnecessarily slow.

Paolo