[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 3/8] spapr: Clean up dt creation for PCI buses
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH 3/8] spapr: Clean up dt creation for PCI buses |
Date: |
Fri, 31 May 2019 20:24:28 +1000 |
User-agent: |
Mutt/1.11.4 (2019-03-13) |
On Thu, May 30, 2019 at 03:43:26PM +1000, Alexey Kardashevskiy wrote:
>
>
> On 30/05/2019 15:33, David Gibson wrote:
> > 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.
>
>
> Just to double check you have not missed this bit :)
Bother, I did miss this bit. Fixed now.
--
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
[Qemu-ppc] [PATCH 6/8] spapr: Don't use bus number for building DRC ids, David Gibson, 2019/05/23
[Qemu-ppc] [PATCH 8/8] spapr: Allow hot plug/unplug of PCI bridges and devices under PCI bridges, David Gibson, 2019/05/23
[Qemu-ppc] [PATCH 4/8] spapr: Clean up spapr_drc_populate_dt(), David Gibson, 2019/05/23
[Qemu-ppc] [PATCH 7/8] spapr: Direct all PCI hotplug to host bridge, rather than P2P bridge, David Gibson, 2019/05/23
Re: [Qemu-ppc] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices, Greg Kurz, 2019/05/24
Re: [Qemu-ppc] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices, Michael S. Tsirkin, 2019/05/28