Re: [PATCH] hw/char/pl011: Fix clock migration failure

[Peter Maydell]

On Wed, 17 Mar 2021 at 10:59, Gavin Shan <gshan@redhat.com> wrote:
>
> Hi Peter,
>
> On 3/17/21 9:40 PM, Peter Maydell wrote:
> > On Wed, 17 Mar 2021 at 10:37, Gavin Shan <gshan@redhat.com> wrote:
> >> On 3/17/21 8:09 PM, Peter Maydell wrote:
> >>> On Wed, 17 Mar 2021 at 04:44, Gavin Shan <gshan@redhat.com> wrote:
> >>>>
> >>>>    static const VMStateDescription vmstate_pl011 = {
> >>>>        .name = "pl011",
> >>>>        .version_id = 2,
> >>>>        .minimum_version_id = 2,
> >>>> +    .post_load = pl011_post_load,
> >>>>        .fields = (VMStateField[]) {
> >>>>            VMSTATE_UINT32(readbuff, PL011State),
> >>>>            VMSTATE_UINT32(flags, PL011State),
> >>>> @@ -355,10 +355,6 @@ static const VMStateDescription vmstate_pl011 = {
> >>>>            VMSTATE_INT32(read_trigger, PL011State),
> >>>>            VMSTATE_END_OF_LIST()
> >>>>        },
> >>>> -    .subsections = (const VMStateDescription * []) {
> >>>> -        &vmstate_pl011_clock,
> >>>> -        NULL
> >>>> -    }
> >>>>    };
> >>>
> >>> Doesn't dropping the subsection break migration compat ?
> >>>
> >>
> >> It's why this patch needs to be backported to stable branches.
> >> In that way, we won't have migration compatible issue.
> >
> > No, migration has to work from the existing already
> > shipped 5.1, 5.2, etc releases to 6.0 (assuming you use
> > the correct "virt-5.2" &c versioned machine type.)
> >
>
> Commit aac63e0e6ea3 ("hw/char/pl011: add a clock input") is merged
> to v5.2.0. The migration failure happens during migration from v6.0
> to v5.1 with machine type as "virt-5.1", instead of migrating from
> v5.1 to v6.0. One question is if we need support backwards migration?

Upstream doesn't care about backwards migration. AIUI
RedHat as a downstream care about the backwards-migration
case in some specific situations, but I don't know if that
would include this one.

I misread the commit message of this patch and hadn't
realised that it was talking about a 5.2 -> 5.1 migration.
>From upstream's point of view, commit aac63e0e6ea3 is fine
because it preserves forwards migration (5.1 -> 5.2).
If there's a change that makes RH-as-a-downstream's life
easier without breaking upstream's forward-compat requirements,
we can look at it: but unless I'm misreading this patch,
it would break 5.2 -> 6.0 migration, because 5.2 sends the
subsection and 6.0 with this patch would say "I don't know
how to deal with this subsection I've been sent".

> If we do support backwards migration, I think there are two options:
> (1) merge this patch and backport it to v5.2+; (2) Backport commit
> aac63e0e6ea3 to v5.2-. I guess (1) would be right way to go because
> it's less effort than (2). Besides, the clock needn't to be migrated
> if I'm correct.

You can't fix a backwards migration issue (if you care about it)
by backporting any patch to anywhere -- you need to deal with what
has already shipped.

thanks
-- PMM