Re: [PATCH v2 5/7] hw/char/exynos4210_uart: Implement Rx FIFO level triggers and timeouts

[Peter Maydell]

On Sat, 18 Jan 2020 at 16:42, Guenter Roeck <address@hidden> wrote:
>
> The driver already implements a receive FIFO, but it does not
> handle receive FIFO trigger levels and timeout. Implement the
> missing functionality.
>
> Signed-off-by: Guenter Roeck <address@hidden>
> ---
> v2: Call exynos4210_uart_rx_timeout_set() from new post_load function
>     to set the receive timeout timer.
>     Add timer to vmstate_exynos4210_uart.
>
>  hw/char/exynos4210_uart.c | 122 ++++++++++++++++++++++++++++++--------
>  hw/char/trace-events      |   3 +-
>  2 files changed, 99 insertions(+), 26 deletions(-)

Since the timeout value depends on s->wordtime, and
exynos4210_uart_update_parameters() can change s->wordtime,
do you need to recalculate the timeout at that point?
This would correspond to if the guest wrote to the
UBRDIV/UFRACVAL/ULCON registers, I think. Maybe this comes
under the heading of "undefined behaviour if the guest does
this odd thing" ? (The exact behaviour of the h/w is probably
undocumented and mildly painful to emulate exactly, so it's
hard to see why QEMU should care about getting it exactly right.)

I did also wonder whether writing the same value to the UCON
timeout-interval field repeatedly really does restart the timer
counting down from 8*(N+1) frames again, but again maybe that's
just weird for a guest to do.

> @@ -553,6 +620,7 @@ static const VMStateDescription vmstate_exynos4210_uart = 
> {
>                         vmstate_exynos4210_uart_fifo, Exynos4210UartFIFO),
>          VMSTATE_UINT32_ARRAY(reg, Exynos4210UartState,
>                               EXYNOS4210_UART_REGS_MEM_SIZE / 
> sizeof(uint32_t)),
> +        VMSTATE_TIMER_PTR(fifo_timeout_timer, Exynos4210UartState),
>          VMSTATE_END_OF_LIST()
>      }
>  };

Unfortunately you can't simply add entries to a VMStateDescription:
it breaks migration compatibility.

The choices here are:
 * the nicest approach if it works is that in the post_load
function you just recalculate the timer timeout. Then there's
no need to migrate the current state of the timer. (In fact
it looks like your code does do this in post_load.)
 * if something really does need adding to the migration state,
then the version_id and minimum_version_id need to be bumped
(so migration fails cleanly rather than confusingly).
 * if you want migration to continue to work across versions
(which we don't care about for the exynos boards but does
apply for boards like 'virt'), this can be done by adding
a 'subsection' to the vmstate.

I think the answer in this case is just "you don't need to
add this line to the vmstate at all". (This does mean that
a vmsave/vmload will slightly extend the rx-timeout and
delay the interrupt because we re-calculate the timer,
but I guess that's OK. If you don't like that and would
prefer the timer to retain the exact timeout value across
migration, then keep the new vmstate array entry, bump the
version fields, and don't call exynos4210_uart_rx_timeout_set()
in post-load.)

thanks
-- PMM