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

[Wei Yang <richardw.yang@linux.intel.com>]

On Wed, May 29, 2019 at 10:57:50AM +0200, Igor Mammedov wrote:
>On Wed, 29 May 2019 08:32:14 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Tue, May 28, 2019 at 02:26:27PM +0200, Igor Mammedov wrote:
>> >On Tue, 28 May 2019 09:35:48 +0800
>> >Wei Yang <richardw.yang@linux.intel.com> 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 <richardw.yang@linux.intel.com> 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.
>
>
>it's not currently, but if it would it would not work with your patch properly
>or break unexpectedly since whoever would change hotplug_handler_pre_plug()
>might not notice that machine code need to be taken care of.
>
>Try to consider devices and machine as separate libraries. Which should
>in reasonable limits be independent and work through documented interfaces.
>In that case likehood of breaking something would be less than relying on
>current code impl./call order with implicit inter-dependencies. 
>

So the logic here is check machine then device, right? I think this is
reasonable. To be honest, this rule is not that obvious.

Anyway, I think what you mentioned make sense. Thanks

-- 
Wei Yang
Help you, Help me