[PATCH v5 0/5] user creatable pnv-phb4 devices

[PATCH v5 0/5] user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

Hi,

This version implements Cedric's review suggestions from v4. No
drastic design changes were made.

Changes from v4:
- patches 1,3,5: unchanged
- patch 2:
  * renamed function to pnv_phb4_xscom_realize()
  * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
- patch 4:
  * changed pnv_phb4_get_stack signature to use chip and phb
  * added a new helper called pnv_pec_stk_default_phb_realize() to
realize the default phb when running with defaults
- v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html

Daniel Henrique Barboza (5):
  ppc/pnv: set phb4 properties in stk_realize()
  ppc/pnv: move PHB4 XSCOM init to phb4_realize()
  ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
  ppc/pnv: Introduce user creatable pnv-phb4 devices
  ppc/pnv: turn pnv_phb4_update_regions() into static

 hw/pci-host/pnv_phb4.c         | 430 ++++++++++++++++++++++++++++++---
 hw/pci-host/pnv_phb4_pec.c     | 329 ++-----------------------
 hw/ppc/pnv.c                   |   2 +
 include/hw/pci-host/pnv_phb4.h |   8 +-
 4 files changed, 431 insertions(+), 338 deletions(-)

-- 
2.33.1

[PATCH v5 1/5] ppc/pnv: set phb4 properties in stk_realize()

[Daniel Henrique Barboza]

Moving all phb4 properties setup to stk_realize() keeps this logic in
a single place instead of having it scattered between stk_realize() and
pec_realize().

'phb->index' can be retrieved using stack->stack_no and
pnv_phb4_pec_get_phb_id(), deprecating the use of 'phb-id' alias that
was being used for this purpose in pec_realize().

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4_pec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index d64310e7db..f8038dff17 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -392,10 +392,8 @@ 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_phb4_pec_get_phb_id(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;
@@ -534,7 +532,6 @@ 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)
@@ -543,6 +540,7 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     PnvPhb4PecState *pec = stack->pec;
     PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
     PnvChip *chip = pec->chip;
+    int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
     uint32_t pec_nest_base;
     uint32_t pec_pci_base;
     char name[64];
@@ -570,6 +568,8 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
 
     object_property_set_int(OBJECT(&stack->phb), "chip-id", pec->chip_id,
                             &error_fatal);
+    object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
+                            &error_fatal);
     object_property_set_int(OBJECT(&stack->phb), "version", pecc->version,
                             &error_fatal);
     object_property_set_link(OBJECT(&stack->phb), "stack", OBJECT(stack),
-- 
2.33.1

[PATCH v5 2/5] ppc/pnv: move PHB4 XSCOM init to phb4_realize()

[Daniel Henrique Barboza]

The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on
'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init()
and add_subregion() to phb4_realize() time is a natural thing to do
since it's strictly PHB related.

The remaining XSCOM initialization is also related to 'stack->phb' but
in a different manner. For instance, 'stack->nest_regs_mr'
MemoryRegionOps, 'pnv_pec_stk_nest_xscom_ops', uses
pnv_pec_stk_nest_xscom_write() as a write callback. When trying to write
the PEC_NEST_STK_BAR_EN reg, pnv_pec_stk_update_map() is called. Inside
this function, pnv_phb4_update_regions() is called twice. This function
uses 'stack->phb' to manipulate memory regions of the phb.

This is not a problem now but, when enabling user creatable phb4s, a
stack that doesn't have an associated phb (i.e. stack->phb = NULL) it
will cause a SIGINT during boot in pnv_phb4_update_regions().

All this can be avoided if all XSCOM realize is moved to phb4_realize(),
when we have certainty about the existence of 'stack->phb'. A lot of
code was moved from pnv_phb4_pec.c to pnv_phb4.c due to static constant
and variables being used but the cleaner logic is worth the trouble.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c     | 304 +++++++++++++++++++++++++++++++++++++
 hw/pci-host/pnv_phb4_pec.c | 292 -----------------------------------
 2 files changed, 304 insertions(+), 292 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index b7b0091f93..94fd8c858d 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -29,6 +29,10 @@
     qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",            \
                   (phb)->chip_id, (phb)->phb_id, ## __VA_ARGS__)
 
+#define phb_pec_error(pec, fmt, ...)                                    \
+    qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n",        \
+                  (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
+
 /*
  * QEMU version of the GETFIELD/SETFIELD macros
  *
@@ -854,6 +858,258 @@ const MemoryRegionOps pnv_phb4_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
+                                            unsigned size)
+{
+    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    uint32_t reg = addr >> 3;
+
+    /* TODO: add list of allowed registers and error out if not */
+    return stack->nest_regs[reg];
+}
+
+static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
+{
+    PnvPhb4PecState *pec = stack->pec;
+    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];
+
+    /*
+     * NOTE: This will really not work well if those are remapped
+     * after the PHB has created its sub regions. We could do better
+     * if we had a way to resize regions but we don't really care
+     * that much in practice as the stuff below really only happens
+     * once early during boot
+     */
+
+    /* Handle unmaps */
+    if (memory_region_is_mapped(&stack->mmbar0) &&
+        !(bar_en & PEC_NEST_STK_BAR_EN_MMIO0)) {
+        memory_region_del_subregion(sysmem, &stack->mmbar0);
+    }
+    if (memory_region_is_mapped(&stack->mmbar1) &&
+        !(bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
+        memory_region_del_subregion(sysmem, &stack->mmbar1);
+    }
+    if (memory_region_is_mapped(&stack->phbbar) &&
+        !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
+        memory_region_del_subregion(sysmem, &stack->phbbar);
+    }
+    if (memory_region_is_mapped(&stack->intbar) &&
+        !(bar_en & PEC_NEST_STK_BAR_EN_INT)) {
+        memory_region_del_subregion(sysmem, &stack->intbar);
+    }
+
+    /* Update PHB */
+    pnv_phb4_update_regions(stack);
+
+    /* Handle maps */
+    if (!memory_region_is_mapped(&stack->mmbar0) &&
+        (bar_en & PEC_NEST_STK_BAR_EN_MMIO0)) {
+        bar = stack->nest_regs[PEC_NEST_STK_MMIO_BAR0] >> 8;
+        mask = stack->nest_regs[PEC_NEST_STK_MMIO_BAR0_MASK];
+        size = ((~mask) >> 8) + 1;
+        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-mmio0",
+                 pec->chip_id, pec->index, stack->stack_no);
+        memory_region_init(&stack->mmbar0, OBJECT(stack), name, size);
+        memory_region_add_subregion(sysmem, bar, &stack->mmbar0);
+        stack->mmio0_base = bar;
+        stack->mmio0_size = size;
+    }
+    if (!memory_region_is_mapped(&stack->mmbar1) &&
+        (bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
+        bar = stack->nest_regs[PEC_NEST_STK_MMIO_BAR1] >> 8;
+        mask = stack->nest_regs[PEC_NEST_STK_MMIO_BAR1_MASK];
+        size = ((~mask) >> 8) + 1;
+        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-mmio1",
+                 pec->chip_id, pec->index, stack->stack_no);
+        memory_region_init(&stack->mmbar1, OBJECT(stack), name, size);
+        memory_region_add_subregion(sysmem, bar, &stack->mmbar1);
+        stack->mmio1_base = bar;
+        stack->mmio1_size = size;
+    }
+    if (!memory_region_is_mapped(&stack->phbbar) &&
+        (bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
+        bar = stack->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
+        size = PNV_PHB4_NUM_REGS << 3;
+        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-phb",
+                 pec->chip_id, pec->index, stack->stack_no);
+        memory_region_init(&stack->phbbar, OBJECT(stack), name, size);
+        memory_region_add_subregion(sysmem, bar, &stack->phbbar);
+    }
+    if (!memory_region_is_mapped(&stack->intbar) &&
+        (bar_en & PEC_NEST_STK_BAR_EN_INT)) {
+        bar = stack->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
+        size = PNV_PHB4_MAX_INTs << 16;
+        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-int",
+                 stack->pec->chip_id, stack->pec->index, stack->stack_no);
+        memory_region_init(&stack->intbar, OBJECT(stack), name, size);
+        memory_region_add_subregion(sysmem, bar, &stack->intbar);
+    }
+
+    /* Update PHB */
+    pnv_phb4_update_regions(stack);
+}
+
+static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
+                                         uint64_t val, unsigned size)
+{
+    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    PnvPhb4PecState *pec = stack->pec;
+    uint32_t reg = addr >> 3;
+
+    switch (reg) {
+    case PEC_NEST_STK_PCI_NEST_FIR:
+        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR] = val;
+        break;
+    case PEC_NEST_STK_PCI_NEST_FIR_CLR:
+        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR] &= val;
+        break;
+    case PEC_NEST_STK_PCI_NEST_FIR_SET:
+        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR] |= val;
+        break;
+    case PEC_NEST_STK_PCI_NEST_FIR_MSK:
+        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR_MSK] = val;
+        break;
+    case PEC_NEST_STK_PCI_NEST_FIR_MSKC:
+        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR_MSK] &= val;
+        break;
+    case PEC_NEST_STK_PCI_NEST_FIR_MSKS:
+        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR_MSK] |= val;
+        break;
+    case PEC_NEST_STK_PCI_NEST_FIR_ACT0:
+    case PEC_NEST_STK_PCI_NEST_FIR_ACT1:
+        stack->nest_regs[reg] = val;
+        break;
+    case PEC_NEST_STK_PCI_NEST_FIR_WOF:
+        stack->nest_regs[reg] = 0;
+        break;
+    case PEC_NEST_STK_ERR_REPORT_0:
+    case PEC_NEST_STK_ERR_REPORT_1:
+    case PEC_NEST_STK_PBCQ_GNRL_STATUS:
+        /* Flag error ? */
+        break;
+    case PEC_NEST_STK_PBCQ_MODE:
+        stack->nest_regs[reg] = val & 0xff00000000000000ull;
+        break;
+    case PEC_NEST_STK_MMIO_BAR0:
+    case PEC_NEST_STK_MMIO_BAR0_MASK:
+    case PEC_NEST_STK_MMIO_BAR1:
+    case PEC_NEST_STK_MMIO_BAR1_MASK:
+        if (stack->nest_regs[PEC_NEST_STK_BAR_EN] &
+            (PEC_NEST_STK_BAR_EN_MMIO0 |
+             PEC_NEST_STK_BAR_EN_MMIO1)) {
+            phb_pec_error(pec, "Changing enabled BAR unsupported\n");
+        }
+        stack->nest_regs[reg] = val & 0xffffffffff000000ull;
+        break;
+    case PEC_NEST_STK_PHB_REGS_BAR:
+        if (stack->nest_regs[PEC_NEST_STK_BAR_EN] & PEC_NEST_STK_BAR_EN_PHB) {
+            phb_pec_error(pec, "Changing enabled BAR unsupported\n");
+        }
+        stack->nest_regs[reg] = val & 0xffffffffffc00000ull;
+        break;
+    case PEC_NEST_STK_INT_BAR:
+        if (stack->nest_regs[PEC_NEST_STK_BAR_EN] & PEC_NEST_STK_BAR_EN_INT) {
+            phb_pec_error(pec, "Changing enabled BAR unsupported\n");
+        }
+        stack->nest_regs[reg] = val & 0xfffffff000000000ull;
+        break;
+    case PEC_NEST_STK_BAR_EN:
+        stack->nest_regs[reg] = val & 0xf000000000000000ull;
+        pnv_pec_stk_update_map(stack);
+        break;
+    case PEC_NEST_STK_DATA_FRZ_TYPE:
+    case PEC_NEST_STK_PBCQ_TUN_BAR:
+        /* Not used for now */
+        stack->nest_regs[reg] = val;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "phb4_pec: nest_xscom_write 0x%"HWADDR_PRIx
+                      "=%"PRIx64"\n", addr, val);
+    }
+}
+
+static const MemoryRegionOps pnv_pec_stk_nest_xscom_ops = {
+    .read = pnv_pec_stk_nest_xscom_read,
+    .write = pnv_pec_stk_nest_xscom_write,
+    .valid.min_access_size = 8,
+    .valid.max_access_size = 8,
+    .impl.min_access_size = 8,
+    .impl.max_access_size = 8,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static uint64_t pnv_pec_stk_pci_xscom_read(void *opaque, hwaddr addr,
+                                           unsigned size)
+{
+    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    uint32_t reg = addr >> 3;
+
+    /* TODO: add list of allowed registers and error out if not */
+    return stack->pci_regs[reg];
+}
+
+static void pnv_pec_stk_pci_xscom_write(void *opaque, hwaddr addr,
+                                        uint64_t val, unsigned size)
+{
+    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    uint32_t reg = addr >> 3;
+
+    switch (reg) {
+    case PEC_PCI_STK_PCI_FIR:
+        stack->nest_regs[reg] = val;
+        break;
+    case PEC_PCI_STK_PCI_FIR_CLR:
+        stack->nest_regs[PEC_PCI_STK_PCI_FIR] &= val;
+        break;
+    case PEC_PCI_STK_PCI_FIR_SET:
+        stack->nest_regs[PEC_PCI_STK_PCI_FIR] |= val;
+        break;
+    case PEC_PCI_STK_PCI_FIR_MSK:
+        stack->nest_regs[reg] = val;
+        break;
+    case PEC_PCI_STK_PCI_FIR_MSKC:
+        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
+        break;
+    case PEC_PCI_STK_PCI_FIR_MSKS:
+        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
+        break;
+    case PEC_PCI_STK_PCI_FIR_ACT0:
+    case PEC_PCI_STK_PCI_FIR_ACT1:
+        stack->nest_regs[reg] = val;
+        break;
+    case PEC_PCI_STK_PCI_FIR_WOF:
+        stack->nest_regs[reg] = 0;
+        break;
+    case PEC_PCI_STK_ETU_RESET:
+        stack->nest_regs[reg] = val & 0x8000000000000000ull;
+        /* TODO: Implement reset */
+        break;
+    case PEC_PCI_STK_PBAIB_ERR_REPORT:
+        break;
+    case PEC_PCI_STK_PBAIB_TX_CMD_CRED:
+    case PEC_PCI_STK_PBAIB_TX_DAT_CRED:
+        stack->nest_regs[reg] = val;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "phb4_pec_stk: pci_xscom_write 0x%"HWADDR_PRIx
+                      "=%"PRIx64"\n", addr, val);
+    }
+}
+
+static const MemoryRegionOps pnv_pec_stk_pci_xscom_ops = {
+    .read = pnv_pec_stk_pci_xscom_read,
+    .write = pnv_pec_stk_pci_xscom_write,
+    .valid.min_access_size = 8,
+    .valid.max_access_size = 8,
+    .impl.min_access_size = 8,
+    .impl.max_access_size = 8,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
 static int pnv_phb4_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     /* Check that out properly ... */
@@ -1175,6 +1431,52 @@ int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index)
     return offset + stack_index;
 }
 
+static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
+{
+    PnvPhb4PecStack *stack = phb->stack;
+    PnvPhb4PecState *pec = stack->pec;
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    uint32_t pec_nest_base;
+    uint32_t pec_pci_base;
+    char name[64];
+
+    assert(pec);
+
+    /* Initialize the XSCOM regions for the stack registers */
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-stack-%d",
+             pec->chip_id, pec->index, stack->stack_no);
+    pnv_xscom_region_init(&stack->nest_regs_mr, OBJECT(stack),
+                          &pnv_pec_stk_nest_xscom_ops, stack, name,
+                          PHB4_PEC_NEST_STK_REGS_COUNT);
+
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d",
+             pec->chip_id, pec->index, stack->stack_no);
+    pnv_xscom_region_init(&stack->pci_regs_mr, OBJECT(stack),
+                          &pnv_pec_stk_pci_xscom_ops, stack, name,
+                          PHB4_PEC_PCI_STK_REGS_COUNT);
+
+    /* PHB pass-through */
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
+             pec->chip_id, pec->index, stack->stack_no);
+    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(phb),
+                          &pnv_phb4_xscom_ops, phb, name, 0x40);
+
+    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(pec->chip,
+                            pec_nest_base + 0x40 * (stack->stack_no + 1),
+                            &stack->nest_regs_mr);
+    pnv_xscom_add_subregion(pec->chip,
+                            pec_pci_base + 0x40 * (stack->stack_no + 1),
+                            &stack->pci_regs_mr);
+    pnv_xscom_add_subregion(pec->chip,
+                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
+                            0x40 * stack->stack_no,
+                            &stack->phb_regs_mr);
+}
+
 static void pnv_phb4_instance_init(Object *obj)
 {
     PnvPHB4 *phb = PNV_PHB4(obj);
@@ -1247,6 +1549,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     pnv_phb4_update_xsrc(phb);
 
     phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
+
+    pnv_phb4_xscom_realize(phb);
 }
 
 static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index f8038dff17..bf0fdf33fd 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -111,258 +111,6 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
-                                            unsigned size)
-{
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
-    uint32_t reg = addr >> 3;
-
-    /* TODO: add list of allowed registers and error out if not */
-    return stack->nest_regs[reg];
-}
-
-static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
-{
-    PnvPhb4PecState *pec = stack->pec;
-    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];
-
-    /*
-     * NOTE: This will really not work well if those are remapped
-     * after the PHB has created its sub regions. We could do better
-     * if we had a way to resize regions but we don't really care
-     * that much in practice as the stuff below really only happens
-     * once early during boot
-     */
-
-    /* Handle unmaps */
-    if (memory_region_is_mapped(&stack->mmbar0) &&
-        !(bar_en & PEC_NEST_STK_BAR_EN_MMIO0)) {
-        memory_region_del_subregion(sysmem, &stack->mmbar0);
-    }
-    if (memory_region_is_mapped(&stack->mmbar1) &&
-        !(bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
-        memory_region_del_subregion(sysmem, &stack->mmbar1);
-    }
-    if (memory_region_is_mapped(&stack->phbbar) &&
-        !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
-        memory_region_del_subregion(sysmem, &stack->phbbar);
-    }
-    if (memory_region_is_mapped(&stack->intbar) &&
-        !(bar_en & PEC_NEST_STK_BAR_EN_INT)) {
-        memory_region_del_subregion(sysmem, &stack->intbar);
-    }
-
-    /* Update PHB */
-    pnv_phb4_update_regions(stack);
-
-    /* Handle maps */
-    if (!memory_region_is_mapped(&stack->mmbar0) &&
-        (bar_en & PEC_NEST_STK_BAR_EN_MMIO0)) {
-        bar = stack->nest_regs[PEC_NEST_STK_MMIO_BAR0] >> 8;
-        mask = stack->nest_regs[PEC_NEST_STK_MMIO_BAR0_MASK];
-        size = ((~mask) >> 8) + 1;
-        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-mmio0",
-                 pec->chip_id, pec->index, stack->stack_no);
-        memory_region_init(&stack->mmbar0, OBJECT(stack), name, size);
-        memory_region_add_subregion(sysmem, bar, &stack->mmbar0);
-        stack->mmio0_base = bar;
-        stack->mmio0_size = size;
-    }
-    if (!memory_region_is_mapped(&stack->mmbar1) &&
-        (bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
-        bar = stack->nest_regs[PEC_NEST_STK_MMIO_BAR1] >> 8;
-        mask = stack->nest_regs[PEC_NEST_STK_MMIO_BAR1_MASK];
-        size = ((~mask) >> 8) + 1;
-        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-mmio1",
-                 pec->chip_id, pec->index, stack->stack_no);
-        memory_region_init(&stack->mmbar1, OBJECT(stack), name, size);
-        memory_region_add_subregion(sysmem, bar, &stack->mmbar1);
-        stack->mmio1_base = bar;
-        stack->mmio1_size = size;
-    }
-    if (!memory_region_is_mapped(&stack->phbbar) &&
-        (bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
-        bar = stack->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
-        size = PNV_PHB4_NUM_REGS << 3;
-        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-phb",
-                 pec->chip_id, pec->index, stack->stack_no);
-        memory_region_init(&stack->phbbar, OBJECT(stack), name, size);
-        memory_region_add_subregion(sysmem, bar, &stack->phbbar);
-    }
-    if (!memory_region_is_mapped(&stack->intbar) &&
-        (bar_en & PEC_NEST_STK_BAR_EN_INT)) {
-        bar = stack->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
-        size = PNV_PHB4_MAX_INTs << 16;
-        snprintf(name, sizeof(name), "pec-%d.%d-stack-%d-int",
-                 stack->pec->chip_id, stack->pec->index, stack->stack_no);
-        memory_region_init(&stack->intbar, OBJECT(stack), name, size);
-        memory_region_add_subregion(sysmem, bar, &stack->intbar);
-    }
-
-    /* Update PHB */
-    pnv_phb4_update_regions(stack);
-}
-
-static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
-                                         uint64_t val, unsigned size)
-{
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
-    PnvPhb4PecState *pec = stack->pec;
-    uint32_t reg = addr >> 3;
-
-    switch (reg) {
-    case PEC_NEST_STK_PCI_NEST_FIR:
-        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR] = val;
-        break;
-    case PEC_NEST_STK_PCI_NEST_FIR_CLR:
-        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR] &= val;
-        break;
-    case PEC_NEST_STK_PCI_NEST_FIR_SET:
-        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR] |= val;
-        break;
-    case PEC_NEST_STK_PCI_NEST_FIR_MSK:
-        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR_MSK] = val;
-        break;
-    case PEC_NEST_STK_PCI_NEST_FIR_MSKC:
-        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR_MSK] &= val;
-        break;
-    case PEC_NEST_STK_PCI_NEST_FIR_MSKS:
-        stack->nest_regs[PEC_NEST_STK_PCI_NEST_FIR_MSK] |= val;
-        break;
-    case PEC_NEST_STK_PCI_NEST_FIR_ACT0:
-    case PEC_NEST_STK_PCI_NEST_FIR_ACT1:
-        stack->nest_regs[reg] = val;
-        break;
-    case PEC_NEST_STK_PCI_NEST_FIR_WOF:
-        stack->nest_regs[reg] = 0;
-        break;
-    case PEC_NEST_STK_ERR_REPORT_0:
-    case PEC_NEST_STK_ERR_REPORT_1:
-    case PEC_NEST_STK_PBCQ_GNRL_STATUS:
-        /* Flag error ? */
-        break;
-    case PEC_NEST_STK_PBCQ_MODE:
-        stack->nest_regs[reg] = val & 0xff00000000000000ull;
-        break;
-    case PEC_NEST_STK_MMIO_BAR0:
-    case PEC_NEST_STK_MMIO_BAR0_MASK:
-    case PEC_NEST_STK_MMIO_BAR1:
-    case PEC_NEST_STK_MMIO_BAR1_MASK:
-        if (stack->nest_regs[PEC_NEST_STK_BAR_EN] &
-            (PEC_NEST_STK_BAR_EN_MMIO0 |
-             PEC_NEST_STK_BAR_EN_MMIO1)) {
-            phb_pec_error(pec, "Changing enabled BAR unsupported\n");
-        }
-        stack->nest_regs[reg] = val & 0xffffffffff000000ull;
-        break;
-    case PEC_NEST_STK_PHB_REGS_BAR:
-        if (stack->nest_regs[PEC_NEST_STK_BAR_EN] & PEC_NEST_STK_BAR_EN_PHB) {
-            phb_pec_error(pec, "Changing enabled BAR unsupported\n");
-        }
-        stack->nest_regs[reg] = val & 0xffffffffffc00000ull;
-        break;
-    case PEC_NEST_STK_INT_BAR:
-        if (stack->nest_regs[PEC_NEST_STK_BAR_EN] & PEC_NEST_STK_BAR_EN_INT) {
-            phb_pec_error(pec, "Changing enabled BAR unsupported\n");
-        }
-        stack->nest_regs[reg] = val & 0xfffffff000000000ull;
-        break;
-    case PEC_NEST_STK_BAR_EN:
-        stack->nest_regs[reg] = val & 0xf000000000000000ull;
-        pnv_pec_stk_update_map(stack);
-        break;
-    case PEC_NEST_STK_DATA_FRZ_TYPE:
-    case PEC_NEST_STK_PBCQ_TUN_BAR:
-        /* Not used for now */
-        stack->nest_regs[reg] = val;
-        break;
-    default:
-        qemu_log_mask(LOG_UNIMP, "phb4_pec: nest_xscom_write 0x%"HWADDR_PRIx
-                      "=%"PRIx64"\n", addr, val);
-    }
-}
-
-static const MemoryRegionOps pnv_pec_stk_nest_xscom_ops = {
-    .read = pnv_pec_stk_nest_xscom_read,
-    .write = pnv_pec_stk_nest_xscom_write,
-    .valid.min_access_size = 8,
-    .valid.max_access_size = 8,
-    .impl.min_access_size = 8,
-    .impl.max_access_size = 8,
-    .endianness = DEVICE_BIG_ENDIAN,
-};
-
-static uint64_t pnv_pec_stk_pci_xscom_read(void *opaque, hwaddr addr,
-                                           unsigned size)
-{
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
-    uint32_t reg = addr >> 3;
-
-    /* TODO: add list of allowed registers and error out if not */
-    return stack->pci_regs[reg];
-}
-
-static void pnv_pec_stk_pci_xscom_write(void *opaque, hwaddr addr,
-                                        uint64_t val, unsigned size)
-{
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
-    uint32_t reg = addr >> 3;
-
-    switch (reg) {
-    case PEC_PCI_STK_PCI_FIR:
-        stack->nest_regs[reg] = val;
-        break;
-    case PEC_PCI_STK_PCI_FIR_CLR:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR] &= val;
-        break;
-    case PEC_PCI_STK_PCI_FIR_SET:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR] |= val;
-        break;
-    case PEC_PCI_STK_PCI_FIR_MSK:
-        stack->nest_regs[reg] = val;
-        break;
-    case PEC_PCI_STK_PCI_FIR_MSKC:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
-        break;
-    case PEC_PCI_STK_PCI_FIR_MSKS:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
-        break;
-    case PEC_PCI_STK_PCI_FIR_ACT0:
-    case PEC_PCI_STK_PCI_FIR_ACT1:
-        stack->nest_regs[reg] = val;
-        break;
-    case PEC_PCI_STK_PCI_FIR_WOF:
-        stack->nest_regs[reg] = 0;
-        break;
-    case PEC_PCI_STK_ETU_RESET:
-        stack->nest_regs[reg] = val & 0x8000000000000000ull;
-        /* TODO: Implement reset */
-        break;
-    case PEC_PCI_STK_PBAIB_ERR_REPORT:
-        break;
-    case PEC_PCI_STK_PBAIB_TX_CMD_CRED:
-    case PEC_PCI_STK_PBAIB_TX_DAT_CRED:
-        stack->nest_regs[reg] = val;
-        break;
-    default:
-        qemu_log_mask(LOG_UNIMP, "phb4_pec_stk: pci_xscom_write 0x%"HWADDR_PRIx
-                      "=%"PRIx64"\n", addr, val);
-    }
-}
-
-static const MemoryRegionOps pnv_pec_stk_pci_xscom_ops = {
-    .read = pnv_pec_stk_pci_xscom_read,
-    .write = pnv_pec_stk_pci_xscom_write,
-    .valid.min_access_size = 8,
-    .valid.max_access_size = 8,
-    .impl.min_access_size = 8,
-    .impl.max_access_size = 8,
-    .endianness = DEVICE_BIG_ENDIAN,
-};
-
 static void pnv_pec_instance_init(Object *obj)
 {
     PnvPhb4PecState *pec = PNV_PHB4_PEC(obj);
@@ -539,32 +287,7 @@ 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;
     int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
-    uint32_t pec_nest_base;
-    uint32_t pec_pci_base;
-    char name[64];
-
-    assert(pec);
-
-    /* Initialize the XSCOM regions for the stack registers */
-    snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-stack-%d",
-             pec->chip_id, pec->index, stack->stack_no);
-    pnv_xscom_region_init(&stack->nest_regs_mr, OBJECT(stack),
-                          &pnv_pec_stk_nest_xscom_ops, stack, name,
-                          PHB4_PEC_NEST_STK_REGS_COUNT);
-
-    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d",
-             pec->chip_id, pec->index, stack->stack_no);
-    pnv_xscom_region_init(&stack->pci_regs_mr, OBJECT(stack),
-                          &pnv_pec_stk_pci_xscom_ops, stack, name,
-                          PHB4_PEC_PCI_STK_REGS_COUNT);
-
-    /* PHB pass-through */
-    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
-             pec->chip_id, pec->index, stack->stack_no);
-    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
-                          &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
 
     object_property_set_int(OBJECT(&stack->phb), "chip-id", pec->chip_id,
                             &error_fatal);
@@ -577,21 +300,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), 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[] = {
-- 
2.33.1

[PATCH v5 3/5] ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack

[Daniel Henrique Barboza]

At this moment, stack->phb is the plain PnvPHB4 device itself instead of
a pointer to the device. This will present a problem when adding user
creatable devices because we can't deal with this struct and the
realize() callback from the user creatable device.

We can't get rid of this attribute, similar to what we did when enabling
pnv-phb3 user creatable devices, because pnv_phb4_update_regions() needs
to access stack->phb to do its job. This function is called twice in
pnv_pec_stk_update_map(), which is one of the nested xscom write
callbacks (via pnv_pec_stk_nest_xscom_write()). In fact,
pnv_pec_stk_update_map() code comment is explicit about how the order of
the unmap/map operations relates with the PHB subregions.

All of this indicates that this code is tied together in a way that we
either go on a crusade, featuring lots of refactories and redesign and
considerable pain, to decouple stack and phb mapping, or we allow stack
update_map operations to access the associated PHB as it is today even
after introducing pnv-phb4 user devices.

This patch chooses the latter. Instead of getting rid of stack->phb,
turn it into a PHB pointer. This will allow us to assign an user created
PHB to an existing stack later. In this process,
pnv_pec_stk_instance_init() is removed because stack->phb is being
initialized in stk_realize() instead.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         |  2 +-
 hw/pci-host/pnv_phb4_pec.c     | 20 +++++++-------------
 include/hw/pci-host/pnv_phb4.h |  7 +++++--
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 94fd8c858d..ee046725ac 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1728,7 +1728,7 @@ type_init(pnv_phb4_register_types);
 
 void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
 {
-    PnvPHB4 *phb = &stack->phb;
+    PnvPHB4 *phb = stack->phb;
 
     /* Unmap first always */
     if (memory_region_is_mapped(&phb->mr_regs)) {
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index bf0fdf33fd..d4c52a5d28 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -275,13 +275,6 @@ static const TypeInfo pnv_pec_type_info = {
     }
 };
 
-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);
-}
-
 static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
 {
     PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
@@ -289,15 +282,17 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
     int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
 
-    object_property_set_int(OBJECT(&stack->phb), "chip-id", pec->chip_id,
+    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
+
+    object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
                             &error_fatal);
-    object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
+    object_property_set_int(OBJECT(stack->phb), "index", phb_id,
                             &error_fatal);
-    object_property_set_int(OBJECT(&stack->phb), "version", pecc->version,
+    object_property_set_int(OBJECT(stack->phb), "version", pecc->version,
                             &error_fatal);
-    object_property_set_link(OBJECT(&stack->phb), "stack", OBJECT(stack),
+    object_property_set_link(OBJECT(stack->phb), "stack", OBJECT(stack),
                              &error_abort);
-    if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
+    if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
         return;
     }
 }
@@ -324,7 +319,6 @@ static const TypeInfo pnv_pec_stk_type_info = {
     .name          = TYPE_PNV_PHB4_PEC_STACK,
     .parent        = TYPE_DEVICE,
     .instance_size = sizeof(PnvPhb4PecStack),
-    .instance_init = pnv_pec_stk_instance_init,
     .class_init    = pnv_pec_stk_class_init,
     .interfaces    = (InterfaceInfo[]) {
         { TYPE_PNV_XSCOM_INTERFACE },
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 5ee996ebc6..82f054cf21 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -177,8 +177,11 @@ struct PnvPhb4PecStack {
     /* The owner PEC */
     PnvPhb4PecState *pec;
 
-    /* The actual PHB */
-    PnvPHB4 phb;
+    /*
+     * PHB4 pointer. pnv_phb4_update_regions() needs to access
+     * the PHB4 via a PnvPhb4PecStack pointer.
+     */
+    PnvPHB4 *phb;
 };
 
 struct PnvPhb4PecState {
-- 
2.33.1

[PATCH v5 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

This patch introduces pnv-phb4 user creatable devices that are created
in a similar manner as pnv-phb3 devices, allowing the user to interact
with the PHBs directly instead of creating PCI Express Controllers that
will create a certain amount of PHBs per controller index.

We accomplish this by doing the following:

- add a pnv_phb4_get_stack() helper to retrieve which stack an user
created phb4 would occupy;

- when dealing with an user created pnv-phb4 (detected by checking if
phb->stack is NULL at the start of phb4_realize()), retrieve its stack
and initialize its properties as done in stk_realize();

- use 'defaults_enabled()' in stk_realize() to avoid creating and
initializing a 'stack->phb' qdev that might be overwritten by an user
created pnv-phb4 device. This process is wrapped into a new helper
called pnv_pec_stk_default_phb_realize().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c     | 74 ++++++++++++++++++++++++++++++++++++--
 hw/pci-host/pnv_phb4_pec.c | 17 +++++++--
 hw/ppc/pnv.c               |  2 ++
 3 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index ee046725ac..ca2f4078e5 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1487,15 +1487,85 @@ static void pnv_phb4_instance_init(Object *obj)
     object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
 }
 
+static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
+                                           Error **errp)
+{
+    Pnv9Chip *chip9 = PNV9_CHIP(chip);
+    int chip_id = phb->chip_id;
+    int index = phb->phb_id;
+    int i, j;
+
+    for (i = 0; i < chip->num_pecs; i++) {
+        /*
+         * For each PEC, check the amount of stacks it supports
+         * and see if the given phb4 index matches a stack.
+         */
+        PnvPhb4PecState *pec = &chip9->pecs[i];
+
+        for (j = 0; j < pec->num_stacks; j++) {
+            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
+                return &pec->stacks[j];
+            }
+        }
+    }
+
+    error_setg(errp,
+               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
+               chip_id, index);
+
+    return NULL;
+}
+
 static void pnv_phb4_realize(DeviceState *dev, Error **errp)
 {
     PnvPHB4 *phb = PNV_PHB4(dev);
     PCIHostState *pci = PCI_HOST_BRIDGE(dev);
     XiveSource *xsrc = &phb->xsrc;
+    Error *local_err = NULL;
     int nr_irqs;
     char name[32];
 
-    assert(phb->stack);
+    /* User created PHB */
+    if (!phb->stack) {
+        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
+        PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
+        PnvPhb4PecClass *pecc;
+        BusState *s;
+
+        if (!chip) {
+            error_setg(errp, "invalid chip id: %d", phb->chip_id);
+            return;
+        }
+
+        phb->stack = pnv_phb4_get_stack(chip, phb, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        /* All other phb properties but 'version' are already set */
+        pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
+        object_property_set_int(OBJECT(phb), "version", pecc->version,
+                                &error_fatal);
+
+        /*
+         * Assign stack->phb since pnv_phb4_update_regions() uses it
+         * to access the phb.
+         */
+        phb->stack->phb = phb;
+
+        /*
+         * Reparent user created devices to the chip to build
+         * correctly the device tree.
+         */
+        pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
+
+        s = qdev_get_parent_bus(DEVICE(chip));
+        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
 
     /* Set the "big_phb" flag */
     phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
@@ -1600,7 +1670,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data)
     dc->realize         = pnv_phb4_realize;
     device_class_set_props(dc, pnv_phb4_properties);
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->user_creatable  = false;
+    dc->user_creatable  = true;
 
     xfc->notify         = pnv_phb4_xive_notify;
 }
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index d4c52a5d28..7fe7f1f007 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -19,6 +19,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/ppc/pnv.h"
 #include "hw/qdev-properties.h"
+#include "sysemu/sysemu.h"
 
 #include <libfdt.h>
 
@@ -275,9 +276,9 @@ static const TypeInfo pnv_pec_type_info = {
     }
 };
 
-static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
+static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
+                                            Error **errp)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
     PnvPhb4PecState *pec = stack->pec;
     PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
     int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
@@ -292,11 +293,23 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
                             &error_fatal);
     object_property_set_link(OBJECT(stack->phb), "stack", OBJECT(stack),
                              &error_abort);
+
     if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
         return;
     }
 }
 
+static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
+{
+    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
+
+    if (!defaults_enabled()) {
+        return;
+    }
+
+    pnv_pec_stk_default_phb_realize(stack, errp);
+}
+
 static Property pnv_pec_stk_properties[] = {
         DEFINE_PROP_UINT32("stack-no", PnvPhb4PecStack, stack_no, 0),
         DEFINE_PROP_LINK("pec", PnvPhb4PecStack, pec, TYPE_PNV_PHB4_PEC,
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index fe7e67e73a..837146a2fb 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
     pmc->dt_power_mgt = pnv_dt_power_mgt;
+
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4);
 }
 
 static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
-- 
2.33.1

Re: [PATCH v5 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Cédric Le Goater]

On 1/11/22 14:10, Daniel Henrique Barboza wrote:
> This patch introduces pnv-phb4 user creatable devices that are created
> in a similar manner as pnv-phb3 devices, allowing the user to interact
> with the PHBs directly instead of creating PCI Express Controllers that
> will create a certain amount of PHBs per controller index.
> 
> We accomplish this by doing the following:
> 
> - add a pnv_phb4_get_stack() helper to retrieve which stack an user
> created phb4 would occupy;
> 
> - when dealing with an user created pnv-phb4 (detected by checking if
> phb->stack is NULL at the start of phb4_realize()), retrieve its stack
> and initialize its properties as done in stk_realize();
> 
> - use 'defaults_enabled()' in stk_realize() to avoid creating and
> initializing a 'stack->phb' qdev that might be overwritten by an user
> created pnv-phb4 device. This process is wrapped into a new helper
> called pnv_pec_stk_default_phb_realize().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Nothing is left in the stack model. I think the next cleanup is to
get rid of it.

Thanks,

C.


> ---
>   hw/pci-host/pnv_phb4.c     | 74 ++++++++++++++++++++++++++++++++++++--
>   hw/pci-host/pnv_phb4_pec.c | 17 +++++++--
>   hw/ppc/pnv.c               |  2 ++
>   3 files changed, 89 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index ee046725ac..ca2f4078e5 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1487,15 +1487,85 @@ static void pnv_phb4_instance_init(Object *obj)
>       object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
>   }
>   
> +static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
> +                                           Error **errp)
> +{
> +    Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +    int chip_id = phb->chip_id;
> +    int index = phb->phb_id;
> +    int i, j;
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
> +        /*
> +         * For each PEC, check the amount of stacks it supports
> +         * and see if the given phb4 index matches a stack.
> +         */
> +        PnvPhb4PecState *pec = &chip9->pecs[i];
> +
> +        for (j = 0; j < pec->num_stacks; j++) {
> +            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> +                return &pec->stacks[j];
> +            }
> +        }
> +    }
> +
> +    error_setg(errp,
> +               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
> +               chip_id, index);
> +
> +    return NULL;
> +}
> +
>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB4 *phb = PNV_PHB4(dev);
>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>       XiveSource *xsrc = &phb->xsrc;
> +    Error *local_err = NULL;
>       int nr_irqs;
>       char name[32];
>   
> -    assert(phb->stack);
> +    /* User created PHB */
> +    if (!phb->stack) {
> +        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +        PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
> +        PnvPhb4PecClass *pecc;
> +        BusState *s;
> +
> +        if (!chip) {
> +            error_setg(errp, "invalid chip id: %d", phb->chip_id);
> +            return;
> +        }
> +
> +        phb->stack = pnv_phb4_get_stack(chip, phb, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +
> +        /* All other phb properties but 'version' are already set */
> +        pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
> +        object_property_set_int(OBJECT(phb), "version", pecc->version,
> +                                &error_fatal);
> +
> +        /*
> +         * Assign stack->phb since pnv_phb4_update_regions() uses it
> +         * to access the phb.
> +         */
> +        phb->stack->phb = phb;
> +
> +        /*
> +         * Reparent user created devices to the chip to build
> +         * correctly the device tree.
> +         */
> +        pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
> +
> +        s = qdev_get_parent_bus(DEVICE(chip));
> +        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
>   
>       /* Set the "big_phb" flag */
>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
> @@ -1600,7 +1670,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data)
>       dc->realize         = pnv_phb4_realize;
>       device_class_set_props(dc, pnv_phb4_properties);
>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    dc->user_creatable  = false;
> +    dc->user_creatable  = true;
>   
>       xfc->notify         = pnv_phb4_xive_notify;
>   }
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index d4c52a5d28..7fe7f1f007 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -19,6 +19,7 @@
>   #include "hw/pci/pci_bus.h"
>   #include "hw/ppc/pnv.h"
>   #include "hw/qdev-properties.h"
> +#include "sysemu/sysemu.h"
>   
>   #include <libfdt.h>
>   
> @@ -275,9 +276,9 @@ static const TypeInfo pnv_pec_type_info = {
>       }
>   };
>   
> -static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> +static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
> +                                            Error **errp)
>   {
> -    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
>       PnvPhb4PecState *pec = stack->pec;
>       PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
> @@ -292,11 +293,23 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>                               &error_fatal);
>       object_property_set_link(OBJECT(stack->phb), "stack", OBJECT(stack),
>                                &error_abort);
> +
>       if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
>           return;
>       }
>   }
>   
> +static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> +{
> +    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
> +
> +    if (!defaults_enabled()) {
> +        return;
> +    }
> +
> +    pnv_pec_stk_default_phb_realize(stack, errp);
> +}
> +
>   static Property pnv_pec_stk_properties[] = {
>           DEFINE_PROP_UINT32("stack-no", PnvPhb4PecStack, stack_no, 0),
>           DEFINE_PROP_LINK("pec", PnvPhb4PecStack, pec, TYPE_PNV_PHB4_PEC,
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index fe7e67e73a..837146a2fb 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>       pmc->compat = compat;
>       pmc->compat_size = sizeof(compat);
>       pmc->dt_power_mgt = pnv_dt_power_mgt;
> +
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4);
>   }
>   
>   static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
> 

Re: [PATCH v5 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

On 1/11/22 11:42, Cédric Le Goater wrote:
> On 1/11/22 14:10, Daniel Henrique Barboza wrote:
>> This patch introduces pnv-phb4 user creatable devices that are created
>> in a similar manner as pnv-phb3 devices, allowing the user to interact
>> with the PHBs directly instead of creating PCI Express Controllers that
>> will create a certain amount of PHBs per controller index.
>>
>> We accomplish this by doing the following:
>>
>> - add a pnv_phb4_get_stack() helper to retrieve which stack an user
>> created phb4 would occupy;
>>
>> - when dealing with an user created pnv-phb4 (detected by checking if
>> phb->stack is NULL at the start of phb4_realize()), retrieve its stack
>> and initialize its properties as done in stk_realize();
>>
>> - use 'defaults_enabled()' in stk_realize() to avoid creating and
>> initializing a 'stack->phb' qdev that might be overwritten by an user
>> created pnv-phb4 device. This process is wrapped into a new helper
>> called pnv_pec_stk_default_phb_realize().
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Nothing is left in the stack model. I think the next cleanup is to
> get rid of it.


The first step would be to move some MemoryOps from the stack to the phb, then
little by little we can get into a point where the stack will just be a pointer
to its phb.

This is something that we can keep working on in smaller bits here and there.
I mean, assuming that we're not going to use this code base for PHB5. If that's
the case then I can prioritize this cleanup.



Thanks,

Daniel




> 
> Thanks,
> 
> C.
> 
> 
>> ---
>>   hw/pci-host/pnv_phb4.c     | 74 ++++++++++++++++++++++++++++++++++++--
>>   hw/pci-host/pnv_phb4_pec.c | 17 +++++++--
>>   hw/ppc/pnv.c               |  2 ++
>>   3 files changed, 89 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index ee046725ac..ca2f4078e5 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1487,15 +1487,85 @@ static void pnv_phb4_instance_init(Object *obj)
>>       object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
>>   }
>> +static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
>> +                                           Error **errp)
>> +{
>> +    Pnv9Chip *chip9 = PNV9_CHIP(chip);
>> +    int chip_id = phb->chip_id;
>> +    int index = phb->phb_id;
>> +    int i, j;
>> +
>> +    for (i = 0; i < chip->num_pecs; i++) {
>> +        /*
>> +         * For each PEC, check the amount of stacks it supports
>> +         * and see if the given phb4 index matches a stack.
>> +         */
>> +        PnvPhb4PecState *pec = &chip9->pecs[i];
>> +
>> +        for (j = 0; j < pec->num_stacks; j++) {
>> +            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
>> +                return &pec->stacks[j];
>> +            }
>> +        }
>> +    }
>> +
>> +    error_setg(errp,
>> +               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
>> +               chip_id, index);
>> +
>> +    return NULL;
>> +}
>> +
>>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvPHB4 *phb = PNV_PHB4(dev);
>>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>>       XiveSource *xsrc = &phb->xsrc;
>> +    Error *local_err = NULL;
>>       int nr_irqs;
>>       char name[32];
>> -    assert(phb->stack);
>> +    /* User created PHB */
>> +    if (!phb->stack) {
>> +        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>> +        PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
>> +        PnvPhb4PecClass *pecc;
>> +        BusState *s;
>> +
>> +        if (!chip) {
>> +            error_setg(errp, "invalid chip id: %d", phb->chip_id);
>> +            return;
>> +        }
>> +
>> +        phb->stack = pnv_phb4_get_stack(chip, phb, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +
>> +        /* All other phb properties but 'version' are already set */
>> +        pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
>> +        object_property_set_int(OBJECT(phb), "version", pecc->version,
>> +                                &error_fatal);
>> +
>> +        /*
>> +         * Assign stack->phb since pnv_phb4_update_regions() uses it
>> +         * to access the phb.
>> +         */
>> +        phb->stack->phb = phb;
>> +
>> +        /*
>> +         * Reparent user created devices to the chip to build
>> +         * correctly the device tree.
>> +         */
>> +        pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
>> +
>> +        s = qdev_get_parent_bus(DEVICE(chip));
>> +        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +    }
>>       /* Set the "big_phb" flag */
>>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
>> @@ -1600,7 +1670,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data)
>>       dc->realize         = pnv_phb4_realize;
>>       device_class_set_props(dc, pnv_phb4_properties);
>>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> -    dc->user_creatable  = false;
>> +    dc->user_creatable  = true;
>>       xfc->notify         = pnv_phb4_xive_notify;
>>   }
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index d4c52a5d28..7fe7f1f007 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -19,6 +19,7 @@
>>   #include "hw/pci/pci_bus.h"
>>   #include "hw/ppc/pnv.h"
>>   #include "hw/qdev-properties.h"
>> +#include "sysemu/sysemu.h"
>>   #include <libfdt.h>
>> @@ -275,9 +276,9 @@ static const TypeInfo pnv_pec_type_info = {
>>       }
>>   };
>> -static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>> +static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
>> +                                            Error **errp)
>>   {
>> -    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
>>       PnvPhb4PecState *pec = stack->pec;
>>       PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
>> @@ -292,11 +293,23 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>>                               &error_fatal);
>>       object_property_set_link(OBJECT(stack->phb), "stack", OBJECT(stack),
>>                                &error_abort);
>> +
>>       if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
>>           return;
>>       }
>>   }
>> +static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
>> +
>> +    if (!defaults_enabled()) {
>> +        return;
>> +    }
>> +
>> +    pnv_pec_stk_default_phb_realize(stack, errp);
>> +}
>> +
>>   static Property pnv_pec_stk_properties[] = {
>>           DEFINE_PROP_UINT32("stack-no", PnvPhb4PecStack, stack_no, 0),
>>           DEFINE_PROP_LINK("pec", PnvPhb4PecStack, pec, TYPE_PNV_PHB4_PEC,
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index fe7e67e73a..837146a2fb 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>>       pmc->compat = compat;
>>       pmc->compat_size = sizeof(compat);
>>       pmc->dt_power_mgt = pnv_dt_power_mgt;
>> +
>> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4);
>>   }
>>   static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
>>
> 

Re: [PATCH v5 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Cédric Le Goater]

On 1/11/22 15:57, Daniel Henrique Barboza wrote:
> 
> 
> On 1/11/22 11:42, Cédric Le Goater wrote:
>> On 1/11/22 14:10, Daniel Henrique Barboza wrote:
>>> This patch introduces pnv-phb4 user creatable devices that are created
>>> in a similar manner as pnv-phb3 devices, allowing the user to interact
>>> with the PHBs directly instead of creating PCI Express Controllers that
>>> will create a certain amount of PHBs per controller index.
>>>
>>> We accomplish this by doing the following:
>>>
>>> - add a pnv_phb4_get_stack() helper to retrieve which stack an user
>>> created phb4 would occupy;
>>>
>>> - when dealing with an user created pnv-phb4 (detected by checking if
>>> phb->stack is NULL at the start of phb4_realize()), retrieve its stack
>>> and initialize its properties as done in stk_realize();
>>>
>>> - use 'defaults_enabled()' in stk_realize() to avoid creating and
>>> initializing a 'stack->phb' qdev that might be overwritten by an user
>>> created pnv-phb4 device. This process is wrapped into a new helper
>>> called pnv_pec_stk_default_phb_realize().
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> Nothing is left in the stack model. I think the next cleanup is to
>> get rid of it.
> 
> 
> The first step would be to move some MemoryOps from the stack to the phb, then
> little by little we can get into a point where the stack will just be a pointer
> to its phb.
> 
> This is something that we can keep working on in smaller bits here and there.
> I mean, assuming that we're not going to use this code base for PHB5. If that's
> the case then I can prioritize this cleanup.

PHB5 uses the same models. Only the PHB version and the root port
model need some adaptation.


On branch https://github.com/legoater/qemu/commits/powernv-7.0,
I have merged :

   ppc/pnv: Move root port allocation under pnv_pec_stk_default_phb_realize()
   ppc/pnv: Add a 'rp_model' class attribute for the PHB4 PEC
   ppc/pnv: Remove PHB4 version property

preparing ground for :

   ppc/pnv: Add model for POWER10 PHB5 PCIe Host bridge

Should we rework slightly your patchset to include them ? Or we don't
care may be. Please advise :)

Thanks,

C.

Re: [PATCH v5 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

On 1/11/22 12:47, Cédric Le Goater wrote:
> On 1/11/22 15:57, Daniel Henrique Barboza wrote:
>>
>>
>> On 1/11/22 11:42, Cédric Le Goater wrote:
>>> On 1/11/22 14:10, Daniel Henrique Barboza wrote:
>>>> This patch introduces pnv-phb4 user creatable devices that are created
>>>> in a similar manner as pnv-phb3 devices, allowing the user to interact
>>>> with the PHBs directly instead of creating PCI Express Controllers that
>>>> will create a certain amount of PHBs per controller index.
>>>>
>>>> We accomplish this by doing the following:
>>>>
>>>> - add a pnv_phb4_get_stack() helper to retrieve which stack an user
>>>> created phb4 would occupy;
>>>>
>>>> - when dealing with an user created pnv-phb4 (detected by checking if
>>>> phb->stack is NULL at the start of phb4_realize()), retrieve its stack
>>>> and initialize its properties as done in stk_realize();
>>>>
>>>> - use 'defaults_enabled()' in stk_realize() to avoid creating and
>>>> initializing a 'stack->phb' qdev that might be overwritten by an user
>>>> created pnv-phb4 device. This process is wrapped into a new helper
>>>> called pnv_pec_stk_default_phb_realize().
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>
>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>
>>> Nothing is left in the stack model. I think the next cleanup is to
>>> get rid of it.
>>
>>
>> The first step would be to move some MemoryOps from the stack to the phb, then
>> little by little we can get into a point where the stack will just be a pointer
>> to its phb.
>>
>> This is something that we can keep working on in smaller bits here and there.
>> I mean, assuming that we're not going to use this code base for PHB5. If that's
>> the case then I can prioritize this cleanup.
> 
> PHB5 uses the same models. Only the PHB version and the root port
> model need some adaptation.
> 
> 
> On branch https://github.com/legoater/qemu/commits/powernv-7.0,
> I have merged :
> 
>    ppc/pnv: Move root port allocation under pnv_pec_stk_default_phb_realize()
>    ppc/pnv: Add a 'rp_model' class attribute for the PHB4 PEC
>    ppc/pnv: Remove PHB4 version property
> 
> preparing ground for :
> 
>    ppc/pnv: Add model for POWER10 PHB5 PCIe Host bridge
> 
> Should we rework slightly your patchset to include them ? Or we don't
> care may be. Please advise :)

I guess it's fine to add those 3 patches. Do you want me to re-send this series with
them included?


Daniel

> 
> Thanks,
> 
> C.
> 

Re: [PATCH v5 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Cédric Le Goater]

>> On branch https://github.com/legoater/qemu/commits/powernv-7.0,
>> I have merged :
>>
>>    ppc/pnv: Move root port allocation under pnv_pec_stk_default_phb_realize()
>>    ppc/pnv: Add a 'rp_model' class attribute for the PHB4 PEC
>>    ppc/pnv: Remove PHB4 version property
>>
>> preparing ground for :
>>
>>    ppc/pnv: Add model for POWER10 PHB5 PCIe Host bridge
>>
>> Should we rework slightly your patchset to include them ? Or we don't
>> care may be. Please advise :)
> 
> I guess it's fine to add those 3 patches. Do you want me to re-send this series with
> them included?

That would be interesting if you could merge the changes in your
others patches. If not, then we will just send them before PHB5
as prereq cleanups.

Thanks,

C.

[PATCH v5 5/5] ppc/pnv: turn pnv_phb4_update_regions() into static

[Daniel Henrique Barboza]

Its only callers are inside pnv_phb4.c.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 52 +++++++++++++++++-----------------
 include/hw/pci-host/pnv_phb4.h |  1 -
 2 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index ca2f4078e5..30e609d78e 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -868,6 +868,32 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
     return stack->nest_regs[reg];
 }
 
+static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
+{
+    PnvPHB4 *phb = stack->phb;
+
+    /* Unmap first always */
+    if (memory_region_is_mapped(&phb->mr_regs)) {
+        memory_region_del_subregion(&stack->phbbar, &phb->mr_regs);
+    }
+    if (memory_region_is_mapped(&phb->xsrc.esb_mmio)) {
+        memory_region_del_subregion(&stack->intbar, &phb->xsrc.esb_mmio);
+    }
+
+    /* Map registers if enabled */
+    if (memory_region_is_mapped(&stack->phbbar)) {
+        memory_region_add_subregion(&stack->phbbar, 0, &phb->mr_regs);
+    }
+
+    /* Map ESB if enabled */
+    if (memory_region_is_mapped(&stack->intbar)) {
+        memory_region_add_subregion(&stack->intbar, 0, &phb->xsrc.esb_mmio);
+    }
+
+    /* Check/update m32 */
+    pnv_phb4_check_all_mbt(phb);
+}
+
 static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
 {
     PnvPhb4PecState *pec = stack->pec;
@@ -1796,32 +1822,6 @@ static void pnv_phb4_register_types(void)
 
 type_init(pnv_phb4_register_types);
 
-void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
-{
-    PnvPHB4 *phb = stack->phb;
-
-    /* Unmap first always */
-    if (memory_region_is_mapped(&phb->mr_regs)) {
-        memory_region_del_subregion(&stack->phbbar, &phb->mr_regs);
-    }
-    if (memory_region_is_mapped(&phb->xsrc.esb_mmio)) {
-        memory_region_del_subregion(&stack->intbar, &phb->xsrc.esb_mmio);
-    }
-
-    /* Map registers if enabled */
-    if (memory_region_is_mapped(&stack->phbbar)) {
-        memory_region_add_subregion(&stack->phbbar, 0, &phb->mr_regs);
-    }
-
-    /* Map ESB if enabled */
-    if (memory_region_is_mapped(&stack->intbar)) {
-        memory_region_add_subregion(&stack->intbar, 0, &phb->xsrc.esb_mmio);
-    }
-
-    /* Check/update m32 */
-    pnv_phb4_check_all_mbt(phb);
-}
-
 void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon)
 {
     uint32_t offset = phb->regs[PHB_INT_NOTIFY_INDEX >> 3];
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 82f054cf21..4b7ce8a723 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -131,7 +131,6 @@ struct PnvPHB4 {
 };
 
 void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
-void pnv_phb4_update_regions(PnvPhb4PecStack *stack);
 int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
 
-- 
2.33.1

Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Cédric Le Goater]

On 1/11/22 14:10, Daniel Henrique Barboza wrote:
> Hi,
> 
> This version implements Cedric's review suggestions from v4. No
> drastic design changes were made.
> 
> Changes from v4:
> - patches 1,3,5: unchanged
> - patch 2:
>    * renamed function to pnv_phb4_xscom_realize()
>    * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
> - patch 4:
>    * changed pnv_phb4_get_stack signature to use chip and phb
>    * added a new helper called pnv_pec_stk_default_phb_realize() to
> realize the default phb when running with defaults
> - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
> 
> Daniel Henrique Barboza (5):
>    ppc/pnv: set phb4 properties in stk_realize()
>    ppc/pnv: move PHB4 XSCOM init to phb4_realize()
>    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>    ppc/pnv: turn pnv_phb4_update_regions() into static
> 
>   hw/pci-host/pnv_phb4.c         | 430 ++++++++++++++++++++++++++++++---
>   hw/pci-host/pnv_phb4_pec.c     | 329 ++-----------------------
>   hw/ppc/pnv.c                   |   2 +
>   include/hw/pci-host/pnv_phb4.h |   8 +-
>   4 files changed, 431 insertions(+), 338 deletions(-)
> 


Applied to ppc7.0.

Thanks,

C.

Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Thomas Huth]

On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
> Hi,
> 
> This version implements Cedric's review suggestions from v4. No
> drastic design changes were made.
> 
> Changes from v4:
> - patches 1,3,5: unchanged
> - patch 2:
>    * renamed function to pnv_phb4_xscom_realize()
>    * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
> - patch 4:
>    * changed pnv_phb4_get_stack signature to use chip and phb
>    * added a new helper called pnv_pec_stk_default_phb_realize() to
> realize the default phb when running with defaults
> - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
> 
> Daniel Henrique Barboza (5):
>    ppc/pnv: set phb4 properties in stk_realize()
>    ppc/pnv: move PHB4 XSCOM init to phb4_realize()
>    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>    ppc/pnv: turn pnv_phb4_update_regions() into static

It's now possible to crash QEMU with the pnv-phb4 device:

$ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
Unexpected error in object_property_try_add() at 
../../devel/qemu/qom/object.c:1229:
qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 
'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
Aborted (core dumped)

Any ideas how to fix this?

  Thomas

Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

On 3/10/22 15:49, Thomas Huth wrote:
> On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This version implements Cedric's review suggestions from v4. No
>> drastic design changes were made.
>>
>> Changes from v4:
>> - patches 1,3,5: unchanged
>> - patch 2:
>>    * renamed function to pnv_phb4_xscom_realize()
>>    * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
>> - patch 4:
>>    * changed pnv_phb4_get_stack signature to use chip and phb
>>    * added a new helper called pnv_pec_stk_default_phb_realize() to
>> realize the default phb when running with defaults
>> - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
>>
>> Daniel Henrique Barboza (5):
>>    ppc/pnv: set phb4 properties in stk_realize()
>>    ppc/pnv: move PHB4 XSCOM init to phb4_realize()
>>    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
>>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>>    ppc/pnv: turn pnv_phb4_update_regions() into static
> 
> It's now possible to crash QEMU with the pnv-phb4 device:
> 
> $ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
> Unexpected error in object_property_try_add() at ../../devel/qemu/qom/object.c:1229:
> qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
> Aborted (core dumped)
> 
> Any ideas how to fix this?

Thanks for catching this up.

The issue here is that we're not handling the case where an user adds a pnv-phb4 device
when running default settings (no -nodefaults). With default settings we are adding all
pnv-phb4 devices that are available to the machine, having no room for any additional
user creatable pnv-phb4 devices.

A similar situation happens with the powernv8 machine which errors out with a different
error message:

$ ./qemu-system-ppc64 -nographic -M powernv8 -device pnv-phb3
qemu-system-ppc64: -device pnv-phb3: Can't add chassis slot, error -16


Adding all possible phbs by default is a behavior these machines had since they were introduced,
and I don't think we want to change it. Thus, a fix would be to forbid user created pnv-phb devices
when running with defaults.


I'll see what I can do. Thanks,



Daniel


> 
>   Thomas
> 

Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Cédric Le Goater]

Hello,

In 3/11/22 03:18, Daniel Henrique Barboza wrote:
> 
> 
> On 3/10/22 15:49, Thomas Huth wrote:
>> On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
>>> Hi,
>>>
>>> This version implements Cedric's review suggestions from v4. No
>>> drastic design changes were made.
>>>
>>> Changes from v4:
>>> - patches 1,3,5: unchanged
>>> - patch 2:
>>>    * renamed function to pnv_phb4_xscom_realize()
>>>    * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
>>> - patch 4:
>>>    * changed pnv_phb4_get_stack signature to use chip and phb
>>>    * added a new helper called pnv_pec_stk_default_phb_realize() to
>>> realize the default phb when running with defaults
>>> - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
>>>
>>> Daniel Henrique Barboza (5):
>>>    ppc/pnv: set phb4 properties in stk_realize()
>>>    ppc/pnv: move PHB4 XSCOM init to phb4_realize()
>>>    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
>>>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>>>    ppc/pnv: turn pnv_phb4_update_regions() into static
>>
>> It's now possible to crash QEMU with the pnv-phb4 device:
>>
>> $ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
>> Unexpected error in object_property_try_add() at ../../devel/qemu/qom/object.c:1229:
>> qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
>> Aborted (core dumped)
>>
>> Any ideas how to fix this?
> 
> Thanks for catching this up.
> 
> The issue here is that we're not handling the case where an user adds a pnv-phb4 device
> when running default settings (no -nodefaults). With default settings we are adding all
> pnv-phb4 devices that are available to the machine, having no room for any additional
> user creatable pnv-phb4 devices.
> 
> A similar situation happens with the powernv8 machine which errors out with a different
> error message:
> 
> $ ./qemu-system-ppc64 -nographic -M powernv8 -device pnv-phb3
> qemu-system-ppc64: -device pnv-phb3: Can't add chassis slot, error -16
> 
> 
> Adding all possible phbs by default is a behavior these machines had since they were introduced,
> and I don't think we want to change it. Thus, a fix would be to forbid user created pnv-phb devices
> when running with defaults.


On a real system with POWER{8,9,10} processors, PHBs are sub-units of
the processor, they can be deactivated by firmware but not plugged in
or out like a PCI adapter on a slot. Nevertheless, it seemed to be
good idea to have user-created PHBs for testing purposes.

Let's come back to the PowerNV requirements.

  1. having a limited set of PHBs speedups boot time.
  2. it is useful to be able to mimic a partially broken topology you
     some time have to deal with during bring-up.

PowerNV is also used for distro install tests and having libvirt
support eases these tasks. libvirt prefers to run the machine with
-nodefaults to be sure not to drag unexpected devices which would need
to be defined in the domain file without being specified on the QEMU
command line. For this reason :

  3. -nodefaults should not include default PHBs

The solution we came with was to introduce user-created PHB{3,4,5}
devices on the powernv{8,9,10} machines. Reality proves to be a bit
more complex, internally when modeling such devices, and externally
when dealing with the user interface.

So, to make sure that we don't ship a crappy feature in QEMU 7.0,
I think we should step back a little.

Req 1. and 2. can be simply addressed differently with a machine option:
"phb-mask=<uint>", which QEMU would use to enable/disable PHB device
nodes when creating the device tree.

For Req 3., we need to make sure we are taking the right approach. It
seems that we should expose a new type of user-created PHB device,
a generic virtualized one, that libvirt would use and not one depending
on the processor revision. This needs more thinking.

Hence, I am proposing we drop user-created PHB{3,4,5} for QEMU-7.0.
All the cleanups we did are not lost and they will be useful for the
next steps in QEMU 7.1.


Thanks,

C.

Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

On 3/11/22 09:45, Cédric Le Goater wrote:
> Hello,
> 
> In 3/11/22 03:18, Daniel Henrique Barboza wrote:
>>
>>
>> On 3/10/22 15:49, Thomas Huth wrote:
>>> On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
>>>> Hi,
>>>>
>>>> This version implements Cedric's review suggestions from v4. No
>>>> drastic design changes were made.
>>>>
>>>> Changes from v4:
>>>> - patches 1,3,5: unchanged
>>>> - patch 2:
>>>>    * renamed function to pnv_phb4_xscom_realize()
>>>>    * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
>>>> - patch 4:
>>>>    * changed pnv_phb4_get_stack signature to use chip and phb
>>>>    * added a new helper called pnv_pec_stk_default_phb_realize() to
>>>> realize the default phb when running with defaults
>>>> - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
>>>>
>>>> Daniel Henrique Barboza (5):
>>>>    ppc/pnv: set phb4 properties in stk_realize()
>>>>    ppc/pnv: move PHB4 XSCOM init to phb4_realize()
>>>>    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
>>>>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>>>>    ppc/pnv: turn pnv_phb4_update_regions() into static
>>>
>>> It's now possible to crash QEMU with the pnv-phb4 device:
>>>
>>> $ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
>>> Unexpected error in object_property_try_add() at ../../devel/qemu/qom/object.c:1229:
>>> qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
>>> Aborted (core dumped)
>>>
>>> Any ideas how to fix this?
>>
>> Thanks for catching this up.
>>
>> The issue here is that we're not handling the case where an user adds a pnv-phb4 device
>> when running default settings (no -nodefaults). With default settings we are adding all
>> pnv-phb4 devices that are available to the machine, having no room for any additional
>> user creatable pnv-phb4 devices.
>>
>> A similar situation happens with the powernv8 machine which errors out with a different
>> error message:
>>
>> $ ./qemu-system-ppc64 -nographic -M powernv8 -device pnv-phb3
>> qemu-system-ppc64: -device pnv-phb3: Can't add chassis slot, error -16
>>
>>
>> Adding all possible phbs by default is a behavior these machines had since they were introduced,
>> and I don't think we want to change it. Thus, a fix would be to forbid user created pnv-phb devices
>> when running with defaults.
> 
> 
> On a real system with POWER{8,9,10} processors, PHBs are sub-units of
> the processor, they can be deactivated by firmware but not plugged in
> or out like a PCI adapter on a slot. Nevertheless, it seemed to be
> good idea to have user-created PHBs for testing purposes.
> 
> Let's come back to the PowerNV requirements.
> 
>   1. having a limited set of PHBs speedups boot time.
>   2. it is useful to be able to mimic a partially broken topology you
>      some time have to deal with during bring-up.
> 
> PowerNV is also used for distro install tests and having libvirt
> support eases these tasks. libvirt prefers to run the machine with
> -nodefaults to be sure not to drag unexpected devices which would need
> to be defined in the domain file without being specified on the QEMU
> command line. For this reason :
> 
>   3. -nodefaults should not include default PHBs
> 
> The solution we came with was to introduce user-created PHB{3,4,5}
> devices on the powernv{8,9,10} machines. Reality proves to be a bit
> more complex, internally when modeling such devices, and externally
> when dealing with the user interface.
> 
> So, to make sure that we don't ship a crappy feature in QEMU 7.0,
> I think we should step back a little.
> 
> Req 1. and 2. can be simply addressed differently with a machine option:
> "phb-mask=<uint>", which QEMU would use to enable/disable PHB device
> nodes when creating the device tree.

Would this property only impact the device-tree?

Let's say that we're running a 2 socket pnv4 machine, with default settings. That
would give us 12 PHBs. So phb-mask=FFFF is kind of a no-op because you're adding
all PHBs, phb-mask=0001 would add just the first PHB (index=0 chip-id=0) and so
on. Is that correct?

> 
> For Req 3., we need to make sure we are taking the right approach. It
> seems that we should expose a new type of user-created PHB device,
> a generic virtualized one, that libvirt would use and not one depending
> on the processor revision. This needs more thinking.

libvirt support isn't upstream yet. We have room to make changes.

I agree that creating generic, un-versioned pnv-phb and pnv-phb-root-port devices
that can be used by all pnv machines would be good for libvirt support.

> 
> Hence, I am proposing we drop user-created PHB{3,4,5} for QEMU-7.0.
> All the cleanups we did are not lost and they will be useful for the
> next steps in QEMU 7.1.


Seems like a good idea for now. Even if we decide to expose them in the end, if we
keep them user visible for the 7.0 release we won't be able to change our minds
in 7.1.


Thanks,


Daniel

> 
> 
> Thanks,
> 
> C.
> 

Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Cédric Le Goater]

On 3/10/22 19:49, Thomas Huth wrote:
> On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This version implements Cedric's review suggestions from v4. No
>> drastic design changes were made.
>>
>> Changes from v4:
>> - patches 1,3,5: unchanged
>> - patch 2:
>>    * renamed function to pnv_phb4_xscom_realize()
>>    * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
>> - patch 4:
>>    * changed pnv_phb4_get_stack signature to use chip and phb
>>    * added a new helper called pnv_pec_stk_default_phb_realize() to
>> realize the default phb when running with defaults
>> - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
>>
>> Daniel Henrique Barboza (5):
>>    ppc/pnv: set phb4 properties in stk_realize()
>>    ppc/pnv: move PHB4 XSCOM init to phb4_realize()
>>    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
>>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>>    ppc/pnv: turn pnv_phb4_update_regions() into static
> 
> It's now possible to crash QEMU with the pnv-phb4 device:
> 
> $ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
> Unexpected error in object_property_try_add() at ../../devel/qemu/qom/object.c:1229:
> qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
> Aborted (core dumped)
> 
> Any ideas how to fix this?

This was introduced by :

   commit 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM hierarchy")

It could be fixed with :

@@ -1598,15 +1598,15 @@ static void pnv_phb4_realize(DeviceState
              error_propagate(errp, local_err);
              return;
          }
-    }
  
-    /* Reparent the PHB to the chip to build the device tree */
-    pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
+        /* Reparent the PHB to the chip to build the device tree */
+        pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
  
-    s = qdev_get_parent_bus(DEVICE(chip));
-    if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
-        error_propagate(errp, local_err);
-        return;
+        s = qdev_get_parent_bus(DEVICE(chip));
+        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
      }
  
      /* Set the "big_phb" flag */


but I am not sure we want to keep user-created PHB* devices.

Thanks,

C.