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

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> 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.

I'd rather have something that was explicitly poisoned rather than just
walking off the end of an uninitialised array and having to break out
gdb.

Dave

> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK