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

[Igor Mammedov]

On Wed, 3 Aug 2022 19:26:30 +0200
Bernhard Beschow <shentey@gmail.com> wrote:

> On Tue, Aug 2, 2022 at 8:37 AM Philippe Mathieu-Daudé via <
> qemu-devel@nongnu.org> wrote:
> 
> > On 28/7/22 15:16, Igor Mammedov wrote:  
> > > On Thu, 28 Jul 2022 13:29:07 +0100
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > >  
> > >> On Thu, 28 Jul 2022 at 12:50, 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)  
> >
> > We already discussed this Frankenstein PIIX4 problem 2 and 4 years ago:
> >
> > https://lore.kernel.org/qemu-devel/4d42697e-ba84-e5af-3a17-a2cc52cf0dbc@redhat.com/
> >
> > https://lore.kernel.org/qemu-devel/20190304210359-mutt-send-email-mst@kernel.org/
> >   
> 
> 
> Interesting reads!
> 
> 
> > >>> 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.  
> >
> > So now MIPS is forced to use meaningless compat[] to satisfy X86.
> >
> > Am I wrong seeing this as a dirty hack creeping in, yet another
> > technical debt that will hit (me...) back in a close future?
> >
> > Are we sure there are no better solution (probably more time consuming
> > and involving refactors) we could do instead?
> >  
> 
> Working on the consolidation of piix3 and -4 soutbridges [1] I've stumbled
> over certain design decisions where board/platform specific assumptions are
> baked into the piix device models. I figure that's the core of the issue.
> 
> In our case the ACPI functionality is implemented by inheritance while
> perhaps it should be implemented using composition. With composition, the
> ACPI functionality could be injected by the caller: The pc board would
> inject it while the Malta board wouldn't. This would solve both the crash
> and above design problem.

While refactoring we should keep migration stream compatible with older
QEMU versions (we must not regress widely x86 code path). Which might be
tricky in this case.

Perhaps the best we could do is follow up on Philippe's idea to make
PIIX4_PM frankenstein x86-specific (the least chance for regressions)
and create/use clean version for anything else.

> I'd be willing to implement it but can't make any promises about the time
> frame since I'm currently doing this in my free time. Any hints regarding
> the implementation would be welcome, though.
> 
> Best regards,
> Bernhard
> 
> [1] https://github.com/shentok/qemu/commits/piix-consolidate
> 
> 
> > Thanks,
> >
> > Phil.
> >  
> > >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > >>
> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/995
> > >>  
> > >>> ---
> > >>> PS:
> > >>> another approach could be setting defaults to disabled state and
> > >>> enabling them using compat props on PC machines (which is more
> > >>> code to deal with => more risky) or continue with PIIX4_PM
> > >>> refactoring to split x86-shism out (which I'm not really
> > >>> interested in due to risk of regressions for not much of
> > >>> benefit)
> > >>> ---
> > >>>   hw/mips/malta.c | 9 +++++++++
> > >>>   1 file changed, 9 insertions(+)
> > >>>
> > >>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > >>> index 7a0ec513b0..0e932988e0 100644
> > >>> --- a/hw/mips/malta.c
> > >>> +++ b/hw/mips/malta.c
> > >>> @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = {
> > >>>       .instance_init = mips_malta_instance_init,
> > >>>   };
> > >>>
> > >>> +GlobalProperty malta_compat[] = {
> > >>> +    { "PIIX4_PM", "memory-hotplug-support", "off" },
> > >>> +    { "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" },
> > >>> +    { "PIIX4_PM", "acpi-root-pci-hotplug", "off" },
> > >>> +    { "PIIX4_PM", "x-not-migrate-acpi-index", "true" },
> > >>> +};  
> > >>
> > >> Is there an easy way to assert in hw/acpi/piix4.c that if
> > >> CONFIG_ACPI_PCIHP was not set then the board has initialized
> > >> all these properties to the don't-use-hotplug state ?
> > >> That would be a guard against similar bugs (though I suppose
> > >> we probably aren't likely to add new piix4 boards...)  
> > >
> > > unfortunately new features still creep in 'pc' machine
> > > ex: "acpi-root-pci-hotplug"), and I don't see an easy
> > > way to compile that nor enforce that in the future.
> > >
> > > Far from easy would be split piix4_pm on base/enhanced
> > > classes so we wouldn't need x86 specific hacks in 'base'
> > > variant (assuming 'enhanced' could maintain the current
> > > VMSTATE to keep cross-version migration working).
> > >  
> > >>> +const size_t malta_compat_len = G_N_ELEMENTS(malta_compat);
> > >>> +
> > >>>   static void mips_malta_machine_init(MachineClass *mc)
> > >>>   {
> > >>>       mc->desc = "MIPS Malta Core LV";
> > >>> @@ -1455,6 +1463,7 @@ static void mips_malta_machine_init(MachineClass  
> > *mc)  
> > >>>       mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
> > >>>   #endif
> > >>>       mc->default_ram_id = "mips_malta.ram";
> > >>> +    compat_props_add(mc->compat_props, malta_compat,  
> > malta_compat_len);  
> > >>>   }
> > >>>
> > >>>   DEFINE_MACHINE("malta", mips_malta_machine_init)
> > >>> --
> > >>> 2.31.1  
> > >>
> > >> thanks
> > >> -- PMM
> > >>  
> > >  
> >
> >
> >