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

[Thomas Huth]

 Hi,

On 11/04/2019 03.56, Wei Yang wrote:
> 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?

Fine for me, I don't mind either way as long as the crash is fixed. Feel
free to send a patch that restores the desired order.

 Thomas