hw/ppc/spapr.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
machine without specifying its 'memdev' property. Let's add a sanity
check to the pre_plug handler to fix this issue.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/ppc/spapr.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f7a1972..22d400a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2808,10 +2808,17 @@ 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;
+ if (!dimm->hostmem) {
+ error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
+ return;
+ }
+
+ mr = ddc->get_memory_region(dimm);
+ 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);
--
1.8.3.1
On Thu, Aug 17, 2017 at 08:33:10PM +0200, Thomas Huth wrote: > QEMU currently crashes when trying to use a 'pc-dimm' on the pseries > machine without specifying its 'memdev' property. Let's add a sanity > check to the pre_plug handler to fix this issue. > > Signed-off-by: Thomas Huth <thuth@redhat.com> Thanks for all these patches fixing little bugs in 2.10. > --- > hw/ppc/spapr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index f7a1972..22d400a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2808,10 +2808,17 @@ 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; > > + if (!dimm->hostmem) { Isn't checking dimm->hostmem directly here an abstraction violation? Could we just check for a NULL return from get_memory_region instead? > + error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set"); > + return; > + } > + > + mr = ddc->get_memory_region(dimm); > + 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); -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 18.08.2017 03:25, David Gibson wrote: > On Thu, Aug 17, 2017 at 08:33:10PM +0200, Thomas Huth wrote: >> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries >> machine without specifying its 'memdev' property. Let's add a sanity >> check to the pre_plug handler to fix this issue. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > Thanks for all these patches fixing little bugs in 2.10. ... or 2.11 ;-) ... not sure if there will be another RC next week or the final 2.10 release? Anyway, the fixes are required for a new qtest that I'm working on (calling device_add + device_del for all available devices), that's why I'm coming up with all these patches now. There is another crash with one of the ppc64 devices, where I don't know how to fix it yet - so if somebody got a clue, help is appreciated: $ qemu-system-ppc64 -nographic -S -nodefaults -monitor stdio -M pseries QEMU 2.9.92 monitor - type 'help' for more information (qemu) device_add macio-oldworld,id=x (qemu) device_del x (qemu) ** ERROR:qemu/qom/object.c:1611:object_get_canonical_path_component: assertion failed: (obj->parent != NULL) Aborted (core dumped) >> --- >> hw/ppc/spapr.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index f7a1972..22d400a 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -2808,10 +2808,17 @@ 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; >> >> + if (!dimm->hostmem) { > > Isn't checking dimm->hostmem directly here an abstraction violation? > Could we just check for a NULL return from get_memory_region instead? The crash happens within get_memory_region: pc_dimm_get_memory_region() calls host_memory_backend_get_memory(), which calls host_memory_backend_mr_inited() - and that function dereferences the NULL pointer. I could add an additional check to one of the called functions and return NULL in case the pointer is already NULL ... do you prefer that? Let me know, then I'll send a v2... > >> + error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set"); >> + return; >> + } >> + >> + mr = ddc->get_memory_region(dimm); >> + 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); > Thomas
On Fri, Aug 18, 2017 at 09:23:53AM +0200, Thomas Huth wrote: > On 18.08.2017 03:25, David Gibson wrote: > > On Thu, Aug 17, 2017 at 08:33:10PM +0200, Thomas Huth wrote: > >> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries > >> machine without specifying its 'memdev' property. Let's add a sanity > >> check to the pre_plug handler to fix this issue. > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > Thanks for all these patches fixing little bugs in 2.10. > > ... or 2.11 ;-) ... not sure if there will be another RC next week or > the final 2.10 release? > > Anyway, the fixes are required for a new qtest that I'm working on > (calling device_add + device_del for all available devices), that's why > I'm coming up with all these patches now. There is another crash with > one of the ppc64 devices, where I don't know how to fix it yet - so if > somebody got a clue, help is appreciated: > > $ qemu-system-ppc64 -nographic -S -nodefaults -monitor stdio -M pseries > QEMU 2.9.92 monitor - type 'help' for more information > (qemu) device_add macio-oldworld,id=x > (qemu) device_del x > (qemu) ** > ERROR:qemu/qom/object.c:1611:object_get_canonical_path_component: > assertion failed: (obj->parent != NULL) > Aborted (core dumped) > > >> --- > >> hw/ppc/spapr.c | 11 +++++++++-- > >> 1 file changed, 9 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index f7a1972..22d400a 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -2808,10 +2808,17 @@ 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; > >> > >> + if (!dimm->hostmem) { > > > > Isn't checking dimm->hostmem directly here an abstraction violation? > > Could we just check for a NULL return from get_memory_region instead? > > The crash happens within get_memory_region: pc_dimm_get_memory_region() > calls host_memory_backend_get_memory(), which calls > host_memory_backend_mr_inited() - and that function dereferences the > NULL pointer. > > I could add an additional check to one of the called functions and > return NULL in case the pointer is already NULL ... do you prefer that? > Let me know, then I'll send a v2... Ah, right. Yeah, I think this is essentially a bug in get_memory_region() or one of its called functions. They're unsafe to call in circumstances that the caller can't really control of determine (without breaking the abstraction wall). -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
© 2016 - 2024 Red Hat, Inc.