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

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

[Daniel Henrique Barboza]

Hi,

This new version contains the remaining v3 patches that weren't accepted
yet, which is the case of patches 10, 2 and 3.

There are fewer patches this time due to 2 design changes made: move the
phb property setup to stk_realize() and all XSCOM initialization to
phb4_realize().

As a note/TODO, at the end of this work it became clear that it would be
interesting to rethink what we expect about the PnvPhb4Stack object
in the overall design. The object ended up with no init() and its realize
is used as a glorified setup for its phb, being a no-op if the user runs
with -nodefaults. There might be an opportunity for further cleanups and
simplifications.

changes from v3:
- patches 2, 3, and 10 from v3: accepted
- patch 1:
  * do not create a function to set the properties
  * set all properties in stk_realize()
  * remove the 'phb-id' link during this process instead of using a separated
patch
- patch 2:
  * move all XSCOM init to phb4_realize()
- patch 4:
  * review changes proposed by Cedric in v3
- patch 5 (new):
  * a trivial cleanup following patch 2
- v3 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg01931.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         | 431 ++++++++++++++++++++++++++++++---
 hw/pci-host/pnv_phb4_pec.c     | 317 +-----------------------
 hw/ppc/pnv.c                   |   2 +
 include/hw/pci-host/pnv_phb4.h |   8 +-
 4 files changed, 422 insertions(+), 336 deletions(-)

-- 
2.33.1

[PATCH v4 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().

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

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

[Cédric Le Goater]

On 1/11/22 01:55, Daniel Henrique Barboza wrote:
> 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().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Thanks,

C.

> ---
>   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),
> 

[PATCH v4 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 init 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.

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..1bd74fd932 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_init(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);
@@ -1195,6 +1497,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
 
     assert(phb->stack);
 
+    pnv_phb4_XSCOM_init(phb);
+
     /* Set the "big_phb" flag */
     phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
 
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

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

[Cédric Le Goater]

On 1/11/22 01:55, Daniel Henrique Barboza wrote:
> 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 init 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.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

One comment below,

> ---
>   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..1bd74fd932 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_init(PnvPHB4 *phb)

I would prefer pnv_phb4_xscom_realize. I will fix it when merging the
patch if you don't mind.

> +{
> +    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);
> @@ -1195,6 +1497,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>   
>       assert(phb->stack);
>   
> +    pnv_phb4_XSCOM_init(phb);
> +

and I would rather install the XSCOM regions at the end of realize

Thanks,

C.



>       /* Set the "big_phb" flag */
>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
>   
> 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[] = {
> 

[PATCH v4 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.

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 1bd74fd932..3ffa8f51e9 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

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

[Cédric Le Goater]

On 1/11/22 01:55, Daniel Henrique Barboza wrote:
> 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.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Thanks,

C.

> ---
>   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 1bd74fd932..3ffa8f51e9 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 {
> 

[PATCH v4 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.

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 3ffa8f51e9..10f8d6a919 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1487,15 +1487,86 @@ static void pnv_phb4_instance_init(Object *obj)
     object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
 }
 
+static PnvPhb4PecStack *pnv_phb4_get_stack(int chip_id, int index,
+                                           Error **errp)
+{
+    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
+    PnvChip *chip = pnv_get_chip(pnv, chip_id);
+    Pnv9Chip *chip9 = PNV9_CHIP(chip);
+    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(phb->chip_id, phb->phb_id,
+                                        &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;
+        }
+    }
 
     pnv_phb4_XSCOM_init(phb);
 
@@ -1600,7 +1671,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..dfd25831d5 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>
 
@@ -282,6 +283,10 @@ 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);
 
+    if (!defaults_enabled()) {
+        return;
+    }
+
     stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
 
     object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
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 v4 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Cédric Le Goater]

On 1/11/22 01:55, 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.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c     | 75 +++++++++++++++++++++++++++++++++++++-
>   hw/pci-host/pnv_phb4_pec.c |  5 +++
>   hw/ppc/pnv.c               |  2 +
>   3 files changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 3ffa8f51e9..10f8d6a919 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1487,15 +1487,86 @@ static void pnv_phb4_instance_init(Object *obj)
>       object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
>   }
>   
> +static PnvPhb4PecStack *pnv_phb4_get_stack(int chip_id, int index,
> +                                           Error **errp)
> +{
> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +    PnvChip *chip = pnv_get_chip(pnv, chip_id);
> +    Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +    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);

Arg. my comment on the pnv_phb4_get_stack() prototype was clearly
a mistake ! We are calling twice pnv_get_chip() to get the chip
and there is no reason for it.

Can you changed it back ? Sorry about that ... You can flame me.

> +        PnvPhb4PecClass *pecc;
> +        BusState *s;
> +
> +        if (!chip) {
> +            error_setg(errp, "invalid chip id: %d", phb->chip_id);
> +            return;
> +        }
> +
> +        phb->stack = pnv_phb4_get_stack(phb->chip_id, phb->phb_id,
> +                                        &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);

Yes :/ This version is a constant and should probably be a class attribute.

I am not sure we need a property anymore. we could change pnv_phb4_reg_read()
with :
    
    switch (off) {
    case PHB_VERSION:
         return PNV_PHB4_PEC_GET_CLASS(phb->stack->pec)->version;


Another small problem to handle is the root port type :

     /* Add a single Root port if running with defaults */
     if (defaults_enabled()) {
         pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
                                  TYPE_PNV_PHB4_ROOT_PORT);
     }


With PHB5, we should use another type. This can come later.

> +
> +        /*
> +         * 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;
> +        }
> +    }
>   
>       pnv_phb4_XSCOM_init(phb);
>   
> @@ -1600,7 +1671,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..dfd25831d5 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>
>   
> @@ -282,6 +283,10 @@ 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);
>   
> +    if (!defaults_enabled()) {> +        return;

May be move all the code doing the PHB4 device realize in a helper routine
and call it under if (defaults_enabled()) { ... }

Thanks,

C.


> +    }
> +
>       stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
>
>       object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
> 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 v4 4/5] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

On 1/11/22 06:56, Cédric Le Goater wrote:
> On 1/11/22 01:55, 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.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb4.c     | 75 +++++++++++++++++++++++++++++++++++++-
>>   hw/pci-host/pnv_phb4_pec.c |  5 +++
>>   hw/ppc/pnv.c               |  2 +
>>   3 files changed, 80 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 3ffa8f51e9..10f8d6a919 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1487,15 +1487,86 @@ static void pnv_phb4_instance_init(Object *obj)
>>       object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
>>   }
>> +static PnvPhb4PecStack *pnv_phb4_get_stack(int chip_id, int index,
>> +                                           Error **errp)
>> +{
>> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>> +    PnvChip *chip = pnv_get_chip(pnv, chip_id);
>> +    Pnv9Chip *chip9 = PNV9_CHIP(chip);
>> +    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);
> 
> Arg. my comment on the pnv_phb4_get_stack() prototype was clearly
> a mistake ! We are calling twice pnv_get_chip() to get the chip
> and there is no reason for it.
> 
> Can you changed it back ? Sorry about that ... You can flame m
hehehe don't worry about. I'll change it back.

I'll also take the opportunity to test the changes proposed in patch 2 in my env
to make sure we're good there as well.


Thanks,


Daniel


> 
>> +        PnvPhb4PecClass *pecc;
>> +        BusState *s;
>> +
>> +        if (!chip) {
>> +            error_setg(errp, "invalid chip id: %d", phb->chip_id);
>> +            return;
>> +        }
>> +
>> +        phb->stack = pnv_phb4_get_stack(phb->chip_id, phb->phb_id,
>> +                                        &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);
> 
> Yes :/ This version is a constant and should probably be a class attribute.
> 
> I am not sure we need a property anymore. we could change pnv_phb4_reg_read()
> with :
>     switch (off) {
>     case PHB_VERSION:
>          return PNV_PHB4_PEC_GET_CLASS(phb->stack->pec)->version;
> 
> 
> Another small problem to handle is the root port type :
> 
>      /* Add a single Root port if running with defaults */
>      if (defaults_enabled()) {
>          pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
>                                   TYPE_PNV_PHB4_ROOT_PORT);
>      }
> 
> 
> With PHB5, we should use another type. This can come later.
> 
>> +
>> +        /*
>> +         * 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;
>> +        }
>> +    }
>>       pnv_phb4_XSCOM_init(phb);
>> @@ -1600,7 +1671,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..dfd25831d5 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>
>> @@ -282,6 +283,10 @@ 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);
>> +    if (!defaults_enabled()) {> +        return;
> 
> May be move all the code doing the PHB4 device realize in a helper routine
> and call it under if (defaults_enabled()) { ... }
> 
> Thanks,
> 
> C.
> 
> 
>> +    }
>> +
>>       stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
>>
>>       object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
>> 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)
>>
> 

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

[Daniel Henrique Barboza]

Its only callers are inside pnv_phb4.c.

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 10f8d6a919..34c43bd0f5 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;
@@ -1797,32 +1823,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 v4 5/5] ppc/pnv: turn pnv_phb4_update_regions() into static

[Cédric Le Goater]

On 1/11/22 01:55, Daniel Henrique Barboza wrote:
> Its only callers are inside pnv_phb4.c.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Thanks,

C.

> ---
>   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 10f8d6a919..34c43bd0f5 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;
> @@ -1797,32 +1823,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;
>   
>