On Thu, 19 Mar 2020 12:04:11 +0100 Philippe Mathieu-Daudé <[email protected]> wrote:
> On 3/19/20 11:44 AM, Igor Mammedov wrote: > > On Wed, 18 Mar 2020 23:15:30 +0100 > > Philippe Mathieu-Daudé <[email protected]> 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é <[email protected]> > > > > 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/[email protected]/msg504270.html > [2] https://www.mail-archive.com/[email protected]/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(), > >> }; > >> > > > >
