Re: [Qemu-devel] [Qemu-ppc] [for-2.11 PATCH 18/26] spapr: create DR connectors for PHBs

[David Gibson]

On Wed, Sep 06, 2017 at 01:32:09PM +0200, Greg Kurz wrote:
> On Mon, 31 Jul 2017 12:58:25 +1000
> David Gibson <address@hidden> 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 <address@hidden> wrote:
> > >   
> > > > On Tue, Jul 25, 2017 at 08:01:50PM +0200, Greg Kurz wrote:  
> > > > > From: Michael Roth <address@hidden>
> > > > > 
> > > > > Signed-off-by: Michael Roth <address@hidden>
> > > > > Reviewed-by: David Gibson <address@hidden>
> > > > > Signed-off-by: Greg Kurz <address@hidden>    
> > > > 
> > > > 
> > > >   
> > > > > ---
> > > > > 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
> > > > >   *
> > > > >     
> > > >   
> > >   
> > 
> > 
> > 
> 



-- 
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

signature.asc
Description: PGP signature