diff mbox

[1/4] spapr: add pre_plug function for memory

Message ID 20170523111812.13469-2-lvivier@redhat.com
State New
Headers show

Commit Message

Laurent Vivier May 23, 2017, 11:18 a.m. UTC 
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(-)

Comments

Greg Kurz May 23, 2017, 3:28 p.m. UTC | #1 
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);
>      }
>  }
Laurent Vivier May 23, 2017, 4:09 p.m. UTC | #2 
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
David Gibson May 24, 2017, 4:52 a.m. UTC | #3 
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.
Greg Kurz May 24, 2017, 9:55 a.m. UTC | #4 
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.");
+    }
+}
+
David Gibson May 24, 2017, 10:27 a.m. UTC | #5 
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 mbox

Patch

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);
     }
 }