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

[Igor Mammedov]

On Wed, 29 May 2019 08:32:14 +0800
Wei Yang <address@hidden> 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 <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.


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. 

> 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.
> >>   
>