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

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

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.

This is why I suggest to change the order here. No functional issue for
current code.

-- 
Wei Yang
Help you, Help me