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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 08 Aug 2022 19:57:23 +0200
BB <shentey@gmail.com> wrote:

> Am 8. August 2022 14:15:40 MESZ schrieb Igor Mammedov <imammedo@redhat.com>:
> >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.  
> 
> Does this particular fix make future compatibility harder or easier or is it that hard already? IIUC it omits the hotplug bits in the vm state for Malta which is what one would expect there, right?
> 
> >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.  
> 
> Having two implementations of the same device means that we'll end up having duplicate code with board/platform-specific assumptions baked in. I guess what Phil cares about is a sustainable solution without hacks that doesn't cause bloat and/or regressions for MIPS, especially for features where MIPS doesn't benefit from. I believe that composition could be such a solution.

maybe creating PIIX4_PM-base without carrying any VMstate code and then
inheriting/branching that into piix4_pm-speced and PIIX4_PM, which will
carry it's own VMstate descriptors (with minimal duplication or somewhat
shared) can be made to work.

> My consolidation work could actually make PIIX4 an option for the PC machine. This means that PIIX4_PM wouldn't be Frankenstein any more. This works already on my branch - for both PC and Malta. Furthermore, it looks like it allowed Malta to benefit more from KVM virtualization, but that's off-topic in this discussion.
> 
> >> 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
> >> > >>    
> >> > >    
> >> >
> >> >
> >> >    
> >  
>