[PATCH-for-5.0 0/2] hw/acpi/piix4: Restrict 'system hotplug' feature to i440fx PC machine

[PATCH-for-5.0 0/2] hw/acpi/piix4: Restrict 'system hotplug' feature to i440fx PC machine

[Philippe Mathieu-Daudé]

This feature is not documented in the PIIX4 datasheet. It appears
to be only used on the i440fx PC machine. Add a property to the
PIIX4_PM device to restrict its use. This fixes MIPS guest crashing
QEMU when accessing ioport 0xaf00.

Philippe Mathieu-Daudé (2):
  hw/acpi/piix4: Add 'system-hotplug-support' property
  hw/acpi/piix4: Restrict system-hotplug-support to x86 i440fx PC
    machine

 include/hw/southbridge/piix.h |  3 ++-
 hw/acpi/piix4.c               | 13 ++++++++++---
 hw/i386/pc_piix.c             |  1 +
 hw/isa/piix4.c                |  2 +-
 4 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.21.1

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

[Philippe Mathieu-Daudé]

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é <philmd@redhat.com>
---
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(),
 };
 
-- 
2.21.1

[PATCH-for-5.0 2/2] hw/acpi/piix4: Restrict system-hotplug-support to x86 i440fx PC machine

[Philippe Mathieu-Daudé]

The PC (i440fx) machine is the only one using the PIIX4 PM
specific system-hotplug-support feature.

Enable this feature in pc_init1(), and let the callers of
piix4_create() get a simple PIIX4 device.
This is the case of the MIPS Malta board.

Doing so we fix a bug on the Malta where a guest writing to
I/O port 0xaf00 crashes QEMU:

  qemu-system-mips: hw/acpi/cpu.c:197: cpu_hotplug_hw_init: Assertion `mc->possible_cpu_arch_ids' failed.
  Aborted (core dumped)
  (gdb) bt
  #0 0x00007f6fd748957f in raise () at /lib64/libc.so.6
  #1 0x00007f6fd7473895 in abort () at /lib64/libc.so.6
  #2 0x00007f6fd7473769 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
  #3 0x00007f6fd7481a26 in .annobin_assert.c_end () at /lib64/libc.so.6
  #4 0x00005646d58ca7bd in cpu_hotplug_hw_init (as=0x5646d6ae3300, owner=0x5646d6fd5b10, state=0x5646d6fd7a30, base_addr=44800) at hw/acpi/cpu.c:197
  #5 0x00005646d58c5284 in acpi_switch_to_modern_cphp (gpe_cpu=0x5646d6fd7910, cpuhp_state=0x5646d6fd7a30, io_port=44800) at hw/acpi/cpu_hotplug.c:107
  #6 0x00005646d58c3431 in piix4_set_cpu_hotplug_legacy (obj=0x5646d6fd5b10, value=false, errp=0x5646d61cdb28 <error_abort>) at hw/acpi/piix4.c:617
  #7 0x00005646d5b00c70 in property_set_bool (obj=0x5646d6fd5b10, v=0x5646d7697d30, name=0x5646d5cf3a90 "cpu-hotplug-legacy", opaque=0x5646d707d110, errp=0x5646d61cdb28 <error_abort>) at qom/object.c:2076
  #8 0x00005646d5afeee6 in object_property_set (obj=0x5646d6fd5b10, v=0x5646d7697d30, name=0x5646d5cf3a90 "cpu-hotplug-legacy", errp=0x5646d61cdb28 <error_abort>) at qom/object.c:1268
  #9 0x00005646d5b01fb8 in object_property_set_qobject (obj=0x5646d6fd5b10, value=0x5646d75b5450, name=0x5646d5cf3a90 "cpu-hotplug-legacy", errp=0x5646d61cdb28 <error_abort>) at qom/qom-qobject.c:26
  #10 0x00005646d5aff1cb in object_property_set_bool (obj=0x5646d6fd5b10, value=false, name=0x5646d5cf3a90 "cpu-hotplug-legacy", errp=0x5646d61cdb28 <error_abort>) at qom/object.c:1334
  #11 0x00005646d58c4fce in cpu_status_write (opaque=0x5646d6fd7910, addr=0, data=0, size=1) at hw/acpi/cpu_hotplug.c:44
  #12 0x00005646d569c707 in memory_region_write_accessor (mr=0x5646d6fd7920, addr=0, value=0x7ffc18053068, size=1, shift=0, mask=255, attrs=...) at memory.c:503
  #13 0x00005646d569c917 in access_with_adjusted_size (addr=0, value=0x7ffc18053068, size=1, access_size_min=1, access_size_max=4, access_fn=0x5646d569c61e <memory_region_write_accessor>, mr=0x5646d6fd7920, attrs=...) at memory.c:569
  #14 0x00005646d569f8f3 in memory_region_dispatch_write (mr=0x5646d6fd7920, addr=0, data=0, size=1, attrs=...) at memory.c:1497
  #15 0x00005646d563e5c5 in flatview_write_continue (fv=0x5646d751b000, addr=44800, attrs=..., buf=0x7ffc180531d4 "", len=4, addr1=0, l=1, mr=0x5646d6fd7920) at exec.c:3324
  #16 0x00005646d563e70a in flatview_write (fv=0x5646d751b000, addr=44800, attrs=..., buf=0x7ffc180531d4 "", len=4) at exec.c:3363
  #17 0x00005646d563ea0f in address_space_write (as=0x5646d618abc0 <address_space_io>, addr=44800, attrs=..., buf=0x7ffc180531d4 "", len=4) at exec.c:3453
  #18 0x00005646d5696ee5 in cpu_outl (addr=44800, val=0) at ioport.c:80

Buglink: https://bugs.launchpad.net/qemu/+bug/1835865
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/southbridge/piix.h | 3 ++-
 hw/acpi/piix4.c               | 4 +++-
 hw/i386/pc_piix.c             | 1 +
 hw/isa/piix4.c                | 2 +-
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 152628c6d9..3a54409cab 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -18,7 +18,8 @@
 
 I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                       qemu_irq sci_irq, qemu_irq smi_irq,
-                      int smm_enabled, DeviceState **piix4_pm);
+                      int smm_enabled, bool system_hotplug_enabled,
+                      DeviceState **piix4_pm);
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 9c970336ac..ec4869452b 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -515,7 +515,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
 
 I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                       qemu_irq sci_irq, qemu_irq smi_irq,
-                      int smm_enabled, DeviceState **piix4_pm)
+                      int smm_enabled, bool system_hotplug_enabled,
+                      DeviceState **piix4_pm)
 {
     DeviceState *dev;
     PIIX4PMState *s;
@@ -533,6 +534,7 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     if (xen_enabled()) {
         s->use_acpi_pci_hotplug = false;
     }
+    s->use_acpi_system_hotplug = system_hotplug_enabled;
 
     qdev_init_nofail(dev);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e2d98243bc..8441f44a14 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -283,6 +283,7 @@ else {
         pcms->smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
                                     x86ms->gsi[9], smi_irq,
                                     x86_machine_is_smm_enabled(x86ms),
+                                    true,
                                     &piix4_pm);
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
 
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 7edec5e149..6d6802d15d 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -262,7 +262,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
     pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
     if (smbus) {
         *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
-                               isa_get_irq(NULL, 9), NULL, 0, NULL);
+                               isa_get_irq(NULL, 9), NULL, 0, false, NULL);
    }
 
     return dev;
-- 
2.21.1

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

[Paolo Bonzini]

On 18/03/20 23:15, Philippe Mathieu-Daudé 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.

Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU.

> +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
> +                     use_acpi_system_hotplug, true),

Why not cpu-hotplug-support?

Paolo

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

[Philippe Mathieu-Daudé]

On 3/19/20 10:36 AM, Paolo Bonzini wrote:
> On 18/03/20 23:15, Philippe Mathieu-Daudé 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.
> 
> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU.
> 
>> +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
>> +                     use_acpi_system_hotplug, true),
> 
> Why not cpu-hotplug-support?

Because I have no idea what this code is about, and it seems more than 
cpu (pci, memory):

static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                                            PCIBus *bus, PIIX4PMState *s)
{
     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
                           "acpi-gpe0", GPE_LEN);
     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);

     acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
                     s->use_acpi_pci_hotplug);

     s->cpu_hotplug_legacy = true;
     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
                              piix4_get_cpu_hotplug_legacy,
                              piix4_set_cpu_hotplug_legacy,
                              NULL);
     legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
                                  PIIX4_CPU_HOTPLUG_IO_BASE);

     if (s->acpi_memory_hotplug.is_enabled) {
         acpi_memory_hotplug_init(parent, OBJECT(s), 
&s->acpi_memory_hotplug,
                                  ACPI_MEMORY_HOTPLUG_BASE);
     }
}

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

[Paolo Bonzini]

On 19/03/20 10:42, Philippe Mathieu-Daudé wrote:
> On 3/19/20 10:36 AM, Paolo Bonzini wrote:
>> On 18/03/20 23:15, Philippe Mathieu-Daudé 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.
>>
>> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU.
>>
>>> +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
>>> +                     use_acpi_system_hotplug, true),
>>
>> Why not cpu-hotplug-support?
> 
> Because I have no idea what this code is about, and it seems more than
> cpu (pci, memory):

Right, I should have been more verbose.  You mentioned I/O port 0xaf00
which is CPU hotplug.  Perhaps unless you can also crash with PCI
hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS
machines, and keep PCI hotplug.

Paolo

> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                                            PCIBus *bus, PIIX4PMState *s)
> {
>     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
>                           "acpi-gpe0", GPE_LEN);
>     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> 
>     acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
>                     s->use_acpi_pci_hotplug);
> 
>     s->cpu_hotplug_legacy = true;
>     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
>                              piix4_get_cpu_hotplug_legacy,
>                              piix4_set_cpu_hotplug_legacy,
>                              NULL);
>     legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
>                                  PIIX4_CPU_HOTPLUG_IO_BASE);
> 
>     if (s->acpi_memory_hotplug.is_enabled) {
>         acpi_memory_hotplug_init(parent, OBJECT(s),
> &s->acpi_memory_hotplug,
>                                  ACPI_MEMORY_HOTPLUG_BASE);
>     }
> }
> 

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

[Igor Mammedov]

On Wed, 18 Mar 2020 23:15:30 +0100
Philippe Mathieu-Daudé <philmd@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é <philmd@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
or
more often used, make stubs from hotplug parts for mips
and link with them.

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

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

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é <philmd@redhat.com> wrote:

> On 3/19/20 11:44 AM, Igor Mammedov wrote:
> > On Wed, 18 Mar 2020 23:15:30 +0100
> > Philippe Mathieu-Daudé <philmd@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é <philmd@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?
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/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(),
> >>   };
> >>     
> >   
> 
> 

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

[Philippe Mathieu-Daudé]

On 3/19/20 4:08 PM, Igor Mammedov wrote:
> On Thu, 19 Mar 2020 12:04:11 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 3/19/20 11:44 AM, Igor Mammedov wrote:
>>> On Wed, 18 Mar 2020 23:15:30 +0100
>>> Philippe Mathieu-Daudé <philmd@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é <philmd@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?
> 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/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(),
>>>>    };
>>>>      
>>>    
>>
>>
> 

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

[Paolo Bonzini]

On 22/03/20 17:27, Philippe Mathieu-Daudé wrote:
>>>
>> 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

It's made up.  The implementation is placed in PIIX4_PM because it is
referenced by the ACPI tables.  Real hardware would probably place this
in the ACPI embedded controller or in the BMC.

Paolo

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

[Marcelo Tosatti]

On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote:
> On 22/03/20 17:27, Philippe Mathieu-Daudé wrote:
> >>>
> >> 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
> 
> It's made up.  The implementation is placed in PIIX4_PM because it is
> referenced by the ACPI tables.  Real hardware would probably place this
> in the ACPI embedded controller or in the BMC.
> 
> Paolo

Yes, there was nothing at 0xafe0 at the time ACPI support was written.

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

[Philippe Mathieu-Daudé]

Hi Igor, Paolo.

On 3/23/20 12:04 PM, Marcelo Tosatti wrote:
> On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote:
>> On 22/03/20 17:27, Philippe Mathieu-Daudé wrote:
>>>>>
>>>> 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
>>
>> It's made up.  The implementation is placed in PIIX4_PM because it is
>> referenced by the ACPI tables.  Real hardware would probably place this
>> in the ACPI embedded controller or in the BMC.
>>
>> Paolo
> 
> Yes, there was nothing at 0xafe0 at the time ACPI support was written.
> 

Igor earlier said:
"it's already pretty twisted code and adding one more knob
to workaround other compat knobs makes it worse."

Is that OK to rename this file "hw/acpi/piix4_twisted.c" and
copy/paste the same content to "hw/acpi/piix4.c" but remove the
non-PIIX4 code (GPE from ICH9)?

This seems counterproductive from a maintenance PoV, but the PIIX4 bug
(https://bugs.launchpad.net/qemu/+bug/1835865) is more than 1 year old
now...

If someone has a clever idea, I'm open to listen and implement it, but
keeping ignoring this issue is not good.

Note there is a similar issue with the LPC bus not existing on the
PIIX, so maybe renaming this to something like "piix_virt.c" and having
someone writing the specs (or differences with the physical datasheet)
is not a such bad idea.

Thanks,

Phil.

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

[Marcelo Tosatti]

On Mon, Aug 03, 2020 at 07:10:11PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Igor, Paolo.
> 
> On 3/23/20 12:04 PM, Marcelo Tosatti wrote:
> > On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote:
> >> On 22/03/20 17:27, Philippe Mathieu-Daudé wrote:
> >>>>>
> >>>> 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
> >>
> >> It's made up.  The implementation is placed in PIIX4_PM because it is
> >> referenced by the ACPI tables.  Real hardware would probably place this
> >> in the ACPI embedded controller or in the BMC.
> >>
> >> Paolo
> > 
> > Yes, there was nothing at 0xafe0 at the time ACPI support was written.
> > 
> 
> Igor earlier said:
> "it's already pretty twisted code and adding one more knob
> to workaround other compat knobs makes it worse."
> 
> Is that OK to rename this file "hw/acpi/piix4_twisted.c" and
> copy/paste the same content to "hw/acpi/piix4.c" but remove the
> non-PIIX4 code (GPE from ICH9)?
> 
> This seems counterproductive from a maintenance PoV, but the PIIX4 bug
> (https://bugs.launchpad.net/qemu/+bug/1835865) is more than 1 year old
> now...
> 
> If someone has a clever idea, I'm open to listen and implement it, but
> keeping ignoring this issue is not good.
> 
> Note there is a similar issue with the LPC bus not existing on the
> PIIX, so maybe renaming this to something like "piix_virt.c" and having
> someone writing the specs (or differences with the physical datasheet)
> is not a such bad idea.
> 
> Thanks,
> 
> Phil.

Make the port address architecture specific ? 

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

[Philippe Mathieu-Daudé]

On 8/3/20 7:33 PM, Marcelo Tosatti wrote:
> On Mon, Aug 03, 2020 at 07:10:11PM +0200, Philippe Mathieu-Daudé wrote:
>> Hi Igor, Paolo.
>>
>> On 3/23/20 12:04 PM, Marcelo Tosatti wrote:
>>> On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote:
>>>> On 22/03/20 17:27, Philippe Mathieu-Daudé wrote:
>>>>>>>
>>>>>> 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
>>>>
>>>> It's made up.  The implementation is placed in PIIX4_PM because it is
>>>> referenced by the ACPI tables.  Real hardware would probably place this
>>>> in the ACPI embedded controller or in the BMC.
>>>>
>>>> Paolo
>>>
>>> Yes, there was nothing at 0xafe0 at the time ACPI support was written.
>>>
>>
>> Igor earlier said:
>> "it's already pretty twisted code and adding one more knob
>> to workaround other compat knobs makes it worse."
>>
>> Is that OK to rename this file "hw/acpi/piix4_twisted.c" and
>> copy/paste the same content to "hw/acpi/piix4.c" but remove the
>> non-PIIX4 code (GPE from ICH9)?
>>
>> This seems counterproductive from a maintenance PoV, but the PIIX4 bug
>> (https://bugs.launchpad.net/qemu/+bug/1835865) is more than 1 year old
>> now...
>>
>> If someone has a clever idea, I'm open to listen and implement it, but
>> keeping ignoring this issue is not good.
>>
>> Note there is a similar issue with the LPC bus not existing on the
>> PIIX, so maybe renaming this to something like "piix_virt.c" and having
>> someone writing the specs (or differences with the physical datasheet)
>> is not a such bad idea.
>>
>> Thanks,
>>
>> Phil.
> 
> Make the port address architecture specific ? 

I find it worse than using a property set on the PC machine only
(that would make the piix4 compiled for each target instead on only
once in common-obj as now). But if Igor is OK with that, I don't
mind much, let's move forward.

Thanks,

Phil.

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

[Philippe Mathieu-Daudé]

On 3/19/20 11:02 AM, Paolo Bonzini wrote:
> On 19/03/20 10:42, Philippe Mathieu-Daudé wrote:
>> On 3/19/20 10:36 AM, Paolo Bonzini wrote:
>>> On 18/03/20 23:15, Philippe Mathieu-Daudé 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.
>>>
>>> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU.
>>>
>>>> +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
>>>> +                     use_acpi_system_hotplug, true),
>>>
>>> Why not cpu-hotplug-support?
>>
>> Because I have no idea what this code is about, and it seems more than
>> cpu (pci, memory):
> 
> Right, I should have been more verbose.  You mentioned I/O port 0xaf00
> which is CPU hotplug.  Perhaps unless you can also crash with PCI
> hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS
> machines, and keep PCI hotplug.

I am sorry I don't understand what PCI hotplug has to do with PIIX which
is a PCI-slave southbridge... If MIPS or other arch is interested in PCI
hotplug feature, that would be managed by the northbridge or another PCI
bridge.

> 
> Paolo
> 
>> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>>                                            PCIBus *bus, PIIX4PMState *s)
>> {
>>     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
>>                           "acpi-gpe0", GPE_LEN);
>>     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>>
>>     acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
>>                     s->use_acpi_pci_hotplug);
>>
>>     s->cpu_hotplug_legacy = true;
>>     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
>>                              piix4_get_cpu_hotplug_legacy,
>>                              piix4_set_cpu_hotplug_legacy,
>>                              NULL);
>>     legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
>>                                  PIIX4_CPU_HOTPLUG_IO_BASE);
>>
>>     if (s->acpi_memory_hotplug.is_enabled) {
>>         acpi_memory_hotplug_init(parent, OBJECT(s),
>> &s->acpi_memory_hotplug,
>>                                  ACPI_MEMORY_HOTPLUG_BASE);
>>     }
>> }
>>
> 

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

[Philippe Mathieu-Daudé]

On 8/5/20 7:56 AM, Philippe Mathieu-Daudé wrote:
> On 3/19/20 11:02 AM, Paolo Bonzini wrote:
>> On 19/03/20 10:42, Philippe Mathieu-Daudé wrote:
>>> On 3/19/20 10:36 AM, Paolo Bonzini wrote:
>>>> On 18/03/20 23:15, Philippe Mathieu-Daudé 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.
>>>>
>>>> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU.
>>>>
>>>>> +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
>>>>> +                     use_acpi_system_hotplug, true),
>>>>
>>>> Why not cpu-hotplug-support?
>>>
>>> Because I have no idea what this code is about, and it seems more than
>>> cpu (pci, memory):
>>
>> Right, I should have been more verbose.  You mentioned I/O port 0xaf00
>> which is CPU hotplug.  Perhaps unless you can also crash with PCI
>> hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS
>> machines, and keep PCI hotplug.
> 
> I am sorry I don't understand what PCI hotplug has to do with PIIX which
> is a PCI-slave southbridge... If MIPS or other arch is interested in PCI
> hotplug feature, that would be managed by the northbridge or another PCI
> bridge.

Ah, writing that comment made me realize the problem might be these
'virtualization' features have been implemented in the wrong place.
Maybe the less disruptive path is to move them to the i440fx
northbridge. That way we shouldn't need to maintain a piix_hw.c and
piix_virt_twisted.c (child of piix_hw overloaded with hotplug features).

I haven't looked at the history yet, maybe the problem happened when
i440fx/piix was split.

> 
>>
>> Paolo
>>
>>> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>>>                                            PCIBus *bus, PIIX4PMState *s)
>>> {
>>>     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
>>>                           "acpi-gpe0", GPE_LEN);
>>>     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>>>
>>>     acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
>>>                     s->use_acpi_pci_hotplug);
>>>
>>>     s->cpu_hotplug_legacy = true;
>>>     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
>>>                              piix4_get_cpu_hotplug_legacy,
>>>                              piix4_set_cpu_hotplug_legacy,
>>>                              NULL);
>>>     legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
>>>                                  PIIX4_CPU_HOTPLUG_IO_BASE);
>>>
>>>     if (s->acpi_memory_hotplug.is_enabled) {
>>>         acpi_memory_hotplug_init(parent, OBJECT(s),
>>> &s->acpi_memory_hotplug,
>>>                                  ACPI_MEMORY_HOTPLUG_BASE);
>>>     }
>>> }
>>>
>>
> 

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

[Igor Mammedov]

On Wed, 5 Aug 2020 08:01:24 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 8/5/20 7:56 AM, Philippe Mathieu-Daudé wrote:
> > On 3/19/20 11:02 AM, Paolo Bonzini wrote:  
> >> On 19/03/20 10:42, Philippe Mathieu-Daudé wrote:  
> >>> On 3/19/20 10:36 AM, Paolo Bonzini wrote:  
> >>>> On 18/03/20 23:15, Philippe Mathieu-Daudé 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.  
> >>>>
> >>>> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU.
> >>>>  
> >>>>> +    DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState,
> >>>>> +                     use_acpi_system_hotplug, true),  
> >>>>
> >>>> Why not cpu-hotplug-support?  
> >>>
> >>> Because I have no idea what this code is about, and it seems more than
> >>> cpu (pci, memory):  
> >>
> >> Right, I should have been more verbose.  You mentioned I/O port 0xaf00
> >> which is CPU hotplug.  Perhaps unless you can also crash with PCI
> >> hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS
> >> machines, and keep PCI hotplug.  
> > 
> > I am sorry I don't understand what PCI hotplug has to do with PIIX which
> > is a PCI-slave southbridge... If MIPS or other arch is interested in PCI
> > hotplug feature, that would be managed by the northbridge or another PCI
> > bridge.  
> 
> Ah, writing that comment made me realize the problem might be these
> 'virtualization' features have been implemented in the wrong place.
> Maybe the less disruptive path is to move them to the i440fx
> northbridge.
not sure if this option is on the table atm.
You will need a means to remap migration state to another device to keep migration working.

>That way we shouldn't need to maintain a piix_hw.c and
> piix_virt_twisted.c (child of piix_hw overloaded with hotplug features).
that's path I'd consider, i.e. split out virt parts out from piix hw
and make virt child that will have our hacks on top of native piix.

Still, You will need to keep typenames on virt part as it's now
for not to break migration stream (but I'm not sure, CCing David).

> 
> I haven't looked at the history yet, maybe the problem happened when
> i440fx/piix was split.
My guess would be due piix_pm having ACPI hw in spec (like sci/pm) so adding
other related ACPI hw was considered as the least disruptive back then.

> 
> >   
> >>
> >> Paolo
> >>  
> >>> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> >>>                                            PCIBus *bus, PIIX4PMState *s)
> >>> {
> >>>     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> >>>                           "acpi-gpe0", GPE_LEN);
> >>>     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> >>>
> >>>     acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> >>>                     s->use_acpi_pci_hotplug);
> >>>
> >>>     s->cpu_hotplug_legacy = true;
> >>>     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> >>>                              piix4_get_cpu_hotplug_legacy,
> >>>                              piix4_set_cpu_hotplug_legacy,
> >>>                              NULL);
> >>>     legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
> >>>                                  PIIX4_CPU_HOTPLUG_IO_BASE);
> >>>
> >>>     if (s->acpi_memory_hotplug.is_enabled) {
> >>>         acpi_memory_hotplug_init(parent, OBJECT(s),
> >>> &s->acpi_memory_hotplug,
> >>>                                  ACPI_MEMORY_HOTPLUG_BASE);
> >>>     }
> >>> }
> >>>  
> >>  
> >   
>