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)
+{
+ 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);
}
}
--
2.9.4
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.
--
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 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.
--
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 - 2026 Red Hat, Inc.