Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/04/19 11:23, Thomas Huth wrote:
> QEMU currently crashes when you try to hot-plug an "nvdimm" device
> on older machine types:
> 
> $ qemu-system-x86_64 -monitor stdio -M pc-1.1
> QEMU 3.1.92 monitor - type 'help' for more information
> (qemu) device_add nvdimm,id=nvdimmn1
> qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv:
>  Assertion `*errp == ((void *)0)' failed.
> Aborted (core dumped)
> 
> The call to hotplug_handler_pre_plug() in pc_memory_pre_plug() has been
> added recently before the check whether nvdimm is enabled. It should
> be done after the check. And while we're at it, also check the errp
> after the hotplug_handler_pre_plug(), otherwise errors are silently
> ignored here.
> 
> Fixes: 9040e6dfa8c3fed87695a3de555d2c775727bb51
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/i386/pc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6077d27361..f2c15bf1f2 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2078,6 +2078,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      const MachineState *ms = MACHINE(hotplug_dev);
>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>      const uint64_t legacy_align = TARGET_PAGE_SIZE;
> +    Error *local_err = NULL;
>  
>      /*
>       * When -no-acpi is used with Q35 machine type, no ACPI is built,
> @@ -2090,13 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
> -
>      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;
> +    }
> +
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>  }
> 

Queued, thanks.

Paolo