Re: [PATCH v3 2/5] hw/char/pl011: add post_load hook for backwards-compatibility

[Peter Maydell]

On Fri, 20 Jan 2023 at 15:54, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
> Previous change slightly modified the way we handle data writes when
> FIFO is disabled. Previously we kept incrementing read_pos and were
> storing data at that position, although we only have a
> single-register-deep FIFO now. Then we changed it to always store data
> at pos 0.
>
> If guest disables FIFO and the proceeds to read data, it will work out
> fine, because we read from current read_pos before setting it to 0.
>
> However, to make code less fragile, introduce a post_load hook for
> PL011State and move fixup read FIFO state when FIFO is disabled. Since
> we are introducing a post_load hook, also do some sanity checking on
> untrusted incoming input state.
>
> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> ---
>  hw/char/pl011.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 3fa3b75d04..4df649a064 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -352,10 +352,35 @@ static const VMStateDescription vmstate_pl011_clock = {
>      }
>  };
>
> +static int pl011_post_load(void *opaque, int version_id)
> +{
> +    PL011State* s = opaque;
> +
> +    /* Sanity-check input state */
> +    if (s->read_pos >= ARRAY_SIZE(s->read_fifo) ||
> +        s->read_count > ARRAY_SIZE(s->read_fifo)) {
> +        return -1;
> +    }
> +
> +    if (version_id < 3 && !pl011_is_fifo_enabled(s)) {
> +        /*
> +         * Older versions of PL011 didn't ensure that the single
> +         * character in the FIFO in FIFO-disabled mode is in
> +         * element 0 of the array; convert to follow the current
> +         * code's assumptions.
> +         */
> +        s->read_fifo[0] = s->read_fifo[s->read_pos];
> +        s->read_pos = 0;
> +    }

You don't need to bump the version id and do this
check based on version ID. You can just check whether
the old state indicates that the data isn't in slot 0
of the array, the way I suggested in my comment on the
previous version of the patchset.

(New->old migration will work fine.)

thanks
-- PMM