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

[Qemu-devel] [PATCH v5 0/4] 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

Needs new slof.bin:
http://patchwork.ozlabs.org/patch/469303/

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

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 (4):
  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 | 266 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 221 insertions(+), 45 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 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>
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 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 v5 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>
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 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 v5 3/4] 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 | 188 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 150 insertions(+), 38 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 8b02a3e..12f1b9c 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"
@@ -742,6 +744,31 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &phb->iommu_as;
 }
 
+
+static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
+                                               PCIDevice *pdev)
+{
+    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
+    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
+                                    (phb->index << 16) |
+                                    (busnr << 8) |
+                                    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);
+}
+
 /* 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 */
@@ -879,12 +906,13 @@ 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,
+                                       sPAPRPHBState *sphb,
                                        const char *drc_name)
 {
     ResourceProps rp;
     bool is_bridge = false;
     int pci_status;
+    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) {
@@ -945,8 +973,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));
@@ -963,30 +996,34 @@ 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;
+
 /* 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 *pdev, sPAPRFDT *p,
+                                     const char *drc_name)
 {
-    void *fdt;
-    int offset, ret, fdt_size;
-    int slot = PCI_SLOT(dev->devfn);
-    int func = PCI_FUNC(dev->devfn);
-    char nodename[512];
+    int offset, ret;
+    char nodename[64];
+    int slot = PCI_SLOT(pdev->devfn);
+    int func = PCI_FUNC(pdev->devfn);
 
-    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,
+    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb,
                                       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,
@@ -996,24 +1033,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);
     }
 }
 
@@ -1043,16 +1082,6 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
     drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
 }
 
-static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
-                                               PCIDevice *pdev)
-{
-    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
-    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
-                                    (phb->index << 16) |
-                                    (busnr << 8) |
-                                    pdev->devfn);
-}
-
 static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
                                      DeviceState *plugged_dev, Error **errp)
 {
@@ -1482,6 +1511,75 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *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 secondary;
+    unsigned int 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_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)
@@ -1521,6 +1619,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 +1670,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

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

[Alexey Kardashevskiy]

On 05/19/2015 06:26 PM, 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 | 188 ++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 150 insertions(+), 38 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 8b02a3e..12f1b9c 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"
> @@ -742,6 +744,31 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>       return &phb->iommu_as;
>   }
>
> +
> +static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
> +                                               PCIDevice *pdev)
> +{
> +    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
> +    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
> +                                    (phb->index << 16) |
> +                                    (busnr << 8) |
> +                                    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);
> +}
> +
>   /* 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 */
> @@ -879,12 +906,13 @@ 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,
> +                                       sPAPRPHBState *sphb,
>                                          const char *drc_name)
>   {
>       ResourceProps rp;
>       bool is_bridge = false;
>       int pci_status;
> +    uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);

Is this drc_index any different from the one which used to be passed to 
this function? If no, then I do not see the point in changing the prototype 
(or make another "this just makes code easier/nicer" patch). If yes, then 
it would be nice to see what the patch changed in this regard in the commit 
log.



>       if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
>           PCI_HEADER_TYPE_BRIDGE) {
> @@ -945,8 +973,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));
> @@ -963,30 +996,34 @@ 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;
> +
>   /* 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 *pdev, sPAPRFDT *p,
> +                                     const char *drc_name)

Why s/dev/pdev/?


>   {
> -    void *fdt;
> -    int offset, ret, fdt_size;
> -    int slot = PCI_SLOT(dev->devfn);
> -    int func = PCI_FUNC(dev->devfn);
> -    char nodename[512];
> +    int offset, ret;
> +    char nodename[64];

Why s/512/64/?

This change and the one above hide what the patch really does to 
spapr_create_pci_child_dt.


> +    int slot = PCI_SLOT(pdev->devfn);
> +    int func = PCI_FUNC(pdev->devfn);
>
> -    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,
> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb,
>                                         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,
> @@ -996,24 +1033,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) {


I understand the patch is not changing this but still while we are here -
spapr_phb_add_pci_device() is only called from spapr_phb_hot_plug_child(), 
how can dev->hotplugged be not true in this function (if it cannot, you 
could get rid of "out:"?



> -        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);
>       }
>   }
>
> @@ -1043,16 +1082,6 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
>       drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
>   }
>
> -static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
> -                                               PCIDevice *pdev)

Just adding forward declaration would make the patch shorter.


> -{
> -    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
> -    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
> -                                    (phb->index << 16) |
> -                                    (busnr << 8) |
> -                                    pdev->devfn);
> -}
> -
>   static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>                                        DeviceState *plugged_dev, Error **errp)
>   {
> @@ -1482,6 +1511,75 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *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 secondary;
> +    unsigned int subordinate = 0xff;
> +
> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
> +         PCI_HEADER_TYPE_BRIDGE)) {


s/==/!=/ and "return" and no need in extra indent below.


> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> +        secondary = *bus_no + 1;


(*bus_no)++;
secondary = *bus_no;

and remove "bus_no = *bus_no + 1" below?
In fact, I do not need much sense in having "secondary" variable in this 
function.

> +        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) {

same here? Just like you did in spapr_populate_pci_devices_dt(). I do not 
insist though. But having less scopes just makes it easier/nicer to wrap 
long lines in QEMU coding style (new line starts under "(").


> +            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)
> @@ -1521,6 +1619,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 +1670,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));


Can we also add a hack here to scan for the "qemu,phb-enumerated" string in 
the SLOF bin image?


> +
> +    /* 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) {
>


-- 
Alexey

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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 05/19/2015 06:26 PM, 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 | 188 ++++++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 150 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 8b02a3e..12f1b9c 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"
>> @@ -742,6 +744,31 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>       return &phb->iommu_as;
>>   }
>>
>> +
>> +static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
>> +                                               PCIDevice *pdev)
>> +{
>> +    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
>> +    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
>> +                                    (phb->index << 16) |
>> +                                    (busnr << 8) |
>> +                                    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);
>> +}
>> +
>>   /* 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 */
>> @@ -879,12 +906,13 @@ 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,
>> +                                       sPAPRPHBState *sphb,
>>                                          const char *drc_name)
>>   {
>>       ResourceProps rp;
>>       bool is_bridge = false;
>>       int pci_status;
>> +    uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
>
> Is this drc_index any different from the one which used to be passed to 
> this function? If no, then I do not see the point in changing the prototype 
> (or make another "this just makes code easier/nicer" patch).

Its the same, I can have a separate patch. As I was changing this code
the drc_index would need to be read in boot and hotplug code. So brought
over the code here.

> If yes, then it would be nice to see what the patch changed in this
> regard in the commit log.
>
>
>
>>       if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
>>           PCI_HEADER_TYPE_BRIDGE) {
>> @@ -945,8 +973,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));
>> @@ -963,30 +996,34 @@ 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;
>> +
>>   /* 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 *pdev, sPAPRFDT *p,
>> +                                     const char *drc_name)
>
> Why s/dev/pdev/?

PCIDev thats the only reason.

>
>
>>   {
>> -    void *fdt;
>> -    int offset, ret, fdt_size;
>> -    int slot = PCI_SLOT(dev->devfn);
>> -    int func = PCI_FUNC(dev->devfn);
>> -    char nodename[512];
>> +    int offset, ret;
>> +    char nodename[64];
>
> Why s/512/64/?

Earlier this was called in recursion, so there was a comment in previous
series to reduce this to lesser number.

>
> This change and the one above hide what the patch really does to 
> spapr_create_pci_child_dt.
>
>
>> +    int slot = PCI_SLOT(pdev->devfn);
>> +    int func = PCI_FUNC(pdev->devfn);
>>
>> -    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,
>> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb,
>>                                         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,
>> @@ -996,24 +1033,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) {
>
>
> I understand the patch is not changing this but still while we are here -
> spapr_phb_add_pci_device() is only called from spapr_phb_hot_plug_child(), 
> how can dev->hotplugged be not true in this function (if it cannot, you 
> could get rid of "out:"?

It gets called even when the devices are added during boot.

>
>> -        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);
>>       }
>>   }
>>
>> @@ -1043,16 +1082,6 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
>>       drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
>>   }
>>
>> -static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
>> -                                               PCIDevice *pdev)
>
> Just adding forward declaration would make the patch shorter.

Yes, I can do that.

>
>> -{
>> -    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
>> -    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
>> -                                    (phb->index << 16) |
>> -                                    (busnr << 8) |
>> -                                    pdev->devfn);
>> -}
>> -
>>   static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>>                                        DeviceState *plugged_dev, Error **errp)
>>   {
>> @@ -1482,6 +1511,75 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *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 secondary;
>> +    unsigned int subordinate = 0xff;
>> +
>> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
>> +         PCI_HEADER_TYPE_BRIDGE)) {
>
>
> s/==/!=/ and "return" and no need in extra indent below.

Right.

>
>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>> +        secondary = *bus_no + 1;
>
>
> (*bus_no)++;
> secondary = *bus_no;
>
> and remove "bus_no = *bus_no + 1" below?
> In fact, I do not need much sense in having "secondary" variable in this 
> function.
>
>> +        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) {
>
> same here? Just like you did in spapr_populate_pci_devices_dt(). I do not 
> insist though. But having less scopes just makes it easier/nicer to wrap 
> long lines in QEMU coding style (new line starts under "(").
>
>
>> +            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)
>> @@ -1521,6 +1619,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 +1670,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));
>
>
> Can we also add a hack here to scan for the "qemu,phb-enumerated" string in 
> the SLOF bin image?

Really ? That would be ugly.

>
>> +
>> +    /* 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) {
>>
>
>
> -- 
> Alexey

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

[Alexey Kardashevskiy]

On 05/25/2015 02:45 PM, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>
>> On 05/19/2015 06:26 PM, 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 | 188 ++++++++++++++++++++++++++++++++++++++++++-----------
>>>    1 file changed, 150 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 8b02a3e..12f1b9c 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"
>>> @@ -742,6 +744,31 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>>        return &phb->iommu_as;
>>>    }
>>>
>>> +
>>> +static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
>>> +                                               PCIDevice *pdev)
>>> +{
>>> +    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
>>> +    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
>>> +                                    (phb->index << 16) |
>>> +                                    (busnr << 8) |
>>> +                                    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);
>>> +}
>>> +
>>>    /* 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 */
>>> @@ -879,12 +906,13 @@ 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,
>>> +                                       sPAPRPHBState *sphb,
>>>                                           const char *drc_name)
>>>    {
>>>        ResourceProps rp;
>>>        bool is_bridge = false;
>>>        int pci_status;
>>> +    uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
>>
>> Is this drc_index any different from the one which used to be passed to
>> this function? If no, then I do not see the point in changing the prototype
>> (or make another "this just makes code easier/nicer" patch).
>
> Its the same, I can have a separate patch. As I was changing this code
> the drc_index would need to be read in boot and hotplug code. So brought
> over the code here.
>
>> If yes, then it would be nice to see what the patch changed in this
>> regard in the commit log.
>>
>>
>>
>>>        if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
>>>            PCI_HEADER_TYPE_BRIDGE) {
>>> @@ -945,8 +973,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));
>>> @@ -963,30 +996,34 @@ 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;
>>> +
>>>    /* 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 *pdev, sPAPRFDT *p,
>>> +                                     const char *drc_name)
>>
>> Why s/dev/pdev/?
>
> PCIDev thats the only reason.


In this file, PCIDevice is called "dev", "pdev", "d", "pci_dev" so if you 
really want to change the name - do it once and for all occurrences OR do 
not do this at all :)


>
>>
>>
>>>    {
>>> -    void *fdt;
>>> -    int offset, ret, fdt_size;
>>> -    int slot = PCI_SLOT(dev->devfn);
>>> -    int func = PCI_FUNC(dev->devfn);
>>> -    char nodename[512];
>>> +    int offset, ret;
>>> +    char nodename[64];
>>
>> Why s/512/64/?
>
> Earlier this was called in recursion, so there was a comment in previous
> series to reduce this to lesser number.

I'd make a separate patch with s/512/64/ and also do s/sprintf/snprintf/ below.



>> This change and the one above hide what the patch really does to
>> spapr_create_pci_child_dt.
>>
>>
>>> +    int slot = PCI_SLOT(pdev->devfn);
>>> +    int func = PCI_FUNC(pdev->devfn);
>>>
>>> -    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,
>>> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
>>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb,
>>>                                          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,
>>> @@ -996,24 +1033,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) {
>>
>>
>> I understand the patch is not changing this but still while we are here -
>> spapr_phb_add_pci_device() is only called from spapr_phb_hot_plug_child(),
>> how can dev->hotplugged be not true in this function (if it cannot, you
>> could get rid of "out:"?
>
> It gets called even when the devices are added during boot.

Where else? I did grep and see just one call:

hw/ppc/spapr_pci.c:1087:static void 
spapr_phb_add_pci_device(sPAPRDRConnector *drc,
hw/ppc/spapr_pci.c:1166:    spapr_phb_add_pci_device(drc, phb, pdev, 
&local_err);



>
>>
>>> -        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);
>>>        }
>>>    }
>>>
>>> @@ -1043,16 +1082,6 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
>>>        drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
>>>    }
>>>
>>> -static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
>>> -                                               PCIDevice *pdev)
>>
>> Just adding forward declaration would make the patch shorter.
>
> Yes, I can do that.
>
>>
>>> -{
>>> -    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
>>> -    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
>>> -                                    (phb->index << 16) |
>>> -                                    (busnr << 8) |
>>> -                                    pdev->devfn);
>>> -}
>>> -
>>>    static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>>>                                         DeviceState *plugged_dev, Error **errp)
>>>    {
>>> @@ -1482,6 +1511,75 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *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 secondary;
>>> +    unsigned int subordinate = 0xff;
>>> +
>>> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
>>> +         PCI_HEADER_TYPE_BRIDGE)) {
>>
>>
>> s/==/!=/ and "return" and no need in extra indent below.
>
> Right.
>
>>
>>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>>> +        secondary = *bus_no + 1;
>>
>>
>> (*bus_no)++;
>> secondary = *bus_no;
>>
>> and remove "bus_no = *bus_no + 1" below?
>> In fact, I do not need much sense in having "secondary" variable in this
>> function.
>>
>>> +        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) {
>>
>> same here? Just like you did in spapr_populate_pci_devices_dt(). I do not
>> insist though. But having less scopes just makes it easier/nicer to wrap
>> long lines in QEMU coding style (new line starts under "(").
>>
>>
>>> +            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)
>>> @@ -1521,6 +1619,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 +1670,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));
>>
>>
>> Can we also add a hack here to scan for the "qemu,phb-enumerated" string in
>> the SLOF bin image?
>
> Really ? That would be ugly.


Well, chances that the binary image will have this line by accident are zero.

And I spent quite some time debugging SRIOV + VFIO when I realized that 
SLOF is old on the test machine where others used to debug too. It would be 
really nice to have a warning that something is wrong. May be extend 
"client-architecture-support" somehow or have some release/date signature 
in known place in SLOF... Thomas (?) also asked for this :)



>
>>
>>> +
>>> +    /* 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) {
>>>




-- 
Alexey

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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 05/25/2015 02:45 PM, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>
>>>>    /* 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 *pdev, sPAPRFDT *p,
>>>> +                                     const char *drc_name)
>>>
>>> Why s/dev/pdev/?
>>
>> PCIDev thats the only reason.
>
>
> In this file, PCIDevice is called "dev", "pdev", "d", "pci_dev" so if you 
> really want to change the name - do it once and for all occurrences OR do 
> not do this at all :)

In that case, lets not do this in this patch.

>
>
>>
>>>
>>>
>>>>    {
>>>> -    void *fdt;
>>>> -    int offset, ret, fdt_size;
>>>> -    int slot = PCI_SLOT(dev->devfn);
>>>> -    int func = PCI_FUNC(dev->devfn);
>>>> -    char nodename[512];
>>>> +    int offset, ret;
>>>> +    char nodename[64];
>>>
>>> Why s/512/64/?
>>
>> Earlier this was called in recursion, so there was a comment in previous
>> series to reduce this to lesser number.
>
> I'd make a separate patch with s/512/64/ and also do
> s/sprintf/snprintf/ below.

Sure.

>
>
>
>>> This change and the one above hide what the patch really does to
>>> spapr_create_pci_child_dt.
>>>
>>>
>>>> +    int slot = PCI_SLOT(pdev->devfn);
>>>> +    int func = PCI_FUNC(pdev->devfn);
>>>>
>>>> -    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,
>>>> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
>>>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb,
>>>>                                          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,
>>>> @@ -996,24 +1033,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) {
>>>
>>>
>>> I understand the patch is not changing this but still while we are here -
>>> spapr_phb_add_pci_device() is only called from spapr_phb_hot_plug_child(),
>>> how can dev->hotplugged be not true in this function (if it cannot, you
>>> could get rid of "out:"?
>>
>> It gets called even when the devices are added during boot.
> Where else? I did grep and see just one call:
>
> hw/ppc/spapr_pci.c:1087:static void 
> spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> hw/ppc/spapr_pci.c:1166:    spapr_phb_add_pci_device(drc, phb, pdev, 
> &local_err);


spapr_phb_add_pci_device gets called for hotplug as well as boot device
through hp->plug = spapr_phb_hot_plug_child

I just tried to verify  passing a pci device with below code:

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index d11e2ab..5737839 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1103,6 +1103,10 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
             error_setg(errp, "Failed to create pci child device tree node");
             goto out;
         }
+    } else {
+      int slot = PCI_SLOT(pdev->devfn);
+      int func = PCI_FUNC(pdev->devfn);
+      fprintf(stderr, "non-hotplug - pci@%d,%d\n", slot, func);
     }

$ ./ppc64-softmmu/qemu-system-ppc64 -machine pseries -m 2G -serial stdio
non-hotplug - pci@0,0
non-hotplug - pci@1,0

SLOF **********************************************************************



>
>
>
>>
>>>
>>>> -        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);
>>>>        }
>>>>    }
>>>>
>>>> @@ -1043,16 +1082,6 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
>>>>        drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
>>>>    }
>>>>
>>>> -static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
>>>> -                                               PCIDevice *pdev)
>>>
>>> Just adding forward declaration would make the patch shorter.
>>
>> Yes, I can do that.
>>
>>>
>>>> -{
>>>> -    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
>>>> -    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
>>>> -                                    (phb->index << 16) |
>>>> -                                    (busnr << 8) |
>>>> -                                    pdev->devfn);
>>>> -}
>>>> -
>>>>    static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>>>>                                         DeviceState *plugged_dev, Error **errp)
>>>>    {
>>>> @@ -1482,6 +1511,75 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *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 secondary;
>>>> +    unsigned int subordinate = 0xff;
>>>> +
>>>> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
>>>> +         PCI_HEADER_TYPE_BRIDGE)) {
>>>
>>>
>>> s/==/!=/ and "return" and no need in extra indent below.
>>
>> Right.
>>
>>>
>>>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>>>> +        secondary = *bus_no + 1;
>>>
>>>
>>> (*bus_no)++;
>>> secondary = *bus_no;
>>>
>>> and remove "bus_no = *bus_no + 1" below?
>>> In fact, I do not need much sense in having "secondary" variable in this
>>> function.
>>>
>>>> +        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) {
>>>
>>> same here? Just like you did in spapr_populate_pci_devices_dt(). I do not
>>> insist though. But having less scopes just makes it easier/nicer to wrap
>>> long lines in QEMU coding style (new line starts under "(").
>>>
>>>
>>>> +            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)
>>>> @@ -1521,6 +1619,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 +1670,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));
>>>
>>>
>>> Can we also add a hack here to scan for the "qemu,phb-enumerated" string in
>>> the SLOF bin image?
>>
>> Really ? That would be ugly.
>
>
> Well, chances that the binary image will have this line by accident are zero.
>
> And I spent quite some time debugging SRIOV + VFIO when I realized that 
> SLOF is old on the test machine where others used to debug too. It would be 
> really nice to have a warning that something is wrong. May be extend 
> "client-architecture-support" somehow or have some release/date signature 
> in known place in SLOF... Thomas (?) also asked for this :)

Sure, I can work on this. I would not recommend grepping though.

Regards
Nikunj

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

[David Gibson]

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

On Mon, May 25, 2015 at 03:53:52PM +0530, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> > On 05/25/2015 02:45 PM, Nikunj A Dadhania wrote:
> >> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
[snip]
> >>> Can we also add a hack here to scan for the "qemu,phb-enumerated" string in
> >>> the SLOF bin image?
> >>
> >> Really ? That would be ugly.
> >
> >
> > Well, chances that the binary image will have this line by accident are zero.
> >
> > And I spent quite some time debugging SRIOV + VFIO when I realized that 
> > SLOF is old on the test machine where others used to debug too. It would be 
> > really nice to have a warning that something is wrong. May be extend 
> > "client-architecture-support" somehow or have some release/date signature 
> > in known place in SLOF... Thomas (?) also asked for this :)
> 
> Sure, I can work on this. I would not recommend grepping though.

Yeah, don't do this.  The ugliness is not worth it.

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

[Qemu-devel] [PATCH v5 4/4] 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>:<slot>.<fn>

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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 12f1b9c..dd77119 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -769,6 +769,61 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
     return drck->get_index(drc);
 }
 
+static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char *path = NULL, *buf = NULL;
+    char *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;
+    char devtype[16] = "qemu";
+
+    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
+        buf = spapr_phb_vfio_get_loc_code(sphb, pdev);
+        if (buf) {
+            return buf;
+        }
+        snprintf(devtype, 4, "vfio");
+    }
+    /*
+     * For emulated devices and VFIO-failure case, make up
+     * the loc-code.
+     */
+    buf = g_strdup_printf("%s_%s:%02d:%02d.%1d",
+                          devtype, pdev->name,
+                          sphb->index, 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 */
@@ -906,12 +961,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
 }
 
 static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
-                                       sPAPRPHBState *sphb,
-                                       const char *drc_name)
+                                       sPAPRPHBState *sphb)
 {
     ResourceProps rp;
     bool is_bridge = false;
     int pci_status;
+    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) ==
@@ -973,9 +1028,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"));
-    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;
+    } else {
+        _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));
@@ -1003,8 +1062,7 @@ typedef struct sPAPRFDT {
 } sPAPRFDT;
 
 /* create OF node for pci device and required OF DT properties */
-static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
-                                     const char *drc_name)
+static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p)
 {
     int offset, ret;
     char nodename[64];
@@ -1017,8 +1075,8 @@ static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
         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->sphb,
-                                      drc_name);
+
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb);
     g_assert(!ret);
     if (ret) {
         return 0;
@@ -1033,7 +1091,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};
 
@@ -1041,7 +1098,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;
@@ -1519,7 +1576,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

Re: [Qemu-devel] [PATCH v5 4/4] spapr_pci: populate ibm,loc-code

[Alexey Kardashevskiy]

On 05/19/2015 06:26 PM, Nikunj A Dadhania wrote:
> 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>:<slot>.<fn>
>
> 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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 69 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 12f1b9c..dd77119 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -769,6 +769,61 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
>       return drck->get_index(drc);
>   }
>
> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
> +{
> +    char *path = NULL, *buf = NULL;
> +    char *host = NULL;

Why not put them all in one line (or split into 3 lines :) )?


> +
> +    /* 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)) {

If path==NULL, you'll return bad buf.


> +        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;
> +    char devtype[16] = "qemu";

I still like "const char *s = "qemu";" better.


> +
> +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
> +        buf = spapr_phb_vfio_get_loc_code(sphb, pdev);
> +        if (buf) {
> +            return buf;
> +        }
> +        snprintf(devtype, 4, "vfio");

With "const", you could just do devtype = "vfio".

Also,
=====
char devtype[16] = { 0xaa,  0xaa, 0xaa, 0xaa, 0xaa };
snprintf(devtype, 4, "vfio");
printf("%x %x %x\n", devtype[2], devtype[3], devtype[4]);
====

produces on my laptop (gcc 4.9.2) this:

[aik@aik ~]$ ./a.out
69 0 ffffffaa


Is it different where you tested this?


> +    }
> +    /*
> +     * For emulated devices and VFIO-failure case, make up
> +     * the loc-code.
> +     */
> +    buf = g_strdup_printf("%s_%s:%02d:%02d.%1d",

How is "1d" different here from just "d"? If @devfn>10, all digits will be 
printed.


> +                          devtype, pdev->name,
> +                          sphb->index, 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 */
> @@ -906,12 +961,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>   }
>
>   static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> -                                       sPAPRPHBState *sphb,
> -                                       const char *drc_name)
> +                                       sPAPRPHBState *sphb)
>   {
>       ResourceProps rp;
>       bool is_bridge = false;
>       int pci_status;
> +    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) ==
> @@ -973,9 +1028,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"));
> -    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;
> +    } else {

No need in "else".


> +        _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));
> @@ -1003,8 +1062,7 @@ typedef struct sPAPRFDT {
>   } sPAPRFDT;
>
>   /* create OF node for pci device and required OF DT properties */
> -static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
> -                                     const char *drc_name)
> +static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p)
>   {
>       int offset, ret;
>       char nodename[64];
> @@ -1017,8 +1075,8 @@ static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
>           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->sphb,
> -                                      drc_name);
> +
> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb);
>       g_assert(!ret);
>       if (ret) {
>           return 0;
> @@ -1033,7 +1091,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};
>
> @@ -1041,7 +1098,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;
> @@ -1519,7 +1576,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;
>


-- 
Alexey

Re: [Qemu-devel] [PATCH v5 4/4] spapr_pci: populate ibm,loc-code

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 05/19/2015 06:26 PM, Nikunj A Dadhania wrote:
>> 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>:<slot>.<fn>
>>
>> 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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 69 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 12f1b9c..dd77119 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -769,6 +769,61 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
>>       return drck->get_index(drc);
>>   }
>>
>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
>> +{
>> +    char *path = NULL, *buf = NULL;
>> +    char *host = NULL;
>
> Why not put them all in one line (or split into 3 lines :) )?

Sure.

>
>
>> +
>> +    /* 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)) {
>
> If path==NULL, you'll return bad buf.

Oh, got it wrong.

>
>
>> +        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;
>> +    char devtype[16] = "qemu";
>
> I still like "const char *s = "qemu";" better.

Yes, was waiting for respin after review.

>
>
>> +
>> +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
>> +        buf = spapr_phb_vfio_get_loc_code(sphb, pdev);
>> +        if (buf) {
>> +            return buf;
>> +        }
>> +        snprintf(devtype, 4, "vfio");
>
> With "const", you could just do devtype = "vfio".
>
> Also,
> =====
> char devtype[16] = { 0xaa,  0xaa, 0xaa, 0xaa, 0xaa };
> snprintf(devtype, 4, "vfio");
> printf("%x %x %x\n", devtype[2], devtype[3], devtype[4]);
> ====
>
> produces on my laptop (gcc 4.9.2) this:
>
> [aik@aik ~]$ ./a.out
> 69 0 ffffffaa
>
>
> Is it different where you tested this?
>
>
>> +    }
>> +    /*
>> +     * For emulated devices and VFIO-failure case, make up
>> +     * the loc-code.
>> +     */
>> +    buf = g_strdup_printf("%s_%s:%02d:%02d.%1d",
>
> How is "1d" different here from just "d"? If @devfn>10, all digits will be 
> printed.

A device can have only 8 functions, right ?

>
>
>> +                          devtype, pdev->name,
>> +                          sphb->index, 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 */
>> @@ -906,12 +961,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>>   }
>>
>>   static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>> -                                       sPAPRPHBState *sphb,
>> -                                       const char *drc_name)
>> +                                       sPAPRPHBState *sphb)
>>   {
>>       ResourceProps rp;
>>       bool is_bridge = false;
>>       int pci_status;
>> +    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) ==
>> @@ -973,9 +1028,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"));
>> -    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;
>> +    } else {
>
> No need in "else".

Yeah.

>
>> +        _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));
>> @@ -1003,8 +1062,7 @@ typedef struct sPAPRFDT {
>>   } sPAPRFDT;
>>
>>   /* create OF node for pci device and required OF DT properties */
>> -static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
>> -                                     const char *drc_name)
>> +static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p)
>>   {
>>       int offset, ret;
>>       char nodename[64];
>> @@ -1017,8 +1075,8 @@ static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
>>           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->sphb,
>> -                                      drc_name);
>> +
>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb);
>>       g_assert(!ret);
>>       if (ret) {
>>           return 0;
>> @@ -1033,7 +1091,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};
>>
>> @@ -1041,7 +1098,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;
>> @@ -1519,7 +1576,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;
>>
>
>
> -- 
> Alexey

Re: [Qemu-devel] [PATCH v5 4/4] spapr_pci: populate ibm,loc-code

[Alexey Kardashevskiy]

On 05/25/2015 02:58 PM, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>
>> On 05/19/2015 06:26 PM, Nikunj A Dadhania wrote:
>>> 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>:<slot>.<fn>
>>>
>>> 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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>>    1 file changed, 69 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 12f1b9c..dd77119 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -769,6 +769,61 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
>>>        return drck->get_index(drc);
>>>    }
>>>
>>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
>>> +{
>>> +    char *path = NULL, *buf = NULL;
>>> +    char *host = NULL;
>>
>> Why not put them all in one line (or split into 3 lines :) )?
>
> Sure.
>
>>
>>
>>> +
>>> +    /* 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)) {
>>
>> If path==NULL, you'll return bad buf.
>
> Oh, got it wrong.
>
>>
>>
>>> +        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;
>>> +    char devtype[16] = "qemu";
>>
>> I still like "const char *s = "qemu";" better.
>
> Yes, was waiting for respin after review.
>
>>
>>
>>> +
>>> +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
>>> +        buf = spapr_phb_vfio_get_loc_code(sphb, pdev);
>>> +        if (buf) {
>>> +            return buf;
>>> +        }
>>> +        snprintf(devtype, 4, "vfio");
>>
>> With "const", you could just do devtype = "vfio".
>>
>> Also,
>> =====
>> char devtype[16] = { 0xaa,  0xaa, 0xaa, 0xaa, 0xaa };
>> snprintf(devtype, 4, "vfio");
>> printf("%x %x %x\n", devtype[2], devtype[3], devtype[4]);
>> ====
>>
>> produces on my laptop (gcc 4.9.2) this:
>>
>> [aik@aik ~]$ ./a.out
>> 69 0 ffffffaa
>>
>>
>> Is it different where you tested this?
>>
>>
>>> +    }
>>> +    /*
>>> +     * For emulated devices and VFIO-failure case, make up
>>> +     * the loc-code.
>>> +     */
>>> +    buf = g_strdup_printf("%s_%s:%02d:%02d.%1d",
>>
>> How is "1d" different here from just "d"? If @devfn>10, all digits will be
>> printed.
>
> A device can have only 8 functions, right ?


Yes. It is "%d" (decimal) so "1" is a minimum field length anyway ("1" only 
makes sense for strings imho). And if a function number is bigger than 9, 
print() will print all digits, not just the last one. So %d==%1d but I 
would never grep for "%02d:%02d.%1d", I would grep for "%02d:%02d.%d".

[few minutes later] And I just did. And I found that most places actually 
use "%02x:%02x.%x" (trace-events, hw/pci/pci.c, hw/vfio/pci.c) and only one 
(get_pci_host_devaddr) uses "%d" for a function (and even there "%02x" is 
used for a bus and a slot, not "%02d").




>
>>
>>
>>> +                          devtype, pdev->name,
>>> +                          sphb->index, 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 */
>>> @@ -906,12 +961,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>>>    }
>>>
>>>    static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>>> -                                       sPAPRPHBState *sphb,
>>> -                                       const char *drc_name)
>>> +                                       sPAPRPHBState *sphb)
>>>    {
>>>        ResourceProps rp;
>>>        bool is_bridge = false;
>>>        int pci_status;
>>> +    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) ==
>>> @@ -973,9 +1028,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"));
>>> -    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;
>>> +    } else {
>>
>> No need in "else".
>
> Yeah.
>
>>
>>> +        _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));
>>> @@ -1003,8 +1062,7 @@ typedef struct sPAPRFDT {
>>>    } sPAPRFDT;
>>>
>>>    /* create OF node for pci device and required OF DT properties */
>>> -static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
>>> -                                     const char *drc_name)
>>> +static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p)
>>>    {
>>>        int offset, ret;
>>>        char nodename[64];
>>> @@ -1017,8 +1075,8 @@ static int spapr_create_pci_child_dt(PCIDevice *pdev, sPAPRFDT *p,
>>>            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->sphb,
>>> -                                      drc_name);
>>> +
>>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb);
>>>        g_assert(!ret);
>>>        if (ret) {
>>>            return 0;
>>> @@ -1033,7 +1091,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};
>>>
>>> @@ -1041,7 +1098,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;
>>> @@ -1519,7 +1576,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;
>>>
>>
>>
>> --
>> Alexey
>


-- 
Alexey

Re: [Qemu-devel] [PATCH v5 4/4] spapr_pci: populate ibm,loc-code

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 05/25/2015 02:58 PM, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>
>>>
>>>> +    }
>>>> +    /*
>>>> +     * For emulated devices and VFIO-failure case, make up
>>>> +     * the loc-code.
>>>> +     */
>>>> +    buf = g_strdup_printf("%s_%s:%02d:%02d.%1d",
>>>
>>> How is "1d" different here from just "d"? If @devfn>10, all digits will be
>>> printed.
>>
>> A device can have only 8 functions, right ?
>
>
> Yes. It is "%d" (decimal) so "1" is a minimum field length anyway ("1" only 
> makes sense for strings imho). And if a function number is bigger than 9, 
> print() will print all digits, not just the last one. So %d==%1d but I 
> would never grep for "%02d:%02d.%1d", I would grep for "%02d:%02d.%d".
>
> [few minutes later] And I just did. And I found that most places actually 
> use "%02x:%02x.%x" (trace-events, hw/pci/pci.c, hw/vfio/pci.c) and only one 
> (get_pci_host_devaddr) uses "%d" for a function (and even there "%02x" is 
> used for a bus and a slot, not "%02d").

Using "%02x:%02x.%x" should be fine then, I will change the code
accordingly.

Regards
Nikunj