| Message ID | 160249772183.757627.7396780936543977766.stgit@bahia.lan |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spapr: Move spapr_create_nvdimm_dr_connectors() to core machine code | expand |
On 10/12/20 12:15 PM, Greg Kurz wrote: > The spapr_create_nvdimm_dr_connectors() function doesn't need to access > any internal details of the sPAPR NVDIMM implementation. Also, pretty > much like for the LMBs, only spapr_machine_init() is responsible for the > creation of DR connectors for NVDIMMs. > > Make this clear by making this function static in hw/ppc/spapr.c. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr.c | 10 ++++++++++ > hw/ppc/spapr_nvdimm.c | 11 ----------- > include/hw/ppc/spapr_nvdimm.h | 1 - > 3 files changed, 10 insertions(+), 12 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Mon, Oct 12, 2020 at 12:15:21PM +0200, Greg Kurz wrote: > The spapr_create_nvdimm_dr_connectors() function doesn't need to access > any internal details of the sPAPR NVDIMM implementation. Also, pretty > much like for the LMBs, only spapr_machine_init() is responsible for the > creation of DR connectors for NVDIMMs. > > Make this clear by making this function static in hw/ppc/spapr.c. > > Signed-off-by: Greg Kurz <groug@kaod.org> Hrm, I'm not really seeing the advantage to moving this. It doesn't have to be in spapr_nvdimm for data hiding, but it is related, and spapr.c is kind of huge. > --- > hw/ppc/spapr.c | 10 ++++++++++ > hw/ppc/spapr_nvdimm.c | 11 ----------- > include/hw/ppc/spapr_nvdimm.h | 1 - > 3 files changed, 10 insertions(+), 12 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 63315f2d0fa9..ee716a12af73 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2641,6 +2641,16 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > return rma_size; > } > > +static void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) > +{ > + MachineState *machine = MACHINE(spapr); > + int i; > + > + for (i = 0; i < machine->ram_slots; i++) { > + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); > + } > +} > + > /* pSeries LPAR / sPAPR hardware init */ > static void spapr_machine_init(MachineState *machine) > { > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c > index b3a489e9fe18..9e3d94071fe1 100644 > --- a/hw/ppc/spapr_nvdimm.c > +++ b/hw/ppc/spapr_nvdimm.c > @@ -106,17 +106,6 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > } > } > > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) > -{ > - MachineState *machine = MACHINE(spapr); > - int i; > - > - for (i = 0; i < machine->ram_slots; i++) { > - spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); > - } > -} > - > - > static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt, > int parent_offset, NVDIMMDevice *nvdimm) > { > diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h > index b834d82f5545..490b19a009f4 100644 > --- a/include/hw/ppc/spapr_nvdimm.h > +++ b/include/hw/ppc/spapr_nvdimm.h > @@ -31,6 +31,5 @@ void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); > bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, > uint64_t size, Error **errp); > void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr); > > #endif > > -- 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 Tue, 13 Oct 2020 11:40:14 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Oct 12, 2020 at 12:15:21PM +0200, Greg Kurz wrote: > > The spapr_create_nvdimm_dr_connectors() function doesn't need to access > > any internal details of the sPAPR NVDIMM implementation. Also, pretty > > much like for the LMBs, only spapr_machine_init() is responsible for the > > creation of DR connectors for NVDIMMs. > > > > Make this clear by making this function static in hw/ppc/spapr.c. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Hrm, I'm not really seeing the advantage to moving this. It doesn't > have to be in spapr_nvdimm for data hiding, but it is related, and > spapr.c is kind of huge. > The only advantage is to give an appropriate scope to this function, as many other functions that create internal devices, eg. other DRC types or the default PHB for which a similar change was accepted 2 years ago. commit 999c9caf2eee66103195e1ec7580b379929db9d2 Author: Greg Kurz <groug@kaod.org> Date: Fri Dec 21 01:35:09 2018 +0100 spapr: move spapr_create_phb() to core machine code This function is only used when creating the default PHB. Let's rename it and move it to the core machine code for clarity. Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> I agree that spapr.c is huge indeed (4943 lines) but this increases its size by _just_ 0.2 %. And there are certainly good candidates that landed in spapr.c by _default_ over the years but should rather be moved to their own compilation unit (eg. a bunch of FDT building functions for various resources or some hotplug related functions that don't need to access machine internals). > > --- > > hw/ppc/spapr.c | 10 ++++++++++ > > hw/ppc/spapr_nvdimm.c | 11 ----------- > > include/hw/ppc/spapr_nvdimm.h | 1 - > > 3 files changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 63315f2d0fa9..ee716a12af73 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2641,6 +2641,16 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > > return rma_size; > > } > > > > +static void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) > > +{ > > + MachineState *machine = MACHINE(spapr); > > + int i; > > + > > + for (i = 0; i < machine->ram_slots; i++) { > > + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); > > + } > > +} > > + > > /* pSeries LPAR / sPAPR hardware init */ > > static void spapr_machine_init(MachineState *machine) > > { > > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c > > index b3a489e9fe18..9e3d94071fe1 100644 > > --- a/hw/ppc/spapr_nvdimm.c > > +++ b/hw/ppc/spapr_nvdimm.c > > @@ -106,17 +106,6 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > > } > > } > > > > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) > > -{ > > - MachineState *machine = MACHINE(spapr); > > - int i; > > - > > - for (i = 0; i < machine->ram_slots; i++) { > > - spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); > > - } > > -} > > - > > - > > static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt, > > int parent_offset, NVDIMMDevice *nvdimm) > > { > > diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h > > index b834d82f5545..490b19a009f4 100644 > > --- a/include/hw/ppc/spapr_nvdimm.h > > +++ b/include/hw/ppc/spapr_nvdimm.h > > @@ -31,6 +31,5 @@ void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); > > bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, > > uint64_t size, Error **errp); > > void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); > > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr); > > > > #endif > > > > >
On Tue, Oct 13, 2020 at 09:33:44AM +0200, Greg Kurz wrote: > On Tue, 13 Oct 2020 11:40:14 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Oct 12, 2020 at 12:15:21PM +0200, Greg Kurz wrote: > > > The spapr_create_nvdimm_dr_connectors() function doesn't need to access > > > any internal details of the sPAPR NVDIMM implementation. Also, pretty > > > much like for the LMBs, only spapr_machine_init() is responsible for the > > > creation of DR connectors for NVDIMMs. > > > > > > Make this clear by making this function static in hw/ppc/spapr.c. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > Hrm, I'm not really seeing the advantage to moving this. It doesn't > > have to be in spapr_nvdimm for data hiding, but it is related, and > > spapr.c is kind of huge. > > > > The only advantage is to give an appropriate scope to this function, > as many other functions that create internal devices, eg. other DRC > types or the default PHB for which a similar change was accepted > 2 years ago. > > commit 999c9caf2eee66103195e1ec7580b379929db9d2 > Author: Greg Kurz <groug@kaod.org> > Date: Fri Dec 21 01:35:09 2018 +0100 > > spapr: move spapr_create_phb() to core machine code > > This function is only used when creating the default PHB. Let's rename > it and move it to the core machine code for clarity. > > Signed-off-by: Greg Kurz <groug@kaod.org> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > I agree that spapr.c is huge indeed (4943 lines) but this increases its > size by _just_ 0.2 %. And there are certainly good candidates that > landed in spapr.c by _default_ over the years but should rather be > moved to their own compilation unit (eg. a bunch of FDT building > functions for various resources or some hotplug related functions > that don't need to access machine internals). Good points. Applied to ppc-for-5.2. > > > > --- > > > hw/ppc/spapr.c | 10 ++++++++++ > > > hw/ppc/spapr_nvdimm.c | 11 ----------- > > > include/hw/ppc/spapr_nvdimm.h | 1 - > > > 3 files changed, 10 insertions(+), 12 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 63315f2d0fa9..ee716a12af73 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -2641,6 +2641,16 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > > > return rma_size; > > > } > > > > > > +static void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) > > > +{ > > > + MachineState *machine = MACHINE(spapr); > > > + int i; > > > + > > > + for (i = 0; i < machine->ram_slots; i++) { > > > + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); > > > + } > > > +} > > > + > > > /* pSeries LPAR / sPAPR hardware init */ > > > static void spapr_machine_init(MachineState *machine) > > > { > > > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c > > > index b3a489e9fe18..9e3d94071fe1 100644 > > > --- a/hw/ppc/spapr_nvdimm.c > > > +++ b/hw/ppc/spapr_nvdimm.c > > > @@ -106,17 +106,6 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > > > } > > > } > > > > > > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) > > > -{ > > > - MachineState *machine = MACHINE(spapr); > > > - int i; > > > - > > > - for (i = 0; i < machine->ram_slots; i++) { > > > - spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); > > > - } > > > -} > > > - > > > - > > > static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt, > > > int parent_offset, NVDIMMDevice *nvdimm) > > > { > > > diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h > > > index b834d82f5545..490b19a009f4 100644 > > > --- a/include/hw/ppc/spapr_nvdimm.h > > > +++ b/include/hw/ppc/spapr_nvdimm.h > > > @@ -31,6 +31,5 @@ void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); > > > bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, > > > uint64_t size, Error **errp); > > > void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); > > > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr); > > > > > > #endif > > > > > > > > > -- 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
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 63315f2d0fa9..ee716a12af73 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2641,6 +2641,16 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) return rma_size; } +static void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) +{ + MachineState *machine = MACHINE(spapr); + int i; + + for (i = 0; i < machine->ram_slots; i++) { + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); + } +} + /* pSeries LPAR / sPAPR hardware init */ static void spapr_machine_init(MachineState *machine) { diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c index b3a489e9fe18..9e3d94071fe1 100644 --- a/hw/ppc/spapr_nvdimm.c +++ b/hw/ppc/spapr_nvdimm.c @@ -106,17 +106,6 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) } } -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr) -{ - MachineState *machine = MACHINE(spapr); - int i; - - for (i = 0; i < machine->ram_slots; i++) { - spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i); - } -} - - static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt, int parent_offset, NVDIMMDevice *nvdimm) { diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h index b834d82f5545..490b19a009f4 100644 --- a/include/hw/ppc/spapr_nvdimm.h +++ b/include/hw/ppc/spapr_nvdimm.h @@ -31,6 +31,5 @@ void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, uint64_t size, Error **errp); void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr); #endif
The spapr_create_nvdimm_dr_connectors() function doesn't need to access any internal details of the sPAPR NVDIMM implementation. Also, pretty much like for the LMBs, only spapr_machine_init() is responsible for the creation of DR connectors for NVDIMMs. Make this clear by making this function static in hw/ppc/spapr.c. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 10 ++++++++++ hw/ppc/spapr_nvdimm.c | 11 ----------- include/hw/ppc/spapr_nvdimm.h | 1 - 3 files changed, 10 insertions(+), 12 deletions(-)