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: 7540 bytes --]

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


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