Re: [Qemu-ppc] [PATCH 02/12] spapr_pci: populate DRC dt entries for PHBs

[Michael Roth]

Quoting Alexey Kardashevskiy (2014-08-26 04:09:36)
> On 08/19/2014 10:21 AM, Michael Roth wrote:
> > Reserve 32 entries of type PCI in each PHB's initial FDT. This
> > advertises to guests that each PHB is DR-capable device with
> > physical hotpluggable slots. This is necessary for allowing
> > hotplugging of devices to it later via bus rescan or guest rpaphp
> > hotplug module.
> > 
> > Each entry is assigned a name of "Slot <<bus_idx>*32 +1>",
> > advertised as a hotpluggable PCI slot, and assigned to power domain
> > -1 to indicate to the guest that power management is handled by the
> > hardware.
> > 
> > This models a DR-capable PCI expansion device attached to a host/lpar
> > via a single PHB with 32 physical hotpluggable slots (as opposed to a
> > virtual bridge device with external management console). Hotplug will
> > be handled by the guest via bus rescan or the rpaphp hotplug module.
> > 
> > Signed-off-by: Michael Roth <address@hidden>
> > ---
> >  hw/ppc/spapr.c              |   3 +-
> >  hw/ppc/spapr_pci.c          | 102 
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci-host/spapr.h |   1 +
> >  3 files changed, 105 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d5e46c3..90b25b3 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -890,7 +890,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
> >      QLIST_FOREACH(phb, &spapr->phbs, list) {
> >          drc_entry = spapr_phb_to_drc_entry(phb->buid);
> >          g_assert(drc_entry);
> > -        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt);
> > +        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, 
> > drc_entry->drc_index,
> > +                                    fdt);
> >      }
> >  
> >      if (ret < 0) {
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index e85134f..924d488 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -851,8 +851,104 @@ static int spapr_phb_children_dt(Object *child, void 
> > *opaque)
> >      return 1;
> >  }
> >  
> > +static void spapr_create_drc_phb_dt_entries(void *fdt, int bus_off, int 
> > phb_index)
> > +{
> > +    char char_buf[1024];
> > +    uint32_t int_buf[SPAPR_DRC_PHB_SLOT_MAX + 1];
> > +    uint32_t *entries;
> > +    int i, ret, offset;
> > +
> > +    /* ibm,drc-indexes */
> > +    memset(int_buf, 0 , sizeof(int_buf));
> > +    int_buf[0] = SPAPR_DRC_PHB_SLOT_MAX;
> > +
> > +    for (i = 1; i <= SPAPR_DRC_PHB_SLOT_MAX; i++) {
> > +        int_buf[i] = SPAPR_DRC_DEV_ID_BASE + (phb_index << 8) + ((i - 1) 
> > << 3);
> > +    }
> > +
> > +    ret = fdt_setprop(fdt, bus_off, "ibm,drc-indexes", int_buf,
> > +                      sizeof(int_buf));
> > +    if (ret) {
> > +        fprintf(stderr, "error adding 'ibm,drc-indexes' field for PHB 
> > FDT");
> > +    }
> > +
> > +    /* ibm,drc-power-domains */
> > +    memset(int_buf, 0, sizeof(int_buf));
> > +    int_buf[0] = SPAPR_DRC_PHB_SLOT_MAX;
> > +
> > +    for (i = 1; i <= SPAPR_DRC_PHB_SLOT_MAX; i++) {
> > +        int_buf[i] = 0xffffffff;
> > +    }
> > +
> > +    ret = fdt_setprop(fdt, bus_off, "ibm,drc-power-domains", int_buf,
> > +                      sizeof(int_buf));
> > +    if (ret) {
> > +        fprintf(stderr,
> > +                "error adding 'ibm,drc-power-domains' field for PHB FDT");
> > +    }
> > +
> > +    /* ibm,drc-names */
> > +    memset(char_buf, 0, sizeof(char_buf));
> > +    entries = (uint32_t *)&char_buf[0];
> > +    *entries = SPAPR_DRC_PHB_SLOT_MAX;
> > +    offset = sizeof(*entries);
> > +
> > +    for (i = 1; i <= SPAPR_DRC_PHB_SLOT_MAX; i++) {
> > +        offset += sprintf(char_buf + offset, "Slot %d",
> > +                          (phb_index * SPAPR_DRC_PHB_SLOT_MAX) + i - 1);
> > +        char_buf[offset++] = '\0';
> > +    }
> > +
> > +    ret = fdt_setprop(fdt, bus_off, "ibm,drc-names", char_buf, offset);
> > +    if (ret) {
> > +        fprintf(stderr, "error adding 'ibm,drc-names' field for PHB FDT");
> > +    }
> > +
> > +    /* ibm,drc-types */
> > +    memset(char_buf, 0, sizeof(char_buf));
> > +    entries = (uint32_t *)&char_buf[0];
> > +    *entries = SPAPR_DRC_PHB_SLOT_MAX;
> > +    offset = sizeof(*entries);
> > +
> > +    for (i = 0; i < SPAPR_DRC_PHB_SLOT_MAX; i++) {
> > +        offset += sprintf(char_buf + offset, "28");
> 
> 
> "28"? Is it for "PHB"?

For each actual PCI slot. C.6.1 in PAPR 2.7 defines a stringified 28 as:

  "A PCI Express Rev 2 slot with 8x lanes."

...there may be more appropriate values to use though, such as "1":

  "A 32-bit, 5 Volt conventional PCI slot which accommodates cards that operate 
up to 33 MHz Only."

since we don't emulate PCIe with spapr-host-bridge. I seem to recall having 
issues with some
of these other values though, will test again and see.

> 
> 
> 
> 
> -- 
> Alexey