[Qemu-devel] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU

[Qemu-devel] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU

[Nikunj A Dadhania]

The patch series creates PCI device tree(DT) nodes in QEMU. The new
hotplug code needs the device node creation in QEMU. While during
boot, nodes were created in SLOF. It makes more sense to consolidate
the code to one place for better maintainability.

Based on David's spapr-next 
https://github.com/dgibson/qemu/tree/spapr-next

Also, patches for populating ibm,loc-code was getting very complicated
with use of RTAS/HCALL

Changelog V2:
 * Fix device tree for 64-bit encoding
 * Fix the class code, was failing xhci
 * Remove macro duplication
 * Fix DT fields generation for boot time device (Michael Roth)

Changelog v1:
 * Correct indent problems reported by checkpatch(David Gibson)
 * Declare sPAPRFDT structure as local (David Gibson)
 * Re-arrange code to avoid multiple indentation (Alexey Kardashevskiy)

Michael Roth (1):
  spapr_pci: fix boot-time device tree fields for pci hotplug

Nikunj A Dadhania (5):
  spapr_pci: remove duplicate macros
  spapr_pci: encode missing 64-bit memory address space
  spapr_pci: encode class code including Prog IF register
  spapr_pci: enumerate and add PCI device tree
  spapr_pci: populate ibm,loc-code

 hw/ppc/spapr_pci.c | 221 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 198 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/4] spapr_pci: encode missing 64-bit memory address space

[Nikunj A Dadhania]

The properties reg/assigned-resources need to encode 64-bit memory
address space as part of phys.hi dword.

  00 if configuration space
  01 if IO region,
  10 if 32-bit MEM region
  11 if 64-bit MEM region

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4d18f2d..b4f4242 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -787,7 +787,13 @@ typedef struct ResourceProps {
  * phys.hi = 0xYYXXXXZZ, where:
  *   0xYY = npt000ss
  *          |||   |
- *          |||   +-- space code: 1 if IO region, 2 if MEM region
+ *          |||   +-- space code
+ *          |||               |
+ *          |||               +  00 if configuration space
+ *          |||               +  01 if IO region,
+ *          |||               +  10 if 32-bit MEM region
+ *          |||               +  11 if 64-bit MEM region
+ *          |||
  *          ||+------ for non-relocatable IO: 1 if aliased
  *          ||        for relocatable IO: 1 if below 64KB
  *          ||        for MEM: 1 if below 1MB
@@ -847,6 +853,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
         reg->phys_hi = cpu_to_be32(dev_id | b_rrrrrrrr(pci_bar(d, i)));
         if (d->io_regions[i].type & PCI_BASE_ADDRESS_SPACE_IO) {
             reg->phys_hi |= cpu_to_be32(b_ss(1));
+        } else if (d->io_regions[i].type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+            reg->phys_hi |= cpu_to_be32(b_ss(3));
         } else {
             reg->phys_hi |= cpu_to_be32(b_ss(2));
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/6] spapr_pci: remove duplicate macros

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2e7590c..4df3a33 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1475,17 +1475,6 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
-/* Macros to operate with address in OF binding to PCI */
-#define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
-#define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
-#define b_p(x)          b_x((x), 30, 1) /* 1 if prefetchable */
-#define b_t(x)          b_x((x), 29, 1) /* 1 if the address is aliased */
-#define b_ss(x)         b_x((x), 24, 2) /* the space code */
-#define b_bbbbbbbb(x)   b_x((x), 16, 8) /* bus number */
-#define b_ddddd(x)      b_x((x), 11, 5) /* device number */
-#define b_fff(x)        b_x((x), 8, 3)  /* function number */
-#define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
-
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
                           void *fdt)
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/4] spapr_pci: encode class code including Prog IF register

[Nikunj A Dadhania]

Current code missed the Prog IF register. All Class Code, Subclass,
and Prog IF registers are needed to identify the accurate device type.

For example: USB controllers use the PROG IF for denoting: USB
FullSpeed, HighSpeed or SuperSpeed.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index b4f4242..821f82e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -900,8 +900,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
     _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
                           pci_default_read_config(dev, PCI_REVISION_ID, 1)));
     _FDT(fdt_setprop_cell(fdt, offset, "class-code",
-                          pci_default_read_config(dev, PCI_CLASS_DEVICE, 2)
-                            << 8));
+                          pci_default_read_config(dev, PCI_CLASS_PROG, 3)));
     if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) {
         _FDT(fdt_setprop_cell(fdt, offset, "interrupts",
                  pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)));
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/6] spapr_pci: encode missing 64-bit memory address space

[Nikunj A Dadhania]

The properties reg/assigned-resources need to encode 64-bit memory
address space as part of phys.hi dword.

  00 if configuration space
  01 if IO region,
  10 if 32-bit MEM region
  11 if 64-bit MEM region

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4df3a33..ea1a092 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -786,7 +786,13 @@ typedef struct ResourceProps {
  * phys.hi = 0xYYXXXXZZ, where:
  *   0xYY = npt000ss
  *          |||   |
- *          |||   +-- space code: 1 if IO region, 2 if MEM region
+ *          |||   +-- space code
+ *          |||               |
+ *          |||               +  00 if configuration space
+ *          |||               +  01 if IO region,
+ *          |||               +  10 if 32-bit MEM region
+ *          |||               +  11 if 64-bit MEM region
+ *          |||
  *          ||+------ for non-relocatable IO: 1 if aliased
  *          ||        for relocatable IO: 1 if below 64KB
  *          ||        for MEM: 1 if below 1MB
@@ -846,6 +852,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
         reg->phys_hi = cpu_to_be32(dev_id | b_rrrrrrrr(pci_bar(d, i)));
         if (d->io_regions[i].type & PCI_BASE_ADDRESS_SPACE_IO) {
             reg->phys_hi |= cpu_to_be32(b_ss(1));
+        } else if (d->io_regions[i].type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+            reg->phys_hi |= cpu_to_be32(b_ss(3));
         } else {
             reg->phys_hi |= cpu_to_be32(b_ss(2));
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/4] spapr: enumerate and add PCI device tree

[Nikunj A Dadhania]

All the PCI enumeration and device node creation was off-loaded to
SLOF. With PCI hotplug support, code needed to be added to add device
node. This creates multiple copy of the code one in SLOF and other in
hotplug code. To unify this, the patch adds the pci device node
creation in Qemu. For backward compatibility, a flag
"qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
do device node creation.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 103 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 821f82e..829c3ef 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -23,6 +23,7 @@
  * THE SOFTWARE.
  */
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
@@ -35,6 +36,7 @@
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
 
+#include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/ppc/spapr_drc.h"
 #include "sysemu/device_tree.h"
@@ -946,7 +948,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
      * processed by OF beforehand
      */
     _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
-    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
+    if (drc_name) {
+        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
+                         strlen(drc_name)));
+    }
     _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
@@ -1002,10 +1007,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
     void *fdt = NULL;
     int fdt_start_offset = 0;
 
-    /* boot-time devices get their device tree node created by SLOF, but for
-     * hotplugged devices we need QEMU to generate it so the guest can fetch
-     * it via RTAS
-     */
     if (dev->hotplugged) {
         fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
                                         &fdt_start_offset);
@@ -1495,6 +1496,89 @@ PCIHostState *spapr_create_phb(sPAPRMachineState *sm, int index)
 #define b_fff(x)        b_x((x), 8, 3)  /* function number */
 #define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
 
+typedef struct sPAPRFDT {
+    void *fdt;
+    int node_off;
+    uint32_t index;
+} sPAPRFDT;
+
+static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
+                                          void *opaque)
+{
+    PCIBus *sec_bus;
+    sPAPRFDT *p = opaque;
+    int ret, offset;
+    int slot = PCI_SLOT(pdev->devfn);
+    int func = PCI_FUNC(pdev->devfn);
+    char nodename[512];
+    sPAPRFDT s_fdt;
+
+    if (func) {
+        sprintf(nodename, "pci@%d,%d", slot, func);
+    } else {
+        sprintf(nodename, "pci@%d", slot);
+    }
+    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    g_assert(!ret);
+
+    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.index = p->index;
+    pci_for_each_device(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)
+{
+    unsigned short *bus_no = (unsigned short *) opaque;
+    unsigned short primary = *bus_no;
+    unsigned short secondary;
+    unsigned short subordinate = 0xff;
+
+    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
+         PCI_HEADER_TYPE_BRIDGE)) {
+        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
+        secondary = *bus_no + 1;
+        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
+        *bus_no = *bus_no + 1;
+        if (sec_bus) {
+            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
+            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                                spapr_phb_pci_enumerate_bridge,
+                                bus_no);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
+        }
+    }
+}
+
+static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
+{
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    unsigned short bus_no = 0;
+
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_phb_pci_enumerate_bridge,
+                        &bus_no);
+
+}
+
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
                           void *fdt)
@@ -1534,6 +1618,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
         cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
     uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
     sPAPRTCETable *tcet;
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    sPAPRFDT s_fdt;
 
     /* Start populating the FDT */
     sprintf(nodename, "pci@%" PRIx64, phb->buid);
@@ -1583,6 +1669,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
                  tcet->liobn, tcet->bus_offset,
                  tcet->nb_table << tcet->page_shift);
 
+    /* Walk the bridges and program the bus numbers*/
+    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.index = phb->index;
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_populate_pci_devices_dt,
+                        &s_fdt);
+
     ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
                                 SPAPR_DR_CONNECTOR_TYPE_PCI);
     if (ret) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/6] spapr_pci: encode class code including Prog IF register

[Nikunj A Dadhania]

Current code missed the Prog IF register. All Class Code, Subclass,
and Prog IF registers are needed to identify the accurate device type.

For example: USB controllers use the PROG IF for denoting: USB
FullSpeed, HighSpeed or SuperSpeed.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ea1a092..8b02a3e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -899,8 +899,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
     _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
                           pci_default_read_config(dev, PCI_REVISION_ID, 1)));
     _FDT(fdt_setprop_cell(fdt, offset, "class-code",
-                          pci_default_read_config(dev, PCI_CLASS_DEVICE, 2)
-                            << 8));
+                          pci_default_read_config(dev, PCI_CLASS_PROG, 3)));
     if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) {
         _FDT(fdt_setprop_cell(fdt, offset, "interrupts",
                  pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)));
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 4/4] spapr: populate ibm,loc-code

[Nikunj A Dadhania]

Each hardware instance has a platform unique location code.  The OF
device tree that describes a part of a hardware entity must include
the “ibm,loc-code” property with a value that represents the location
code for that hardware entity.

Populate ibm,loc-code.
1) PCI passthru devices need to identify with its own ibm,loc-code
   available on the host.
2) Emulated devices encode as following:
   qemu_<name>:<phb-index>:<slot>.<fn>

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 75 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 829c3ef..67b5cc0 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -745,6 +745,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &phb->iommu_as;
 }
 
+static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
+{
+    char *host;
+    char path[PATH_MAX];
+
+    host = object_property_get_str(OBJECT(pdev), "host", NULL);
+    if (!host) {
+        return false;
+    }
+
+    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
+    g_free(host);
+
+    return g_file_get_contents(path, value, NULL, NULL);
+}
+
+static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char path[PATH_MAX], *buf = NULL;
+
+    /* We have a vfio host bridge lets get the path. */
+    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
+        return NULL;
+    }
+
+    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
+    g_free(buf);
+
+    if (g_file_get_contents(path, &buf, NULL, NULL)) {
+        return buf;
+    } else {
+        return NULL;
+    }
+}
+
+static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char *path = g_malloc(PATH_MAX);
+
+    if (!path) {
+        return NULL;
+    }
+
+    /*
+     * For non-vfio devices and failures make up the location code out
+     * of the name, slot and function.
+     *
+     *       qemu_<name>:<phb-index>:<slot>.<fn>
+     */
+    snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
+             sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    return path;
+}
+
+
+static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
+{
+    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
+        return spapr_phb_vfio_get_loc_code(sphb, pdev);
+    } else {
+        return spapr_phb_get_loc_code(sphb, pdev);
+    }
+}
+
 /* Macros to operate with address in OF binding to PCI */
 #define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
 #define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
@@ -882,12 +946,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
 }
 
 static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
-                                       int phb_index, int drc_index,
-                                       const char *drc_name)
+                                       sPAPRPHBState *phb, int drc_index)
 {
     ResourceProps rp;
     bool is_bridge = false;
     int pci_status;
+    char *buf = NULL;
 
     if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
         PCI_HEADER_TYPE_BRIDGE) {
@@ -948,9 +1012,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
      * processed by OF beforehand
      */
     _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
-    if (drc_name) {
-        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
-                         strlen(drc_name)));
+    buf = spapr_ibm_get_loc_code(phb, dev);
+    if (buf) {
+        _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
+        g_free(buf);
     }
     _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
 
@@ -987,8 +1052,7 @@ static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(fdt, 0, nodename);
-    ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb->index, drc_index,
-                                      drc_name);
+    ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb, drc_index);
     g_assert(!ret);
 
     *dt_offset = offset;
@@ -1499,7 +1563,7 @@ PCIHostState *spapr_create_phb(sPAPRMachineState *sm, int index)
 typedef struct sPAPRFDT {
     void *fdt;
     int node_off;
-    uint32_t index;
+    sPAPRPHBState *sphb;
 } sPAPRFDT;
 
 static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
@@ -1519,7 +1583,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
-    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb, 0);
     g_assert(!ret);
 
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
@@ -1534,7 +1598,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
 
     s_fdt.fdt = p->fdt;
     s_fdt.node_off = offset;
-    s_fdt.index = p->index;
+    s_fdt.sphb = p->sphb;
     pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
                         spapr_populate_pci_devices_dt,
                         &s_fdt);
@@ -1676,7 +1740,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     /* Populate tree nodes with PCI devices attached */
     s_fdt.fdt = fdt;
     s_fdt.node_off = bus_off;
-    s_fdt.index = phb->index;
+    s_fdt.sphb = phb;
     pci_for_each_device(bus, pci_bus_num(bus),
                         spapr_populate_pci_devices_dt,
                         &s_fdt);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree

[Nikunj A Dadhania]

All the PCI enumeration and device node creation was off-loaded to
SLOF. With PCI hotplug support, code needed to be added to add device
node. This creates multiple copy of the code one in SLOF and other in
hotplug code. To unify this, the patch adds the pci device node
creation in Qemu. For backward compatibility, a flag
"qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
do device node creation.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 103 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 8b02a3e..103284a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -23,6 +23,7 @@
  * THE SOFTWARE.
  */
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
@@ -35,6 +36,7 @@
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
 
+#include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/ppc/spapr_drc.h"
 #include "sysemu/device_tree.h"
@@ -945,7 +947,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
      * processed by OF beforehand
      */
     _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
-    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
+    if (drc_name) {
+        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
+                         strlen(drc_name)));
+    }
     _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
@@ -1001,10 +1006,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
     void *fdt = NULL;
     int fdt_start_offset = 0;
 
-    /* boot-time devices get their device tree node created by SLOF, but for
-     * hotplugged devices we need QEMU to generate it so the guest can fetch
-     * it via RTAS
-     */
     if (dev->hotplugged) {
         fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
                                         &fdt_start_offset);
@@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
+typedef struct sPAPRFDT {
+    void *fdt;
+    int node_off;
+    uint32_t index;
+} sPAPRFDT;
+
+static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
+                                          void *opaque)
+{
+    PCIBus *sec_bus;
+    sPAPRFDT *p = opaque;
+    int ret, offset;
+    int slot = PCI_SLOT(pdev->devfn);
+    int func = PCI_FUNC(pdev->devfn);
+    char nodename[512];
+    sPAPRFDT s_fdt;
+
+    if (func) {
+        sprintf(nodename, "pci@%d,%d", slot, func);
+    } else {
+        sprintf(nodename, "pci@%d", slot);
+    }
+    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    g_assert(!ret);
+
+    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.index = p->index;
+    pci_for_each_device(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)
+{
+    unsigned short *bus_no = (unsigned short *) opaque;
+    unsigned short primary = *bus_no;
+    unsigned short secondary;
+    unsigned short subordinate = 0xff;
+
+    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
+         PCI_HEADER_TYPE_BRIDGE)) {
+        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
+        secondary = *bus_no + 1;
+        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
+        *bus_no = *bus_no + 1;
+        if (sec_bus) {
+            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
+            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                                spapr_phb_pci_enumerate_bridge,
+                                bus_no);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
+        }
+    }
+}
+
+static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
+{
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    unsigned short bus_no = 0;
+
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_phb_pci_enumerate_bridge,
+                        &bus_no);
+
+}
+
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
                           void *fdt)
@@ -1521,6 +1605,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
         cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
     uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
     sPAPRTCETable *tcet;
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    sPAPRFDT s_fdt;
 
     /* Start populating the FDT */
     sprintf(nodename, "pci@%" PRIx64, phb->buid);
@@ -1570,6 +1656,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
                  tcet->liobn, tcet->bus_offset,
                  tcet->nb_table << tcet->page_shift);
 
+    /* Walk the bridges and program the bus numbers*/
+    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.index = phb->index;
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_populate_pci_devices_dt,
+                        &s_fdt);
+
     ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
                                 SPAPR_DR_CONNECTOR_TYPE_PCI);
     if (ret) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 5/6] spapr_pci: fix boot-time device tree fields for pci hotplug

[Nikunj A Dadhania]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

We need to set the proper drc_index values in ibm,my-drc-index
fields in order to allow a PCI device that was present at
boot-time to be unplugged.

Previously SLOF handles this, but with QEMU handling the DT we
need to do it there as well.

This patch slightly changes how SLOF handled it in the past,
which was to allows add an ibm,my-drc-index value based on
PCI slot/devices topology. Now we only add it when the slot
supports hotplug and has a DR connector, which is more inline
with PAPR.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 103284a..cbd5661 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -951,7 +951,9 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
         _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
                          strlen(drc_name)));
     }
-    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    if (drc_index) {
+        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    }
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
                           RESOURCE_CELLS_ADDRESS));
@@ -1483,6 +1485,20 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
+static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
+                                            PCIDevice *pdev)
+{
+    sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
+    sPAPRDRConnectorClass *drck;
+
+    if (!drc) {
+        return 0;
+    }
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    return drck->get_index(drc);
+}
+
 typedef struct sPAPRFDT {
     void *fdt;
     int node_off;
@@ -1499,6 +1515,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
     int func = PCI_FUNC(pdev->devfn);
     char nodename[512];
     sPAPRFDT s_fdt;
+    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, pdev);
 
     if (func) {
         sprintf(nodename, "pci@%d,%d", slot, func);
@@ -1506,7 +1523,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
-    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, drc_index, NULL);
     g_assert(!ret);
 
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm,loc-code

[Nikunj A Dadhania]

Each hardware instance has a platform unique location code.  The OF
device tree that describes a part of a hardware entity must include
the “ibm,loc-code” property with a value that represents the location
code for that hardware entity.

Populate ibm,loc-code.
1) PCI passthru devices need to identify with its own ibm,loc-code
   available on the host.
2) Emulated devices encode as following:
   qemu_<name>:<phb-index>:<slot>.<fn>

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 75 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cbd5661..eacf0bd 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &phb->iommu_as;
 }
 
+static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
+{
+    char *host;
+    char path[PATH_MAX];
+
+    host = object_property_get_str(OBJECT(pdev), "host", NULL);
+    if (!host) {
+        return false;
+    }
+
+    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
+    g_free(host);
+
+    return g_file_get_contents(path, value, NULL, NULL);
+}
+
+static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char path[PATH_MAX], *buf = NULL;
+
+    /* We have a vfio host bridge lets get the path. */
+    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
+        return NULL;
+    }
+
+    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
+    g_free(buf);
+
+    if (g_file_get_contents(path, &buf, NULL, NULL)) {
+        return buf;
+    } else {
+        return NULL;
+    }
+}
+
+static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char *path = g_malloc(PATH_MAX);
+
+    if (!path) {
+        return NULL;
+    }
+
+    /*
+     * For non-vfio devices and failures make up the location code out
+     * of the name, slot and function.
+     *
+     *       qemu_<name>:<phb-index>:<slot>.<fn>
+     */
+    snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
+             sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    return path;
+}
+
+
+static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
+{
+    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
+        return spapr_phb_vfio_get_loc_code(sphb, pdev);
+    } else {
+        return spapr_phb_get_loc_code(sphb, pdev);
+    }
+}
+
 /* Macros to operate with address in OF binding to PCI */
 #define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
 #define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
@@ -881,12 +945,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
 }
 
 static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
-                                       int phb_index, int drc_index,
-                                       const char *drc_name)
+                                       sPAPRPHBState *phb, int drc_index)
 {
     ResourceProps rp;
     bool is_bridge = false;
     int pci_status;
+    char *buf = NULL;
 
     if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
         PCI_HEADER_TYPE_BRIDGE) {
@@ -947,9 +1011,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
      * processed by OF beforehand
      */
     _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
-    if (drc_name) {
-        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
-                         strlen(drc_name)));
+    buf = spapr_ibm_get_loc_code(phb, dev);
+    if (buf) {
+        _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
+        g_free(buf);
     }
     if (drc_index) {
         _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
@@ -988,8 +1053,7 @@ static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(fdt, 0, nodename);
-    ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb->index, drc_index,
-                                      drc_name);
+    ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb, drc_index);
     g_assert(!ret);
 
     *dt_offset = offset;
@@ -1502,7 +1566,7 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
 typedef struct sPAPRFDT {
     void *fdt;
     int node_off;
-    uint32_t index;
+    sPAPRPHBState *sphb;
 } sPAPRFDT;
 
 static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
@@ -1523,7 +1587,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
-    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, drc_index, NULL);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb, drc_index);
     g_assert(!ret);
 
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
@@ -1538,7 +1602,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
 
     s_fdt.fdt = p->fdt;
     s_fdt.node_off = offset;
-    s_fdt.index = p->index;
+    s_fdt.sphb = p->sphb;
     pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
                         spapr_populate_pci_devices_dt,
                         &s_fdt);
@@ -1680,7 +1744,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     /* Populate tree nodes with PCI devices attached */
     s_fdt.fdt = fdt;
     s_fdt.node_off = bus_off;
-    s_fdt.index = phb->index;
+    s_fdt.sphb = phb;
     pci_for_each_device(bus, pci_bus_num(bus),
                         spapr_populate_pci_devices_dt,
                         &s_fdt);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU

[Nikunj A Dadhania]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
> The patch series creates PCI device tree(DT) nodes in QEMU. The new
> hotplug code needs the device node creation in QEMU. While during
> boot, nodes were created in SLOF. It makes more sense to consolidate
> the code to one place for better maintainability.
>
> Based on David's spapr-next 
> https://github.com/dgibson/qemu/tree/spapr-next
>
> Also, patches for populating ibm,loc-code was getting very complicated
> with use of RTAS/HCALL
>

Something went wrong in my git-send email, will repost correcting the
duplication of patches.


> Changelog V2:
>  * Fix device tree for 64-bit encoding
>  * Fix the class code, was failing xhci
>  * Remove macro duplication
>  * Fix DT fields generation for boot time device (Michael Roth)
>
> Changelog v1:
>  * Correct indent problems reported by checkpatch(David Gibson)
>  * Declare sPAPRFDT structure as local (David Gibson)
>  * Re-arrange code to avoid multiple indentation (Alexey Kardashevskiy)
>
> Michael Roth (1):
>   spapr_pci: fix boot-time device tree fields for pci hotplug
>
> Nikunj A Dadhania (5):
>   spapr_pci: remove duplicate macros
>   spapr_pci: encode missing 64-bit memory address space
>   spapr_pci: encode class code including Prog IF register
>   spapr_pci: enumerate and add PCI device tree
>   spapr_pci: populate ibm,loc-code
>
>  hw/ppc/spapr_pci.c | 221 +++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 198 insertions(+), 23 deletions(-)
>
> -- 
> 1.8.3.1

Regards,
Nikunj

Re: [Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Tue,  5 May 2015 14:23:54 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> All the PCI enumeration and device node creation was off-loaded to
>> SLOF. With PCI hotplug support, code needed to be added to add device
>> node. This creates multiple copy of the code one in SLOF and other in
>> hotplug code. To unify this, the patch adds the pci device node
>> creation in Qemu. For backward compatibility, a flag
>> "qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
>> do device node creation.
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 103 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 8b02a3e..103284a 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -23,6 +23,7 @@
>>   * THE SOFTWARE.
>>   */
>>  #include "hw/hw.h"
>> +#include "hw/sysbus.h"
>>  #include "hw/pci/pci.h"
>>  #include "hw/pci/msi.h"
>>  #include "hw/pci/msix.h"
>> @@ -35,6 +36,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qapi/qmp/qerror.h"
>>  
>> +#include "hw/pci/pci_bridge.h"
>>  #include "hw/pci/pci_bus.h"
>>  #include "hw/ppc/spapr_drc.h"
>>  #include "sysemu/device_tree.h"
>> @@ -945,7 +947,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>>       * processed by OF beforehand
>>       */
>>      _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
>> -    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
>> +    if (drc_name) {
>> +        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
>> +                         strlen(drc_name)));
>> +    }
>>      _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>>  
>>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
>> @@ -1001,10 +1006,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>>      void *fdt = NULL;
>>      int fdt_start_offset = 0;
>>  
>> -    /* boot-time devices get their device tree node created by SLOF, but for
>> -     * hotplugged devices we need QEMU to generate it so the guest can fetch
>> -     * it via RTAS
>> -     */
>>      if (dev->hotplugged) {
>>          fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
>>                                          &fdt_start_offset);
>> @@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>>      return PCI_HOST_BRIDGE(dev);
>>  }
>>  
>> +typedef struct sPAPRFDT {
>> +    void *fdt;
>> +    int node_off;
>> +    uint32_t index;
>> +} sPAPRFDT;
>> +
>> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>> +                                          void *opaque)
>> +{
>> +    PCIBus *sec_bus;
>> +    sPAPRFDT *p = opaque;
>> +    int ret, offset;
>> +    int slot = PCI_SLOT(pdev->devfn);
>> +    int func = PCI_FUNC(pdev->devfn);
>> +    char nodename[512];
>
> That's quite a big array ....
>
>> +    sPAPRFDT s_fdt;
>> +
>> +    if (func) {
>> +        sprintf(nodename, "pci@%d,%d", slot, func);
>> +    } else {
>> +        sprintf(nodename, "pci@%d", slot);
>> +    }
>
> ... just for holding such a small string. Could you maybe use
> a smaller array size for nodename (especially since you call this
> function recursively)?

Sure, will change this to 32 bytes, that should be sufficient. 

>
>> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
>
> The above code sequence looks pretty much similar to
> spapr_create_pci_child_dt() ... maybe it's worth the effort to merge
> the common code of both functions into a separate helper function
> to avoid the duplication? ... not sure if this is worth the effort,
> it's just a suggestion.

I had thought about it earlier but something was not matching. Let me
have a relook, things have changed.

>
>> +    g_assert(!ret);
>
> You know that assert statements can simply be disabled by a
> preprocessor switch - and in that case there is no more error handling
> here at all and the code continues silently with a partial initialized
> node!

Thanks for bringing this to notice, assert() ?

> So it's maybe better to do a proper error handling here instead?

Will change this error handling here.

>
>> +    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.index = p->index;
>> +    pci_for_each_device(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)
>> +{
>> +    unsigned short *bus_no = (unsigned short *) opaque;
>
> opaque is a void pointer, so I think you don't need the typecast here.

Ok, QEMU has strong type checking, so by default I typecast them every time.

>
>> +    unsigned short primary = *bus_no;
>> +    unsigned short secondary;
>> +    unsigned short subordinate = 0xff;
>
> Is there a special reason for using unsigned short variables here?

No special reason, actually unsigned char should do, as the max is bus
number can only be 255

> "unsigned int" would sound much more natural to me.
>
>> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
>> +         PCI_HEADER_TYPE_BRIDGE)) {
>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>> +        secondary = *bus_no + 1;
>> +        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
>> +        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
>> +        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
>> +        *bus_no = *bus_no + 1;
>> +        if (sec_bus) {
>> +            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
>> +            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
>> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
>
> You write all value here again ... I think you c\ould either drop the
> write to the PRIMARY and SECONDARY BUS registers, or you could use a
> proper if-else instead.

I will drop the PRIMARY and SECONDARY here, that would do.

>
>> +            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
>> +                                spapr_phb_pci_enumerate_bridge,
>> +                                bus_no);
>> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
>> +        }
>> +    }
>> +}
>> +
>> +static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
>> +{
>> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
>> +    unsigned short bus_no = 0;
>
> Again, why "short" ?
>
>> +    pci_for_each_device(bus, pci_bus_num(bus),
>> +                        spapr_phb_pci_enumerate_bridge,
>> +                        &bus_no);
>> +
>> +}
>> +
>>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>>                            uint32_t xics_phandle,
>>                            void *fdt)
>> @@ -1521,6 +1605,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>>          cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
>>      uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
>>      sPAPRTCETable *tcet;
>> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
>> +    sPAPRFDT s_fdt;
>>  
>>      /* Start populating the FDT */
>>      sprintf(nodename, "pci@%" PRIx64, phb->buid);
>> @@ -1570,6 +1656,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>>                   tcet->liobn, tcet->bus_offset,
>>                   tcet->nb_table << tcet->page_shift);
>>  
>> +    /* Walk the bridges and program the bus numbers*/
>> +    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.index = phb->index;
>> +    pci_for_each_device(bus, pci_bus_num(bus),
>> +                        spapr_populate_pci_devices_dt,
>> +                        &s_fdt);
>> +
>>      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
>>                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
>>      if (ret) {
>
>  Thomas

Regards
Nikunj

Re: [Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree

[Thomas Huth]

On Tue,  5 May 2015 14:23:54 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> All the PCI enumeration and device node creation was off-loaded to
> SLOF. With PCI hotplug support, code needed to be added to add device
> node. This creates multiple copy of the code one in SLOF and other in
> hotplug code. To unify this, the patch adds the pci device node
> creation in Qemu. For backward compatibility, a flag
> "qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
> do device node creation.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 103 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 8b02a3e..103284a 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -23,6 +23,7 @@
>   * THE SOFTWARE.
>   */
>  #include "hw/hw.h"
> +#include "hw/sysbus.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/msi.h"
>  #include "hw/pci/msix.h"
> @@ -35,6 +36,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/qmp/qerror.h"
>  
> +#include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/ppc/spapr_drc.h"
>  #include "sysemu/device_tree.h"
> @@ -945,7 +947,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>       * processed by OF beforehand
>       */
>      _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
> -    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
> +    if (drc_name) {
> +        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
> +                         strlen(drc_name)));
> +    }
>      _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>  
>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> @@ -1001,10 +1006,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>      void *fdt = NULL;
>      int fdt_start_offset = 0;
>  
> -    /* boot-time devices get their device tree node created by SLOF, but for
> -     * hotplugged devices we need QEMU to generate it so the guest can fetch
> -     * it via RTAS
> -     */
>      if (dev->hotplugged) {
>          fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
>                                          &fdt_start_offset);
> @@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> +typedef struct sPAPRFDT {
> +    void *fdt;
> +    int node_off;
> +    uint32_t index;
> +} sPAPRFDT;
> +
> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> +                                          void *opaque)
> +{
> +    PCIBus *sec_bus;
> +    sPAPRFDT *p = opaque;
> +    int ret, offset;
> +    int slot = PCI_SLOT(pdev->devfn);
> +    int func = PCI_FUNC(pdev->devfn);
> +    char nodename[512];

That's quite a big array ....

> +    sPAPRFDT s_fdt;
> +
> +    if (func) {
> +        sprintf(nodename, "pci@%d,%d", slot, func);
> +    } else {
> +        sprintf(nodename, "pci@%d", slot);
> +    }

... just for holding such a small string. Could you maybe use
a smaller array size for nodename (especially since you call this
function recursively)?

> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);

The above code sequence looks pretty much similar to
spapr_create_pci_child_dt() ... maybe it's worth the effort to merge
the common code of both functions into a separate helper function
to avoid the duplication? ... not sure if this is worth the effort,
it's just a suggestion.

> +    g_assert(!ret);

You know that assert statements can simply be disabled by a
preprocessor switch - and in that case there is no more error handling
here at all and the code continues silently with a partial initialized
node! So it's maybe better to do a proper error handling here instead?

> +    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.index = p->index;
> +    pci_for_each_device(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)
> +{
> +    unsigned short *bus_no = (unsigned short *) opaque;

opaque is a void pointer, so I think you don't need the typecast here.

> +    unsigned short primary = *bus_no;
> +    unsigned short secondary;
> +    unsigned short subordinate = 0xff;

Is there a special reason for using unsigned short variables here?
"unsigned int" would sound much more natural to me.

> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
> +         PCI_HEADER_TYPE_BRIDGE)) {
> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> +        secondary = *bus_no + 1;
> +        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
> +        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
> +        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
> +        *bus_no = *bus_no + 1;
> +        if (sec_bus) {
> +            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
> +            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);

You write all value here again ... I think you could either drop the
write to the PRIMARY and SECONDARY BUS registers, or you could use a
proper if-else instead.

> +            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> +                                spapr_phb_pci_enumerate_bridge,
> +                                bus_no);
> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
> +        }
> +    }
> +}
> +
> +static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
> +{
> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> +    unsigned short bus_no = 0;

Again, why "short" ?

> +    pci_for_each_device(bus, pci_bus_num(bus),
> +                        spapr_phb_pci_enumerate_bridge,
> +                        &bus_no);
> +
> +}
> +
>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                            uint32_t xics_phandle,
>                            void *fdt)
> @@ -1521,6 +1605,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>          cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
>      uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
>      sPAPRTCETable *tcet;
> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> +    sPAPRFDT s_fdt;
>  
>      /* Start populating the FDT */
>      sprintf(nodename, "pci@%" PRIx64, phb->buid);
> @@ -1570,6 +1656,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                   tcet->liobn, tcet->bus_offset,
>                   tcet->nb_table << tcet->page_shift);
>  
> +    /* Walk the bridges and program the bus numbers*/
> +    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.index = phb->index;
> +    pci_for_each_device(bus, pci_bus_num(bus),
> +                        spapr_populate_pci_devices_dt,
> +                        &s_fdt);
> +
>      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
>                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
>      if (ret) {

 Thomas

[Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree

[Nikunj A Dadhania]

All the PCI enumeration and device node creation was off-loaded to
SLOF. With PCI hotplug support, code needed to be added to add device
node. This creates multiple copy of the code one in SLOF and other in
hotplug code. To unify this, the patch adds the pci device node
creation in Qemu. For backward compatibility, a flag
"qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
do device node creation.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 103 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 8b02a3e..103284a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -23,6 +23,7 @@
  * THE SOFTWARE.
  */
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
@@ -35,6 +36,7 @@
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
 
+#include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/ppc/spapr_drc.h"
 #include "sysemu/device_tree.h"
@@ -945,7 +947,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
      * processed by OF beforehand
      */
     _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
-    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
+    if (drc_name) {
+        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
+                         strlen(drc_name)));
+    }
     _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
@@ -1001,10 +1006,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
     void *fdt = NULL;
     int fdt_start_offset = 0;
 
-    /* boot-time devices get their device tree node created by SLOF, but for
-     * hotplugged devices we need QEMU to generate it so the guest can fetch
-     * it via RTAS
-     */
     if (dev->hotplugged) {
         fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
                                         &fdt_start_offset);
@@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
+typedef struct sPAPRFDT {
+    void *fdt;
+    int node_off;
+    uint32_t index;
+} sPAPRFDT;
+
+static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
+                                          void *opaque)
+{
+    PCIBus *sec_bus;
+    sPAPRFDT *p = opaque;
+    int ret, offset;
+    int slot = PCI_SLOT(pdev->devfn);
+    int func = PCI_FUNC(pdev->devfn);
+    char nodename[512];
+    sPAPRFDT s_fdt;
+
+    if (func) {
+        sprintf(nodename, "pci@%d,%d", slot, func);
+    } else {
+        sprintf(nodename, "pci@%d", slot);
+    }
+    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    g_assert(!ret);
+
+    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.index = p->index;
+    pci_for_each_device(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)
+{
+    unsigned short *bus_no = (unsigned short *) opaque;
+    unsigned short primary = *bus_no;
+    unsigned short secondary;
+    unsigned short subordinate = 0xff;
+
+    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
+         PCI_HEADER_TYPE_BRIDGE)) {
+        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
+        secondary = *bus_no + 1;
+        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
+        *bus_no = *bus_no + 1;
+        if (sec_bus) {
+            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
+            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                                spapr_phb_pci_enumerate_bridge,
+                                bus_no);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
+        }
+    }
+}
+
+static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
+{
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    unsigned short bus_no = 0;
+
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_phb_pci_enumerate_bridge,
+                        &bus_no);
+
+}
+
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
                           void *fdt)
@@ -1521,6 +1605,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
         cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
     uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
     sPAPRTCETable *tcet;
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    sPAPRFDT s_fdt;
 
     /* Start populating the FDT */
     sprintf(nodename, "pci@%" PRIx64, phb->buid);
@@ -1570,6 +1656,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
                  tcet->liobn, tcet->bus_offset,
                  tcet->nb_table << tcet->page_shift);
 
+    /* Walk the bridges and program the bus numbers*/
+    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.index = phb->index;
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_populate_pci_devices_dt,
+                        &s_fdt);
+
     ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
                                 SPAPR_DR_CONNECTOR_TYPE_PCI);
     if (ret) {
-- 
1.8.3.1