[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH-for-5.0 1/2] hw/acpi/piix4: Add 'system-hotplug-support' property
Date: Sun, 22 Mar 2020 17:27:24 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 3/19/20 4:08 PM, Igor Mammedov wrote:
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.

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.

OK. Can you point me to the documentation for this feature? I can find reference of GPE in the ICH9, but I can't find where this IO address on the PIIX4 comes from:

#define GPE_BASE 0xafe0

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)

Is not for fun.

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;
- 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);
@@ -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),

reply via email to

[Prev in Thread] Current Thread [Next in Thread]