Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug

[Igor Mammedov]

On Thu, 28 Feb 2019 09:13:25 +0800
Wei Yang <address@hidden> wrote:

> On Wed, Feb 27, 2019 at 07:04:02PM +0100, Igor Mammedov wrote:
> >On Mon, 25 Feb 2019 09:15:34 +0800
> >Wei Yang <address@hidden> wrote:
> >  
> >> On Mon, Feb 25, 2019 at 09:07:08AM +0800, Wei Yang wrote:  
> >> >Currently we do device realization like below:
> >> >
> >> >   hotplug_handler_pre_plug()
> >> >   dc->realize()
> >> >   hotplug_handler_plug()
> >> >
> >> >Before we do device realization and plug, we should allocate necessary
> >> >resources and check if memory-hotplug-support property is enabled.
> >> >
> >> >At the piix4 and ich9, the memory-hotplug-support property is checked at
> >> >plug stage. This means that device has been realized and mapped into guest
> >> >address space 'pc_dimm_plug()' by the time acpi plug handler is called,
> >> >where it might fail and crash QEMU due to reaching g_assert_not_reached()
> >> >(piix4) or error_abort (ich9).
> >> >
> >> >Fix it by checking if memory hotplug is enabled at pre_plug stage
> >> >where we can gracefully abort hotplug request.
> >> >
> >> >Signed-off-by: Wei Yang <address@hidden>
> >> >CC: Igor Mammedov <address@hidden>
> >> >CC: Eric Blake <address@hidden>
> >> >Signed-off-by: Wei Yang <address@hidden>
> >> >
> >> >---
> >> >v5:
> >> >   * rebase on latest upstream
> >> >   * remove a comment before hotplug_handler_pre_plug
> >> >   * fix alignment for ich9_pm_device_pre_plug_cb
> >> >v4:
> >> >   * fix code alignment of piix4_device_pre_plug_cb
> >> >v3:
> >> >   * replace acpi_memory_hotplug with memory-hotplug-support in changelog
> >> >   * fix code alignment of ich9_pm_device_pre_plug_cb
> >> >   * print which device type memory-hotplug-support is disabled in
> >> >     ich9_pm_device_pre_plug_cb and piix4_device_pre_plug_cb
> >> >v2:
> >> >   * (address@hidden)
> >> >     - Almost the whole third paragraph
> >> >   * apply this change to ich9
> >> >   * use hotplug_handler_pre_plug() instead of open-coding check
> >> >---
> >> > hw/acpi/ich9.c         | 15 +++++++++++++--
> >> > hw/acpi/piix4.c        | 13 ++++++++++---
> >> > hw/i386/pc.c           |  2 ++
> >> > hw/isa/lpc_ich9.c      |  1 +
> >> > include/hw/acpi/ich9.h |  2 ++
> >> > 5 files changed, 28 insertions(+), 5 deletions(-)
> >> >
> >> >diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >> >index c5d8646abc..e53dfe1ee3 100644
> >> >--- a/hw/acpi/ich9.c
> >> >+++ b/hw/acpi/ich9.c
> >> >@@ -483,13 +483,24 @@ void ich9_pm_add_properties(Object *obj, 
> >> >ICH9LPCPMRegs *pm, Error **errp)
> >> >                              NULL);
> >> > }
> >> > 
> >> >+void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState 
> >> >*dev,
> >> >+                                Error **errp)
> >> >+{
> >> >+    ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
> >> >+
> >> >+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> >> >+        !lpc->pm.acpi_memory_hotplug.is_enabled)
> >> >+        error_setg(errp,
> >> >+                   "memory hotplug is not enabled: 
> >> >%s.memory-hotplug-support "
> >> >+                   "is not set", object_get_typename(OBJECT(lpc)));
> >> >+}
> >> >+
> >> > void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState 
> >> > *dev,
> >> >                             Error **errp)
> >> > {
> >> >     ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
> >> > 
> >> >-    if (lpc->pm.acpi_memory_hotplug.is_enabled &&
> >> >-        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >> >+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >> >         if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> >> >             nvdimm_acpi_plug_cb(hotplug_dev, dev);
> >> >         } else {
> >> >diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >> >index df8c0db909..8b9654e437 100644
> >> >--- a/hw/acpi/piix4.c
> >> >+++ b/hw/acpi/piix4.c
> >> >@@ -374,9 +374,16 @@ static void piix4_pm_powerdown_req(Notifier *n, void 
> >> >*opaque)
> >> > static void piix4_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> >> >                                     DeviceState *dev, Error **errp)    
> >> 
> >> Hmm, I found one interesting thing.
> >> 
> >> This function is introduced in
> >> 
> >>     commit ec266f408882fd38475f72c4e864ed576228643b
> >>     pci/pcihp: perform check for bus capability in pre_plug handler
> >> 
> >> This is just implemented in piix4, but don't has the counterpart in ich9.
> >> 
> >> So I suggest to have a follow up patch to create the counterpart for ich9 
> >> and
> >> then apply this patch on top of that.  
> >not sure what do you mean, could you be more specific?
> >  
> 
> Let me reply here.
> 
> Everything starts from device_set_realized().
> 
>     device_set_realized()
>        hotplug_handler_pre_plug()
>        dc->realize()
>        hotplug_handler_plug()
> 
> There is two choice of HotplugHandler :
> 
>     * dev's bus hotplug_handler
>     * machine_hotplug_handler
> 
> Take PIIX as an example, machine_hotplug_handler and pci_bus hotplug_handler
> is:
> 
>     pc_machine_device_[pre_]plug_cb
>     piix4_device_[pre_]plug_cb
> 
> if I am correct.
> 
> Now back to your question.
> 
> Commit ec266f408882 introduced piix4_device_pre_plug_cb() to move the check in
> pre_plug handler for PIIX. Then I am wondering the counter part of ich9, But I
> don't see something for PCI_DEVICE in ich9_pm_device_plug_cb(). So on ich9,
> PCI_DEVICE is not pluggable? Maybe I am lost here.
> 
> Or in other words, in case other machine supports PCI_DEVICE hotplug, do we
> need to move similar check to pre_plug too?
it depends, in case of ICH9. I think acpi hotplug is not used (thankfully)
it uses native PCI-E hotplug path. Following pops out when looking for it.
 hw/pci/pcie.c:void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, 
DeviceState *dev,
 hw/pci/pcie_port.c:    hc->pre_plug = pcie_cap_slot_pre_plug_cb;