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(-)
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(-)
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 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
© 2016 - 2024 Red Hat, Inc.