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