Message ID | 20180607165218.9558-4-david@redhat.com |
---|---|
State | New |
Headers | show |
Series | pc/spapr/s390x: machine hotplug handler cleanups | expand |
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); > }
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); > }
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); >> } >
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 --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); }
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(-)