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

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 7697 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]