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

[Dr. David Alan Gilbert]

* Calvin Lee (address@hidden) 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 <address@hidden>

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 / address@hidden / Manchester, UK