[RFC,v2,2/2] PC Chipset: Send serial bytes at correct rate

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 <cyrus296@gmail.com>
---
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(-)

* Calvin Lee (cyrus296@gmail.com) 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 <cyrus296@gmail.com>

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

> ---
> 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(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 4159a46a2f..542d949ccd 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -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),

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.

>          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

> +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);
>  
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 0acfbbc382..09aece90fb 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -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;
> -- 
> 2.17.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

> 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.

>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