On 08/04/2019 15.45, Wei Yang wrote: > 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; > + }
Not sure, but I think you can not rely on the fact that the caller set *errp = NULL already... that's why it is more common to use a local_err variable and error_propagate() for such cases (which is what I did in my patch). Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in '-M'" check to be done first? Thomas > 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 <th...@redhat.com> >> --- >> 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 >