Re: [Qemu-ppc] [PATCH 3/8] spapr: Clean up dt creation for PCI buses

[David Gibson]

On Fri, May 24, 2019 at 03:31:54PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 23/05/2019 15:29, David Gibson wrote:
> > Device nodes for PCI bridges (both host and P2P) describe both the bridge
> > device itself and the bus hanging off it, handling of this is a bit of a
> > mess.
> > 
> > spapr_dt_pci_device() has a few things it only adds for non-bridges, but
> > always adds #address-cells and #size-cells which should only appear for
> > bridges.  But the walking down the subordinate PCI bus is done in one of
> > its callers spapr_populate_pci_devices_dt().  The PHB dt creation in
> > spapr_populate_pci_dt() open codes some similar logic to the bridge case.
> > 
> > This patch consolidates things in a bunch of ways:
> >  * Bus specific dt info is now created in spapr_dt_pci_bus() used for both
> >    P2P bridges and the host bridge.  This includes walking subordinate
> >    devices
> >  * spapr_dt_pci_device() now calls spapr_dt_pci_bus() when called on a
> >    P2P bridge
> >  * We do detection of bridges with the is_bridge field of the device class,
> >    rather than checking PCI config space directly, for consistency with
> >    qemu's core PCI code.
> >  * Several things are renamed for brevity and clarity
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr.c              |   7 +-
> >  hw/ppc/spapr_pci.c          | 140 +++++++++++++++++++-----------------
> >  include/hw/pci-host/spapr.h |   4 +-
> >  3 files changed, 79 insertions(+), 72 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e2b33e5890..44573adce7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1309,8 +1309,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
> >      }
> >  
> >      QLIST_FOREACH(phb, &spapr->phbs, list) {
> > -        ret = spapr_populate_pci_dt(phb, PHANDLE_INTC, fdt,
> > -                                    spapr->irq->nr_msis, NULL);
> > +        ret = spapr_dt_phb(phb, PHANDLE_INTC, fdt, spapr->irq->nr_msis, 
> > NULL);
> >          if (ret < 0) {
> >              error_report("couldn't setup PCI devices in fdt");
> >              exit(1);
> > @@ -3917,8 +3916,8 @@ int spapr_phb_dt_populate(SpaprDrc *drc, 
> > SpaprMachineState *spapr,
> >          return -1;
> >      }
> >  
> > -    if (spapr_populate_pci_dt(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> > -                              fdt_start_offset)) {
> > +    if (spapr_dt_phb(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> > +                     fdt_start_offset)) {
> >          error_setg(errp, "unable to create FDT node for PHB %d", 
> > sphb->index);
> >          return -1;
> >      }
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 4075b433fd..c166df4145 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1219,6 +1219,60 @@ static const char *dt_name_from_class(uint8_t class, 
> > uint8_t subclass,
> >  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> >                                              PCIDevice *pdev);
> >  
> > +typedef struct PciWalkFdt {
> > +    void *fdt;
> > +    int offset;
> > +    SpaprPhbState *sphb;
> > +    int err;
> > +} PciWalkFdt;
> > +
> > +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> > +                               void *fdt, int parent_offset);
> > +
> > +static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
> > +                                   void *opaque)
> > +{
> > +    PciWalkFdt *p = opaque;
> > +    int err;
> > +
> > +    if (p->err) {
> > +        /* Something's already broken, don't keep going */
> > +        return;
> > +    }
> > +
> > +    err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
> > +    if (err < 0) {
> > +        p->err = err;
> > +    }
> > +}
> > +
> > +/* Augment PCI device node with bridge specific information */
> > +static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
> > +                               void *fdt, int offset)
> > +{
> > +    PciWalkFdt cbinfo = {
> > +        .fdt = fdt,
> > +        .offset = offset,
> > +        .sphb = sphb,
> > +        .err = 0,
> > +    };
> > +
> > +    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> > +                          RESOURCE_CELLS_ADDRESS));
> > +    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> > +                          RESOURCE_CELLS_SIZE));
> > +
> > +    if (bus) {
> > +        pci_for_each_device_reverse(bus, pci_bus_num(bus),
> > +                                    spapr_dt_pci_device_cb, &cbinfo);
> > +        if (cbinfo.err) {
> > +            return cbinfo.err;
> > +        }
> > +    }
> > +
> > +    return offset;
> 
> 
> This spapr_dt_pci_bus() returns 0 or an offset or an error.
> 
> But:
> spapr_dt_phb() returns 0 or error; and so does spapr_dt_drc().
> 
> Not extremely consistent.

No, it's not.  But the inconsistency is already there between the
device function which needs to return an offset and the PHB function
and others which don't.

I have some ideas for how to clean this up, along with a bunch of
other dt creation stuff, but I don't think it's reasonably in scope
for this series.

> It looks like spapr_dt_pci_bus() does not need to return @offset as it
> does not change it and the caller - spapr_dt_pci_device() - can have 1
> "return offset" in the end.

True, but changing this here just shuffles the inconsistency around
without really improving it.  I've put cleaning this up more widely on
my list of things to tackle when I have time.

> 
> 
> > +}
> > +
> >  /* create OF node for pci device and required OF DT properties */
> >  static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >                                 void *fdt, int parent_offset)
> > @@ -1228,10 +1282,9 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, 
> > PCIDevice *dev,
> >      char *nodename;
> >      int slot = PCI_SLOT(dev->devfn);
> >      int func = PCI_FUNC(dev->devfn);
> > +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> >      ResourceProps rp;
> >      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> > -    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 
> > 1);
> > -    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
> >      uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
> >      uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
> >      uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 
> > 1);
> > @@ -1268,13 +1321,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, 
> > PCIDevice *dev,
> >          _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
> >      }
> >  
> > -    if (!is_bridge) {
> > -        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> > -        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 
> > 1);
> > -        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> > -        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> > -    }
> > -
> >      if (subsystem_id) {
> >          _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
> >      }
> > @@ -1309,11 +1355,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, 
> > PCIDevice *dev,
> >          _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> >      }
> >  
> > -    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> > -                          RESOURCE_CELLS_ADDRESS));
> > -    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> > -                          RESOURCE_CELLS_SIZE));
> > -
> >      if (msi_present(dev)) {
> >          uint32_t max_msi = msi_nr_vectors_allocated(dev);
> >          if (max_msi) {
> > @@ -1338,7 +1379,18 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, 
> > PCIDevice *dev,
> >  
> >      spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
> >  
> > -    return offset;
> > +    if (!pc->is_bridge) {
> > +        /* Properties only for non-bridges */
> > +        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> > +        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 
> > 1);
> > +        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> > +        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> > +        return offset;
> > +    } else {
> > +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> > +
> > +        return spapr_dt_pci_bus(sphb, sec_bus, fdt, offset);
> > +    }
> >  }
> >  
> >  /* Callback to be called during DRC release. */
> > @@ -2063,44 +2115,6 @@ static const TypeInfo spapr_phb_info = {
> >      }
> >  };
> >  
> > -typedef struct SpaprFdt {
> > -    void *fdt;
> > -    int node_off;
> > -    SpaprPhbState *sphb;
> > -} SpaprFdt;
> > -
> > -static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> > -                                          void *opaque)
> > -{
> > -    PCIBus *sec_bus;
> > -    SpaprFdt *p = opaque;
> > -    int offset;
> > -    SpaprFdt s_fdt;
> > -
> > -    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
> > -    if (!offset) {
> > -        error_report("Failed to create pci child device tree node");
> > -        return;
> > -    }
> > -
> > -    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
> > -         PCI_HEADER_TYPE_BRIDGE)) {
> > -        return;
> > -    }
> > -
> > -    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> > -    if (!sec_bus) {
> > -        return;
> > -    }
> > -
> > -    s_fdt.fdt = p->fdt;
> > -    s_fdt.node_off = offset;
> > -    s_fdt.sphb = p->sphb;
> > -    pci_for_each_device_reverse(sec_bus, pci_bus_num(sec_bus),
> > -                                spapr_populate_pci_devices_dt,
> > -                                &s_fdt);
> > -}
> > -
> >  static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
> >                                             void *opaque)
> >  {
> > @@ -2138,8 +2152,8 @@ static void spapr_phb_pci_enumerate(SpaprPhbState 
> > *phb)
> >  
> >  }
> >  
> > -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void 
> > *fdt,
> > -                          uint32_t nr_msis, int *node_offset)
> > +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> > +                 uint32_t nr_msis, int *node_offset)
> >  {
> >      int bus_off, i, j, ret;
> >      uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
> > @@ -2186,8 +2200,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, 
> > uint32_t intc_phandle, void *fdt,
> >                                  cpu_to_be32(0x0),
> >                                  cpu_to_be32(phb->numa_node)};
> >      SpaprTceTable *tcet;
> > -    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> > -    SpaprFdt s_fdt;
> >      SpaprDrc *drc;
> >      Error *errp = NULL;
> >  
> > @@ -2200,8 +2212,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, 
> > uint32_t intc_phandle, void *fdt,
> >      /* Write PHB properties */
> >      _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
> >      _FDT(fdt_setprop_string(fdt, bus_off, "compatible", 
> > "IBM,Logical_PHB"));
> > -    _FDT(fdt_setprop_cell(fdt, bus_off, "#address-cells", 0x3));
> > -    _FDT(fdt_setprop_cell(fdt, bus_off, "#size-cells", 0x2));
> >      _FDT(fdt_setprop_cell(fdt, bus_off, "#interrupt-cells", 0x1));
> >      _FDT(fdt_setprop(fdt, bus_off, "used-by-rtas", NULL, 0));
> >      _FDT(fdt_setprop(fdt, bus_off, "bus-range", &bus_range, 
> > sizeof(bus_range)));
> > @@ -2266,13 +2276,11 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, 
> > uint32_t intc_phandle, void *fdt,
> >      spapr_phb_pci_enumerate(phb);
> >      _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
> >  
> > -    /* Populate tree nodes with PCI devices attached */
> > -    s_fdt.fdt = fdt;
> > -    s_fdt.node_off = bus_off;
> > -    s_fdt.sphb = phb;
> > -    pci_for_each_device_reverse(bus, pci_bus_num(bus),
> > -                                spapr_populate_pci_devices_dt,
> > -                                &s_fdt);
> > +    /* Walk the bridge and subordinate buses */
> > +    ret = spapr_dt_pci_bus(phb, PCI_HOST_BRIDGE(phb)->bus, fdt, bus_off);
> > +    if (ret) {
> 
> 
> if (ret < 0)
> 
> otherwise it returns prematurely and NVLink stuff does not make it to
> the FDT.
> 
> 
> Otherwise the whole patchset is a good cleanup and seems working fine.
> 
> 
> > +        return ret;
> > +    }
> >  
> >      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
> >                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
> > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > index 53519c835e..1b61162f91 100644
> > --- a/include/hw/pci-host/spapr.h
> > +++ b/include/hw/pci-host/spapr.h
> > @@ -131,8 +131,8 @@ static inline qemu_irq spapr_phb_lsi_qirq(struct 
> > SpaprPhbState *phb, int pin)
> >      return spapr_qirq(spapr, phb->lsi_table[pin].irq);
> >  }
> >  
> > -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void 
> > *fdt,
> > -                          uint32_t nr_msis, int *node_offset);
> > +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> > +                 uint32_t nr_msis, int *node_offset);
> >  
> >  void spapr_pci_rtas_init(void);
> >  
> > 
> 

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