[PATCH 00/14] ppc/pnv: Add support for user created PHB3/PHB4 devices

[PATCH 00/14] ppc/pnv: Add support for user created PHB3/PHB4 devices

[Cédric Le Goater]

Hello,

On the POWER8 processor, powernv8 machine, PHB3 devices can simply be
created with :

   -device pnv-phb3,chip-id=0,index=1 

with a maximum of 3 PHB3s per chip, each PHB3 adding a new PCIe bus.

On the POWER9 processor, powernv9 machine, the logic is different. The
the chip comes with 3 PHB4 PECs (PCI Express Controller) and each PEC
can have several PHBs :

  * PEC0 provides 1 PHB  (PHB0)
  * PEC1 provides 2 PHBs (PHB1 and PHB2)
  * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)

The PEC devices can be created with :

   -device pnv-phb4-pec,chip-id=0,index=1

And the number of added PHB4 devices depends on the PEC index. Each
PHB4 adds a new PCIe bus.

The following changes are mostly cleanups and improvements of the
PHB3/4 realize routines to enable support. One important change is
related to the way the powernv machine populates the device tree. It
depends on the object hierarchy and it is necessary to reparent user
created devices to the chip they belong to (see PATCH 5). PHB3 is a
little more sophisticated because of its SysBusDevice nature (see
PATCH 6).

Thanks,

C.

Cédric Le Goater (14):
  ppc/pnv: Reduce the maximum of PHB3 devices
  ppc/pnv: Drop "num-phbs" property
  ppc/pnv: Move mapping of the PHB3 CQ regions under pnv_pbcq_realize()
  ppc/pnv: Introduce support for user created PHB3 devices
  ppc/pnv: Reparent user created PHB3 devices to the PnvChip
  ppc/pnv: Complete user created PHB3 devices
  ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices
  ppc/pnv: Introduce version and device_id class atributes for PHB4
    devices
  ppc/pnv: Introduce a "chip" property under the PHB4 model
  ppc/pnv: Introduce a num_stack class attribute
  ppc/pnv: Compute the PHB index from the PHB4 PEC model
  ppc/pnv: Remove "system-memory" property for he PHB4 PEC model
  ppc/pnv: Move realize of PEC stacks under the PEC model
  ppc/pnv: Introduce support for user created PHB4 devices

 include/hw/pci-host/pnv_phb3.h |   3 +
 include/hw/pci-host/pnv_phb4.h |   5 ++
 include/hw/ppc/pnv.h           |   7 +-
 hw/pci-host/pnv_phb3.c         |  27 ++++++-
 hw/pci-host/pnv_phb3_pbcq.c    |  11 +++
 hw/pci-host/pnv_phb4_pec.c     |  96 ++++++++++++++++++++++---
 hw/ppc/pnv.c                   | 125 ++++++++++++++-------------------
 7 files changed, 190 insertions(+), 84 deletions(-)

-- 
2.31.1

[PATCH 01/14] ppc/pnv: Reduce the maximum of PHB3 devices

[Cédric Le Goater]

All POWER8 machines have a maximum of 3 PHB3 devices. Adapt the
PNV8_CHIP_PHB3_MAX definition for consistency.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h | 2 +-
 hw/ppc/pnv.c         | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index aa08d79d24de..6f498c8f1b5f 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -79,7 +79,7 @@ struct Pnv8Chip {
     PnvOCC       occ;
     PnvHomer     homer;
 
-#define PNV8_CHIP_PHB3_MAX 4
+#define PNV8_CHIP_PHB3_MAX 3
     PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
 
     XICSFabric    *xics;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 71e45515f136..bd768dcc28ad 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1256,7 +1256,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
 
     k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
     k->cores_mask = POWER8E_CORE_MASK;
-    k->num_phbs = 3;
+    k->num_phbs = PNV8_CHIP_PHB3_MAX;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
     k->intc_reset = pnv_chip_power8_intc_reset;
@@ -1280,7 +1280,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
 
     k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
     k->cores_mask = POWER8_CORE_MASK;
-    k->num_phbs = 3;
+    k->num_phbs = PNV8_CHIP_PHB3_MAX;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
     k->intc_reset = pnv_chip_power8_intc_reset;
@@ -1304,7 +1304,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
 
     k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
     k->cores_mask = POWER8_CORE_MASK;
-    k->num_phbs = 3;
+    k->num_phbs = PNV8_CHIP_PHB3_MAX;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
     k->intc_reset = pnv_chip_power8_intc_reset;
-- 
2.31.1

[PATCH 02/14] ppc/pnv: Drop the "num-phbs" property

[Cédric Le Goater]

It is never used.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index bd768dcc28ad..988b305398b2 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1764,7 +1764,6 @@ static Property pnv_chip_properties[] = {
     DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1),
     DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0),
     DEFINE_PROP_UINT32("nr-threads", PnvChip, nr_threads, 1),
-    DEFINE_PROP_UINT32("num-phbs", PnvChip, num_phbs, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.31.1

[PATCH 03/14] ppc/pnv: Move mapping of the PHB3 CQ regions under pnv_pbcq_realize()

[Cédric Le Goater]

This requires a link to the chip to add the regions under the XSCOM
address space. This change will help us providing support for user
created PHB3 devices.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/pci-host/pnv_phb3.h |  3 +++
 hw/pci-host/pnv_phb3.c         |  1 +
 hw/pci-host/pnv_phb3_pbcq.c    | 11 +++++++++++
 hw/ppc/pnv.c                   | 14 ++------------
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
index e2a2e3624532..e9c13e6bd821 100644
--- a/include/hw/pci-host/pnv_phb3.h
+++ b/include/hw/pci-host/pnv_phb3.h
@@ -16,6 +16,7 @@
 #include "qom/object.h"
 
 typedef struct PnvPHB3 PnvPHB3;
+typedef struct PnvChip PnvChip;
 
 /*
  * PHB3 XICS Source for MSIs
@@ -157,6 +158,8 @@ struct PnvPHB3 {
     PnvPHB3RootPort root;
 
     QLIST_HEAD(, PnvPhb3DMASpace) dma_spaces;
+
+    PnvChip *chip;
 };
 
 uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, unsigned size);
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index a7f96850055a..3aa42ef9d4b9 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1092,6 +1092,7 @@ static const char *pnv_phb3_root_bus_path(PCIHostState *host_bridge,
 static Property pnv_phb3_properties[] = {
         DEFINE_PROP_UINT32("index", PnvPHB3, phb_id, 0),
         DEFINE_PROP_UINT32("chip-id", PnvPHB3, chip_id, 0),
+        DEFINE_PROP_LINK("chip", PnvPHB3, chip, TYPE_PNV_CHIP, PnvChip *),
         DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/pci-host/pnv_phb3_pbcq.c b/hw/pci-host/pnv_phb3_pbcq.c
index a0526aa1eca3..c7426cd27a20 100644
--- a/hw/pci-host/pnv_phb3_pbcq.c
+++ b/hw/pci-host/pnv_phb3_pbcq.c
@@ -284,6 +284,17 @@ static void pnv_pbcq_realize(DeviceState *dev, Error **errp)
     pnv_xscom_region_init(&pbcq->xscom_spci_regs, OBJECT(dev),
                           &pnv_pbcq_spci_xscom_ops, pbcq, name,
                           PNV_XSCOM_PBCQ_SPCI_SIZE);
+
+    /* Populate the XSCOM address space. */
+    pnv_xscom_add_subregion(phb->chip,
+                            PNV_XSCOM_PBCQ_NEST_BASE + 0x400 * phb->phb_id,
+                            &pbcq->xscom_nest_regs);
+    pnv_xscom_add_subregion(phb->chip,
+                            PNV_XSCOM_PBCQ_PCI_BASE + 0x400 * phb->phb_id,
+                            &pbcq->xscom_pci_regs);
+    pnv_xscom_add_subregion(phb->chip,
+                            PNV_XSCOM_PBCQ_SPCI_BASE + 0x040 * phb->phb_id,
+                            &pbcq->xscom_spci_regs);
 }
 
 static int pnv_pbcq_dt_xscom(PnvXScomInterface *dev, void *fdt,
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 988b305398b2..de277c457838 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1221,25 +1221,15 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
     /* PHB3 controllers */
     for (i = 0; i < chip->num_phbs; i++) {
         PnvPHB3 *phb = &chip8->phbs[i];
-        PnvPBCQState *pbcq = &phb->pbcq;
 
         object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
         object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
                                 &error_fatal);
+        object_property_set_link(OBJECT(phb), "chip", OBJECT(chip),
+                                 &error_fatal);
         if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
             return;
         }
-
-        /* Populate the XSCOM address space. */
-        pnv_xscom_add_subregion(chip,
-                                PNV_XSCOM_PBCQ_NEST_BASE + 0x400 * phb->phb_id,
-                                &pbcq->xscom_nest_regs);
-        pnv_xscom_add_subregion(chip,
-                                PNV_XSCOM_PBCQ_PCI_BASE + 0x400 * phb->phb_id,
-                                &pbcq->xscom_pci_regs);
-        pnv_xscom_add_subregion(chip,
-                                PNV_XSCOM_PBCQ_SPCI_BASE + 0x040 * phb->phb_id,
-                                &pbcq->xscom_spci_regs);
     }
 }
 
-- 
2.31.1

[PATCH 04/14] ppc/pnv: Introduce support for user created PHB3 devices

[Cédric Le Goater]

PHB3 devices and PCI devices can now be added to the powernv8 machine
using :

  -device pnv-phb3,chip-id=0,index=1 \
  -device nec-usb-xhci,bus=pci.1,addr=0x0

The 'index' property identifies the PHB3 in the chip. In case of user
created devices, a lookup on 'chip-id' is required to assign the
owning chip.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h   |  2 ++
 hw/pci-host/pnv_phb3.c | 11 ++++++++++-
 hw/ppc/pnv.c           | 23 ++++++++++++++++++-----
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 6f498c8f1b5f..0710673a7fd8 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -215,6 +215,8 @@ struct PnvMachineState {
     hwaddr       fw_load_addr;
 };
 
+PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
+
 #define PNV_FDT_ADDR          0x01000000
 #define PNV_TIMEBASE_FREQ     512000000ULL
 
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 3aa42ef9d4b9..dd1cf37288a0 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -998,6 +998,15 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /* User created devices */
+    if (!phb->chip) {
+        phb->chip = pnv_get_chip(pnv, phb->chip_id);
+        if (!phb->chip) {
+            error_setg(errp, "invalid chip id: %d", phb->chip_id);
+            return;
+        }
+    }
+
     /* LSI sources */
     object_property_set_link(OBJECT(&phb->lsis), "xics", OBJECT(pnv),
                              &error_abort);
@@ -1105,7 +1114,7 @@ static void pnv_phb3_class_init(ObjectClass *klass, void *data)
     dc->realize = pnv_phb3_realize;
     device_class_set_props(dc, pnv_phb3_properties);
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 }
 
 static const TypeInfo pnv_phb3_type_info = {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index de277c457838..d7fe92cb082d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1097,14 +1097,14 @@ static void pnv_chip_power8_instance_init(Object *obj)
 
     object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
 
-    for (i = 0; i < pcc->num_phbs; i++) {
+    if (defaults_enabled()) {
+        chip->num_phbs = pcc->num_phbs;
+    }
+
+    for (i = 0; i < chip->num_phbs; i++) {
         object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
     }
 
-    /*
-     * Number of PHBs is the chip default
-     */
-    chip->num_phbs = pcc->num_phbs;
 }
 
 static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
@@ -1784,6 +1784,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir)
     return NULL;
 }
 
+PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
+{
+    int i;
+
+    for (i = 0; i < pnv->num_chips; i++) {
+        PnvChip *chip = pnv->chips[i];
+        if (chip->chip_id == chip_id) {
+            return chip;
+        }
+    }
+    return NULL;
+}
+
 static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
 {
     PnvMachineState *pnv = PNV_MACHINE(xi);
-- 
2.31.1

[PATCH 05/14] ppc/pnv: Reparent user created PHB3 devices to the PnvChip

[Cédric Le Goater]

The powernv machine uses the object hierarchy to populate the device
tree and each device should be parented to the chip it belongs to.
This is not the case for user created devices which are parented to
the container "/unattached".

Make sure a PHB3 device is parented to its chip by reparenting the
object if necessary.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h   |  1 +
 hw/pci-host/pnv_phb3.c |  6 ++++++
 hw/ppc/pnv.c           | 17 +++++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 0710673a7fd8..247379ef1f88 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -175,6 +175,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
                          TYPE_PNV_CHIP_POWER10)
 
 PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
+void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index);
 
 #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
 typedef struct PnvMachineClass PnvMachineClass;
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index dd1cf37288a0..e91f658b0060 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1005,6 +1005,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "invalid chip id: %d", phb->chip_id);
             return;
         }
+
+        /*
+         * Reparent user created devices to the chip to build
+         * correctly the device tree.
+         */
+        pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
     }
 
     /* LSI sources */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d7fe92cb082d..9a458655efd9 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1784,6 +1784,23 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir)
     return NULL;
 }
 
+void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index)
+{
+    Object *parent = OBJECT(chip);
+    g_autofree char *default_id =
+        g_strdup_printf("%s[%d]", object_get_typename(obj), index);
+
+    if (obj->parent == parent) {
+        return;
+    }
+
+    object_ref(obj);
+    object_unparent(obj);
+    object_property_add_child(
+        parent, DEVICE(obj)->id ? DEVICE(obj)->id : default_id, obj);
+    object_unref(obj);
+}
+
 PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
 {
     int i;
-- 
2.31.1

[PATCH 06/14] ppc/pnv: Complete user created PHB3 devices

[Cédric Le Goater]

PHB3s ared SysBus devices and should be allowed to be dynamically
created.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb3.c | 9 +++++++++
 hw/ppc/pnv.c           | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index e91f658b0060..b61f9c369f64 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1000,6 +1000,9 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
 
     /* User created devices */
     if (!phb->chip) {
+        Error *local_err = NULL;
+        BusState *s;
+
         phb->chip = pnv_get_chip(pnv, phb->chip_id);
         if (!phb->chip) {
             error_setg(errp, "invalid chip id: %d", phb->chip_id);
@@ -1011,6 +1014,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
          * correctly the device tree.
          */
         pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
+
+        s = qdev_get_parent_bus(DEVICE(phb->chip));
+        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     /* LSI sources */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 9a458655efd9..45d8ecbf2bf7 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1927,6 +1927,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
 
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
+
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB3);
 }
 
 static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
-- 
2.31.1

[PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices

[Cédric Le Goater]

POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
each PEC can have several PHBs :

  * PEC0 provides 1 PHB  (PHB0)
  * PEC1 provides 2 PHBs (PHB1 and PHB2)
  * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)

A num_pecs class attribute represents better the logic units of the
POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
This will ease adding support for user created devices.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h |  2 ++
 hw/ppc/pnv.c         | 20 +++++++++-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 247379ef1f88..f2c238062f4a 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -53,6 +53,7 @@ struct PnvChip {
     PnvCore      **cores;
 
     uint32_t     num_phbs;
+    uint32_t     num_pecs;
 
     MemoryRegion xscom_mmio;
     MemoryRegion xscom;
@@ -136,6 +137,7 @@ struct PnvChipClass {
     uint64_t     chip_cfam_id;
     uint64_t     cores_mask;
     uint32_t     num_phbs;
+    uint32_t     num_pecs;
 
     DeviceRealize parent_realize;
 
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 45d8ecbf2bf7..185464a1d443 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -658,7 +658,7 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip, Monitor *mon)
     pnv_xive_pic_print_info(&chip9->xive, mon);
     pnv_psi_pic_print_info(&chip9->psi, mon);
 
-    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+    for (i = 0; i < chip->num_pecs; i++) {
         PnvPhb4PecState *pec = &chip9->pecs[i];
         for (j = 0; j < pec->num_stacks; j++) {
             pnv_phb4_pic_print_info(&pec->stacks[j].phb, mon);
@@ -1330,15 +1330,14 @@ static void pnv_chip_power9_instance_init(Object *obj)
 
     object_initialize_child(obj, "homer", &chip9->homer, TYPE_PNV9_HOMER);
 
-    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+    if (defaults_enabled()) {
+        chip->num_pecs = pcc->num_pecs;
+    }
+
+    for (i = 0; i < chip->num_pecs; i++) {
         object_initialize_child(obj, "pec[*]", &chip9->pecs[i],
                                 TYPE_PNV_PHB4_PEC);
     }
-
-    /*
-     * Number of PHBs is the chip default
-     */
-    chip->num_phbs = pcc->num_phbs;
 }
 
 static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
@@ -1374,7 +1373,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
     int i, j;
     int phb_id = 0;
 
-    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+    for (i = 0; i < chip->num_pecs; i++) {
         PnvPhb4PecState *pec = &chip9->pecs[i];
         PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
         uint32_t pec_nest_base;
@@ -1402,8 +1401,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
         pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
         pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
 
-        for (j = 0; j < pec->num_stacks && phb_id < chip->num_phbs;
-             j++, phb_id++) {
+        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
             PnvPhb4PecStack *stack = &pec->stacks[j];
             Object *obj = OBJECT(&stack->phb);
 
@@ -1559,7 +1557,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->xscom_core_base = pnv_chip_power9_xscom_core_base;
     k->xscom_pcba = pnv_chip_power9_xscom_pcba;
     dc->desc = "PowerNV Chip POWER9";
-    k->num_phbs = 6;
+    k->num_pecs = PNV9_CHIP_MAX_PEC;
 
     device_class_set_parent_realize(dc, pnv_chip_power9_realize,
                                     &k->parent_realize);
-- 
2.31.1

[PATCH 08/14] ppc/pnv: Introduce version and device_id class atributes for PHB4 devices

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/pci-host/pnv_phb4.h | 2 ++
 hw/pci-host/pnv_phb4_pec.c     | 2 ++
 hw/ppc/pnv.c                   | 4 ++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 27556ae53425..b2864233641e 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -219,6 +219,8 @@ struct PnvPhb4PecClass {
     int compat_size;
     const char *stk_compat;
     int stk_compat_size;
+    uint64_t version;
+    uint64_t device_id;
 };
 
 #endif /* PCI_HOST_PNV_PHB4_H */
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 741ddc90ed8d..9f722729ac50 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -499,6 +499,8 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
     pecc->compat_size = sizeof(compat);
     pecc->stk_compat = stk_compat;
     pecc->stk_compat_size = sizeof(stk_compat);
+    pecc->version = PNV_PHB4_VERSION;
+    pecc->device_id = PNV_PHB4_DEVICE_ID;
 }
 
 static const TypeInfo pnv_pec_type_info = {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 185464a1d443..0c65e1e88cf5 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1408,9 +1408,9 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
             object_property_set_int(obj, "index", phb_id, &error_fatal);
             object_property_set_int(obj, "chip-id", chip->chip_id,
                                     &error_fatal);
-            object_property_set_int(obj, "version", PNV_PHB4_VERSION,
+            object_property_set_int(obj, "version", pecc->version,
                                     &error_fatal);
-            object_property_set_int(obj, "device-id", PNV_PHB4_DEVICE_ID,
+            object_property_set_int(obj, "device-id", pecc->device_id,
                                     &error_fatal);
             object_property_set_link(obj, "stack", OBJECT(stack),
                                      &error_abort);
-- 
2.31.1

[PATCH 09/14] ppc/pnv: Introduce a "chip" property under the PHB4 model

[Cédric Le Goater]

Next changes will make use of it.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/pci-host/pnv_phb4.h | 2 ++
 hw/pci-host/pnv_phb4_pec.c     | 2 ++
 hw/ppc/pnv.c                   | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index b2864233641e..8a585c9a42f7 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -205,6 +205,8 @@ struct PnvPhb4PecState {
     #define PHB4_PEC_MAX_STACKS     3
     uint32_t num_stacks;
     PnvPhb4PecStack stacks[PHB4_PEC_MAX_STACKS];
+
+    PnvChip *chip;
 };
 
 
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 9f722729ac50..e9750c41c595 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -462,6 +462,8 @@ static Property pnv_pec_properties[] = {
         DEFINE_PROP_UINT32("index", PnvPhb4PecState, index, 0),
         DEFINE_PROP_UINT32("num-stacks", PnvPhb4PecState, num_stacks, 0),
         DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
+        DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
+                         PnvChip *),
         DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
                      TYPE_MEMORY_REGION, MemoryRegion *),
         DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0c65e1e88cf5..76b2f5468b38 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1389,6 +1389,8 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
                                 &error_fatal);
         object_property_set_int(OBJECT(pec), "chip-id", chip->chip_id,
                                 &error_fatal);
+        object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
+                                 &error_fatal);
         object_property_set_link(OBJECT(pec), "system-memory",
                                  OBJECT(get_system_memory()), &error_abort);
         if (!qdev_realize(DEVICE(pec), NULL, errp)) {
-- 
2.31.1

[PATCH 10/14] ppc/pnv: Introduce a num_stack class attribute

[Cédric Le Goater]

Each PEC devices of the POWER9 chip has a predefined number of stacks,
equivalent of a root port complex:

  PEC0 -> 1 stack
  PEC1 -> 2 stacks
  PEC2 -> 3 stacks

Introduce a class attribute to hold these values and remove the
"num-stacks" property.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/pci-host/pnv_phb4.h |  1 +
 hw/pci-host/pnv_phb4_pec.c     | 17 ++++++++++++++++-
 hw/ppc/pnv.c                   |  7 -------
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 8a585c9a42f7..60de3031a622 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -223,6 +223,7 @@ struct PnvPhb4PecClass {
     int stk_compat_size;
     uint64_t version;
     uint64_t device_id;
+    const uint32_t *num_stacks;
 };
 
 #endif /* PCI_HOST_PNV_PHB4_H */
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index e9750c41c595..293909b5cb90 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -377,11 +377,19 @@ static void pnv_pec_instance_init(Object *obj)
 static void pnv_pec_realize(DeviceState *dev, Error **errp)
 {
     PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
     char name[64];
     int i;
 
     assert(pec->system_memory);
 
+    if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
+        error_setg(errp, "invalid PEC index: %d", pec->index);
+        return;
+    }
+
+    pec->num_stacks = pecc->num_stacks[pec->index];
+
     /* Create stacks */
     for (i = 0; i < pec->num_stacks; i++) {
         PnvPhb4PecStack *stack = &pec->stacks[i];
@@ -460,7 +468,6 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
 
 static Property pnv_pec_properties[] = {
         DEFINE_PROP_UINT32("index", PnvPhb4PecState, index, 0),
-        DEFINE_PROP_UINT32("num-stacks", PnvPhb4PecState, num_stacks, 0),
         DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
         DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
                          PnvChip *),
@@ -479,6 +486,13 @@ static uint32_t pnv_pec_xscom_nest_base(PnvPhb4PecState *pec)
     return PNV9_XSCOM_PEC_NEST_BASE + 0x400 * pec->index;
 }
 
+/*
+ * PEC0 -> 1 stack
+ * PEC1 -> 2 stacks
+ * PEC2 -> 3 stacks
+ */
+static const uint32_t pnv_pec_num_stacks[] = { 1, 2, 3 };
+
 static void pnv_pec_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -503,6 +517,7 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
     pecc->stk_compat_size = sizeof(stk_compat);
     pecc->version = PNV_PHB4_VERSION;
     pecc->device_id = PNV_PHB4_DEVICE_ID;
+    pecc->num_stacks = pnv_pec_num_stacks;
 }
 
 static const TypeInfo pnv_pec_type_info = {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 76b2f5468b38..957f0bdfaa6b 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1380,13 +1380,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
         uint32_t pec_pci_base;
 
         object_property_set_int(OBJECT(pec), "index", i, &error_fatal);
-        /*
-         * PEC0 -> 1 stack
-         * PEC1 -> 2 stacks
-         * PEC2 -> 3 stacks
-         */
-        object_property_set_int(OBJECT(pec), "num-stacks", i + 1,
-                                &error_fatal);
         object_property_set_int(OBJECT(pec), "chip-id", chip->chip_id,
                                 &error_fatal);
         object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
-- 
2.31.1

[PATCH 11/14] ppc/pnv: Compute the PHB index from the PHB4 PEC model

[Cédric Le Goater]

Use the num_stacks class attribute to compute the PHB index depending
on the PEC index :

  * PEC0 provides 1 PHB  (PHB0)
  * PEC1 provides 2 PHBs (PHB1 and PHB2)
  * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb4_pec.c | 16 ++++++++++++++++
 hw/ppc/pnv.c               |  4 +---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 293909b5cb90..a7dd4173d598 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -374,6 +374,19 @@ static void pnv_pec_instance_init(Object *obj)
     }
 }
 
+static int pnv_pec_phb_offset(PnvPhb4PecState *pec)
+{
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    int index = pec->index;
+    int offset = 0;
+
+    while (index--) {
+        offset += pecc->num_stacks[index];
+    }
+
+    return offset;
+}
+
 static void pnv_pec_realize(DeviceState *dev, Error **errp)
 {
     PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
@@ -394,8 +407,10 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < pec->num_stacks; i++) {
         PnvPhb4PecStack *stack = &pec->stacks[i];
         Object *stk_obj = OBJECT(stack);
+        int phb_id = pnv_pec_phb_offset(pec) + i;
 
         object_property_set_int(stk_obj, "stack-no", i, &error_abort);
+        object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
         object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
         if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
             return;
@@ -538,6 +553,7 @@ static void pnv_pec_stk_instance_init(Object *obj)
     PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
 
     object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4);
+    object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index");
 }
 
 static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 957f0bdfaa6b..f8b0b2a28383 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1371,7 +1371,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
 {
     Pnv9Chip *chip9 = PNV9_CHIP(chip);
     int i, j;
-    int phb_id = 0;
 
     for (i = 0; i < chip->num_pecs; i++) {
         PnvPhb4PecState *pec = &chip9->pecs[i];
@@ -1396,11 +1395,10 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
         pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
         pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
 
-        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
+        for (j = 0; j < pec->num_stacks; j++) {
             PnvPhb4PecStack *stack = &pec->stacks[j];
             Object *obj = OBJECT(&stack->phb);
 
-            object_property_set_int(obj, "index", phb_id, &error_fatal);
             object_property_set_int(obj, "chip-id", chip->chip_id,
                                     &error_fatal);
             object_property_set_int(obj, "version", pecc->version,
-- 
2.31.1

[PATCH 12/14] ppc/pnv: Remove "system-memory" property for he PHB4 PEC model

[Cédric Le Goater]

This is not useful and will be in the way for support of user created
PHB4 devices.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb4_pec.c | 6 +-----
 hw/ppc/pnv.c               | 2 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index a7dd4173d598..dfed2af0f7df 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -124,7 +124,7 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
 static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
 {
     PnvPhb4PecState *pec = stack->pec;
-    MemoryRegion *sysmem = pec->system_memory;
+    MemoryRegion *sysmem = get_system_memory();
     uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
     uint64_t bar, mask, size;
     char name[64];
@@ -394,8 +394,6 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
     char name[64];
     int i;
 
-    assert(pec->system_memory);
-
     if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
         error_setg(errp, "invalid PEC index: %d", pec->index);
         return;
@@ -486,8 +484,6 @@ static Property pnv_pec_properties[] = {
         DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
         DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
                          PnvChip *),
-        DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
-                     TYPE_MEMORY_REGION, MemoryRegion *),
         DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index f8b0b2a28383..3a550eed0f36 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1383,8 +1383,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
                                 &error_fatal);
         object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
                                  &error_fatal);
-        object_property_set_link(OBJECT(pec), "system-memory",
-                                 OBJECT(get_system_memory()), &error_abort);
         if (!qdev_realize(DEVICE(pec), NULL, errp)) {
             return;
         }
-- 
2.31.1

[PATCH 13/14] ppc/pnv: Move realize of PEC stacks under the PEC model

[Cédric Le Goater]

This change will help us providing support for user created PHB4
devices.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb4_pec.c | 36 ++++++++++++++++++++++++++++++++----
 hw/ppc/pnv.c               | 31 +------------------------------
 2 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index dfed2af0f7df..9b081d543057 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -556,6 +556,10 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
 {
     PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
     PnvPhb4PecState *pec = stack->pec;
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    PnvChip *chip = pec->chip;
+    uint32_t pec_nest_base;
+    uint32_t pec_pci_base;
     char name[64];
 
     assert(pec);
@@ -579,10 +583,34 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
                           &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
 
-    /*
-     * Let the machine/chip realize the PHB object to customize more
-     * easily some fields
-     */
+    {
+        Object *obj = OBJECT(&stack->phb);
+
+        object_property_set_int(obj, "chip-id", pec->chip_id, &error_fatal);
+        object_property_set_int(obj, "version", pecc->version, &error_fatal);
+        object_property_set_int(obj, "device-id", pecc->device_id,
+                                &error_fatal);
+        object_property_set_link(obj, "stack", OBJECT(stack),
+                                 &error_abort);
+        if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
+            return;
+        }
+    }
+
+    pec_nest_base = pecc->xscom_nest_base(pec);
+    pec_pci_base = pecc->xscom_pci_base(pec);
+
+    /* Populate the XSCOM address space. */
+    pnv_xscom_add_subregion(chip,
+                            pec_nest_base + 0x40 * (stack->stack_no + 1),
+                            &stack->nest_regs_mr);
+    pnv_xscom_add_subregion(chip,
+                            pec_pci_base + 0x40 * (stack->stack_no + 1),
+                            &stack->pci_regs_mr);
+    pnv_xscom_add_subregion(chip,
+                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
+                            0x40 * stack->stack_no,
+                            &stack->phb_regs_mr);
 }
 
 static Property pnv_pec_stk_properties[] = {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3a550eed0f36..7e13b15241fd 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1370,7 +1370,7 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
 static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
 {
     Pnv9Chip *chip9 = PNV9_CHIP(chip);
-    int i, j;
+    int i;
 
     for (i = 0; i < chip->num_pecs; i++) {
         PnvPhb4PecState *pec = &chip9->pecs[i];
@@ -1392,35 +1392,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
 
         pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
         pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
-
-        for (j = 0; j < pec->num_stacks; j++) {
-            PnvPhb4PecStack *stack = &pec->stacks[j];
-            Object *obj = OBJECT(&stack->phb);
-
-            object_property_set_int(obj, "chip-id", chip->chip_id,
-                                    &error_fatal);
-            object_property_set_int(obj, "version", pecc->version,
-                                    &error_fatal);
-            object_property_set_int(obj, "device-id", pecc->device_id,
-                                    &error_fatal);
-            object_property_set_link(obj, "stack", OBJECT(stack),
-                                     &error_abort);
-            if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
-                return;
-            }
-
-            /* Populate the XSCOM address space. */
-            pnv_xscom_add_subregion(chip,
-                                   pec_nest_base + 0x40 * (stack->stack_no + 1),
-                                   &stack->nest_regs_mr);
-            pnv_xscom_add_subregion(chip,
-                                    pec_pci_base + 0x40 * (stack->stack_no + 1),
-                                    &stack->pci_regs_mr);
-            pnv_xscom_add_subregion(chip,
-                                    pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
-                                    0x40 * stack->stack_no,
-                                    &stack->phb_regs_mr);
-        }
     }
 }
 
-- 
2.31.1

[PATCH 14/14] ppc/pnv: Introduce support for user created PHB4 devices

[Cédric Le Goater]

PHB4 devices and PCI devices can now be added to the powernv9 machine
using:

  -device pnv-phb4-pec,chip-id=0,index=0
  -device nec-usb-xhci,bus=pci.0,addr=0x0

In case of user created devices, a lookup on 'chip-id' is required to
assign the owning chip.

To be noted, that the PEC PHB4 devices can add more than one PHB4
devices:

  * PEC0 provides 1 PHB  (PHB0)
  * PEC1 provides 2 PHBs (PHB1 and PHB2)
  * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb4_pec.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 9b081d543057..4ee92f11945c 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -394,6 +394,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
     char name[64];
     int i;
 
+    /* User created devices */
+    if (!pec->chip) {
+        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
+
+        pec->chip = pnv_get_chip(pnv, pec->chip_id);
+        if (!pec->chip) {
+            error_setg(errp, "invalid chip id: %d", pec->chip_id);
+            return;
+        }
+    }
+
     if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
         error_setg(errp, "invalid PEC index: %d", pec->index);
         return;
@@ -401,6 +412,12 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
 
     pec->num_stacks = pecc->num_stacks[pec->index];
 
+    /*
+     * Reparent user created devices to the chip to build correctly
+     * the device tree.
+     */
+    pnv_chip_parent_fixup(pec->chip, OBJECT(pec), pec->index);
+
     /* Create stacks */
     for (i = 0; i < pec->num_stacks; i++) {
         PnvPhb4PecStack *stack = &pec->stacks[i];
@@ -516,7 +533,7 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
 
     dc->realize = pnv_pec_realize;
     device_class_set_props(dc, pnv_pec_properties);
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 
     pecc->xscom_nest_base = pnv_pec_xscom_nest_base;
     pecc->xscom_pci_base  = pnv_pec_xscom_pci_base;
-- 
2.31.1

Re: [PATCH 01/14] ppc/pnv: Reduce the maximum of PHB3 devices

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> All POWER8 machines have a maximum of 3 PHB3 devices. Adapt the
> PNV8_CHIP_PHB3_MAX definition for consistency.

I suggest "3 PHB3 devices per chip" for clarity.

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/ppc/pnv.h | 2 +-
>   hw/ppc/pnv.c         | 6 +++---
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index aa08d79d24de..6f498c8f1b5f 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -79,7 +79,7 @@ struct Pnv8Chip {
>       PnvOCC       occ;
>       PnvHomer     homer;
>   
> -#define PNV8_CHIP_PHB3_MAX 4
> +#define PNV8_CHIP_PHB3_MAX 3
>       PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>   
>       XICSFabric    *xics;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 71e45515f136..bd768dcc28ad 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1256,7 +1256,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>   
>       k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>       k->cores_mask = POWER8E_CORE_MASK;
> -    k->num_phbs = 3;
> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
>       k->intc_reset = pnv_chip_power8_intc_reset;
> @@ -1280,7 +1280,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>   
>       k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>       k->cores_mask = POWER8_CORE_MASK;
> -    k->num_phbs = 3;
> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
>       k->intc_reset = pnv_chip_power8_intc_reset;
> @@ -1304,7 +1304,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>   
>       k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>       k->cores_mask = POWER8_CORE_MASK;
> -    k->num_phbs = 3;
> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
>       k->intc_reset = pnv_chip_power8_intc_reset;
> 

Re: [PATCH 02/14] ppc/pnv: Drop the "num-phbs" property

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> It is never used.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/ppc/pnv.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index bd768dcc28ad..988b305398b2 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1764,7 +1764,6 @@ static Property pnv_chip_properties[] = {
>       DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1),
>       DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0),
>       DEFINE_PROP_UINT32("nr-threads", PnvChip, nr_threads, 1),
> -    DEFINE_PROP_UINT32("num-phbs", PnvChip, num_phbs, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> 

Re: [PATCH 03/14] ppc/pnv: Move mapping of the PHB3 CQ regions under pnv_pbcq_realize()

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> This requires a link to the chip to add the regions under the XSCOM
> address space. This change will help us providing support for user
> created PHB3 devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/pci-host/pnv_phb3.h |  3 +++
>   hw/pci-host/pnv_phb3.c         |  1 +
>   hw/pci-host/pnv_phb3_pbcq.c    | 11 +++++++++++
>   hw/ppc/pnv.c                   | 14 ++------------
>   4 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
> index e2a2e3624532..e9c13e6bd821 100644
> --- a/include/hw/pci-host/pnv_phb3.h
> +++ b/include/hw/pci-host/pnv_phb3.h
> @@ -16,6 +16,7 @@
>   #include "qom/object.h"
>   
>   typedef struct PnvPHB3 PnvPHB3;
> +typedef struct PnvChip PnvChip;
>   
>   /*
>    * PHB3 XICS Source for MSIs
> @@ -157,6 +158,8 @@ struct PnvPHB3 {
>       PnvPHB3RootPort root;
>   
>       QLIST_HEAD(, PnvPhb3DMASpace) dma_spaces;
> +
> +    PnvChip *chip;
>   };
>   
>   uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, unsigned size);
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index a7f96850055a..3aa42ef9d4b9 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1092,6 +1092,7 @@ static const char *pnv_phb3_root_bus_path(PCIHostState *host_bridge,
>   static Property pnv_phb3_properties[] = {
>           DEFINE_PROP_UINT32("index", PnvPHB3, phb_id, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPHB3, chip_id, 0),
> +        DEFINE_PROP_LINK("chip", PnvPHB3, chip, TYPE_PNV_CHIP, PnvChip *),
>           DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/pci-host/pnv_phb3_pbcq.c b/hw/pci-host/pnv_phb3_pbcq.c
> index a0526aa1eca3..c7426cd27a20 100644
> --- a/hw/pci-host/pnv_phb3_pbcq.c
> +++ b/hw/pci-host/pnv_phb3_pbcq.c
> @@ -284,6 +284,17 @@ static void pnv_pbcq_realize(DeviceState *dev, Error **errp)
>       pnv_xscom_region_init(&pbcq->xscom_spci_regs, OBJECT(dev),
>                             &pnv_pbcq_spci_xscom_ops, pbcq, name,
>                             PNV_XSCOM_PBCQ_SPCI_SIZE);
> +
> +    /* Populate the XSCOM address space. */
> +    pnv_xscom_add_subregion(phb->chip,
> +                            PNV_XSCOM_PBCQ_NEST_BASE + 0x400 * phb->phb_id,
> +                            &pbcq->xscom_nest_regs);
> +    pnv_xscom_add_subregion(phb->chip,
> +                            PNV_XSCOM_PBCQ_PCI_BASE + 0x400 * phb->phb_id,
> +                            &pbcq->xscom_pci_regs);
> +    pnv_xscom_add_subregion(phb->chip,
> +                            PNV_XSCOM_PBCQ_SPCI_BASE + 0x040 * phb->phb_id,
> +                            &pbcq->xscom_spci_regs);
>   }
>   
>   static int pnv_pbcq_dt_xscom(PnvXScomInterface *dev, void *fdt,
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 988b305398b2..de277c457838 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1221,25 +1221,15 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>       /* PHB3 controllers */
>       for (i = 0; i < chip->num_phbs; i++) {
>           PnvPHB3 *phb = &chip8->phbs[i];
> -        PnvPBCQState *pbcq = &phb->pbcq;
>   
>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
>                                   &error_fatal);
> +        object_property_set_link(OBJECT(phb), "chip", OBJECT(chip),
> +                                 &error_fatal);
>           if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>               return;
>           }
> -
> -        /* Populate the XSCOM address space. */
> -        pnv_xscom_add_subregion(chip,
> -                                PNV_XSCOM_PBCQ_NEST_BASE + 0x400 * phb->phb_id,
> -                                &pbcq->xscom_nest_regs);
> -        pnv_xscom_add_subregion(chip,
> -                                PNV_XSCOM_PBCQ_PCI_BASE + 0x400 * phb->phb_id,
> -                                &pbcq->xscom_pci_regs);
> -        pnv_xscom_add_subregion(chip,
> -                                PNV_XSCOM_PBCQ_SPCI_BASE + 0x040 * phb->phb_id,
> -                                &pbcq->xscom_spci_regs);
>       }
>   }
>   
> 

Re: [PATCH 04/14] ppc/pnv: Introduce support for user created PHB3 devices

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> PHB3 devices and PCI devices can now be added to the powernv8 machine
> using :
> 
>    -device pnv-phb3,chip-id=0,index=1 \
>    -device nec-usb-xhci,bus=pci.1,addr=0x0
> 
> The 'index' property identifies the PHB3 in the chip. In case of user
> created devices, a lookup on 'chip-id' is required to assign the
> owning chip.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/ppc/pnv.h   |  2 ++
>   hw/pci-host/pnv_phb3.c | 11 ++++++++++-
>   hw/ppc/pnv.c           | 23 ++++++++++++++++++-----
>   3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 6f498c8f1b5f..0710673a7fd8 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -215,6 +215,8 @@ struct PnvMachineState {
>       hwaddr       fw_load_addr;
>   };
>   
> +PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
> +
>   #define PNV_FDT_ADDR          0x01000000
>   #define PNV_TIMEBASE_FREQ     512000000ULL
>   
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 3aa42ef9d4b9..dd1cf37288a0 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -998,6 +998,15 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> +    /* User created devices */
> +    if (!phb->chip) {
> +        phb->chip = pnv_get_chip(pnv, phb->chip_id);
> +        if (!phb->chip) {
> +            error_setg(errp, "invalid chip id: %d", phb->chip_id);
> +            return;
> +        }
> +    }
> +
>       /* LSI sources */
>       object_property_set_link(OBJECT(&phb->lsis), "xics", OBJECT(pnv),
>                                &error_abort);
> @@ -1105,7 +1114,7 @@ static void pnv_phb3_class_init(ObjectClass *klass, void *data)
>       dc->realize = pnv_phb3_realize;
>       device_class_set_props(dc, pnv_phb3_properties);
>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    dc->user_creatable = false;
> +    dc->user_creatable = true;
>   }
>   
>   static const TypeInfo pnv_phb3_type_info = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index de277c457838..d7fe92cb082d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1097,14 +1097,14 @@ static void pnv_chip_power8_instance_init(Object *obj)
>   
>       object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
>   
> -    for (i = 0; i < pcc->num_phbs; i++) {
> +    if (defaults_enabled()) {
> +        chip->num_phbs = pcc->num_phbs;
> +    }
> +
> +    for (i = 0; i < chip->num_phbs; i++) {
>           object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
>       }
>   
> -    /*
> -     * Number of PHBs is the chip default
> -     */
> -    chip->num_phbs = pcc->num_phbs;
>   }
>   
>   static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
> @@ -1784,6 +1784,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir)
>       return NULL;
>   }
>   
> +PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
> +{
> +    int i;
> +
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        PnvChip *chip = pnv->chips[i];
> +        if (chip->chip_id == chip_id) {
> +            return chip;
> +        }
> +    }
> +    return NULL;
> +}
> +
>   static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>   {
>       PnvMachineState *pnv = PNV_MACHINE(xi);
> 

Re: [PATCH 05/14] ppc/pnv: Reparent user created PHB3 devices to the PnvChip

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> The powernv machine uses the object hierarchy to populate the device
> tree and each device should be parented to the chip it belongs to.
> This is not the case for user created devices which are parented to
> the container "/unattached".
> 
> Make sure a PHB3 device is parented to its chip by reparenting the
> object if necessary.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/ppc/pnv.h   |  1 +
>   hw/pci-host/pnv_phb3.c |  6 ++++++
>   hw/ppc/pnv.c           | 17 +++++++++++++++++
>   3 files changed, 24 insertions(+)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 0710673a7fd8..247379ef1f88 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -175,6 +175,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
>                            TYPE_PNV_CHIP_POWER10)
>   
>   PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
> +void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index);
>   
>   #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>   typedef struct PnvMachineClass PnvMachineClass;
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index dd1cf37288a0..e91f658b0060 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1005,6 +1005,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>               error_setg(errp, "invalid chip id: %d", phb->chip_id);
>               return;
>           }
> +
> +        /*
> +         * Reparent user created devices to the chip to build
> +         * correctly the device tree.
> +         */
> +        pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
>       }
>   
>       /* LSI sources */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d7fe92cb082d..9a458655efd9 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1784,6 +1784,23 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir)
>       return NULL;
>   }
>   
> +void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index)
> +{
> +    Object *parent = OBJECT(chip);
> +    g_autofree char *default_id =
> +        g_strdup_printf("%s[%d]", object_get_typename(obj), index);
> +
> +    if (obj->parent == parent) {
> +        return;
> +    }
> +
> +    object_ref(obj);
> +    object_unparent(obj);
> +    object_property_add_child(
> +        parent, DEVICE(obj)->id ? DEVICE(obj)->id : default_id, obj);
> +    object_unref(obj);
> +}
> +
>   PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
>   {
>       int i;
> 

Re: [PATCH 06/14] ppc/pnv: Complete user created PHB3 devices

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> PHB3s ared SysBus devices and should be allowed to be dynamically
> created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci-host/pnv_phb3.c | 9 +++++++++
>   hw/ppc/pnv.c           | 2 ++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index e91f658b0060..b61f9c369f64 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1000,6 +1000,9 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>   
>       /* User created devices */
>       if (!phb->chip) {
> +        Error *local_err = NULL;
> +        BusState *s;
> +
>           phb->chip = pnv_get_chip(pnv, phb->chip_id);
>           if (!phb->chip) {
>               error_setg(errp, "invalid chip id: %d", phb->chip_id);
> @@ -1011,6 +1014,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>            * correctly the device tree.
>            */
>           pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
> +
> +        s = qdev_get_parent_bus(DEVICE(phb->chip));
> +        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>       }
>   
>       /* LSI sources */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 9a458655efd9..45d8ecbf2bf7 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1927,6 +1927,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
>   
>       pmc->compat = compat;
>       pmc->compat_size = sizeof(compat);
> +
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB3);
>   }
>   
>   static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
> 

Re: [PATCH 01/14] ppc/pnv: Reduce the maximum of PHB3 devices

[Cédric Le Goater]

On 12/2/21 18:27, Daniel Henrique Barboza wrote:
> 
> 
> On 12/2/21 11:42, Cédric Le Goater wrote:
>> All POWER8 machines have a maximum of 3 PHB3 devices. Adapt the
>> PNV8_CHIP_PHB3_MAX definition for consistency.
> 
> I suggest "3 PHB3 devices per chip" for clarity.

Fixed.

Thanks,

C.

> 
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
>>   include/hw/ppc/pnv.h | 2 +-
>>   hw/ppc/pnv.c         | 6 +++---
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index aa08d79d24de..6f498c8f1b5f 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -79,7 +79,7 @@ struct Pnv8Chip {
>>       PnvOCC       occ;
>>       PnvHomer     homer;
>> -#define PNV8_CHIP_PHB3_MAX 4
>> +#define PNV8_CHIP_PHB3_MAX 3
>>       PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>>       XICSFabric    *xics;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 71e45515f136..bd768dcc28ad 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1256,7 +1256,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>>       k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>>       k->cores_mask = POWER8E_CORE_MASK;
>> -    k->num_phbs = 3;
>> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>>       k->core_pir = pnv_chip_core_pir_p8;
>>       k->intc_create = pnv_chip_power8_intc_create;
>>       k->intc_reset = pnv_chip_power8_intc_reset;
>> @@ -1280,7 +1280,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>>       k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>>       k->cores_mask = POWER8_CORE_MASK;
>> -    k->num_phbs = 3;
>> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>>       k->core_pir = pnv_chip_core_pir_p8;
>>       k->intc_create = pnv_chip_power8_intc_create;
>>       k->intc_reset = pnv_chip_power8_intc_reset;
>> @@ -1304,7 +1304,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>>       k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>>       k->cores_mask = POWER8_CORE_MASK;
>> -    k->num_phbs = 3;
>> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>>       k->core_pir = pnv_chip_core_pir_p8;
>>       k->intc_create = pnv_chip_power8_intc_create;
>>       k->intc_reset = pnv_chip_power8_intc_reset;
>>

Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
> each PEC can have several PHBs :
> 
>    * PEC0 provides 1 PHB  (PHB0)
>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
> 
> A num_pecs class attribute represents better the logic units of the
> POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
> This will ease adding support for user created devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/ppc/pnv.h |  2 ++
>   hw/ppc/pnv.c         | 20 +++++++++-----------
>   2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 247379ef1f88..f2c238062f4a 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -53,6 +53,7 @@ struct PnvChip {
>       PnvCore      **cores;
>   
>       uint32_t     num_phbs;
> +    uint32_t     num_pecs;
>   
>       MemoryRegion xscom_mmio;
>       MemoryRegion xscom;
> @@ -136,6 +137,7 @@ struct PnvChipClass {
>       uint64_t     chip_cfam_id;
>       uint64_t     cores_mask;
>       uint32_t     num_phbs;
> +    uint32_t     num_pecs;
>   
>       DeviceRealize parent_realize;
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 45d8ecbf2bf7..185464a1d443 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -658,7 +658,7 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip, Monitor *mon)
>       pnv_xive_pic_print_info(&chip9->xive, mon);
>       pnv_psi_pic_print_info(&chip9->psi, mon);
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>           for (j = 0; j < pec->num_stacks; j++) {
>               pnv_phb4_pic_print_info(&pec->stacks[j].phb, mon);
> @@ -1330,15 +1330,14 @@ static void pnv_chip_power9_instance_init(Object *obj)
>   
>       object_initialize_child(obj, "homer", &chip9->homer, TYPE_PNV9_HOMER);
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    if (defaults_enabled()) {
> +        chip->num_pecs = pcc->num_pecs;
> +    }
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
>           object_initialize_child(obj, "pec[*]", &chip9->pecs[i],
>                                   TYPE_PNV_PHB4_PEC);
>       }
> -
> -    /*
> -     * Number of PHBs is the chip default
> -     */
> -    chip->num_phbs = pcc->num_phbs;
>   }
>   
>   static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
> @@ -1374,7 +1373,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>       int i, j;
>       int phb_id = 0;
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>           PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>           uint32_t pec_nest_base;
> @@ -1402,8 +1401,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
>   
> -        for (j = 0; j < pec->num_stacks && phb_id < chip->num_phbs;
> -             j++, phb_id++) {
> +        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
>               PnvPhb4PecStack *stack = &pec->stacks[j];
>               Object *obj = OBJECT(&stack->phb);
>   
> @@ -1559,7 +1557,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>       k->xscom_core_base = pnv_chip_power9_xscom_core_base;
>       k->xscom_pcba = pnv_chip_power9_xscom_pcba;
>       dc->desc = "PowerNV Chip POWER9";
> -    k->num_phbs = 6;
> +    k->num_pecs = PNV9_CHIP_MAX_PEC;
>   
>       device_class_set_parent_realize(dc, pnv_chip_power9_realize,
>                                       &k->parent_realize);
> 

Re: [PATCH 08/14] ppc/pnv: Introduce version and device_id class atributes for PHB4 devices

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/pci-host/pnv_phb4.h | 2 ++
>   hw/pci-host/pnv_phb4_pec.c     | 2 ++
>   hw/ppc/pnv.c                   | 4 ++--
>   3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 27556ae53425..b2864233641e 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -219,6 +219,8 @@ struct PnvPhb4PecClass {
>       int compat_size;
>       const char *stk_compat;
>       int stk_compat_size;
> +    uint64_t version;
> +    uint64_t device_id;
>   };
>   
>   #endif /* PCI_HOST_PNV_PHB4_H */
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 741ddc90ed8d..9f722729ac50 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -499,6 +499,8 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>       pecc->compat_size = sizeof(compat);
>       pecc->stk_compat = stk_compat;
>       pecc->stk_compat_size = sizeof(stk_compat);
> +    pecc->version = PNV_PHB4_VERSION;
> +    pecc->device_id = PNV_PHB4_DEVICE_ID;
>   }
>   
>   static const TypeInfo pnv_pec_type_info = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 185464a1d443..0c65e1e88cf5 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1408,9 +1408,9 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>               object_property_set_int(obj, "index", phb_id, &error_fatal);
>               object_property_set_int(obj, "chip-id", chip->chip_id,
>                                       &error_fatal);
> -            object_property_set_int(obj, "version", PNV_PHB4_VERSION,
> +            object_property_set_int(obj, "version", pecc->version,
>                                       &error_fatal);
> -            object_property_set_int(obj, "device-id", PNV_PHB4_DEVICE_ID,
> +            object_property_set_int(obj, "device-id", pecc->device_id,
>                                       &error_fatal);
>               object_property_set_link(obj, "stack", OBJECT(stack),
>                                        &error_abort);
> 

Re: [PATCH 09/14] ppc/pnv: Introduce a "chip" property under the PHB4 model

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> Next changes will make use of it.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/pci-host/pnv_phb4.h | 2 ++
>   hw/pci-host/pnv_phb4_pec.c     | 2 ++
>   hw/ppc/pnv.c                   | 2 ++
>   3 files changed, 6 insertions(+)
> 
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index b2864233641e..8a585c9a42f7 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -205,6 +205,8 @@ struct PnvPhb4PecState {
>       #define PHB4_PEC_MAX_STACKS     3
>       uint32_t num_stacks;
>       PnvPhb4PecStack stacks[PHB4_PEC_MAX_STACKS];
> +
> +    PnvChip *chip;
>   };
>   
>   
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 9f722729ac50..e9750c41c595 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -462,6 +462,8 @@ static Property pnv_pec_properties[] = {
>           DEFINE_PROP_UINT32("index", PnvPhb4PecState, index, 0),
>           DEFINE_PROP_UINT32("num-stacks", PnvPhb4PecState, num_stacks, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
> +        DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
> +                         PnvChip *),
>           DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
>                        TYPE_MEMORY_REGION, MemoryRegion *),
>           DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0c65e1e88cf5..76b2f5468b38 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1389,6 +1389,8 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>                                   &error_fatal);
>           object_property_set_int(OBJECT(pec), "chip-id", chip->chip_id,
>                                   &error_fatal);
> +        object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
> +                                 &error_fatal);
>           object_property_set_link(OBJECT(pec), "system-memory",
>                                    OBJECT(get_system_memory()), &error_abort);
>           if (!qdev_realize(DEVICE(pec), NULL, errp)) {
> 

Re: [PATCH 10/14] ppc/pnv: Introduce a num_stack class attribute

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> Each PEC devices of the POWER9 chip has a predefined number of stacks,

s/devices/device ?

> equivalent of a root port complex:
> 
>    PEC0 -> 1 stack
>    PEC1 -> 2 stacks
>    PEC2 -> 3 stacks
> 
> Introduce a class attribute to hold these values and remove the
> "num-stacks" property.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/pci-host/pnv_phb4.h |  1 +
>   hw/pci-host/pnv_phb4_pec.c     | 17 ++++++++++++++++-
>   hw/ppc/pnv.c                   |  7 -------
>   3 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 8a585c9a42f7..60de3031a622 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -223,6 +223,7 @@ struct PnvPhb4PecClass {
>       int stk_compat_size;
>       uint64_t version;
>       uint64_t device_id;
> +    const uint32_t *num_stacks;
>   };
>   
>   #endif /* PCI_HOST_PNV_PHB4_H */
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index e9750c41c595..293909b5cb90 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -377,11 +377,19 @@ static void pnv_pec_instance_init(Object *obj)
>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>       char name[64];
>       int i;
>   
>       assert(pec->system_memory);
>   
> +    if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
> +        error_setg(errp, "invalid PEC index: %d", pec->index);
> +        return;
> +    }
> +
> +    pec->num_stacks = pecc->num_stacks[pec->index];
> +
>       /* Create stacks */
>       for (i = 0; i < pec->num_stacks; i++) {
>           PnvPhb4PecStack *stack = &pec->stacks[i];
> @@ -460,7 +468,6 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>   
>   static Property pnv_pec_properties[] = {
>           DEFINE_PROP_UINT32("index", PnvPhb4PecState, index, 0),
> -        DEFINE_PROP_UINT32("num-stacks", PnvPhb4PecState, num_stacks, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
>           DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
>                            PnvChip *),
> @@ -479,6 +486,13 @@ static uint32_t pnv_pec_xscom_nest_base(PnvPhb4PecState *pec)
>       return PNV9_XSCOM_PEC_NEST_BASE + 0x400 * pec->index;
>   }
>   
> +/*
> + * PEC0 -> 1 stack
> + * PEC1 -> 2 stacks
> + * PEC2 -> 3 stacks
> + */
> +static const uint32_t pnv_pec_num_stacks[] = { 1, 2, 3 };
> +
>   static void pnv_pec_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -503,6 +517,7 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>       pecc->stk_compat_size = sizeof(stk_compat);
>       pecc->version = PNV_PHB4_VERSION;
>       pecc->device_id = PNV_PHB4_DEVICE_ID;
> +    pecc->num_stacks = pnv_pec_num_stacks;
>   }
>   
>   static const TypeInfo pnv_pec_type_info = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 76b2f5468b38..957f0bdfaa6b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1380,13 +1380,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>           uint32_t pec_pci_base;
>   
>           object_property_set_int(OBJECT(pec), "index", i, &error_fatal);
> -        /*
> -         * PEC0 -> 1 stack
> -         * PEC1 -> 2 stacks
> -         * PEC2 -> 3 stacks
> -         */
> -        object_property_set_int(OBJECT(pec), "num-stacks", i + 1,
> -                                &error_fatal);
>           object_property_set_int(OBJECT(pec), "chip-id", chip->chip_id,
>                                   &error_fatal);
>           object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
> 

Re: [PATCH 11/14] ppc/pnv: Compute the PHB index from the PHB4 PEC model

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> Use the num_stacks class attribute to compute the PHB index depending
> on the PEC index :
> 
>    * PEC0 provides 1 PHB  (PHB0)
>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci-host/pnv_phb4_pec.c | 16 ++++++++++++++++
>   hw/ppc/pnv.c               |  4 +---
>   2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 293909b5cb90..a7dd4173d598 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -374,6 +374,19 @@ static void pnv_pec_instance_init(Object *obj)
>       }
>   }
>   
> +static int pnv_pec_phb_offset(PnvPhb4PecState *pec)
> +{
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    int index = pec->index;
> +    int offset = 0;
> +
> +    while (index--) {
> +        offset += pecc->num_stacks[index];
> +    }
> +
> +    return offset;
> +}
> +
>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
> @@ -394,8 +407,10 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       for (i = 0; i < pec->num_stacks; i++) {
>           PnvPhb4PecStack *stack = &pec->stacks[i];
>           Object *stk_obj = OBJECT(stack);
> +        int phb_id = pnv_pec_phb_offset(pec) + i;
>   
>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
> +        object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
>           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
> @@ -538,6 +553,7 @@ static void pnv_pec_stk_instance_init(Object *obj)
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
>   
>       object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4);
> +    object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index");
>   }
>   
>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 957f0bdfaa6b..f8b0b2a28383 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1371,7 +1371,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   {
>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
>       int i, j;
> -    int phb_id = 0;
>   
>       for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
> @@ -1396,11 +1395,10 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
>   
> -        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
> +        for (j = 0; j < pec->num_stacks; j++) {
>               PnvPhb4PecStack *stack = &pec->stacks[j];
>               Object *obj = OBJECT(&stack->phb);
>   
> -            object_property_set_int(obj, "index", phb_id, &error_fatal);
>               object_property_set_int(obj, "chip-id", chip->chip_id,
>                                       &error_fatal);
>               object_property_set_int(obj, "version", pecc->version,
> 

Re: [PATCH 12/14] ppc/pnv: Remove "system-memory" property for he PHB4 PEC model

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> This is not useful and will be in the way for support of user created
> PHB4 devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci-host/pnv_phb4_pec.c | 6 +-----
>   hw/ppc/pnv.c               | 2 --
>   2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index a7dd4173d598..dfed2af0f7df 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -124,7 +124,7 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
>   static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>   {
>       PnvPhb4PecState *pec = stack->pec;
> -    MemoryRegion *sysmem = pec->system_memory;
> +    MemoryRegion *sysmem = get_system_memory();
>       uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
>       uint64_t bar, mask, size;
>       char name[64];
> @@ -394,8 +394,6 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       char name[64];
>       int i;
>   
> -    assert(pec->system_memory);
> -
>       if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
>           error_setg(errp, "invalid PEC index: %d", pec->index);
>           return;
> @@ -486,8 +484,6 @@ static Property pnv_pec_properties[] = {
>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
>           DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
>                            PnvChip *),
> -        DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
> -                     TYPE_MEMORY_REGION, MemoryRegion *),
>           DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index f8b0b2a28383..3a550eed0f36 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1383,8 +1383,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>                                   &error_fatal);
>           object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
>                                    &error_fatal);
> -        object_property_set_link(OBJECT(pec), "system-memory",
> -                                 OBJECT(get_system_memory()), &error_abort);
>           if (!qdev_realize(DEVICE(pec), NULL, errp)) {
>               return;
>           }
> 

Re: [PATCH 13/14] ppc/pnv: Move realize of PEC stacks under the PEC model

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> This change will help us providing support for user created PHB4
> devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci-host/pnv_phb4_pec.c | 36 ++++++++++++++++++++++++++++++++----
>   hw/ppc/pnv.c               | 31 +------------------------------
>   2 files changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index dfed2af0f7df..9b081d543057 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -556,6 +556,10 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
>       PnvPhb4PecState *pec = stack->pec;
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    PnvChip *chip = pec->chip;
> +    uint32_t pec_nest_base;
> +    uint32_t pec_pci_base;
>       char name[64];
>   
>       assert(pec);
> @@ -579,10 +583,34 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>       pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
>                             &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
>   
> -    /*
> -     * Let the machine/chip realize the PHB object to customize more
> -     * easily some fields
> -     */
> +    {
> +        Object *obj = OBJECT(&stack->phb);
> +
> +        object_property_set_int(obj, "chip-id", pec->chip_id, &error_fatal);
> +        object_property_set_int(obj, "version", pecc->version, &error_fatal);
> +        object_property_set_int(obj, "device-id", pecc->device_id,
> +                                &error_fatal);
> +        object_property_set_link(obj, "stack", OBJECT(stack),
> +                                 &error_abort);
> +        if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
> +            return;
> +        }
> +    }
> +
> +    pec_nest_base = pecc->xscom_nest_base(pec);
> +    pec_pci_base = pecc->xscom_pci_base(pec);
> +
> +    /* Populate the XSCOM address space. */
> +    pnv_xscom_add_subregion(chip,
> +                            pec_nest_base + 0x40 * (stack->stack_no + 1),
> +                            &stack->nest_regs_mr);
> +    pnv_xscom_add_subregion(chip,
> +                            pec_pci_base + 0x40 * (stack->stack_no + 1),
> +                            &stack->pci_regs_mr);
> +    pnv_xscom_add_subregion(chip,
> +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> +                            0x40 * stack->stack_no,
> +                            &stack->phb_regs_mr);
>   }
>   
>   static Property pnv_pec_stk_properties[] = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3a550eed0f36..7e13b15241fd 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1370,7 +1370,7 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
>   static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   {
>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
> -    int i, j;
> +    int i;
>   
>       for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
> @@ -1392,35 +1392,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
> -
> -        for (j = 0; j < pec->num_stacks; j++) {
> -            PnvPhb4PecStack *stack = &pec->stacks[j];
> -            Object *obj = OBJECT(&stack->phb);
> -
> -            object_property_set_int(obj, "chip-id", chip->chip_id,
> -                                    &error_fatal);
> -            object_property_set_int(obj, "version", pecc->version,
> -                                    &error_fatal);
> -            object_property_set_int(obj, "device-id", pecc->device_id,
> -                                    &error_fatal);
> -            object_property_set_link(obj, "stack", OBJECT(stack),
> -                                     &error_abort);
> -            if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
> -                return;
> -            }
> -
> -            /* Populate the XSCOM address space. */
> -            pnv_xscom_add_subregion(chip,
> -                                   pec_nest_base + 0x40 * (stack->stack_no + 1),
> -                                   &stack->nest_regs_mr);
> -            pnv_xscom_add_subregion(chip,
> -                                    pec_pci_base + 0x40 * (stack->stack_no + 1),
> -                                    &stack->pci_regs_mr);
> -            pnv_xscom_add_subregion(chip,
> -                                    pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> -                                    0x40 * stack->stack_no,
> -                                    &stack->phb_regs_mr);
> -        }
>       }
>   }
>   
> 

Re: [PATCH 14/14] ppc/pnv: Introduce support for user created PHB4 devices

[Daniel Henrique Barboza]

On 12/2/21 11:42, Cédric Le Goater wrote:
> PHB4 devices and PCI devices can now be added to the powernv9 machine
> using:
> 
>    -device pnv-phb4-pec,chip-id=0,index=0
>    -device nec-usb-xhci,bus=pci.0,addr=0x0
> 
> In case of user created devices, a lookup on 'chip-id' is required to
> assign the owning chip.
> 
> To be noted, that the PEC PHB4 devices can add more than one PHB4
> devices:
> 
>    * PEC0 provides 1 PHB  (PHB0)
>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci-host/pnv_phb4_pec.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 9b081d543057..4ee92f11945c 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -394,6 +394,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       char name[64];
>       int i;
>   
> +    /* User created devices */
> +    if (!pec->chip) {
> +        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +
> +        pec->chip = pnv_get_chip(pnv, pec->chip_id);
> +        if (!pec->chip) {
> +            error_setg(errp, "invalid chip id: %d", pec->chip_id);
> +            return;
> +        }
> +    }
> +
>       if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
>           error_setg(errp, "invalid PEC index: %d", pec->index);
>           return;
> @@ -401,6 +412,12 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   
>       pec->num_stacks = pecc->num_stacks[pec->index];
>   
> +    /*
> +     * Reparent user created devices to the chip to build correctly
> +     * the device tree.
> +     */
> +    pnv_chip_parent_fixup(pec->chip, OBJECT(pec), pec->index);
> +
>       /* Create stacks */
>       for (i = 0; i < pec->num_stacks; i++) {
>           PnvPhb4PecStack *stack = &pec->stacks[i];
> @@ -516,7 +533,7 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>   
>       dc->realize = pnv_pec_realize;
>       device_class_set_props(dc, pnv_pec_properties);
> -    dc->user_creatable = false;
> +    dc->user_creatable = true;
>   
>       pecc->xscom_nest_base = pnv_pec_xscom_nest_base;
>       pecc->xscom_pci_base  = pnv_pec_xscom_pci_base;
> 

Re: [PATCH 01/14] ppc/pnv: Reduce the maximum of PHB3 devices

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> All POWER8 machines have a maximum of 3 PHB3 devices. Adapt the
> PNV8_CHIP_PHB3_MAX definition for consistency.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


The Naples chip (Garrison) can have 4 PHBs and it seems we have a 
power8nvl machine type for it. So I guess we should keep a max PHB count 
of 4 there.

   Fred



>   include/hw/ppc/pnv.h | 2 +-
>   hw/ppc/pnv.c         | 6 +++---
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index aa08d79d24de..6f498c8f1b5f 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -79,7 +79,7 @@ struct Pnv8Chip {
>       PnvOCC       occ;
>       PnvHomer     homer;
>   
> -#define PNV8_CHIP_PHB3_MAX 4
> +#define PNV8_CHIP_PHB3_MAX 3
>       PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>   
>       XICSFabric    *xics;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 71e45515f136..bd768dcc28ad 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1256,7 +1256,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>   
>       k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>       k->cores_mask = POWER8E_CORE_MASK;
> -    k->num_phbs = 3;
> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
>       k->intc_reset = pnv_chip_power8_intc_reset;
> @@ -1280,7 +1280,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>   
>       k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>       k->cores_mask = POWER8_CORE_MASK;
> -    k->num_phbs = 3;
> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
>       k->intc_reset = pnv_chip_power8_intc_reset;
> @@ -1304,7 +1304,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>   
>       k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>       k->cores_mask = POWER8_CORE_MASK;
> -    k->num_phbs = 3;
> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
>       k->intc_reset = pnv_chip_power8_intc_reset;
> 

Re: [PATCH 03/14] ppc/pnv: Move mapping of the PHB3 CQ regions under pnv_pbcq_realize()

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> This requires a link to the chip to add the regions under the XSCOM
> address space. This change will help us providing support for user
> created PHB3 devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   include/hw/pci-host/pnv_phb3.h |  3 +++
>   hw/pci-host/pnv_phb3.c         |  1 +
>   hw/pci-host/pnv_phb3_pbcq.c    | 11 +++++++++++
>   hw/ppc/pnv.c                   | 14 ++------------
>   4 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
> index e2a2e3624532..e9c13e6bd821 100644
> --- a/include/hw/pci-host/pnv_phb3.h
> +++ b/include/hw/pci-host/pnv_phb3.h
> @@ -16,6 +16,7 @@
>   #include "qom/object.h"
>   
>   typedef struct PnvPHB3 PnvPHB3;
> +typedef struct PnvChip PnvChip;
>   
>   /*
>    * PHB3 XICS Source for MSIs
> @@ -157,6 +158,8 @@ struct PnvPHB3 {
>       PnvPHB3RootPort root;
>   
>       QLIST_HEAD(, PnvPhb3DMASpace) dma_spaces;
> +
> +    PnvChip *chip;
>   };
>   
>   uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, unsigned size);
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index a7f96850055a..3aa42ef9d4b9 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1092,6 +1092,7 @@ static const char *pnv_phb3_root_bus_path(PCIHostState *host_bridge,
>   static Property pnv_phb3_properties[] = {
>           DEFINE_PROP_UINT32("index", PnvPHB3, phb_id, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPHB3, chip_id, 0),
> +        DEFINE_PROP_LINK("chip", PnvPHB3, chip, TYPE_PNV_CHIP, PnvChip *),
>           DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/pci-host/pnv_phb3_pbcq.c b/hw/pci-host/pnv_phb3_pbcq.c
> index a0526aa1eca3..c7426cd27a20 100644
> --- a/hw/pci-host/pnv_phb3_pbcq.c
> +++ b/hw/pci-host/pnv_phb3_pbcq.c
> @@ -284,6 +284,17 @@ static void pnv_pbcq_realize(DeviceState *dev, Error **errp)
>       pnv_xscom_region_init(&pbcq->xscom_spci_regs, OBJECT(dev),
>                             &pnv_pbcq_spci_xscom_ops, pbcq, name,
>                             PNV_XSCOM_PBCQ_SPCI_SIZE);
> +
> +    /* Populate the XSCOM address space. */
> +    pnv_xscom_add_subregion(phb->chip,
> +                            PNV_XSCOM_PBCQ_NEST_BASE + 0x400 * phb->phb_id,
> +                            &pbcq->xscom_nest_regs);
> +    pnv_xscom_add_subregion(phb->chip,
> +                            PNV_XSCOM_PBCQ_PCI_BASE + 0x400 * phb->phb_id,
> +                            &pbcq->xscom_pci_regs);
> +    pnv_xscom_add_subregion(phb->chip,
> +                            PNV_XSCOM_PBCQ_SPCI_BASE + 0x040 * phb->phb_id,
> +                            &pbcq->xscom_spci_regs);
>   }
>   
>   static int pnv_pbcq_dt_xscom(PnvXScomInterface *dev, void *fdt,
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 988b305398b2..de277c457838 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1221,25 +1221,15 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>       /* PHB3 controllers */
>       for (i = 0; i < chip->num_phbs; i++) {
>           PnvPHB3 *phb = &chip8->phbs[i];
> -        PnvPBCQState *pbcq = &phb->pbcq;
>   
>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
>                                   &error_fatal);
> +        object_property_set_link(OBJECT(phb), "chip", OBJECT(chip),
> +                                 &error_fatal);
>           if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>               return;
>           }
> -
> -        /* Populate the XSCOM address space. */
> -        pnv_xscom_add_subregion(chip,
> -                                PNV_XSCOM_PBCQ_NEST_BASE + 0x400 * phb->phb_id,
> -                                &pbcq->xscom_nest_regs);
> -        pnv_xscom_add_subregion(chip,
> -                                PNV_XSCOM_PBCQ_PCI_BASE + 0x400 * phb->phb_id,
> -                                &pbcq->xscom_pci_regs);
> -        pnv_xscom_add_subregion(chip,
> -                                PNV_XSCOM_PBCQ_SPCI_BASE + 0x040 * phb->phb_id,
> -                                &pbcq->xscom_spci_regs);
>       }
>   }
>   
> 

Re: [PATCH 02/14] ppc/pnv: Drop the "num-phbs" property

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> It is never used.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   hw/ppc/pnv.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index bd768dcc28ad..988b305398b2 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1764,7 +1764,6 @@ static Property pnv_chip_properties[] = {
>       DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1),
>       DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0),
>       DEFINE_PROP_UINT32("nr-threads", PnvChip, nr_threads, 1),
> -    DEFINE_PROP_UINT32("num-phbs", PnvChip, num_phbs, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> 

Re: [PATCH 04/14] ppc/pnv: Introduce support for user created PHB3 devices

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> PHB3 devices and PCI devices can now be added to the powernv8 machine
> using :
> 
>    -device pnv-phb3,chip-id=0,index=1 \
>    -device nec-usb-xhci,bus=pci.1,addr=0x0
> 
> The 'index' property identifies the PHB3 in the chip. In case of user
> created devices, a lookup on 'chip-id' is required to assign the
> owning chip.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index de277c457838..d7fe92cb082d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1097,14 +1097,14 @@ static void pnv_chip_power8_instance_init(Object *obj)
>   
>       object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
>   
> -    for (i = 0; i < pcc->num_phbs; i++) {
> +    if (defaults_enabled()) {
> +        chip->num_phbs = pcc->num_phbs;
> +    }
> +
> +    for (i = 0; i < chip->num_phbs; i++) {
>           object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
>       }
>   
> -    /*
> -     * Number of PHBs is the chip default
> -     */
> -    chip->num_phbs = pcc->num_phbs;
>   }


So if "-nodefaults" is mentioned on the command line, then 
chip->num_phbs is not set. It seems the intention is to have only the 
PHBs defined on the CLI, which is fine. However, I don't see where 
chip->num_phbs is incremented in that case.

   Fred



>   
>   static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
> @@ -1784,6 +1784,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir)
>       return NULL;
>   }
>   
> +PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
> +{
> +    int i;
> +
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        PnvChip *chip = pnv->chips[i];
> +        if (chip->chip_id == chip_id) {
> +            return chip;
> +        }
> +    }
> +    return NULL;
> +}
> +
>   static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>   {
>       PnvMachineState *pnv = PNV_MACHINE(xi);
> 

Re: [PATCH 05/14] ppc/pnv: Reparent user created PHB3 devices to the PnvChip

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> The powernv machine uses the object hierarchy to populate the device
> tree and each device should be parented to the chip it belongs to.
> This is not the case for user created devices which are parented to
> the container "/unattached".
> 
> Make sure a PHB3 device is parented to its chip by reparenting the
> object if necessary.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


It will also be used later for P9, which explains why it's done that 
way, I think. Looks ok to me.

Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   include/hw/ppc/pnv.h   |  1 +
>   hw/pci-host/pnv_phb3.c |  6 ++++++
>   hw/ppc/pnv.c           | 17 +++++++++++++++++
>   3 files changed, 24 insertions(+)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 0710673a7fd8..247379ef1f88 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -175,6 +175,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
>                            TYPE_PNV_CHIP_POWER10)
>   
>   PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
> +void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index);
>   
>   #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>   typedef struct PnvMachineClass PnvMachineClass;
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index dd1cf37288a0..e91f658b0060 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1005,6 +1005,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>               error_setg(errp, "invalid chip id: %d", phb->chip_id);
>               return;
>           }
> +
> +        /*
> +         * Reparent user created devices to the chip to build
> +         * correctly the device tree.
> +         */
> +        pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
>       }
>   
>       /* LSI sources */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d7fe92cb082d..9a458655efd9 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1784,6 +1784,23 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir)
>       return NULL;
>   }
>   
> +void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index)
> +{
> +    Object *parent = OBJECT(chip);
> +    g_autofree char *default_id =
> +        g_strdup_printf("%s[%d]", object_get_typename(obj), index);
> +
> +    if (obj->parent == parent) {
> +        return;
> +    }
> +
> +    object_ref(obj);
> +    object_unparent(obj);
> +    object_property_add_child(
> +        parent, DEVICE(obj)->id ? DEVICE(obj)->id : default_id, obj);
> +    object_unref(obj);
> +}
> +
>   PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
>   {
>       int i;
> 

Re: [PATCH 06/14] ppc/pnv: Complete user created PHB3 devices

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> PHB3s ared SysBus devices and should be allowed to be dynamically
> created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

This one is a bit of black magic for me. I don't see an equivalent for 
P9 though. Not needed there? I'll have another comment about P8/P9 later.

   Fred


>   hw/pci-host/pnv_phb3.c | 9 +++++++++
>   hw/ppc/pnv.c           | 2 ++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index e91f658b0060..b61f9c369f64 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1000,6 +1000,9 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>   
>       /* User created devices */
>       if (!phb->chip) {
> +        Error *local_err = NULL;
> +        BusState *s;
> +
>           phb->chip = pnv_get_chip(pnv, phb->chip_id);
>           if (!phb->chip) {
>               error_setg(errp, "invalid chip id: %d", phb->chip_id);
> @@ -1011,6 +1014,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>            * correctly the device tree.
>            */
>           pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
> +
> +        s = qdev_get_parent_bus(DEVICE(phb->chip));
> +        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>       }
>   
>       /* LSI sources */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 9a458655efd9..45d8ecbf2bf7 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1927,6 +1927,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
>   
>       pmc->compat = compat;
>       pmc->compat_size = sizeof(compat);
> +
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB3);
>   }
>   
>   static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
> 

Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
> each PEC can have several PHBs :
> 
>    * PEC0 provides 1 PHB  (PHB0)
>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
> 
> A num_pecs class attribute represents better the logic units of the
> POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
> This will ease adding support for user created devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

With this patch, chip->num_phbs is only defined and used on P8. We may 
want to add a comment to make it clear.

As I review this series, something is bugging me though: the difference 
of handling between P8 and P9.
On P9, we seem to have a more logical hiearachy:
phb <- PCI controller (PEC) <- chip

With P8, we don't have an explicit PEC, but we have a PBCQ object, which 
is somewhat similar. The hierarchy seems also more convoluted.
I don't see why it's treated differently. It seems both chips could be 
treated the same, which would make the code easier to follow.
That's outside of the scope of this series though. So maybe for a future 
patch? Who knows, I might volunteer...

   Fred



>   include/hw/ppc/pnv.h |  2 ++
>   hw/ppc/pnv.c         | 20 +++++++++-----------
>   2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 247379ef1f88..f2c238062f4a 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -53,6 +53,7 @@ struct PnvChip {
>       PnvCore      **cores;
>   
>       uint32_t     num_phbs;
> +    uint32_t     num_pecs;
>   
>       MemoryRegion xscom_mmio;
>       MemoryRegion xscom;
> @@ -136,6 +137,7 @@ struct PnvChipClass {
>       uint64_t     chip_cfam_id;
>       uint64_t     cores_mask;
>       uint32_t     num_phbs;
> +    uint32_t     num_pecs;
>   
>       DeviceRealize parent_realize;
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 45d8ecbf2bf7..185464a1d443 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -658,7 +658,7 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip, Monitor *mon)
>       pnv_xive_pic_print_info(&chip9->xive, mon);
>       pnv_psi_pic_print_info(&chip9->psi, mon);
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>           for (j = 0; j < pec->num_stacks; j++) {
>               pnv_phb4_pic_print_info(&pec->stacks[j].phb, mon);
> @@ -1330,15 +1330,14 @@ static void pnv_chip_power9_instance_init(Object *obj)
>   
>       object_initialize_child(obj, "homer", &chip9->homer, TYPE_PNV9_HOMER);
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    if (defaults_enabled()) {
> +        chip->num_pecs = pcc->num_pecs;
> +    }
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
>           object_initialize_child(obj, "pec[*]", &chip9->pecs[i],
>                                   TYPE_PNV_PHB4_PEC);
>       }
> -
> -    /*
> -     * Number of PHBs is the chip default
> -     */
> -    chip->num_phbs = pcc->num_phbs;
>   }
>   
>   static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
> @@ -1374,7 +1373,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>       int i, j;
>       int phb_id = 0;
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>           PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>           uint32_t pec_nest_base;
> @@ -1402,8 +1401,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
>   
> -        for (j = 0; j < pec->num_stacks && phb_id < chip->num_phbs;
> -             j++, phb_id++) {
> +        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
>               PnvPhb4PecStack *stack = &pec->stacks[j];
>               Object *obj = OBJECT(&stack->phb);
>   
> @@ -1559,7 +1557,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>       k->xscom_core_base = pnv_chip_power9_xscom_core_base;
>       k->xscom_pcba = pnv_chip_power9_xscom_pcba;
>       dc->desc = "PowerNV Chip POWER9";
> -    k->num_phbs = 6;
> +    k->num_pecs = PNV9_CHIP_MAX_PEC;
>   
>       device_class_set_parent_realize(dc, pnv_chip_power9_realize,
>                                       &k->parent_realize);
> 

Re: [PATCH 08/14] ppc/pnv: Introduce version and device_id class atributes for PHB4 devices

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


Empty log message ok in qemu?
But it looks ok to me.
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   include/hw/pci-host/pnv_phb4.h | 2 ++
>   hw/pci-host/pnv_phb4_pec.c     | 2 ++
>   hw/ppc/pnv.c                   | 4 ++--
>   3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 27556ae53425..b2864233641e 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -219,6 +219,8 @@ struct PnvPhb4PecClass {
>       int compat_size;
>       const char *stk_compat;
>       int stk_compat_size;
> +    uint64_t version;
> +    uint64_t device_id;
>   };
>   
>   #endif /* PCI_HOST_PNV_PHB4_H */
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 741ddc90ed8d..9f722729ac50 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -499,6 +499,8 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>       pecc->compat_size = sizeof(compat);
>       pecc->stk_compat = stk_compat;
>       pecc->stk_compat_size = sizeof(stk_compat);
> +    pecc->version = PNV_PHB4_VERSION;
> +    pecc->device_id = PNV_PHB4_DEVICE_ID;
>   }
>   
>   static const TypeInfo pnv_pec_type_info = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 185464a1d443..0c65e1e88cf5 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1408,9 +1408,9 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>               object_property_set_int(obj, "index", phb_id, &error_fatal);
>               object_property_set_int(obj, "chip-id", chip->chip_id,
>                                       &error_fatal);
> -            object_property_set_int(obj, "version", PNV_PHB4_VERSION,
> +            object_property_set_int(obj, "version", pecc->version,
>                                       &error_fatal);
> -            object_property_set_int(obj, "device-id", PNV_PHB4_DEVICE_ID,
> +            object_property_set_int(obj, "device-id", pecc->device_id,
>                                       &error_fatal);
>               object_property_set_link(obj, "stack", OBJECT(stack),
>                                        &error_abort);
> 

Re: [PATCH 09/14] ppc/pnv: Introduce a "chip" property under the PHB4 model

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> Next changes will make use of it.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   include/hw/pci-host/pnv_phb4.h | 2 ++
>   hw/pci-host/pnv_phb4_pec.c     | 2 ++
>   hw/ppc/pnv.c                   | 2 ++
>   3 files changed, 6 insertions(+)
> 
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index b2864233641e..8a585c9a42f7 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -205,6 +205,8 @@ struct PnvPhb4PecState {
>       #define PHB4_PEC_MAX_STACKS     3
>       uint32_t num_stacks;
>       PnvPhb4PecStack stacks[PHB4_PEC_MAX_STACKS];
> +
> +    PnvChip *chip;
>   };
>   
>   
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 9f722729ac50..e9750c41c595 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -462,6 +462,8 @@ static Property pnv_pec_properties[] = {
>           DEFINE_PROP_UINT32("index", PnvPhb4PecState, index, 0),
>           DEFINE_PROP_UINT32("num-stacks", PnvPhb4PecState, num_stacks, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
> +        DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
> +                         PnvChip *),
>           DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
>                        TYPE_MEMORY_REGION, MemoryRegion *),
>           DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0c65e1e88cf5..76b2f5468b38 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1389,6 +1389,8 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>                                   &error_fatal);
>           object_property_set_int(OBJECT(pec), "chip-id", chip->chip_id,
>                                   &error_fatal);
> +        object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
> +                                 &error_fatal);
>           object_property_set_link(OBJECT(pec), "system-memory",
>                                    OBJECT(get_system_memory()), &error_abort);
>           if (!qdev_realize(DEVICE(pec), NULL, errp)) {
> 

Re: [PATCH 10/14] ppc/pnv: Introduce a num_stack class attribute

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> Each PEC devices of the POWER9 chip has a predefined number of stacks,
> equivalent of a root port complex:
> 
>    PEC0 -> 1 stack
>    PEC1 -> 2 stacks
>    PEC2 -> 3 stacks
> 
> Introduce a class attribute to hold these values and remove the
> "num-stacks" property.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   include/hw/pci-host/pnv_phb4.h |  1 +
>   hw/pci-host/pnv_phb4_pec.c     | 17 ++++++++++++++++-
>   hw/ppc/pnv.c                   |  7 -------
>   3 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 8a585c9a42f7..60de3031a622 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -223,6 +223,7 @@ struct PnvPhb4PecClass {
>       int stk_compat_size;
>       uint64_t version;
>       uint64_t device_id;
> +    const uint32_t *num_stacks;
>   };
>   
>   #endif /* PCI_HOST_PNV_PHB4_H */
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index e9750c41c595..293909b5cb90 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -377,11 +377,19 @@ static void pnv_pec_instance_init(Object *obj)
>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>       char name[64];
>       int i;
>   
>       assert(pec->system_memory);
>   
> +    if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
> +        error_setg(errp, "invalid PEC index: %d", pec->index);
> +        return;
> +    }
> +
> +    pec->num_stacks = pecc->num_stacks[pec->index];
> +
>       /* Create stacks */
>       for (i = 0; i < pec->num_stacks; i++) {
>           PnvPhb4PecStack *stack = &pec->stacks[i];
> @@ -460,7 +468,6 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>   
>   static Property pnv_pec_properties[] = {
>           DEFINE_PROP_UINT32("index", PnvPhb4PecState, index, 0),
> -        DEFINE_PROP_UINT32("num-stacks", PnvPhb4PecState, num_stacks, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
>           DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
>                            PnvChip *),
> @@ -479,6 +486,13 @@ static uint32_t pnv_pec_xscom_nest_base(PnvPhb4PecState *pec)
>       return PNV9_XSCOM_PEC_NEST_BASE + 0x400 * pec->index;
>   }
>   
> +/*
> + * PEC0 -> 1 stack
> + * PEC1 -> 2 stacks
> + * PEC2 -> 3 stacks
> + */
> +static const uint32_t pnv_pec_num_stacks[] = { 1, 2, 3 };
> +
>   static void pnv_pec_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -503,6 +517,7 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>       pecc->stk_compat_size = sizeof(stk_compat);
>       pecc->version = PNV_PHB4_VERSION;
>       pecc->device_id = PNV_PHB4_DEVICE_ID;
> +    pecc->num_stacks = pnv_pec_num_stacks;
>   }
>   
>   static const TypeInfo pnv_pec_type_info = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 76b2f5468b38..957f0bdfaa6b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1380,13 +1380,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>           uint32_t pec_pci_base;
>   
>           object_property_set_int(OBJECT(pec), "index", i, &error_fatal);
> -        /*
> -         * PEC0 -> 1 stack
> -         * PEC1 -> 2 stacks
> -         * PEC2 -> 3 stacks
> -         */
> -        object_property_set_int(OBJECT(pec), "num-stacks", i + 1,
> -                                &error_fatal);
>           object_property_set_int(OBJECT(pec), "chip-id", chip->chip_id,
>                                   &error_fatal);
>           object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
> 

Re: [PATCH 11/14] ppc/pnv: Compute the PHB index from the PHB4 PEC model

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> Use the num_stacks class attribute to compute the PHB index depending
> on the PEC index :
> 
>    * PEC0 provides 1 PHB  (PHB0)
>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/pci-host/pnv_phb4_pec.c | 16 ++++++++++++++++
>   hw/ppc/pnv.c               |  4 +---
>   2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 293909b5cb90..a7dd4173d598 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -374,6 +374,19 @@ static void pnv_pec_instance_init(Object *obj)
>       }
>   }
>   
> +static int pnv_pec_phb_offset(PnvPhb4PecState *pec)
> +{
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    int index = pec->index;
> +    int offset = 0;
> +
> +    while (index--) {
> +        offset += pecc->num_stacks[index];
> +    }
> +
> +    return offset;
> +}
> +


That seems overly complicated to me and not very readable. The log 
message is a lot more clear. I'd prefer we have a switch() statement 
returning the base PHB ID based on the PEC index.

   Fred



>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
> @@ -394,8 +407,10 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       for (i = 0; i < pec->num_stacks; i++) {
>           PnvPhb4PecStack *stack = &pec->stacks[i];
>           Object *stk_obj = OBJECT(stack);
> +        int phb_id = pnv_pec_phb_offset(pec) + i;
>   
>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
> +        object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
>           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
> @@ -538,6 +553,7 @@ static void pnv_pec_stk_instance_init(Object *obj)
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
>   
>       object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4);
> +    object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index");
>   }
>   
>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 957f0bdfaa6b..f8b0b2a28383 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1371,7 +1371,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   {
>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
>       int i, j;
> -    int phb_id = 0;
>   
>       for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
> @@ -1396,11 +1395,10 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
>   
> -        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
> +        for (j = 0; j < pec->num_stacks; j++) {
>               PnvPhb4PecStack *stack = &pec->stacks[j];
>               Object *obj = OBJECT(&stack->phb);
>   
> -            object_property_set_int(obj, "index", phb_id, &error_fatal);
>               object_property_set_int(obj, "chip-id", chip->chip_id,
>                                       &error_fatal);
>               object_property_set_int(obj, "version", pecc->version,
> 

Re: [PATCH 12/14] ppc/pnv: Remove "system-memory" property for he PHB4 PEC model

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> This is not useful and will be in the way for support of user created
> PHB4 devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


I doubt I see all the implications here, but it doesn't look wrong to 
me, so:
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   hw/pci-host/pnv_phb4_pec.c | 6 +-----
>   hw/ppc/pnv.c               | 2 --
>   2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index a7dd4173d598..dfed2af0f7df 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -124,7 +124,7 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
>   static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>   {
>       PnvPhb4PecState *pec = stack->pec;
> -    MemoryRegion *sysmem = pec->system_memory;
> +    MemoryRegion *sysmem = get_system_memory();
>       uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
>       uint64_t bar, mask, size;
>       char name[64];
> @@ -394,8 +394,6 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       char name[64];
>       int i;
>   
> -    assert(pec->system_memory);
> -
>       if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
>           error_setg(errp, "invalid PEC index: %d", pec->index);
>           return;
> @@ -486,8 +484,6 @@ static Property pnv_pec_properties[] = {
>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
>           DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
>                            PnvChip *),
> -        DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
> -                     TYPE_MEMORY_REGION, MemoryRegion *),
>           DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index f8b0b2a28383..3a550eed0f36 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1383,8 +1383,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>                                   &error_fatal);
>           object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
>                                    &error_fatal);
> -        object_property_set_link(OBJECT(pec), "system-memory",
> -                                 OBJECT(get_system_memory()), &error_abort);
>           if (!qdev_realize(DEVICE(pec), NULL, errp)) {
>               return;
>           }
> 

Re: [PATCH 13/14] ppc/pnv: Move realize of PEC stacks under the PEC model

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> This change will help us providing support for user created PHB4
> devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/pci-host/pnv_phb4_pec.c | 36 ++++++++++++++++++++++++++++++++----
>   hw/ppc/pnv.c               | 31 +------------------------------
>   2 files changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index dfed2af0f7df..9b081d543057 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -556,6 +556,10 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
>       PnvPhb4PecState *pec = stack->pec;
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    PnvChip *chip = pec->chip;
> +    uint32_t pec_nest_base;
> +    uint32_t pec_pci_base;
>       char name[64];
>   
>       assert(pec);
> @@ -579,10 +583,34 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>       pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
>                             &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
>   
> -    /*
> -     * Let the machine/chip realize the PHB object to customize more
> -     * easily some fields
> -     */
> +    {
> +        Object *obj = OBJECT(&stack->phb);
> +
> +        object_property_set_int(obj, "chip-id", pec->chip_id, &error_fatal);
> +        object_property_set_int(obj, "version", pecc->version, &error_fatal);
> +        object_property_set_int(obj, "device-id", pecc->device_id,
> +                                &error_fatal);
> +        object_property_set_link(obj, "stack", OBJECT(stack),
> +                                 &error_abort);
> +        if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
> +            return;
> +        }
> +    }


Do we really need the extra sub-scope here? It looks off.


> +
> +    pec_nest_base = pecc->xscom_nest_base(pec);
> +    pec_pci_base = pecc->xscom_pci_base(pec);
> +
> +    /* Populate the XSCOM address space. */
> +    pnv_xscom_add_subregion(chip,
> +                            pec_nest_base + 0x40 * (stack->stack_no + 1),
> +                            &stack->nest_regs_mr);
> +    pnv_xscom_add_subregion(chip,
> +                            pec_pci_base + 0x40 * (stack->stack_no + 1),
> +                            &stack->pci_regs_mr);
> +    pnv_xscom_add_subregion(chip,
> +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> +                            0x40 * stack->stack_no,
> +                            &stack->phb_regs_mr);
>   }
>   
>   static Property pnv_pec_stk_properties[] = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3a550eed0f36..7e13b15241fd 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1370,7 +1370,7 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
>   static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   {


With that change, we should really rename pnv_chip_power9_phb_realize() 
to pnv_chip_power9_pec_realize().

   Fred


>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
> -    int i, j;
> +    int i;
>   
>       for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
> @@ -1392,35 +1392,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
> -
> -        for (j = 0; j < pec->num_stacks; j++) {
> -            PnvPhb4PecStack *stack = &pec->stacks[j];
> -            Object *obj = OBJECT(&stack->phb);
> -
> -            object_property_set_int(obj, "chip-id", chip->chip_id,
> -                                    &error_fatal);
> -            object_property_set_int(obj, "version", pecc->version,
> -                                    &error_fatal);
> -            object_property_set_int(obj, "device-id", pecc->device_id,
> -                                    &error_fatal);
> -            object_property_set_link(obj, "stack", OBJECT(stack),
> -                                     &error_abort);
> -            if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
> -                return;
> -            }
> -
> -            /* Populate the XSCOM address space. */
> -            pnv_xscom_add_subregion(chip,
> -                                   pec_nest_base + 0x40 * (stack->stack_no + 1),
> -                                   &stack->nest_regs_mr);
> -            pnv_xscom_add_subregion(chip,
> -                                    pec_pci_base + 0x40 * (stack->stack_no + 1),
> -                                    &stack->pci_regs_mr);
> -            pnv_xscom_add_subregion(chip,
> -                                    pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> -                                    0x40 * stack->stack_no,
> -                                    &stack->phb_regs_mr);
> -        }
>       }
>   }
>   
> 

Re: [PATCH 01/14] ppc/pnv: Reduce the maximum of PHB3 devices

[Cédric Le Goater]

On 12/7/21 10:40, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> All POWER8 machines have a maximum of 3 PHB3 devices. Adapt the
>> PNV8_CHIP_PHB3_MAX definition for consistency.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> 
> The Naples chip (Garrison) can have 4 PHBs and it seems we have a power8nvl machine type for it. So I guess we should keep a max PHB count of 4 there.

Ah yes. This is the reason behind the 4. I should update the power8nvl
chip then.

Thanks,

C.

> 
>    Fred
> 
> 
> 
>>   include/hw/ppc/pnv.h | 2 +-
>>   hw/ppc/pnv.c         | 6 +++---
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index aa08d79d24de..6f498c8f1b5f 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -79,7 +79,7 @@ struct Pnv8Chip {
>>       PnvOCC       occ;
>>       PnvHomer     homer;
>> -#define PNV8_CHIP_PHB3_MAX 4
>> +#define PNV8_CHIP_PHB3_MAX 3
>>       PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>>       XICSFabric    *xics;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 71e45515f136..bd768dcc28ad 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1256,7 +1256,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>>       k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>>       k->cores_mask = POWER8E_CORE_MASK;
>> -    k->num_phbs = 3;
>> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>>       k->core_pir = pnv_chip_core_pir_p8;
>>       k->intc_create = pnv_chip_power8_intc_create;
>>       k->intc_reset = pnv_chip_power8_intc_reset;
>> @@ -1280,7 +1280,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>>       k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>>       k->cores_mask = POWER8_CORE_MASK;
>> -    k->num_phbs = 3;
>> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>>       k->core_pir = pnv_chip_core_pir_p8;
>>       k->intc_create = pnv_chip_power8_intc_create;
>>       k->intc_reset = pnv_chip_power8_intc_reset;
>> @@ -1304,7 +1304,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>>       k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>>       k->cores_mask = POWER8_CORE_MASK;
>> -    k->num_phbs = 3;
>> +    k->num_phbs = PNV8_CHIP_PHB3_MAX;
>>       k->core_pir = pnv_chip_core_pir_p8;
>>       k->intc_create = pnv_chip_power8_intc_create;
>>       k->intc_reset = pnv_chip_power8_intc_reset;
>>

Re: [PATCH 12/14] ppc/pnv: Remove "system-memory" property for he PHB4 PEC model

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> This is not useful and will be in the way for support of user created
> PHB4 devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


I forgot to mention the typo in the commit title: "he PHB4".

   Fred


>   hw/pci-host/pnv_phb4_pec.c | 6 +-----
>   hw/ppc/pnv.c               | 2 --
>   2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index a7dd4173d598..dfed2af0f7df 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -124,7 +124,7 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
>   static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>   {
>       PnvPhb4PecState *pec = stack->pec;
> -    MemoryRegion *sysmem = pec->system_memory;
> +    MemoryRegion *sysmem = get_system_memory();
>       uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
>       uint64_t bar, mask, size;
>       char name[64];
> @@ -394,8 +394,6 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       char name[64];
>       int i;
>   
> -    assert(pec->system_memory);
> -
>       if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
>           error_setg(errp, "invalid PEC index: %d", pec->index);
>           return;
> @@ -486,8 +484,6 @@ static Property pnv_pec_properties[] = {
>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
>           DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
>                            PnvChip *),
> -        DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
> -                     TYPE_MEMORY_REGION, MemoryRegion *),
>           DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index f8b0b2a28383..3a550eed0f36 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1383,8 +1383,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>                                   &error_fatal);
>           object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
>                                    &error_fatal);
> -        object_property_set_link(OBJECT(pec), "system-memory",
> -                                 OBJECT(get_system_memory()), &error_abort);
>           if (!qdev_realize(DEVICE(pec), NULL, errp)) {
>               return;
>           }
> 

Re: [PATCH 14/14] ppc/pnv: Introduce support for user created PHB4 devices

[Frederic Barrat]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> PHB4 devices and PCI devices can now be added to the powernv9 machine
> using:
> 
>    -device pnv-phb4-pec,chip-id=0,index=0
>    -device nec-usb-xhci,bus=pci.0,addr=0x0
> 
> In case of user created devices, a lookup on 'chip-id' is required to
> assign the owning chip.
> 
> To be noted, that the PEC PHB4 devices can add more than one PHB4
> devices:
> 
>    * PEC0 provides 1 PHB  (PHB0)
>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   hw/pci-host/pnv_phb4_pec.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 9b081d543057..4ee92f11945c 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -394,6 +394,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       char name[64];
>       int i;
>   
> +    /* User created devices */
> +    if (!pec->chip) {
> +        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +
> +        pec->chip = pnv_get_chip(pnv, pec->chip_id);
> +        if (!pec->chip) {
> +            error_setg(errp, "invalid chip id: %d", pec->chip_id);
> +            return;
> +        }
> +    }
> +
>       if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
>           error_setg(errp, "invalid PEC index: %d", pec->index);
>           return;
> @@ -401,6 +412,12 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   
>       pec->num_stacks = pecc->num_stacks[pec->index];
>   
> +    /*
> +     * Reparent user created devices to the chip to build correctly
> +     * the device tree.
> +     */
> +    pnv_chip_parent_fixup(pec->chip, OBJECT(pec), pec->index);
> +
>       /* Create stacks */
>       for (i = 0; i < pec->num_stacks; i++) {
>           PnvPhb4PecStack *stack = &pec->stacks[i];
> @@ -516,7 +533,7 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>   
>       dc->realize = pnv_pec_realize;
>       device_class_set_props(dc, pnv_pec_properties);
> -    dc->user_creatable = false;
> +    dc->user_creatable = true;
>   
>       pecc->xscom_nest_base = pnv_pec_xscom_nest_base;
>       pecc->xscom_pci_base  = pnv_pec_xscom_pci_base;
> 

Re: [PATCH 04/14] ppc/pnv: Introduce support for user created PHB3 devices

[Cédric Le Goater]

On 12/7/21 10:47, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> PHB3 devices and PCI devices can now be added to the powernv8 machine
>> using :
>>
>>    -device pnv-phb3,chip-id=0,index=1 \
>>    -device nec-usb-xhci,bus=pci.1,addr=0x0
>>
>> The 'index' property identifies the PHB3 in the chip. In case of user
>> created devices, a lookup on 'chip-id' is required to assign the
>> owning chip.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index de277c457838..d7fe92cb082d 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1097,14 +1097,14 @@ static void pnv_chip_power8_instance_init(Object *obj)
>>       object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
>> -    for (i = 0; i < pcc->num_phbs; i++) {
>> +    if (defaults_enabled()) {
>> +        chip->num_phbs = pcc->num_phbs;
>> +    }
>> +
>> +    for (i = 0; i < chip->num_phbs; i++) {
>>           object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
>>       }
>> -    /*
>> -     * Number of PHBs is the chip default
>> -     */
>> -    chip->num_phbs = pcc->num_phbs;
>>   }
> 
> 
> So if "-nodefaults" is mentioned on the command line, then chip->num_phbs is not set. It seems the intention is to have only the PHBs defined on the CLI, which is fine. However, I don't see where chip->num_phbs is incremented in that case.

Good catch :) That's why we need another patch fixing all this because
it is breaking the XICS fabric handlers, ics_get and ics_resend.
'info pic' is impacted also.

Here is the proposed fix for v2 :

  https://github.com/legoater/qemu/commit/b47bce3109f316a65aa2fa2a46651b2960e93fca

I chose to loop on the children of the chip to find the user
created devices and leave the PnvChip model with empty defaults.


'info pic' is impacted the same on P9

   https://github.com/legoater/qemu/commit/d4733edca94c95f717f4ee35bbea6dc085365286

Thanks,

C.

Re: [PATCH 06/14] ppc/pnv: Complete user created PHB3 devices

[Cédric Le Goater]

On 12/7/21 10:53, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> PHB3s ared SysBus devices and should be allowed to be dynamically
>> created.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> This one is a bit of black magic for me. 

Yes. QEMU internals related to sysbus. I am not an expert either.

> I don't see an equivalent for P9 though. Not needed there? 

No because the phb4-pec devices are simple devices. Not tied to sysbus.

Thanks,

C.

> I'll have another comment about P8/P9 later.
> 
>    Fred
> 
> 
>>   hw/pci-host/pnv_phb3.c | 9 +++++++++
>>   hw/ppc/pnv.c           | 2 ++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index e91f658b0060..b61f9c369f64 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -1000,6 +1000,9 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>>       /* User created devices */
>>       if (!phb->chip) {
>> +        Error *local_err = NULL;
>> +        BusState *s;
>> +
>>           phb->chip = pnv_get_chip(pnv, phb->chip_id);
>>           if (!phb->chip) {
>>               error_setg(errp, "invalid chip id: %d", phb->chip_id);
>> @@ -1011,6 +1014,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>>            * correctly the device tree.
>>            */
>>           pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
>> +
>> +        s = qdev_get_parent_bus(DEVICE(phb->chip));
>> +        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>>       }
>>       /* LSI sources */
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 9a458655efd9..45d8ecbf2bf7 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1927,6 +1927,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
>>       pmc->compat = compat;
>>       pmc->compat_size = sizeof(compat);
>> +
>> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB3);
>>   }
>>   static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>>

Re: [PATCH 08/14] ppc/pnv: Introduce version and device_id class atributes for PHB4 devices

[Cédric Le Goater]

On 12/7/21 11:01, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> 
> Empty log message ok in qemu?

checkpatch didn't complain :) I might make an effort for v2.

Thanks,

C.


> But it looks ok to me.> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> 
> 
>>   include/hw/pci-host/pnv_phb4.h | 2 ++
>>   hw/pci-host/pnv_phb4_pec.c     | 2 ++
>>   hw/ppc/pnv.c                   | 4 ++--
>>   3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
>> index 27556ae53425..b2864233641e 100644
>> --- a/include/hw/pci-host/pnv_phb4.h
>> +++ b/include/hw/pci-host/pnv_phb4.h
>> @@ -219,6 +219,8 @@ struct PnvPhb4PecClass {
>>       int compat_size;
>>       const char *stk_compat;
>>       int stk_compat_size;
>> +    uint64_t version;
>> +    uint64_t device_id;
>>   };
>>   #endif /* PCI_HOST_PNV_PHB4_H */
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 741ddc90ed8d..9f722729ac50 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -499,6 +499,8 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>>       pecc->compat_size = sizeof(compat);
>>       pecc->stk_compat = stk_compat;
>>       pecc->stk_compat_size = sizeof(stk_compat);
>> +    pecc->version = PNV_PHB4_VERSION;
>> +    pecc->device_id = PNV_PHB4_DEVICE_ID;
>>   }
>>   static const TypeInfo pnv_pec_type_info = {
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 185464a1d443..0c65e1e88cf5 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1408,9 +1408,9 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>>               object_property_set_int(obj, "index", phb_id, &error_fatal);
>>               object_property_set_int(obj, "chip-id", chip->chip_id,
>>                                       &error_fatal);
>> -            object_property_set_int(obj, "version", PNV_PHB4_VERSION,
>> +            object_property_set_int(obj, "version", pecc->version,
>>                                       &error_fatal);
>> -            object_property_set_int(obj, "device-id", PNV_PHB4_DEVICE_ID,
>> +            object_property_set_int(obj, "device-id", pecc->device_id,
>>                                       &error_fatal);
>>               object_property_set_link(obj, "stack", OBJECT(stack),
>>                                        &error_abort);
>>

Re: [PATCH 11/14] ppc/pnv: Compute the PHB index from the PHB4 PEC model

[Cédric Le Goater]

On 12/7/21 11:06, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> Use the num_stacks class attribute to compute the PHB index depending
>> on the PEC index :
>>
>>    * PEC0 provides 1 PHB  (PHB0)
>>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/pci-host/pnv_phb4_pec.c | 16 ++++++++++++++++
>>   hw/ppc/pnv.c               |  4 +---
>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 293909b5cb90..a7dd4173d598 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -374,6 +374,19 @@ static void pnv_pec_instance_init(Object *obj)
>>       }
>>   }
>> +static int pnv_pec_phb_offset(PnvPhb4PecState *pec)
>> +{
>> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>> +    int index = pec->index;
>> +    int offset = 0;
>> +
>> +    while (index--) {
>> +        offset += pecc->num_stacks[index];
>> +    }
>> +
>> +    return offset;
>> +}
>> +
> 
> 
> That seems overly complicated to me and not very readable. The log message is a lot more clear. I'd prefer we have a switch() statement returning the base PHB ID based on the PEC index.

yes I agree but PHB5 is on its way and this is compatible. See :

   https://github.com/legoater/qemu/commit/d7caa1b74f3f8a90815fa086b87a1bd467b7c988

I will take any good idea fitting both :)

Thanks,

C.

> 
>    Fred
> 
> 
> 
>>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
>> @@ -394,8 +407,10 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>>       for (i = 0; i < pec->num_stacks; i++) {
>>           PnvPhb4PecStack *stack = &pec->stacks[i];
>>           Object *stk_obj = OBJECT(stack);
>> +        int phb_id = pnv_pec_phb_offset(pec) + i;
>>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
>> +        object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
>>           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
>>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>>               return;
>> @@ -538,6 +553,7 @@ static void pnv_pec_stk_instance_init(Object *obj)
>>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
>>       object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4);
>> +    object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index");
>>   }
>>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 957f0bdfaa6b..f8b0b2a28383 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1371,7 +1371,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>>   {
>>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
>>       int i, j;
>> -    int phb_id = 0;
>>       for (i = 0; i < chip->num_pecs; i++) {
>>           PnvPhb4PecState *pec = &chip9->pecs[i];
>> @@ -1396,11 +1395,10 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
>> -        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
>> +        for (j = 0; j < pec->num_stacks; j++) {
>>               PnvPhb4PecStack *stack = &pec->stacks[j];
>>               Object *obj = OBJECT(&stack->phb);
>> -            object_property_set_int(obj, "index", phb_id, &error_fatal);
>>               object_property_set_int(obj, "chip-id", chip->chip_id,
>>                                       &error_fatal);
>>               object_property_set_int(obj, "version", pecc->version,
>>

Re: [PATCH 12/14] ppc/pnv: Remove "system-memory" property for he PHB4 PEC model

[Cédric Le Goater]

On 12/7/21 11:08, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> This is not useful and will be in the way for support of user created
>> PHB4 devices.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> 
> I doubt I see all the implications here, 

It is good practice to avoid statics in models or calls like
get_system_memory() or qdev_get_machine(). With dynamic models,
it becomes more complex.

Thanks,

C.

> but it doesn't look wrong to me, so:
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> 
>    Fred
> 
> 
>>   hw/pci-host/pnv_phb4_pec.c | 6 +-----
>>   hw/ppc/pnv.c               | 2 --
>>   2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index a7dd4173d598..dfed2af0f7df 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -124,7 +124,7 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
>>   static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>>   {
>>       PnvPhb4PecState *pec = stack->pec;
>> -    MemoryRegion *sysmem = pec->system_memory;
>> +    MemoryRegion *sysmem = get_system_memory();
>>       uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
>>       uint64_t bar, mask, size;
>>       char name[64];
>> @@ -394,8 +394,6 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>>       char name[64];
>>       int i;
>> -    assert(pec->system_memory);
>> -
>>       if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
>>           error_setg(errp, "invalid PEC index: %d", pec->index);
>>           return;
>> @@ -486,8 +484,6 @@ static Property pnv_pec_properties[] = {
>>           DEFINE_PROP_UINT32("chip-id", PnvPhb4PecState, chip_id, 0),
>>           DEFINE_PROP_LINK("chip", PnvPhb4PecState, chip, TYPE_PNV_CHIP,
>>                            PnvChip *),
>> -        DEFINE_PROP_LINK("system-memory", PnvPhb4PecState, system_memory,
>> -                     TYPE_MEMORY_REGION, MemoryRegion *),
>>           DEFINE_PROP_END_OF_LIST(),
>>   };
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index f8b0b2a28383..3a550eed0f36 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1383,8 +1383,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>>                                   &error_fatal);
>>           object_property_set_link(OBJECT(pec), "chip", OBJECT(chip),
>>                                    &error_fatal);
>> -        object_property_set_link(OBJECT(pec), "system-memory",
>> -                                 OBJECT(get_system_memory()), &error_abort);
>>           if (!qdev_realize(DEVICE(pec), NULL, errp)) {
>>               return;
>>           }
>>

Re: [PATCH 13/14] ppc/pnv: Move realize of PEC stacks under the PEC model

[Cédric Le Goater]

On 12/7/21 11:10, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> This change will help us providing support for user created PHB4
>> devices.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/pci-host/pnv_phb4_pec.c | 36 ++++++++++++++++++++++++++++++++----
>>   hw/ppc/pnv.c               | 31 +------------------------------
>>   2 files changed, 33 insertions(+), 34 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index dfed2af0f7df..9b081d543057 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -556,6 +556,10 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
>>       PnvPhb4PecState *pec = stack->pec;
>> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>> +    PnvChip *chip = pec->chip;
>> +    uint32_t pec_nest_base;
>> +    uint32_t pec_pci_base;
>>       char name[64];
>>       assert(pec);
>> @@ -579,10 +583,34 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>>       pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
>>                             &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
>> -    /*
>> -     * Let the machine/chip realize the PHB object to customize more
>> -     * easily some fields
>> -     */
>> +    {
>> +        Object *obj = OBJECT(&stack->phb);
>> +
>> +        object_property_set_int(obj, "chip-id", pec->chip_id, &error_fatal);
>> +        object_property_set_int(obj, "version", pecc->version, &error_fatal);
>> +        object_property_set_int(obj, "device-id", pecc->device_id,
>> +                                &error_fatal);
>> +        object_property_set_link(obj, "stack", OBJECT(stack),
>> +                                 &error_abort);
>> +        if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
>> +            return;
>> +        }
>> +    }
> 
> 
> Do we really need the extra sub-scope here? It looks off.

No. That's a left over from the initial patches I worked on.

>> +
>> +    pec_nest_base = pecc->xscom_nest_base(pec);
>> +    pec_pci_base = pecc->xscom_pci_base(pec);
>> +
>> +    /* Populate the XSCOM address space. */
>> +    pnv_xscom_add_subregion(chip,
>> +                            pec_nest_base + 0x40 * (stack->stack_no + 1),
>> +                            &stack->nest_regs_mr);
>> +    pnv_xscom_add_subregion(chip,
>> +                            pec_pci_base + 0x40 * (stack->stack_no + 1),
>> +                            &stack->pci_regs_mr);
>> +    pnv_xscom_add_subregion(chip,
>> +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>> +                            0x40 * stack->stack_no,
>> +                            &stack->phb_regs_mr);
>>   }
>>   static Property pnv_pec_stk_properties[] = {
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3a550eed0f36..7e13b15241fd 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1370,7 +1370,7 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
>>   static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>>   {
> 
> 
> With that change, we should really rename pnv_chip_power9_phb_realize() to pnv_chip_power9_pec_realize().

yes.

Thanks,

C.


> 
>    Fred
> 
> 
>>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
>> -    int i, j;
>> +    int i;
>>       for (i = 0; i < chip->num_pecs; i++) {
>>           PnvPhb4PecState *pec = &chip9->pecs[i];
>> @@ -1392,35 +1392,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
>> -
>> -        for (j = 0; j < pec->num_stacks; j++) {
>> -            PnvPhb4PecStack *stack = &pec->stacks[j];
>> -            Object *obj = OBJECT(&stack->phb);
>> -
>> -            object_property_set_int(obj, "chip-id", chip->chip_id,
>> -                                    &error_fatal);
>> -            object_property_set_int(obj, "version", pecc->version,
>> -                                    &error_fatal);
>> -            object_property_set_int(obj, "device-id", pecc->device_id,
>> -                                    &error_fatal);
>> -            object_property_set_link(obj, "stack", OBJECT(stack),
>> -                                     &error_abort);
>> -            if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
>> -                return;
>> -            }
>> -
>> -            /* Populate the XSCOM address space. */
>> -            pnv_xscom_add_subregion(chip,
>> -                                   pec_nest_base + 0x40 * (stack->stack_no + 1),
>> -                                   &stack->nest_regs_mr);
>> -            pnv_xscom_add_subregion(chip,
>> -                                    pec_pci_base + 0x40 * (stack->stack_no + 1),
>> -                                    &stack->pci_regs_mr);
>> -            pnv_xscom_add_subregion(chip,
>> -                                    pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>> -                                    0x40 * stack->stack_no,
>> -                                    &stack->phb_regs_mr);
>> -        }
>>       }
>>   }
>>

Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices

[Cédric Le Goater]

On 12/7/21 11:00, Frederic Barrat wrote:
> 
> 
> On 02/12/2021 15:42, Cédric Le Goater wrote:
>> POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
>> each PEC can have several PHBs :
>>
>>    * PEC0 provides 1 PHB  (PHB0)
>>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
>>
>> A num_pecs class attribute represents better the logic units of the
>> POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
>> This will ease adding support for user created devices.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> With this patch, chip->num_phbs is only defined and used on P8. We may want to add a comment to make it clear.

Yes.

With the latest changes, I think we can now move num_phbs under PnvChip8
and num_pecs under PnvChip9 since they are only used in these routines :

P8:
     static void pnv_chip_power8_instance_init(Object *obj)
             chip->num_phbs = pcc->num_phbs;
         for (i = 0; i < chip->num_phbs; i++) {

     static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
         for (i = 0; i < chip->num_phbs; i++) {
     
P9:
     static void pnv_chip_power9_instance_init(Object *obj)
             chip->num_pecs = pcc->num_pecs;
         for (i = 0; i < chip->num_pecs; i++) {
	
     static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
         for (i = 0; i < chip->num_pecs; i++) {
     

> As I review this series, something is bugging me though: the difference of handling between P8 and P9.
> On P9, we seem to have a more logical hiearachy:
> phb <- PCI controller (PEC) <- chip

Yes. It's cleaner than P8 in terms of logic. P8 initial support was
done hastily for skiboot bringup in 2014.

> With P8, we don't have an explicit PEC, but we have a PBCQ object, which is somewhat similar. The hierarchy seems also more convoluted.

But we don't have stacks on P8. Do we ?

> I don't see why it's treated differently. It seems both chips could be treated the same, which would make the code easier to follow.

I agree. Daniel certainly would also :)

> That's outside of the scope of this series though. 

Well, this patchset enables libvirt support for the PowerNV machines.
Once this is pushed, we need to keep the API, the object model names
being part of it.

7.0 is a good time for a change, really. After that, we won't be able
to change the QOM hierarchy that much.

> So maybe for a future patch? Who knows, I might volunteer...

You would introduce a phb3-pec on top of the phb3s ?

Let me send a v2 first and may be we could rework the object hierarchy
in the 7.0 time frame. We don't have to merge this ASAP.

Thanks,

C.

Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices

[Frederic Barrat]

On 07/12/2021 11:45, Cédric Le Goater wrote:
> On 12/7/21 11:00, Frederic Barrat wrote:
>>
>>
>> On 02/12/2021 15:42, Cédric Le Goater wrote:
>>> POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
>>> each PEC can have several PHBs :
>>>
>>>    * PEC0 provides 1 PHB  (PHB0)
>>>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>>>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
>>>
>>> A num_pecs class attribute represents better the logic units of the
>>> POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
>>> This will ease adding support for user created devices.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>
>> With this patch, chip->num_phbs is only defined and used on P8. We may 
>> want to add a comment to make it clear.
> 
> Yes.
> 
> With the latest changes, I think we can now move num_phbs under PnvChip8
> and num_pecs under PnvChip9 since they are only used in these routines :
> 
> P8:
>      static void pnv_chip_power8_instance_init(Object *obj)
>              chip->num_phbs = pcc->num_phbs;
>          for (i = 0; i < chip->num_phbs; i++) {
> 
>      static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>          for (i = 0; i < chip->num_phbs; i++) {
> P9:
>      static void pnv_chip_power9_instance_init(Object *obj)
>              chip->num_pecs = pcc->num_pecs;
>          for (i = 0; i < chip->num_pecs; i++) {
> 
>      static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>          for (i = 0; i < chip->num_pecs; i++) {
> 
>> As I review this series, something is bugging me though: the 
>> difference of handling between P8 and P9.
>> On P9, we seem to have a more logical hiearachy:
>> phb <- PCI controller (PEC) <- chip
> 
> Yes. It's cleaner than P8 in terms of logic. P8 initial support was
> done hastily for skiboot bringup in 2014.
> 
>> With P8, we don't have an explicit PEC, but we have a PBCQ object, 
>> which is somewhat similar. The hierarchy seems also more convoluted.
> 
> But we don't have stacks on P8. Do we ?


Stacks were introduced on P9 because all the lanes handled by a PEC 
could be grouped differently, each group being called a stack. And each 
stack is associated to a PHB.
On P8, there's no such split, so the doc didn't mention stacks. But each 
PEC handles exactly one PHB. So we could still keep the same abstractions.
On all chips, a PEC would really be equal to a pbcq interface to the 
power bus. The pbcq is servicing one (on P8) or more (on P9/P10) PHBs.



>> I don't see why it's treated differently. It seems both chips could be 
>> treated the same, which would make the code easier to follow.
> 
> I agree. Daniel certainly would also :)
> 
>> That's outside of the scope of this series though. 
> 
> Well, this patchset enables libvirt support for the PowerNV machines.
> Once this is pushed, we need to keep the API, the object model names
> being part of it.
> 
> 7.0 is a good time for a change, really. After that, we won't be able
> to change the QOM hierarchy that much.
> 
>> So maybe for a future patch? Who knows, I might volunteer...
> 
> You would introduce a phb3-pec on top of the phb3s ?


Or rename pnv_phb3_pbcq.c to pnv_phb3_pec.c and starts from there. 
Conceptually, the TYPE_PNV_PBCQ and TYPE_PNV_PHB4_PEC_STACK objects seem 
close. But that's easy to say in an email...

   Fred


> Let me send a v2 first and may be we could rework the object hierarchy
> in the 7.0 time frame. We don't have to merge this ASAP.
> 
> Thanks,
> 
> C.

Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices

[Cédric Le Goater]

On 12/7/21 15:03, Frederic Barrat wrote:
> 
> 
> On 07/12/2021 11:45, Cédric Le Goater wrote:
>> On 12/7/21 11:00, Frederic Barrat wrote:
>>>
>>>
>>> On 02/12/2021 15:42, Cédric Le Goater wrote:
>>>> POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
>>>> each PEC can have several PHBs :
>>>>
>>>>    * PEC0 provides 1 PHB  (PHB0)
>>>>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>>>>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
>>>>
>>>> A num_pecs class attribute represents better the logic units of the
>>>> POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
>>>> This will ease adding support for user created devices.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>
>>> With this patch, chip->num_phbs is only defined and used on P8. We may want to add a comment to make it clear.
>>
>> Yes.
>>
>> With the latest changes, I think we can now move num_phbs under PnvChip8
>> and num_pecs under PnvChip9 since they are only used in these routines :
>>
>> P8:
>>      static void pnv_chip_power8_instance_init(Object *obj)
>>              chip->num_phbs = pcc->num_phbs;
>>          for (i = 0; i < chip->num_phbs; i++) {
>>
>>      static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>>          for (i = 0; i < chip->num_phbs; i++) {
>> P9:
>>      static void pnv_chip_power9_instance_init(Object *obj)
>>              chip->num_pecs = pcc->num_pecs;
>>          for (i = 0; i < chip->num_pecs; i++) {
>>
>>      static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>>          for (i = 0; i < chip->num_pecs; i++) {
>>
>>> As I review this series, something is bugging me though: the difference of handling between P8 and P9.
>>> On P9, we seem to have a more logical hiearachy:
>>> phb <- PCI controller (PEC) <- chip
>>
>> Yes. It's cleaner than P8 in terms of logic. P8 initial support was
>> done hastily for skiboot bringup in 2014.
>>
>>> With P8, we don't have an explicit PEC, but we have a PBCQ object, which is somewhat similar. The hierarchy seems also more convoluted.
>>
>> But we don't have stacks on P8. Do we ?
> 
> 
> Stacks were introduced on P9 because all the lanes handled by a PEC could be grouped differently, each group being called a stack. And each stack is associated to a PHB.
> On P8, there's no such split, so the doc didn't mention stacks. But each PEC handles exactly one PHB. So we could still keep the same abstractions.
> On all chips, a PEC would really be equal to a pbcq interface to the power bus. The pbcq is servicing one (on P8) or more (on P9/P10) PHBs.
> 
> 
> 
>>> I don't see why it's treated differently. It seems both chips could be treated the same, which would make the code easier to follow.
>>
>> I agree. Daniel certainly would also :)
>>
>>> That's outside of the scope of this series though. 
>>
>> Well, this patchset enables libvirt support for the PowerNV machines.
>> Once this is pushed, we need to keep the API, the object model names
>> being part of it.
>>
>> 7.0 is a good time for a change, really. After that, we won't be able
>> to change the QOM hierarchy that much.
>>
>>> So maybe for a future patch? Who knows, I might volunteer...
>>
>> You would introduce a phb3-pec on top of the phb3s ?
> 
> Or rename pnv_phb3_pbcq.c to pnv_phb3_pec.c and starts from there. Conceptually, the TYPE_PNV_PBCQ and TYPE_PNV_PHB4_PEC_STACK objects seem close. But that's easy to say in an email...

It's a start.

Here is the PHB3 QOM tree :

    /pnv-phb3[0] (pnv-phb3)
       /lsi (ics)
       /msi (phb3-msi)
       /msi32[0] (memory-region)
       /msi64[0] (memory-region)
       /pbcq (pnv-pbcq)
         /pbcq-mmio0[0] (memory-region)
         /pbcq-mmio1[0] (memory-region)
         /pbcq-phb[0] (memory-region)
         /xscom-pbcq-nest-0.0[0] (memory-region)
         /xscom-pbcq-pci-0.0[0] (memory-region)
         /xscom-pbcq-spci-0.0[0] (memory-region)
       /pci-io[0] (memory-region)
       /pci-mmio[0] (memory-region)
       /pcie-mmcfg-mmio[0] (memory-region)
       /phb3-m32[0] (memory-region)
       /phb3-regs[0] (memory-region)
       /phb3_iommu[0] (pnv-phb3-iommu-memory-region)
       /root (pnv-phb3-root-port)
         /bus master container[0] (memory-region)
         /bus master[0] (memory-region)
         /pci_bridge_io[0] (memory-region)
         /pci_bridge_io[1] (memory-region)
         /pci_bridge_mem[0] (memory-region)
         /pci_bridge_pci[0] (memory-region)
         /pci_bridge_pref_mem[0] (memory-region)
         /pci_bridge_vga_io_hi[0] (memory-region)
         /pci_bridge_vga_io_lo[0] (memory-region)
         /pci_bridge_vga_mem[0] (memory-region)
         /pcie.0 (PCIE)
       /root-bus (pnv-phb3-root-bus)

We would swap 'pnv-phb3' and 'pnv-pbcq' and rename it to 'pnv-phb3-pec'.
Looks good to me. This should clarify the relationship between objects.

I never like the back pointer to the phb under pbcq:

     (qemu) qom-list /machine/chip[0]/pnv-phb3[0]/pbcq
     type (string)
     parent_bus (link<bus>)
     realized (bool)
     hotplugged (bool)
     hotpluggable (bool)
     pbcq-mmio0[0] (child<memory-region>)
     xscom-pbcq-spci-0.0[0] (child<memory-region>)
     xscom-pbcq-nest-0.0[0] (child<memory-region>)
     pbcq-mmio1[0] (child<memory-region>)
     phb (link<pnv-phb3>)
     pbcq-phb[0] (child<memory-region>)
     xscom-pbcq-pci-0.0[0] (child<memory-region>)
     

That's P9 for the reference :

     /pnv-phb4-pec[0] (pnv-phb4-pec)
       /stack[0] (pnv-phb4-pec-stack)
         /pec-0.0-stack-0-int[0] (memory-region)
         /pec-0.0-stack-0-mmio0[0] (memory-region)
         /pec-0.0-stack-0-mmio1[0] (memory-region)
         /pec-0.0-stack-0-phb[0] (memory-region)
         /phb (pnv-phb4)
           /msi32[0] (memory-region)
           /msi64[0] (memory-region)
           /pcie-mmcfg-mmio[0] (memory-region)
           /phb4-0.0-iommu[0] (pnv-phb4-iommu-memory-region)
           /phb4-0.0-pci-io[0] (memory-region)
           /phb4-0.0-pci-mmio[0] (memory-region)
           /phb4-0.0-regs[0] (memory-region)
           /phb4-mbar0[0] (memory-region)
           /root (pnv-phb4-root-port)
             /bus master container[0] (memory-region)
             /bus master[0] (memory-region)
             /pci_bridge_io[0] (memory-region)
             /pci_bridge_io[1] (memory-region)
             /pci_bridge_mem[0] (memory-region)
             /pci_bridge_pci[0] (memory-region)
             /pci_bridge_pref_mem[0] (memory-region)
             /pci_bridge_vga_io_hi[0] (memory-region)
             /pci_bridge_vga_io_lo[0] (memory-region)
             /pci_bridge_vga_mem[0] (memory-region)
             /pcie.0 (PCIE)
           /root-bus (pnv-phb4-root-bus)
           /source (xive-source)
             /xive.esb-emulated[0] (memory-region)
             /xive.esb[0] (memory-region)
           /xscom-pec-0.0-pci-stack-0-phb[0] (memory-region)
         /xscom-pec-0.0-nest-stack-0[0] (memory-region)
         /xscom-pec-0.0-pci-stack-0[0] (memory-region)
       /xscom-pec-0.0-nest[0] (memory-region)
       /xscom-pec-0.0-pci[0] (memory-region)


Thanks,

C.