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

[Gavin Shan]

Hi Drew,

On 3/17/21 11:54 PM, Andrew Jones wrote:
> On Wed, Mar 17, 2021 at 11:14:56AM +0000, Peter Maydell wrote:
> > On Wed, 17 Mar 2021 at 10:59, Gavin Shan <gshan@redhat.com> wrote:
> > > 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.
>
> Right, we do prefer to be able to support "ping-pong" migrations. For
> example, if we start a virt-5.1 machine on a 5.1 build of QEMU, and then
> migrate it to a 5.2 build of QEMU, we'd like to also be able to go back
> to the 5.1 build.
>
> I agree this patch is not the right approach. I think the right approach
> is to introduce a compat property and make the "new" section dependent
> on it. And then update the hw_compat_* arrays. Gavin, please take a look
> at "Connecting subsections to properties" of docs/devel/migration.rst.

Agree and thanks for the pointer. I will post another patch to have
something in hw_compat_5_1 to address this particular issue.

> I'm also curious what the state of mach-virt's machine types are for
> migration. It'd be nice to exhaustively test both forward migration of
> all machine types and ping-pong migrations of all machine types. We can
> then consider each issue we find (the pessimist in me suggests we'll find
> more than this pl011 issue) and how/if we want to resolve them.

Yeah, I will think about it and do the testing to see if there are more
issues. Also, it'd better to be integrated to existing testing framework
as you suggested.

Thanks,
Gavin