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