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

[Igor Mammedov]

On Thu, 19 Mar 2020 12:04:11 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:

> 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?
That 'ugly' is typically used within QEMU to deal with such things
probably due to its low complexity.

> 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.
properly implementing spec would be quite a task
(although if motivation is just for fun, then why not)

> 
> 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(),
> >>   };
> >>     
> >   
> 
>