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

[Wei Yang]

On Mon, Apr 08, 2019 at 09:29:11PM +0000, Wei Yang wrote:
>>> 
>>> 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

Do you think this is a reasonable explanation?

-- 
Wei Yang
Help you, Help me