Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types

[Wei Yang]

On Sun, Apr 07, 2019 at 11:23:14AM +0200, Thomas Huth wrote:
>QEMU currently crashes when you try to hot-plug an "nvdimm" device
>on older machine types:
>
>$ qemu-system-x86_64 -monitor stdio -M pc-1.1
>QEMU 3.1.92 monitor - type 'help' for more information
>(qemu) device_add nvdimm,id=nvdimmn1
>qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv:
> Assertion `*errp == ((void *)0)' failed.
>Aborted (core dumped)
>
>The call to hotplug_handler_pre_plug() in pc_memory_pre_plug() has been
>added recently before the check whether nvdimm is enabled. It should
>be done after the check. And while we're at it, also check the errp
>after the hotplug_handler_pre_plug(), otherwise errors are silently
>ignored here.

Thomas,

Thanks for pointing this out, while I have some different idea on how to fix
this.

The reason of the core dump is errp already been set in
hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
The order of this check is correct, while we should  return when errp is set
in hotplug_handler_pre_plug().

I got a fix like this, which I have tested and looks good to me.


diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6077d27361..b11f3b15c1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
     }
 
     hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
+    if (*errp) {
+        return;
+    }
 
     if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");

>
>Fixes: 9040e6dfa8c3fed87695a3de555d2c775727bb51
>Signed-off-by: Thomas Huth <address@hidden>
>---
> hw/i386/pc.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index 6077d27361..f2c15bf1f2 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -2078,6 +2078,7 @@ static void pc_memory_pre_plug(HotplugHandler 
>*hotplug_dev, DeviceState *dev,
>     const MachineState *ms = MACHINE(hotplug_dev);
>     const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>     const uint64_t legacy_align = TARGET_PAGE_SIZE;
>+    Error *local_err = NULL;
> 
>     /*
>      * When -no-acpi is used with Q35 machine type, no ACPI is built,
>@@ -2090,13 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler 
>*hotplug_dev, DeviceState *dev,
>         return;
>     }
> 
>-    hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>-
>     if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>         return;
>     }
> 
>+    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>+    if (local_err) {
>+        error_propagate(errp, local_err);
>+        return;
>+    }
>+
>     pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>                      pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
> }
>-- 
>2.21.0

-- 
Wei Yang
Help you, Help me