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

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
>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).
>

Ok, that's fine for me.

>Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
>'-M'" check to be done first?
>

Because this function pc_memory_pre_plug() will be called not only when
nvdimm is hot-plugged but also dimm is hot-plugged. And
hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug
capability.

So the check in pc_memory_pre_plug() is from generic to specific: 
    1. Do we have capability to hot-plug?
    2. If the device is nvdimm, do we enabled nvdimm?

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

-- 
Wei Yang
Help you, Help me