Message ID | 20170523111812.13469-2-lvivier@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, 23 May 2017 13:18:09 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > This allows to manage errors before the memory > has started to be hotplugged. We already have > the function for the CPU cores. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/ppc/spapr.c | 45 ++++++++++++++++++++++++++++++--------------- > 1 file changed, 30 insertions(+), 15 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0980d73..0e8d8d1 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2569,20 +2569,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > uint64_t align = memory_region_get_alignment(mr); > uint64_t size = memory_region_size(mr); > uint64_t addr; > - char *mem_dev; > - > - if (size % SPAPR_MEMORY_BLOCK_SIZE) { > - error_setg(&local_err, "Hotplugged memory size must be a multiple of " > - "%lld MB", SPAPR_MEMORY_BLOCK_SIZE/M_BYTE); > - goto out; > - } > - > - mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); > - if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { > - error_setg(&local_err, "Memory backend has bad page size. " > - "Use 'memory-backend-file' with correct mem-path."); > - goto out; > - } > > pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err); > if (local_err) { > @@ -2603,6 +2589,33 @@ out: > error_propagate(errp, local_err); > } > > +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp) Indentation nit > +{ > + 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); > + Error *local_err = NULL; > + char *mem_dev; > + > + if (size % SPAPR_MEMORY_BLOCK_SIZE) { > + error_setg(&local_err, "Hotplugged memory size must be a multiple of " > + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); > + goto out; > + } > + > + mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); > + if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { > + error_setg(&local_err, "Memory backend has bad page size. " > + "Use 'memory-backend-file' with correct mem-path."); > + goto out; > + } > + > +out: > + error_propagate(errp, local_err); As recently discussed with Markus Armbruster, it isn't necessary to have a local Error * if you don't do anything else with it but propagate it. Message-ID: <20170522090055.GN30246@umbus.fritz.box> The patch looks good anyway. Reviewed-by: Greg Kurz <groug@kaod.org> > +} > + > typedef struct sPAPRDIMMState { > uint32_t nr_lmbs; > } sPAPRDIMMState; > @@ -2990,7 +3003,9 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, > static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > - if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > + spapr_memory_pre_plug(hotplug_dev, dev, errp); > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > spapr_core_pre_plug(hotplug_dev, dev, errp); > } > }
On 23/05/2017 17:28, Greg Kurz wrote: > On Tue, 23 May 2017 13:18:09 +0200 > Laurent Vivier <lvivier@redhat.com> wrote: > >> This allows to manage errors before the memory >> has started to be hotplugged. We already have >> the function for the CPU cores. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> hw/ppc/spapr.c | 45 ++++++++++++++++++++++++++++++--------------- >> 1 file changed, 30 insertions(+), 15 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 0980d73..0e8d8d1 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -2569,20 +2569,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >> uint64_t align = memory_region_get_alignment(mr); >> uint64_t size = memory_region_size(mr); >> uint64_t addr; >> - char *mem_dev; >> - >> - if (size % SPAPR_MEMORY_BLOCK_SIZE) { >> - error_setg(&local_err, "Hotplugged memory size must be a multiple of " >> - "%lld MB", SPAPR_MEMORY_BLOCK_SIZE/M_BYTE); >> - goto out; >> - } >> - >> - mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); >> - if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { >> - error_setg(&local_err, "Memory backend has bad page size. " >> - "Use 'memory-backend-file' with correct mem-path."); >> - goto out; >> - } >> >> pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err); >> if (local_err) { >> @@ -2603,6 +2589,33 @@ out: >> error_propagate(errp, local_err); >> } >> >> +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >> + Error **errp) > > Indentation nit ok > >> +{ >> + 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); >> + Error *local_err = NULL; >> + char *mem_dev; >> + >> + if (size % SPAPR_MEMORY_BLOCK_SIZE) { >> + error_setg(&local_err, "Hotplugged memory size must be a multiple of " >> + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); >> + goto out; >> + } >> + >> + mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); >> + if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { >> + error_setg(&local_err, "Memory backend has bad page size. " >> + "Use 'memory-backend-file' with correct mem-path."); >> + goto out; >> + } >> + >> +out: >> + error_propagate(errp, local_err); > > As recently discussed with Markus Armbruster, it isn't necessary to have a > local Error * if you don't do anything else with it but propagate it. Yes, you are right, it's a stupid cut'n'paste. Thanks, Laurent
On Tue, May 23, 2017 at 06:09:00PM +0200, Laurent Vivier wrote: > On 23/05/2017 17:28, Greg Kurz wrote: > > On Tue, 23 May 2017 13:18:09 +0200 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> This allows to manage errors before the memory > >> has started to be hotplugged. We already have > >> the function for the CPU cores. > >> > >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> > >> --- > >> hw/ppc/spapr.c | 45 ++++++++++++++++++++++++++++++--------------- > >> 1 file changed, 30 insertions(+), 15 deletions(-) > >> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index 0980d73..0e8d8d1 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -2569,20 +2569,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > >> uint64_t align = memory_region_get_alignment(mr); > >> uint64_t size = memory_region_size(mr); > >> uint64_t addr; > >> - char *mem_dev; > >> - > >> - if (size % SPAPR_MEMORY_BLOCK_SIZE) { > >> - error_setg(&local_err, "Hotplugged memory size must be a multiple of " > >> - "%lld MB", SPAPR_MEMORY_BLOCK_SIZE/M_BYTE); > >> - goto out; > >> - } > >> - > >> - mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); > >> - if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { > >> - error_setg(&local_err, "Memory backend has bad page size. " > >> - "Use 'memory-backend-file' with correct mem-path."); > >> - goto out; > >> - } > >> > >> pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err); > >> if (local_err) { > >> @@ -2603,6 +2589,33 @@ out: > >> error_propagate(errp, local_err); > >> } > >> > >> +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > >> + Error **errp) > > > > Indentation nit > > ok > > > > >> +{ > >> + 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); > >> + Error *local_err = NULL; > >> + char *mem_dev; > >> + > >> + if (size % SPAPR_MEMORY_BLOCK_SIZE) { > >> + error_setg(&local_err, "Hotplugged memory size must be a multiple of " > >> + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); > >> + goto out; > >> + } > >> + > >> + mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); > >> + if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { > >> + error_setg(&local_err, "Memory backend has bad page size. " > >> + "Use 'memory-backend-file' with correct mem-path."); > >> + goto out; > >> + } > >> + > >> +out: > >> + error_propagate(errp, local_err); > > > > As recently discussed with Markus Armbruster, it isn't necessary to have a > > local Error * if you don't do anything else with it but propagate it. > > Yes, you are right, it's a stupid cut'n'paste. This patch seems like a good idea regardless of the rest, so I've fixed the minor nits Greg pointed out and merged to ppc-for-2.10.
On Wed, 24 May 2017 14:52:36 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: [...] > > This patch seems like a good idea regardless of the rest, so I've > fixed the minor nits Greg pointed out and merged to ppc-for-2.10. > David, Commit d2e4c6a1437fab2fbb4553b598f25e282c475199 in your ppc-for-2.10 branch doesn't compile: +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, + Error **errp) +{ + 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); + char *mem_dev; + + if (size % SPAPR_MEMORY_BLOCK_SIZE) { + error_setg(&local_err, "Hotplugged memory size must be a multiple of " s/&local_err/errp/ + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); + goto out; s/goto out/return/ + } + + mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); + if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { + error_setg(errp, "Memory backend has bad page size. " + "Use 'memory-backend-file' with correct mem-path."); + } +} +
On Wed, May 24, 2017 at 11:55:13AM +0200, Greg Kurz wrote: 1;4601;0c> On Wed, 24 May 2017 14:52:36 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > [...] > > > > This patch seems like a good idea regardless of the rest, so I've > > fixed the minor nits Greg pointed out and merged to ppc-for-2.10. > > > > David, > > Commit d2e4c6a1437fab2fbb4553b598f25e282c475199 in your ppc-for-2.10 branch > doesn't compile: > > +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp) > +{ > + 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); > + char *mem_dev; > + > + if (size % SPAPR_MEMORY_BLOCK_SIZE) { > + error_setg(&local_err, "Hotplugged memory size must be a multiple of " > > s/&local_err/errp/ > > + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); > + goto out; > > s/goto out/return/ > > + } > + > + mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); > + if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { > + error_setg(errp, "Memory backend has bad page size. " > + "Use 'memory-backend-file' with correct mem-path."); > + } > +} > + Sorry, I found and fixed that already, but forgot to push the update.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0980d73..0e8d8d1 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2569,20 +2569,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, uint64_t align = memory_region_get_alignment(mr); uint64_t size = memory_region_size(mr); uint64_t addr; - char *mem_dev; - - if (size % SPAPR_MEMORY_BLOCK_SIZE) { - error_setg(&local_err, "Hotplugged memory size must be a multiple of " - "%lld MB", SPAPR_MEMORY_BLOCK_SIZE/M_BYTE); - goto out; - } - - mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); - if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { - error_setg(&local_err, "Memory backend has bad page size. " - "Use 'memory-backend-file' with correct mem-path."); - goto out; - } pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err); if (local_err) { @@ -2603,6 +2589,33 @@ out: error_propagate(errp, local_err); } +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, + Error **errp) +{ + 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); + Error *local_err = NULL; + char *mem_dev; + + if (size % SPAPR_MEMORY_BLOCK_SIZE) { + error_setg(&local_err, "Hotplugged memory size must be a multiple of " + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); + goto out; + } + + mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); + if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { + error_setg(&local_err, "Memory backend has bad page size. " + "Use 'memory-backend-file' with correct mem-path."); + goto out; + } + +out: + error_propagate(errp, local_err); +} + typedef struct sPAPRDIMMState { uint32_t nr_lmbs; } sPAPRDIMMState; @@ -2990,7 +3003,9 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { + spapr_memory_pre_plug(hotplug_dev, dev, errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { spapr_core_pre_plug(hotplug_dev, dev, errp); } }
This allows to manage errors before the memory has started to be hotplugged. We already have the function for the CPU cores. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- hw/ppc/spapr.c | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-)