Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

[Peter Maydell]

On Fri, 29 Jul 2022 at 10:57, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 28 Jul 2022 16:12:34 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Thu, 28 Jul 2022 at 16:09, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> > >
> > > * Igor Mammedov (imammedo@redhat.com) wrote:
> > > > On Thu, 28 Jul 2022 15:44:20 +0100
> > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > > >
> > > > > * Igor Mammedov (imammedo@redhat.com) wrote:
> > > > > > QEMU crashes trying to save VMSTATE when only MIPS target are 
> > > > > > compiled in
> > > > > >   $ qemu-system-mips -monitor stdio
> > > > > >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> > > > > >   Segmentation fault (core dumped)
> > > > > >
> > > > > > It happens due to PIIX4_PM trying to parse hotplug vmstate 
> > > > > > structures
> > > > > > which are valid only for x86 and not for MIPS (as it requires ACPI
> > > > > > tables support which is not existent for ithe later)
> > > > > >
> > > > > > Issue was probably exposed by trying to cleanup/compile out unused
> > > > > > ACPI bits from MIPS target (but forgetting about migration bits).
> > > > > >
> > > > > > Disable compiled out features using compat properties as the least
> > > > > > risky way to deal with issue.
> > > > >
> > > > > Isn't the problem partially due to a 'stub' vmsd which isn't 
> > > > > terminated?
> > > >
> > > > Not sure what "'stub' vmsd" is, can you explain?
> > >
> > > In hw/acpi/acpi-pci-hotplug-stub.c there is :
> > > const VMStateDescription vmstate_acpi_pcihp_pci_status;
> I think that one is there only for linking purposes and not meant
> to be actually used.

Yes, exactly. The problem is that without this patch which
sets various properties it *does* get used...

> > > this seg happens when the migration code walks into that - this should
> > > always get populated with some of the minimal fields, in particular the
> > > .name and .fields array terminated with VMSTATE_END_OF_LIST().
> >
> > Either:
> >  (1) we should be sure the vmstate struct does not get used if the
> >      compile-time config has ended up with the stub
> > or
>
> >  (2) it needs to actually match the real vmstate struct, otherwise
> >      migration between a QEMU built with a config that just got the
> >      stub version and a QEMU built with a config that got the full
> >      version will break
> >
> > This patch does the former. Segfaulting if we got something wrong
> > and tried to use the vmstate when we weren't expecting to is
> > arguably better than producing an incompatible migration stream.
>
> > (Better still would be if we caught this on machine startup rather
> > than only when savevm was invoked.)
> Theoretically possible with a bunch of mips and x86 stubs, but ...
> we typically don't do this kind of checks for migration sake
> as that complicates things a lot in general.
> i.e. it's common to let migration fail in case of incompatible
> migration stream. It's not exactly friendly to user but it's
> graceful failure (assuming code is correct and not crashes QEMU)

The point here is that if we ever try to do a migrate with the
stub vmstate struct then that's a bug in QEMU. We should prefer
to catch those early and clearly.

-- PMM