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

[Qemu-devel] [PATCH v7 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

New slof.bin is already there in spapr-next

Changelog V6:
* Open coded fdt_setprop_string as the return would 
  leak memory (Thomas Huth)

Changelog V5:
* Reworked enumerate bridges to reduce scopes (Alexey)
* Fixed unit address which should be hex
* Added FDT_NAME_MAX and use snprintf for composing name (Alexey)
* Added patch to drop redundant args and change prototype (Alexey)
* Added bus number in the ibm,loc-code for qemu emulated or failed
  vfio device. 
* Now qemu_<name>:<phb-index>:<bus>:<slot>.<fn> is having
  hex encoding (Alexey)

Changelog V4:
* Refactored and simplified the "ibm,loc-code" patch

Changelog V3:
* Dropped duplicate macro patches
* Squashed Michael's drc_index changes to enumeration patch
* Use common create routine for boottime and hotplug case
* Proper error handling not depending on g_assert
* Encode vfio loc-code if getting loc-code from host fails

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)

Nikunj A Dadhania (6):
  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: set device node unit address as hex
  spapr_pci: populate ibm,loc-code
  spapr_pci: drop redundant args in spapr_populate_pci_child_dt

 hw/ppc/spapr_pci.c | 261 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 223 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v7 1/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>
Reviewed-by: Thomas Huth <thuth@redhat.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 75bab03..3ebbcd5 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 v7 2/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>
Reviewed-by: Thomas Huth <thuth@redhat.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 3ebbcd5..33254b3 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 v7 3/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>
[ Squashed Michael's drc_index changes ]
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 | 167 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 142 insertions(+), 25 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 33254b3..6ef7f44 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,8 +948,13 @@ 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)));
-    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    if (drc_name) {
+        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
+                         strlen(drc_name)));
+    }
+    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));
@@ -964,30 +971,38 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
     return 0;
 }
 
+typedef struct sPAPRFDT {
+    void *fdt;
+    int node_off;
+    sPAPRPHBState *sphb;
+} sPAPRFDT;
+
+static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
+                                            PCIDevice *pdev);
+
 /* create OF node for pci device and required OF DT properties */
-static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
-                                       int drc_index, const char *drc_name,
-                                       int *dt_offset)
+static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p,
+                                     const char *drc_name)
 {
-    void *fdt;
-    int offset, ret, fdt_size;
+    int offset, ret;
     int slot = PCI_SLOT(dev->devfn);
     int func = PCI_FUNC(dev->devfn);
     char nodename[512];
+    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, dev);
 
-    fdt = create_device_tree(&fdt_size);
     if (func != 0) {
         sprintf(nodename, "pci@%d,%d", slot, func);
     } else {
         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);
+    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
+    ret = spapr_populate_pci_child_dt(dev, p->fdt, offset, p->sphb->index,
+                                      drc_index, drc_name);
     g_assert(!ret);
-
-    *dt_offset = offset;
-    return fdt;
+    if (ret) {
+        return 0;
+    }
+    return offset;
 }
 
 static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
@@ -997,24 +1012,26 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
 {
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     DeviceState *dev = DEVICE(pdev);
-    int drc_index = drck->get_index(drc);
     const char *drc_name = drck->get_name(drc);
-    void *fdt = NULL;
-    int fdt_start_offset = 0;
+    int fdt_start_offset = 0, fdt_size;
+    sPAPRFDT s_fdt = {NULL, 0, NULL};
 
-    /* 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);
+        s_fdt.fdt = create_device_tree(&fdt_size);
+        s_fdt.sphb = phb;
+        s_fdt.node_off = 0;
+        fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name);
+        if (!fdt_start_offset) {
+            error_setg(errp, "Failed to create pci child device tree node");
+            goto out;
+        }
     }
 
     drck->attach(drc, DEVICE(pdev),
-                 fdt, fdt_start_offset, !dev->hotplugged, errp);
+                 s_fdt.fdt, fdt_start_offset, !dev->hotplugged, errp);
+out:
     if (*errp) {
-        g_free(fdt);
+        g_free(s_fdt.fdt);
     }
 }
 
@@ -1054,6 +1071,20 @@ static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
                                     pdev->devfn);
 }
 
+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);
+}
+
 static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
                                      DeviceState *plugged_dev, Error **errp)
 {
@@ -1484,6 +1515,78 @@ PCIHostState *spapr_create_phb(sPAPRMachineState *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
+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_create_pci_child_dt(pdev, p, NULL);
+    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(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 int *bus_no = opaque;
+    unsigned int primary = *bus_no;
+    unsigned int subordinate = 0xff;
+    PCIBus *sec_bus = NULL;
+
+    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
+         PCI_HEADER_TYPE_BRIDGE)) {
+        return;
+    }
+
+    (*bus_no)++;
+    pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+    pci_default_write_config(pdev, PCI_SECONDARY_BUS, *bus_no, 1);
+    pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
+
+    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
+    if (!sec_bus) {
+        return;
+    }
+
+    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 int 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)
@@ -1523,6 +1626,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);
@@ -1572,6 +1677,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.sphb = phb;
+    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 v7 4/6] spapr_pci: set device node unit address as hex

[Nikunj A Dadhania]

Device node names should encode the unit address as hex, while the
code was encodind it as integers.

Also, use FDT_NAME_MAX macro for allocating and composing the name.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 6ef7f44..a289ae9 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -52,6 +52,8 @@
 #define RTAS_TYPE_MSI           1
 #define RTAS_TYPE_MSIX          2
 
+#define FDT_NAME_MAX          128
+
 #define _FDT(exp) \
     do { \
         int ret = (exp);                                           \
@@ -987,13 +989,13 @@ static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p,
     int offset, ret;
     int slot = PCI_SLOT(dev->devfn);
     int func = PCI_FUNC(dev->devfn);
-    char nodename[512];
+    char nodename[FDT_NAME_MAX];
     uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, dev);
 
     if (func != 0) {
-        sprintf(nodename, "pci@%d,%d", slot, func);
+        snprintf(nodename, FDT_NAME_MAX, "pci@%x,%x", slot, func);
     } else {
-        sprintf(nodename, "pci@%d", slot);
+        snprintf(nodename, FDT_NAME_MAX, "pci@%x", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
     ret = spapr_populate_pci_child_dt(dev, p->fdt, offset, p->sphb->index,
@@ -1592,7 +1594,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           void *fdt)
 {
     int bus_off, i, j, ret;
-    char nodename[256];
+    char nodename[FDT_NAME_MAX];
     uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
     const uint64_t mmiosize = memory_region_size(&phb->memwindow);
     const uint64_t w32max = (1ULL << 32) - SPAPR_PCI_MEM_WIN_BUS_OFFSET;
@@ -1630,7 +1632,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     sPAPRFDT s_fdt;
 
     /* Start populating the FDT */
-    sprintf(nodename, "pci@%" PRIx64, phb->buid);
+    snprintf(nodename, FDT_NAME_MAX, "pci@%" PRIx64, phb->buid);
     bus_off = fdt_add_subnode(fdt, 0, nodename);
     if (bus_off < 0) {
         return bus_off;
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 5/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. In failure cases use:
   vfio_<name>:<phb-index>:<bus>:<slot>.<fn>

2) Emulated devices encode as following:
   qemu_<name>:<phb-index>:<bus>:<slot>.<fn>

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

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index a289ae9..a7c17be 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -747,6 +747,60 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &phb->iommu_as;
 }
 
+static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char *path = NULL, *buf = NULL, *host = NULL;
+
+    /* Get the PCI VFIO host id */
+    host = object_property_get_str(OBJECT(pdev), "host", NULL);
+    if (!host) {
+        goto err_out;
+    }
+
+    /* Construct the path of the file that will give us the DT location */
+    path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
+    g_free(host);
+    if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) {
+        goto err_out;
+    }
+    g_free(path);
+
+    /* Construct and read from host device tree the loc-code */
+    path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
+    g_free(buf);
+    if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) {
+        goto err_out;
+    }
+    return buf;
+
+err_out:
+    g_free(path);
+    return NULL;
+}
+
+static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
+{
+    char *buf;
+    const char *devtype = "qemu";
+    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
+
+    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
+        buf = spapr_phb_vfio_get_loc_code(sphb, pdev);
+        if (buf) {
+            return buf;
+        }
+        devtype = "vfio";
+    }
+    /*
+     * For emulated devices and VFIO-failure case, make up
+     * the loc-code.
+     */
+    buf = g_strdup_printf("%s_%s:%04x:%02x:%02x.%x",
+                          devtype, pdev->name, sphb->index, busnr,
+                          PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    return buf;
+}
+
 /* 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 */
@@ -885,11 +939,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 *sphb)
 {
     ResourceProps rp;
     bool is_bridge = false;
-    int pci_status;
+    int pci_status, err;
+    char *buf = NULL;
 
     if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
         PCI_HEADER_TYPE_BRIDGE) {
@@ -950,10 +1005,18 @@ 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_phb_get_loc_code(sphb, dev);
+    if (!buf) {
+        error_report("Failed setting the ibm,loc-code");
+        return -1;
     }
+
+    err = fdt_setprop_string(fdt, offset, "ibm,loc-code", buf);
+    g_free(buf);
+    if (err < 0) {
+        return err;
+    }
+
     if (drc_index) {
         _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
     }
@@ -983,8 +1046,7 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
                                             PCIDevice *pdev);
 
 /* create OF node for pci device and required OF DT properties */
-static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p,
-                                     const char *drc_name)
+static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p)
 {
     int offset, ret;
     int slot = PCI_SLOT(dev->devfn);
@@ -999,7 +1061,7 @@ static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p,
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
     ret = spapr_populate_pci_child_dt(dev, p->fdt, offset, p->sphb->index,
-                                      drc_index, drc_name);
+                                      drc_index, p->sphb);
     g_assert(!ret);
     if (ret) {
         return 0;
@@ -1014,7 +1076,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
 {
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     DeviceState *dev = DEVICE(pdev);
-    const char *drc_name = drck->get_name(drc);
     int fdt_start_offset = 0, fdt_size;
     sPAPRFDT s_fdt = {NULL, 0, NULL};
 
@@ -1022,7 +1083,7 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
         s_fdt.fdt = create_device_tree(&fdt_size);
         s_fdt.sphb = phb;
         s_fdt.node_off = 0;
-        fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name);
+        fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt);
         if (!fdt_start_offset) {
             error_setg(errp, "Failed to create pci child device tree node");
             goto out;
@@ -1525,7 +1586,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
     int offset;
     sPAPRFDT s_fdt;
 
-    offset = spapr_create_pci_child_dt(pdev, p, NULL);
+    offset = spapr_create_pci_child_dt(pdev, p);
     if (!offset) {
         error_report("Failed to create pci child device tree node");
         return;
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 6/6] spapr_pci: drop redundant args in spapr_populate_pci_child_dt

[Nikunj A Dadhania]

* phb_index is not being used and if required can be obtained from sphb
* use helper to get drc_index in this function

Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index a7c17be..0594f38 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -937,14 +937,17 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
     rp->assigned_len = assigned_idx * sizeof(ResourceFields);
 }
 
+static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
+                                            PCIDevice *pdev);
+
 static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
-                                       int phb_index, int drc_index,
                                        sPAPRPHBState *sphb)
 {
     ResourceProps rp;
     bool is_bridge = false;
     int pci_status, err;
     char *buf = NULL;
+    uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
 
     if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
         PCI_HEADER_TYPE_BRIDGE) {
@@ -1042,9 +1045,6 @@ typedef struct sPAPRFDT {
     sPAPRPHBState *sphb;
 } sPAPRFDT;
 
-static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
-                                            PCIDevice *pdev);
-
 /* create OF node for pci device and required OF DT properties */
 static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p)
 {
@@ -1052,7 +1052,6 @@ static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p)
     int slot = PCI_SLOT(dev->devfn);
     int func = PCI_FUNC(dev->devfn);
     char nodename[FDT_NAME_MAX];
-    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, dev);
 
     if (func != 0) {
         snprintf(nodename, FDT_NAME_MAX, "pci@%x,%x", slot, func);
@@ -1060,8 +1059,7 @@ static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p)
         snprintf(nodename, FDT_NAME_MAX, "pci@%x", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
-    ret = spapr_populate_pci_child_dt(dev, p->fdt, offset, p->sphb->index,
-                                      drc_index, p->sphb);
+    ret = spapr_populate_pci_child_dt(dev, p->fdt, offset, p->sphb);
     g_assert(!ret);
     if (ret) {
         return 0;
-- 
1.8.3.1

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10643 bytes --]

On Thu, Jun 11, 2015 at 04:32:26PM +0530, Nikunj A Dadhania 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>
> [ Squashed Michael's drc_index changes ]
> 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 | 167 +++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 142 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 33254b3..6ef7f44 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,8 +948,13 @@ 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)));
> -    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> +    if (drc_name) {
> +        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
> +                         strlen(drc_name)));
> +    }
> +    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));
> @@ -964,30 +971,38 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>      return 0;
>  }
>  
> +typedef struct sPAPRFDT {
> +    void *fdt;
> +    int node_off;
> +    sPAPRPHBState *sphb;
> +} sPAPRFDT;

I don't really like this structure - it seems a very ad-hoc collection
of things.  Even though it means there will be a lot of parameters to
the function, I'd prefer passing them separately to
spapr_create_pci_child_dt() rather than using this structure.

> +static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
> +                                            PCIDevice *pdev);
> +
>  /* create OF node for pci device and required OF DT properties */
> -static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
> -                                       int drc_index, const char *drc_name,
> -                                       int *dt_offset)
> +static int spapr_create_pci_child_dt(PCIDevice *dev, sPAPRFDT *p,
> +                                     const char *drc_name)
>  {
> -    void *fdt;
> -    int offset, ret, fdt_size;
> +    int offset, ret;
>      int slot = PCI_SLOT(dev->devfn);
>      int func = PCI_FUNC(dev->devfn);
>      char nodename[512];
> +    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, dev);
>  
> -    fdt = create_device_tree(&fdt_size);
>      if (func != 0) {
>          sprintf(nodename, "pci@%d,%d", slot, func);
>      } else {
>          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);
> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
> +    ret = spapr_populate_pci_child_dt(dev, p->fdt, offset, p->sphb->index,
> +                                      drc_index, drc_name);
>      g_assert(!ret);
> -
> -    *dt_offset = offset;
> -    return fdt;
> +    if (ret) {
> +        return 0;
> +    }
> +    return offset;
>  }
>  
>  static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> @@ -997,24 +1012,26 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>  {
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      DeviceState *dev = DEVICE(pdev);
> -    int drc_index = drck->get_index(drc);
>      const char *drc_name = drck->get_name(drc);
> -    void *fdt = NULL;
> -    int fdt_start_offset = 0;
> +    int fdt_start_offset = 0, fdt_size;
> +    sPAPRFDT s_fdt = {NULL, 0, NULL};
>  
> -    /* 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);
> +        s_fdt.fdt = create_device_tree(&fdt_size);
> +        s_fdt.sphb = phb;
> +        s_fdt.node_off = 0;
> +        fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name);
> +        if (!fdt_start_offset) {
> +            error_setg(errp, "Failed to create pci child device tree node");
> +            goto out;
> +        }
>      }
>  
>      drck->attach(drc, DEVICE(pdev),
> -                 fdt, fdt_start_offset, !dev->hotplugged, errp);
> +                 s_fdt.fdt, fdt_start_offset, !dev->hotplugged, errp);
> +out:
>      if (*errp) {
> -        g_free(fdt);
> +        g_free(s_fdt.fdt);
>      }
>  }
>  
> @@ -1054,6 +1071,20 @@ static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
>                                      pdev->devfn);
>  }
>  
> +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);
> +}
> +
>  static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>                                       DeviceState *plugged_dev, Error **errp)
>  {
> @@ -1484,6 +1515,78 @@ PCIHostState *spapr_create_phb(sPAPRMachineState *spapr, int index)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> +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_create_pci_child_dt(pdev, p, NULL);
> +    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(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 int *bus_no = opaque;
> +    unsigned int primary = *bus_no;
> +    unsigned int subordinate = 0xff;
> +    PCIBus *sec_bus = NULL;
> +
> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
> +         PCI_HEADER_TYPE_BRIDGE)) {
> +        return;
> +    }
> +
> +    (*bus_no)++;
> +    pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
> +    pci_default_write_config(pdev, PCI_SECONDARY_BUS, *bus_no, 1);
> +    pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
> +
> +    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> +    if (!sec_bus) {
> +        return;
> +    }
> +
> +    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 int 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)
> @@ -1523,6 +1626,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);
> @@ -1572,6 +1677,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.sphb = phb;
> +    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) {

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v7 1/6] spapr_pci: encode missing 64-bit memory address space

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

On Thu, Jun 11, 2015 at 04:32:24PM +0530, Nikunj A Dadhania wrote:
> 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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v7 2/6] spapr_pci: encode class code including Prog IF register

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Thu, Jun 11, 2015 at 04:32:25PM +0530, Nikunj A Dadhania wrote:
> 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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v7 4/6] spapr_pci: set device node unit address as hex

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Thu, Jun 11, 2015 at 04:32:27PM +0530, Nikunj A Dadhania wrote:
> Device node names should encode the unit address as hex, while the
> code was encodind it as integers.
> 
> Also, use FDT_NAME_MAX macro for allocating and composing the name.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Nikunj A Dadhania]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Thu, Jun 11, 2015 at 04:32:26PM +0530, Nikunj A Dadhania 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>
>> [ Squashed Michael's drc_index changes ]
>> 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 | 167 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 142 insertions(+), 25 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 33254b3..6ef7f44 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,8 +948,13 @@ 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)));
>> -    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>> +    if (drc_name) {
>> +        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
>> +                         strlen(drc_name)));
>> +    }
>> +    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));
>> @@ -964,30 +971,38 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>>      return 0;
>>  }
>>  
>> +typedef struct sPAPRFDT {
>> +    void *fdt;
>> +    int node_off;
>> +    sPAPRPHBState *sphb;
>> +} sPAPRFDT;
>
> I don't really like this structure - it seems a very ad-hoc collection
> of things.  Even though it means there will be a lot of parameters to
> the function, I'd prefer passing them separately to
> spapr_create_pci_child_dt() rather than using this structure.

I added this structure with pci_for_each_device() in mind, which has
following prototype.

    void pci_for_each_device(PCIBus *bus, int bus_num,
                             void (*fn)(PCIBus *bus, PCIDevice *d, void *opaque),
                             void *opaque);

So per device we get this structure and populate PCI device tree entry
and scan and populate bridge recursively if needed. So I had continued
using this structure in spapr_create_pci_child_dt().

We cannot remove sPAPRFDT completely as we need it for PCI device tree
creation.

So if needed, I can change spapr_create_pci_child_dt() with more args.
And structure sPAPRFDT to be used by spapr_populate_pci_devices_dt()
called by pci_for_each_device().

>> @@ -997,24 +1012,26 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>>  {
>>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>>      DeviceState *dev = DEVICE(pdev);
>> -    int drc_index = drck->get_index(drc);
>>      const char *drc_name = drck->get_name(drc);
>> -    void *fdt = NULL;
>> -    int fdt_start_offset = 0;
>> +    int fdt_start_offset = 0, fdt_size;
>> +    sPAPRFDT s_fdt = {NULL, 0, NULL};
>>  
>> -    /* 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);
>> +        s_fdt.fdt = create_device_tree(&fdt_size);
>> +        s_fdt.sphb = phb;
>> +        s_fdt.node_off = 0;
>> +        fdt_start_offset = spapr_create_pci_child_dt(pdev, &s_fdt, drc_name);
>> +        if (!fdt_start_offset) {
>> +            error_setg(errp, "Failed to create pci child device tree node");
>> +            goto out;
>> +        }
>>      }
>>  
>>      drck->attach(drc, DEVICE(pdev),
>> -                 fdt, fdt_start_offset, !dev->hotplugged, errp);
>> +                 s_fdt.fdt, fdt_start_offset, !dev->hotplugged, errp);
>> +out:
>>      if (*errp) {
>> -        g_free(fdt);
>> +        g_free(s_fdt.fdt);
>>      }
>>  }

[SNIP]

>> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>> +                                          void *opaque)
>> +{
>> +    PCIBus *sec_bus;
>> +    sPAPRFDT *p = opaque;

[ SNIP ]

>> +    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(sec_bus, pci_bus_num(sec_bus),
>> +                        spapr_populate_pci_devices_dt,
>> +                        &s_fdt);
>> +}

[ SNIP ]

>> @@ -1523,6 +1626,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);
>> @@ -1572,6 +1677,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.sphb = phb;
>> +    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) {
>

Regards,
Nikunj

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Thu, Jun 11, 2015 at 04:32:23PM +0530, Nikunj A Dadhania wrote:
> 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
> 
> New slof.bin is already there in spapr-next

I've applied the bug fixes (1/6, 2/6 and 4/6) to spapr-next.  The
reset I'd prefer to see respun.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]

On Wed, Jun 17, 2015 at 02:12:14PM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Thu, Jun 11, 2015 at 04:32:26PM +0530, Nikunj A Dadhania 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>
> >> [ Squashed Michael's drc_index changes ]
> >> 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 | 167 +++++++++++++++++++++++++++++++++++++++++++++--------
> >>  1 file changed, 142 insertions(+), 25 deletions(-)
> >> 
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index 33254b3..6ef7f44 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,8 +948,13 @@ 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)));
> >> -    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> >> +    if (drc_name) {
> >> +        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
> >> +                         strlen(drc_name)));
> >> +    }
> >> +    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));
> >> @@ -964,30 +971,38 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >>      return 0;
> >>  }
> >>  
> >> +typedef struct sPAPRFDT {
> >> +    void *fdt;
> >> +    int node_off;
> >> +    sPAPRPHBState *sphb;
> >> +} sPAPRFDT;
> >
> > I don't really like this structure - it seems a very ad-hoc collection
> > of things.  Even though it means there will be a lot of parameters to
> > the function, I'd prefer passing them separately to
> > spapr_create_pci_child_dt() rather than using this structure.
> 
> I added this structure with pci_for_each_device() in mind, which has
> following prototype.
> 
>     void pci_for_each_device(PCIBus *bus, int bus_num,
>                              void (*fn)(PCIBus *bus, PCIDevice *d, void *opaque),
>                              void *opaque);
> 
> So per device we get this structure and populate PCI device tree entry
> and scan and populate bridge recursively if needed. So I had continued
> using this structure in spapr_create_pci_child_dt().
> 
> We cannot remove sPAPRFDT completely as we need it for PCI device tree
> creation.

Ah, yes, I see.

> So if needed, I can change spapr_create_pci_child_dt() with more args.
> And structure sPAPRFDT to be used by spapr_populate_pci_devices_dt()
> called by pci_for_each_device().

Ok, I'd still prefer to see this structure localized to just the
callback function.  Which  see you've done in the next spin, thanks.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]