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

[Igor Mammedov]

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.

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.

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