Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

On Tue, May 28, 2019 at 02:26:27PM +0200, Igor Mammedov wrote:
>On Tue, 28 May 2019 09:35:48 +0800
>Wei Yang <address@hidden> wrote:
>
>> On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:
>> >On Thu, 11 Apr 2019 15:17:39 +0800
>> >Wei Yang <address@hidden> wrote:
>> >  
>> >> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> >> nvdimm. This is more proper to check apci hotplug capability before
>> >> check nvdimm specific capability.  
>> >not sure what this about.
>> >Currently we are checking if ACPI is enabled
>> >  if (!pcms->acpi_dev || !acpi_enabled) { ...
>> >before nvdimm check and it looks better to me that we cancel
>> >nvdimm hotplug earlier than passing it to
>> >    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
>> >with this patch ACPI device handler will be called before
>> >nvdimm check happens, so it's +1 unnecessary call chain in
>> >the case of nvdimm, which I'd rather not have.
>> >
>> >Are there any issues with current call flow?
>> >(commit message doesn't really explaining why we need this patch)
>> >  
>> 
>> My idea is to check more generic requirement and then specific one.
>> 
>> For example, the call flow looks like this:
>> 
>> pc_memory_pre_plug
>> 
>>     piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
>>         if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>>             !lpc->pm.acpi_memory_hotplug.is_enabled)
>> 
>>     if (is_nvdimm && !ms->nvdimms_state->is_enabled)
>>     
>> 
>> In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And 
>> then
>> if it has memory hotplug capability and is nvdimm, we check whether nvdimm is
>> enabled.
>
>I don't think pc_memory_pre_plug() should rely on what 
>hotplug_handler_pre_plug()
>checks or does. Similarly the later is taking care of whatever piix4 needs to 
>care
>and shouldn't care about what machine code does.
>

Agree. It is not proper to let hotplug_handler_pre_plug() take care about
machine code.

>Moreover when hotplug_handler_pre_plug() starts to reserve resources, then
>if you move check as suggested you'd need to rollback all that
>hotplug_handler_pre_plug() done to gracefully abort hotplug.
>

Confused.

hotplug_handler_pre_plug() doesn't reserve resources.
pc_dimm_pre_plug() does.

I didn't plan to move the code after pc_dimm_pre_plug().

>So I'd leave the code as it is now, since it doesn't depend on concrete
>hotplug_handler_pre_plug() implementation and won't break if
>hotplug_handler_pre_plug() will start consuming resources (which could
>happen and you won't even notice it since changed code is in piix4/q35
>files when reviewing patches).
>> This is why I suggest to change the order here. No functional issue for
>> current code.
>> 

-- 
Wei Yang
Help you, Help me