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 <da...@gibson.dropbear.id.au>
>>> ---
>>> 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 :)
--
Alexey
