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