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

[Igor Mammedov <imammedo@redhat.com>]

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)

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/i386/pc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf1f2..d48b6f9582 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> -        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> -        return;
> -    }
> -
>      hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> +    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> +        return;
> +    }
> +
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>  }