diff mbox series

[v1,3/8] spapr: move all DIMM checks into spapr_memory_plug

Message ID 20180607165218.9558-4-david@redhat.com
State New
Headers show
Series pc/spapr/s390x: machine hotplug handler cleanups | expand

Commit Message

David Hildenbrand June 7, 2018, 4:52 p.m. UTC 
Let's clean the hotplug handler up by moving everything into
spapr_memory_plug().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Comments

David Gibson June 8, 2018, 3:28 a.m. UTC | #1 
On Thu, Jun 07, 2018 at 06:52:13PM +0200, David Hildenbrand wrote:
> Let's clean the hotplug handler up by moving everything into
> spapr_memory_plug().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d038f3243e..a12be24ca9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>  }
>  
>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> -                              uint32_t node, Error **errp)
> +                              Error **errp)
>  {
>      Error *local_err = NULL;
>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr;
>      uint64_t align, size, addr;
> +    uint32_t node;
> +
> +    if (!smc->dr_lmb_enabled) {
> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
> +        goto out;
> +    }
> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>  
>      mr = ddc->get_memory_region(dimm, &local_err);
>      if (local_err) {
> @@ -3568,19 +3576,8 @@ out:
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> -    MachineState *ms = MACHINE(hotplug_dev);
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> -
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        int node;
> -
> -        if (!smc->dr_lmb_enabled) {
> -            error_setg(errp, "Memory hotplug not supported for this machine");
> -            return;
> -        }
> -        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
> -
> -        spapr_memory_plug(hotplug_dev, dev, node, errp);
> +        spapr_memory_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          spapr_core_plug(hotplug_dev, dev, errp);
>      }
Greg Kurz June 8, 2018, 8:05 a.m. UTC | #2 
On Thu,  7 Jun 2018 18:52:13 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's clean the hotplug handler up by moving everything into
> spapr_memory_plug().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/ppc/spapr.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d038f3243e..a12be24ca9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>  }
>  
>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> -                              uint32_t node, Error **errp)
> +                              Error **errp)
>  {
>      Error *local_err = NULL;
>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr;
>      uint64_t align, size, addr;
> +    uint32_t node;
> +
> +    if (!smc->dr_lmb_enabled) {
> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
> +        goto out;
> +    }

Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?

> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>  
>      mr = ddc->get_memory_region(dimm, &local_err);
>      if (local_err) {
> @@ -3568,19 +3576,8 @@ out:
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> -    MachineState *ms = MACHINE(hotplug_dev);
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> -
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        int node;
> -
> -        if (!smc->dr_lmb_enabled) {
> -            error_setg(errp, "Memory hotplug not supported for this machine");
> -            return;
> -        }
> -        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
> -
> -        spapr_memory_plug(hotplug_dev, dev, node, errp);
> +        spapr_memory_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          spapr_core_plug(hotplug_dev, dev, errp);
>      }
David Hildenbrand June 8, 2018, 8:07 a.m. UTC | #3 
On 08.06.2018 10:05, Greg Kurz wrote:
> On Thu,  7 Jun 2018 18:52:13 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's clean the hotplug handler up by moving everything into
>> spapr_memory_plug().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 23 ++++++++++-------------
>>  1 file changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index d038f3243e..a12be24ca9 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>>  }
>>  
>>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> -                              uint32_t node, Error **errp)
>> +                              Error **errp)
>>  {
>>      Error *local_err = NULL;
>>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>>      PCDIMMDevice *dimm = PC_DIMM(dev);
>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>>      MemoryRegion *mr;
>>      uint64_t align, size, addr;
>> +    uint32_t node;
>> +
>> +    if (!smc->dr_lmb_enabled) {
>> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
>> +        goto out;
>> +    }
> 
> Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?

Think you're right (and as spapr_memory_pre_plug() already exists, it's
easy), other opinions? Thanks.

> 
>> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>>  
>>      mr = ddc->get_memory_region(dimm, &local_err);
>>      if (local_err) {
>> @@ -3568,19 +3576,8 @@ out:
>>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>>                                        DeviceState *dev, Error **errp)
>>  {
>> -    MachineState *ms = MACHINE(hotplug_dev);
>> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>> -
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        int node;
>> -
>> -        if (!smc->dr_lmb_enabled) {
>> -            error_setg(errp, "Memory hotplug not supported for this machine");
>> -            return;
>> -        }
>> -        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>> -
>> -        spapr_memory_plug(hotplug_dev, dev, node, errp);
>> +        spapr_memory_plug(hotplug_dev, dev, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>>          spapr_core_plug(hotplug_dev, dev, errp);
>>      }
>
Igor Mammedov June 8, 2018, 8:41 a.m. UTC | #4 
On Fri, 8 Jun 2018 10:07:59 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 10:05, Greg Kurz wrote:
> > On Thu,  7 Jun 2018 18:52:13 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Let's clean the hotplug handler up by moving everything into
> >> spapr_memory_plug().
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/ppc/spapr.c | 23 ++++++++++-------------
> >>  1 file changed, 10 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index d038f3243e..a12be24ca9 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> >>  }
> >>  
> >>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> -                              uint32_t node, Error **errp)
> >> +                              Error **errp)
> >>  {
> >>      Error *local_err = NULL;
> >>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> >>      PCDIMMDevice *dimm = PC_DIMM(dev);
> >>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >>      MemoryRegion *mr;
> >>      uint64_t align, size, addr;
> >> +    uint32_t node;
> >> +
> >> +    if (!smc->dr_lmb_enabled) {
> >> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
> >> +        goto out;
> >> +    }  
> > 
> > Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?  
> 
> Think you're right (and as spapr_memory_pre_plug() already exists, it's
> easy), other opinions? Thanks.
I also think that it should go to preplug

> 
> >   
> >> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
> >>  
> >>      mr = ddc->get_memory_region(dimm, &local_err);
> >>      if (local_err) {
> >> @@ -3568,19 +3576,8 @@ out:
> >>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >>                                        DeviceState *dev, Error **errp)
> >>  {
> >> -    MachineState *ms = MACHINE(hotplug_dev);
> >> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> >> -
> >>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >> -        int node;
> >> -
> >> -        if (!smc->dr_lmb_enabled) {
> >> -            error_setg(errp, "Memory hotplug not supported for this machine");
> >> -            return;
> >> -        }
> >> -        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
> >> -
> >> -        spapr_memory_plug(hotplug_dev, dev, node, errp);
> >> +        spapr_memory_plug(hotplug_dev, dev, errp);
> >>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> >>          spapr_core_plug(hotplug_dev, dev, errp);
> >>      }  
> >   
> 
>
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d038f3243e..a12be24ca9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3136,14 +3136,22 @@  static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                              uint32_t node, Error **errp)
+                              Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t align, size, addr;
+    uint32_t node;
+
+    if (!smc->dr_lmb_enabled) {
+        error_setg(&local_err, "Memory hotplug not supported for this machine");
+        goto out;
+    }
+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {
@@ -3568,19 +3576,8 @@  out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
-    MachineState *ms = MACHINE(hotplug_dev);
-    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        int node;
-
-        if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
-        }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
-
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     }