[Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices

[Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices

[David Gibson]

spapr_populate_pci_child_dt() adds a 'name' property to the device tree
node for PCI devices.  This is never necessary for a flattened device tree,
it is implicit in the name added when the node is constructed.  In fact
anything we do add to a 'name' property will be overwritten with something
derived from the structural name in the guest firmware (but in fact it is
exactly the same bytes).

So, remove that.  In addition, pci_get_node_name() is very simple, so fold
it into its (also simple) sole caller spapr_create_pci_child_dt().

While we're there rename pci_find_device_name() to the shorter and more
accurate dt_name_from_class().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 97961b0128..b2db46ef1d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1173,8 +1173,8 @@ static const PCIClass pci_classes[] = {
     { "data-processing-controller", spc_subclass },
 };
 
-static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
-                                        uint8_t iface)
+static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
+                                      uint8_t iface)
 {
     const PCIClass *pclass;
     const PCISubClass *psubclass;
@@ -1216,23 +1216,6 @@ static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
     return name;
 }
 
-static gchar *pci_get_node_name(PCIDevice *dev)
-{
-    int slot = PCI_SLOT(dev->devfn);
-    int func = PCI_FUNC(dev->devfn);
-    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
-    const char *name;
-
-    name = pci_find_device_name((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
-                                ccode & 0xff);
-
-    if (func != 0) {
-        return g_strdup_printf("%s@%x,%x", name, slot, func);
-    } else {
-        return g_strdup_printf("%s@%x", name, slot);
-    }
-}
-
 static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
                                             PCIDevice *pdev);
 
@@ -1300,11 +1283,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
         _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
     }
 
-    _FDT(fdt_setprop_string(fdt, offset, "name",
-                            pci_find_device_name((ccode >> 16) & 0xff,
-                                                 (ccode >> 8) & 0xff,
-                                                 ccode & 0xff)));
-
     buf = spapr_phb_get_loc_code(sphb, dev);
     _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
     g_free(buf);
@@ -1348,10 +1326,23 @@ static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
                                      void *fdt, int node_offset)
 {
     int offset;
-    gchar *nodename;
+    const gchar *basename;
+    char *nodename;
+    int slot = PCI_SLOT(dev->devfn);
+    int func = PCI_FUNC(dev->devfn);
+    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
+
+    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
+                                  ccode & 0xff);
+
+    if (func != 0) {
+        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
+    } else {
+        nodename = g_strdup_printf("%s@%x", basename, slot);
+    }
 
-    nodename = pci_get_node_name(dev);
     _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
+
     g_free(nodename);
 
     spapr_populate_pci_child_dt(dev, fdt, offset, phb);
-- 
2.21.0

[Qemu-devel] [PATCH 2/8] spapr: Clean up device tree construction for PCI devices

[David Gibson]

spapr_create_pci_child_dt() is a trivial wrapper around
spapr_populate_pci_child_dt(), but is the latter's only caller.  So fold
them together into spapr_dt_pci_device(), which closer matches our modern
naming convention.

While there, make a number of cleanups to the function itself.  This is
mostly using more temporary locals to avoid awkwardly long lines, and in
some cases avoiding double reads of PCI config space variables.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 119 +++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 64 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index b2db46ef1d..4075b433fd 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1219,57 +1219,75 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
 static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
                                             PCIDevice *pdev);
 
-static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
-                                       SpaprPhbState *sphb)
+/* create OF node for pci device and required OF DT properties */
+static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
+                               void *fdt, int parent_offset)
 {
+    int offset;
+    const gchar *basename;
+    char *nodename;
+    int slot = PCI_SLOT(dev->devfn);
+    int func = PCI_FUNC(dev->devfn);
     ResourceProps rp;
-    bool is_bridge = false;
-    int pci_status;
-    char *buf = NULL;
     uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
+    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
+    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
+    uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
+    uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
+    uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
     uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
-    uint32_t max_msi, max_msix;
+    uint32_t irq_pin = pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1);
+    uint32_t subsystem_id = pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2);
+    uint32_t subsystem_vendor_id =
+        pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2);
+    uint32_t cache_line_size =
+        pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1);
+    uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
+    gchar *loc_code;
+    
+    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
+                                  ccode & 0xff);
 
-    if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
-        PCI_HEADER_TYPE_BRIDGE) {
-        is_bridge = true;
+    if (func != 0) {
+        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
+    } else {
+        nodename = g_strdup_printf("%s@%x", basename, slot);
     }
 
+    _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename));
+
+    g_free(nodename);
+
     /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */
-    _FDT(fdt_setprop_cell(fdt, offset, "vendor-id",
-                          pci_default_read_config(dev, PCI_VENDOR_ID, 2)));
-    _FDT(fdt_setprop_cell(fdt, offset, "device-id",
-                          pci_default_read_config(dev, PCI_DEVICE_ID, 2)));
-    _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
-                          pci_default_read_config(dev, PCI_REVISION_ID, 1)));
+    _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id));
+    _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id));
+    _FDT(fdt_setprop_cell(fdt, offset, "revision-id", revision_id));
+
     _FDT(fdt_setprop_cell(fdt, offset, "class-code", ccode));
-    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)));
+    if (irq_pin) {
+        _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
     }
 
     if (!is_bridge) {
-        _FDT(fdt_setprop_cell(fdt, offset, "min-grant",
-            pci_default_read_config(dev, PCI_MIN_GNT, 1)));
-        _FDT(fdt_setprop_cell(fdt, offset, "max-latency",
-            pci_default_read_config(dev, PCI_MAX_LAT, 1)));
+        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
+        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
+        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
+        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
     }
 
-    if (pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)) {
-        _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id",
-                 pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)));
+    if (subsystem_id) {
+        _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
     }
 
-    if (pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)) {
+    if (subsystem_vendor_id) {
         _FDT(fdt_setprop_cell(fdt, offset, "subsystem-vendor-id",
-                 pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)));
+                              subsystem_vendor_id));
     }
 
-    _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size",
-        pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1)));
+    _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size", cache_line_size));
+
 
     /* the following fdt cells are masked off the pci status register */
-    pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
     _FDT(fdt_setprop_cell(fdt, offset, "devsel-speed",
                           PCI_STATUS_DEVSEL_MASK & pci_status));
 
@@ -1283,9 +1301,9 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
         _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
     }
 
-    buf = spapr_phb_get_loc_code(sphb, dev);
-    _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
-    g_free(buf);
+    loc_code = spapr_phb_get_loc_code(sphb, dev);
+    _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", loc_code));
+    g_free(loc_code);
 
     if (drc_index) {
         _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
@@ -1297,13 +1315,13 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
                           RESOURCE_CELLS_SIZE));
 
     if (msi_present(dev)) {
-        max_msi = msi_nr_vectors_allocated(dev);
+        uint32_t max_msi = msi_nr_vectors_allocated(dev);
         if (max_msi) {
             _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
         }
     }
     if (msix_present(dev)) {
-        max_msix = dev->msix_entries_nr;
+        uint32_t max_msix = dev->msix_entries_nr;
         if (max_msix) {
             _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x", max_msix));
         }
@@ -1319,33 +1337,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
     }
 
     spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
-}
-
-/* create OF node for pci device and required OF DT properties */
-static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
-                                     void *fdt, int node_offset)
-{
-    int offset;
-    const gchar *basename;
-    char *nodename;
-    int slot = PCI_SLOT(dev->devfn);
-    int func = PCI_FUNC(dev->devfn);
-    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
-
-    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
-                                  ccode & 0xff);
-
-    if (func != 0) {
-        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
-    } else {
-        nodename = g_strdup_printf("%s@%x", basename, slot);
-    }
-
-    _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
-
-    g_free(nodename);
-
-    spapr_populate_pci_child_dt(dev, fdt, offset, phb);
 
     return offset;
 }
@@ -1393,7 +1384,7 @@ int spapr_pci_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(plug_handler);
     PCIDevice *pdev = PCI_DEVICE(drc->dev);
 
-    *fdt_start_offset = spapr_create_pci_child_dt(sphb, pdev, fdt, 0);
+    *fdt_start_offset = spapr_dt_pci_device(sphb, pdev, fdt, 0);
     return 0;
 }
 
@@ -2086,7 +2077,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
     int offset;
     SpaprFdt s_fdt;
 
-    offset = spapr_create_pci_child_dt(p->sphb, pdev, p->fdt, p->node_off);
+    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
     if (!offset) {
         error_report("Failed to create pci child device tree node");
         return;
-- 
2.21.0

Re: [Qemu-devel] [PATCH 2/8] spapr: Clean up device tree construction for PCI devices

[Greg Kurz]

On Thu, 23 May 2019 15:29:12 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> spapr_create_pci_child_dt() is a trivial wrapper around
> spapr_populate_pci_child_dt(), but is the latter's only caller.  So fold
> them together into spapr_dt_pci_device(), which closer matches our modern
> naming convention.
> 
> While there, make a number of cleanups to the function itself.  This is
> mostly using more temporary locals to avoid awkwardly long lines, and in
> some cases avoiding double reads of PCI config space variables.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 119 +++++++++++++++++++++------------------------
>  1 file changed, 55 insertions(+), 64 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index b2db46ef1d..4075b433fd 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1219,57 +1219,75 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
>  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
>                                              PCIDevice *pdev);
>  
> -static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> -                                       SpaprPhbState *sphb)
> +/* create OF node for pci device and required OF DT properties */
> +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> +                               void *fdt, int parent_offset)
>  {
> +    int offset;
> +    const gchar *basename;
> +    char *nodename;

Being curious... what's the rationale for using gchar or char, if any ?

> +    int slot = PCI_SLOT(dev->devfn);
> +    int func = PCI_FUNC(dev->devfn);
>      ResourceProps rp;
> -    bool is_bridge = false;
> -    int pci_status;
> -    char *buf = NULL;
>      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> +    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
> +    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
> +    uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
> +    uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
> +    uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
>      uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> -    uint32_t max_msi, max_msix;
> +    uint32_t irq_pin = pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1);
> +    uint32_t subsystem_id = pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2);
> +    uint32_t subsystem_vendor_id =
> +        pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2);
> +    uint32_t cache_line_size =
> +        pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1);
> +    uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
> +    gchar *loc_code;
> +    

trailing space ^^

Apart from that, LGTM.

Reviewed-by: Greg Kurz <groug@kaod.org>

> +    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> +                                  ccode & 0xff);
>  
> -    if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
> -        PCI_HEADER_TYPE_BRIDGE) {
> -        is_bridge = true;
> +    if (func != 0) {
> +        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> +    } else {
> +        nodename = g_strdup_printf("%s@%x", basename, slot);
>      }
>  
> +    _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename));
> +
> +    g_free(nodename);
> +
>      /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */
> -    _FDT(fdt_setprop_cell(fdt, offset, "vendor-id",
> -                          pci_default_read_config(dev, PCI_VENDOR_ID, 2)));
> -    _FDT(fdt_setprop_cell(fdt, offset, "device-id",
> -                          pci_default_read_config(dev, PCI_DEVICE_ID, 2)));
> -    _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
> -                          pci_default_read_config(dev, PCI_REVISION_ID, 1)));
> +    _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id));
> +    _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id));
> +    _FDT(fdt_setprop_cell(fdt, offset, "revision-id", revision_id));
> +
>      _FDT(fdt_setprop_cell(fdt, offset, "class-code", ccode));
> -    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)));
> +    if (irq_pin) {
> +        _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
>      }
>  
>      if (!is_bridge) {
> -        _FDT(fdt_setprop_cell(fdt, offset, "min-grant",
> -            pci_default_read_config(dev, PCI_MIN_GNT, 1)));
> -        _FDT(fdt_setprop_cell(fdt, offset, "max-latency",
> -            pci_default_read_config(dev, PCI_MAX_LAT, 1)));
> +        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> +        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> +        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> +        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
>      }
>  
> -    if (pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)) {
> -        _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id",
> -                 pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)));
> +    if (subsystem_id) {
> +        _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
>      }
>  
> -    if (pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)) {
> +    if (subsystem_vendor_id) {
>          _FDT(fdt_setprop_cell(fdt, offset, "subsystem-vendor-id",
> -                 pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)));
> +                              subsystem_vendor_id));
>      }
>  
> -    _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size",
> -        pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1)));
> +    _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size", cache_line_size));
> +
>  
>      /* the following fdt cells are masked off the pci status register */
> -    pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
>      _FDT(fdt_setprop_cell(fdt, offset, "devsel-speed",
>                            PCI_STATUS_DEVSEL_MASK & pci_status));
>  
> @@ -1283,9 +1301,9 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>          _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
>      }
>  
> -    buf = spapr_phb_get_loc_code(sphb, dev);
> -    _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
> -    g_free(buf);
> +    loc_code = spapr_phb_get_loc_code(sphb, dev);
> +    _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", loc_code));
> +    g_free(loc_code);
>  
>      if (drc_index) {
>          _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> @@ -1297,13 +1315,13 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>                            RESOURCE_CELLS_SIZE));
>  
>      if (msi_present(dev)) {
> -        max_msi = msi_nr_vectors_allocated(dev);
> +        uint32_t max_msi = msi_nr_vectors_allocated(dev);
>          if (max_msi) {
>              _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
>          }
>      }
>      if (msix_present(dev)) {
> -        max_msix = dev->msix_entries_nr;
> +        uint32_t max_msix = dev->msix_entries_nr;
>          if (max_msix) {
>              _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x", max_msix));
>          }
> @@ -1319,33 +1337,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>      }
>  
>      spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
> -}
> -
> -/* create OF node for pci device and required OF DT properties */
> -static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
> -                                     void *fdt, int node_offset)
> -{
> -    int offset;
> -    const gchar *basename;
> -    char *nodename;
> -    int slot = PCI_SLOT(dev->devfn);
> -    int func = PCI_FUNC(dev->devfn);
> -    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> -
> -    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> -                                  ccode & 0xff);
> -
> -    if (func != 0) {
> -        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> -    } else {
> -        nodename = g_strdup_printf("%s@%x", basename, slot);
> -    }
> -
> -    _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> -
> -    g_free(nodename);
> -
> -    spapr_populate_pci_child_dt(dev, fdt, offset, phb);
>  
>      return offset;
>  }
> @@ -1393,7 +1384,7 @@ int spapr_pci_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(plug_handler);
>      PCIDevice *pdev = PCI_DEVICE(drc->dev);
>  
> -    *fdt_start_offset = spapr_create_pci_child_dt(sphb, pdev, fdt, 0);
> +    *fdt_start_offset = spapr_dt_pci_device(sphb, pdev, fdt, 0);
>      return 0;
>  }
>  
> @@ -2086,7 +2077,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>      int offset;
>      SpaprFdt s_fdt;
>  
> -    offset = spapr_create_pci_child_dt(p->sphb, pdev, p->fdt, p->node_off);
> +    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
>      if (!offset) {
>          error_report("Failed to create pci child device tree node");
>          return;

Re: [Qemu-devel] [PATCH 2/8] spapr: Clean up device tree construction for PCI devices

[David Gibson]

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

On Fri, May 24, 2019 at 05:34:10PM +0200, Greg Kurz wrote:
65;5603;1c> On Thu, 23 May 2019 15:29:12 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > spapr_create_pci_child_dt() is a trivial wrapper around
> > spapr_populate_pci_child_dt(), but is the latter's only caller.  So fold
> > them together into spapr_dt_pci_device(), which closer matches our modern
> > naming convention.
> > 
> > While there, make a number of cleanups to the function itself.  This is
> > mostly using more temporary locals to avoid awkwardly long lines, and in
> > some cases avoiding double reads of PCI config space variables.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c | 119 +++++++++++++++++++++------------------------
> >  1 file changed, 55 insertions(+), 64 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index b2db46ef1d..4075b433fd 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1219,57 +1219,75 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> >  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> >                                              PCIDevice *pdev);
> >  
> > -static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> > -                                       SpaprPhbState *sphb)
> > +/* create OF node for pci device and required OF DT properties */
> > +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> > +                               void *fdt, int parent_offset)
> >  {
> > +    int offset;
> > +    const gchar *basename;
> > +    char *nodename;
> 
> Being curious... what's the rationale for using gchar or char, if
> any ?

There isn't one really.  I've changed this to gchar for consistency.

> 
> > +    int slot = PCI_SLOT(dev->devfn);
> > +    int func = PCI_FUNC(dev->devfn);
> >      ResourceProps rp;
> > -    bool is_bridge = false;
> > -    int pci_status;
> > -    char *buf = NULL;
> >      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> > +    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
> > +    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
> > +    uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
> > +    uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
> > +    uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
> >      uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > -    uint32_t max_msi, max_msix;
> > +    uint32_t irq_pin = pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1);
> > +    uint32_t subsystem_id = pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2);
> > +    uint32_t subsystem_vendor_id =
> > +        pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2);
> > +    uint32_t cache_line_size =
> > +        pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1);
> > +    uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
> > +    gchar *loc_code;
> > +    
> 
> trailing space ^^

Fixed.
> 
> Apart from that, LGTM.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > +                                  ccode & 0xff);
> >  
> > -    if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
> > -        PCI_HEADER_TYPE_BRIDGE) {
> > -        is_bridge = true;
> > +    if (func != 0) {
> > +        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> > +    } else {
> > +        nodename = g_strdup_printf("%s@%x", basename, slot);
> >      }
> >  
> > +    _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename));
> > +
> > +    g_free(nodename);
> > +
> >      /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */
> > -    _FDT(fdt_setprop_cell(fdt, offset, "vendor-id",
> > -                          pci_default_read_config(dev, PCI_VENDOR_ID, 2)));
> > -    _FDT(fdt_setprop_cell(fdt, offset, "device-id",
> > -                          pci_default_read_config(dev, PCI_DEVICE_ID, 2)));
> > -    _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
> > -                          pci_default_read_config(dev, PCI_REVISION_ID, 1)));
> > +    _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id));
> > +    _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id));
> > +    _FDT(fdt_setprop_cell(fdt, offset, "revision-id", revision_id));
> > +
> >      _FDT(fdt_setprop_cell(fdt, offset, "class-code", ccode));
> > -    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)));
> > +    if (irq_pin) {
> > +        _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
> >      }
> >  
> >      if (!is_bridge) {
> > -        _FDT(fdt_setprop_cell(fdt, offset, "min-grant",
> > -            pci_default_read_config(dev, PCI_MIN_GNT, 1)));
> > -        _FDT(fdt_setprop_cell(fdt, offset, "max-latency",
> > -            pci_default_read_config(dev, PCI_MAX_LAT, 1)));
> > +        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> > +        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> > +        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> > +        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> >      }
> >  
> > -    if (pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)) {
> > -        _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id",
> > -                 pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)));
> > +    if (subsystem_id) {
> > +        _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
> >      }
> >  
> > -    if (pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)) {
> > +    if (subsystem_vendor_id) {
> >          _FDT(fdt_setprop_cell(fdt, offset, "subsystem-vendor-id",
> > -                 pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)));
> > +                              subsystem_vendor_id));
> >      }
> >  
> > -    _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size",
> > -        pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1)));
> > +    _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size", cache_line_size));
> > +
> >  
> >      /* the following fdt cells are masked off the pci status register */
> > -    pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
> >      _FDT(fdt_setprop_cell(fdt, offset, "devsel-speed",
> >                            PCI_STATUS_DEVSEL_MASK & pci_status));
> >  
> > @@ -1283,9 +1301,9 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >          _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
> >      }
> >  
> > -    buf = spapr_phb_get_loc_code(sphb, dev);
> > -    _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
> > -    g_free(buf);
> > +    loc_code = spapr_phb_get_loc_code(sphb, dev);
> > +    _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", loc_code));
> > +    g_free(loc_code);
> >  
> >      if (drc_index) {
> >          _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> > @@ -1297,13 +1315,13 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >                            RESOURCE_CELLS_SIZE));
> >  
> >      if (msi_present(dev)) {
> > -        max_msi = msi_nr_vectors_allocated(dev);
> > +        uint32_t max_msi = msi_nr_vectors_allocated(dev);
> >          if (max_msi) {
> >              _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
> >          }
> >      }
> >      if (msix_present(dev)) {
> > -        max_msix = dev->msix_entries_nr;
> > +        uint32_t max_msix = dev->msix_entries_nr;
> >          if (max_msix) {
> >              _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x", max_msix));
> >          }
> > @@ -1319,33 +1337,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >      }
> >  
> >      spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
> > -}
> > -
> > -/* create OF node for pci device and required OF DT properties */
> > -static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
> > -                                     void *fdt, int node_offset)
> > -{
> > -    int offset;
> > -    const gchar *basename;
> > -    char *nodename;
> > -    int slot = PCI_SLOT(dev->devfn);
> > -    int func = PCI_FUNC(dev->devfn);
> > -    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > -
> > -    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > -                                  ccode & 0xff);
> > -
> > -    if (func != 0) {
> > -        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> > -    } else {
> > -        nodename = g_strdup_printf("%s@%x", basename, slot);
> > -    }
> > -
> > -    _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> > -
> > -    g_free(nodename);
> > -
> > -    spapr_populate_pci_child_dt(dev, fdt, offset, phb);
> >  
> >      return offset;
> >  }
> > @@ -1393,7 +1384,7 @@ int spapr_pci_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> >      SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(plug_handler);
> >      PCIDevice *pdev = PCI_DEVICE(drc->dev);
> >  
> > -    *fdt_start_offset = spapr_create_pci_child_dt(sphb, pdev, fdt, 0);
> > +    *fdt_start_offset = spapr_dt_pci_device(sphb, pdev, fdt, 0);
> >      return 0;
> >  }
> >  
> > @@ -2086,7 +2077,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> >      int offset;
> >      SpaprFdt s_fdt;
> >  
> > -    offset = spapr_create_pci_child_dt(p->sphb, pdev, p->fdt, p->node_off);
> > +    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
> >      if (!offset) {
> >          error_report("Failed to create pci child device tree node");
> >          return;
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 3/8] spapr: Clean up dt creation for PCI buses

[David Gibson]

Device nodes for PCI bridges (both host and P2P) describe both the bridge
device itself and the bus hanging off it, handling of this is a bit of a
mess.

spapr_dt_pci_device() has a few things it only adds for non-bridges, but
always adds #address-cells and #size-cells which should only appear for
bridges.  But the walking down the subordinate PCI bus is done in one of
its callers spapr_populate_pci_devices_dt().  The PHB dt creation in
spapr_populate_pci_dt() open codes some similar logic to the bridge case.

This patch consolidates things in a bunch of ways:
 * Bus specific dt info is now created in spapr_dt_pci_bus() used for both
   P2P bridges and the host bridge.  This includes walking subordinate
   devices
 * spapr_dt_pci_device() now calls spapr_dt_pci_bus() when called on a
   P2P bridge
 * We do detection of bridges with the is_bridge field of the device class,
   rather than checking PCI config space directly, for consistency with
   qemu's core PCI code.
 * Several things are renamed for brevity and clarity

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              |   7 +-
 hw/ppc/spapr_pci.c          | 140 +++++++++++++++++++-----------------
 include/hw/pci-host/spapr.h |   4 +-
 3 files changed, 79 insertions(+), 72 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e2b33e5890..44573adce7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1309,8 +1309,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
     }
 
     QLIST_FOREACH(phb, &spapr->phbs, list) {
-        ret = spapr_populate_pci_dt(phb, PHANDLE_INTC, fdt,
-                                    spapr->irq->nr_msis, NULL);
+        ret = spapr_dt_phb(phb, PHANDLE_INTC, fdt, spapr->irq->nr_msis, NULL);
         if (ret < 0) {
             error_report("couldn't setup PCI devices in fdt");
             exit(1);
@@ -3917,8 +3916,8 @@ int spapr_phb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
         return -1;
     }
 
-    if (spapr_populate_pci_dt(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
-                              fdt_start_offset)) {
+    if (spapr_dt_phb(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
+                     fdt_start_offset)) {
         error_setg(errp, "unable to create FDT node for PHB %d", sphb->index);
         return -1;
     }
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4075b433fd..c166df4145 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1219,6 +1219,60 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
 static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
                                             PCIDevice *pdev);
 
+typedef struct PciWalkFdt {
+    void *fdt;
+    int offset;
+    SpaprPhbState *sphb;
+    int err;
+} PciWalkFdt;
+
+static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
+                               void *fdt, int parent_offset);
+
+static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
+                                   void *opaque)
+{
+    PciWalkFdt *p = opaque;
+    int err;
+
+    if (p->err) {
+        /* Something's already broken, don't keep going */
+        return;
+    }
+
+    err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
+    if (err < 0) {
+        p->err = err;
+    }
+}
+
+/* Augment PCI device node with bridge specific information */
+static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
+                               void *fdt, int offset)
+{
+    PciWalkFdt cbinfo = {
+        .fdt = fdt,
+        .offset = offset,
+        .sphb = sphb,
+        .err = 0,
+    };
+
+    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
+                          RESOURCE_CELLS_ADDRESS));
+    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
+                          RESOURCE_CELLS_SIZE));
+
+    if (bus) {
+        pci_for_each_device_reverse(bus, pci_bus_num(bus),
+                                    spapr_dt_pci_device_cb, &cbinfo);
+        if (cbinfo.err) {
+            return cbinfo.err;
+        }
+    }
+
+    return offset;
+}
+
 /* create OF node for pci device and required OF DT properties */
 static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
                                void *fdt, int parent_offset)
@@ -1228,10 +1282,9 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
     char *nodename;
     int slot = PCI_SLOT(dev->devfn);
     int func = PCI_FUNC(dev->devfn);
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
     ResourceProps rp;
     uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
-    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
-    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
     uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
     uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
     uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
@@ -1268,13 +1321,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
         _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
     }
 
-    if (!is_bridge) {
-        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
-        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
-        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
-        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
-    }
-
     if (subsystem_id) {
         _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
     }
@@ -1309,11 +1355,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
         _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
     }
 
-    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
-                          RESOURCE_CELLS_ADDRESS));
-    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
-                          RESOURCE_CELLS_SIZE));
-
     if (msi_present(dev)) {
         uint32_t max_msi = msi_nr_vectors_allocated(dev);
         if (max_msi) {
@@ -1338,7 +1379,18 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
 
     spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
 
-    return offset;
+    if (!pc->is_bridge) {
+        /* Properties only for non-bridges */
+        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
+        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
+        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
+        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
+        return offset;
+    } else {
+        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
+
+        return spapr_dt_pci_bus(sphb, sec_bus, fdt, offset);
+    }
 }
 
 /* Callback to be called during DRC release. */
@@ -2063,44 +2115,6 @@ static const TypeInfo spapr_phb_info = {
     }
 };
 
-typedef struct SpaprFdt {
-    void *fdt;
-    int node_off;
-    SpaprPhbState *sphb;
-} SpaprFdt;
-
-static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
-                                          void *opaque)
-{
-    PCIBus *sec_bus;
-    SpaprFdt *p = opaque;
-    int offset;
-    SpaprFdt s_fdt;
-
-    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
-    if (!offset) {
-        error_report("Failed to create pci child device tree node");
-        return;
-    }
-
-    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
-         PCI_HEADER_TYPE_BRIDGE)) {
-        return;
-    }
-
-    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
-    if (!sec_bus) {
-        return;
-    }
-
-    s_fdt.fdt = p->fdt;
-    s_fdt.node_off = offset;
-    s_fdt.sphb = p->sphb;
-    pci_for_each_device_reverse(sec_bus, pci_bus_num(sec_bus),
-                                spapr_populate_pci_devices_dt,
-                                &s_fdt);
-}
-
 static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
                                            void *opaque)
 {
@@ -2138,8 +2152,8 @@ static void spapr_phb_pci_enumerate(SpaprPhbState *phb)
 
 }
 
-int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
-                          uint32_t nr_msis, int *node_offset)
+int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
+                 uint32_t nr_msis, int *node_offset)
 {
     int bus_off, i, j, ret;
     uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
@@ -2186,8 +2200,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
                                 cpu_to_be32(0x0),
                                 cpu_to_be32(phb->numa_node)};
     SpaprTceTable *tcet;
-    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
-    SpaprFdt s_fdt;
     SpaprDrc *drc;
     Error *errp = NULL;
 
@@ -2200,8 +2212,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
     /* Write PHB properties */
     _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
     _FDT(fdt_setprop_string(fdt, bus_off, "compatible", "IBM,Logical_PHB"));
-    _FDT(fdt_setprop_cell(fdt, bus_off, "#address-cells", 0x3));
-    _FDT(fdt_setprop_cell(fdt, bus_off, "#size-cells", 0x2));
     _FDT(fdt_setprop_cell(fdt, bus_off, "#interrupt-cells", 0x1));
     _FDT(fdt_setprop(fdt, bus_off, "used-by-rtas", NULL, 0));
     _FDT(fdt_setprop(fdt, bus_off, "bus-range", &bus_range, sizeof(bus_range)));
@@ -2266,13 +2276,11 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
     spapr_phb_pci_enumerate(phb);
     _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
 
-    /* Populate tree nodes with PCI devices attached */
-    s_fdt.fdt = fdt;
-    s_fdt.node_off = bus_off;
-    s_fdt.sphb = phb;
-    pci_for_each_device_reverse(bus, pci_bus_num(bus),
-                                spapr_populate_pci_devices_dt,
-                                &s_fdt);
+    /* Walk the bridge and subordinate buses */
+    ret = spapr_dt_pci_bus(phb, PCI_HOST_BRIDGE(phb)->bus, fdt, bus_off);
+    if (ret) {
+        return ret;
+    }
 
     ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
                                 SPAPR_DR_CONNECTOR_TYPE_PCI);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 53519c835e..1b61162f91 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -131,8 +131,8 @@ static inline qemu_irq spapr_phb_lsi_qirq(struct SpaprPhbState *phb, int pin)
     return spapr_qirq(spapr, phb->lsi_table[pin].irq);
 }
 
-int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
-                          uint32_t nr_msis, int *node_offset);
+int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
+                 uint32_t nr_msis, int *node_offset);
 
 void spapr_pci_rtas_init(void);
 
-- 
2.21.0

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/8] spapr: Clean up dt creation for PCI buses

[Alexey Kardashevskiy]

On 23/05/2019 15:29, David Gibson wrote:
> Device nodes for PCI bridges (both host and P2P) describe both the bridge
> device itself and the bus hanging off it, handling of this is a bit of a
> mess.
> 
> spapr_dt_pci_device() has a few things it only adds for non-bridges, but
> always adds #address-cells and #size-cells which should only appear for
> bridges.  But the walking down the subordinate PCI bus is done in one of
> its callers spapr_populate_pci_devices_dt().  The PHB dt creation in
> spapr_populate_pci_dt() open codes some similar logic to the bridge case.
> 
> This patch consolidates things in a bunch of ways:
>  * Bus specific dt info is now created in spapr_dt_pci_bus() used for both
>    P2P bridges and the host bridge.  This includes walking subordinate
>    devices
>  * spapr_dt_pci_device() now calls spapr_dt_pci_bus() when called on a
>    P2P bridge
>  * We do detection of bridges with the is_bridge field of the device class,
>    rather than checking PCI config space directly, for consistency with
>    qemu's core PCI code.
>  * Several things are renamed for brevity and clarity
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c              |   7 +-
>  hw/ppc/spapr_pci.c          | 140 +++++++++++++++++++-----------------
>  include/hw/pci-host/spapr.h |   4 +-
>  3 files changed, 79 insertions(+), 72 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e2b33e5890..44573adce7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1309,8 +1309,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
>      }
>  
>      QLIST_FOREACH(phb, &spapr->phbs, list) {
> -        ret = spapr_populate_pci_dt(phb, PHANDLE_INTC, fdt,
> -                                    spapr->irq->nr_msis, NULL);
> +        ret = spapr_dt_phb(phb, PHANDLE_INTC, fdt, spapr->irq->nr_msis, NULL);
>          if (ret < 0) {
>              error_report("couldn't setup PCI devices in fdt");
>              exit(1);
> @@ -3917,8 +3916,8 @@ int spapr_phb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>          return -1;
>      }
>  
> -    if (spapr_populate_pci_dt(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> -                              fdt_start_offset)) {
> +    if (spapr_dt_phb(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> +                     fdt_start_offset)) {
>          error_setg(errp, "unable to create FDT node for PHB %d", sphb->index);
>          return -1;
>      }
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 4075b433fd..c166df4145 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1219,6 +1219,60 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
>  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
>                                              PCIDevice *pdev);
>  
> +typedef struct PciWalkFdt {
> +    void *fdt;
> +    int offset;
> +    SpaprPhbState *sphb;
> +    int err;
> +} PciWalkFdt;
> +
> +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> +                               void *fdt, int parent_offset);
> +
> +static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
> +                                   void *opaque)
> +{
> +    PciWalkFdt *p = opaque;
> +    int err;
> +
> +    if (p->err) {
> +        /* Something's already broken, don't keep going */
> +        return;
> +    }
> +
> +    err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
> +    if (err < 0) {
> +        p->err = err;
> +    }
> +}
> +
> +/* Augment PCI device node with bridge specific information */
> +static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
> +                               void *fdt, int offset)
> +{
> +    PciWalkFdt cbinfo = {
> +        .fdt = fdt,
> +        .offset = offset,
> +        .sphb = sphb,
> +        .err = 0,
> +    };
> +
> +    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> +                          RESOURCE_CELLS_ADDRESS));
> +    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> +                          RESOURCE_CELLS_SIZE));
> +
> +    if (bus) {
> +        pci_for_each_device_reverse(bus, pci_bus_num(bus),
> +                                    spapr_dt_pci_device_cb, &cbinfo);
> +        if (cbinfo.err) {
> +            return cbinfo.err;
> +        }
> +    }
> +
> +    return offset;


This spapr_dt_pci_bus() returns 0 or an offset or an error.

But:
spapr_dt_phb() returns 0 or error; and so does spapr_dt_drc().

Not extremely consistent.

It looks like spapr_dt_pci_bus() does not need to return @offset as it
does not change it and the caller - spapr_dt_pci_device() - can have 1
"return offset" in the end.



> +}
> +
>  /* create OF node for pci device and required OF DT properties */
>  static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>                                 void *fdt, int parent_offset)
> @@ -1228,10 +1282,9 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>      char *nodename;
>      int slot = PCI_SLOT(dev->devfn);
>      int func = PCI_FUNC(dev->devfn);
> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>      ResourceProps rp;
>      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> -    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
> -    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
>      uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
>      uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
>      uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
> @@ -1268,13 +1321,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>          _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
>      }
>  
> -    if (!is_bridge) {
> -        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> -        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> -        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> -        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> -    }
> -
>      if (subsystem_id) {
>          _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
>      }
> @@ -1309,11 +1355,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>          _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>      }
>  
> -    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> -                          RESOURCE_CELLS_ADDRESS));
> -    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> -                          RESOURCE_CELLS_SIZE));
> -
>      if (msi_present(dev)) {
>          uint32_t max_msi = msi_nr_vectors_allocated(dev);
>          if (max_msi) {
> @@ -1338,7 +1379,18 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>  
>      spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
>  
> -    return offset;
> +    if (!pc->is_bridge) {
> +        /* Properties only for non-bridges */
> +        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> +        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> +        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> +        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> +        return offset;
> +    } else {
> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> +
> +        return spapr_dt_pci_bus(sphb, sec_bus, fdt, offset);
> +    }
>  }
>  
>  /* Callback to be called during DRC release. */
> @@ -2063,44 +2115,6 @@ static const TypeInfo spapr_phb_info = {
>      }
>  };
>  
> -typedef struct SpaprFdt {
> -    void *fdt;
> -    int node_off;
> -    SpaprPhbState *sphb;
> -} SpaprFdt;
> -
> -static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> -                                          void *opaque)
> -{
> -    PCIBus *sec_bus;
> -    SpaprFdt *p = opaque;
> -    int offset;
> -    SpaprFdt s_fdt;
> -
> -    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
> -    if (!offset) {
> -        error_report("Failed to create pci child device tree node");
> -        return;
> -    }
> -
> -    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
> -         PCI_HEADER_TYPE_BRIDGE)) {
> -        return;
> -    }
> -
> -    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> -    if (!sec_bus) {
> -        return;
> -    }
> -
> -    s_fdt.fdt = p->fdt;
> -    s_fdt.node_off = offset;
> -    s_fdt.sphb = p->sphb;
> -    pci_for_each_device_reverse(sec_bus, pci_bus_num(sec_bus),
> -                                spapr_populate_pci_devices_dt,
> -                                &s_fdt);
> -}
> -
>  static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
>                                             void *opaque)
>  {
> @@ -2138,8 +2152,8 @@ static void spapr_phb_pci_enumerate(SpaprPhbState *phb)
>  
>  }
>  
> -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> -                          uint32_t nr_msis, int *node_offset)
> +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> +                 uint32_t nr_msis, int *node_offset)
>  {
>      int bus_off, i, j, ret;
>      uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
> @@ -2186,8 +2200,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>                                  cpu_to_be32(0x0),
>                                  cpu_to_be32(phb->numa_node)};
>      SpaprTceTable *tcet;
> -    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> -    SpaprFdt s_fdt;
>      SpaprDrc *drc;
>      Error *errp = NULL;
>  
> @@ -2200,8 +2212,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>      /* Write PHB properties */
>      _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
>      _FDT(fdt_setprop_string(fdt, bus_off, "compatible", "IBM,Logical_PHB"));
> -    _FDT(fdt_setprop_cell(fdt, bus_off, "#address-cells", 0x3));
> -    _FDT(fdt_setprop_cell(fdt, bus_off, "#size-cells", 0x2));
>      _FDT(fdt_setprop_cell(fdt, bus_off, "#interrupt-cells", 0x1));
>      _FDT(fdt_setprop(fdt, bus_off, "used-by-rtas", NULL, 0));
>      _FDT(fdt_setprop(fdt, bus_off, "bus-range", &bus_range, sizeof(bus_range)));
> @@ -2266,13 +2276,11 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>      spapr_phb_pci_enumerate(phb);
>      _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
>  
> -    /* Populate tree nodes with PCI devices attached */
> -    s_fdt.fdt = fdt;
> -    s_fdt.node_off = bus_off;
> -    s_fdt.sphb = phb;
> -    pci_for_each_device_reverse(bus, pci_bus_num(bus),
> -                                spapr_populate_pci_devices_dt,
> -                                &s_fdt);
> +    /* Walk the bridge and subordinate buses */
> +    ret = spapr_dt_pci_bus(phb, PCI_HOST_BRIDGE(phb)->bus, fdt, bus_off);
> +    if (ret) {


if (ret < 0)

otherwise it returns prematurely and NVLink stuff does not make it to
the FDT.


Otherwise the whole patchset is a good cleanup and seems working fine.


> +        return ret;
> +    }
>  
>      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
>                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 53519c835e..1b61162f91 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -131,8 +131,8 @@ static inline qemu_irq spapr_phb_lsi_qirq(struct SpaprPhbState *phb, int pin)
>      return spapr_qirq(spapr, phb->lsi_table[pin].irq);
>  }
>  
> -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> -                          uint32_t nr_msis, int *node_offset);
> +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> +                 uint32_t nr_msis, int *node_offset);
>  
>  void spapr_pci_rtas_init(void);
>  
> 

-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/8] spapr: Clean up dt creation for PCI buses

[David Gibson]

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

On Fri, May 24, 2019 at 03:31:54PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 23/05/2019 15:29, David Gibson wrote:
> > Device nodes for PCI bridges (both host and P2P) describe both the bridge
> > device itself and the bus hanging off it, handling of this is a bit of a
> > mess.
> > 
> > spapr_dt_pci_device() has a few things it only adds for non-bridges, but
> > always adds #address-cells and #size-cells which should only appear for
> > bridges.  But the walking down the subordinate PCI bus is done in one of
> > its callers spapr_populate_pci_devices_dt().  The PHB dt creation in
> > spapr_populate_pci_dt() open codes some similar logic to the bridge case.
> > 
> > This patch consolidates things in a bunch of ways:
> >  * Bus specific dt info is now created in spapr_dt_pci_bus() used for both
> >    P2P bridges and the host bridge.  This includes walking subordinate
> >    devices
> >  * spapr_dt_pci_device() now calls spapr_dt_pci_bus() when called on a
> >    P2P bridge
> >  * We do detection of bridges with the is_bridge field of the device class,
> >    rather than checking PCI config space directly, for consistency with
> >    qemu's core PCI code.
> >  * Several things are renamed for brevity and clarity
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c              |   7 +-
> >  hw/ppc/spapr_pci.c          | 140 +++++++++++++++++++-----------------
> >  include/hw/pci-host/spapr.h |   4 +-
> >  3 files changed, 79 insertions(+), 72 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e2b33e5890..44573adce7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1309,8 +1309,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
> >      }
> >  
> >      QLIST_FOREACH(phb, &spapr->phbs, list) {
> > -        ret = spapr_populate_pci_dt(phb, PHANDLE_INTC, fdt,
> > -                                    spapr->irq->nr_msis, NULL);
> > +        ret = spapr_dt_phb(phb, PHANDLE_INTC, fdt, spapr->irq->nr_msis, NULL);
> >          if (ret < 0) {
> >              error_report("couldn't setup PCI devices in fdt");
> >              exit(1);
> > @@ -3917,8 +3916,8 @@ int spapr_phb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> >          return -1;
> >      }
> >  
> > -    if (spapr_populate_pci_dt(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> > -                              fdt_start_offset)) {
> > +    if (spapr_dt_phb(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> > +                     fdt_start_offset)) {
> >          error_setg(errp, "unable to create FDT node for PHB %d", sphb->index);
> >          return -1;
> >      }
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 4075b433fd..c166df4145 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1219,6 +1219,60 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> >  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> >                                              PCIDevice *pdev);
> >  
> > +typedef struct PciWalkFdt {
> > +    void *fdt;
> > +    int offset;
> > +    SpaprPhbState *sphb;
> > +    int err;
> > +} PciWalkFdt;
> > +
> > +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> > +                               void *fdt, int parent_offset);
> > +
> > +static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
> > +                                   void *opaque)
> > +{
> > +    PciWalkFdt *p = opaque;
> > +    int err;
> > +
> > +    if (p->err) {
> > +        /* Something's already broken, don't keep going */
> > +        return;
> > +    }
> > +
> > +    err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
> > +    if (err < 0) {
> > +        p->err = err;
> > +    }
> > +}
> > +
> > +/* Augment PCI device node with bridge specific information */
> > +static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
> > +                               void *fdt, int offset)
> > +{
> > +    PciWalkFdt cbinfo = {
> > +        .fdt = fdt,
> > +        .offset = offset,
> > +        .sphb = sphb,
> > +        .err = 0,
> > +    };
> > +
> > +    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> > +                          RESOURCE_CELLS_ADDRESS));
> > +    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> > +                          RESOURCE_CELLS_SIZE));
> > +
> > +    if (bus) {
> > +        pci_for_each_device_reverse(bus, pci_bus_num(bus),
> > +                                    spapr_dt_pci_device_cb, &cbinfo);
> > +        if (cbinfo.err) {
> > +            return cbinfo.err;
> > +        }
> > +    }
> > +
> > +    return offset;
> 
> 
> This spapr_dt_pci_bus() returns 0 or an offset or an error.
> 
> But:
> spapr_dt_phb() returns 0 or error; and so does spapr_dt_drc().
> 
> Not extremely consistent.

No, it's not.  But the inconsistency is already there between the
device function which needs to return an offset and the PHB function
and others which don't.

I have some ideas for how to clean this up, along with a bunch of
other dt creation stuff, but I don't think it's reasonably in scope
for this series.

> It looks like spapr_dt_pci_bus() does not need to return @offset as it
> does not change it and the caller - spapr_dt_pci_device() - can have 1
> "return offset" in the end.

True, but changing this here just shuffles the inconsistency around
without really improving it.  I've put cleaning this up more widely on
my list of things to tackle when I have time.

> 
> 
> > +}
> > +
> >  /* create OF node for pci device and required OF DT properties */
> >  static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >                                 void *fdt, int parent_offset)
> > @@ -1228,10 +1282,9 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >      char *nodename;
> >      int slot = PCI_SLOT(dev->devfn);
> >      int func = PCI_FUNC(dev->devfn);
> > +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> >      ResourceProps rp;
> >      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> > -    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
> > -    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
> >      uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
> >      uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
> >      uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
> > @@ -1268,13 +1321,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >          _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
> >      }
> >  
> > -    if (!is_bridge) {
> > -        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> > -        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> > -        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> > -        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> > -    }
> > -
> >      if (subsystem_id) {
> >          _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
> >      }
> > @@ -1309,11 +1355,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >          _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> >      }
> >  
> > -    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> > -                          RESOURCE_CELLS_ADDRESS));
> > -    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> > -                          RESOURCE_CELLS_SIZE));
> > -
> >      if (msi_present(dev)) {
> >          uint32_t max_msi = msi_nr_vectors_allocated(dev);
> >          if (max_msi) {
> > @@ -1338,7 +1379,18 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >  
> >      spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
> >  
> > -    return offset;
> > +    if (!pc->is_bridge) {
> > +        /* Properties only for non-bridges */
> > +        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> > +        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> > +        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> > +        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> > +        return offset;
> > +    } else {
> > +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> > +
> > +        return spapr_dt_pci_bus(sphb, sec_bus, fdt, offset);
> > +    }
> >  }
> >  
> >  /* Callback to be called during DRC release. */
> > @@ -2063,44 +2115,6 @@ static const TypeInfo spapr_phb_info = {
> >      }
> >  };
> >  
> > -typedef struct SpaprFdt {
> > -    void *fdt;
> > -    int node_off;
> > -    SpaprPhbState *sphb;
> > -} SpaprFdt;
> > -
> > -static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> > -                                          void *opaque)
> > -{
> > -    PCIBus *sec_bus;
> > -    SpaprFdt *p = opaque;
> > -    int offset;
> > -    SpaprFdt s_fdt;
> > -
> > -    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
> > -    if (!offset) {
> > -        error_report("Failed to create pci child device tree node");
> > -        return;
> > -    }
> > -
> > -    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
> > -         PCI_HEADER_TYPE_BRIDGE)) {
> > -        return;
> > -    }
> > -
> > -    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> > -    if (!sec_bus) {
> > -        return;
> > -    }
> > -
> > -    s_fdt.fdt = p->fdt;
> > -    s_fdt.node_off = offset;
> > -    s_fdt.sphb = p->sphb;
> > -    pci_for_each_device_reverse(sec_bus, pci_bus_num(sec_bus),
> > -                                spapr_populate_pci_devices_dt,
> > -                                &s_fdt);
> > -}
> > -
> >  static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
> >                                             void *opaque)
> >  {
> > @@ -2138,8 +2152,8 @@ static void spapr_phb_pci_enumerate(SpaprPhbState *phb)
> >  
> >  }
> >  
> > -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> > -                          uint32_t nr_msis, int *node_offset)
> > +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> > +                 uint32_t nr_msis, int *node_offset)
> >  {
> >      int bus_off, i, j, ret;
> >      uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
> > @@ -2186,8 +2200,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >                                  cpu_to_be32(0x0),
> >                                  cpu_to_be32(phb->numa_node)};
> >      SpaprTceTable *tcet;
> > -    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> > -    SpaprFdt s_fdt;
> >      SpaprDrc *drc;
> >      Error *errp = NULL;
> >  
> > @@ -2200,8 +2212,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >      /* Write PHB properties */
> >      _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
> >      _FDT(fdt_setprop_string(fdt, bus_off, "compatible", "IBM,Logical_PHB"));
> > -    _FDT(fdt_setprop_cell(fdt, bus_off, "#address-cells", 0x3));
> > -    _FDT(fdt_setprop_cell(fdt, bus_off, "#size-cells", 0x2));
> >      _FDT(fdt_setprop_cell(fdt, bus_off, "#interrupt-cells", 0x1));
> >      _FDT(fdt_setprop(fdt, bus_off, "used-by-rtas", NULL, 0));
> >      _FDT(fdt_setprop(fdt, bus_off, "bus-range", &bus_range, sizeof(bus_range)));
> > @@ -2266,13 +2276,11 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >      spapr_phb_pci_enumerate(phb);
> >      _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
> >  
> > -    /* Populate tree nodes with PCI devices attached */
> > -    s_fdt.fdt = fdt;
> > -    s_fdt.node_off = bus_off;
> > -    s_fdt.sphb = phb;
> > -    pci_for_each_device_reverse(bus, pci_bus_num(bus),
> > -                                spapr_populate_pci_devices_dt,
> > -                                &s_fdt);
> > +    /* Walk the bridge and subordinate buses */
> > +    ret = spapr_dt_pci_bus(phb, PCI_HOST_BRIDGE(phb)->bus, fdt, bus_off);
> > +    if (ret) {
> 
> 
> if (ret < 0)
> 
> otherwise it returns prematurely and NVLink stuff does not make it to
> the FDT.
> 
> 
> Otherwise the whole patchset is a good cleanup and seems working fine.
> 
> 
> > +        return ret;
> > +    }
> >  
> >      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
> >                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
> > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > index 53519c835e..1b61162f91 100644
> > --- a/include/hw/pci-host/spapr.h
> > +++ b/include/hw/pci-host/spapr.h
> > @@ -131,8 +131,8 @@ static inline qemu_irq spapr_phb_lsi_qirq(struct SpaprPhbState *phb, int pin)
> >      return spapr_qirq(spapr, phb->lsi_table[pin].irq);
> >  }
> >  
> > -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> > -                          uint32_t nr_msis, int *node_offset);
> > +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> > +                 uint32_t nr_msis, int *node_offset);
> >  
> >  void spapr_pci_rtas_init(void);
> >  
> > 
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/8] spapr: Clean up dt creation for PCI buses

[Alexey Kardashevskiy]

On 30/05/2019 15:33, David Gibson wrote:
> On Fri, May 24, 2019 at 03:31:54PM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 23/05/2019 15:29, David Gibson wrote:
>>> Device nodes for PCI bridges (both host and P2P) describe both the bridge
>>> device itself and the bus hanging off it, handling of this is a bit of a
>>> mess.
>>>
>>> spapr_dt_pci_device() has a few things it only adds for non-bridges, but
>>> always adds #address-cells and #size-cells which should only appear for
>>> bridges.  But the walking down the subordinate PCI bus is done in one of
>>> its callers spapr_populate_pci_devices_dt().  The PHB dt creation in
>>> spapr_populate_pci_dt() open codes some similar logic to the bridge case.
>>>
>>> This patch consolidates things in a bunch of ways:
>>>  * Bus specific dt info is now created in spapr_dt_pci_bus() used for both
>>>    P2P bridges and the host bridge.  This includes walking subordinate
>>>    devices
>>>  * spapr_dt_pci_device() now calls spapr_dt_pci_bus() when called on a
>>>    P2P bridge
>>>  * We do detection of bridges with the is_bridge field of the device class,
>>>    rather than checking PCI config space directly, for consistency with
>>>    qemu's core PCI code.
>>>  * Several things are renamed for brevity and clarity
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  hw/ppc/spapr.c              |   7 +-
>>>  hw/ppc/spapr_pci.c          | 140 +++++++++++++++++++-----------------
>>>  include/hw/pci-host/spapr.h |   4 +-
>>>  3 files changed, 79 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index e2b33e5890..44573adce7 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -1309,8 +1309,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
>>>      }
>>>  
>>>      QLIST_FOREACH(phb, &spapr->phbs, list) {
>>> -        ret = spapr_populate_pci_dt(phb, PHANDLE_INTC, fdt,
>>> -                                    spapr->irq->nr_msis, NULL);
>>> +        ret = spapr_dt_phb(phb, PHANDLE_INTC, fdt, spapr->irq->nr_msis, NULL);
>>>          if (ret < 0) {
>>>              error_report("couldn't setup PCI devices in fdt");
>>>              exit(1);
>>> @@ -3917,8 +3916,8 @@ int spapr_phb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>>>          return -1;
>>>      }
>>>  
>>> -    if (spapr_populate_pci_dt(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
>>> -                              fdt_start_offset)) {
>>> +    if (spapr_dt_phb(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
>>> +                     fdt_start_offset)) {
>>>          error_setg(errp, "unable to create FDT node for PHB %d", sphb->index);
>>>          return -1;
>>>      }
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 4075b433fd..c166df4145 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1219,6 +1219,60 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
>>>  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
>>>                                              PCIDevice *pdev);
>>>  
>>> +typedef struct PciWalkFdt {
>>> +    void *fdt;
>>> +    int offset;
>>> +    SpaprPhbState *sphb;
>>> +    int err;
>>> +} PciWalkFdt;
>>> +
>>> +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>>> +                               void *fdt, int parent_offset);
>>> +
>>> +static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
>>> +                                   void *opaque)
>>> +{
>>> +    PciWalkFdt *p = opaque;
>>> +    int err;
>>> +
>>> +    if (p->err) {
>>> +        /* Something's already broken, don't keep going */
>>> +        return;
>>> +    }
>>> +
>>> +    err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
>>> +    if (err < 0) {
>>> +        p->err = err;
>>> +    }
>>> +}
>>> +
>>> +/* Augment PCI device node with bridge specific information */
>>> +static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
>>> +                               void *fdt, int offset)
>>> +{
>>> +    PciWalkFdt cbinfo = {
>>> +        .fdt = fdt,
>>> +        .offset = offset,
>>> +        .sphb = sphb,
>>> +        .err = 0,
>>> +    };
>>> +
>>> +    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
>>> +                          RESOURCE_CELLS_ADDRESS));
>>> +    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
>>> +                          RESOURCE_CELLS_SIZE));
>>> +
>>> +    if (bus) {
>>> +        pci_for_each_device_reverse(bus, pci_bus_num(bus),
>>> +                                    spapr_dt_pci_device_cb, &cbinfo);
>>> +        if (cbinfo.err) {
>>> +            return cbinfo.err;
>>> +        }
>>> +    }
>>> +
>>> +    return offset;
>>
>>
>> This spapr_dt_pci_bus() returns 0 or an offset or an error.
>>
>> But:
>> spapr_dt_phb() returns 0 or error; and so does spapr_dt_drc().
>>
>> Not extremely consistent.
> 
> No, it's not.  But the inconsistency is already there between the
> device function which needs to return an offset and the PHB function
> and others which don't.
> 
> I have some ideas for how to clean this up, along with a bunch of
> other dt creation stuff, but I don't think it's reasonably in scope
> for this series.
> 
>> It looks like spapr_dt_pci_bus() does not need to return @offset as it
>> does not change it and the caller - spapr_dt_pci_device() - can have 1
>> "return offset" in the end.
> 
> True, but changing this here just shuffles the inconsistency around
> without really improving it.  I've put cleaning this up more widely on
> my list of things to tackle when I have time.
> 
>>
>>
>>> +}
>>> +
>>>  /* create OF node for pci device and required OF DT properties */
>>>  static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>>>                                 void *fdt, int parent_offset)
>>> @@ -1228,10 +1282,9 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>>>      char *nodename;
>>>      int slot = PCI_SLOT(dev->devfn);
>>>      int func = PCI_FUNC(dev->devfn);
>>> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>>>      ResourceProps rp;
>>>      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
>>> -    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
>>> -    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
>>>      uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
>>>      uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
>>>      uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
>>> @@ -1268,13 +1321,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>>>          _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
>>>      }
>>>  
>>> -    if (!is_bridge) {
>>> -        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
>>> -        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
>>> -        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
>>> -        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
>>> -    }
>>> -
>>>      if (subsystem_id) {
>>>          _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
>>>      }
>>> @@ -1309,11 +1355,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>>>          _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>>>      }
>>>  
>>> -    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
>>> -                          RESOURCE_CELLS_ADDRESS));
>>> -    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
>>> -                          RESOURCE_CELLS_SIZE));
>>> -
>>>      if (msi_present(dev)) {
>>>          uint32_t max_msi = msi_nr_vectors_allocated(dev);
>>>          if (max_msi) {
>>> @@ -1338,7 +1379,18 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>>>  
>>>      spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
>>>  
>>> -    return offset;
>>> +    if (!pc->is_bridge) {
>>> +        /* Properties only for non-bridges */
>>> +        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
>>> +        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
>>> +        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
>>> +        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
>>> +        return offset;
>>> +    } else {
>>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
>>> +
>>> +        return spapr_dt_pci_bus(sphb, sec_bus, fdt, offset);
>>> +    }
>>>  }
>>>  
>>>  /* Callback to be called during DRC release. */
>>> @@ -2063,44 +2115,6 @@ static const TypeInfo spapr_phb_info = {
>>>      }
>>>  };
>>>  
>>> -typedef struct SpaprFdt {
>>> -    void *fdt;
>>> -    int node_off;
>>> -    SpaprPhbState *sphb;
>>> -} SpaprFdt;
>>> -
>>> -static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>>> -                                          void *opaque)
>>> -{
>>> -    PCIBus *sec_bus;
>>> -    SpaprFdt *p = opaque;
>>> -    int offset;
>>> -    SpaprFdt s_fdt;
>>> -
>>> -    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
>>> -    if (!offset) {
>>> -        error_report("Failed to create pci child device tree node");
>>> -        return;
>>> -    }
>>> -
>>> -    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
>>> -         PCI_HEADER_TYPE_BRIDGE)) {
>>> -        return;
>>> -    }
>>> -
>>> -    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>>> -    if (!sec_bus) {
>>> -        return;
>>> -    }
>>> -
>>> -    s_fdt.fdt = p->fdt;
>>> -    s_fdt.node_off = offset;
>>> -    s_fdt.sphb = p->sphb;
>>> -    pci_for_each_device_reverse(sec_bus, pci_bus_num(sec_bus),
>>> -                                spapr_populate_pci_devices_dt,
>>> -                                &s_fdt);
>>> -}
>>> -
>>>  static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
>>>                                             void *opaque)
>>>  {
>>> @@ -2138,8 +2152,8 @@ static void spapr_phb_pci_enumerate(SpaprPhbState *phb)
>>>  
>>>  }
>>>  
>>> -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>>> -                          uint32_t nr_msis, int *node_offset)
>>> +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>>> +                 uint32_t nr_msis, int *node_offset)
>>>  {
>>>      int bus_off, i, j, ret;
>>>      uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
>>> @@ -2186,8 +2200,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>>>                                  cpu_to_be32(0x0),
>>>                                  cpu_to_be32(phb->numa_node)};
>>>      SpaprTceTable *tcet;
>>> -    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
>>> -    SpaprFdt s_fdt;
>>>      SpaprDrc *drc;
>>>      Error *errp = NULL;
>>>  
>>> @@ -2200,8 +2212,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>>>      /* Write PHB properties */
>>>      _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
>>>      _FDT(fdt_setprop_string(fdt, bus_off, "compatible", "IBM,Logical_PHB"));
>>> -    _FDT(fdt_setprop_cell(fdt, bus_off, "#address-cells", 0x3));
>>> -    _FDT(fdt_setprop_cell(fdt, bus_off, "#size-cells", 0x2));
>>>      _FDT(fdt_setprop_cell(fdt, bus_off, "#interrupt-cells", 0x1));
>>>      _FDT(fdt_setprop(fdt, bus_off, "used-by-rtas", NULL, 0));
>>>      _FDT(fdt_setprop(fdt, bus_off, "bus-range", &bus_range, sizeof(bus_range)));
>>> @@ -2266,13 +2276,11 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>>>      spapr_phb_pci_enumerate(phb);
>>>      _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
>>>  
>>> -    /* Populate tree nodes with PCI devices attached */
>>> -    s_fdt.fdt = fdt;
>>> -    s_fdt.node_off = bus_off;
>>> -    s_fdt.sphb = phb;
>>> -    pci_for_each_device_reverse(bus, pci_bus_num(bus),
>>> -                                spapr_populate_pci_devices_dt,
>>> -                                &s_fdt);
>>> +    /* Walk the bridge and subordinate buses */
>>> +    ret = spapr_dt_pci_bus(phb, PCI_HOST_BRIDGE(phb)->bus, fdt, bus_off);
>>> +    if (ret) {
>>
>>
>> if (ret < 0)
>>
>> otherwise it returns prematurely and NVLink stuff does not make it to
>> the FDT.
>>
>>
>> Otherwise the whole patchset is a good cleanup and seems working fine.


Just to double check you have not missed this bit :)



-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/8] spapr: Clean up dt creation for PCI buses

[David Gibson]

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

On Thu, May 30, 2019 at 03:43:26PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 30/05/2019 15:33, David Gibson wrote:
> > On Fri, May 24, 2019 at 03:31:54PM +1000, Alexey Kardashevskiy wrote:
> >>
> >>
> >> On 23/05/2019 15:29, David Gibson wrote:
> >>> Device nodes for PCI bridges (both host and P2P) describe both the bridge
> >>> device itself and the bus hanging off it, handling of this is a bit of a
> >>> mess.
> >>>
> >>> spapr_dt_pci_device() has a few things it only adds for non-bridges, but
> >>> always adds #address-cells and #size-cells which should only appear for
> >>> bridges.  But the walking down the subordinate PCI bus is done in one of
> >>> its callers spapr_populate_pci_devices_dt().  The PHB dt creation in
> >>> spapr_populate_pci_dt() open codes some similar logic to the bridge case.
> >>>
> >>> This patch consolidates things in a bunch of ways:
> >>>  * Bus specific dt info is now created in spapr_dt_pci_bus() used for both
> >>>    P2P bridges and the host bridge.  This includes walking subordinate
> >>>    devices
> >>>  * spapr_dt_pci_device() now calls spapr_dt_pci_bus() when called on a
> >>>    P2P bridge
> >>>  * We do detection of bridges with the is_bridge field of the device class,
> >>>    rather than checking PCI config space directly, for consistency with
> >>>    qemu's core PCI code.
> >>>  * Several things are renamed for brevity and clarity
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>> ---
> >>>  hw/ppc/spapr.c              |   7 +-
> >>>  hw/ppc/spapr_pci.c          | 140 +++++++++++++++++++-----------------
> >>>  include/hw/pci-host/spapr.h |   4 +-
> >>>  3 files changed, 79 insertions(+), 72 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index e2b33e5890..44573adce7 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -1309,8 +1309,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
> >>>      }
> >>>  
> >>>      QLIST_FOREACH(phb, &spapr->phbs, list) {
> >>> -        ret = spapr_populate_pci_dt(phb, PHANDLE_INTC, fdt,
> >>> -                                    spapr->irq->nr_msis, NULL);
> >>> +        ret = spapr_dt_phb(phb, PHANDLE_INTC, fdt, spapr->irq->nr_msis, NULL);
> >>>          if (ret < 0) {
> >>>              error_report("couldn't setup PCI devices in fdt");
> >>>              exit(1);
> >>> @@ -3917,8 +3916,8 @@ int spapr_phb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> >>>          return -1;
> >>>      }
> >>>  
> >>> -    if (spapr_populate_pci_dt(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> >>> -                              fdt_start_offset)) {
> >>> +    if (spapr_dt_phb(sphb, intc_phandle, fdt, spapr->irq->nr_msis,
> >>> +                     fdt_start_offset)) {
> >>>          error_setg(errp, "unable to create FDT node for PHB %d", sphb->index);
> >>>          return -1;
> >>>      }
> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >>> index 4075b433fd..c166df4145 100644
> >>> --- a/hw/ppc/spapr_pci.c
> >>> +++ b/hw/ppc/spapr_pci.c
> >>> @@ -1219,6 +1219,60 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> >>>  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> >>>                                              PCIDevice *pdev);
> >>>  
> >>> +typedef struct PciWalkFdt {
> >>> +    void *fdt;
> >>> +    int offset;
> >>> +    SpaprPhbState *sphb;
> >>> +    int err;
> >>> +} PciWalkFdt;
> >>> +
> >>> +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >>> +                               void *fdt, int parent_offset);
> >>> +
> >>> +static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
> >>> +                                   void *opaque)
> >>> +{
> >>> +    PciWalkFdt *p = opaque;
> >>> +    int err;
> >>> +
> >>> +    if (p->err) {
> >>> +        /* Something's already broken, don't keep going */
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
> >>> +    if (err < 0) {
> >>> +        p->err = err;
> >>> +    }
> >>> +}
> >>> +
> >>> +/* Augment PCI device node with bridge specific information */
> >>> +static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
> >>> +                               void *fdt, int offset)
> >>> +{
> >>> +    PciWalkFdt cbinfo = {
> >>> +        .fdt = fdt,
> >>> +        .offset = offset,
> >>> +        .sphb = sphb,
> >>> +        .err = 0,
> >>> +    };
> >>> +
> >>> +    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> >>> +                          RESOURCE_CELLS_ADDRESS));
> >>> +    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> >>> +                          RESOURCE_CELLS_SIZE));
> >>> +
> >>> +    if (bus) {
> >>> +        pci_for_each_device_reverse(bus, pci_bus_num(bus),
> >>> +                                    spapr_dt_pci_device_cb, &cbinfo);
> >>> +        if (cbinfo.err) {
> >>> +            return cbinfo.err;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    return offset;
> >>
> >>
> >> This spapr_dt_pci_bus() returns 0 or an offset or an error.
> >>
> >> But:
> >> spapr_dt_phb() returns 0 or error; and so does spapr_dt_drc().
> >>
> >> Not extremely consistent.
> > 
> > No, it's not.  But the inconsistency is already there between the
> > device function which needs to return an offset and the PHB function
> > and others which don't.
> > 
> > I have some ideas for how to clean this up, along with a bunch of
> > other dt creation stuff, but I don't think it's reasonably in scope
> > for this series.
> > 
> >> It looks like spapr_dt_pci_bus() does not need to return @offset as it
> >> does not change it and the caller - spapr_dt_pci_device() - can have 1
> >> "return offset" in the end.
> > 
> > True, but changing this here just shuffles the inconsistency around
> > without really improving it.  I've put cleaning this up more widely on
> > my list of things to tackle when I have time.
> > 
> >>
> >>
> >>> +}
> >>> +
> >>>  /* create OF node for pci device and required OF DT properties */
> >>>  static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >>>                                 void *fdt, int parent_offset)
> >>> @@ -1228,10 +1282,9 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >>>      char *nodename;
> >>>      int slot = PCI_SLOT(dev->devfn);
> >>>      int func = PCI_FUNC(dev->devfn);
> >>> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> >>>      ResourceProps rp;
> >>>      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> >>> -    uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
> >>> -    bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
> >>>      uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
> >>>      uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
> >>>      uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
> >>> @@ -1268,13 +1321,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >>>          _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
> >>>      }
> >>>  
> >>> -    if (!is_bridge) {
> >>> -        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> >>> -        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> >>> -        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> >>> -        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> >>> -    }
> >>> -
> >>>      if (subsystem_id) {
> >>>          _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
> >>>      }
> >>> @@ -1309,11 +1355,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >>>          _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> >>>      }
> >>>  
> >>> -    _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> >>> -                          RESOURCE_CELLS_ADDRESS));
> >>> -    _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> >>> -                          RESOURCE_CELLS_SIZE));
> >>> -
> >>>      if (msi_present(dev)) {
> >>>          uint32_t max_msi = msi_nr_vectors_allocated(dev);
> >>>          if (max_msi) {
> >>> @@ -1338,7 +1379,18 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> >>>  
> >>>      spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
> >>>  
> >>> -    return offset;
> >>> +    if (!pc->is_bridge) {
> >>> +        /* Properties only for non-bridges */
> >>> +        uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> >>> +        uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> >>> +        _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> >>> +        _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> >>> +        return offset;
> >>> +    } else {
> >>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> >>> +
> >>> +        return spapr_dt_pci_bus(sphb, sec_bus, fdt, offset);
> >>> +    }
> >>>  }
> >>>  
> >>>  /* Callback to be called during DRC release. */
> >>> @@ -2063,44 +2115,6 @@ static const TypeInfo spapr_phb_info = {
> >>>      }
> >>>  };
> >>>  
> >>> -typedef struct SpaprFdt {
> >>> -    void *fdt;
> >>> -    int node_off;
> >>> -    SpaprPhbState *sphb;
> >>> -} SpaprFdt;
> >>> -
> >>> -static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> >>> -                                          void *opaque)
> >>> -{
> >>> -    PCIBus *sec_bus;
> >>> -    SpaprFdt *p = opaque;
> >>> -    int offset;
> >>> -    SpaprFdt s_fdt;
> >>> -
> >>> -    offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
> >>> -    if (!offset) {
> >>> -        error_report("Failed to create pci child device tree node");
> >>> -        return;
> >>> -    }
> >>> -
> >>> -    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
> >>> -         PCI_HEADER_TYPE_BRIDGE)) {
> >>> -        return;
> >>> -    }
> >>> -
> >>> -    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> >>> -    if (!sec_bus) {
> >>> -        return;
> >>> -    }
> >>> -
> >>> -    s_fdt.fdt = p->fdt;
> >>> -    s_fdt.node_off = offset;
> >>> -    s_fdt.sphb = p->sphb;
> >>> -    pci_for_each_device_reverse(sec_bus, pci_bus_num(sec_bus),
> >>> -                                spapr_populate_pci_devices_dt,
> >>> -                                &s_fdt);
> >>> -}
> >>> -
> >>>  static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
> >>>                                             void *opaque)
> >>>  {
> >>> @@ -2138,8 +2152,8 @@ static void spapr_phb_pci_enumerate(SpaprPhbState *phb)
> >>>  
> >>>  }
> >>>  
> >>> -int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >>> -                          uint32_t nr_msis, int *node_offset)
> >>> +int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >>> +                 uint32_t nr_msis, int *node_offset)
> >>>  {
> >>>      int bus_off, i, j, ret;
> >>>      uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
> >>> @@ -2186,8 +2200,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >>>                                  cpu_to_be32(0x0),
> >>>                                  cpu_to_be32(phb->numa_node)};
> >>>      SpaprTceTable *tcet;
> >>> -    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> >>> -    SpaprFdt s_fdt;
> >>>      SpaprDrc *drc;
> >>>      Error *errp = NULL;
> >>>  
> >>> @@ -2200,8 +2212,6 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >>>      /* Write PHB properties */
> >>>      _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
> >>>      _FDT(fdt_setprop_string(fdt, bus_off, "compatible", "IBM,Logical_PHB"));
> >>> -    _FDT(fdt_setprop_cell(fdt, bus_off, "#address-cells", 0x3));
> >>> -    _FDT(fdt_setprop_cell(fdt, bus_off, "#size-cells", 0x2));
> >>>      _FDT(fdt_setprop_cell(fdt, bus_off, "#interrupt-cells", 0x1));
> >>>      _FDT(fdt_setprop(fdt, bus_off, "used-by-rtas", NULL, 0));
> >>>      _FDT(fdt_setprop(fdt, bus_off, "bus-range", &bus_range, sizeof(bus_range)));
> >>> @@ -2266,13 +2276,11 @@ int spapr_populate_pci_dt(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
> >>>      spapr_phb_pci_enumerate(phb);
> >>>      _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
> >>>  
> >>> -    /* Populate tree nodes with PCI devices attached */
> >>> -    s_fdt.fdt = fdt;
> >>> -    s_fdt.node_off = bus_off;
> >>> -    s_fdt.sphb = phb;
> >>> -    pci_for_each_device_reverse(bus, pci_bus_num(bus),
> >>> -                                spapr_populate_pci_devices_dt,
> >>> -                                &s_fdt);
> >>> +    /* Walk the bridge and subordinate buses */
> >>> +    ret = spapr_dt_pci_bus(phb, PCI_HOST_BRIDGE(phb)->bus, fdt, bus_off);
> >>> +    if (ret) {
> >>
> >>
> >> if (ret < 0)
> >>
> >> otherwise it returns prematurely and NVLink stuff does not make it to
> >> the FDT.
> >>
> >>
> >> Otherwise the whole patchset is a good cleanup and seems working fine.
> 
> 
> Just to double check you have not missed this bit :)

Bother, I did miss this bit.  Fixed now.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 4/8] spapr: Clean up spapr_drc_populate_dt()

[David Gibson]

This makes some minor cleanups to spapr_drc_populate_dt(), renaming it to
the shorter and more idiomatic spapr_dt_drc() along the way.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c             |  7 +++----
 hw/ppc/spapr_drc.c         | 13 ++++++-------
 hw/ppc/spapr_pci.c         |  3 +--
 include/hw/ppc/spapr_drc.h |  3 +--
 4 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 44573adce7..507fd50dd5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1320,13 +1320,12 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
     spapr_populate_cpus_dt_node(fdt, spapr);
 
     if (smc->dr_lmb_enabled) {
-        _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
+        _FDT(spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
     }
 
     if (mc->has_hotpluggable_cpus) {
         int offset = fdt_path_offset(fdt, "/cpus");
-        ret = spapr_drc_populate_dt(fdt, offset, NULL,
-                                    SPAPR_DR_CONNECTOR_TYPE_CPU);
+        ret = spapr_dt_drc(fdt, offset, NULL, SPAPR_DR_CONNECTOR_TYPE_CPU);
         if (ret < 0) {
             error_report("Couldn't set up CPU DR device tree properties");
             exit(1);
@@ -1363,7 +1362,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
     }
 
     if (smc->dr_phb_enabled) {
-        ret = spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
+        ret = spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
         if (ret < 0) {
             error_report("Couldn't set up PHB DR device tree properties");
             exit(1);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 597f236b9c..bacadfcac5 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -781,7 +781,7 @@ SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id)
 }
 
 /**
- * spapr_drc_populate_dt
+ * spapr_dt_drc
  *
  * @fdt: libfdt device tree
  * @path: path in the DT to generate properties
@@ -794,8 +794,7 @@ SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id)
  *
  * as documented in PAPR+ v2.1, 13.5.2
  */
-int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
-                          uint32_t drc_type_mask)
+int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
 {
     Object *root_container;
     ObjectProperty *prop;
@@ -873,7 +872,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
     *(uint32_t *)drc_names->str = cpu_to_be32(drc_count);
     *(uint32_t *)drc_types->str = cpu_to_be32(drc_count);
 
-    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-indexes",
+    ret = fdt_setprop(fdt, offset, "ibm,drc-indexes",
                       drc_indexes->data,
                       drc_indexes->len * sizeof(uint32_t));
     if (ret) {
@@ -881,7 +880,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
         goto out;
     }
 
-    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-power-domains",
+    ret = fdt_setprop(fdt, offset, "ibm,drc-power-domains",
                       drc_power_domains->data,
                       drc_power_domains->len * sizeof(uint32_t));
     if (ret) {
@@ -889,14 +888,14 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
         goto out;
     }
 
-    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-names",
+    ret = fdt_setprop(fdt, offset, "ibm,drc-names",
                       drc_names->str, drc_names->len);
     if (ret) {
         error_report("Couldn't finalize ibm,drc-names property");
         goto out;
     }
 
-    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-types",
+    ret = fdt_setprop(fdt, offset, "ibm,drc-types",
                       drc_types->str, drc_types->len);
     if (ret) {
         error_report("Couldn't finalize ibm,drc-types property");
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index c166df4145..04855d3125 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2282,8 +2282,7 @@ int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
         return ret;
     }
 
-    ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
-                                SPAPR_DR_CONNECTOR_TYPE_PCI);
+    ret = spapr_dt_drc(fdt, bus_off, OBJECT(phb), SPAPR_DR_CONNECTOR_TYPE_PCI);
     if (ret) {
         return ret;
     }
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index fad0a887f9..c2c543a591 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -266,8 +266,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
                                          uint32_t id);
 SpaprDrc *spapr_drc_by_index(uint32_t index);
 SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
-int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
-                          uint32_t drc_type_mask);
+int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask);
 
 void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
 void spapr_drc_detach(SpaprDrc *drc);
-- 
2.21.0

Re: [Qemu-devel] [PATCH 4/8] spapr: Clean up spapr_drc_populate_dt()

[Greg Kurz]

On Thu, 23 May 2019 15:29:14 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> This makes some minor cleanups to spapr_drc_populate_dt(), renaming it to
> the shorter and more idiomatic spapr_dt_drc() along the way.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c             |  7 +++----
>  hw/ppc/spapr_drc.c         | 13 ++++++-------
>  hw/ppc/spapr_pci.c         |  3 +--
>  include/hw/ppc/spapr_drc.h |  3 +--
>  4 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 44573adce7..507fd50dd5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1320,13 +1320,12 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
>      spapr_populate_cpus_dt_node(fdt, spapr);
>  
>      if (smc->dr_lmb_enabled) {
> -        _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> +        _FDT(spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
>      }
>  
>      if (mc->has_hotpluggable_cpus) {
>          int offset = fdt_path_offset(fdt, "/cpus");
> -        ret = spapr_drc_populate_dt(fdt, offset, NULL,
> -                                    SPAPR_DR_CONNECTOR_TYPE_CPU);
> +        ret = spapr_dt_drc(fdt, offset, NULL, SPAPR_DR_CONNECTOR_TYPE_CPU);
>          if (ret < 0) {
>              error_report("Couldn't set up CPU DR device tree properties");
>              exit(1);
> @@ -1363,7 +1362,7 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
>      }
>  
>      if (smc->dr_phb_enabled) {
> -        ret = spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
> +        ret = spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
>          if (ret < 0) {
>              error_report("Couldn't set up PHB DR device tree properties");
>              exit(1);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 597f236b9c..bacadfcac5 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -781,7 +781,7 @@ SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id)
>  }
>  
>  /**
> - * spapr_drc_populate_dt
> + * spapr_dt_drc
>   *
>   * @fdt: libfdt device tree
>   * @path: path in the DT to generate properties
> @@ -794,8 +794,7 @@ SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id)
>   *
>   * as documented in PAPR+ v2.1, 13.5.2
>   */
> -int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> -                          uint32_t drc_type_mask)
> +int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
>  {
>      Object *root_container;
>      ObjectProperty *prop;
> @@ -873,7 +872,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>      *(uint32_t *)drc_names->str = cpu_to_be32(drc_count);
>      *(uint32_t *)drc_types->str = cpu_to_be32(drc_count);
>  
> -    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-indexes",
> +    ret = fdt_setprop(fdt, offset, "ibm,drc-indexes",
>                        drc_indexes->data,
>                        drc_indexes->len * sizeof(uint32_t));
>      if (ret) {
> @@ -881,7 +880,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>          goto out;
>      }
>  
> -    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-power-domains",
> +    ret = fdt_setprop(fdt, offset, "ibm,drc-power-domains",
>                        drc_power_domains->data,
>                        drc_power_domains->len * sizeof(uint32_t));
>      if (ret) {
> @@ -889,14 +888,14 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>          goto out;
>      }
>  
> -    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-names",
> +    ret = fdt_setprop(fdt, offset, "ibm,drc-names",
>                        drc_names->str, drc_names->len);
>      if (ret) {
>          error_report("Couldn't finalize ibm,drc-names property");
>          goto out;
>      }
>  
> -    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-types",
> +    ret = fdt_setprop(fdt, offset, "ibm,drc-types",
>                        drc_types->str, drc_types->len);
>      if (ret) {
>          error_report("Couldn't finalize ibm,drc-types property");
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c166df4145..04855d3125 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2282,8 +2282,7 @@ int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
>          return ret;
>      }
>  
> -    ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
> -                                SPAPR_DR_CONNECTOR_TYPE_PCI);
> +    ret = spapr_dt_drc(fdt, bus_off, OBJECT(phb), SPAPR_DR_CONNECTOR_TYPE_PCI);
>      if (ret) {
>          return ret;
>      }
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index fad0a887f9..c2c543a591 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -266,8 +266,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
>                                           uint32_t id);
>  SpaprDrc *spapr_drc_by_index(uint32_t index);
>  SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
> -int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> -                          uint32_t drc_type_mask);
> +int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask);
>  
>  void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
>  void spapr_drc_detach(SpaprDrc *drc);

[Qemu-devel] [PATCH 5/8] spapr: Clean up DRC index construction

[David Gibson]

spapr_pci.c currently has several confusingly similarly named functions for
various conversions between representations of DRCs.  Make things clearer
by renaming things in a more consistent XXX_from_YYY() manner and remove
some called-only-once variants in favour of open coding.

While we're at it, move this code together in the file to avoid some extra
forward references, and split out construction and removal of DRCs for the
host bridge into helper functions.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 124 +++++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 56 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 04855d3125..f799f8b423 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1216,8 +1216,60 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
     return name;
 }
 
-static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
-                                            PCIDevice *pdev);
+/*
+ * DRC helper functions
+ */
+
+static uint32_t drc_id_from_devfn(SpaprPhbState *phb,
+                                  uint32_t busnr,
+                                  int32_t devfn)
+{
+    return (phb->index << 16) | (busnr << 8) | devfn;
+}
+
+static SpaprDrc *drc_from_devfn(SpaprPhbState *phb,
+                                uint32_t busnr, int32_t devfn)
+{
+    return spapr_drc_by_id(TYPE_SPAPR_DRC_PCI,
+                           drc_id_from_devfn(phb, busnr, devfn));
+}
+
+static SpaprDrc *drc_from_dev(SpaprPhbState *phb, PCIDevice *dev)
+{
+    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(dev))));
+    return drc_from_devfn(phb, busnr, dev->devfn);
+}
+
+static void add_drcs(SpaprPhbState *phb)
+{
+    int i;
+
+    if (!phb->dr_enabled) {
+        return;
+    }
+
+    for (i = 0; i < PCI_SLOT_MAX * PCI_FUNC_MAX; i++) {
+        spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
+                               drc_id_from_devfn(phb, 0, i));
+    }
+}
+
+static void remove_drcs(SpaprPhbState *phb)
+{
+    int i;
+
+    if (!phb->dr_enabled) {
+        return;
+    }
+
+    for (i = PCI_SLOT_MAX * PCI_FUNC_MAX - 1; i >= 0; i--) {
+        SpaprDrc *drc = drc_from_devfn(phb, 0, i);
+
+        if (drc) {
+            object_unparent(OBJECT(drc));
+        }
+    }
+}
 
 typedef struct PciWalkFdt {
     void *fdt;
@@ -1284,7 +1336,7 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
     int func = PCI_FUNC(dev->devfn);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
     ResourceProps rp;
-    uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
+    SpaprDrc *drc = drc_from_dev(sphb, dev);
     uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
     uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
     uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
@@ -1351,8 +1403,9 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
     _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", loc_code));
     g_free(loc_code);
 
-    if (drc_index) {
-        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    if (drc) {
+        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index",
+                              spapr_drc_index(drc)));
     }
 
     if (msi_present(dev)) {
@@ -1402,33 +1455,6 @@ void spapr_phb_remove_pci_device_cb(DeviceState *dev)
     object_unparent(OBJECT(dev));
 }
 
-static SpaprDrc *spapr_phb_get_pci_func_drc(SpaprPhbState *phb,
-                                                    uint32_t busnr,
-                                                    int32_t devfn)
-{
-    return spapr_drc_by_id(TYPE_SPAPR_DRC_PCI,
-                           (phb->index << 16) | (busnr << 8) | devfn);
-}
-
-static SpaprDrc *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_phb_get_pci_func_drc(phb, busnr, pdev->devfn);
-}
-
-static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
-                                            PCIDevice *pdev)
-{
-    SpaprDrc *drc = spapr_phb_get_pci_drc(phb, pdev);
-
-    if (!drc) {
-        return 0;
-    }
-
-    return spapr_drc_index(drc);
-}
-
 int spapr_pci_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
                           void *fdt, int *fdt_start_offset, Error **errp)
 {
@@ -1445,7 +1471,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
 {
     SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
     PCIDevice *pdev = PCI_DEVICE(plugged_dev);
-    SpaprDrc *drc = spapr_phb_get_pci_drc(phb, pdev);
+    SpaprDrc *drc = drc_from_dev(phb, pdev);
     Error *local_err = NULL;
     PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
     uint32_t slotnr = PCI_SLOT(pdev->devfn);
@@ -1496,8 +1522,8 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
             SpaprDrcClass *func_drck;
             SpaprDREntitySense state;
 
-            func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
-                                                  PCI_DEVFN(slotnr, i));
+            func_drc = drc_from_devfn(phb, pci_bus_num(bus),
+                                      PCI_DEVFN(slotnr, i));
             func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
             state = func_drck->dr_entity_sense(func_drc);
 
@@ -1533,7 +1559,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
 {
     SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
     PCIDevice *pdev = PCI_DEVICE(plugged_dev);
-    SpaprDrc *drc = spapr_phb_get_pci_drc(phb, pdev);
+    SpaprDrc *drc = drc_from_dev(phb, pdev);
 
     if (!phb->dr_enabled) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG,
@@ -1555,8 +1581,8 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
         /* ensure any other present functions are pending unplug */
         if (PCI_FUNC(pdev->devfn) == 0) {
             for (i = 1; i < 8; i++) {
-                func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
-                                                      PCI_DEVFN(slotnr, i));
+                func_drc = drc_from_devfn(phb, pci_bus_num(bus),
+                                          PCI_DEVFN(slotnr, i));
                 func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
                 state = func_drck->dr_entity_sense(func_drc);
                 if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
@@ -1577,8 +1603,8 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
          */
         if (PCI_FUNC(pdev->devfn) == 0) {
             for (i = 7; i >= 0; i--) {
-                func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
-                                                      PCI_DEVFN(slotnr, i));
+                func_drc = drc_from_devfn(phb, pci_bus_num(bus),
+                                          PCI_DEVFN(slotnr, i));
                 func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
                 state = func_drck->dr_entity_sense(func_drc);
                 if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
@@ -1626,16 +1652,7 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
         }
     }
 
-    if (sphb->dr_enabled) {
-        for (i = PCI_SLOT_MAX * 8 - 1; i >= 0; i--) {
-            SpaprDrc *drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PCI,
-                                                    (sphb->index << 16) | i);
-
-            if (drc) {
-                object_unparent(OBJECT(drc));
-            }
-        }
-    }
+    remove_drcs(sphb);
 
     for (i = PCI_NUM_PINS - 1; i >= 0; i--) {
         if (sphb->lsi_table[i].irq) {
@@ -1861,12 +1878,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     }
 
     /* allocate connectors for child PCI devices */
-    if (sphb->dr_enabled) {
-        for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
-            spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
-                                   (sphb->index << 16) | i);
-        }
-    }
+    add_drcs(sphb);
 
     /* DMA setup */
     for (i = 0; i < windows_supported; ++i) {
-- 
2.21.0

[Qemu-devel] [PATCH 6/8] spapr: Don't use bus number for building DRC ids

[David Gibson]

DRC ids are more or less arbitrary, as long as they're consistent.  For
PCI, we notionally build them from the phb's index along with PCI bus
number, slot and function number.

Using bus number is broken, however, because it can change if the guest
re-enumerates the PCI topology for whatever reason (e.g. due to hotplug
of a bridge, which we don't support yet but want to).

Fortunately, there's an alternative.  Bridges are required to have a unique
non-zero "chassis number" that we can use instead.  Adjust the code to
use that instead.

This looks like it would introduce a guest visible breaking change, but
in fact it does not because we don't yet ever use non-zero bus numbers.
Both chassis and bus number are always 0 for the root bus, so there's no
change for the existing cases.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 54 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f799f8b423..94691fcfc2 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1221,23 +1221,40 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
  */
 
 static uint32_t drc_id_from_devfn(SpaprPhbState *phb,
-                                  uint32_t busnr,
-                                  int32_t devfn)
+                                  uint8_t chassis, int32_t devfn)
 {
-    return (phb->index << 16) | (busnr << 8) | devfn;
+    return (phb->index << 16) | (chassis << 8) | devfn;
 }
 
 static SpaprDrc *drc_from_devfn(SpaprPhbState *phb,
-                                uint32_t busnr, int32_t devfn)
+                                uint8_t chassis, int32_t devfn)
 {
     return spapr_drc_by_id(TYPE_SPAPR_DRC_PCI,
-                           drc_id_from_devfn(phb, busnr, devfn));
+                           drc_id_from_devfn(phb, chassis, devfn));
+}
+
+static uint8_t chassis_from_bus(PCIBus *bus, Error **errp)
+{
+    if (pci_bus_is_root(bus)) {
+        return 0;
+    } else {
+        PCIDevice *bridge = pci_bridge_get_device(bus);
+
+        return object_property_get_uint(OBJECT(bridge), "chassis_nr", errp);
+    }
 }
 
 static SpaprDrc *drc_from_dev(SpaprPhbState *phb, PCIDevice *dev)
 {
-    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(dev))));
-    return drc_from_devfn(phb, busnr, dev->devfn);
+    Error *local_err = NULL;
+    uint8_t chassis = chassis_from_bus(pci_get_bus(dev), &local_err);
+
+    if (local_err) {
+        error_report_err(local_err);
+        return NULL;
+    }
+
+    return drc_from_devfn(phb, chassis, dev->devfn);
 }
 
 static void add_drcs(SpaprPhbState *phb)
@@ -1516,14 +1533,19 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
         spapr_drc_reset(drc);
     } else if (PCI_FUNC(pdev->devfn) == 0) {
         int i;
+        uint8_t chassis = chassis_from_bus(pci_get_bus(pdev), &local_err);
+
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
 
         for (i = 0; i < 8; i++) {
             SpaprDrc *func_drc;
             SpaprDrcClass *func_drck;
             SpaprDREntitySense state;
 
-            func_drc = drc_from_devfn(phb, pci_bus_num(bus),
-                                      PCI_DEVFN(slotnr, i));
+            func_drc = drc_from_devfn(phb, chassis, PCI_DEVFN(slotnr, i));
             func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
             state = func_drck->dr_entity_sense(func_drc);
 
@@ -1571,18 +1593,23 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
     g_assert(drc->dev == plugged_dev);
 
     if (!spapr_drc_unplug_requested(drc)) {
-        PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
         uint32_t slotnr = PCI_SLOT(pdev->devfn);
         SpaprDrc *func_drc;
         SpaprDrcClass *func_drck;
         SpaprDREntitySense state;
         int i;
+        Error *local_err = NULL;
+        uint8_t chassis = chassis_from_bus(pci_get_bus(pdev), &local_err);
+
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
 
         /* ensure any other present functions are pending unplug */
         if (PCI_FUNC(pdev->devfn) == 0) {
             for (i = 1; i < 8; i++) {
-                func_drc = drc_from_devfn(phb, pci_bus_num(bus),
-                                          PCI_DEVFN(slotnr, i));
+                func_drc = drc_from_devfn(phb, chassis, PCI_DEVFN(slotnr, i));
                 func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
                 state = func_drck->dr_entity_sense(func_drc);
                 if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
@@ -1603,8 +1630,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
          */
         if (PCI_FUNC(pdev->devfn) == 0) {
             for (i = 7; i >= 0; i--) {
-                func_drc = drc_from_devfn(phb, pci_bus_num(bus),
-                                          PCI_DEVFN(slotnr, i));
+                func_drc = drc_from_devfn(phb, chassis, PCI_DEVFN(slotnr, i));
                 func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
                 state = func_drck->dr_entity_sense(func_drc);
                 if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
-- 
2.21.0

[Qemu-devel] [PATCH 7/8] spapr: Direct all PCI hotplug to host bridge, rather than P2P bridge

[David Gibson]

A P2P bridge will attempt to handle the hotplug with SHPC, which doesn't
work in the PAPR environment.  Instead we want to direct all PCI hotplug
actions to the PAPR specific host bridge which will use the PAPR hotplug
mechanism.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 507fd50dd5..6dd8aaac33 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4094,6 +4094,17 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
         object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
         return HOTPLUG_HANDLER(machine);
     }
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        PCIDevice *pcidev = PCI_DEVICE(dev);
+        PCIBus *root = pci_device_root_bus(pcidev);
+        SpaprPhbState *phb =
+            (SpaprPhbState *)object_dynamic_cast(OBJECT(BUS(root)->parent),
+                                                 TYPE_SPAPR_PCI_HOST_BRIDGE);
+
+        if (phb) {
+            return HOTPLUG_HANDLER(phb);
+        }
+    }
     return NULL;
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH 8/8] spapr: Allow hot plug/unplug of PCI bridges and devices under PCI bridges

[David Gibson]

The pseries machine type already allows PCI hotplug and unplug via the
PAPR mechanism, but only on the root bus of each PHB.  This patch extends
this to allow PCI to PCI bridges to be hotplugged, and devices to be
hotplugged or unplugged under P2P bridges.

For now we disallow hot unplugging P2P bridges.  I tried doing that, but
haven't managed to get it working, I think due to some guest side problems
that need further investigation.

To do this we dynamically construct DRCs when bridges are hot (or cold)
added, which can in turn be used to hotplug devices under the bridge.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 115 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 94691fcfc2..a4d5e46525 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1257,30 +1257,53 @@ static SpaprDrc *drc_from_dev(SpaprPhbState *phb, PCIDevice *dev)
     return drc_from_devfn(phb, chassis, dev->devfn);
 }
 
-static void add_drcs(SpaprPhbState *phb)
+static void add_drcs(SpaprPhbState *phb, PCIBus *bus, Error **errp)
 {
+    Object *owner;
     int i;
+    uint8_t chassis;
+    Error *local_err = NULL;
 
     if (!phb->dr_enabled) {
         return;
     }
 
+    chassis = chassis_from_bus(bus, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (pci_bus_is_root(bus)) {
+        owner = OBJECT(phb);
+    } else {
+        owner = OBJECT(pci_bridge_get_device(bus));
+    }
+
     for (i = 0; i < PCI_SLOT_MAX * PCI_FUNC_MAX; i++) {
-        spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
-                               drc_id_from_devfn(phb, 0, i));
+        spapr_dr_connector_new(owner, TYPE_SPAPR_DRC_PCI,
+                               drc_id_from_devfn(phb, chassis, i));
     }
 }
 
-static void remove_drcs(SpaprPhbState *phb)
+static void remove_drcs(SpaprPhbState *phb, PCIBus *bus, Error **errp)
 {
     int i;
+    uint8_t chassis;
+    Error *local_err = NULL;
 
     if (!phb->dr_enabled) {
         return;
     }
 
+    chassis = chassis_from_bus(bus, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     for (i = PCI_SLOT_MAX * PCI_FUNC_MAX - 1; i >= 0; i--) {
-        SpaprDrc *drc = drc_from_devfn(phb, 0, i);
+        SpaprDrc *drc = drc_from_devfn(phb, chassis, i);
 
         if (drc) {
             object_unparent(OBJECT(drc));
@@ -1325,6 +1348,7 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
         .sphb = sphb,
         .err = 0,
     };
+    int ret;
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
                           RESOURCE_CELLS_ADDRESS));
@@ -1339,6 +1363,12 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus,
         }
     }
 
+    ret = spapr_dt_drc(fdt, offset, OBJECT(bus->parent_dev),
+                       SPAPR_DR_CONNECTOR_TYPE_PCI);
+    if (ret) {
+        return ret;
+    }
+
     return offset;
 }
 
@@ -1483,11 +1513,26 @@ int spapr_pci_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     return 0;
 }
 
+static void spapr_pci_bridge_plug(SpaprPhbState *phb,
+                                  PCIBridge *bridge,
+                                  Error **errp)
+{
+    Error *local_err = NULL;
+    PCIBus *bus = pci_bridge_get_sec_bus(bridge);
+
+    add_drcs(phb, bus, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
 static void spapr_pci_plug(HotplugHandler *plug_handler,
                            DeviceState *plugged_dev, Error **errp)
 {
     SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
     PCIDevice *pdev = PCI_DEVICE(plugged_dev);
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
     SpaprDrc *drc = drc_from_dev(phb, pdev);
     Error *local_err = NULL;
     PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
@@ -1509,6 +1554,14 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
 
     g_assert(drc);
 
+    if (pc->is_bridge) {
+        spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev), &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
     /* Following the QEMU convention used for PCIe multifunction
      * hotplug, we do not allow functions to be hotplugged to a
      * slot that already has function 0 present
@@ -1559,9 +1612,26 @@ out:
     error_propagate(errp, local_err);
 }
 
+static void spapr_pci_bridge_unplug(SpaprPhbState *phb,
+                                    PCIBridge *bridge,
+                                    Error **errp)
+{
+    Error *local_err = NULL;
+    PCIBus *bus = pci_bridge_get_sec_bus(bridge);
+
+    remove_drcs(phb, bus, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
 static void spapr_pci_unplug(HotplugHandler *plug_handler,
                              DeviceState *plugged_dev, Error **errp)
 {
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
+    SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
+
     /* some version guests do not wait for completion of a device
      * cleanup (generally done asynchronously by the kernel) before
      * signaling to QEMU that the device is safe, but instead sleep
@@ -1573,6 +1643,16 @@ static void spapr_pci_unplug(HotplugHandler *plug_handler,
      * an 'idle' state, as the device cleanup code expects.
      */
     pci_device_reset(PCI_DEVICE(plugged_dev));
+
+    if (pc->is_bridge) {
+        Error *local_err = NULL;
+        spapr_pci_bridge_unplug(phb, PCI_BRIDGE(plugged_dev), &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+        }
+        return;
+    }
+
     object_property_set_bool(OBJECT(plugged_dev), false, "realized", NULL);
 }
 
@@ -1593,6 +1673,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
     g_assert(drc->dev == plugged_dev);
 
     if (!spapr_drc_unplug_requested(drc)) {
+        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
         uint32_t slotnr = PCI_SLOT(pdev->devfn);
         SpaprDrc *func_drc;
         SpaprDrcClass *func_drck;
@@ -1606,6 +1687,10 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
             return;
         }
 
+        if (pc->is_bridge) {
+            error_setg(errp, "PCI: Hot unplug of PCI bridges not supported");
+        }
+
         /* ensure any other present functions are pending unplug */
         if (PCI_FUNC(pdev->devfn) == 0) {
             for (i = 1; i < 8; i++) {
@@ -1658,6 +1743,7 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
     SpaprTceTable *tcet;
     int i;
     const unsigned windows_supported = spapr_phb_windows_supported(sphb);
+    Error *local_err = NULL;
 
     spapr_phb_nvgpu_free(sphb);
 
@@ -1678,7 +1764,11 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
         }
     }
 
-    remove_drcs(sphb);
+    remove_drcs(sphb, phb->bus, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     for (i = PCI_NUM_PINS - 1; i >= 0; i--) {
         if (sphb->lsi_table[i].irq) {
@@ -1743,6 +1833,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     uint64_t msi_window_size = 4096;
     SpaprTceTable *tcet;
     const unsigned windows_supported = spapr_phb_windows_supported(sphb);
+    Error *local_err = NULL;
 
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
@@ -1879,7 +1970,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     /* Initialize the LSI table */
     for (i = 0; i < PCI_NUM_PINS; i++) {
         uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
-        Error *local_err = NULL;
 
         if (smc->legacy_irq_allocation) {
             irq = spapr_irq_findone(spapr, &local_err);
@@ -1904,7 +1994,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     }
 
     /* allocate connectors for child PCI devices */
-    add_drcs(sphb);
+    add_drcs(sphb, phb->bus, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto unrealize;
+    }
 
     /* DMA setup */
     for (i = 0; i < windows_supported; ++i) {
@@ -2320,11 +2414,6 @@ int spapr_dt_phb(SpaprPhbState *phb, uint32_t intc_phandle, void *fdt,
         return ret;
     }
 
-    ret = spapr_dt_drc(fdt, bus_off, OBJECT(phb), SPAPR_DR_CONNECTOR_TYPE_PCI);
-    if (ret) {
-        return ret;
-    }
-
     spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, &errp);
     if (errp) {
         error_report_err(errp);
-- 
2.21.0

Re: [Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices

[Greg Kurz]

On Thu, 23 May 2019 15:29:11 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> spapr_populate_pci_child_dt() adds a 'name' property to the device tree
> node for PCI devices.  This is never necessary for a flattened device tree,
> it is implicit in the name added when the node is constructed.  In fact
> anything we do add to a 'name' property will be overwritten with something
> derived from the structural name in the guest firmware (but in fact it is
> exactly the same bytes).
> 
> So, remove that.  In addition, pci_get_node_name() is very simple, so fold
> it into its (also simple) sole caller spapr_create_pci_child_dt().
> 
> While we're there rename pci_find_device_name() to the shorter and more
> accurate dt_name_from_class().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 97961b0128..b2db46ef1d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1173,8 +1173,8 @@ static const PCIClass pci_classes[] = {
>      { "data-processing-controller", spc_subclass },
>  };
>  
> -static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> -                                        uint8_t iface)
> +static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> +                                      uint8_t iface)
>  {
>      const PCIClass *pclass;
>      const PCISubClass *psubclass;
> @@ -1216,23 +1216,6 @@ static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
>      return name;
>  }
>  
> -static gchar *pci_get_node_name(PCIDevice *dev)
> -{
> -    int slot = PCI_SLOT(dev->devfn);
> -    int func = PCI_FUNC(dev->devfn);
> -    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> -    const char *name;
> -
> -    name = pci_find_device_name((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> -                                ccode & 0xff);
> -
> -    if (func != 0) {
> -        return g_strdup_printf("%s@%x,%x", name, slot, func);
> -    } else {
> -        return g_strdup_printf("%s@%x", name, slot);
> -    }
> -}
> -
>  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
>                                              PCIDevice *pdev);
>  
> @@ -1300,11 +1283,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>          _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
>      }
>  
> -    _FDT(fdt_setprop_string(fdt, offset, "name",
> -                            pci_find_device_name((ccode >> 16) & 0xff,
> -                                                 (ccode >> 8) & 0xff,
> -                                                 ccode & 0xff)));
> -
>      buf = spapr_phb_get_loc_code(sphb, dev);
>      _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
>      g_free(buf);
> @@ -1348,10 +1326,23 @@ static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
>                                       void *fdt, int node_offset)
>  {
>      int offset;
> -    gchar *nodename;
> +    const gchar *basename;
> +    char *nodename;

Not sure why you're changing nodename to be a char * instead of a gchar * ...

Apart from that, LGTM.

Reviewed-by: Greg Kurz <groug@kaod.org>

> +    int slot = PCI_SLOT(dev->devfn);
> +    int func = PCI_FUNC(dev->devfn);
> +    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> +
> +    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> +                                  ccode & 0xff);
> +
> +    if (func != 0) {
> +        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> +    } else {
> +        nodename = g_strdup_printf("%s@%x", basename, slot);
> +    }
>  
> -    nodename = pci_get_node_name(dev);
>      _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> +
>      g_free(nodename);
>  
>      spapr_populate_pci_child_dt(dev, fdt, offset, phb);

Re: [Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices

[David Gibson]

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

On Fri, May 24, 2019 at 03:32:19PM +0200, Greg Kurz wrote:
> On Thu, 23 May 2019 15:29:11 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > spapr_populate_pci_child_dt() adds a 'name' property to the device tree
> > node for PCI devices.  This is never necessary for a flattened device tree,
> > it is implicit in the name added when the node is constructed.  In fact
> > anything we do add to a 'name' property will be overwritten with something
> > derived from the structural name in the guest firmware (but in fact it is
> > exactly the same bytes).
> > 
> > So, remove that.  In addition, pci_get_node_name() is very simple, so fold
> > it into its (also simple) sole caller spapr_create_pci_child_dt().
> > 
> > While we're there rename pci_find_device_name() to the shorter and more
> > accurate dt_name_from_class().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c | 43 +++++++++++++++++--------------------------
> >  1 file changed, 17 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 97961b0128..b2db46ef1d 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1173,8 +1173,8 @@ static const PCIClass pci_classes[] = {
> >      { "data-processing-controller", spc_subclass },
> >  };
> >  
> > -static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> > -                                        uint8_t iface)
> > +static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> > +                                      uint8_t iface)
> >  {
> >      const PCIClass *pclass;
> >      const PCISubClass *psubclass;
> > @@ -1216,23 +1216,6 @@ static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> >      return name;
> >  }
> >  
> > -static gchar *pci_get_node_name(PCIDevice *dev)
> > -{
> > -    int slot = PCI_SLOT(dev->devfn);
> > -    int func = PCI_FUNC(dev->devfn);
> > -    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > -    const char *name;
> > -
> > -    name = pci_find_device_name((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > -                                ccode & 0xff);
> > -
> > -    if (func != 0) {
> > -        return g_strdup_printf("%s@%x,%x", name, slot, func);
> > -    } else {
> > -        return g_strdup_printf("%s@%x", name, slot);
> > -    }
> > -}
> > -
> >  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> >                                              PCIDevice *pdev);
> >  
> > @@ -1300,11 +1283,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >          _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
> >      }
> >  
> > -    _FDT(fdt_setprop_string(fdt, offset, "name",
> > -                            pci_find_device_name((ccode >> 16) & 0xff,
> > -                                                 (ccode >> 8) & 0xff,
> > -                                                 ccode & 0xff)));
> > -
> >      buf = spapr_phb_get_loc_code(sphb, dev);
> >      _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
> >      g_free(buf);
> > @@ -1348,10 +1326,23 @@ static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
> >                                       void *fdt, int node_offset)
> >  {
> >      int offset;
> > -    gchar *nodename;
> > +    const gchar *basename;
> > +    char *nodename;
> 
> Not sure why you're changing nodename to be a char * instead of a gchar * ...

Uh... by mistake.  I'll remove that.

> Apart from that, LGTM.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +    int slot = PCI_SLOT(dev->devfn);
> > +    int func = PCI_FUNC(dev->devfn);
> > +    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > +
> > +    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > +                                  ccode & 0xff);
> > +
> > +    if (func != 0) {
> > +        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> > +    } else {
> > +        nodename = g_strdup_printf("%s@%x", basename, slot);
> > +    }
> >  
> > -    nodename = pci_get_node_name(dev);
> >      _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> > +
> >      g_free(nodename);
> >  
> >      spapr_populate_pci_child_dt(dev, fdt, offset, phb);
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices

[Michael S. Tsirkin]

On Thu, May 23, 2019 at 03:29:11PM +1000, David Gibson wrote:
> spapr_populate_pci_child_dt() adds a 'name' property to the device tree
> node for PCI devices.  This is never necessary for a flattened device tree,
> it is implicit in the name added when the node is constructed.  In fact
> anything we do add to a 'name' property will be overwritten with something
> derived from the structural name in the guest firmware (but in fact it is
> exactly the same bytes).
> 
> So, remove that.  In addition, pci_get_node_name() is very simple, so fold
> it into its (also simple) sole caller spapr_create_pci_child_dt().
> 
> While we're there rename pci_find_device_name() to the shorter and more
> accurate dt_name_from_class().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

The threading is broken here btw.

I was CC'd but it's mostly PPC stuff.
I like how pci_XX functions that are not in pci.c are
going away :)

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  hw/ppc/spapr_pci.c | 43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 97961b0128..b2db46ef1d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1173,8 +1173,8 @@ static const PCIClass pci_classes[] = {
>      { "data-processing-controller", spc_subclass },
>  };
>  
> -static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> -                                        uint8_t iface)
> +static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> +                                      uint8_t iface)
>  {
>      const PCIClass *pclass;
>      const PCISubClass *psubclass;
> @@ -1216,23 +1216,6 @@ static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
>      return name;
>  }
>  
> -static gchar *pci_get_node_name(PCIDevice *dev)
> -{
> -    int slot = PCI_SLOT(dev->devfn);
> -    int func = PCI_FUNC(dev->devfn);
> -    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> -    const char *name;
> -
> -    name = pci_find_device_name((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> -                                ccode & 0xff);
> -
> -    if (func != 0) {
> -        return g_strdup_printf("%s@%x,%x", name, slot, func);
> -    } else {
> -        return g_strdup_printf("%s@%x", name, slot);
> -    }
> -}
> -
>  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
>                                              PCIDevice *pdev);
>  
> @@ -1300,11 +1283,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>          _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
>      }
>  
> -    _FDT(fdt_setprop_string(fdt, offset, "name",
> -                            pci_find_device_name((ccode >> 16) & 0xff,
> -                                                 (ccode >> 8) & 0xff,
> -                                                 ccode & 0xff)));
> -
>      buf = spapr_phb_get_loc_code(sphb, dev);
>      _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
>      g_free(buf);
> @@ -1348,10 +1326,23 @@ static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
>                                       void *fdt, int node_offset)
>  {
>      int offset;
> -    gchar *nodename;
> +    const gchar *basename;
> +    char *nodename;
> +    int slot = PCI_SLOT(dev->devfn);
> +    int func = PCI_FUNC(dev->devfn);
> +    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> +
> +    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> +                                  ccode & 0xff);
> +
> +    if (func != 0) {
> +        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> +    } else {
> +        nodename = g_strdup_printf("%s@%x", basename, slot);
> +    }
>  
> -    nodename = pci_get_node_name(dev);
>      _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> +
>      g_free(nodename);
>  
>      spapr_populate_pci_child_dt(dev, fdt, offset, phb);
> -- 
> 2.21.0

Re: [Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices

[Michael S. Tsirkin]

On Tue, May 28, 2019 at 11:23:54PM -0400, Michael S. Tsirkin wrote:
> On Thu, May 23, 2019 at 03:29:11PM +1000, David Gibson wrote:
> > spapr_populate_pci_child_dt() adds a 'name' property to the device tree
> > node for PCI devices.  This is never necessary for a flattened device tree,
> > it is implicit in the name added when the node is constructed.  In fact
> > anything we do add to a 'name' property will be overwritten with something
> > derived from the structural name in the guest firmware (but in fact it is
> > exactly the same bytes).
> > 
> > So, remove that.  In addition, pci_get_node_name() is very simple, so fold
> > it into its (also simple) sole caller spapr_create_pci_child_dt().
> > 
> > While we're there rename pci_find_device_name() to the shorter and more
> > accurate dt_name_from_class().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> The threading is broken here btw.
> 
> I was CC'd but it's mostly PPC stuff.
> I like how pci_XX functions that are not in pci.c are
> going away :)
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

and that's for the whole patchset.

> 
> > ---
> >  hw/ppc/spapr_pci.c | 43 +++++++++++++++++--------------------------
> >  1 file changed, 17 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 97961b0128..b2db46ef1d 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1173,8 +1173,8 @@ static const PCIClass pci_classes[] = {
> >      { "data-processing-controller", spc_subclass },
> >  };
> >  
> > -static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> > -                                        uint8_t iface)
> > +static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> > +                                      uint8_t iface)
> >  {
> >      const PCIClass *pclass;
> >      const PCISubClass *psubclass;
> > @@ -1216,23 +1216,6 @@ static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> >      return name;
> >  }
> >  
> > -static gchar *pci_get_node_name(PCIDevice *dev)
> > -{
> > -    int slot = PCI_SLOT(dev->devfn);
> > -    int func = PCI_FUNC(dev->devfn);
> > -    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > -    const char *name;
> > -
> > -    name = pci_find_device_name((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > -                                ccode & 0xff);
> > -
> > -    if (func != 0) {
> > -        return g_strdup_printf("%s@%x,%x", name, slot, func);
> > -    } else {
> > -        return g_strdup_printf("%s@%x", name, slot);
> > -    }
> > -}
> > -
> >  static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> >                                              PCIDevice *pdev);
> >  
> > @@ -1300,11 +1283,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >          _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
> >      }
> >  
> > -    _FDT(fdt_setprop_string(fdt, offset, "name",
> > -                            pci_find_device_name((ccode >> 16) & 0xff,
> > -                                                 (ccode >> 8) & 0xff,
> > -                                                 ccode & 0xff)));
> > -
> >      buf = spapr_phb_get_loc_code(sphb, dev);
> >      _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
> >      g_free(buf);
> > @@ -1348,10 +1326,23 @@ static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
> >                                       void *fdt, int node_offset)
> >  {
> >      int offset;
> > -    gchar *nodename;
> > +    const gchar *basename;
> > +    char *nodename;
> > +    int slot = PCI_SLOT(dev->devfn);
> > +    int func = PCI_FUNC(dev->devfn);
> > +    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > +
> > +    basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > +                                  ccode & 0xff);
> > +
> > +    if (func != 0) {
> > +        nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> > +    } else {
> > +        nodename = g_strdup_printf("%s@%x", basename, slot);
> > +    }
> >  
> > -    nodename = pci_get_node_name(dev);
> >      _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> > +
> >      g_free(nodename);
> >  
> >      spapr_populate_pci_child_dt(dev, fdt, offset, phb);
> > -- 
> > 2.21.0

Re: [Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices

[David Gibson]

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

On Tue, May 28, 2019 at 11:23:54PM -0400, Michael S. Tsirkin wrote:
> On Thu, May 23, 2019 at 03:29:11PM +1000, David Gibson wrote:
> > spapr_populate_pci_child_dt() adds a 'name' property to the device tree
> > node for PCI devices.  This is never necessary for a flattened device tree,
> > it is implicit in the name added when the node is constructed.  In fact
> > anything we do add to a 'name' property will be overwritten with something
> > derived from the structural name in the guest firmware (but in fact it is
> > exactly the same bytes).
> > 
> > So, remove that.  In addition, pci_get_node_name() is very simple, so fold
> > it into its (also simple) sole caller spapr_create_pci_child_dt().
> > 
> > While we're there rename pci_find_device_name() to the shorter and more
> > accurate dt_name_from_class().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> The threading is broken here btw.

I'm not entirely sure what you mean by that.  I did forget to add a
cover letter.

> I was CC'd but it's mostly PPC stuff.

Yeah, this was just for reference, since it is PCI stuff for ppc.

> I like how pci_XX functions that are not in pci.c are
> going away :)

You're welcome.

> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]