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

[Igor Mammedov]

On Tue, 26 Feb 2019 11:37:32 +0800
Wei Yang <address@hidden> wrote:

> On Mon, Feb 25, 2019 at 09:15:34AM +0800, Wei Yang 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)  
> >  
> 
> Where will we invoke this check for pci devices?
> 
> pc_machine_device_pre_plug_cb() seems has no connection with this function. So
> how this pre_plug handler takes effect?
hotplug handler doesn't have to be machine, on contrary it's typically a bus 
owner
see hw/core/qdev.c: device_set_realized() and qdev_get_hotplug_handler()

you also might want to check relevant
 'PATCH v1 0/3] qdev: Hotplug handler chaining' thread
that's hopefully to be merged soon

> 
> Do I miss something?
> 
>