[PATCH 0/4] user-created PHB cleanup

[PATCH 0/4] user-created PHB cleanup

[Frederic Barrat]

A short series with some cleanup around user-created PHB. The main
point is to remove errors seen from the firmware (skiboot) when using
user-created PHBs, as we were always showing all the default PHBs in
the device tree, so skiboot tried to probe non-existing devices. The
first 2 patches allow to only export the user-created PHBs in the
device tree when 'nodefaults' is used.
The last 2 are more cosmetic and moving code around where it makes
more sense.


Frederic Barrat (4):
  pnv_phb4_pec: Keep track of instantiated PHBs
  pnv_phb4_pec: Only export existing PHBs to the device tree
  pnv_phb4_pec: Move pnv_phb4_get_pec() to rightful file
  pnv_phb4_pec: Simplify/align code to parent user-created PHBs

 hw/pci-host/pnv_phb.c          | 11 +++++-
 hw/pci-host/pnv_phb4_pec.c     | 61 +++++++++++++++++++++++++----
 hw/ppc/pnv.c                   | 70 ++++------------------------------
 include/hw/pci-host/pnv_phb4.h |  3 ++
 include/hw/ppc/pnv.h           |  2 +-
 5 files changed, 76 insertions(+), 71 deletions(-)

-- 
2.39.2

[PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Frederic Barrat]

Add an array on the PEC object to keep track of the PHBs which are
instantiated. The array can be sparsely populated when using
user-created PHBs. It will be useful for the next patch to only export
instantiated PHBs in the device tree.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
 hw/ppc/pnv.c                   |  1 +
 include/hw/pci-host/pnv_phb4.h |  2 ++
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 43267a428f..97c06bb0a0 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -112,9 +112,9 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
-                                        int stack_no,
-                                        Error **errp)
+static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
+                                           int stack_no,
+                                           Error **errp)
 {
     PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
     int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
@@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
                             &error_fatal);
 
     if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
-        return;
+        return NULL;
     }
+    return phb;
 }
 
 static void pnv_pec_realize(DeviceState *dev, Error **errp)
@@ -148,8 +149,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
 
     /* Create PHBs if running with defaults */
     if (defaults_enabled()) {
+        g_assert(pec->num_phbs <= MAX_PHBS_PER_PEC);
         for (i = 0; i < pec->num_phbs; i++) {
-            pnv_pec_default_phb_realize(pec, i, errp);
+            pec->phbs[i] = pnv_pec_default_phb_realize(pec, i, errp);
         }
     }
 
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 44b1fbbc93..24bf8461d6 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -314,6 +314,7 @@ static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
 
         for (j = 0; j < pec->num_phbs; j++) {
             if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
+                pec->phbs[j] = phb->phb_base;
                 return pec;
             }
         }
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 28d61b96c7..0b72ef1471 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -185,6 +185,8 @@ struct PnvPhb4PecState {
 
     /* PHBs */
     uint32_t num_phbs;
+#define MAX_PHBS_PER_PEC        3
+    PnvPHB *phbs[MAX_PHBS_PER_PEC];
 
     PnvChip *chip;
 };
-- 
2.39.2

[PATCH 2/4] pnv_phb4_pec: Only export existing PHBs to the device tree

[Frederic Barrat]

So far, we were always exporting all possible PHBs to the device
tree. It works well when using the default config but it potentially
adds non-existing devices when using '-nodefaults' and user-created
PHBs, causing the firmware (skiboot) to report errors when probing
those PHBs. This patch only exports PHBs which have been realized to
the device tree.

Fixes: d786be3fe746 ("ppc/pnv: enable user created pnv-phb for powernv9")
Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/pci-host/pnv_phb4_pec.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 97c06bb0a0..6c9b386069 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -199,9 +199,12 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
                       pecc->compat_size)));
 
     for (i = 0; i < pec->num_phbs; i++) {
-        int phb_id = pnv_phb4_pec_get_phb_id(pec, i);
         int stk_offset;
 
+        if (!pec->phbs[i]) {
+            continue;
+        }
+
         name = g_strdup_printf("stack@%x", i);
         stk_offset = fdt_add_subnode(fdt, offset, name);
         _FDT(stk_offset);
@@ -209,7 +212,8 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
         _FDT((fdt_setprop(fdt, stk_offset, "compatible", pecc->stk_compat,
                           pecc->stk_compat_size)));
         _FDT((fdt_setprop_cell(fdt, stk_offset, "reg", i)));
-        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb_id)));
+        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index",
+                               pec->phbs[i]->phb_id)));
     }
 
     return 0;
-- 
2.39.2

[PATCH 3/4] pnv_phb4_pec: Move pnv_phb4_get_pec() to rightful file

[Frederic Barrat]

The function pnv_phb4_get_pec() exposes some internals of the PEC and
PHB logic, yet it was in the higher level hw/ppc/pnv.c file for
historical reasons: P8 implements the PHBs from pnv.c directly, but on
P9/P10, it's done through the CEC model, which has its own file. So
move pnv_phb4_get_pec() to hw/pci-host/pnv_phb4_pec.c, where it fits
naturally.

While at it, replace the PnvPHB4 parameter by the PnvPHB front-end,
since it has all the information needed and simplify it a bit.

No functional changes.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/pci-host/pnv_phb4_pec.c     | 40 +++++++++++++++++++++++++++++++
 hw/ppc/pnv.c                   | 44 +---------------------------------
 include/hw/pci-host/pnv_phb4.h |  1 +
 3 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 6c9b386069..6e2e5ae186 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -112,6 +112,46 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp)
+{
+    PnvPhb4PecState *pecs = NULL;
+    int chip_id = phb->chip_id;
+    int index = phb->phb_id;
+    int i, j;
+
+    if (phb->version == 4) {
+        Pnv9Chip *chip9 = PNV9_CHIP(chip);
+
+        pecs = chip9->pecs;
+    } else if (phb->version == 5) {
+        Pnv10Chip *chip10 = PNV10_CHIP(chip);
+
+        pecs = chip10->pecs;
+    } else {
+        g_assert_not_reached();
+    }
+
+    for (i = 0; i < chip->num_pecs; i++) {
+        /*
+         * For each PEC, check the amount of phbs it supports
+         * and see if the given phb4 index matches an index.
+         */
+        PnvPhb4PecState *pec = &pecs[i];
+
+        for (j = 0; j < pec->num_phbs; j++) {
+            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
+                pec->phbs[j] = phb;
+                return pec;
+            }
+        }
+    }
+    error_setg(errp,
+               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
+               chip_id, index);
+
+    return NULL;
+}
+
 static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
                                            int stack_no,
                                            Error **errp)
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 24bf8461d6..46010b30ad 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -284,48 +284,6 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
     g_free(reg);
 }
 
-static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
-                                         Error **errp)
-{
-    PnvPHB *phb_base = phb->phb_base;
-    PnvPhb4PecState *pecs = NULL;
-    int chip_id = phb->chip_id;
-    int index = phb->phb_id;
-    int i, j;
-
-    if (phb_base->version == 4) {
-        Pnv9Chip *chip9 = PNV9_CHIP(chip);
-
-        pecs = chip9->pecs;
-    } else if (phb_base->version == 5) {
-        Pnv10Chip *chip10 = PNV10_CHIP(chip);
-
-        pecs = chip10->pecs;
-    } else {
-        g_assert_not_reached();
-    }
-
-    for (i = 0; i < chip->num_pecs; i++) {
-        /*
-         * For each PEC, check the amount of phbs it supports
-         * and see if the given phb4 index matches an index.
-         */
-        PnvPhb4PecState *pec = &pecs[i];
-
-        for (j = 0; j < pec->num_phbs; j++) {
-            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
-                pec->phbs[j] = phb->phb_base;
-                return pec;
-            }
-        }
-    }
-    error_setg(errp,
-               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
-               chip_id, index);
-
-    return NULL;
-}
-
 /*
  * Adds a PnvPHB to the chip. Returns the parent obj of the
  * PHB which varies with each version (phb version 3 is parented
@@ -349,7 +307,7 @@ Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
         return OBJECT(chip);
     }
 
-    phb->pec = pnv_phb4_get_pec(chip, PNV_PHB4(phb->backend), errp);
+    phb->pec = pnv_phb4_get_pec(chip, phb, errp);
 
     return OBJECT(phb->pec);
 }
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 0b72ef1471..5c5edb2941 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -157,6 +157,7 @@ struct PnvPHB4 {
 
 void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
 int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
+PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp);
 void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
 
-- 
2.39.2

[PATCH 4/4] pnv_phb4_pec: Simplify/align code to parent user-created PHBs

[Frederic Barrat]

When instantiating a user-created PHB on P9/P10, we don't really have
a reason any more to go through an indirection in pnv_chip_add_phb()
in pnv.c, we can go straight to the right function in
pnv_phb4_pec.c. That way, default PHBs and user-created PHBs are all
handled in the same file.  This patch also renames pnv_phb4_get_pec()
to pnv_pec_add_phb() to better reflect that it "hooks" a PHB to a PEC.

For P8, the PHBs are parented to the chip directly, so it makes sense
to keep calling pnv_chip_add_phb() in pnv.c, to also be consistent
with where default PHBs are handled. The only change here is that,
since that function is now only used for P8, we can refine the return
type.

So overall, the PnvPHB front-end now has a pnv_phb_user_get_parent()
function which handles the parenting of the user-created PHBs by
calling the right function in the right file based on the processor
version. It's also easily extensible if we ever need to support a
different parent object.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/pci-host/pnv_phb.c          | 11 ++++++++++-
 hw/pci-host/pnv_phb4_pec.c     |  3 ++-
 hw/ppc/pnv.c                   | 29 ++++++++---------------------
 include/hw/pci-host/pnv_phb4.h |  2 +-
 include/hw/ppc/pnv.h           |  2 +-
 5 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index c62b08538a..82332d7a05 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -62,6 +62,15 @@ static bool pnv_parent_fixup(Object *parent, BusState *parent_bus,
     return true;
 }
 
+static Object *pnv_phb_user_get_parent(PnvChip *chip, PnvPHB *phb, Error **errp)
+{
+    if (phb->version == 3) {
+        return OBJECT(pnv_chip_add_phb(chip, phb));
+    } else {
+        return OBJECT(pnv_pec_add_phb(chip, phb, errp));
+    }
+}
+
 /*
  * User created devices won't have the initial setup that default
  * devices have. This setup consists of assigning a parent device
@@ -79,7 +88,7 @@ static bool pnv_phb_user_device_init(PnvPHB *phb, Error **errp)
         return false;
     }
 
-    parent = pnv_chip_add_phb(chip, phb, errp);
+    parent = pnv_phb_user_get_parent(chip, phb, errp);
     if (!parent) {
         return false;
     }
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 6e2e5ae186..3b2850f7a3 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -112,7 +112,7 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp)
+PnvPhb4PecState *pnv_pec_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
 {
     PnvPhb4PecState *pecs = NULL;
     int chip_id = phb->chip_id;
@@ -141,6 +141,7 @@ PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp)
         for (j = 0; j < pec->num_phbs; j++) {
             if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
                 pec->phbs[j] = phb;
+                phb->pec = pec;
                 return pec;
             }
         }
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 46010b30ad..11cb48af2f 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -285,31 +285,18 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
 }
 
 /*
- * Adds a PnvPHB to the chip. Returns the parent obj of the
- * PHB which varies with each version (phb version 3 is parented
- * by the chip, version 4 and 5 are parented by the PEC
- * device).
- *
- * TODO: for version 3 we're still parenting the PHB with the
- * chip. We should parent with a (so far not implemented)
- * PHB3 PEC device.
+ * Adds a PnvPHB to the chip on P8.
+ * Implemented here, like for defaults PHBs
  */
-Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
+PnvChip *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb)
 {
-    if (phb->version == 3) {
-        Pnv8Chip *chip8 = PNV8_CHIP(chip);
-
-        phb->chip = chip;
-
-        chip8->phbs[chip8->num_phbs] = phb;
-        chip8->num_phbs++;
-
-        return OBJECT(chip);
-    }
+    Pnv8Chip *chip8 = PNV8_CHIP(chip);
 
-    phb->pec = pnv_phb4_get_pec(chip, phb, errp);
+    phb->chip = chip;
 
-    return OBJECT(phb->pec);
+    chip8->phbs[chip8->num_phbs] = phb;
+    chip8->num_phbs++;
+    return chip;
 }
 
 static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 5c5edb2941..2d026db9a3 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -157,7 +157,7 @@ struct PnvPHB4 {
 
 void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
 int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
-PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp);
+PnvPhb4PecState *pnv_pec_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
 void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
 
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 96fb850419..409f3bf763 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -100,7 +100,7 @@ struct PnvMachineState {
 };
 
 PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
-Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
+PnvChip *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb);
 
 #define PNV_FDT_ADDR          0x01000000
 #define PNV_TIMEBASE_FREQ     512000000ULL
-- 
2.39.2

Re: [PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Philippe Mathieu-Daudé]

Hi Frederic,

On 2/3/23 17:37, Frederic Barrat wrote:
> Add an array on the PEC object to keep track of the PHBs which are
> instantiated. The array can be sparsely populated when using
> user-created PHBs. It will be useful for the next patch to only export
> instantiated PHBs in the device tree.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
>   hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
>   hw/ppc/pnv.c                   |  1 +
>   include/hw/pci-host/pnv_phb4.h |  2 ++
>   3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 43267a428f..97c06bb0a0 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c

> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
> +                                           int stack_no,
> +                                           Error **errp)
>   {
>       PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
> @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>                               &error_fatal);
>   
>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
> -        return;
> +        return NULL;
>       }
> +    return phb;
>   }


> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 28d61b96c7..0b72ef1471 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -185,6 +185,8 @@ struct PnvPhb4PecState {
>   
>       /* PHBs */
>       uint32_t num_phbs;
> +#define MAX_PHBS_PER_PEC        3
> +    PnvPHB *phbs[MAX_PHBS_PER_PEC];
>   
>       PnvChip *chip;
>   };

 From QOM PoV, better would be to 'embed' the PnvPHB structure (not only
a pointer to it), and initialize the PnvPHB instance calling
object_initialize_child() instead of qdev_new().

See for example the recent conversion of OHCISysBusState in commit
01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset").

Regards,

Phil.

Re: [PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Daniel Henrique Barboza]

On 3/2/23 19:21, Philippe Mathieu-Daudé wrote:
> Hi Frederic,
> 
> On 2/3/23 17:37, Frederic Barrat wrote:
>> Add an array on the PEC object to keep track of the PHBs which are
>> instantiated. The array can be sparsely populated when using
>> user-created PHBs. It will be useful for the next patch to only export
>> instantiated PHBs in the device tree.
>>
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>>   hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
>>   hw/ppc/pnv.c                   |  1 +
>>   include/hw/pci-host/pnv_phb4.h |  2 ++
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 43267a428f..97c06bb0a0 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
> 
>> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>> +                                           int stack_no,
>> +                                           Error **errp)
>>   {
>>       PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
>>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
>> @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>>                               &error_fatal);
>>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>> -        return;
>> +        return NULL;
>>       }
>> +    return phb;
>>   }
> 
> 
>> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
>> index 28d61b96c7..0b72ef1471 100644
>> --- a/include/hw/pci-host/pnv_phb4.h
>> +++ b/include/hw/pci-host/pnv_phb4.h
>> @@ -185,6 +185,8 @@ struct PnvPhb4PecState {
>>       /* PHBs */
>>       uint32_t num_phbs;
>> +#define MAX_PHBS_PER_PEC        3
>> +    PnvPHB *phbs[MAX_PHBS_PER_PEC];
>>       PnvChip *chip;
>>   };
> 
>  From QOM PoV, better would be to 'embed' the PnvPHB structure (not only
> a pointer to it), and initialize the PnvPHB instance calling
> object_initialize_child() instead of qdev_new().


We were doing something similar in this PHB controllers one year ago. The
reason we moved to host pointers instead of using embed structures was to
allow user-created phbs. There's no way of telling beforehand if the user
wants one, two or three PHBs in the PEC.


Thanks,

Daniel

> 
> See for example the recent conversion of OHCISysBusState in commit
> 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset").
> 
> Regards,
> 
> Phil.

Re: [PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Daniel Henrique Barboza]

On 3/2/23 13:37, Frederic Barrat wrote:
> Add an array on the PEC object to keep track of the PHBs which are
> instantiated. The array can be sparsely populated when using
> user-created PHBs. It will be useful for the next patch to only export
> instantiated PHBs in the device tree.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---


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

>   hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
>   hw/ppc/pnv.c                   |  1 +
>   include/hw/pci-host/pnv_phb4.h |  2 ++
>   3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 43267a428f..97c06bb0a0 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -112,9 +112,9 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
>       .endianness = DEVICE_BIG_ENDIAN,
>   };
>   
> -static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
> -                                        int stack_no,
> -                                        Error **errp)
> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
> +                                           int stack_no,
> +                                           Error **errp)
>   {
>       PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
> @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>                               &error_fatal);
>   
>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
> -        return;
> +        return NULL;
>       }
> +    return phb;
>   }
>   
>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
> @@ -148,8 +149,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   
>       /* Create PHBs if running with defaults */
>       if (defaults_enabled()) {
> +        g_assert(pec->num_phbs <= MAX_PHBS_PER_PEC);
>           for (i = 0; i < pec->num_phbs; i++) {
> -            pnv_pec_default_phb_realize(pec, i, errp);
> +            pec->phbs[i] = pnv_pec_default_phb_realize(pec, i, errp);
>           }
>       }
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 44b1fbbc93..24bf8461d6 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -314,6 +314,7 @@ static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
>   
>           for (j = 0; j < pec->num_phbs; j++) {
>               if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> +                pec->phbs[j] = phb->phb_base;
>                   return pec;
>               }
>           }
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 28d61b96c7..0b72ef1471 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -185,6 +185,8 @@ struct PnvPhb4PecState {
>   
>       /* PHBs */
>       uint32_t num_phbs;
> +#define MAX_PHBS_PER_PEC        3
> +    PnvPHB *phbs[MAX_PHBS_PER_PEC];
>   
>       PnvChip *chip;
>   };

Re: [PATCH 2/4] pnv_phb4_pec: Only export existing PHBs to the device tree

[Daniel Henrique Barboza]

On 3/2/23 13:37, Frederic Barrat wrote:
> So far, we were always exporting all possible PHBs to the device
> tree. It works well when using the default config but it potentially
> adds non-existing devices when using '-nodefaults' and user-created
> PHBs, causing the firmware (skiboot) to report errors when probing
> those PHBs. This patch only exports PHBs which have been realized to
> the device tree.
> 
> Fixes: d786be3fe746 ("ppc/pnv: enable user created pnv-phb for powernv9")
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---

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

>   hw/pci-host/pnv_phb4_pec.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 97c06bb0a0..6c9b386069 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -199,9 +199,12 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>                         pecc->compat_size)));
>   
>       for (i = 0; i < pec->num_phbs; i++) {
> -        int phb_id = pnv_phb4_pec_get_phb_id(pec, i);
>           int stk_offset;
>   
> +        if (!pec->phbs[i]) {
> +            continue;
> +        }
> +
>           name = g_strdup_printf("stack@%x", i);
>           stk_offset = fdt_add_subnode(fdt, offset, name);
>           _FDT(stk_offset);
> @@ -209,7 +212,8 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>           _FDT((fdt_setprop(fdt, stk_offset, "compatible", pecc->stk_compat,
>                             pecc->stk_compat_size)));
>           _FDT((fdt_setprop_cell(fdt, stk_offset, "reg", i)));
> -        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb_id)));
> +        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index",
> +                               pec->phbs[i]->phb_id)));
>       }
>   
>       return 0;

Re: [PATCH 3/4] pnv_phb4_pec: Move pnv_phb4_get_pec() to rightful file

[Daniel Henrique Barboza]

On 3/2/23 13:37, Frederic Barrat wrote:
> The function pnv_phb4_get_pec() exposes some internals of the PEC and
> PHB logic, yet it was in the higher level hw/ppc/pnv.c file for
> historical reasons: P8 implements the PHBs from pnv.c directly, but on
> P9/P10, it's done through the CEC model, which has its own file. So
> move pnv_phb4_get_pec() to hw/pci-host/pnv_phb4_pec.c, where it fits
> naturally.
> 
> While at it, replace the PnvPHB4 parameter by the PnvPHB front-end,
> since it has all the information needed and simplify it a bit.
> 
> No functional changes.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---

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

>   hw/pci-host/pnv_phb4_pec.c     | 40 +++++++++++++++++++++++++++++++
>   hw/ppc/pnv.c                   | 44 +---------------------------------
>   include/hw/pci-host/pnv_phb4.h |  1 +
>   3 files changed, 42 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 6c9b386069..6e2e5ae186 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -112,6 +112,46 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
>       .endianness = DEVICE_BIG_ENDIAN,
>   };
>   
> +PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp)
> +{
> +    PnvPhb4PecState *pecs = NULL;
> +    int chip_id = phb->chip_id;
> +    int index = phb->phb_id;
> +    int i, j;
> +
> +    if (phb->version == 4) {
> +        Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +
> +        pecs = chip9->pecs;
> +    } else if (phb->version == 5) {
> +        Pnv10Chip *chip10 = PNV10_CHIP(chip);
> +
> +        pecs = chip10->pecs;
> +    } else {
> +        g_assert_not_reached();
> +    }
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
> +        /*
> +         * For each PEC, check the amount of phbs it supports
> +         * and see if the given phb4 index matches an index.
> +         */
> +        PnvPhb4PecState *pec = &pecs[i];
> +
> +        for (j = 0; j < pec->num_phbs; j++) {
> +            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> +                pec->phbs[j] = phb;
> +                return pec;
> +            }
> +        }
> +    }
> +    error_setg(errp,
> +               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
> +               chip_id, index);
> +
> +    return NULL;
> +}
> +
>   static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>                                              int stack_no,
>                                              Error **errp)
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 24bf8461d6..46010b30ad 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -284,48 +284,6 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>       g_free(reg);
>   }
>   
> -static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
> -                                         Error **errp)
> -{
> -    PnvPHB *phb_base = phb->phb_base;
> -    PnvPhb4PecState *pecs = NULL;
> -    int chip_id = phb->chip_id;
> -    int index = phb->phb_id;
> -    int i, j;
> -
> -    if (phb_base->version == 4) {
> -        Pnv9Chip *chip9 = PNV9_CHIP(chip);
> -
> -        pecs = chip9->pecs;
> -    } else if (phb_base->version == 5) {
> -        Pnv10Chip *chip10 = PNV10_CHIP(chip);
> -
> -        pecs = chip10->pecs;
> -    } else {
> -        g_assert_not_reached();
> -    }
> -
> -    for (i = 0; i < chip->num_pecs; i++) {
> -        /*
> -         * For each PEC, check the amount of phbs it supports
> -         * and see if the given phb4 index matches an index.
> -         */
> -        PnvPhb4PecState *pec = &pecs[i];
> -
> -        for (j = 0; j < pec->num_phbs; j++) {
> -            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> -                pec->phbs[j] = phb->phb_base;
> -                return pec;
> -            }
> -        }
> -    }
> -    error_setg(errp,
> -               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
> -               chip_id, index);
> -
> -    return NULL;
> -}
> -
>   /*
>    * Adds a PnvPHB to the chip. Returns the parent obj of the
>    * PHB which varies with each version (phb version 3 is parented
> @@ -349,7 +307,7 @@ Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
>           return OBJECT(chip);
>       }
>   
> -    phb->pec = pnv_phb4_get_pec(chip, PNV_PHB4(phb->backend), errp);
> +    phb->pec = pnv_phb4_get_pec(chip, phb, errp);
>   
>       return OBJECT(phb->pec);
>   }
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 0b72ef1471..5c5edb2941 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -157,6 +157,7 @@ struct PnvPHB4 {
>   
>   void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
>   int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
> +PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp);
>   void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb);
>   extern const MemoryRegionOps pnv_phb4_xscom_ops;
>   

Re: [PATCH 4/4] pnv_phb4_pec: Simplify/align code to parent user-created PHBs

[Daniel Henrique Barboza]

On 3/2/23 13:37, Frederic Barrat wrote:
> When instantiating a user-created PHB on P9/P10, we don't really have
> a reason any more to go through an indirection in pnv_chip_add_phb()
> in pnv.c, we can go straight to the right function in
> pnv_phb4_pec.c. That way, default PHBs and user-created PHBs are all
> handled in the same file.  This patch also renames pnv_phb4_get_pec()
> to pnv_pec_add_phb() to better reflect that it "hooks" a PHB to a PEC.
> 
> For P8, the PHBs are parented to the chip directly, so it makes sense
> to keep calling pnv_chip_add_phb() in pnv.c, to also be consistent
> with where default PHBs are handled. The only change here is that,
> since that function is now only used for P8, we can refine the return
> type.
> 
> So overall, the PnvPHB front-end now has a pnv_phb_user_get_parent()
> function which handles the parenting of the user-created PHBs by
> calling the right function in the right file based on the processor
> version. It's also easily extensible if we ever need to support a
> different parent object.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---

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

>   hw/pci-host/pnv_phb.c          | 11 ++++++++++-
>   hw/pci-host/pnv_phb4_pec.c     |  3 ++-
>   hw/ppc/pnv.c                   | 29 ++++++++---------------------
>   include/hw/pci-host/pnv_phb4.h |  2 +-
>   include/hw/ppc/pnv.h           |  2 +-
>   5 files changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index c62b08538a..82332d7a05 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -62,6 +62,15 @@ static bool pnv_parent_fixup(Object *parent, BusState *parent_bus,
>       return true;
>   }
>   
> +static Object *pnv_phb_user_get_parent(PnvChip *chip, PnvPHB *phb, Error **errp)
> +{
> +    if (phb->version == 3) {
> +        return OBJECT(pnv_chip_add_phb(chip, phb));
> +    } else {
> +        return OBJECT(pnv_pec_add_phb(chip, phb, errp));
> +    }
> +}
> +
>   /*
>    * User created devices won't have the initial setup that default
>    * devices have. This setup consists of assigning a parent device
> @@ -79,7 +88,7 @@ static bool pnv_phb_user_device_init(PnvPHB *phb, Error **errp)
>           return false;
>       }
>   
> -    parent = pnv_chip_add_phb(chip, phb, errp);
> +    parent = pnv_phb_user_get_parent(chip, phb, errp);
>       if (!parent) {
>           return false;
>       }
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 6e2e5ae186..3b2850f7a3 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -112,7 +112,7 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
>       .endianness = DEVICE_BIG_ENDIAN,
>   };
>   
> -PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp)
> +PnvPhb4PecState *pnv_pec_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
>   {
>       PnvPhb4PecState *pecs = NULL;
>       int chip_id = phb->chip_id;
> @@ -141,6 +141,7 @@ PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp)
>           for (j = 0; j < pec->num_phbs; j++) {
>               if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
>                   pec->phbs[j] = phb;
> +                phb->pec = pec;
>                   return pec;
>               }
>           }
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 46010b30ad..11cb48af2f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -285,31 +285,18 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>   }
>   
>   /*
> - * Adds a PnvPHB to the chip. Returns the parent obj of the
> - * PHB which varies with each version (phb version 3 is parented
> - * by the chip, version 4 and 5 are parented by the PEC
> - * device).
> - *
> - * TODO: for version 3 we're still parenting the PHB with the
> - * chip. We should parent with a (so far not implemented)
> - * PHB3 PEC device.
> + * Adds a PnvPHB to the chip on P8.
> + * Implemented here, like for defaults PHBs
>    */
> -Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
> +PnvChip *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb)
>   {
> -    if (phb->version == 3) {
> -        Pnv8Chip *chip8 = PNV8_CHIP(chip);
> -
> -        phb->chip = chip;
> -
> -        chip8->phbs[chip8->num_phbs] = phb;
> -        chip8->num_phbs++;
> -
> -        return OBJECT(chip);
> -    }
> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
>   
> -    phb->pec = pnv_phb4_get_pec(chip, phb, errp);
> +    phb->chip = chip;
>   
> -    return OBJECT(phb->pec);
> +    chip8->phbs[chip8->num_phbs] = phb;
> +    chip8->num_phbs++;
> +    return chip;
>   }
>   
>   static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 5c5edb2941..2d026db9a3 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -157,7 +157,7 @@ struct PnvPHB4 {
>   
>   void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
>   int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
> -PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB *phb, Error **errp);
> +PnvPhb4PecState *pnv_pec_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
>   void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb);
>   extern const MemoryRegionOps pnv_phb4_xscom_ops;
>   
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 96fb850419..409f3bf763 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -100,7 +100,7 @@ struct PnvMachineState {
>   };
>   
>   PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
> -Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
> +PnvChip *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb);
>   
>   #define PNV_FDT_ADDR          0x01000000
>   #define PNV_TIMEBASE_FREQ     512000000ULL

Re: [PATCH 0/4] user-created PHB cleanup

[Daniel Henrique Barboza]

On 3/2/23 13:37, Frederic Barrat wrote:
> A short series with some cleanup around user-created PHB. The main
> point is to remove errors seen from the firmware (skiboot) when using
> user-created PHBs, as we were always showing all the default PHBs in
> the device tree, so skiboot tried to probe non-existing devices. The
> first 2 patches allow to only export the user-created PHBs in the
> device tree when 'nodefaults' is used.
> The last 2 are more cosmetic and moving code around where it makes
> more sense.
> 

I'm aware of Phil's comments in patch 1 and the solution can probably be improved
on (as with any other solution and code).

Patch 1 is a requirement to the actual fix in patch 2 though, and Phil's comments
applies to more stuff in the current pnv-phb code. Since we can handle design changes
later on while still fixing the bug, I queued all these for 8.0.


Thanks,

Daniel

> 
> Frederic Barrat (4):
>    pnv_phb4_pec: Keep track of instantiated PHBs
>    pnv_phb4_pec: Only export existing PHBs to the device tree
>    pnv_phb4_pec: Move pnv_phb4_get_pec() to rightful file
>    pnv_phb4_pec: Simplify/align code to parent user-created PHBs
> 
>   hw/pci-host/pnv_phb.c          | 11 +++++-
>   hw/pci-host/pnv_phb4_pec.c     | 61 +++++++++++++++++++++++++----
>   hw/ppc/pnv.c                   | 70 ++++------------------------------
>   include/hw/pci-host/pnv_phb4.h |  3 ++
>   include/hw/ppc/pnv.h           |  2 +-
>   5 files changed, 76 insertions(+), 71 deletions(-)
> 

Re: [PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Frederic Barrat]

On 02/03/2023 23:21, Philippe Mathieu-Daudé wrote:
> Hi Frederic,
> 
> On 2/3/23 17:37, Frederic Barrat wrote:
>> Add an array on the PEC object to keep track of the PHBs which are
>> instantiated. The array can be sparsely populated when using
>> user-created PHBs. It will be useful for the next patch to only export
>> instantiated PHBs in the device tree.
>>
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>>   hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
>>   hw/ppc/pnv.c                   |  1 +
>>   include/hw/pci-host/pnv_phb4.h |  2 ++
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 43267a428f..97c06bb0a0 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
> 
>> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>> +                                           int stack_no,
>> +                                           Error **errp)
>>   {
>>       PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
>>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
>> @@ -128,8 +128,9 @@ static void 
>> pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>>                               &error_fatal);
>>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>> -        return;
>> +        return NULL;
>>       }
>> +    return phb;
>>   }
> 
> 
>> diff --git a/include/hw/pci-host/pnv_phb4.h 
>> b/include/hw/pci-host/pnv_phb4.h
>> index 28d61b96c7..0b72ef1471 100644
>> --- a/include/hw/pci-host/pnv_phb4.h
>> +++ b/include/hw/pci-host/pnv_phb4.h
>> @@ -185,6 +185,8 @@ struct PnvPhb4PecState {
>>       /* PHBs */
>>       uint32_t num_phbs;
>> +#define MAX_PHBS_PER_PEC        3
>> +    PnvPHB *phbs[MAX_PHBS_PER_PEC];
>>       PnvChip *chip;
>>   };
> 
>  From QOM PoV, better would be to 'embed' the PnvPHB structure (not only
> a pointer to it), and initialize the PnvPHB instance calling
> object_initialize_child() instead of qdev_new().

Hi Phil,
Daniel beat me to it, but we used to do precisely that but went the
opposite direction (see 0d512c7120a2), because we can now specify from 
the command line what PHB to implement so we don't want to allocate all 
of them.

   Fred


> 
> See for example the recent conversion of OHCISysBusState in commit
> 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset").
> 
> Regards,
> 
> Phil.