| Message ID | 1503297029-28436-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > >
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; >
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
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; > > >
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.
> > > > 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 --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;
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(-)