Re: [PATCH-for-5.0 1/2] hw/acpi/piix4: Add 'system-hotplug-support' property

[Philippe Mathieu-Daudé]

On 3/19/20 11:44 AM, Igor Mammedov wrote:
> On Wed, 18 Mar 2020 23:15:30 +0100
> Philippe Mathieu-Daudé <address@hidden> wrote:
>
> > The I/O ranges registered by the piix4_acpi_system_hot_add_init()
> > function are not documented in the PIIX4 datasheet.
> > This appears to be a PC-only feature added in commit 5e3cb5347e
> > ("initialize hot add system / acpi gpe") which was then moved
> > to the PIIX4 device model in commit 9d5e77a22f ("make
> > qemu_system_device_hot_add piix independent")
> > Add a property (default enabled, to not modify the current
> > behavior) to allow machines wanting to model a simple PIIX4
> > to disable this feature.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>
> it's already pretty twisted code and adding one more knob
> to workaround other compat knobs makes it worse.
>
> Even though it's not really welcomed approach,
> we can ifdef all hotplug parts and compile them out for mips
> dropping along the way linking with not needed dependencies

We can't use use target-specific poisoned definitions to ifdef out in 
generic hw/ code.

> or
> more often used, make stubs from hotplug parts for mips
> and link with them.

So the problem is this device doesn't match the hardware datasheet, has 
extra features helping virtualization, and now we can not simplify it 
due to backward compat.

Once Michael said he doesn't care about the PIIX4, only the PIIX3 
southbridge [1] [2], but then the i440fx pc machine uses a PIIX3 with a 
pci PM function from PIIX4, and made that PII4_PM Frankenstein.

You are asking me to choose between worse versus ugly?

The saner outcome I see is make the current PIIX4_PM x86-specific, not 
modifying the code, and start a fresh new copy respecting the datasheet.

Note I'm not particularly interested in MIPS here, but having model 
respecting the hardware.

[1] https://www.mail-archive.com/address@hidden/msg504270.html
[2] https://www.mail-archive.com/address@hidden/msg601512.html

> > ---
> > Should I squash this with the next patch and start with
> > default=false, which is closer to the hardware model?
> > ---
> >   hw/acpi/piix4.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 964d6f5990..9c970336ac 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
> >   
> >       AcpiPciHpState acpi_pci_hotplug;
> >       bool use_acpi_pci_hotplug;
> > +    bool use_acpi_system_hotplug;
> >   
> >       uint8_t disable_s3;
> >       uint8_t disable_s4;
> > @@ -503,8 +504,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> >       s->machine_ready.notify = piix4_pm_machine_ready;
> >       qemu_add_machine_init_done_notifier(&s->machine_ready);
> >   
> > -    piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
> > -                                   pci_get_bus(dev), s);
> > +    if (s->use_acpi_system_hotplug) {
> > +        piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
> > +                                       pci_get_bus(dev), s);
> > +    }
> >       qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort);
> >   
> >       piix4_pm_add_propeties(s);
> > @@ -635,6 +638,8 @@ static Property piix4_pm_properties[] = {
> >                        use_acpi_pci_hotplug, true),
> >       DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> >                        acpi_memory_hotplug.is_enabled, true),
> > +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
> > +                     use_acpi_system_hotplug, true),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >