diff mbox

[v2] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'

Message ID 1503297029-28436-1-git-send-email-thuth@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Huth Aug. 21, 2017, 6:30 a.m. UTC
QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
machine without specifying its 'memdev' property. This happens because
pc_dimm_get_memory_region() does not check whether the 'memdev' property
has properly been set by the user. Looking closer at this function, it's
also obvious that it is using &error_abort to call another function - and
this is bad in a function that is used in the hot-plugging calling chain
since this can also cause QEMU to exit unexpectedly.

So let's fix these issues in a proper way now: Add a "Error **errp"
parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
property has not been set by the user, and which we can use instead of
the &error_abort, and change the callers of get_memory_region() to make
use of this "errp" parameter for proper error checking.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/pc.c             | 14 ++++++++++++--
 hw/mem/nvdimm.c          |  2 +-
 hw/mem/pc-dimm.c         | 14 +++++++++++---
 hw/ppc/spapr.c           | 42 ++++++++++++++++++++++++++++++------------
 include/hw/mem/pc-dimm.h |  2 +-
 5 files changed, 55 insertions(+), 19 deletions(-)

Comments

Pankaj Gupta Aug. 21, 2017, 7:36 a.m. UTC | #1
Hello Thomas,

> 
> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> machine without specifying its 'memdev' property. This happens because
> pc_dimm_get_memory_region() does not check whether the 'memdev' property
> has properly been set by the user. Looking closer at this function, it's
> also obvious that it is using &error_abort to call another function - and
> this is bad in a function that is used in the hot-plugging calling chain
> since this can also cause QEMU to exit unexpectedly.

In place of checking if 'memdev' is already allocated/present while fetching 
'get_functions' every place. If 'memdev' is required for memory hotplug operation 
can we just put a check at some common place and don't start or instantiate if 
'memdev' is not present.

I can see:

'pc_dimm_realize' function checks for : in my case prints warning
and avoid Qemu to start.
...
...
if (!dimm->hostmem) {
        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
        return;
    } 
...

Thanks,
Pankaj

> 
> So let's fix these issues in a proper way now: Add a "Error **errp"
> parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> property has not been set by the user, and which we can use instead of
> the &error_abort, and change the callers of get_memory_region() to make
> use of this "errp" parameter for proper error checking.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/i386/pc.c             | 14 ++++++++++++--
>  hw/mem/nvdimm.c          |  2 +-
>  hw/mem/pc-dimm.c         | 14 +++++++++++---
>  hw/ppc/spapr.c           | 42 ++++++++++++++++++++++++++++++------------
>  include/hw/mem/pc-dimm.h |  2 +-
>  5 files changed, 55 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5943539..2108104 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr;
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
>      if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
> @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler
> *hotplug_dev,
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>  
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index db896b0..952fce5 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
>                          NULL, NULL);
>  }
>  
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error
> **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ea67b46..bdf6649 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v,
> const char *name,
>      PCDIMMDevice *dimm = PC_DIMM(obj);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>  
> -    mr = ddc->get_memory_region(dimm);
> +    mr = ddc->get_memory_region(dimm, errp);
> +    if (!mr) {
> +        return;
> +    }
>      value = memory_region_size(mr);
>  
>      visit_type_uint64(v, name, &value, errp);
> @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error
> **errp)
>      host_memory_backend_set_mapped(dimm->hostmem, false);
>  }
>  
> -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error
> **errp)
>  {
> -    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> +    if (!dimm->hostmem) {
> +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> +        return NULL;
> +    }
> +
> +    return host_memory_backend_get_memory(dimm->hostmem, errp);
>  }
>  
>  static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f7a1972..cec441c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t align = memory_region_get_alignment(mr);
> -    uint64_t size = memory_region_size(mr);
> -    uint64_t addr;
> +    MemoryRegion *mr;
> +    uint64_t align, size, addr;
> +
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    align = memory_region_get_alignment(mr);
> +    size = memory_region_size(mr);
>  
>      pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
>      if (local_err) {
> @@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t size = memory_region_size(mr);
> +    MemoryRegion *mr;
> +    uint64_t size;
>      char *mem_dev;
>  
> +    mr = ddc->get_memory_region(dimm, errp);
> +    if (!mr) {
> +        return;
> +    }
> +    size = memory_region_size(mr);
> +
>      if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>          error_setg(errp, "Hotplugged memory size must be a multiple of "
>                        "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> @@ -2882,7 +2893,7 @@ static sPAPRDIMMState
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>  {
>      sPAPRDRConnector *drc;
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      uint64_t size = memory_region_size(mr);
>      uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>      uint32_t avail_lmbs = 0;
> @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr,
>      PC_DIMM(dev));
>  
>      /* This information will get lost if a migration occurs
> @@ -2945,12 +2956,19 @@ static void
> spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t size = memory_region_size(mr);
> -    uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> -    uint64_t addr_start, addr;
> +    MemoryRegion *mr;
> +    uint32_t nr_lmbs;
> +    uint64_t size, addr_start, addr;
>      int i;
>      sPAPRDRConnector *drc;
> +
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    size = memory_region_size(mr);
> +    nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> +
>      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
>                                           &local_err);
>      if (local_err) {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2..6f8c3eb 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass {
>  
>      /* public */
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
> -    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
> +    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  
> --
> 1.8.3.1
> 
> 
>
David Gibson Aug. 21, 2017, 7:55 a.m. UTC | #2
On Mon, Aug 21, 2017 at 08:30:29AM +0200, Thomas Huth wrote:
> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> machine without specifying its 'memdev' property. This happens because
> pc_dimm_get_memory_region() does not check whether the 'memdev' property
> has properly been set by the user. Looking closer at this function, it's
> also obvious that it is using &error_abort to call another function - and
> this is bad in a function that is used in the hot-plugging calling chain
> since this can also cause QEMU to exit unexpectedly.
> 
> So let's fix these issues in a proper way now: Add a "Error **errp"
> parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> property has not been set by the user, and which we can use instead of
> the &error_abort, and change the callers of get_memory_region() to make
> use of this "errp" parameter for proper error checking.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

ppc portions

Acked-by: David Gibson <david@gibson.dropbear.id.au>

and the rest

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

I'm happy to queue this or have someone else queue it.

> ---
>  hw/i386/pc.c             | 14 ++++++++++++--
>  hw/mem/nvdimm.c          |  2 +-
>  hw/mem/pc-dimm.c         | 14 +++++++++++---
>  hw/ppc/spapr.c           | 42 ++++++++++++++++++++++++++++++------------
>  include/hw/mem/pc-dimm.h |  2 +-
>  5 files changed, 55 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5943539..2108104 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr;
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
>      if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
> @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>  
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index db896b0..952fce5 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
>                          NULL, NULL);
>  }
>  
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ea67b46..bdf6649 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>      PCDIMMDevice *dimm = PC_DIMM(obj);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>  
> -    mr = ddc->get_memory_region(dimm);
> +    mr = ddc->get_memory_region(dimm, errp);
> +    if (!mr) {
> +        return;
> +    }
>      value = memory_region_size(mr);
>  
>      visit_type_uint64(v, name, &value, errp);
> @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
>      host_memory_backend_set_mapped(dimm->hostmem, false);
>  }
>  
> -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  {
> -    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> +    if (!dimm->hostmem) {
> +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> +        return NULL;
> +    }
> +
> +    return host_memory_backend_get_memory(dimm->hostmem, errp);
>  }
>  
>  static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f7a1972..cec441c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t align = memory_region_get_alignment(mr);
> -    uint64_t size = memory_region_size(mr);
> -    uint64_t addr;
> +    MemoryRegion *mr;
> +    uint64_t align, size, addr;
> +
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    align = memory_region_get_alignment(mr);
> +    size = memory_region_size(mr);
>  
>      pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
>      if (local_err) {
> @@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t size = memory_region_size(mr);
> +    MemoryRegion *mr;
> +    uint64_t size;
>      char *mem_dev;
>  
> +    mr = ddc->get_memory_region(dimm, errp);
> +    if (!mr) {
> +        return;
> +    }
> +    size = memory_region_size(mr);
> +
>      if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>          error_setg(errp, "Hotplugged memory size must be a multiple of "
>                        "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> @@ -2882,7 +2893,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>  {
>      sPAPRDRConnector *drc;
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      uint64_t size = memory_region_size(mr);
>      uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>      uint32_t avail_lmbs = 0;
> @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
>  
>      /* This information will get lost if a migration occurs
> @@ -2945,12 +2956,19 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t size = memory_region_size(mr);
> -    uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> -    uint64_t addr_start, addr;
> +    MemoryRegion *mr;
> +    uint32_t nr_lmbs;
> +    uint64_t size, addr_start, addr;
>      int i;
>      sPAPRDRConnector *drc;
> +
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    size = memory_region_size(mr);
> +    nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> +
>      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
>                                           &local_err);
>      if (local_err) {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2..6f8c3eb 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass {
>  
>      /* public */
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
> -    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
> +    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>
Thomas Huth Aug. 21, 2017, 8 a.m. UTC | #3
Hi,

On 21.08.2017 09:36, Pankaj Gupta wrote:
> 
> Hello Thomas,
>>
>> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
>> machine without specifying its 'memdev' property. This happens because
>> pc_dimm_get_memory_region() does not check whether the 'memdev' property
>> has properly been set by the user. Looking closer at this function, it's
>> also obvious that it is using &error_abort to call another function - and
>> this is bad in a function that is used in the hot-plugging calling chain
>> since this can also cause QEMU to exit unexpectedly.
> 
> In place of checking if 'memdev' is already allocated/present while fetching 
> 'get_functions' every place. If 'memdev' is required for memory hotplug operation 
> can we just put a check at some common place and don't start or instantiate if 
> 'memdev' is not present.
> 
> I can see:
> 
> 'pc_dimm_realize' function checks for : in my case prints warning
> and avoid Qemu to start.
> ...
> ...
> if (!dimm->hostmem) {
>         error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
>         return;
>     } 
> ...

That check unfortunately does not apply here: The crash happens in the
pre_plug() handler of the spapr machine, and this is called *before* the
realize function of the pc-dimm is called!

But even if we could find another common place where we could check
dimm->hostmem first, you still have the problem that
host_memory_backend_get_memory() could return an error and thus
pc_dimm_get_memory_region() still needs a way to fail gracefully during
the hotplug (or also hot-unplug) operation. So I think my patch is the
right way to go here.

 Thomas
Igor Mammedov Aug. 21, 2017, 8:45 a.m. UTC | #4
On Mon, 21 Aug 2017 17:55:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Aug 21, 2017 at 08:30:29AM +0200, Thomas Huth wrote:
> > QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> > machine without specifying its 'memdev' property. This happens because
> > pc_dimm_get_memory_region() does not check whether the 'memdev' property
> > has properly been set by the user. Looking closer at this function, it's
> > also obvious that it is using &error_abort to call another function - and
> > this is bad in a function that is used in the hot-plugging calling chain
> > since this can also cause QEMU to exit unexpectedly.
> > 
> > So let's fix these issues in a proper way now: Add a "Error **errp"
> > parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> > property has not been set by the user, and which we can use instead of
> > the &error_abort, and change the callers of get_memory_region() to make
> > use of this "errp" parameter for proper error checking.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>  
> 
> ppc portions
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> 
> and the rest
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

 
> I'm happy to queue this or have someone else queue it.
Pls, go ahead and queue it via your tree

> 
> > ---
> >  hw/i386/pc.c             | 14 ++++++++++++--
> >  hw/mem/nvdimm.c          |  2 +-
> >  hw/mem/pc-dimm.c         | 14 +++++++++++---
> >  hw/ppc/spapr.c           | 42 ++++++++++++++++++++++++++++++------------
> >  include/hw/mem/pc-dimm.h |  2 +-
> >  5 files changed, 55 insertions(+), 19 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 5943539..2108104 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > +    MemoryRegion *mr;
> >      uint64_t align = TARGET_PAGE_SIZE;
> >      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >  
> > +    mr = ddc->get_memory_region(dimm, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> >      if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> >          align = memory_region_get_alignment(mr);
> >      }
> > @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > +    MemoryRegion *mr;
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >  
> > +    mr = ddc->get_memory_region(dimm, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> >      hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> >  
> > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> > index db896b0..952fce5 100644
> > --- a/hw/mem/nvdimm.c
> > +++ b/hw/mem/nvdimm.c
> > @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
> >                          NULL, NULL);
> >  }
> >  
> > -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
> > +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> >  {
> >      NVDIMMDevice *nvdimm = NVDIMM(dimm);
> >  
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index ea67b46..bdf6649 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
> >      PCDIMMDevice *dimm = PC_DIMM(obj);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
> >  
> > -    mr = ddc->get_memory_region(dimm);
> > +    mr = ddc->get_memory_region(dimm, errp);
> > +    if (!mr) {
> > +        return;
> > +    }
> >      value = memory_region_size(mr);
> >  
> >      visit_type_uint64(v, name, &value, errp);
> > @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
> >      host_memory_backend_set_mapped(dimm->hostmem, false);
> >  }
> >  
> > -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> > +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> >  {
> > -    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> > +    if (!dimm->hostmem) {
> > +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> > +        return NULL;
> > +    }
> > +
> > +    return host_memory_backend_get_memory(dimm->hostmem, errp);
> >  }
> >  
> >  static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f7a1972..cec441c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > -    uint64_t align = memory_region_get_alignment(mr);
> > -    uint64_t size = memory_region_size(mr);
> > -    uint64_t addr;
> > +    MemoryRegion *mr;
> > +    uint64_t align, size, addr;
> > +
> > +    mr = ddc->get_memory_region(dimm, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    align = memory_region_get_alignment(mr);
> > +    size = memory_region_size(mr);
> >  
> >      pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
> >      if (local_err) {
> > @@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >  {
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > -    uint64_t size = memory_region_size(mr);
> > +    MemoryRegion *mr;
> > +    uint64_t size;
> >      char *mem_dev;
> >  
> > +    mr = ddc->get_memory_region(dimm, errp);
> > +    if (!mr) {
> > +        return;
> > +    }
> > +    size = memory_region_size(mr);
> > +
> >      if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> >          error_setg(errp, "Hotplugged memory size must be a multiple of "
> >                        "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> > @@ -2882,7 +2893,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> >  {
> >      sPAPRDRConnector *drc;
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> >      uint64_t size = memory_region_size(mr);
> >      uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> >      uint32_t avail_lmbs = 0;
> > @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev)
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> >      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
> >  
> >      /* This information will get lost if a migration occurs
> > @@ -2945,12 +2956,19 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> >      Error *local_err = NULL;
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > -    uint64_t size = memory_region_size(mr);
> > -    uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> > -    uint64_t addr_start, addr;
> > +    MemoryRegion *mr;
> > +    uint32_t nr_lmbs;
> > +    uint64_t size, addr_start, addr;
> >      int i;
> >      sPAPRDRConnector *drc;
> > +
> > +    mr = ddc->get_memory_region(dimm, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    size = memory_region_size(mr);
> > +    nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> > +
> >      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> >                                           &local_err);
> >      if (local_err) {
> > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > index 1e483f2..6f8c3eb 100644
> > --- a/include/hw/mem/pc-dimm.h
> > +++ b/include/hw/mem/pc-dimm.h
> > @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass {
> >  
> >      /* public */
> >      void (*realize)(PCDIMMDevice *dimm, Error **errp);
> > -    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
> > +    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> >      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> >  } PCDIMMDeviceClass;
> >    
>
David Gibson Aug. 21, 2017, 8:47 a.m. UTC | #5
On Mon, Aug 21, 2017 at 10:45:02AM +0200, Igor Mammedov wrote:
> On Mon, 21 Aug 2017 17:55:20 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Aug 21, 2017 at 08:30:29AM +0200, Thomas Huth wrote:
> > > QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> > > machine without specifying its 'memdev' property. This happens because
> > > pc_dimm_get_memory_region() does not check whether the 'memdev' property
> > > has properly been set by the user. Looking closer at this function, it's
> > > also obvious that it is using &error_abort to call another function - and
> > > this is bad in a function that is used in the hot-plugging calling chain
> > > since this can also cause QEMU to exit unexpectedly.
> > > 
> > > So let's fix these issues in a proper way now: Add a "Error **errp"
> > > parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> > > property has not been set by the user, and which we can use instead of
> > > the &error_abort, and change the callers of get_memory_region() to make
> > > use of this "errp" parameter for proper error checking.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>  
> > 
> > ppc portions
> > 
> > Acked-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > and the rest
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
>  
> > I'm happy to queue this or have someone else queue it.
> Pls, go ahead and queue it via your tree

Done.
Pankaj Gupta Aug. 21, 2017, 9:34 a.m. UTC | #6
> > 
> > Hello Thomas,
> >>
> >> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> >> machine without specifying its 'memdev' property. This happens because
> >> pc_dimm_get_memory_region() does not check whether the 'memdev' property
> >> has properly been set by the user. Looking closer at this function, it's
> >> also obvious that it is using &error_abort to call another function - and
> >> this is bad in a function that is used in the hot-plugging calling chain
> >> since this can also cause QEMU to exit unexpectedly.
> > 
> > In place of checking if 'memdev' is already allocated/present while
> > fetching
> > 'get_functions' every place. If 'memdev' is required for memory hotplug
> > operation
> > can we just put a check at some common place and don't start or instantiate
> > if
> > 'memdev' is not present.
> > 
> > I can see:
> > 
> > 'pc_dimm_realize' function checks for : in my case prints warning
> > and avoid Qemu to start.
> > ...
> > ...
> > if (!dimm->hostmem) {
> >         error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
> >         return;
> >     }
> > ...
> 
> That check unfortunately does not apply here: The crash happens in the
> pre_plug() handler of the spapr machine, and this is called *before* the
> realize function of the pc-dimm is called!

o.k I guessed so.
> 
> But even if we could find another common place where we could check
> dimm->hostmem first, you still have the problem that
> host_memory_backend_get_memory() could return an error and thus
> pc_dimm_get_memory_region() still needs a way to fail gracefully during
> the hotplug (or also hot-unplug) operation. So I think my patch is the
> right way to go here.

Right. Thanks for the explanation.

Pankaj
> 
>  Thomas
>
diff mbox

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5943539..2108104 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1691,10 +1691,15 @@  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
+    MemoryRegion *mr;
     uint64_t align = TARGET_PAGE_SIZE;
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
+    mr = ddc->get_memory_region(dimm, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
     if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
         align = memory_region_get_alignment(mr);
     }
@@ -1758,10 +1763,15 @@  static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
+    MemoryRegion *mr;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
 
+    mr = ddc->get_memory_region(dimm, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index db896b0..952fce5 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -71,7 +71,7 @@  static void nvdimm_init(Object *obj)
                         NULL, NULL);
 }
 
-static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
 {
     NVDIMMDevice *nvdimm = NVDIMM(dimm);
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index ea67b46..bdf6649 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -363,7 +363,10 @@  static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
     PCDIMMDevice *dimm = PC_DIMM(obj);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
 
-    mr = ddc->get_memory_region(dimm);
+    mr = ddc->get_memory_region(dimm, errp);
+    if (!mr) {
+        return;
+    }
     value = memory_region_size(mr);
 
     visit_type_uint64(v, name, &value, errp);
@@ -411,9 +414,14 @@  static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
     host_memory_backend_set_mapped(dimm->hostmem, false);
 }
 
-static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
+static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
 {
-    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+    if (!dimm->hostmem) {
+        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
+        return NULL;
+    }
+
+    return host_memory_backend_get_memory(dimm->hostmem, errp);
 }
 
 static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f7a1972..cec441c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2772,10 +2772,15 @@  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
-    uint64_t align = memory_region_get_alignment(mr);
-    uint64_t size = memory_region_size(mr);
-    uint64_t addr;
+    MemoryRegion *mr;
+    uint64_t align, size, addr;
+
+    mr = ddc->get_memory_region(dimm, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    align = memory_region_get_alignment(mr);
+    size = memory_region_size(mr);
 
     pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
     if (local_err) {
@@ -2808,10 +2813,16 @@  static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
-    uint64_t size = memory_region_size(mr);
+    MemoryRegion *mr;
+    uint64_t size;
     char *mem_dev;
 
+    mr = ddc->get_memory_region(dimm, errp);
+    if (!mr) {
+        return;
+    }
+    size = memory_region_size(mr);
+
     if (size % SPAPR_MEMORY_BLOCK_SIZE) {
         error_setg(errp, "Hotplugged memory size must be a multiple of "
                       "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
@@ -2882,7 +2893,7 @@  static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 {
     sPAPRDRConnector *drc;
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     uint64_t size = memory_region_size(mr);
     uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
     uint32_t avail_lmbs = 0;
@@ -2912,7 +2923,7 @@  void spapr_lmb_release(DeviceState *dev)
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs
@@ -2945,12 +2956,19 @@  static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm);
-    uint64_t size = memory_region_size(mr);
-    uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
-    uint64_t addr_start, addr;
+    MemoryRegion *mr;
+    uint32_t nr_lmbs;
+    uint64_t size, addr_start, addr;
     int i;
     sPAPRDRConnector *drc;
+
+    mr = ddc->get_memory_region(dimm, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    size = memory_region_size(mr);
+    nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
+
     addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
                                          &local_err);
     if (local_err) {
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 1e483f2..6f8c3eb 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -71,7 +71,7 @@  typedef struct PCDIMMDeviceClass {
 
     /* public */
     void (*realize)(PCDIMMDevice *dimm, Error **errp);
-    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
+    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;