* Calvin Lee (cyrus...@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 <cyrus...@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 / dgilb...@redhat.com / Manchester, UK
