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

[Thomas Huth]

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 <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
>