diff mbox

[for-2.11,18/26] spapr: create DR connectors for PHBs

Message ID 150100571083.27487.4628655387393519076.stgit@bahia
State New
Headers show

Commit Message

Greg Kurz July 25, 2017, 6:01 p.m. UTC
From: Michael Roth <mdroth@linux.vnet.ibm.com>

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
Changes since RFC:
- rebased against ppc-for-2.10 (reset hooks registering already merged)
- added new DRC type for PHB
---
 hw/ppc/spapr.c             |   15 +++++++++++++++
 hw/ppc/spapr_drc.c         |   17 +++++++++++++++++
 include/hw/ppc/spapr_drc.h |    8 ++++++++
 3 files changed, 40 insertions(+)

Comments

David Gibson July 28, 2017, 3:49 a.m. UTC | #1
On Tue, Jul 25, 2017 at 08:01:50PM +0200, Greg Kurz wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Greg Kurz <groug@kaod.org>



> ---
> Changes since RFC:
> - rebased against ppc-for-2.10 (reset hooks registering already merged)
> - added new DRC type for PHB
> ---
>  hw/ppc/spapr.c             |   15 +++++++++++++++
>  hw/ppc/spapr_drc.c         |   17 +++++++++++++++++
>  include/hw/ppc/spapr_drc.h |    8 ++++++++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8dc505343c0f..5950c009ab7e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -98,6 +98,9 @@
>  
>  #define PHANDLE_XICP            0x00001111
>  
> +/* maximum number of hotpluggable PHBs */
> +#define SPAPR_DRC_MAX_PHB       256

I wonder if we should actually make this a machine property.

>  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>                                    const char *type_ics,
>                                    int nr_irqs, Error **errp)
> @@ -2384,6 +2387,18 @@ static void ppc_spapr_init(MachineState *machine)
>  
>      spapr->dr_phb_enabled = smc->dr_phb_enabled;
>  
> +    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
> +     * connectors (described in root DT node's "ibm,drc-types" property)
> +     * are pre-initialized here. additional child connectors (such as
> +     * connectors for a PHBs PCI slots) are added as needed during their
> +     * parent's realization.
> +     */
> +    if (spapr->dr_phb_enabled) {
> +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> +            spapr_dr_connector_new(OBJECT(machine), TYPE_SPAPR_DRC_PHB, i);
> +        }
> +    }
> +
>      /* Set up PCI */
>      spapr_pci_rtas_init();
>  
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index eb8024d37c54..2e1049ce61c7 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -697,6 +697,15 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
>      drck->release = spapr_lmb_release;
>  }
>  
> +static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> +{
> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> +    drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> +    drck->typename = "PHB";
> +    drck->drc_name_prefix = "PHB ";
> +}
> +
>  static const TypeInfo spapr_dr_connector_info = {
>      .name          = TYPE_SPAPR_DR_CONNECTOR,
>      .parent        = TYPE_DEVICE,
> @@ -740,6 +749,13 @@ static const TypeInfo spapr_drc_lmb_info = {
>      .class_init    = spapr_drc_lmb_class_init,
>  };
>  
> +static const TypeInfo spapr_drc_phb_info = {
> +    .name          = TYPE_SPAPR_DRC_PHB,
> +    .parent        = TYPE_SPAPR_DRC_LOGICAL,

I thought PHB DRCs were physical..

> +    .instance_size = sizeof(sPAPRDRConnector),
> +    .class_init    = spapr_drc_phb_class_init,
> +};
> +
>  /* helper functions for external users */
>  
>  sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
> @@ -1179,6 +1195,7 @@ static void spapr_drc_register_types(void)
>      type_register_static(&spapr_drc_cpu_info);
>      type_register_static(&spapr_drc_pci_info);
>      type_register_static(&spapr_drc_lmb_info);
> +    type_register_static(&spapr_drc_phb_info);
>  
>      spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
>                          rtas_set_indicator);
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index a7958d0a8d14..535fc61b98a8 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -69,6 +69,14 @@
>  #define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
>                                          TYPE_SPAPR_DRC_LMB)
>  
> +#define TYPE_SPAPR_DRC_PHB "spapr-drc-phb"
> +#define SPAPR_DRC_PHB_GET_CLASS(obj) \
> +        OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHB)
> +#define SPAPR_DRC_PHB_CLASS(klass) \
> +        OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PHB)
> +#define SPAPR_DRC_PHB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> +                                        TYPE_SPAPR_DRC_PHB)
> +
>  /*
>   * Various hotplug types managed by sPAPRDRConnector
>   *
>
Greg Kurz July 28, 2017, 10:30 a.m. UTC | #2
On Fri, 28 Jul 2017 13:49:25 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jul 25, 2017 at 08:01:50PM +0200, Greg Kurz wrote:
> > From: Michael Roth <mdroth@linux.vnet.ibm.com>
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> 
> 
> > ---
> > Changes since RFC:
> > - rebased against ppc-for-2.10 (reset hooks registering already merged)
> > - added new DRC type for PHB
> > ---
> >  hw/ppc/spapr.c             |   15 +++++++++++++++
> >  hw/ppc/spapr_drc.c         |   17 +++++++++++++++++
> >  include/hw/ppc/spapr_drc.h |    8 ++++++++
> >  3 files changed, 40 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 8dc505343c0f..5950c009ab7e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -98,6 +98,9 @@
> >  
> >  #define PHANDLE_XICP            0x00001111
> >  
> > +/* maximum number of hotpluggable PHBs */
> > +#define SPAPR_DRC_MAX_PHB       256  
> 
> I wonder if we should actually make this a machine property.
> 

It makes sense.

Also, if all PHBs are instanciated with index != -1, we're limited to 31.
Maybe this could be the default value for the machine property instead of
256 then ?

> >  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> >                                    const char *type_ics,
> >                                    int nr_irqs, Error **errp)
> > @@ -2384,6 +2387,18 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >      spapr->dr_phb_enabled = smc->dr_phb_enabled;
> >  
> > +    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
> > +     * connectors (described in root DT node's "ibm,drc-types" property)
> > +     * are pre-initialized here. additional child connectors (such as
> > +     * connectors for a PHBs PCI slots) are added as needed during their
> > +     * parent's realization.
> > +     */
> > +    if (spapr->dr_phb_enabled) {
> > +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> > +            spapr_dr_connector_new(OBJECT(machine), TYPE_SPAPR_DRC_PHB, i);
> > +        }
> > +    }
> > +
> >      /* Set up PCI */
> >      spapr_pci_rtas_init();
> >  
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index eb8024d37c54..2e1049ce61c7 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -697,6 +697,15 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> >      drck->release = spapr_lmb_release;
> >  }
> >  
> > +static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > +{
> > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > +    drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > +    drck->typename = "PHB";
> > +    drck->drc_name_prefix = "PHB ";
> > +}
> > +
> >  static const TypeInfo spapr_dr_connector_info = {
> >      .name          = TYPE_SPAPR_DR_CONNECTOR,
> >      .parent        = TYPE_DEVICE,
> > @@ -740,6 +749,13 @@ static const TypeInfo spapr_drc_lmb_info = {
> >      .class_init    = spapr_drc_lmb_class_init,
> >  };
> >  
> > +static const TypeInfo spapr_drc_phb_info = {
> > +    .name          = TYPE_SPAPR_DRC_PHB,
> > +    .parent        = TYPE_SPAPR_DRC_LOGICAL,  
> 
> I thought PHB DRCs were physical..
> 

My understanding is that only PCI IOAs need a physical DRC.

From LoPAPR v1.1 (March 24, 2016):

13.7 Logical Resource Dynamic Reconfiguration (LRDR)

The Logical Resource Dynamic Reconfiguration option allows a platform to make available and recover platform re-
sources such as CPUs, Memory Regions, Processor Host Bridges, and I/O slots to/from its operating OS image(s).

...

The device tree contains logical resource DR connectors for the maximum number of resources that the platform can
allocate to the specific OS. In some cases such as for processors and PHBs...

and

Table 240. Currently Defined DR Connector Types

| PHB | Logical PCI Host Bridge |

> > +    .instance_size = sizeof(sPAPRDRConnector),
> > +    .class_init    = spapr_drc_phb_class_init,
> > +};
> > +
> >  /* helper functions for external users */
> >  
> >  sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
> > @@ -1179,6 +1195,7 @@ static void spapr_drc_register_types(void)
> >      type_register_static(&spapr_drc_cpu_info);
> >      type_register_static(&spapr_drc_pci_info);
> >      type_register_static(&spapr_drc_lmb_info);
> > +    type_register_static(&spapr_drc_phb_info);
> >  
> >      spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> >                          rtas_set_indicator);
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index a7958d0a8d14..535fc61b98a8 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -69,6 +69,14 @@
> >  #define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> >                                          TYPE_SPAPR_DRC_LMB)
> >  
> > +#define TYPE_SPAPR_DRC_PHB "spapr-drc-phb"
> > +#define SPAPR_DRC_PHB_GET_CLASS(obj) \
> > +        OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHB)
> > +#define SPAPR_DRC_PHB_CLASS(klass) \
> > +        OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PHB)
> > +#define SPAPR_DRC_PHB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > +                                        TYPE_SPAPR_DRC_PHB)
> > +
> >  /*
> >   * Various hotplug types managed by sPAPRDRConnector
> >   *
> >   
>
David Gibson July 31, 2017, 2:58 a.m. UTC | #3
On Fri, Jul 28, 2017 at 12:30:35PM +0200, Greg Kurz wrote:
> On Fri, 28 Jul 2017 13:49:25 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jul 25, 2017 at 08:01:50PM +0200, Greg Kurz wrote:
> > > From: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > 
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > 
> > 
> > > ---
> > > Changes since RFC:
> > > - rebased against ppc-for-2.10 (reset hooks registering already merged)
> > > - added new DRC type for PHB
> > > ---
> > >  hw/ppc/spapr.c             |   15 +++++++++++++++
> > >  hw/ppc/spapr_drc.c         |   17 +++++++++++++++++
> > >  include/hw/ppc/spapr_drc.h |    8 ++++++++
> > >  3 files changed, 40 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 8dc505343c0f..5950c009ab7e 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -98,6 +98,9 @@
> > >  
> > >  #define PHANDLE_XICP            0x00001111
> > >  
> > > +/* maximum number of hotpluggable PHBs */
> > > +#define SPAPR_DRC_MAX_PHB       256  
> > 
> > I wonder if we should actually make this a machine property.
> > 
> 
> It makes sense.
> 
> Also, if all PHBs are instanciated with index != -1, we're limited to 31.
> Maybe this could be the default value for the machine property instead of
> 256 then ?

Actually, if we're binding it back to index, which has a hard limit,
then it no longer makes sense to have it as a property and we should
go back to a constant (well, it could vary by machine type version).

> > >  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> > >                                    const char *type_ics,
> > >                                    int nr_irqs, Error **errp)
> > > @@ -2384,6 +2387,18 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >      spapr->dr_phb_enabled = smc->dr_phb_enabled;
> > >  
> > > +    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
> > > +     * connectors (described in root DT node's "ibm,drc-types" property)
> > > +     * are pre-initialized here. additional child connectors (such as
> > > +     * connectors for a PHBs PCI slots) are added as needed during their
> > > +     * parent's realization.
> > > +     */
> > > +    if (spapr->dr_phb_enabled) {
> > > +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> > > +            spapr_dr_connector_new(OBJECT(machine), TYPE_SPAPR_DRC_PHB, i);
> > > +        }
> > > +    }
> > > +
> > >      /* Set up PCI */
> > >      spapr_pci_rtas_init();
> > >  
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index eb8024d37c54..2e1049ce61c7 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -697,6 +697,15 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > >      drck->release = spapr_lmb_release;
> > >  }
> > >  
> > > +static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > > +{
> > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > > +
> > > +    drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > > +    drck->typename = "PHB";
> > > +    drck->drc_name_prefix = "PHB ";
> > > +}
> > > +
> > >  static const TypeInfo spapr_dr_connector_info = {
> > >      .name          = TYPE_SPAPR_DR_CONNECTOR,
> > >      .parent        = TYPE_DEVICE,
> > > @@ -740,6 +749,13 @@ static const TypeInfo spapr_drc_lmb_info = {
> > >      .class_init    = spapr_drc_lmb_class_init,
> > >  };
> > >  
> > > +static const TypeInfo spapr_drc_phb_info = {
> > > +    .name          = TYPE_SPAPR_DRC_PHB,
> > > +    .parent        = TYPE_SPAPR_DRC_LOGICAL,  
> > 
> > I thought PHB DRCs were physical..
> > 
> 
> My understanding is that only PCI IOAs need a physical DRC.
> 
> From LoPAPR v1.1 (March 24, 2016):
> 
> 13.7 Logical Resource Dynamic Reconfiguration (LRDR)
> 
> The Logical Resource Dynamic Reconfiguration option allows a platform to make available and recover platform re-
> sources such as CPUs, Memory Regions, Processor Host Bridges, and I/O slots to/from its operating OS image(s).
> 
> ...
> 
> The device tree contains logical resource DR connectors for the maximum number of resources that the platform can
> allocate to the specific OS. In some cases such as for processors and PHBs...
> 
> and
> 
> Table 240. Currently Defined DR Connector Types
> 
> | PHB | Logical PCI Host Bridge |

Ah, my mistake.

> 
> > > +    .instance_size = sizeof(sPAPRDRConnector),
> > > +    .class_init    = spapr_drc_phb_class_init,
> > > +};
> > > +
> > >  /* helper functions for external users */
> > >  
> > >  sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
> > > @@ -1179,6 +1195,7 @@ static void spapr_drc_register_types(void)
> > >      type_register_static(&spapr_drc_cpu_info);
> > >      type_register_static(&spapr_drc_pci_info);
> > >      type_register_static(&spapr_drc_lmb_info);
> > > +    type_register_static(&spapr_drc_phb_info);
> > >  
> > >      spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> > >                          rtas_set_indicator);
> > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > index a7958d0a8d14..535fc61b98a8 100644
> > > --- a/include/hw/ppc/spapr_drc.h
> > > +++ b/include/hw/ppc/spapr_drc.h
> > > @@ -69,6 +69,14 @@
> > >  #define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > >                                          TYPE_SPAPR_DRC_LMB)
> > >  
> > > +#define TYPE_SPAPR_DRC_PHB "spapr-drc-phb"
> > > +#define SPAPR_DRC_PHB_GET_CLASS(obj) \
> > > +        OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHB)
> > > +#define SPAPR_DRC_PHB_CLASS(klass) \
> > > +        OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PHB)
> > > +#define SPAPR_DRC_PHB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > > +                                        TYPE_SPAPR_DRC_PHB)
> > > +
> > >  /*
> > >   * Various hotplug types managed by sPAPRDRConnector
> > >   *
> > >   
> > 
>
Greg Kurz Sept. 6, 2017, 11:32 a.m. UTC | #4
On Mon, 31 Jul 2017 12:58:25 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 28, 2017 at 12:30:35PM +0200, Greg Kurz wrote:
> > On Fri, 28 Jul 2017 13:49:25 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Jul 25, 2017 at 08:01:50PM +0200, Greg Kurz wrote:  
> > > > From: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > 
> > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > 
> > > 
> > >   
> > > > ---
> > > > Changes since RFC:
> > > > - rebased against ppc-for-2.10 (reset hooks registering already merged)
> > > > - added new DRC type for PHB
> > > > ---
> > > >  hw/ppc/spapr.c             |   15 +++++++++++++++
> > > >  hw/ppc/spapr_drc.c         |   17 +++++++++++++++++
> > > >  include/hw/ppc/spapr_drc.h |    8 ++++++++
> > > >  3 files changed, 40 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 8dc505343c0f..5950c009ab7e 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -98,6 +98,9 @@
> > > >  
> > > >  #define PHANDLE_XICP            0x00001111
> > > >  
> > > > +/* maximum number of hotpluggable PHBs */
> > > > +#define SPAPR_DRC_MAX_PHB       256    
> > > 
> > > I wonder if we should actually make this a machine property.
> > >   
> > 
> > It makes sense.
> > 
> > Also, if all PHBs are instanciated with index != -1, we're limited to 31.
> > Maybe this could be the default value for the machine property instead of
> > 256 then ?  
> 
> Actually, if we're binding it back to index, which has a hard limit,
> then it no longer makes sense to have it as a property and we should
> go back to a constant (well, it could vary by machine type version).
> 

But I guess that the hard limit of 31 as described in the changelog of
commit 357d1e3bc7d2d80e5271bc4f3ac8537e30dc8046 still holds, doesn't it ?

    Because some guest versions (including most current distro kernels) can't
    access PCI MMIO above 64 TiB, we put all the PCI windows between 32 TiB and
    64 TiB.  This is broken into 1 TiB chunks.  The first 1 TiB contains the
    PIO (64 kiB) and 32-bit MMIO (2 GiB) windows for all of the PHBs.  Each
    subsequent TiB chunk contains a naturally aligned 64-bit MMIO window for
    one PHB each.
    
    This reduces the number of allowed PHBs (without full manual configuration
    of all the windows) from 256 to 31, but this should still be plenty in
    practice.

Not sure why a machine type version would have a different limit. Can
you think of a use case ?

> > > >  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> > > >                                    const char *type_ics,
> > > >                                    int nr_irqs, Error **errp)
> > > > @@ -2384,6 +2387,18 @@ static void ppc_spapr_init(MachineState *machine)
> > > >  
> > > >      spapr->dr_phb_enabled = smc->dr_phb_enabled;
> > > >  
> > > > +    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
> > > > +     * connectors (described in root DT node's "ibm,drc-types" property)
> > > > +     * are pre-initialized here. additional child connectors (such as
> > > > +     * connectors for a PHBs PCI slots) are added as needed during their
> > > > +     * parent's realization.
> > > > +     */
> > > > +    if (spapr->dr_phb_enabled) {
> > > > +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> > > > +            spapr_dr_connector_new(OBJECT(machine), TYPE_SPAPR_DRC_PHB, i);
> > > > +        }
> > > > +    }
> > > > +
> > > >      /* Set up PCI */
> > > >      spapr_pci_rtas_init();
> > > >  
> > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > > index eb8024d37c54..2e1049ce61c7 100644
> > > > --- a/hw/ppc/spapr_drc.c
> > > > +++ b/hw/ppc/spapr_drc.c
> > > > @@ -697,6 +697,15 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > > >      drck->release = spapr_lmb_release;
> > > >  }
> > > >  
> > > > +static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > > > +{
> > > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > > > +
> > > > +    drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > > > +    drck->typename = "PHB";
> > > > +    drck->drc_name_prefix = "PHB ";
> > > > +}
> > > > +
> > > >  static const TypeInfo spapr_dr_connector_info = {
> > > >      .name          = TYPE_SPAPR_DR_CONNECTOR,
> > > >      .parent        = TYPE_DEVICE,
> > > > @@ -740,6 +749,13 @@ static const TypeInfo spapr_drc_lmb_info = {
> > > >      .class_init    = spapr_drc_lmb_class_init,
> > > >  };
> > > >  
> > > > +static const TypeInfo spapr_drc_phb_info = {
> > > > +    .name          = TYPE_SPAPR_DRC_PHB,
> > > > +    .parent        = TYPE_SPAPR_DRC_LOGICAL,    
> > > 
> > > I thought PHB DRCs were physical..
> > >   
> > 
> > My understanding is that only PCI IOAs need a physical DRC.
> > 
> > From LoPAPR v1.1 (March 24, 2016):
> > 
> > 13.7 Logical Resource Dynamic Reconfiguration (LRDR)
> > 
> > The Logical Resource Dynamic Reconfiguration option allows a platform to make available and recover platform re-
> > sources such as CPUs, Memory Regions, Processor Host Bridges, and I/O slots to/from its operating OS image(s).
> > 
> > ...
> > 
> > The device tree contains logical resource DR connectors for the maximum number of resources that the platform can
> > allocate to the specific OS. In some cases such as for processors and PHBs...
> > 
> > and
> > 
> > Table 240. Currently Defined DR Connector Types
> > 
> > | PHB | Logical PCI Host Bridge |  
> 
> Ah, my mistake.
> 
> >   
> > > > +    .instance_size = sizeof(sPAPRDRConnector),
> > > > +    .class_init    = spapr_drc_phb_class_init,
> > > > +};
> > > > +
> > > >  /* helper functions for external users */
> > > >  
> > > >  sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
> > > > @@ -1179,6 +1195,7 @@ static void spapr_drc_register_types(void)
> > > >      type_register_static(&spapr_drc_cpu_info);
> > > >      type_register_static(&spapr_drc_pci_info);
> > > >      type_register_static(&spapr_drc_lmb_info);
> > > > +    type_register_static(&spapr_drc_phb_info);
> > > >  
> > > >      spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> > > >                          rtas_set_indicator);
> > > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > > index a7958d0a8d14..535fc61b98a8 100644
> > > > --- a/include/hw/ppc/spapr_drc.h
> > > > +++ b/include/hw/ppc/spapr_drc.h
> > > > @@ -69,6 +69,14 @@
> > > >  #define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > > >                                          TYPE_SPAPR_DRC_LMB)
> > > >  
> > > > +#define TYPE_SPAPR_DRC_PHB "spapr-drc-phb"
> > > > +#define SPAPR_DRC_PHB_GET_CLASS(obj) \
> > > > +        OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHB)
> > > > +#define SPAPR_DRC_PHB_CLASS(klass) \
> > > > +        OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PHB)
> > > > +#define SPAPR_DRC_PHB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > > > +                                        TYPE_SPAPR_DRC_PHB)
> > > > +
> > > >  /*
> > > >   * Various hotplug types managed by sPAPRDRConnector
> > > >   *
> > > >     
> > >   
> >   
> 
> 
>
David Gibson Sept. 13, 2017, 12:23 p.m. UTC | #5
On Wed, Sep 06, 2017 at 01:32:09PM +0200, Greg Kurz wrote:
> On Mon, 31 Jul 2017 12:58:25 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 28, 2017 at 12:30:35PM +0200, Greg Kurz wrote:
> > > On Fri, 28 Jul 2017 13:49:25 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Tue, Jul 25, 2017 at 08:01:50PM +0200, Greg Kurz wrote:  
> > > > > From: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > 
> > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > 
> > > >   
> > > > > ---
> > > > > Changes since RFC:
> > > > > - rebased against ppc-for-2.10 (reset hooks registering already merged)
> > > > > - added new DRC type for PHB
> > > > > ---
> > > > >  hw/ppc/spapr.c             |   15 +++++++++++++++
> > > > >  hw/ppc/spapr_drc.c         |   17 +++++++++++++++++
> > > > >  include/hw/ppc/spapr_drc.h |    8 ++++++++
> > > > >  3 files changed, 40 insertions(+)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 8dc505343c0f..5950c009ab7e 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -98,6 +98,9 @@
> > > > >  
> > > > >  #define PHANDLE_XICP            0x00001111
> > > > >  
> > > > > +/* maximum number of hotpluggable PHBs */
> > > > > +#define SPAPR_DRC_MAX_PHB       256    
> > > > 
> > > > I wonder if we should actually make this a machine property.
> > > >   
> > > 
> > > It makes sense.
> > > 
> > > Also, if all PHBs are instanciated with index != -1, we're limited to 31.
> > > Maybe this could be the default value for the machine property instead of
> > > 256 then ?  
> > 
> > Actually, if we're binding it back to index, which has a hard limit,
> > then it no longer makes sense to have it as a property and we should
> > go back to a constant (well, it could vary by machine type version).

Sorry I've taken so long to reply.

> But I guess that the hard limit of 31 as described in the changelog of
> commit 357d1e3bc7d2d80e5271bc4f3ac8537e30dc8046 still holds, doesn't
> it ?

That's right.  Note that that is a limit of *31* PHBs (numbered
0..30), not 32 PHBs numbered 0..31.

>     Because some guest versions (including most current distro kernels) can't
>     access PCI MMIO above 64 TiB, we put all the PCI windows between 32 TiB and
>     64 TiB.  This is broken into 1 TiB chunks.  The first 1 TiB contains the
>     PIO (64 kiB) and 32-bit MMIO (2 GiB) windows for all of the PHBs.  Each
>     subsequent TiB chunk contains a naturally aligned 64-bit MMIO window for
>     one PHB each.
>     
>     This reduces the number of allowed PHBs (without full manual configuration
>     of all the windows) from 256 to 31, but this should still be plenty in
>     practice.
> 
> Not sure why a machine type version would have a different limit. Can
> you think of a use case ?

Well, the older machine types had a different layout.  It allowed for
more indexes, but had smaller windows, which meant certain cards (e.g.
GPGPUs with huge BARs) didn't work properly.  It also had some weird
alignments that meant we were a bit wasteful of address space.

But we can't change the location of PHB windows on migration, so we
had to maintain that old layout for old machine types.  That's why
there's a different limit depending on machine type version.

> 
> > > > >  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> > > > >                                    const char *type_ics,
> > > > >                                    int nr_irqs, Error **errp)
> > > > > @@ -2384,6 +2387,18 @@ static void ppc_spapr_init(MachineState *machine)
> > > > >  
> > > > >      spapr->dr_phb_enabled = smc->dr_phb_enabled;
> > > > >  
> > > > > +    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
> > > > > +     * connectors (described in root DT node's "ibm,drc-types" property)
> > > > > +     * are pre-initialized here. additional child connectors (such as
> > > > > +     * connectors for a PHBs PCI slots) are added as needed during their
> > > > > +     * parent's realization.
> > > > > +     */
> > > > > +    if (spapr->dr_phb_enabled) {
> > > > > +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> > > > > +            spapr_dr_connector_new(OBJECT(machine), TYPE_SPAPR_DRC_PHB, i);
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > >      /* Set up PCI */
> > > > >      spapr_pci_rtas_init();
> > > > >  
> > > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > > > index eb8024d37c54..2e1049ce61c7 100644
> > > > > --- a/hw/ppc/spapr_drc.c
> > > > > +++ b/hw/ppc/spapr_drc.c
> > > > > @@ -697,6 +697,15 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > > > >      drck->release = spapr_lmb_release;
> > > > >  }
> > > > >  
> > > > > +static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > > > > +{
> > > > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > > > > +
> > > > > +    drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > > > > +    drck->typename = "PHB";
> > > > > +    drck->drc_name_prefix = "PHB ";
> > > > > +}
> > > > > +
> > > > >  static const TypeInfo spapr_dr_connector_info = {
> > > > >      .name          = TYPE_SPAPR_DR_CONNECTOR,
> > > > >      .parent        = TYPE_DEVICE,
> > > > > @@ -740,6 +749,13 @@ static const TypeInfo spapr_drc_lmb_info = {
> > > > >      .class_init    = spapr_drc_lmb_class_init,
> > > > >  };
> > > > >  
> > > > > +static const TypeInfo spapr_drc_phb_info = {
> > > > > +    .name          = TYPE_SPAPR_DRC_PHB,
> > > > > +    .parent        = TYPE_SPAPR_DRC_LOGICAL,    
> > > > 
> > > > I thought PHB DRCs were physical..
> > > >   
> > > 
> > > My understanding is that only PCI IOAs need a physical DRC.
> > > 
> > > From LoPAPR v1.1 (March 24, 2016):
> > > 
> > > 13.7 Logical Resource Dynamic Reconfiguration (LRDR)
> > > 
> > > The Logical Resource Dynamic Reconfiguration option allows a platform to make available and recover platform re-
> > > sources such as CPUs, Memory Regions, Processor Host Bridges, and I/O slots to/from its operating OS image(s).
> > > 
> > > ...
> > > 
> > > The device tree contains logical resource DR connectors for the maximum number of resources that the platform can
> > > allocate to the specific OS. In some cases such as for processors and PHBs...
> > > 
> > > and
> > > 
> > > Table 240. Currently Defined DR Connector Types
> > > 
> > > | PHB | Logical PCI Host Bridge |  
> > 
> > Ah, my mistake.
> > 
> > >   
> > > > > +    .instance_size = sizeof(sPAPRDRConnector),
> > > > > +    .class_init    = spapr_drc_phb_class_init,
> > > > > +};
> > > > > +
> > > > >  /* helper functions for external users */
> > > > >  
> > > > >  sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
> > > > > @@ -1179,6 +1195,7 @@ static void spapr_drc_register_types(void)
> > > > >      type_register_static(&spapr_drc_cpu_info);
> > > > >      type_register_static(&spapr_drc_pci_info);
> > > > >      type_register_static(&spapr_drc_lmb_info);
> > > > > +    type_register_static(&spapr_drc_phb_info);
> > > > >  
> > > > >      spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> > > > >                          rtas_set_indicator);
> > > > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > > > index a7958d0a8d14..535fc61b98a8 100644
> > > > > --- a/include/hw/ppc/spapr_drc.h
> > > > > +++ b/include/hw/ppc/spapr_drc.h
> > > > > @@ -69,6 +69,14 @@
> > > > >  #define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > > > >                                          TYPE_SPAPR_DRC_LMB)
> > > > >  
> > > > > +#define TYPE_SPAPR_DRC_PHB "spapr-drc-phb"
> > > > > +#define SPAPR_DRC_PHB_GET_CLASS(obj) \
> > > > > +        OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHB)
> > > > > +#define SPAPR_DRC_PHB_CLASS(klass) \
> > > > > +        OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PHB)
> > > > > +#define SPAPR_DRC_PHB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > > > > +                                        TYPE_SPAPR_DRC_PHB)
> > > > > +
> > > > >  /*
> > > > >   * Various hotplug types managed by sPAPRDRConnector
> > > > >   *
> > > > >     
> > > >   
> > >   
> > 
> > 
> > 
>
Greg Kurz Sept. 13, 2017, 12:56 p.m. UTC | #6
On Wed, 13 Sep 2017 22:23:29 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

[...snip...]
> > > > Also, if all PHBs are instanciated with index != -1, we're limited to 31.
> > > > Maybe this could be the default value for the machine property instead of
> > > > 256 then ?    
> > > 
> > > Actually, if we're binding it back to index, which has a hard limit,
> > > then it no longer makes sense to have it as a property and we should
> > > go back to a constant (well, it could vary by machine type version).  
> 
> Sorry I've taken so long to reply.
> 

Oh, don't mention it. :)

> > But I guess that the hard limit of 31 as described in the changelog of
> > commit 357d1e3bc7d2d80e5271bc4f3ac8537e30dc8046 still holds, doesn't
> > it ?  
> 
> That's right.  Note that that is a limit of *31* PHBs (numbered
> 0..30), not 32 PHBs numbered 0..31.
> 

Yeah I saw that.

> >     Because some guest versions (including most current distro kernels) can't
> >     access PCI MMIO above 64 TiB, we put all the PCI windows between 32 TiB and
> >     64 TiB.  This is broken into 1 TiB chunks.  The first 1 TiB contains the
> >     PIO (64 kiB) and 32-bit MMIO (2 GiB) windows for all of the PHBs.  Each
> >     subsequent TiB chunk contains a naturally aligned 64-bit MMIO window for
> >     one PHB each.
> >     
> >     This reduces the number of allowed PHBs (without full manual configuration
> >     of all the windows) from 256 to 31, but this should still be plenty in
> >     practice.
> > 
> > Not sure why a machine type version would have a different limit. Can
> > you think of a use case ?  
> 
> Well, the older machine types had a different layout.  It allowed for
> more indexes, but had smaller windows, which meant certain cards (e.g.
> GPGPUs with huge BARs) didn't work properly.  It also had some weird
> alignments that meant we were a bit wasteful of address space.
> 
> But we can't change the location of PHB windows on migration, so we
> had to maintain that old layout for old machine types.  That's why
> there's a different limit depending on machine type version.
> 

Ok, so we *just* have 2 different maximum number of PHBs:
- 256 for pseries <= 2.7
- 31 for newer machine types

I sent a RFC patch last monday:

https://lists.nongnu.org/archive/html/qemu-ppc/2017-09/msg00203.html

> >   
> > > > > >  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> > > > > >                                    const char *type_ics,
> > > > > >                                    int nr_irqs, Error **errp)
> > > > > > @@ -2384,6 +2387,18 @@ static void ppc_spapr_init(MachineState *machine)
> > > > > >  
> > > > > >      spapr->dr_phb_enabled = smc->dr_phb_enabled;
> > > > > >  
> > > > > > +    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
> > > > > > +     * connectors (described in root DT node's "ibm,drc-types" property)
> > > > > > +     * are pre-initialized here. additional child connectors (such as
> > > > > > +     * connectors for a PHBs PCI slots) are added as needed during their
> > > > > > +     * parent's realization.
> > > > > > +     */
> > > > > > +    if (spapr->dr_phb_enabled) {
> > > > > > +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> > > > > > +            spapr_dr_connector_new(OBJECT(machine), TYPE_SPAPR_DRC_PHB, i);
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > >      /* Set up PCI */
> > > > > >      spapr_pci_rtas_init();
> > > > > >  
> > > > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > > > > index eb8024d37c54..2e1049ce61c7 100644
> > > > > > --- a/hw/ppc/spapr_drc.c
> > > > > > +++ b/hw/ppc/spapr_drc.c
> > > > > > @@ -697,6 +697,15 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > > > > >      drck->release = spapr_lmb_release;
> > > > > >  }
> > > > > >  
> > > > > > +static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > > > > > +{
> > > > > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > > > > > +
> > > > > > +    drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > > > > > +    drck->typename = "PHB";
> > > > > > +    drck->drc_name_prefix = "PHB ";
> > > > > > +}
> > > > > > +
> > > > > >  static const TypeInfo spapr_dr_connector_info = {
> > > > > >      .name          = TYPE_SPAPR_DR_CONNECTOR,
> > > > > >      .parent        = TYPE_DEVICE,
> > > > > > @@ -740,6 +749,13 @@ static const TypeInfo spapr_drc_lmb_info = {
> > > > > >      .class_init    = spapr_drc_lmb_class_init,
> > > > > >  };
> > > > > >  
> > > > > > +static const TypeInfo spapr_drc_phb_info = {
> > > > > > +    .name          = TYPE_SPAPR_DRC_PHB,
> > > > > > +    .parent        = TYPE_SPAPR_DRC_LOGICAL,      
> > > > > 
> > > > > I thought PHB DRCs were physical..
> > > > >     
> > > > 
> > > > My understanding is that only PCI IOAs need a physical DRC.
> > > > 
> > > > From LoPAPR v1.1 (March 24, 2016):
> > > > 
> > > > 13.7 Logical Resource Dynamic Reconfiguration (LRDR)
> > > > 
> > > > The Logical Resource Dynamic Reconfiguration option allows a platform to make available and recover platform re-
> > > > sources such as CPUs, Memory Regions, Processor Host Bridges, and I/O slots to/from its operating OS image(s).
> > > > 
> > > > ...
> > > > 
> > > > The device tree contains logical resource DR connectors for the maximum number of resources that the platform can
> > > > allocate to the specific OS. In some cases such as for processors and PHBs...
> > > > 
> > > > and
> > > > 
> > > > Table 240. Currently Defined DR Connector Types
> > > > 
> > > > | PHB | Logical PCI Host Bridge |    
> > > 
> > > Ah, my mistake.
> > >   
> > > >     
> > > > > > +    .instance_size = sizeof(sPAPRDRConnector),
> > > > > > +    .class_init    = spapr_drc_phb_class_init,
> > > > > > +};
> > > > > > +
> > > > > >  /* helper functions for external users */
> > > > > >  
> > > > > >  sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
> > > > > > @@ -1179,6 +1195,7 @@ static void spapr_drc_register_types(void)
> > > > > >      type_register_static(&spapr_drc_cpu_info);
> > > > > >      type_register_static(&spapr_drc_pci_info);
> > > > > >      type_register_static(&spapr_drc_lmb_info);
> > > > > > +    type_register_static(&spapr_drc_phb_info);
> > > > > >  
> > > > > >      spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> > > > > >                          rtas_set_indicator);
> > > > > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > > > > index a7958d0a8d14..535fc61b98a8 100644
> > > > > > --- a/include/hw/ppc/spapr_drc.h
> > > > > > +++ b/include/hw/ppc/spapr_drc.h
> > > > > > @@ -69,6 +69,14 @@
> > > > > >  #define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > > > > >                                          TYPE_SPAPR_DRC_LMB)
> > > > > >  
> > > > > > +#define TYPE_SPAPR_DRC_PHB "spapr-drc-phb"
> > > > > > +#define SPAPR_DRC_PHB_GET_CLASS(obj) \
> > > > > > +        OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHB)
> > > > > > +#define SPAPR_DRC_PHB_CLASS(klass) \
> > > > > > +        OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PHB)
> > > > > > +#define SPAPR_DRC_PHB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > > > > > +                                        TYPE_SPAPR_DRC_PHB)
> > > > > > +
> > > > > >  /*
> > > > > >   * Various hotplug types managed by sPAPRDRConnector
> > > > > >   *
> > > > > >       
> > > > >     
> > > >     
> > > 
> > > 
> > >   
> >   
> 
> 
>
David Gibson Sept. 15, 2017, 9:09 a.m. UTC | #7
On Wed, Sep 13, 2017 at 02:56:51PM +0200, Greg Kurz wrote:
> On Wed, 13 Sep 2017 22:23:29 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> [...snip...]
> > > > > Also, if all PHBs are instanciated with index != -1, we're limited to 31.
> > > > > Maybe this could be the default value for the machine property instead of
> > > > > 256 then ?    
> > > > 
> > > > Actually, if we're binding it back to index, which has a hard limit,
> > > > then it no longer makes sense to have it as a property and we should
> > > > go back to a constant (well, it could vary by machine type version).  
> > 
> > Sorry I've taken so long to reply.
> > 
> 
> Oh, don't mention it. :)
> 
> > > But I guess that the hard limit of 31 as described in the changelog of
> > > commit 357d1e3bc7d2d80e5271bc4f3ac8537e30dc8046 still holds, doesn't
> > > it ?  
> > 
> > That's right.  Note that that is a limit of *31* PHBs (numbered
> > 0..30), not 32 PHBs numbered 0..31.
> > 
> 
> Yeah I saw that.
> 
> > >     Because some guest versions (including most current distro kernels) can't
> > >     access PCI MMIO above 64 TiB, we put all the PCI windows between 32 TiB and
> > >     64 TiB.  This is broken into 1 TiB chunks.  The first 1 TiB contains the
> > >     PIO (64 kiB) and 32-bit MMIO (2 GiB) windows for all of the PHBs.  Each
> > >     subsequent TiB chunk contains a naturally aligned 64-bit MMIO window for
> > >     one PHB each.
> > >     
> > >     This reduces the number of allowed PHBs (without full manual configuration
> > >     of all the windows) from 256 to 31, but this should still be plenty in
> > >     practice.
> > > 
> > > Not sure why a machine type version would have a different limit. Can
> > > you think of a use case ?  
> > 
> > Well, the older machine types had a different layout.  It allowed for
> > more indexes, but had smaller windows, which meant certain cards (e.g.
> > GPGPUs with huge BARs) didn't work properly.  It also had some weird
> > alignments that meant we were a bit wasteful of address space.
> > 
> > But we can't change the location of PHB windows on migration, so we
> > had to maintain that old layout for old machine types.  That's why
> > there's a different limit depending on machine type version.
> > 
> 
> Ok, so we *just* have 2 different maximum number of PHBs:
> - 256 for pseries <= 2.7
> - 31 for newer machine types

Yes, at least for now.  If we ever discover that 31 PHBs *isn't*
enough, we may have to come up with yet another layout, which could
have a different limit.
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8dc505343c0f..5950c009ab7e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -98,6 +98,9 @@ 
 
 #define PHANDLE_XICP            0x00001111
 
+/* maximum number of hotpluggable PHBs */
+#define SPAPR_DRC_MAX_PHB       256
+
 static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
                                   const char *type_ics,
                                   int nr_irqs, Error **errp)
@@ -2384,6 +2387,18 @@  static void ppc_spapr_init(MachineState *machine)
 
     spapr->dr_phb_enabled = smc->dr_phb_enabled;
 
+    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
+     * connectors (described in root DT node's "ibm,drc-types" property)
+     * are pre-initialized here. additional child connectors (such as
+     * connectors for a PHBs PCI slots) are added as needed during their
+     * parent's realization.
+     */
+    if (spapr->dr_phb_enabled) {
+        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
+            spapr_dr_connector_new(OBJECT(machine), TYPE_SPAPR_DRC_PHB, i);
+        }
+    }
+
     /* Set up PCI */
     spapr_pci_rtas_init();
 
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index eb8024d37c54..2e1049ce61c7 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -697,6 +697,15 @@  static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
     drck->release = spapr_lmb_release;
 }
 
+static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
+{
+    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+    drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
+    drck->typename = "PHB";
+    drck->drc_name_prefix = "PHB ";
+}
+
 static const TypeInfo spapr_dr_connector_info = {
     .name          = TYPE_SPAPR_DR_CONNECTOR,
     .parent        = TYPE_DEVICE,
@@ -740,6 +749,13 @@  static const TypeInfo spapr_drc_lmb_info = {
     .class_init    = spapr_drc_lmb_class_init,
 };
 
+static const TypeInfo spapr_drc_phb_info = {
+    .name          = TYPE_SPAPR_DRC_PHB,
+    .parent        = TYPE_SPAPR_DRC_LOGICAL,
+    .instance_size = sizeof(sPAPRDRConnector),
+    .class_init    = spapr_drc_phb_class_init,
+};
+
 /* helper functions for external users */
 
 sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
@@ -1179,6 +1195,7 @@  static void spapr_drc_register_types(void)
     type_register_static(&spapr_drc_cpu_info);
     type_register_static(&spapr_drc_pci_info);
     type_register_static(&spapr_drc_lmb_info);
+    type_register_static(&spapr_drc_phb_info);
 
     spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
                         rtas_set_indicator);
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index a7958d0a8d14..535fc61b98a8 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -69,6 +69,14 @@ 
 #define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
                                         TYPE_SPAPR_DRC_LMB)
 
+#define TYPE_SPAPR_DRC_PHB "spapr-drc-phb"
+#define SPAPR_DRC_PHB_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHB)
+#define SPAPR_DRC_PHB_CLASS(klass) \
+        OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PHB)
+#define SPAPR_DRC_PHB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+                                        TYPE_SPAPR_DRC_PHB)
+
 /*
  * Various hotplug types managed by sPAPRDRConnector
  *