[PATCH 00/17] remove PnvPhb4PecStack from Powernv9

[PATCH 00/17] remove PnvPhb4PecStack from Powernv9

[Daniel Henrique Barboza]

Hi,

After all the done enabling pnv-phb4 user devices, it became clear that
the stack object is just a container of the PHB and its resources than
something that needs to be maintained by its own. Removing the
PnvPhb4PecStack object promotes a simpler code where we're dealing only
with PECs and PHB4s.

One thing that isn't handled in this series is the nested regs names.
There are 30+ nested per-stack registers with names such as
'PEC_NEST_STK*' or 'PEC_PCI_STK*' that are left as is. Renaming them to
remove the 'STK' reference can be done in a follow up when we're
satisfied with what it is presented here.

No functional change is intended with this series. The series is based
on top of current master (at f8d75e10d3),

Daniel Henrique Barboza (17):
  ppc/pnv: use PHB4 obj in pnv_pec_stk_pci_xscom_ops
  ppc/pnv: move PCI registers to PnvPHB4
  ppc/pnv: move phbbar to PnvPHB4
  ppc/pnv: move intbar to PnvPHB4
  ppc/pnv: change pnv_phb4_update_regions() to use PnvPHB4
  ppc/pnv: move mmbar0/mmbar1 and friends to PnvPHB4
  ppc/pnv: move nest_regs[] to PnvPHB4
  ppc/pnv: change pnv_pec_stk_update_map() to use PnvPHB4
  ppc/pnv: move nest_regs_mr to PnvPHB4
  ppc/pnv: move phb_regs_mr to PnvPHB4
  ppc/pnv: introduce PnvPHB4 'phb_number' property
  ppc/pnv: introduce PnvPHB4 'pec' property
  ppc/pnv: remove stack pointer from PnvPHB4
  ppc/pnv: move default_phb_realize() to pec_realize()
  ppc/pnv: convert pec->stacks[] into pec->phbs[]
  ppc/pnv: remove PnvPhb4PecStack object
  ppc/pnv: rename pnv_pec_stk_update_map()

 hw/pci-host/pnv_phb4.c         | 271 ++++++++++++++++-----------------
 hw/pci-host/pnv_phb4_pec.c     | 122 ++++-----------
 include/hw/pci-host/pnv_phb4.h |  84 +++++-----
 3 files changed, 200 insertions(+), 277 deletions(-)

-- 
2.33.1

[PATCH 01/17] ppc/pnv: use PHB4 obj in pnv_pec_stk_pci_xscom_ops

[Daniel Henrique Barboza]

The current relationship between PnvPhb4PecStack and PnvPHB4 objects is
overly complex. Recent work done in pnv_phb4.c and pnv_phb4_pec.c shows
that the stack obj role in the overall design is more of a placeholder for
its 'phb' object, having no atributes that stand on its own. This became
clearer after pnv-phb4 user creatable devices were implemented.

What remains now are a lot of stack->phb and phb->stack pointers
throughout .read and .write callbacks of MemoryRegionOps that are being
initialized in phb4_realize() time. stk_realize() is a no-op if the
machine is being run with -nodefaults.

The first step of trying to decouple the stack and phb relationship is
to move the MemoryRegionOps that belongs to PnvPhb4PecStack to PhbPHB4.
Unfortunately this can't be done  without some preliminary steps to
change the usage of 'stack' and replace it with 'phb' in these
read/write callbacks.

This patch starts this process by using a PnvPHB4 opaque in
pnv_pec_stk_pci_xscom_ops instead of PnvPhb4PecStack.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index a7b638831e..e010572376 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1071,7 +1071,7 @@ static const MemoryRegionOps pnv_pec_stk_nest_xscom_ops = {
 static uint64_t pnv_pec_stk_pci_xscom_read(void *opaque, hwaddr addr,
                                            unsigned size)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    PnvPhb4PecStack *stack = PNV_PHB4(opaque)->stack;
     uint32_t reg = addr >> 3;
 
     /* TODO: add list of allowed registers and error out if not */
@@ -1081,7 +1081,7 @@ static uint64_t pnv_pec_stk_pci_xscom_read(void *opaque, hwaddr addr,
 static void pnv_pec_stk_pci_xscom_write(void *opaque, hwaddr addr,
                                         uint64_t val, unsigned size)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    PnvPhb4PecStack *stack = PNV_PHB4(opaque)->stack;
     uint32_t reg = addr >> 3;
 
     switch (reg) {
@@ -1475,10 +1475,10 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
                           &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",
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%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,
+    pnv_xscom_region_init(&stack->pci_regs_mr, OBJECT(phb),
+                          &pnv_pec_stk_pci_xscom_ops, phb, name,
                           PHB4_PEC_PCI_STK_REGS_COUNT);
 
     /* PHB pass-through */
-- 
2.33.1

Re: [PATCH 01/17] ppc/pnv: use PHB4 obj in pnv_pec_stk_pci_xscom_ops

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> The current relationship between PnvPhb4PecStack and PnvPHB4 objects is
> overly complex. Recent work done in pnv_phb4.c and pnv_phb4_pec.c shows
> that the stack obj role in the overall design is more of a placeholder for
> its 'phb' object, having no atributes that stand on its own. This became
> clearer after pnv-phb4 user creatable devices were implemented.
> 
> What remains now are a lot of stack->phb and phb->stack pointers
> throughout .read and .write callbacks of MemoryRegionOps that are being
> initialized in phb4_realize() time. stk_realize() is a no-op if the
> machine is being run with -nodefaults.
> 
> The first step of trying to decouple the stack and phb relationship is
> to move the MemoryRegionOps that belongs to PnvPhb4PecStack to PhbPHB4.
> Unfortunately this can't be done  without some preliminary steps to
> change the usage of 'stack' and replace it with 'phb' in these
> read/write callbacks.
> 
> This patch starts this process by using a PnvPHB4 opaque in
> pnv_pec_stk_pci_xscom_ops instead of PnvPhb4PecStack.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>


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

Thanks,

C.

[PATCH 02/17] ppc/pnv: move PCI registers to PnvPHB4

[Daniel Henrique Barboza]

Previous patch changed pnv_pec_stk_pci_xscom_read() and
pnv_pec_stk_pci_xscom_write() to use a PnvPHB4 opaque, making it easier
to move both pci_regs[] and the pci_regs_mr MemoryRegion to the PnvHB4
object.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 30 +++++++++++++++---------------
 include/hw/pci-host/pnv_phb4.h | 10 +++++-----
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index e010572376..fd9f6af4b3 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1071,54 +1071,54 @@ static const MemoryRegionOps pnv_pec_stk_nest_xscom_ops = {
 static uint64_t pnv_pec_stk_pci_xscom_read(void *opaque, hwaddr addr,
                                            unsigned size)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4(opaque)->stack;
+    PnvPHB4 *phb = PNV_PHB4(opaque);
     uint32_t reg = addr >> 3;
 
     /* TODO: add list of allowed registers and error out if not */
-    return stack->pci_regs[reg];
+    return phb->pci_regs[reg];
 }
 
 static void pnv_pec_stk_pci_xscom_write(void *opaque, hwaddr addr,
                                         uint64_t val, unsigned size)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4(opaque)->stack;
+    PnvPHB4 *phb = PNV_PHB4(opaque);
     uint32_t reg = addr >> 3;
 
     switch (reg) {
     case PEC_PCI_STK_PCI_FIR:
-        stack->pci_regs[reg] = val;
+        phb->pci_regs[reg] = val;
         break;
     case PEC_PCI_STK_PCI_FIR_CLR:
-        stack->pci_regs[PEC_PCI_STK_PCI_FIR] &= val;
+        phb->pci_regs[PEC_PCI_STK_PCI_FIR] &= val;
         break;
     case PEC_PCI_STK_PCI_FIR_SET:
-        stack->pci_regs[PEC_PCI_STK_PCI_FIR] |= val;
+        phb->pci_regs[PEC_PCI_STK_PCI_FIR] |= val;
         break;
     case PEC_PCI_STK_PCI_FIR_MSK:
-        stack->pci_regs[reg] = val;
+        phb->pci_regs[reg] = val;
         break;
     case PEC_PCI_STK_PCI_FIR_MSKC:
-        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
+        phb->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
         break;
     case PEC_PCI_STK_PCI_FIR_MSKS:
-        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
+        phb->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
         break;
     case PEC_PCI_STK_PCI_FIR_ACT0:
     case PEC_PCI_STK_PCI_FIR_ACT1:
-        stack->pci_regs[reg] = val;
+        phb->pci_regs[reg] = val;
         break;
     case PEC_PCI_STK_PCI_FIR_WOF:
-        stack->pci_regs[reg] = 0;
+        phb->pci_regs[reg] = 0;
         break;
     case PEC_PCI_STK_ETU_RESET:
-        stack->pci_regs[reg] = val & 0x8000000000000000ull;
+        phb->pci_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->pci_regs[reg] = val;
+        phb->pci_regs[reg] = val;
         break;
     default:
         qemu_log_mask(LOG_UNIMP, "phb4_pec_stk: pci_xscom_write 0x%"HWADDR_PRIx
@@ -1477,7 +1477,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
 
     snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
              pec->chip_id, pec->index, stack->stack_no);
-    pnv_xscom_region_init(&stack->pci_regs_mr, OBJECT(phb),
+    pnv_xscom_region_init(&phb->pci_regs_mr, OBJECT(phb),
                           &pnv_pec_stk_pci_xscom_ops, phb, name,
                           PHB4_PEC_PCI_STK_REGS_COUNT);
 
@@ -1496,7 +1496,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
                             &stack->nest_regs_mr);
     pnv_xscom_add_subregion(pec->chip,
                             pec_pci_base + 0x40 * (stack->stack_no + 1),
-                            &stack->pci_regs_mr);
+                            &phb->pci_regs_mr);
     pnv_xscom_add_subregion(pec->chip,
                             pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
                             0x40 * stack->stack_no,
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 4b7ce8a723..4487c3a6e2 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -107,6 +107,11 @@ struct PnvPHB4 {
     MemoryRegion pci_mmio;
     MemoryRegion pci_io;
 
+    /* PCI registers (excluding pass-through) */
+#define PHB4_PEC_PCI_STK_REGS_COUNT  0xf
+    uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
+    MemoryRegion pci_regs_mr;
+
     /* On-chip IODA tables */
     uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
     uint64_t ioda_MIST[PNV_PHB4_MAX_MIST];
@@ -155,11 +160,6 @@ struct PnvPhb4PecStack {
     uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
     MemoryRegion nest_regs_mr;
 
-    /* PCI registers (excluding pass-through) */
-#define PHB4_PEC_PCI_STK_REGS_COUNT  0xf
-    uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
-    MemoryRegion pci_regs_mr;
-
     /* PHB pass-through XSCOM */
     MemoryRegion phb_regs_mr;
 
-- 
2.33.1

Re: [PATCH 02/17] ppc/pnv: move PCI registers to PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> Previous patch changed pnv_pec_stk_pci_xscom_read() and
> pnv_pec_stk_pci_xscom_write() to use a PnvPHB4 opaque, making it easier
> to move both pci_regs[] and the pci_regs_mr MemoryRegion to the PnvHB4
> object.
> 
> 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         | 30 +++++++++++++++---------------
>   include/hw/pci-host/pnv_phb4.h | 10 +++++-----
>   2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index e010572376..fd9f6af4b3 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1071,54 +1071,54 @@ static const MemoryRegionOps pnv_pec_stk_nest_xscom_ops = {
>   static uint64_t pnv_pec_stk_pci_xscom_read(void *opaque, hwaddr addr,
>                                              unsigned size)
>   {
> -    PnvPhb4PecStack *stack = PNV_PHB4(opaque)->stack;
> +    PnvPHB4 *phb = PNV_PHB4(opaque);
>       uint32_t reg = addr >> 3;
>   
>       /* TODO: add list of allowed registers and error out if not */
> -    return stack->pci_regs[reg];
> +    return phb->pci_regs[reg];
>   }
>   
>   static void pnv_pec_stk_pci_xscom_write(void *opaque, hwaddr addr,
>                                           uint64_t val, unsigned size)
>   {
> -    PnvPhb4PecStack *stack = PNV_PHB4(opaque)->stack;
> +    PnvPHB4 *phb = PNV_PHB4(opaque);
>       uint32_t reg = addr >> 3;
>   
>       switch (reg) {
>       case PEC_PCI_STK_PCI_FIR:
> -        stack->pci_regs[reg] = val;
> +        phb->pci_regs[reg] = val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_CLR:
> -        stack->pci_regs[PEC_PCI_STK_PCI_FIR] &= val;
> +        phb->pci_regs[PEC_PCI_STK_PCI_FIR] &= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_SET:
> -        stack->pci_regs[PEC_PCI_STK_PCI_FIR] |= val;
> +        phb->pci_regs[PEC_PCI_STK_PCI_FIR] |= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_MSK:
> -        stack->pci_regs[reg] = val;
> +        phb->pci_regs[reg] = val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_MSKC:
> -        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
> +        phb->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_MSKS:
> -        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
> +        phb->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_ACT0:
>       case PEC_PCI_STK_PCI_FIR_ACT1:
> -        stack->pci_regs[reg] = val;
> +        phb->pci_regs[reg] = val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_WOF:
> -        stack->pci_regs[reg] = 0;
> +        phb->pci_regs[reg] = 0;
>           break;
>       case PEC_PCI_STK_ETU_RESET:
> -        stack->pci_regs[reg] = val & 0x8000000000000000ull;
> +        phb->pci_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->pci_regs[reg] = val;
> +        phb->pci_regs[reg] = val;
>           break;
>       default:
>           qemu_log_mask(LOG_UNIMP, "phb4_pec_stk: pci_xscom_write 0x%"HWADDR_PRIx
> @@ -1477,7 +1477,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>   
>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
>                pec->chip_id, pec->index, stack->stack_no);
> -    pnv_xscom_region_init(&stack->pci_regs_mr, OBJECT(phb),
> +    pnv_xscom_region_init(&phb->pci_regs_mr, OBJECT(phb),
>                             &pnv_pec_stk_pci_xscom_ops, phb, name,
>                             PHB4_PEC_PCI_STK_REGS_COUNT);
>   
> @@ -1496,7 +1496,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>                               &stack->nest_regs_mr);
>       pnv_xscom_add_subregion(pec->chip,
>                               pec_pci_base + 0x40 * (stack->stack_no + 1),
> -                            &stack->pci_regs_mr);
> +                            &phb->pci_regs_mr);
>       pnv_xscom_add_subregion(pec->chip,
>                               pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>                               0x40 * stack->stack_no,
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 4b7ce8a723..4487c3a6e2 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -107,6 +107,11 @@ struct PnvPHB4 {
>       MemoryRegion pci_mmio;
>       MemoryRegion pci_io;
>   
> +    /* PCI registers (excluding pass-through) */
> +#define PHB4_PEC_PCI_STK_REGS_COUNT  0xf
> +    uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
> +    MemoryRegion pci_regs_mr;
> +
>       /* On-chip IODA tables */
>       uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
>       uint64_t ioda_MIST[PNV_PHB4_MAX_MIST];
> @@ -155,11 +160,6 @@ struct PnvPhb4PecStack {
>       uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
>       MemoryRegion nest_regs_mr;
>   
> -    /* PCI registers (excluding pass-through) */
> -#define PHB4_PEC_PCI_STK_REGS_COUNT  0xf
> -    uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
> -    MemoryRegion pci_regs_mr;
> -
>       /* PHB pass-through XSCOM */
>       MemoryRegion phb_regs_mr;
>   
> 

[PATCH 03/17] ppc/pnv: move phbbar to PnvPHB4

[Daniel Henrique Barboza]

This MemoryRegion is simple enough to be moved in a single step.

A 'stack->phb' pointer had to be introduced in pnv_pec_stk_update_map()
because this function isn't ready to be fully converted to use a PnvPHB4
pointer instead. This will be dealt with in the following patches.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 19 ++++++++++---------
 include/hw/pci-host/pnv_phb4.h |  4 +++-
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index fd9f6af4b3..00eaf91fca 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -874,15 +874,15 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
 
     /* Unmap first always */
     if (memory_region_is_mapped(&phb->mr_regs)) {
-        memory_region_del_subregion(&stack->phbbar, &phb->mr_regs);
+        memory_region_del_subregion(&phb->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);
+    if (memory_region_is_mapped(&phb->phbbar)) {
+        memory_region_add_subregion(&phb->phbbar, 0, &phb->mr_regs);
     }
 
     /* Map ESB if enabled */
@@ -897,6 +897,7 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
 static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
 {
     PnvPhb4PecState *pec = stack->pec;
+    PnvPHB4 *phb = stack->phb;
     MemoryRegion *sysmem = get_system_memory();
     uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
     uint64_t bar, mask, size;
@@ -919,9 +920,9 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
         !(bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
         memory_region_del_subregion(sysmem, &stack->mmbar1);
     }
-    if (memory_region_is_mapped(&stack->phbbar) &&
+    if (memory_region_is_mapped(&phb->phbbar) &&
         !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
-        memory_region_del_subregion(sysmem, &stack->phbbar);
+        memory_region_del_subregion(sysmem, &phb->phbbar);
     }
     if (memory_region_is_mapped(&stack->intbar) &&
         !(bar_en & PEC_NEST_STK_BAR_EN_INT)) {
@@ -956,14 +957,14 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
         stack->mmio1_base = bar;
         stack->mmio1_size = size;
     }
-    if (!memory_region_is_mapped(&stack->phbbar) &&
+    if (!memory_region_is_mapped(&phb->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",
+        snprintf(name, sizeof(name), "pec-%d.%d-phb-%d",
                  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);
+        memory_region_init(&phb->phbbar, OBJECT(phb), name, size);
+        memory_region_add_subregion(sysmem, bar, &phb->phbbar);
     }
     if (!memory_region_is_mapped(&stack->intbar) &&
         (bar_en & PEC_NEST_STK_BAR_EN_INT)) {
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 4487c3a6e2..b11fa80e81 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -112,6 +112,9 @@ struct PnvPHB4 {
     uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
     MemoryRegion pci_regs_mr;
 
+    /* Memory windows from PowerBus to PHB */
+    MemoryRegion phbbar;
+
     /* On-chip IODA tables */
     uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
     uint64_t ioda_MIST[PNV_PHB4_MAX_MIST];
@@ -166,7 +169,6 @@ struct PnvPhb4PecStack {
     /* Memory windows from PowerBus to PHB */
     MemoryRegion mmbar0;
     MemoryRegion mmbar1;
-    MemoryRegion phbbar;
     MemoryRegion intbar;
     uint64_t mmio0_base;
     uint64_t mmio0_size;
-- 
2.33.1

Re: [PATCH 03/17] ppc/pnv: move phbbar to PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> This MemoryRegion is simple enough to be moved in a single step.
> 
> A 'stack->phb' pointer had to be introduced in pnv_pec_stk_update_map()
> because this function isn't ready to be fully converted to use a PnvPHB4
> pointer instead. This will be dealt with in the following patches.
> 
> 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         | 19 ++++++++++---------
>   include/hw/pci-host/pnv_phb4.h |  4 +++-
>   2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index fd9f6af4b3..00eaf91fca 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -874,15 +874,15 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
>   
>       /* Unmap first always */
>       if (memory_region_is_mapped(&phb->mr_regs)) {
> -        memory_region_del_subregion(&stack->phbbar, &phb->mr_regs);
> +        memory_region_del_subregion(&phb->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);
> +    if (memory_region_is_mapped(&phb->phbbar)) {
> +        memory_region_add_subregion(&phb->phbbar, 0, &phb->mr_regs);
>       }
>   
>       /* Map ESB if enabled */
> @@ -897,6 +897,7 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
>   static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>   {
>       PnvPhb4PecState *pec = stack->pec;
> +    PnvPHB4 *phb = stack->phb;
>       MemoryRegion *sysmem = get_system_memory();
>       uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
>       uint64_t bar, mask, size;
> @@ -919,9 +920,9 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>           !(bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
>           memory_region_del_subregion(sysmem, &stack->mmbar1);
>       }
> -    if (memory_region_is_mapped(&stack->phbbar) &&
> +    if (memory_region_is_mapped(&phb->phbbar) &&
>           !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
> -        memory_region_del_subregion(sysmem, &stack->phbbar);
> +        memory_region_del_subregion(sysmem, &phb->phbbar);
>       }
>       if (memory_region_is_mapped(&stack->intbar) &&
>           !(bar_en & PEC_NEST_STK_BAR_EN_INT)) {
> @@ -956,14 +957,14 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>           stack->mmio1_base = bar;
>           stack->mmio1_size = size;
>       }
> -    if (!memory_region_is_mapped(&stack->phbbar) &&
> +    if (!memory_region_is_mapped(&phb->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",
> +        snprintf(name, sizeof(name), "pec-%d.%d-phb-%d",
>                    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);
> +        memory_region_init(&phb->phbbar, OBJECT(phb), name, size);
> +        memory_region_add_subregion(sysmem, bar, &phb->phbbar);
>       }
>       if (!memory_region_is_mapped(&stack->intbar) &&
>           (bar_en & PEC_NEST_STK_BAR_EN_INT)) {
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 4487c3a6e2..b11fa80e81 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -112,6 +112,9 @@ struct PnvPHB4 {
>       uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
>       MemoryRegion pci_regs_mr;
>   
> +    /* Memory windows from PowerBus to PHB */
> +    MemoryRegion phbbar;
> +
>       /* On-chip IODA tables */
>       uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
>       uint64_t ioda_MIST[PNV_PHB4_MAX_MIST];
> @@ -166,7 +169,6 @@ struct PnvPhb4PecStack {
>       /* Memory windows from PowerBus to PHB */
>       MemoryRegion mmbar0;
>       MemoryRegion mmbar1;
> -    MemoryRegion phbbar;
>       MemoryRegion intbar;
>       uint64_t mmio0_base;
>       uint64_t mmio0_size;
> 

[PATCH 04/17] ppc/pnv: move intbar to PnvPHB4

[Daniel Henrique Barboza]

This MemoryRegion can also be moved in a single step.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 18 +++++++++---------
 include/hw/pci-host/pnv_phb4.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 00eaf91fca..fbc475f27a 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -877,7 +877,7 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
         memory_region_del_subregion(&phb->phbbar, &phb->mr_regs);
     }
     if (memory_region_is_mapped(&phb->xsrc.esb_mmio)) {
-        memory_region_del_subregion(&stack->intbar, &phb->xsrc.esb_mmio);
+        memory_region_del_subregion(&phb->intbar, &phb->xsrc.esb_mmio);
     }
 
     /* Map registers if enabled */
@@ -886,8 +886,8 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
     }
 
     /* Map ESB if enabled */
-    if (memory_region_is_mapped(&stack->intbar)) {
-        memory_region_add_subregion(&stack->intbar, 0, &phb->xsrc.esb_mmio);
+    if (memory_region_is_mapped(&phb->intbar)) {
+        memory_region_add_subregion(&phb->intbar, 0, &phb->xsrc.esb_mmio);
     }
 
     /* Check/update m32 */
@@ -924,9 +924,9 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
         !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
         memory_region_del_subregion(sysmem, &phb->phbbar);
     }
-    if (memory_region_is_mapped(&stack->intbar) &&
+    if (memory_region_is_mapped(&phb->intbar) &&
         !(bar_en & PEC_NEST_STK_BAR_EN_INT)) {
-        memory_region_del_subregion(sysmem, &stack->intbar);
+        memory_region_del_subregion(sysmem, &phb->intbar);
     }
 
     /* Update PHB */
@@ -966,14 +966,14 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
         memory_region_init(&phb->phbbar, OBJECT(phb), name, size);
         memory_region_add_subregion(sysmem, bar, &phb->phbbar);
     }
-    if (!memory_region_is_mapped(&stack->intbar) &&
+    if (!memory_region_is_mapped(&phb->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",
+        snprintf(name, sizeof(name), "pec-%d.%d-phb-%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);
+        memory_region_init(&phb->intbar, OBJECT(phb), name, size);
+        memory_region_add_subregion(sysmem, bar, &phb->intbar);
     }
 
     /* Update PHB */
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index b11fa80e81..cf5dd4009c 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -114,6 +114,7 @@ struct PnvPHB4 {
 
     /* Memory windows from PowerBus to PHB */
     MemoryRegion phbbar;
+    MemoryRegion intbar;
 
     /* On-chip IODA tables */
     uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
@@ -169,7 +170,6 @@ struct PnvPhb4PecStack {
     /* Memory windows from PowerBus to PHB */
     MemoryRegion mmbar0;
     MemoryRegion mmbar1;
-    MemoryRegion intbar;
     uint64_t mmio0_base;
     uint64_t mmio0_size;
     uint64_t mmio1_base;
-- 
2.33.1

Re: [PATCH 04/17] ppc/pnv: move intbar to PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> This MemoryRegion can also be moved in a single step.
> 
> 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         | 18 +++++++++---------
>   include/hw/pci-host/pnv_phb4.h |  2 +-
>   2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 00eaf91fca..fbc475f27a 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -877,7 +877,7 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
>           memory_region_del_subregion(&phb->phbbar, &phb->mr_regs);
>       }
>       if (memory_region_is_mapped(&phb->xsrc.esb_mmio)) {
> -        memory_region_del_subregion(&stack->intbar, &phb->xsrc.esb_mmio);
> +        memory_region_del_subregion(&phb->intbar, &phb->xsrc.esb_mmio);
>       }
>   
>       /* Map registers if enabled */
> @@ -886,8 +886,8 @@ static void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
>       }
>   
>       /* Map ESB if enabled */
> -    if (memory_region_is_mapped(&stack->intbar)) {
> -        memory_region_add_subregion(&stack->intbar, 0, &phb->xsrc.esb_mmio);
> +    if (memory_region_is_mapped(&phb->intbar)) {
> +        memory_region_add_subregion(&phb->intbar, 0, &phb->xsrc.esb_mmio);
>       }
>   
>       /* Check/update m32 */
> @@ -924,9 +924,9 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>           !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
>           memory_region_del_subregion(sysmem, &phb->phbbar);
>       }
> -    if (memory_region_is_mapped(&stack->intbar) &&
> +    if (memory_region_is_mapped(&phb->intbar) &&
>           !(bar_en & PEC_NEST_STK_BAR_EN_INT)) {
> -        memory_region_del_subregion(sysmem, &stack->intbar);
> +        memory_region_del_subregion(sysmem, &phb->intbar);
>       }
>   
>       /* Update PHB */
> @@ -966,14 +966,14 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>           memory_region_init(&phb->phbbar, OBJECT(phb), name, size);
>           memory_region_add_subregion(sysmem, bar, &phb->phbbar);
>       }
> -    if (!memory_region_is_mapped(&stack->intbar) &&
> +    if (!memory_region_is_mapped(&phb->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",
> +        snprintf(name, sizeof(name), "pec-%d.%d-phb-%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);
> +        memory_region_init(&phb->intbar, OBJECT(phb), name, size);
> +        memory_region_add_subregion(sysmem, bar, &phb->intbar);
>       }
>   
>       /* Update PHB */
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index b11fa80e81..cf5dd4009c 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -114,6 +114,7 @@ struct PnvPHB4 {
>   
>       /* Memory windows from PowerBus to PHB */
>       MemoryRegion phbbar;
> +    MemoryRegion intbar;
>   
>       /* On-chip IODA tables */
>       uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
> @@ -169,7 +170,6 @@ struct PnvPhb4PecStack {
>       /* Memory windows from PowerBus to PHB */
>       MemoryRegion mmbar0;
>       MemoryRegion mmbar1;
> -    MemoryRegion intbar;
>       uint64_t mmio0_base;
>       uint64_t mmio0_size;
>       uint64_t mmio1_base;
> 

[PATCH 05/17] ppc/pnv: change pnv_phb4_update_regions() to use PnvPHB4

[Daniel Henrique Barboza]

The function does not rely on stack for anything it does anymore. This
is also one less instance of 'stack->phb' that we need to worry about.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index fbc475f27a..034721f159 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -868,10 +868,8 @@ 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)
+static void pnv_phb4_update_regions(PnvPHB4 *phb)
 {
-    PnvPHB4 *phb = stack->phb;
-
     /* Unmap first always */
     if (memory_region_is_mapped(&phb->mr_regs)) {
         memory_region_del_subregion(&phb->phbbar, &phb->mr_regs);
@@ -930,7 +928,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     }
 
     /* Update PHB */
-    pnv_phb4_update_regions(stack);
+    pnv_phb4_update_regions(phb);
 
     /* Handle maps */
     if (!memory_region_is_mapped(&stack->mmbar0) &&
@@ -977,7 +975,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     }
 
     /* Update PHB */
-    pnv_phb4_update_regions(stack);
+    pnv_phb4_update_regions(phb);
 }
 
 static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
-- 
2.33.1

Re: [PATCH 05/17] ppc/pnv: change pnv_phb4_update_regions() to use PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> The function does not rely on stack for anything it does anymore. This
> is also one less instance of 'stack->phb' that we need to worry about.
> 
> 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 | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index fbc475f27a..034721f159 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -868,10 +868,8 @@ 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)
> +static void pnv_phb4_update_regions(PnvPHB4 *phb)
>   {
> -    PnvPHB4 *phb = stack->phb;
> -
>       /* Unmap first always */
>       if (memory_region_is_mapped(&phb->mr_regs)) {
>           memory_region_del_subregion(&phb->phbbar, &phb->mr_regs);
> @@ -930,7 +928,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       }
>   
>       /* Update PHB */
> -    pnv_phb4_update_regions(stack);
> +    pnv_phb4_update_regions(phb);
>   
>       /* Handle maps */
>       if (!memory_region_is_mapped(&stack->mmbar0) &&
> @@ -977,7 +975,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       }
>   
>       /* Update PHB */
> -    pnv_phb4_update_regions(stack);
> +    pnv_phb4_update_regions(phb);
>   }
>   
>   static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
> 

[PATCH 06/17] ppc/pnv: move mmbar0/mmbar1 and friends to PnvPHB4

[Daniel Henrique Barboza]

These 2 MemoryRegions, together with mmio(0|1)_base and mmio(0|1)_size
variables, are used together in the same functions. We're better of
moving them all in a single step.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 52 +++++++++++++++++-----------------
 include/hw/pci-host/pnv_phb4.h | 14 ++++-----
 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 034721f159..dc4db091e4 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -228,16 +228,16 @@ static void pnv_phb4_check_mbt(PnvPHB4 *phb, uint32_t index)
     /* TODO: Figure out how to implemet/decode AOMASK */
 
     /* Check if it matches an enabled MMIO region in the PEC stack */
-    if (memory_region_is_mapped(&phb->stack->mmbar0) &&
-        base >= phb->stack->mmio0_base &&
-        (base + size) <= (phb->stack->mmio0_base + phb->stack->mmio0_size)) {
-        parent = &phb->stack->mmbar0;
-        base -= phb->stack->mmio0_base;
-    } else if (memory_region_is_mapped(&phb->stack->mmbar1) &&
-        base >= phb->stack->mmio1_base &&
-        (base + size) <= (phb->stack->mmio1_base + phb->stack->mmio1_size)) {
-        parent = &phb->stack->mmbar1;
-        base -= phb->stack->mmio1_base;
+    if (memory_region_is_mapped(&phb->mmbar0) &&
+        base >= phb->mmio0_base &&
+        (base + size) <= (phb->mmio0_base + phb->mmio0_size)) {
+        parent = &phb->mmbar0;
+        base -= phb->mmio0_base;
+    } else if (memory_region_is_mapped(&phb->mmbar1) &&
+        base >= phb->mmio1_base &&
+        (base + size) <= (phb->mmio1_base + phb->mmio1_size)) {
+        parent = &phb->mmbar1;
+        base -= phb->mmio1_base;
     } else {
         phb_error(phb, "PHB MBAR %d out of parent bounds", index);
         return;
@@ -910,13 +910,13 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
      */
 
     /* Handle unmaps */
-    if (memory_region_is_mapped(&stack->mmbar0) &&
+    if (memory_region_is_mapped(&phb->mmbar0) &&
         !(bar_en & PEC_NEST_STK_BAR_EN_MMIO0)) {
-        memory_region_del_subregion(sysmem, &stack->mmbar0);
+        memory_region_del_subregion(sysmem, &phb->mmbar0);
     }
-    if (memory_region_is_mapped(&stack->mmbar1) &&
+    if (memory_region_is_mapped(&phb->mmbar1) &&
         !(bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
-        memory_region_del_subregion(sysmem, &stack->mmbar1);
+        memory_region_del_subregion(sysmem, &phb->mmbar1);
     }
     if (memory_region_is_mapped(&phb->phbbar) &&
         !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
@@ -931,29 +931,29 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     pnv_phb4_update_regions(phb);
 
     /* Handle maps */
-    if (!memory_region_is_mapped(&stack->mmbar0) &&
+    if (!memory_region_is_mapped(&phb->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",
+        snprintf(name, sizeof(name), "pec-%d.%d-phb-%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;
+        memory_region_init(&phb->mmbar0, OBJECT(phb), name, size);
+        memory_region_add_subregion(sysmem, bar, &phb->mmbar0);
+        phb->mmio0_base = bar;
+        phb->mmio0_size = size;
     }
-    if (!memory_region_is_mapped(&stack->mmbar1) &&
+    if (!memory_region_is_mapped(&phb->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",
+        snprintf(name, sizeof(name), "pec-%d.%d-phb-%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;
+        memory_region_init(&phb->mmbar1, OBJECT(phb), name, size);
+        memory_region_add_subregion(sysmem, bar, &phb->mmbar1);
+        phb->mmio1_base = bar;
+        phb->mmio1_size = size;
     }
     if (!memory_region_is_mapped(&phb->phbbar) &&
         (bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index cf5dd4009c..4a8f510f6d 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -115,6 +115,12 @@ struct PnvPHB4 {
     /* Memory windows from PowerBus to PHB */
     MemoryRegion phbbar;
     MemoryRegion intbar;
+    MemoryRegion mmbar0;
+    MemoryRegion mmbar1;
+    uint64_t mmio0_base;
+    uint64_t mmio0_size;
+    uint64_t mmio1_base;
+    uint64_t mmio1_size;
 
     /* On-chip IODA tables */
     uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
@@ -167,14 +173,6 @@ struct PnvPhb4PecStack {
     /* PHB pass-through XSCOM */
     MemoryRegion phb_regs_mr;
 
-    /* Memory windows from PowerBus to PHB */
-    MemoryRegion mmbar0;
-    MemoryRegion mmbar1;
-    uint64_t mmio0_base;
-    uint64_t mmio0_size;
-    uint64_t mmio1_base;
-    uint64_t mmio1_size;
-
     /* The owner PEC */
     PnvPhb4PecState *pec;
 
-- 
2.33.1

Re: [PATCH 06/17] ppc/pnv: move mmbar0/mmbar1 and friends to PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> These 2 MemoryRegions, together with mmio(0|1)_base and mmio(0|1)_size
> variables, are used together in the same functions. We're better of
> moving them all in a single step.
> 
> 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 | 14 ++++-----
>   2 files changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 034721f159..dc4db091e4 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -228,16 +228,16 @@ static void pnv_phb4_check_mbt(PnvPHB4 *phb, uint32_t index)
>       /* TODO: Figure out how to implemet/decode AOMASK */
>   
>       /* Check if it matches an enabled MMIO region in the PEC stack */
> -    if (memory_region_is_mapped(&phb->stack->mmbar0) &&
> -        base >= phb->stack->mmio0_base &&
> -        (base + size) <= (phb->stack->mmio0_base + phb->stack->mmio0_size)) {
> -        parent = &phb->stack->mmbar0;
> -        base -= phb->stack->mmio0_base;
> -    } else if (memory_region_is_mapped(&phb->stack->mmbar1) &&
> -        base >= phb->stack->mmio1_base &&
> -        (base + size) <= (phb->stack->mmio1_base + phb->stack->mmio1_size)) {
> -        parent = &phb->stack->mmbar1;
> -        base -= phb->stack->mmio1_base;
> +    if (memory_region_is_mapped(&phb->mmbar0) &&
> +        base >= phb->mmio0_base &&
> +        (base + size) <= (phb->mmio0_base + phb->mmio0_size)) {
> +        parent = &phb->mmbar0;
> +        base -= phb->mmio0_base;
> +    } else if (memory_region_is_mapped(&phb->mmbar1) &&
> +        base >= phb->mmio1_base &&
> +        (base + size) <= (phb->mmio1_base + phb->mmio1_size)) {
> +        parent = &phb->mmbar1;
> +        base -= phb->mmio1_base;
>       } else {
>           phb_error(phb, "PHB MBAR %d out of parent bounds", index);
>           return;
> @@ -910,13 +910,13 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>        */
>   
>       /* Handle unmaps */
> -    if (memory_region_is_mapped(&stack->mmbar0) &&
> +    if (memory_region_is_mapped(&phb->mmbar0) &&
>           !(bar_en & PEC_NEST_STK_BAR_EN_MMIO0)) {
> -        memory_region_del_subregion(sysmem, &stack->mmbar0);
> +        memory_region_del_subregion(sysmem, &phb->mmbar0);
>       }
> -    if (memory_region_is_mapped(&stack->mmbar1) &&
> +    if (memory_region_is_mapped(&phb->mmbar1) &&
>           !(bar_en & PEC_NEST_STK_BAR_EN_MMIO1)) {
> -        memory_region_del_subregion(sysmem, &stack->mmbar1);
> +        memory_region_del_subregion(sysmem, &phb->mmbar1);
>       }
>       if (memory_region_is_mapped(&phb->phbbar) &&
>           !(bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
> @@ -931,29 +931,29 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       pnv_phb4_update_regions(phb);
>   
>       /* Handle maps */
> -    if (!memory_region_is_mapped(&stack->mmbar0) &&
> +    if (!memory_region_is_mapped(&phb->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",
> +        snprintf(name, sizeof(name), "pec-%d.%d-phb-%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;
> +        memory_region_init(&phb->mmbar0, OBJECT(phb), name, size);
> +        memory_region_add_subregion(sysmem, bar, &phb->mmbar0);
> +        phb->mmio0_base = bar;
> +        phb->mmio0_size = size;
>       }
> -    if (!memory_region_is_mapped(&stack->mmbar1) &&
> +    if (!memory_region_is_mapped(&phb->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",
> +        snprintf(name, sizeof(name), "pec-%d.%d-phb-%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;
> +        memory_region_init(&phb->mmbar1, OBJECT(phb), name, size);
> +        memory_region_add_subregion(sysmem, bar, &phb->mmbar1);
> +        phb->mmio1_base = bar;
> +        phb->mmio1_size = size;
>       }
>       if (!memory_region_is_mapped(&phb->phbbar) &&
>           (bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index cf5dd4009c..4a8f510f6d 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -115,6 +115,12 @@ struct PnvPHB4 {
>       /* Memory windows from PowerBus to PHB */
>       MemoryRegion phbbar;
>       MemoryRegion intbar;
> +    MemoryRegion mmbar0;
> +    MemoryRegion mmbar1;
> +    uint64_t mmio0_base;
> +    uint64_t mmio0_size;
> +    uint64_t mmio1_base;
> +    uint64_t mmio1_size;
>   
>       /* On-chip IODA tables */
>       uint64_t ioda_LIST[PNV_PHB4_MAX_LSIs];
> @@ -167,14 +173,6 @@ struct PnvPhb4PecStack {
>       /* PHB pass-through XSCOM */
>       MemoryRegion phb_regs_mr;
>   
> -    /* Memory windows from PowerBus to PHB */
> -    MemoryRegion mmbar0;
> -    MemoryRegion mmbar1;
> -    uint64_t mmio0_base;
> -    uint64_t mmio0_size;
> -    uint64_t mmio1_base;
> -    uint64_t mmio1_size;
> -
>       /* The owner PEC */
>       PnvPhb4PecState *pec;
>   
> 

[PATCH 07/17] ppc/pnv: move nest_regs[] to PnvPHB4

[Daniel Henrique Barboza]

stack->nest_regs[] is used in several XSCOM functions and it's one of
the main culprits of having to deal with stack->phb pointers around the
code.

Sure, we're having to add 2 extra stack->phb pointers to ease
nest_regs[] migration to PnvPHB4. They'll be dealt with shortly.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 52 ++++++++++++++++++----------------
 include/hw/pci-host/pnv_phb4.h |  7 +++--
 2 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index dc4db091e4..916a7a3cf0 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -862,10 +862,11 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
                                             unsigned size)
 {
     PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    PnvPHB4 *phb = stack->phb;
     uint32_t reg = addr >> 3;
 
     /* TODO: add list of allowed registers and error out if not */
-    return stack->nest_regs[reg];
+    return phb->nest_regs[reg];
 }
 
 static void pnv_phb4_update_regions(PnvPHB4 *phb)
@@ -897,7 +898,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     PnvPhb4PecState *pec = stack->pec;
     PnvPHB4 *phb = stack->phb;
     MemoryRegion *sysmem = get_system_memory();
-    uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
+    uint64_t bar_en = phb->nest_regs[PEC_NEST_STK_BAR_EN];
     uint64_t bar, mask, size;
     char name[64];
 
@@ -933,8 +934,8 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     /* Handle maps */
     if (!memory_region_is_mapped(&phb->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];
+        bar = phb->nest_regs[PEC_NEST_STK_MMIO_BAR0] >> 8;
+        mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR0_MASK];
         size = ((~mask) >> 8) + 1;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio0",
                  pec->chip_id, pec->index, stack->stack_no);
@@ -945,8 +946,8 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     }
     if (!memory_region_is_mapped(&phb->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];
+        bar = phb->nest_regs[PEC_NEST_STK_MMIO_BAR1] >> 8;
+        mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR1_MASK];
         size = ((~mask) >> 8) + 1;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio1",
                  pec->chip_id, pec->index, stack->stack_no);
@@ -957,7 +958,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     }
     if (!memory_region_is_mapped(&phb->phbbar) &&
         (bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
-        bar = stack->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
+        bar = phb->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
         size = PNV_PHB4_NUM_REGS << 3;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d",
                  pec->chip_id, pec->index, stack->stack_no);
@@ -966,7 +967,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
     }
     if (!memory_region_is_mapped(&phb->intbar) &&
         (bar_en & PEC_NEST_STK_BAR_EN_INT)) {
-        bar = stack->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
+        bar = phb->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
         size = PNV_PHB4_MAX_INTs << 16;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-int",
                  stack->pec->chip_id, stack->pec->index, stack->stack_no);
@@ -982,34 +983,35 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
                                          uint64_t val, unsigned size)
 {
     PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
+    PnvPHB4 *phb = stack->phb;
     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;
+        phb->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;
+        phb->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;
+        phb->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;
+        phb->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;
+        phb->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;
+        phb->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;
+        phb->nest_regs[reg] = val;
         break;
     case PEC_NEST_STK_PCI_NEST_FIR_WOF:
-        stack->nest_regs[reg] = 0;
+        phb->nest_regs[reg] = 0;
         break;
     case PEC_NEST_STK_ERR_REPORT_0:
     case PEC_NEST_STK_ERR_REPORT_1:
@@ -1017,39 +1019,39 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
         /* Flag error ? */
         break;
     case PEC_NEST_STK_PBCQ_MODE:
-        stack->nest_regs[reg] = val & 0xff00000000000000ull;
+        phb->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] &
+        if (phb->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;
+        phb->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) {
+        if (phb->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;
+        phb->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) {
+        if (phb->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;
+        phb->nest_regs[reg] = val & 0xfffffff000000000ull;
         break;
     case PEC_NEST_STK_BAR_EN:
-        stack->nest_regs[reg] = val & 0xf000000000000000ull;
+        phb->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;
+        phb->nest_regs[reg] = val;
         break;
     default:
         qemu_log_mask(LOG_UNIMP, "phb4_pec: nest_xscom_write 0x%"HWADDR_PRIx
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 4a8f510f6d..a7e08772c1 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -112,6 +112,10 @@ struct PnvPHB4 {
     uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
     MemoryRegion pci_regs_mr;
 
+    /* Nest registers */
+#define PHB4_PEC_NEST_STK_REGS_COUNT  0x17
+    uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
+
     /* Memory windows from PowerBus to PHB */
     MemoryRegion phbbar;
     MemoryRegion intbar;
@@ -165,9 +169,6 @@ struct PnvPhb4PecStack {
     /* My own stack number */
     uint32_t stack_no;
 
-    /* Nest registers */
-#define PHB4_PEC_NEST_STK_REGS_COUNT  0x17
-    uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
     MemoryRegion nest_regs_mr;
 
     /* PHB pass-through XSCOM */
-- 
2.33.1

Re: [PATCH 07/17] ppc/pnv: move nest_regs[] to PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> stack->nest_regs[] is used in several XSCOM functions and it's one of
> the main culprits of having to deal with stack->phb pointers around the
> code.
> 
> Sure, we're having to add 2 extra stack->phb pointers to ease
> nest_regs[] migration to PnvPHB4. They'll be dealt with shortly.
> 
> 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 |  7 +++--
>   2 files changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index dc4db091e4..916a7a3cf0 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -862,10 +862,11 @@ static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
>                                               unsigned size)
>   {
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
> +    PnvPHB4 *phb = stack->phb;
>       uint32_t reg = addr >> 3;
>   
>       /* TODO: add list of allowed registers and error out if not */
> -    return stack->nest_regs[reg];
> +    return phb->nest_regs[reg];
>   }
>   
>   static void pnv_phb4_update_regions(PnvPHB4 *phb)
> @@ -897,7 +898,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       PnvPhb4PecState *pec = stack->pec;
>       PnvPHB4 *phb = stack->phb;
>       MemoryRegion *sysmem = get_system_memory();
> -    uint64_t bar_en = stack->nest_regs[PEC_NEST_STK_BAR_EN];
> +    uint64_t bar_en = phb->nest_regs[PEC_NEST_STK_BAR_EN];
>       uint64_t bar, mask, size;
>       char name[64];
>   
> @@ -933,8 +934,8 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       /* Handle maps */
>       if (!memory_region_is_mapped(&phb->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];
> +        bar = phb->nest_regs[PEC_NEST_STK_MMIO_BAR0] >> 8;
> +        mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR0_MASK];
>           size = ((~mask) >> 8) + 1;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio0",
>                    pec->chip_id, pec->index, stack->stack_no);
> @@ -945,8 +946,8 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       }
>       if (!memory_region_is_mapped(&phb->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];
> +        bar = phb->nest_regs[PEC_NEST_STK_MMIO_BAR1] >> 8;
> +        mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR1_MASK];
>           size = ((~mask) >> 8) + 1;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio1",
>                    pec->chip_id, pec->index, stack->stack_no);
> @@ -957,7 +958,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       }
>       if (!memory_region_is_mapped(&phb->phbbar) &&
>           (bar_en & PEC_NEST_STK_BAR_EN_PHB)) {
> -        bar = stack->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
> +        bar = phb->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
>           size = PNV_PHB4_NUM_REGS << 3;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d",
>                    pec->chip_id, pec->index, stack->stack_no);
> @@ -966,7 +967,7 @@ static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
>       }
>       if (!memory_region_is_mapped(&phb->intbar) &&
>           (bar_en & PEC_NEST_STK_BAR_EN_INT)) {
> -        bar = stack->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
> +        bar = phb->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
>           size = PNV_PHB4_MAX_INTs << 16;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-int",
>                    stack->pec->chip_id, stack->pec->index, stack->stack_no);
> @@ -982,34 +983,35 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
>                                            uint64_t val, unsigned size)
>   {
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
> +    PnvPHB4 *phb = stack->phb;
>       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;
> +        phb->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;
> +        phb->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;
> +        phb->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;
> +        phb->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;
> +        phb->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;
> +        phb->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;
> +        phb->nest_regs[reg] = val;
>           break;
>       case PEC_NEST_STK_PCI_NEST_FIR_WOF:
> -        stack->nest_regs[reg] = 0;
> +        phb->nest_regs[reg] = 0;
>           break;
>       case PEC_NEST_STK_ERR_REPORT_0:
>       case PEC_NEST_STK_ERR_REPORT_1:
> @@ -1017,39 +1019,39 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
>           /* Flag error ? */
>           break;
>       case PEC_NEST_STK_PBCQ_MODE:
> -        stack->nest_regs[reg] = val & 0xff00000000000000ull;
> +        phb->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] &
> +        if (phb->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;
> +        phb->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) {
> +        if (phb->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;
> +        phb->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) {
> +        if (phb->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;
> +        phb->nest_regs[reg] = val & 0xfffffff000000000ull;
>           break;
>       case PEC_NEST_STK_BAR_EN:
> -        stack->nest_regs[reg] = val & 0xf000000000000000ull;
> +        phb->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;
> +        phb->nest_regs[reg] = val;
>           break;
>       default:
>           qemu_log_mask(LOG_UNIMP, "phb4_pec: nest_xscom_write 0x%"HWADDR_PRIx
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 4a8f510f6d..a7e08772c1 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -112,6 +112,10 @@ struct PnvPHB4 {
>       uint64_t pci_regs[PHB4_PEC_PCI_STK_REGS_COUNT];
>       MemoryRegion pci_regs_mr;
>   
> +    /* Nest registers */
> +#define PHB4_PEC_NEST_STK_REGS_COUNT  0x17
> +    uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
> +
>       /* Memory windows from PowerBus to PHB */
>       MemoryRegion phbbar;
>       MemoryRegion intbar;
> @@ -165,9 +169,6 @@ struct PnvPhb4PecStack {
>       /* My own stack number */
>       uint32_t stack_no;
>   
> -    /* Nest registers */
> -#define PHB4_PEC_NEST_STK_REGS_COUNT  0x17
> -    uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
>       MemoryRegion nest_regs_mr;
>   
>       /* PHB pass-through XSCOM */
> 

[PATCH 08/17] ppc/pnv: change pnv_pec_stk_update_map() to use PnvPHB4

[Daniel Henrique Barboza]

stack->nest_regs_mr wasn't migrated to PnvPHB4 together with phb->nest_regs[] in
the previous patch. We were unable to cleanly convert its write MemoryRegionOps,
pnv_pec_stk_nest_xscom_write(), to use PnvPHB4 instead of PnvPhb4PecStack due to
pnv_pec_stk_update_map() using a stack. Thing is, we're now able to convert
pnv_pec_stk_update_map() because of what the did in previous patch.

The need for this intermediate step is a good example of the interconnected
relationship between stack and phb that we aim to cleanup.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 916a7a3cf0..0f4464ec67 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -893,10 +893,10 @@ static void pnv_phb4_update_regions(PnvPHB4 *phb)
     pnv_phb4_check_all_mbt(phb);
 }
 
-static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
+static void pnv_pec_stk_update_map(PnvPHB4 *phb)
 {
+    PnvPhb4PecStack *stack = phb->stack;
     PnvPhb4PecState *pec = stack->pec;
-    PnvPHB4 *phb = stack->phb;
     MemoryRegion *sysmem = get_system_memory();
     uint64_t bar_en = phb->nest_regs[PEC_NEST_STK_BAR_EN];
     uint64_t bar, mask, size;
@@ -1046,7 +1046,7 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
         break;
     case PEC_NEST_STK_BAR_EN:
         phb->nest_regs[reg] = val & 0xf000000000000000ull;
-        pnv_pec_stk_update_map(stack);
+        pnv_pec_stk_update_map(phb);
         break;
     case PEC_NEST_STK_DATA_FRZ_TYPE:
     case PEC_NEST_STK_PBCQ_TUN_BAR:
-- 
2.33.1

Re: [PATCH 08/17] ppc/pnv: change pnv_pec_stk_update_map() to use PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> stack->nest_regs_mr wasn't migrated to PnvPHB4 together with phb->nest_regs[] in
> the previous patch. We were unable to cleanly convert its write MemoryRegionOps,
> pnv_pec_stk_nest_xscom_write(), to use PnvPHB4 instead of PnvPhb4PecStack due to
> pnv_pec_stk_update_map() using a stack. Thing is, we're now able to convert
> pnv_pec_stk_update_map() because of what the did in previous patch.
> 
> The need for this intermediate step is a good example of the interconnected
> relationship between stack and phb that we aim to cleanup.
> 
> 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 | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 916a7a3cf0..0f4464ec67 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -893,10 +893,10 @@ static void pnv_phb4_update_regions(PnvPHB4 *phb)
>       pnv_phb4_check_all_mbt(phb);
>   }
>   
> -static void pnv_pec_stk_update_map(PnvPhb4PecStack *stack)
> +static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>   {
> +    PnvPhb4PecStack *stack = phb->stack;
>       PnvPhb4PecState *pec = stack->pec;
> -    PnvPHB4 *phb = stack->phb;
>       MemoryRegion *sysmem = get_system_memory();
>       uint64_t bar_en = phb->nest_regs[PEC_NEST_STK_BAR_EN];
>       uint64_t bar, mask, size;
> @@ -1046,7 +1046,7 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
>           break;
>       case PEC_NEST_STK_BAR_EN:
>           phb->nest_regs[reg] = val & 0xf000000000000000ull;
> -        pnv_pec_stk_update_map(stack);
> +        pnv_pec_stk_update_map(phb);
>           break;
>       case PEC_NEST_STK_DATA_FRZ_TYPE:
>       case PEC_NEST_STK_PBCQ_TUN_BAR:
> 

[PATCH 09/17] ppc/pnv: move nest_regs_mr to PnvPHB4

[Daniel Henrique Barboza]

We're now able to cleanly move nest_regs_mr to the PnvPHB4 device.

One thing of notice here is the need to use a phb->stack->pec pointer
because pnv_pec_stk_nest_xscom_write requires a PEC object. Another
thing that can be noticed in the use of 'stack->stack_no' that still
remains throughout the XSCOM code.

After moving all MemoryRegions to the PnvPHB4 object, this illustrates
what is the remaining role of the stack: provide a PEC pointer and the
'stack_no' information. If we can provide these in the PnvPHB4 object
instead (spoiler: we can, and we will), the PnvPhb4PecStack device will
be deprecated and can be removed.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 16 +++++++---------
 include/hw/pci-host/pnv_phb4.h |  3 +--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 0f4464ec67..37bab10fcb 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -861,8 +861,7 @@ const MemoryRegionOps pnv_phb4_xscom_ops = {
 static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
                                             unsigned size)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
-    PnvPHB4 *phb = stack->phb;
+    PnvPHB4 *phb = PNV_PHB4(opaque);
     uint32_t reg = addr >> 3;
 
     /* TODO: add list of allowed registers and error out if not */
@@ -982,9 +981,8 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
 static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
                                          uint64_t val, unsigned size)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
-    PnvPHB4 *phb = stack->phb;
-    PnvPhb4PecState *pec = stack->pec;
+    PnvPHB4 *phb = PNV_PHB4(opaque);
+    PnvPhb4PecState *pec = phb->stack->pec;
     uint32_t reg = addr >> 3;
 
     switch (reg) {
@@ -1470,10 +1468,10 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
     assert(pec);
 
     /* Initialize the XSCOM regions for the stack registers */
-    snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-stack-%d",
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-phb-%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,
+    pnv_xscom_region_init(&phb->nest_regs_mr, OBJECT(phb),
+                          &pnv_pec_stk_nest_xscom_ops, phb, name,
                           PHB4_PEC_NEST_STK_REGS_COUNT);
 
     snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
@@ -1494,7 +1492,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
     /* Populate the XSCOM address space. */
     pnv_xscom_add_subregion(pec->chip,
                             pec_nest_base + 0x40 * (stack->stack_no + 1),
-                            &stack->nest_regs_mr);
+                            &phb->nest_regs_mr);
     pnv_xscom_add_subregion(pec->chip,
                             pec_pci_base + 0x40 * (stack->stack_no + 1),
                             &phb->pci_regs_mr);
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index a7e08772c1..1d53dda0ed 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -115,6 +115,7 @@ struct PnvPHB4 {
     /* Nest registers */
 #define PHB4_PEC_NEST_STK_REGS_COUNT  0x17
     uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
+    MemoryRegion nest_regs_mr;
 
     /* Memory windows from PowerBus to PHB */
     MemoryRegion phbbar;
@@ -169,8 +170,6 @@ struct PnvPhb4PecStack {
     /* My own stack number */
     uint32_t stack_no;
 
-    MemoryRegion nest_regs_mr;
-
     /* PHB pass-through XSCOM */
     MemoryRegion phb_regs_mr;
 
-- 
2.33.1

Re: [PATCH 09/17] ppc/pnv: move nest_regs_mr to PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> We're now able to cleanly move nest_regs_mr to the PnvPHB4 device.
> 
> One thing of notice here is the need to use a phb->stack->pec pointer
> because pnv_pec_stk_nest_xscom_write requires a PEC object. Another
> thing that can be noticed in the use of 'stack->stack_no' that still
> remains throughout the XSCOM code.
> 
> After moving all MemoryRegions to the PnvPHB4 object, this illustrates
> what is the remaining role of the stack: provide a PEC pointer and the
> 'stack_no' information. If we can provide these in the PnvPHB4 object
> instead (spoiler: we can, and we will), the PnvPhb4PecStack device will
> be deprecated and can be removed.
> 
> 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         | 16 +++++++---------
>   include/hw/pci-host/pnv_phb4.h |  3 +--
>   2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 0f4464ec67..37bab10fcb 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -861,8 +861,7 @@ const MemoryRegionOps pnv_phb4_xscom_ops = {
>   static uint64_t pnv_pec_stk_nest_xscom_read(void *opaque, hwaddr addr,
>                                               unsigned size)
>   {
> -    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
> -    PnvPHB4 *phb = stack->phb;
> +    PnvPHB4 *phb = PNV_PHB4(opaque);
>       uint32_t reg = addr >> 3;
>   
>       /* TODO: add list of allowed registers and error out if not */
> @@ -982,9 +981,8 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>   static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
>                                            uint64_t val, unsigned size)
>   {
> -    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(opaque);
> -    PnvPHB4 *phb = stack->phb;
> -    PnvPhb4PecState *pec = stack->pec;
> +    PnvPHB4 *phb = PNV_PHB4(opaque);
> +    PnvPhb4PecState *pec = phb->stack->pec;
>       uint32_t reg = addr >> 3;
>   
>       switch (reg) {
> @@ -1470,10 +1468,10 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>       assert(pec);
>   
>       /* Initialize the XSCOM regions for the stack registers */
> -    snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-stack-%d",
> +    snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-phb-%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,
> +    pnv_xscom_region_init(&phb->nest_regs_mr, OBJECT(phb),
> +                          &pnv_pec_stk_nest_xscom_ops, phb, name,
>                             PHB4_PEC_NEST_STK_REGS_COUNT);
>   
>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
> @@ -1494,7 +1492,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>       /* Populate the XSCOM address space. */
>       pnv_xscom_add_subregion(pec->chip,
>                               pec_nest_base + 0x40 * (stack->stack_no + 1),
> -                            &stack->nest_regs_mr);
> +                            &phb->nest_regs_mr);
>       pnv_xscom_add_subregion(pec->chip,
>                               pec_pci_base + 0x40 * (stack->stack_no + 1),
>                               &phb->pci_regs_mr);
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index a7e08772c1..1d53dda0ed 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -115,6 +115,7 @@ struct PnvPHB4 {
>       /* Nest registers */
>   #define PHB4_PEC_NEST_STK_REGS_COUNT  0x17
>       uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
> +    MemoryRegion nest_regs_mr;
>   
>       /* Memory windows from PowerBus to PHB */
>       MemoryRegion phbbar;
> @@ -169,8 +170,6 @@ struct PnvPhb4PecStack {
>       /* My own stack number */
>       uint32_t stack_no;
>   
> -    MemoryRegion nest_regs_mr;
> -
>       /* PHB pass-through XSCOM */
>       MemoryRegion phb_regs_mr;
>   
> 

[PATCH 10/17] ppc/pnv: move phb_regs_mr to PnvPHB4

[Daniel Henrique Barboza]

After recent changes, this MemoryRegion can be migrated to PnvPHB4
without too much trouble.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 6 +++---
 include/hw/pci-host/pnv_phb4.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 37bab10fcb..b5045fca64 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1481,9 +1481,9 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
                           PHB4_PEC_PCI_STK_REGS_COUNT);
 
     /* PHB pass-through */
-    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
              pec->chip_id, pec->index, stack->stack_no);
-    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(phb),
+    pnv_xscom_region_init(&phb->phb_regs_mr, OBJECT(phb),
                           &pnv_phb4_xscom_ops, phb, name, 0x40);
 
     pec_nest_base = pecc->xscom_nest_base(pec);
@@ -1499,7 +1499,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
     pnv_xscom_add_subregion(pec->chip,
                             pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
                             0x40 * stack->stack_no,
-                            &stack->phb_regs_mr);
+                            &phb->phb_regs_mr);
 }
 
 static void pnv_phb4_instance_init(Object *obj)
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 1d53dda0ed..6968efaba8 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -117,6 +117,9 @@ struct PnvPHB4 {
     uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
     MemoryRegion nest_regs_mr;
 
+    /* PHB pass-through XSCOM */
+    MemoryRegion phb_regs_mr;
+
     /* Memory windows from PowerBus to PHB */
     MemoryRegion phbbar;
     MemoryRegion intbar;
@@ -170,9 +173,6 @@ struct PnvPhb4PecStack {
     /* My own stack number */
     uint32_t stack_no;
 
-    /* PHB pass-through XSCOM */
-    MemoryRegion phb_regs_mr;
-
     /* The owner PEC */
     PnvPhb4PecState *pec;
 
-- 
2.33.1

Re: [PATCH 10/17] ppc/pnv: move phb_regs_mr to PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> After recent changes, this MemoryRegion can be migrated to PnvPHB4
> without too much trouble.
> 
> 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         | 6 +++---
>   include/hw/pci-host/pnv_phb4.h | 6 +++---
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 37bab10fcb..b5045fca64 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1481,9 +1481,9 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>                             PHB4_PEC_PCI_STK_REGS_COUNT);
>   
>       /* PHB pass-through */
> -    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
> +    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
>                pec->chip_id, pec->index, stack->stack_no);
> -    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(phb),
> +    pnv_xscom_region_init(&phb->phb_regs_mr, OBJECT(phb),
>                             &pnv_phb4_xscom_ops, phb, name, 0x40);
>   
>       pec_nest_base = pecc->xscom_nest_base(pec);
> @@ -1499,7 +1499,7 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>       pnv_xscom_add_subregion(pec->chip,
>                               pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>                               0x40 * stack->stack_no,
> -                            &stack->phb_regs_mr);
> +                            &phb->phb_regs_mr);
>   }
>   
>   static void pnv_phb4_instance_init(Object *obj)
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 1d53dda0ed..6968efaba8 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -117,6 +117,9 @@ struct PnvPHB4 {
>       uint64_t nest_regs[PHB4_PEC_NEST_STK_REGS_COUNT];
>       MemoryRegion nest_regs_mr;
>   
> +    /* PHB pass-through XSCOM */
> +    MemoryRegion phb_regs_mr;
> +
>       /* Memory windows from PowerBus to PHB */
>       MemoryRegion phbbar;
>       MemoryRegion intbar;
> @@ -170,9 +173,6 @@ struct PnvPhb4PecStack {
>       /* My own stack number */
>       uint32_t stack_no;
>   
> -    /* PHB pass-through XSCOM */
> -    MemoryRegion phb_regs_mr;
> -
>       /* The owner PEC */
>       PnvPhb4PecState *pec;
>   
> 

[PATCH 11/17] ppc/pnv: introduce PnvPHB4 'phb_number' property

[Daniel Henrique Barboza]

One of the remaining dependencies we have on the PnvPhb4PecStack object
is the stack->stack_no property. This is set as the position the stack
occupies in the pec->stacks[] array.

We need a way to report this same value in the PnvPHB4. This patch
creates a new property called 'phb_number' to be used in existing code
in all instances stack->stack_no is currently being used.

The 'phb_number' name is an indication of our future intention to convert
the pec->stacks[] array into a pec->phbs[] array, when the PEC object will
deal directly with phb4 objects.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 28 +++++++++++++++++-----------
 hw/pci-host/pnv_phb4_pec.c     |  2 ++
 include/hw/pci-host/pnv_phb4.h |  3 +++
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index b5045fca64..44f3087913 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -937,7 +937,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
         mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR0_MASK];
         size = ((~mask) >> 8) + 1;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio0",
-                 pec->chip_id, pec->index, stack->stack_no);
+                 pec->chip_id, pec->index, phb->phb_number);
         memory_region_init(&phb->mmbar0, OBJECT(phb), name, size);
         memory_region_add_subregion(sysmem, bar, &phb->mmbar0);
         phb->mmio0_base = bar;
@@ -949,7 +949,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
         mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR1_MASK];
         size = ((~mask) >> 8) + 1;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio1",
-                 pec->chip_id, pec->index, stack->stack_no);
+                 pec->chip_id, pec->index, phb->phb_number);
         memory_region_init(&phb->mmbar1, OBJECT(phb), name, size);
         memory_region_add_subregion(sysmem, bar, &phb->mmbar1);
         phb->mmio1_base = bar;
@@ -960,7 +960,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
         bar = phb->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
         size = PNV_PHB4_NUM_REGS << 3;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d",
-                 pec->chip_id, pec->index, stack->stack_no);
+                 pec->chip_id, pec->index, phb->phb_number);
         memory_region_init(&phb->phbbar, OBJECT(phb), name, size);
         memory_region_add_subregion(sysmem, bar, &phb->phbbar);
     }
@@ -969,7 +969,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
         bar = phb->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
         size = PNV_PHB4_MAX_INTs << 16;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-int",
-                 stack->pec->chip_id, stack->pec->index, stack->stack_no);
+                 stack->pec->chip_id, stack->pec->index, phb->phb_number);
         memory_region_init(&phb->intbar, OBJECT(phb), name, size);
         memory_region_add_subregion(sysmem, bar, &phb->intbar);
     }
@@ -1469,20 +1469,20 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
 
     /* Initialize the XSCOM regions for the stack registers */
     snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-phb-%d",
-             pec->chip_id, pec->index, stack->stack_no);
+             pec->chip_id, pec->index, phb->phb_number);
     pnv_xscom_region_init(&phb->nest_regs_mr, OBJECT(phb),
                           &pnv_pec_stk_nest_xscom_ops, phb, name,
                           PHB4_PEC_NEST_STK_REGS_COUNT);
 
     snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
-             pec->chip_id, pec->index, stack->stack_no);
+             pec->chip_id, pec->index, phb->phb_number);
     pnv_xscom_region_init(&phb->pci_regs_mr, OBJECT(phb),
                           &pnv_pec_stk_pci_xscom_ops, phb, name,
                           PHB4_PEC_PCI_STK_REGS_COUNT);
 
     /* PHB pass-through */
     snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
-             pec->chip_id, pec->index, stack->stack_no);
+             pec->chip_id, pec->index, phb->phb_number);
     pnv_xscom_region_init(&phb->phb_regs_mr, OBJECT(phb),
                           &pnv_phb4_xscom_ops, phb, name, 0x40);
 
@@ -1491,14 +1491,14 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
 
     /* Populate the XSCOM address space. */
     pnv_xscom_add_subregion(pec->chip,
-                            pec_nest_base + 0x40 * (stack->stack_no + 1),
+                            pec_nest_base + 0x40 * (phb->phb_number + 1),
                             &phb->nest_regs_mr);
     pnv_xscom_add_subregion(pec->chip,
-                            pec_pci_base + 0x40 * (stack->stack_no + 1),
+                            pec_pci_base + 0x40 * (phb->phb_number + 1),
                             &phb->pci_regs_mr);
     pnv_xscom_add_subregion(pec->chip,
                             pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
-                            0x40 * stack->stack_no,
+                            0x40 * phb->phb_number,
                             &phb->phb_regs_mr);
 }
 
@@ -1568,10 +1568,15 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        /* All other phb properties but 'version' are already set */
+        /*
+         * All other phb properties but 'version' and 'phb-number'
+         * are already set.
+         */
         pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
         object_property_set_int(OBJECT(phb), "version", pecc->version,
                                 &error_fatal);
+        object_property_set_int(OBJECT(phb), "phb-number",
+                                phb->stack->stack_no, &error_abort);
 
         /*
          * Assign stack->phb since pnv_phb4_update_regions() uses it
@@ -1677,6 +1682,7 @@ static void pnv_phb4_xive_notify(XiveNotifier *xf, uint32_t srcno)
 }
 
 static Property pnv_phb4_properties[] = {
+        DEFINE_PROP_UINT32("phb-number", PnvPHB4, phb_number, 0),
         DEFINE_PROP_UINT32("index", PnvPHB4, phb_id, 0),
         DEFINE_PROP_UINT32("chip-id", PnvPHB4, chip_id, 0),
         DEFINE_PROP_UINT64("version", PnvPHB4, version, 0),
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 7fe7f1f007..7c4b4023df 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -285,6 +285,8 @@ static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
 
     stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
 
+    object_property_set_int(OBJECT(stack->phb), "phb-number", stack->stack_no,
+                            &error_abort);
     object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
                             &error_fatal);
     object_property_set_int(OBJECT(stack->phb), "index", phb_id,
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 6968efaba8..fc7807be1c 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -84,6 +84,9 @@ struct PnvPHB4 {
 
     uint64_t version;
 
+    /* My own PHB number */
+    uint32_t phb_number;
+
     char bus_path[8];
 
     /* Main register images */
-- 
2.33.1

Re: [PATCH 11/17] ppc/pnv: introduce PnvPHB4 'phb_number' property

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> One of the remaining dependencies we have on the PnvPhb4PecStack object
> is the stack->stack_no property. This is set as the position the stack
> occupies in the pec->stacks[] array.
> 
> We need a way to report this same value in the PnvPHB4. This patch
> creates a new property called 'phb_number' to be used in existing code
> in all instances stack->stack_no is currently being used.
> 
> The 'phb_number' name is an indication of our future intention to convert
> the pec->stacks[] array into a pec->phbs[] array, when the PEC object will
> deal directly with phb4 objects.


So the PHB would have a 'phb_number' and a 'index' property ? That's
confusing. Can we simplify ? compute one from another ?

or keep 'stack_no' to make it clear this belongs to the stack subunit
logic.

Thanks,

C.

> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c         | 28 +++++++++++++++++-----------
>   hw/pci-host/pnv_phb4_pec.c     |  2 ++
>   include/hw/pci-host/pnv_phb4.h |  3 +++
>   3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index b5045fca64..44f3087913 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -937,7 +937,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>           mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR0_MASK];
>           size = ((~mask) >> 8) + 1;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio0",
> -                 pec->chip_id, pec->index, stack->stack_no);
> +                 pec->chip_id, pec->index, phb->phb_number);
>           memory_region_init(&phb->mmbar0, OBJECT(phb), name, size);
>           memory_region_add_subregion(sysmem, bar, &phb->mmbar0);
>           phb->mmio0_base = bar;
> @@ -949,7 +949,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>           mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR1_MASK];
>           size = ((~mask) >> 8) + 1;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio1",
> -                 pec->chip_id, pec->index, stack->stack_no);
> +                 pec->chip_id, pec->index, phb->phb_number);
>           memory_region_init(&phb->mmbar1, OBJECT(phb), name, size);
>           memory_region_add_subregion(sysmem, bar, &phb->mmbar1);
>           phb->mmio1_base = bar;
> @@ -960,7 +960,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>           bar = phb->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
>           size = PNV_PHB4_NUM_REGS << 3;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d",
> -                 pec->chip_id, pec->index, stack->stack_no);
> +                 pec->chip_id, pec->index, phb->phb_number);
>           memory_region_init(&phb->phbbar, OBJECT(phb), name, size);
>           memory_region_add_subregion(sysmem, bar, &phb->phbbar);
>       }
> @@ -969,7 +969,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>           bar = phb->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
>           size = PNV_PHB4_MAX_INTs << 16;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-int",
> -                 stack->pec->chip_id, stack->pec->index, stack->stack_no);
> +                 stack->pec->chip_id, stack->pec->index, phb->phb_number);
>           memory_region_init(&phb->intbar, OBJECT(phb), name, size);
>           memory_region_add_subregion(sysmem, bar, &phb->intbar);
>       }
> @@ -1469,20 +1469,20 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>   
>       /* Initialize the XSCOM regions for the stack registers */
>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-phb-%d",
> -             pec->chip_id, pec->index, stack->stack_no);
> +             pec->chip_id, pec->index, phb->phb_number);
>       pnv_xscom_region_init(&phb->nest_regs_mr, OBJECT(phb),
>                             &pnv_pec_stk_nest_xscom_ops, phb, name,
>                             PHB4_PEC_NEST_STK_REGS_COUNT);
>   
>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
> -             pec->chip_id, pec->index, stack->stack_no);
> +             pec->chip_id, pec->index, phb->phb_number);
>       pnv_xscom_region_init(&phb->pci_regs_mr, OBJECT(phb),
>                             &pnv_pec_stk_pci_xscom_ops, phb, name,
>                             PHB4_PEC_PCI_STK_REGS_COUNT);
>   
>       /* PHB pass-through */
>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
> -             pec->chip_id, pec->index, stack->stack_no);
> +             pec->chip_id, pec->index, phb->phb_number);
>       pnv_xscom_region_init(&phb->phb_regs_mr, OBJECT(phb),
>                             &pnv_phb4_xscom_ops, phb, name, 0x40);
>   
> @@ -1491,14 +1491,14 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>   
>       /* Populate the XSCOM address space. */
>       pnv_xscom_add_subregion(pec->chip,
> -                            pec_nest_base + 0x40 * (stack->stack_no + 1),
> +                            pec_nest_base + 0x40 * (phb->phb_number + 1),
>                               &phb->nest_regs_mr);
>       pnv_xscom_add_subregion(pec->chip,
> -                            pec_pci_base + 0x40 * (stack->stack_no + 1),
> +                            pec_pci_base + 0x40 * (phb->phb_number + 1),
>                               &phb->pci_regs_mr);
>       pnv_xscom_add_subregion(pec->chip,
>                               pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> -                            0x40 * stack->stack_no,
> +                            0x40 * phb->phb_number,
>                               &phb->phb_regs_mr);
>   }
>   
> @@ -1568,10 +1568,15 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>   
> -        /* All other phb properties but 'version' are already set */
> +        /*
> +         * All other phb properties but 'version' and 'phb-number'
> +         * are already set.
> +         */
>           pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
>           object_property_set_int(OBJECT(phb), "version", pecc->version,
>                                   &error_fatal);
> +        object_property_set_int(OBJECT(phb), "phb-number",
> +                                phb->stack->stack_no, &error_abort);
>   
>           /*
>            * Assign stack->phb since pnv_phb4_update_regions() uses it
> @@ -1677,6 +1682,7 @@ static void pnv_phb4_xive_notify(XiveNotifier *xf, uint32_t srcno)
>   }
>   
>   static Property pnv_phb4_properties[] = {
> +        DEFINE_PROP_UINT32("phb-number", PnvPHB4, phb_number, 0),
>           DEFINE_PROP_UINT32("index", PnvPHB4, phb_id, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPHB4, chip_id, 0),
>           DEFINE_PROP_UINT64("version", PnvPHB4, version, 0),
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 7fe7f1f007..7c4b4023df 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -285,6 +285,8 @@ static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
>   
>       stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
>   
> +    object_property_set_int(OBJECT(stack->phb), "phb-number", stack->stack_no,
> +                            &error_abort);
>       object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
>                               &error_fatal);
>       object_property_set_int(OBJECT(stack->phb), "index", phb_id,
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 6968efaba8..fc7807be1c 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -84,6 +84,9 @@ struct PnvPHB4 {
>   
>       uint64_t version;
>   
> +    /* My own PHB number */
> +    uint32_t phb_number;
> +
>       char bus_path[8];
>   
>       /* Main register images */
> 

Re: [PATCH 11/17] ppc/pnv: introduce PnvPHB4 'phb_number' property

[Daniel Henrique Barboza]

On 1/14/22 07:46, Cédric Le Goater wrote:
> On 1/13/22 20:29, Daniel Henrique Barboza wrote:
>> One of the remaining dependencies we have on the PnvPhb4PecStack object
>> is the stack->stack_no property. This is set as the position the stack
>> occupies in the pec->stacks[] array.
>>
>> We need a way to report this same value in the PnvPHB4. This patch
>> creates a new property called 'phb_number' to be used in existing code
>> in all instances stack->stack_no is currently being used.
>>
>> The 'phb_number' name is an indication of our future intention to convert
>> the pec->stacks[] array into a pec->phbs[] array, when the PEC object will
>> deal directly with phb4 objects.
> 
> 
> So the PHB would have a 'phb_number' and a 'index' property ? That's
> confusing. Can we simplify ? compute one from another ?
> 
> or keep 'stack_no' to make it clear this belongs to the stack subunit
> logic.


I guess for now we can keep it as phb->stack_no. We can think about reworking the
logic (my initial reaction is to keep 'index' and then derive the 'stack_no' from
it when needed) in a follow up.



Thanks,


Daniel

> 
> Thanks,
> 
> C.
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb4.c         | 28 +++++++++++++++++-----------
>>   hw/pci-host/pnv_phb4_pec.c     |  2 ++
>>   include/hw/pci-host/pnv_phb4.h |  3 +++
>>   3 files changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index b5045fca64..44f3087913 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -937,7 +937,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>>           mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR0_MASK];
>>           size = ((~mask) >> 8) + 1;
>>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio0",
>> -                 pec->chip_id, pec->index, stack->stack_no);
>> +                 pec->chip_id, pec->index, phb->phb_number);
>>           memory_region_init(&phb->mmbar0, OBJECT(phb), name, size);
>>           memory_region_add_subregion(sysmem, bar, &phb->mmbar0);
>>           phb->mmio0_base = bar;
>> @@ -949,7 +949,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>>           mask = phb->nest_regs[PEC_NEST_STK_MMIO_BAR1_MASK];
>>           size = ((~mask) >> 8) + 1;
>>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-mmio1",
>> -                 pec->chip_id, pec->index, stack->stack_no);
>> +                 pec->chip_id, pec->index, phb->phb_number);
>>           memory_region_init(&phb->mmbar1, OBJECT(phb), name, size);
>>           memory_region_add_subregion(sysmem, bar, &phb->mmbar1);
>>           phb->mmio1_base = bar;
>> @@ -960,7 +960,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>>           bar = phb->nest_regs[PEC_NEST_STK_PHB_REGS_BAR] >> 8;
>>           size = PNV_PHB4_NUM_REGS << 3;
>>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d",
>> -                 pec->chip_id, pec->index, stack->stack_no);
>> +                 pec->chip_id, pec->index, phb->phb_number);
>>           memory_region_init(&phb->phbbar, OBJECT(phb), name, size);
>>           memory_region_add_subregion(sysmem, bar, &phb->phbbar);
>>       }
>> @@ -969,7 +969,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>>           bar = phb->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
>>           size = PNV_PHB4_MAX_INTs << 16;
>>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-int",
>> -                 stack->pec->chip_id, stack->pec->index, stack->stack_no);
>> +                 stack->pec->chip_id, stack->pec->index, phb->phb_number);
>>           memory_region_init(&phb->intbar, OBJECT(phb), name, size);
>>           memory_region_add_subregion(sysmem, bar, &phb->intbar);
>>       }
>> @@ -1469,20 +1469,20 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>>       /* Initialize the XSCOM regions for the stack registers */
>>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest-phb-%d",
>> -             pec->chip_id, pec->index, stack->stack_no);
>> +             pec->chip_id, pec->index, phb->phb_number);
>>       pnv_xscom_region_init(&phb->nest_regs_mr, OBJECT(phb),
>>                             &pnv_pec_stk_nest_xscom_ops, phb, name,
>>                             PHB4_PEC_NEST_STK_REGS_COUNT);
>>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
>> -             pec->chip_id, pec->index, stack->stack_no);
>> +             pec->chip_id, pec->index, phb->phb_number);
>>       pnv_xscom_region_init(&phb->pci_regs_mr, OBJECT(phb),
>>                             &pnv_pec_stk_pci_xscom_ops, phb, name,
>>                             PHB4_PEC_PCI_STK_REGS_COUNT);
>>       /* PHB pass-through */
>>       snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-phb-%d",
>> -             pec->chip_id, pec->index, stack->stack_no);
>> +             pec->chip_id, pec->index, phb->phb_number);
>>       pnv_xscom_region_init(&phb->phb_regs_mr, OBJECT(phb),
>>                             &pnv_phb4_xscom_ops, phb, name, 0x40);
>> @@ -1491,14 +1491,14 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>>       /* Populate the XSCOM address space. */
>>       pnv_xscom_add_subregion(pec->chip,
>> -                            pec_nest_base + 0x40 * (stack->stack_no + 1),
>> +                            pec_nest_base + 0x40 * (phb->phb_number + 1),
>>                               &phb->nest_regs_mr);
>>       pnv_xscom_add_subregion(pec->chip,
>> -                            pec_pci_base + 0x40 * (stack->stack_no + 1),
>> +                            pec_pci_base + 0x40 * (phb->phb_number + 1),
>>                               &phb->pci_regs_mr);
>>       pnv_xscom_add_subregion(pec->chip,
>>                               pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>> -                            0x40 * stack->stack_no,
>> +                            0x40 * phb->phb_number,
>>                               &phb->phb_regs_mr);
>>   }
>> @@ -1568,10 +1568,15 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>               return;
>>           }
>> -        /* All other phb properties but 'version' are already set */
>> +        /*
>> +         * All other phb properties but 'version' and 'phb-number'
>> +         * are already set.
>> +         */
>>           pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
>>           object_property_set_int(OBJECT(phb), "version", pecc->version,
>>                                   &error_fatal);
>> +        object_property_set_int(OBJECT(phb), "phb-number",
>> +                                phb->stack->stack_no, &error_abort);
>>           /*
>>            * Assign stack->phb since pnv_phb4_update_regions() uses it
>> @@ -1677,6 +1682,7 @@ static void pnv_phb4_xive_notify(XiveNotifier *xf, uint32_t srcno)
>>   }
>>   static Property pnv_phb4_properties[] = {
>> +        DEFINE_PROP_UINT32("phb-number", PnvPHB4, phb_number, 0),
>>           DEFINE_PROP_UINT32("index", PnvPHB4, phb_id, 0),
>>           DEFINE_PROP_UINT32("chip-id", PnvPHB4, chip_id, 0),
>>           DEFINE_PROP_UINT64("version", PnvPHB4, version, 0),
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 7fe7f1f007..7c4b4023df 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -285,6 +285,8 @@ static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
>>       stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
>> +    object_property_set_int(OBJECT(stack->phb), "phb-number", stack->stack_no,
>> +                            &error_abort);
>>       object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
>>                               &error_fatal);
>>       object_property_set_int(OBJECT(stack->phb), "index", phb_id,
>> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
>> index 6968efaba8..fc7807be1c 100644
>> --- a/include/hw/pci-host/pnv_phb4.h
>> +++ b/include/hw/pci-host/pnv_phb4.h
>> @@ -84,6 +84,9 @@ struct PnvPHB4 {
>>       uint64_t version;
>> +    /* My own PHB number */
>> +    uint32_t phb_number;
>> +
>>       char bus_path[8];
>>       /* Main register images */
>>
> 

Re: [PATCH 11/17] ppc/pnv: introduce PnvPHB4 'phb_number' property

[Cédric Le Goater]

On 1/14/22 12:29, Daniel Henrique Barboza wrote:
> 
> 
> On 1/14/22 07:46, Cédric Le Goater wrote:
>> On 1/13/22 20:29, Daniel Henrique Barboza wrote:
>>> One of the remaining dependencies we have on the PnvPhb4PecStack object
>>> is the stack->stack_no property. This is set as the position the stack
>>> occupies in the pec->stacks[] array.
>>>
>>> We need a way to report this same value in the PnvPHB4. This patch
>>> creates a new property called 'phb_number' to be used in existing code
>>> in all instances stack->stack_no is currently being used.
>>>
>>> The 'phb_number' name is an indication of our future intention to convert
>>> the pec->stacks[] array into a pec->phbs[] array, when the PEC object will
>>> deal directly with phb4 objects.
>>
>>
>> So the PHB would have a 'phb_number' and a 'index' property ? That's
>> confusing. Can we simplify ? compute one from another ?
>>
>> or keep 'stack_no' to make it clear this belongs to the stack subunit
>> logic.
> 
> 
> I guess for now we can keep it as phb->stack_no. We can think about reworking the
> logic (my initial reaction is to keep 'index' and then derive the 'stack_no' from
> it when needed) in a follow up.

ok. Patches 1-10 are fine. no need to resend.

Thanks,

C.

[PATCH 12/17] ppc/pnv: introduce PnvPHB4 'pec' property

[Daniel Henrique Barboza]

This property will track the owner PEC of this PHB. For now it's
redundant since we can retrieve the PEC via phb->stack->pec but it
will not be redundant when we get rid of the stack device.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 20 +++++++++++---------
 hw/pci-host/pnv_phb4_pec.c     |  2 ++
 include/hw/pci-host/pnv_phb4.h |  3 +++
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 44f3087913..c9117221b2 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -894,8 +894,7 @@ static void pnv_phb4_update_regions(PnvPHB4 *phb)
 
 static void pnv_pec_stk_update_map(PnvPHB4 *phb)
 {
-    PnvPhb4PecStack *stack = phb->stack;
-    PnvPhb4PecState *pec = stack->pec;
+    PnvPhb4PecState *pec = phb->pec;
     MemoryRegion *sysmem = get_system_memory();
     uint64_t bar_en = phb->nest_regs[PEC_NEST_STK_BAR_EN];
     uint64_t bar, mask, size;
@@ -969,7 +968,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
         bar = phb->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
         size = PNV_PHB4_MAX_INTs << 16;
         snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-int",
-                 stack->pec->chip_id, stack->pec->index, phb->phb_number);
+                 phb->pec->chip_id, phb->pec->index, phb->phb_number);
         memory_region_init(&phb->intbar, OBJECT(phb), name, size);
         memory_region_add_subregion(sysmem, bar, &phb->intbar);
     }
@@ -982,7 +981,7 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
                                          uint64_t val, unsigned size)
 {
     PnvPHB4 *phb = PNV_PHB4(opaque);
-    PnvPhb4PecState *pec = phb->stack->pec;
+    PnvPhb4PecState *pec = phb->pec;
     uint32_t reg = addr >> 3;
 
     switch (reg) {
@@ -1458,8 +1457,7 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 
 static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
 {
-    PnvPhb4PecStack *stack = phb->stack;
-    PnvPhb4PecState *pec = stack->pec;
+    PnvPhb4PecState *pec = phb->pec;
     PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
     uint32_t pec_nest_base;
     uint32_t pec_pci_base;
@@ -1569,10 +1567,12 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
         }
 
         /*
-         * All other phb properties but 'version' and 'phb-number'
-         * are already set.
+         * All other phb properties but 'pec', 'version' and
+         * 'phb-number' are already set.
          */
-        pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
+        object_property_set_link(OBJECT(phb), "pec", OBJECT(phb->stack->pec),
+                                 &error_abort);
+        pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
         object_property_set_int(OBJECT(phb), "version", pecc->version,
                                 &error_fatal);
         object_property_set_int(OBJECT(phb), "phb-number",
@@ -1688,6 +1688,8 @@ static Property pnv_phb4_properties[] = {
         DEFINE_PROP_UINT64("version", PnvPHB4, version, 0),
         DEFINE_PROP_LINK("stack", PnvPHB4, stack, TYPE_PNV_PHB4_PEC_STACK,
                          PnvPhb4PecStack *),
+        DEFINE_PROP_LINK("pec", PnvPHB4, pec, TYPE_PNV_PHB4_PEC,
+                         PnvPhb4PecState *),
         DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 7c4b4023df..36cc4ffe7c 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -287,6 +287,8 @@ static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
 
     object_property_set_int(OBJECT(stack->phb), "phb-number", stack->stack_no,
                             &error_abort);
+    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
+                             &error_abort);
     object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
                             &error_fatal);
     object_property_set_int(OBJECT(stack->phb), "index", phb_id,
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index fc7807be1c..f66bc76b78 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -87,6 +87,9 @@ struct PnvPHB4 {
     /* My own PHB number */
     uint32_t phb_number;
 
+    /* The owner PEC */
+    PnvPhb4PecState *pec;
+
     char bus_path[8];
 
     /* Main register images */
-- 
2.33.1

Re: [PATCH 12/17] ppc/pnv: introduce PnvPHB4 'pec' property

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> This property will track the owner PEC of this PHB. For now it's
> redundant since we can retrieve the PEC via phb->stack->pec but it
> will not be redundant when we get rid of the stack device.
> 
> 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         | 20 +++++++++++---------
>   hw/pci-host/pnv_phb4_pec.c     |  2 ++
>   include/hw/pci-host/pnv_phb4.h |  3 +++
>   3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 44f3087913..c9117221b2 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -894,8 +894,7 @@ static void pnv_phb4_update_regions(PnvPHB4 *phb)
>   
>   static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>   {
> -    PnvPhb4PecStack *stack = phb->stack;
> -    PnvPhb4PecState *pec = stack->pec;
> +    PnvPhb4PecState *pec = phb->pec;
>       MemoryRegion *sysmem = get_system_memory();
>       uint64_t bar_en = phb->nest_regs[PEC_NEST_STK_BAR_EN];
>       uint64_t bar, mask, size;
> @@ -969,7 +968,7 @@ static void pnv_pec_stk_update_map(PnvPHB4 *phb)
>           bar = phb->nest_regs[PEC_NEST_STK_INT_BAR] >> 8;
>           size = PNV_PHB4_MAX_INTs << 16;
>           snprintf(name, sizeof(name), "pec-%d.%d-phb-%d-int",
> -                 stack->pec->chip_id, stack->pec->index, phb->phb_number);
> +                 phb->pec->chip_id, phb->pec->index, phb->phb_number);
>           memory_region_init(&phb->intbar, OBJECT(phb), name, size);
>           memory_region_add_subregion(sysmem, bar, &phb->intbar);
>       }
> @@ -982,7 +981,7 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
>                                            uint64_t val, unsigned size)
>   {
>       PnvPHB4 *phb = PNV_PHB4(opaque);
> -    PnvPhb4PecState *pec = phb->stack->pec;
> +    PnvPhb4PecState *pec = phb->pec;
>       uint32_t reg = addr >> 3;
>   
>       switch (reg) {
> @@ -1458,8 +1457,7 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>   
>   static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>   {
> -    PnvPhb4PecStack *stack = phb->stack;
> -    PnvPhb4PecState *pec = stack->pec;
> +    PnvPhb4PecState *pec = phb->pec;
>       PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>       uint32_t pec_nest_base;
>       uint32_t pec_pci_base;
> @@ -1569,10 +1567,12 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>           }
>   
>           /*
> -         * All other phb properties but 'version' and 'phb-number'
> -         * are already set.
> +         * All other phb properties but 'pec', 'version' and
> +         * 'phb-number' are already set.
>            */
> -        pecc = PNV_PHB4_PEC_GET_CLASS(phb->stack->pec);
> +        object_property_set_link(OBJECT(phb), "pec", OBJECT(phb->stack->pec),
> +                                 &error_abort);
> +        pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
>           object_property_set_int(OBJECT(phb), "version", pecc->version,
>                                   &error_fatal);
>           object_property_set_int(OBJECT(phb), "phb-number",
> @@ -1688,6 +1688,8 @@ static Property pnv_phb4_properties[] = {
>           DEFINE_PROP_UINT64("version", PnvPHB4, version, 0),
>           DEFINE_PROP_LINK("stack", PnvPHB4, stack, TYPE_PNV_PHB4_PEC_STACK,
>                            PnvPhb4PecStack *),
> +        DEFINE_PROP_LINK("pec", PnvPHB4, pec, TYPE_PNV_PHB4_PEC,
> +                         PnvPhb4PecState *),
>           DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 7c4b4023df..36cc4ffe7c 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -287,6 +287,8 @@ static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
>   
>       object_property_set_int(OBJECT(stack->phb), "phb-number", stack->stack_no,
>                               &error_abort);
> +    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
> +                             &error_abort);
>       object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
>                               &error_fatal);
>       object_property_set_int(OBJECT(stack->phb), "index", phb_id,
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index fc7807be1c..f66bc76b78 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -87,6 +87,9 @@ struct PnvPHB4 {
>       /* My own PHB number */
>       uint32_t phb_number;
>   
> +    /* The owner PEC */
> +    PnvPhb4PecState *pec;
> +
>       char bus_path[8];
>   
>       /* Main register images */
> 

[PATCH 13/17] ppc/pnv: remove stack pointer from PnvPHB4

[Daniel Henrique Barboza]

This pointer was being used for two reasons: pnv_phb4_update_regions()
was using it to access the PHB and phb4_realize() was using it as a way
to determine if the PHB was user created.

We can determine if the PHB is user created via phb->pec, introduced in
the previous patch, and pnv_phb4_update_regions() is no longer using
stack->phb.

Remove the pointer from the PnvPHB4 device.

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index c9117221b2..25b4248776 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1549,9 +1549,10 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     char name[32];
 
     /* User created PHB */
-    if (!phb->stack) {
+    if (!phb->pec) {
         PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
         PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
+        PnvPhb4PecStack *stack;
         PnvPhb4PecClass *pecc;
         BusState *s;
 
@@ -1560,7 +1561,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        phb->stack = pnv_phb4_get_stack(chip, phb, &local_err);
+        stack = pnv_phb4_get_stack(chip, phb, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
@@ -1570,19 +1571,13 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
          * All other phb properties but 'pec', 'version' and
          * 'phb-number' are already set.
          */
-        object_property_set_link(OBJECT(phb), "pec", OBJECT(phb->stack->pec),
+        object_property_set_link(OBJECT(phb), "pec", OBJECT(stack->pec),
                                  &error_abort);
         pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
         object_property_set_int(OBJECT(phb), "version", pecc->version,
                                 &error_fatal);
         object_property_set_int(OBJECT(phb), "phb-number",
-                                phb->stack->stack_no, &error_abort);
-
-        /*
-         * Assign stack->phb since pnv_phb4_update_regions() uses it
-         * to access the phb.
-         */
-        phb->stack->phb = phb;
+                                stack->stack_no, &error_abort);
 
         /*
          * Reparent user created devices to the chip to build
@@ -1686,8 +1681,6 @@ static Property pnv_phb4_properties[] = {
         DEFINE_PROP_UINT32("index", PnvPHB4, phb_id, 0),
         DEFINE_PROP_UINT32("chip-id", PnvPHB4, chip_id, 0),
         DEFINE_PROP_UINT64("version", PnvPHB4, version, 0),
-        DEFINE_PROP_LINK("stack", PnvPHB4, stack, TYPE_PNV_PHB4_PEC_STACK,
-                         PnvPhb4PecStack *),
         DEFINE_PROP_LINK("pec", PnvPHB4, pec, TYPE_PNV_PHB4_PEC,
                          PnvPhb4PecState *),
         DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 36cc4ffe7c..1de0eb9adc 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -295,8 +295,6 @@ static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
                             &error_fatal);
     object_property_set_int(OBJECT(stack->phb), "version", pecc->version,
                             &error_fatal);
-    object_property_set_link(OBJECT(stack->phb), "stack", OBJECT(stack),
-                             &error_abort);
 
     if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
         return;
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index f66bc76b78..90eb4575f8 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -154,8 +154,6 @@ struct PnvPHB4 {
     XiveSource xsrc;
     qemu_irq *qirqs;
 
-    PnvPhb4PecStack *stack;
-
     QLIST_HEAD(, PnvPhb4DMASpace) dma_spaces;
 };
 
-- 
2.33.1

Re: [PATCH 13/17] ppc/pnv: remove stack pointer from PnvPHB4

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> This pointer was being used for two reasons: pnv_phb4_update_regions()
> was using it to access the PHB and phb4_realize() was using it as a way
> to determine if the PHB was user created.
> 
> We can determine if the PHB is user created via phb->pec, introduced in
> the previous patch, and pnv_phb4_update_regions() is no longer using
> stack->phb.
> 
> Remove the pointer from the PnvPHB4 device.

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

Thanks,

C.



> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c         | 17 +++++------------
>   hw/pci-host/pnv_phb4_pec.c     |  2 --
>   include/hw/pci-host/pnv_phb4.h |  2 --
>   3 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index c9117221b2..25b4248776 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1549,9 +1549,10 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       char name[32];
>   
>       /* User created PHB */
> -    if (!phb->stack) {
> +    if (!phb->pec) {
>           PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>           PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
> +        PnvPhb4PecStack *stack;
>           PnvPhb4PecClass *pecc;
>           BusState *s;
>   
> @@ -1560,7 +1561,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>   
> -        phb->stack = pnv_phb4_get_stack(chip, phb, &local_err);
> +        stack = pnv_phb4_get_stack(chip, phb, &local_err);
>           if (local_err) {
>               error_propagate(errp, local_err);
>               return;
> @@ -1570,19 +1571,13 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>            * All other phb properties but 'pec', 'version' and
>            * 'phb-number' are already set.
>            */
> -        object_property_set_link(OBJECT(phb), "pec", OBJECT(phb->stack->pec),
> +        object_property_set_link(OBJECT(phb), "pec", OBJECT(stack->pec),
>                                    &error_abort);
>           pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
>           object_property_set_int(OBJECT(phb), "version", pecc->version,
>                                   &error_fatal);
>           object_property_set_int(OBJECT(phb), "phb-number",
> -                                phb->stack->stack_no, &error_abort);
> -
> -        /*
> -         * Assign stack->phb since pnv_phb4_update_regions() uses it
> -         * to access the phb.
> -         */
> -        phb->stack->phb = phb;
> +                                stack->stack_no, &error_abort);
>   
>           /*
>            * Reparent user created devices to the chip to build
> @@ -1686,8 +1681,6 @@ static Property pnv_phb4_properties[] = {
>           DEFINE_PROP_UINT32("index", PnvPHB4, phb_id, 0),
>           DEFINE_PROP_UINT32("chip-id", PnvPHB4, chip_id, 0),
>           DEFINE_PROP_UINT64("version", PnvPHB4, version, 0),
> -        DEFINE_PROP_LINK("stack", PnvPHB4, stack, TYPE_PNV_PHB4_PEC_STACK,
> -                         PnvPhb4PecStack *),
>           DEFINE_PROP_LINK("pec", PnvPHB4, pec, TYPE_PNV_PHB4_PEC,
>                            PnvPhb4PecState *),
>           DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 36cc4ffe7c..1de0eb9adc 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -295,8 +295,6 @@ static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
>                               &error_fatal);
>       object_property_set_int(OBJECT(stack->phb), "version", pecc->version,
>                               &error_fatal);
> -    object_property_set_link(OBJECT(stack->phb), "stack", OBJECT(stack),
> -                             &error_abort);
>   
>       if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
>           return;
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index f66bc76b78..90eb4575f8 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -154,8 +154,6 @@ struct PnvPHB4 {
>       XiveSource xsrc;
>       qemu_irq *qirqs;
>   
> -    PnvPhb4PecStack *stack;
> -
>       QLIST_HEAD(, PnvPhb4DMASpace) dma_spaces;
>   };
>   
> 

[PATCH 14/17] ppc/pnv: move default_phb_realize() to pec_realize()

[Daniel Henrique Barboza]

This is the last step before making the PEC device uses PHB4s directly.
Move the current pnv_pec_stk_default_phb_realize() call to
pec_realize(), renaming the function to pnv_pec_default_phb_realize(),
and set the PHB attributes using the PEC object directly.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4_pec.c | 67 ++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 1de0eb9adc..3339e0ea3d 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -112,6 +112,32 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void pnv_pec_default_phb_realize(PnvPhb4PecStack *stack,
+                                        int phb_number,
+                                        Error **errp)
+{
+    PnvPhb4PecState *pec = stack->pec;
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    int phb_id = pnv_phb4_pec_get_phb_id(pec, phb_number);
+
+    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
+
+    object_property_set_int(OBJECT(stack->phb), "phb-number", phb_number,
+                            &error_abort);
+    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
+                             &error_abort);
+    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);
+
+    if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
+        return;
+    }
+}
+
 static void pnv_pec_instance_init(Object *obj)
 {
     PnvPhb4PecState *pec = PNV_PHB4_PEC(obj);
@@ -144,6 +170,15 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
 
         object_property_set_int(stk_obj, "stack-no", i, &error_abort);
         object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
+
+        if (defaults_enabled()) {
+            pnv_pec_default_phb_realize(stack, i, errp);
+        }
+
+        /*
+         * qdev gets angry if we don't realize 'stack' here, even
+         * if stk_realize() is now empty.
+         */
         if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
             return;
         }
@@ -276,40 +311,8 @@ static const TypeInfo pnv_pec_type_info = {
     }
 };
 
-static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
-                                            Error **errp)
-{
-    PnvPhb4PecState *pec = stack->pec;
-    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
-    int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
-
-    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
-
-    object_property_set_int(OBJECT(stack->phb), "phb-number", stack->stack_no,
-                            &error_abort);
-    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
-                             &error_abort);
-    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);
-
-    if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
-        return;
-    }
-}
-
 static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
-
-    if (!defaults_enabled()) {
-        return;
-    }
-
-    pnv_pec_stk_default_phb_realize(stack, errp);
 }
 
 static Property pnv_pec_stk_properties[] = {
-- 
2.33.1

Re: [PATCH 14/17] ppc/pnv: move default_phb_realize() to pec_realize()

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> This is the last step before making the PEC device uses PHB4s directly.
> Move the current pnv_pec_stk_default_phb_realize() call to
> pec_realize(), renaming the function to pnv_pec_default_phb_realize(),
> and set the PHB attributes using the PEC object directly.
> 
> 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 | 67 ++++++++++++++++++++------------------
>   1 file changed, 35 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 1de0eb9adc..3339e0ea3d 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -112,6 +112,32 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
>       .endianness = DEVICE_BIG_ENDIAN,
>   };
>   
> +static void pnv_pec_default_phb_realize(PnvPhb4PecStack *stack,
> +                                        int phb_number,
> +                                        Error **errp)
> +{
> +    PnvPhb4PecState *pec = stack->pec;
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    int phb_id = pnv_phb4_pec_get_phb_id(pec, phb_number);
> +
> +    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> +
> +    object_property_set_int(OBJECT(stack->phb), "phb-number", phb_number,
> +                            &error_abort);
> +    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
> +                             &error_abort);
> +    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);
> +
> +    if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
> +        return;
> +    }
> +}
> +
>   static void pnv_pec_instance_init(Object *obj)
>   {
>       PnvPhb4PecState *pec = PNV_PHB4_PEC(obj);
> @@ -144,6 +170,15 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   
>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
>           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
> +
> +        if (defaults_enabled()) {
> +            pnv_pec_default_phb_realize(stack, i, errp);
> +        }
> +
> +        /*
> +         * qdev gets angry if we don't realize 'stack' here, even
> +         * if stk_realize() is now empty.
> +         */
>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
>           }
> @@ -276,40 +311,8 @@ static const TypeInfo pnv_pec_type_info = {
>       }
>   };
>   
> -static void pnv_pec_stk_default_phb_realize(PnvPhb4PecStack *stack,
> -                                            Error **errp)
> -{
> -    PnvPhb4PecState *pec = stack->pec;
> -    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> -    int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
> -
> -    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> -
> -    object_property_set_int(OBJECT(stack->phb), "phb-number", stack->stack_no,
> -                            &error_abort);
> -    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
> -                             &error_abort);
> -    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);
> -
> -    if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
> -        return;
> -    }
> -}
> -
>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>   {
> -    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
> -
> -    if (!defaults_enabled()) {
> -        return;
> -    }
> -
> -    pnv_pec_stk_default_phb_realize(stack, errp);
>   }
>   
>   static Property pnv_pec_stk_properties[] = {
> 

[PATCH 15/17] ppc/pnv: convert pec->stacks[] into pec->phbs[]

[Daniel Henrique Barboza]

This patch changes the design of the PEC device to use PHB4s instead of
PecStacks. After all the recent changes, PHB4s now contain all the
information needed for their proper functioning, not relying on PecStack
in any capacity.

All changes are being made in a single patch to avoid renaming parts of
the PecState and leaving the code in a strange way. E.g. rename
PecClass->num_stacks to num_phbs, which would then read a
pnv_pec_num_stacks[] array. To avoid mixing the old and new design more
than necessary it's clearer to do these changes in a single step.

The name changes made are:

- in PnvPhb4PecState, rename PHB4_PEC_MAX_STACKS to PHB4_PEC_MAX_PHBS,
'num_stacks' to 'num_phbs' and convert "PnvPhb4PecStack
stacks[PHB4_PEC_MAX_STACKS]" to "PnvPHB4 *phbs[PHB4_PEC_MAX_PHBS]";

- in PnvPhb4PecClass, rename *num_stacks to *num_phbs;

- pnv_pec_num_stacks[] is renamed to pnv_pec_num_phbs[].

The logical changes:

- pnv_pec_default_phb_realize():
  * init the PnvPHB4 qdev and assign it to the corresponding
pec->phbs[phb_number];
  * do not use stack->phb anymore;

- pnv_pec_realize():
  * use the new default_phb_realize() to init/realize each PHB if
running with defaults;

- pnv_pec_instance_init(): removed since we're creating the PHBs during
pec_realize();

- pnv_phb4_get_stack():
  * renamed to pnv_phb4_get_pec() and returns a PnvPhb4PecState*;
  * assign the right pec->phbs[] pointer to the phb;
  * set 'phb_number' of the PHB given that the information is already
available;

- pnv_phb4_realize(): use 'phb->pec' instead of 'stack'.

This design change shouldn't caused any behavioral change in the runtime
of the machine.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 31 +++++++--------
 hw/pci-host/pnv_phb4_pec.c     | 71 ++++++++++------------------------
 include/hw/pci-host/pnv_phb4.h | 10 ++---
 3 files changed, 40 insertions(+), 72 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 25b4248776..a9ec42ce2c 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1360,7 +1360,7 @@ int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index)
     int offset = 0;
 
     while (index--) {
-        offset += pecc->num_stacks[index];
+        offset += pecc->num_phbs[index];
     }
 
     return offset + stack_index;
@@ -1510,8 +1510,8 @@ static void pnv_phb4_instance_init(Object *obj)
     object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
 }
 
-static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
-                                           Error **errp)
+static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
+                                         Error **errp)
 {
     Pnv9Chip *chip9 = PNV9_CHIP(chip);
     int chip_id = phb->chip_id;
@@ -1520,14 +1520,19 @@ static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
 
     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.
+         * For each PEC, check the amount of phbs it supports
+         * and see if the given phb4 index matches an index.
          */
         PnvPhb4PecState *pec = &chip9->pecs[i];
 
-        for (j = 0; j < pec->num_stacks; j++) {
+        for (j = 0; j < pec->num_phbs; j++) {
             if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
-                return &pec->stacks[j];
+                pec->phbs[j] = phb;
+
+                /* Set phb-number now since we already have it */
+                object_property_set_int(OBJECT(phb), "phb-number",
+                                               j, &error_abort);
+                return pec;
             }
         }
     }
@@ -1552,7 +1557,6 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     if (!phb->pec) {
         PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
         PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
-        PnvPhb4PecStack *stack;
         PnvPhb4PecClass *pecc;
         BusState *s;
 
@@ -1561,23 +1565,16 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        stack = pnv_phb4_get_stack(chip, phb, &local_err);
+        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
         }
 
-        /*
-         * All other phb properties but 'pec', 'version' and
-         * 'phb-number' are already set.
-         */
-        object_property_set_link(OBJECT(phb), "pec", OBJECT(stack->pec),
-                                 &error_abort);
+        /* All other phb properties are already set */
         pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
         object_property_set_int(OBJECT(phb), "version", pecc->version,
                                 &error_fatal);
-        object_property_set_int(OBJECT(phb), "phb-number",
-                                stack->stack_no, &error_abort);
 
         /*
          * Reparent user created devices to the chip to build
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 3339e0ea3d..61d7add25a 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -112,40 +112,29 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void pnv_pec_default_phb_realize(PnvPhb4PecStack *stack,
+static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
                                         int phb_number,
                                         Error **errp)
 {
-    PnvPhb4PecState *pec = stack->pec;
+    PnvPHB4 *phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
     PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
     int phb_id = pnv_phb4_pec_get_phb_id(pec, phb_number);
 
-    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
-
-    object_property_set_int(OBJECT(stack->phb), "phb-number", phb_number,
+    object_property_set_int(OBJECT(phb), "phb-number", phb_number,
                             &error_abort);
-    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
+    object_property_set_link(OBJECT(phb), "pec", OBJECT(pec),
                              &error_abort);
-    object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
+    object_property_set_int(OBJECT(phb), "chip-id", pec->chip_id,
                             &error_fatal);
-    object_property_set_int(OBJECT(stack->phb), "index", phb_id,
+    object_property_set_int(OBJECT(phb), "index", phb_id,
                             &error_fatal);
-    object_property_set_int(OBJECT(stack->phb), "version", pecc->version,
+    object_property_set_int(OBJECT(phb), "version", pecc->version,
                             &error_fatal);
 
-    if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
-        return;
-    }
-}
-
-static void pnv_pec_instance_init(Object *obj)
-{
-    PnvPhb4PecState *pec = PNV_PHB4_PEC(obj);
-    int i;
+    pec->phbs[phb_number] = phb;
 
-    for (i = 0; i < PHB4_PEC_MAX_STACKS; i++) {
-        object_initialize_child(obj, "stack[*]", &pec->stacks[i],
-                                TYPE_PNV_PHB4_PEC_STACK);
+    if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
+        return;
     }
 }
 
@@ -161,30 +150,13 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    pec->num_stacks = pecc->num_stacks[pec->index];
-
-    /* Create stacks */
-    for (i = 0; i < pec->num_stacks; i++) {
-        PnvPhb4PecStack *stack = &pec->stacks[i];
-        Object *stk_obj = OBJECT(stack);
-
-        object_property_set_int(stk_obj, "stack-no", i, &error_abort);
-        object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
+    pec->num_phbs = pecc->num_phbs[pec->index];
 
-        if (defaults_enabled()) {
-            pnv_pec_default_phb_realize(stack, i, errp);
+    /* Create PHBs if running with defaults */
+    if (defaults_enabled()) {
+        for (i = 0; i < pec->num_phbs; i++) {
+            pnv_pec_default_phb_realize(pec, i, errp);
         }
-
-        /*
-         * qdev gets angry if we don't realize 'stack' here, even
-         * if stk_realize() is now empty.
-         */
-        if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
-            return;
-        }
-    }
-    for (; i < PHB4_PEC_MAX_STACKS; i++) {
-        object_unparent(OBJECT(&pec->stacks[i]));
     }
 
     /* Initialize the XSCOM regions for the PEC registers */
@@ -230,7 +202,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
     _FDT((fdt_setprop(fdt, offset, "compatible", pecc->compat,
                       pecc->compat_size)));
 
-    for (i = 0; i < pec->num_stacks; i++) {
+    for (i = 0; i < pec->num_phbs; i++) {
         int phb_id = pnv_phb4_pec_get_phb_id(pec, i);
         int stk_offset;
 
@@ -266,11 +238,11 @@ static uint32_t pnv_pec_xscom_nest_base(PnvPhb4PecState *pec)
 }
 
 /*
- * PEC0 -> 1 stack
- * PEC1 -> 2 stacks
- * PEC2 -> 3 stacks
+ * PEC0 -> 1 phb
+ * PEC1 -> 2 phb
+ * PEC2 -> 3 phbs
  */
-static const uint32_t pnv_pec_num_stacks[] = { 1, 2, 3 };
+static const uint32_t pnv_pec_num_phbs[] = { 1, 2, 3 };
 
 static void pnv_pec_class_init(ObjectClass *klass, void *data)
 {
@@ -295,14 +267,13 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
     pecc->stk_compat = stk_compat;
     pecc->stk_compat_size = sizeof(stk_compat);
     pecc->version = PNV_PHB4_VERSION;
-    pecc->num_stacks = pnv_pec_num_stacks;
+    pecc->num_phbs = pnv_pec_num_phbs;
 }
 
 static const TypeInfo pnv_pec_type_info = {
     .name          = TYPE_PNV_PHB4_PEC,
     .parent        = TYPE_DEVICE,
     .instance_size = sizeof(PnvPhb4PecState),
-    .instance_init = pnv_pec_instance_init,
     .class_init    = pnv_pec_class_init,
     .class_size    = sizeof(PnvPhb4PecClass),
     .interfaces    = (InterfaceInfo[]) {
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 90eb4575f8..170de2e752 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -206,10 +206,10 @@ struct PnvPhb4PecState {
     uint64_t pci_regs[PHB4_PEC_PCI_REGS_COUNT];
     MemoryRegion pci_regs_mr;
 
-    /* Stacks */
-    #define PHB4_PEC_MAX_STACKS     3
-    uint32_t num_stacks;
-    PnvPhb4PecStack stacks[PHB4_PEC_MAX_STACKS];
+    /* PHBs */
+    #define PHB4_PEC_MAX_PHBS     3
+    uint32_t num_phbs;
+    PnvPHB4 *phbs[PHB4_PEC_MAX_PHBS];
 
     PnvChip *chip;
 };
@@ -227,7 +227,7 @@ struct PnvPhb4PecClass {
     const char *stk_compat;
     int stk_compat_size;
     uint64_t version;
-    const uint32_t *num_stacks;
+    const uint32_t *num_phbs;
 };
 
 #endif /* PCI_HOST_PNV_PHB4_H */
-- 
2.33.1

Re: [PATCH 15/17] ppc/pnv: convert pec->stacks[] into pec->phbs[]

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> This patch changes the design of the PEC device to use PHB4s instead of
> PecStacks. After all the recent changes, PHB4s now contain all the
> information needed for their proper functioning, not relying on PecStack
> in any capacity.
> 
> All changes are being made in a single patch to avoid renaming parts of
> the PecState and leaving the code in a strange way. E.g. rename
> PecClass->num_stacks to num_phbs, which would then read a
> pnv_pec_num_stacks[] array. To avoid mixing the old and new design more
> than necessary it's clearer to do these changes in a single step.
> 
> The name changes made are:
> 
> - in PnvPhb4PecState, rename PHB4_PEC_MAX_STACKS to PHB4_PEC_MAX_PHBS,
> 'num_stacks' to 'num_phbs' and convert "PnvPhb4PecStack
> stacks[PHB4_PEC_MAX_STACKS]" to "PnvPHB4 *phbs[PHB4_PEC_MAX_PHBS]";
> 
> - in PnvPhb4PecClass, rename *num_stacks to *num_phbs;
> 
> - pnv_pec_num_stacks[] is renamed to pnv_pec_num_phbs[].
> 
> The logical changes:
> 
> - pnv_pec_default_phb_realize():
>    * init the PnvPHB4 qdev and assign it to the corresponding
> pec->phbs[phb_number];
>    * do not use stack->phb anymore;
> 
> - pnv_pec_realize():
>    * use the new default_phb_realize() to init/realize each PHB if
> running with defaults;
> 
> - pnv_pec_instance_init(): removed since we're creating the PHBs during
> pec_realize();
> 
> - pnv_phb4_get_stack():
>    * renamed to pnv_phb4_get_pec() and returns a PnvPhb4PecState*;
>    * assign the right pec->phbs[] pointer to the phb;
>    * set 'phb_number' of the PHB given that the information is already
> available;
> 
> - pnv_phb4_realize(): use 'phb->pec' instead of 'stack'.
> 
> This design change shouldn't caused any behavioral change in the runtime
> of the machine.
> 
> 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         | 31 +++++++--------
>   hw/pci-host/pnv_phb4_pec.c     | 71 ++++++++++------------------------
>   include/hw/pci-host/pnv_phb4.h | 10 ++---
>   3 files changed, 40 insertions(+), 72 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 25b4248776..a9ec42ce2c 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1360,7 +1360,7 @@ int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index)
>       int offset = 0;
>   
>       while (index--) {
> -        offset += pecc->num_stacks[index];
> +        offset += pecc->num_phbs[index];
>       }
>   
>       return offset + stack_index;
> @@ -1510,8 +1510,8 @@ static void pnv_phb4_instance_init(Object *obj)
>       object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
>   }
>   
> -static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
> -                                           Error **errp)
> +static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
> +                                         Error **errp)
>   {
>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
>       int chip_id = phb->chip_id;
> @@ -1520,14 +1520,19 @@ static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
>   
>       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.
> +         * For each PEC, check the amount of phbs it supports
> +         * and see if the given phb4 index matches an index.
>            */
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>   
> -        for (j = 0; j < pec->num_stacks; j++) {
> +        for (j = 0; j < pec->num_phbs; j++) {
>               if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> -                return &pec->stacks[j];
> +                pec->phbs[j] = phb;
> +
> +                /* Set phb-number now since we already have it */
> +                object_property_set_int(OBJECT(phb), "phb-number",
> +                                               j, &error_abort);
> +                return pec;
>               }
>           }
>       }
> @@ -1552,7 +1557,6 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       if (!phb->pec) {
>           PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>           PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
> -        PnvPhb4PecStack *stack;
>           PnvPhb4PecClass *pecc;
>           BusState *s;
>   
> @@ -1561,23 +1565,16 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>   
> -        stack = pnv_phb4_get_stack(chip, phb, &local_err);
> +        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
>           if (local_err) {
>               error_propagate(errp, local_err);
>               return;
>           }
>   
> -        /*
> -         * All other phb properties but 'pec', 'version' and
> -         * 'phb-number' are already set.
> -         */
> -        object_property_set_link(OBJECT(phb), "pec", OBJECT(stack->pec),
> -                                 &error_abort);
> +        /* All other phb properties are already set */
>           pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
>           object_property_set_int(OBJECT(phb), "version", pecc->version,
>                                   &error_fatal);
> -        object_property_set_int(OBJECT(phb), "phb-number",
> -                                stack->stack_no, &error_abort);
>   
>           /*
>            * Reparent user created devices to the chip to build
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 3339e0ea3d..61d7add25a 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -112,40 +112,29 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = {
>       .endianness = DEVICE_BIG_ENDIAN,
>   };
>   
> -static void pnv_pec_default_phb_realize(PnvPhb4PecStack *stack,
> +static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>                                           int phb_number,
>                                           Error **errp)
>   {
> -    PnvPhb4PecState *pec = stack->pec;
> +    PnvPHB4 *phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
>       PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>       int phb_id = pnv_phb4_pec_get_phb_id(pec, phb_number);
>   
> -    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> -
> -    object_property_set_int(OBJECT(stack->phb), "phb-number", phb_number,
> +    object_property_set_int(OBJECT(phb), "phb-number", phb_number,
>                               &error_abort);
> -    object_property_set_link(OBJECT(stack->phb), "pec", OBJECT(pec),
> +    object_property_set_link(OBJECT(phb), "pec", OBJECT(pec),
>                                &error_abort);
> -    object_property_set_int(OBJECT(stack->phb), "chip-id", pec->chip_id,
> +    object_property_set_int(OBJECT(phb), "chip-id", pec->chip_id,
>                               &error_fatal);
> -    object_property_set_int(OBJECT(stack->phb), "index", phb_id,
> +    object_property_set_int(OBJECT(phb), "index", phb_id,
>                               &error_fatal);
> -    object_property_set_int(OBJECT(stack->phb), "version", pecc->version,
> +    object_property_set_int(OBJECT(phb), "version", pecc->version,
>                               &error_fatal);
>   
> -    if (!sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
> -        return;
> -    }
> -}
> -
> -static void pnv_pec_instance_init(Object *obj)
> -{
> -    PnvPhb4PecState *pec = PNV_PHB4_PEC(obj);
> -    int i;
> +    pec->phbs[phb_number] = phb;
>   
> -    for (i = 0; i < PHB4_PEC_MAX_STACKS; i++) {
> -        object_initialize_child(obj, "stack[*]", &pec->stacks[i],
> -                                TYPE_PNV_PHB4_PEC_STACK);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
> +        return;
>       }
>   }
>   
> @@ -161,30 +150,13 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    pec->num_stacks = pecc->num_stacks[pec->index];
> -
> -    /* Create stacks */
> -    for (i = 0; i < pec->num_stacks; i++) {
> -        PnvPhb4PecStack *stack = &pec->stacks[i];
> -        Object *stk_obj = OBJECT(stack);
> -
> -        object_property_set_int(stk_obj, "stack-no", i, &error_abort);
> -        object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
> +    pec->num_phbs = pecc->num_phbs[pec->index];
>   
> -        if (defaults_enabled()) {
> -            pnv_pec_default_phb_realize(stack, i, errp);
> +    /* Create PHBs if running with defaults */
> +    if (defaults_enabled()) {
> +        for (i = 0; i < pec->num_phbs; i++) {
> +            pnv_pec_default_phb_realize(pec, i, errp);
>           }
> -
> -        /*
> -         * qdev gets angry if we don't realize 'stack' here, even
> -         * if stk_realize() is now empty.
> -         */
> -        if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
> -            return;
> -        }
> -    }
> -    for (; i < PHB4_PEC_MAX_STACKS; i++) {
> -        object_unparent(OBJECT(&pec->stacks[i]));
>       }
>   
>       /* Initialize the XSCOM regions for the PEC registers */
> @@ -230,7 +202,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>       _FDT((fdt_setprop(fdt, offset, "compatible", pecc->compat,
>                         pecc->compat_size)));
>   
> -    for (i = 0; i < pec->num_stacks; i++) {
> +    for (i = 0; i < pec->num_phbs; i++) {
>           int phb_id = pnv_phb4_pec_get_phb_id(pec, i);
>           int stk_offset;
>   
> @@ -266,11 +238,11 @@ static uint32_t pnv_pec_xscom_nest_base(PnvPhb4PecState *pec)
>   }
>   
>   /*
> - * PEC0 -> 1 stack
> - * PEC1 -> 2 stacks
> - * PEC2 -> 3 stacks
> + * PEC0 -> 1 phb
> + * PEC1 -> 2 phb
> + * PEC2 -> 3 phbs
>    */
> -static const uint32_t pnv_pec_num_stacks[] = { 1, 2, 3 };
> +static const uint32_t pnv_pec_num_phbs[] = { 1, 2, 3 };
>   
>   static void pnv_pec_class_init(ObjectClass *klass, void *data)
>   {
> @@ -295,14 +267,13 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
>       pecc->stk_compat = stk_compat;
>       pecc->stk_compat_size = sizeof(stk_compat);
>       pecc->version = PNV_PHB4_VERSION;
> -    pecc->num_stacks = pnv_pec_num_stacks;
> +    pecc->num_phbs = pnv_pec_num_phbs;
>   }
>   
>   static const TypeInfo pnv_pec_type_info = {
>       .name          = TYPE_PNV_PHB4_PEC,
>       .parent        = TYPE_DEVICE,
>       .instance_size = sizeof(PnvPhb4PecState),
> -    .instance_init = pnv_pec_instance_init,
>       .class_init    = pnv_pec_class_init,
>       .class_size    = sizeof(PnvPhb4PecClass),
>       .interfaces    = (InterfaceInfo[]) {
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 90eb4575f8..170de2e752 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -206,10 +206,10 @@ struct PnvPhb4PecState {
>       uint64_t pci_regs[PHB4_PEC_PCI_REGS_COUNT];
>       MemoryRegion pci_regs_mr;
>   
> -    /* Stacks */
> -    #define PHB4_PEC_MAX_STACKS     3
> -    uint32_t num_stacks;
> -    PnvPhb4PecStack stacks[PHB4_PEC_MAX_STACKS];
> +    /* PHBs */
> +    #define PHB4_PEC_MAX_PHBS     3
> +    uint32_t num_phbs;
> +    PnvPHB4 *phbs[PHB4_PEC_MAX_PHBS];
>   
>       PnvChip *chip;
>   };
> @@ -227,7 +227,7 @@ struct PnvPhb4PecClass {
>       const char *stk_compat;
>       int stk_compat_size;
>       uint64_t version;
> -    const uint32_t *num_stacks;
> +    const uint32_t *num_phbs;
>   };
>   
>   #endif /* PCI_HOST_PNV_PHB4_H */
> 

Re: [PATCH 15/17] ppc/pnv: convert pec->stacks[] into pec->phbs[]

[Cédric Le Goater]

> @@ -1520,14 +1520,19 @@ static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
>   
>       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.
> +         * For each PEC, check the amount of phbs it supports
> +         * and see if the given phb4 index matches an index.
>            */
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>   
> -        for (j = 0; j < pec->num_stacks; j++) {
> +        for (j = 0; j < pec->num_phbs; j++) {
>               if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> -                return &pec->stacks[j];
> +                pec->phbs[j] = phb;

Why do we need this array ?

> +
> +                /* Set phb-number now since we already have it */
> +                object_property_set_int(OBJECT(phb), "phb-number",
> +                                               j, &error_abort);

that's ugly :/

C.

> +                return pec;
>               }
>           }
>       }

Re: [PATCH 15/17] ppc/pnv: convert pec->stacks[] into pec->phbs[]

[Daniel Henrique Barboza]

On 1/14/22 10:33, Cédric Le Goater wrote:
>> @@ -1520,14 +1520,19 @@ static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
>>       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.
>> +         * For each PEC, check the amount of phbs it supports
>> +         * and see if the given phb4 index matches an index.
>>            */
>>           PnvPhb4PecState *pec = &chip9->pecs[i];
>> -        for (j = 0; j < pec->num_stacks; j++) {
>> +        for (j = 0; j < pec->num_phbs; j++) {
>>               if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
>> -                return &pec->stacks[j];
>> +                pec->phbs[j] = phb;
> 
> Why do we need this array ?


Actually we don't. While making  these patches I forgot to assign this pointer back
to the array and everything worked. We don't search the PHB back from the PEC at
any point.

This is being kept because I refrain from doing too much design changes at once. We
can drop it though - either in this patch or in a follow up.

> 
>> +
>> +                /* Set phb-number now since we already have it */
>> +                object_property_set_int(OBJECT(phb), "phb-number",
>> +                                               j, &error_abort);
> 
> that's ugly :/

Not my proudest line of code indeed.

Perhaps we're better of trying to get rid of stack->stack_no altogether before even
converting it to phb->stack_no. I'll see how that goes.



Daniel


> 
> C.
> 
>> +                return pec;
>>               }
>>           }
>>       }

[PATCH 16/17] ppc/pnv: remove PnvPhb4PecStack object

[Daniel Henrique Barboza]

All the complexity that was scattered between PnvPhb4PecStack and
PnvPHB4 are now centered in the PnvPHB4 device. PnvPhb4PecStack does not
serve any purpose in the current code base.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4_pec.c     | 34 ----------------------------------
 include/hw/pci-host/pnv_phb4.h | 20 --------------------
 2 files changed, 54 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 61d7add25a..02e7689372 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -282,43 +282,9 @@ static const TypeInfo pnv_pec_type_info = {
     }
 };
 
-static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
-{
-}
-
-static Property pnv_pec_stk_properties[] = {
-        DEFINE_PROP_UINT32("stack-no", PnvPhb4PecStack, stack_no, 0),
-        DEFINE_PROP_LINK("pec", PnvPhb4PecStack, pec, TYPE_PNV_PHB4_PEC,
-                         PnvPhb4PecState *),
-        DEFINE_PROP_END_OF_LIST(),
-};
-
-static void pnv_pec_stk_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    device_class_set_props(dc, pnv_pec_stk_properties);
-    dc->realize = pnv_pec_stk_realize;
-    dc->user_creatable = false;
-
-    /* TODO: reset regs ? */
-}
-
-static const TypeInfo pnv_pec_stk_type_info = {
-    .name          = TYPE_PNV_PHB4_PEC_STACK,
-    .parent        = TYPE_DEVICE,
-    .instance_size = sizeof(PnvPhb4PecStack),
-    .class_init    = pnv_pec_stk_class_init,
-    .interfaces    = (InterfaceInfo[]) {
-        { TYPE_PNV_XSCOM_INTERFACE },
-        { }
-    }
-};
-
 static void pnv_pec_register_types(void)
 {
     type_register_static(&pnv_pec_type_info);
-    type_register_static(&pnv_pec_stk_type_info);
 }
 
 type_init(pnv_pec_register_types);
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 170de2e752..96e8583e48 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -167,26 +167,6 @@ extern const MemoryRegionOps pnv_phb4_xscom_ops;
 #define TYPE_PNV_PHB4_PEC "pnv-phb4-pec"
 OBJECT_DECLARE_TYPE(PnvPhb4PecState, PnvPhb4PecClass, PNV_PHB4_PEC)
 
-#define TYPE_PNV_PHB4_PEC_STACK "pnv-phb4-pec-stack"
-OBJECT_DECLARE_SIMPLE_TYPE(PnvPhb4PecStack, PNV_PHB4_PEC_STACK)
-
-/* Per-stack data */
-struct PnvPhb4PecStack {
-    DeviceState parent;
-
-    /* My own stack number */
-    uint32_t stack_no;
-
-    /* The owner PEC */
-    PnvPhb4PecState *pec;
-
-    /*
-     * PHB4 pointer. pnv_phb4_update_regions() needs to access
-     * the PHB4 via a PnvPhb4PecStack pointer.
-     */
-    PnvPHB4 *phb;
-};
-
 struct PnvPhb4PecState {
     DeviceState parent;
 
-- 
2.33.1

Re: [PATCH 16/17] ppc/pnv: remove PnvPhb4PecStack object

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> All the complexity that was scattered between PnvPhb4PecStack and
> PnvPHB4 are now centered in the PnvPHB4 device. PnvPhb4PecStack does not
> serve any purpose in the current code base.
> 
> 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     | 34 ----------------------------------
>   include/hw/pci-host/pnv_phb4.h | 20 --------------------
>   2 files changed, 54 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 61d7add25a..02e7689372 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -282,43 +282,9 @@ static const TypeInfo pnv_pec_type_info = {
>       }
>   };
>   
> -static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> -{
> -}
> -
> -static Property pnv_pec_stk_properties[] = {
> -        DEFINE_PROP_UINT32("stack-no", PnvPhb4PecStack, stack_no, 0),
> -        DEFINE_PROP_LINK("pec", PnvPhb4PecStack, pec, TYPE_PNV_PHB4_PEC,
> -                         PnvPhb4PecState *),
> -        DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void pnv_pec_stk_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    device_class_set_props(dc, pnv_pec_stk_properties);
> -    dc->realize = pnv_pec_stk_realize;
> -    dc->user_creatable = false;
> -
> -    /* TODO: reset regs ? */
> -}
> -
> -static const TypeInfo pnv_pec_stk_type_info = {
> -    .name          = TYPE_PNV_PHB4_PEC_STACK,
> -    .parent        = TYPE_DEVICE,
> -    .instance_size = sizeof(PnvPhb4PecStack),
> -    .class_init    = pnv_pec_stk_class_init,
> -    .interfaces    = (InterfaceInfo[]) {
> -        { TYPE_PNV_XSCOM_INTERFACE },
> -        { }
> -    }
> -};
> -
>   static void pnv_pec_register_types(void)
>   {
>       type_register_static(&pnv_pec_type_info);
> -    type_register_static(&pnv_pec_stk_type_info);
>   }
>   
>   type_init(pnv_pec_register_types);
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 170de2e752..96e8583e48 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -167,26 +167,6 @@ extern const MemoryRegionOps pnv_phb4_xscom_ops;
>   #define TYPE_PNV_PHB4_PEC "pnv-phb4-pec"
>   OBJECT_DECLARE_TYPE(PnvPhb4PecState, PnvPhb4PecClass, PNV_PHB4_PEC)
>   
> -#define TYPE_PNV_PHB4_PEC_STACK "pnv-phb4-pec-stack"
> -OBJECT_DECLARE_SIMPLE_TYPE(PnvPhb4PecStack, PNV_PHB4_PEC_STACK)
> -
> -/* Per-stack data */
> -struct PnvPhb4PecStack {
> -    DeviceState parent;
> -
> -    /* My own stack number */
> -    uint32_t stack_no;
> -
> -    /* The owner PEC */
> -    PnvPhb4PecState *pec;
> -
> -    /*
> -     * PHB4 pointer. pnv_phb4_update_regions() needs to access
> -     * the PHB4 via a PnvPhb4PecStack pointer.
> -     */
> -    PnvPHB4 *phb;
> -};
> -
>   struct PnvPhb4PecState {
>       DeviceState parent;
>   
> 

[PATCH 17/17] ppc/pnv: rename pnv_pec_stk_update_map()

[Daniel Henrique Barboza]

This function does not use 'stack' anymore. Rename it to
pnv_pec_phb_update_map().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index a9ec42ce2c..d27b62a50a 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -892,7 +892,7 @@ static void pnv_phb4_update_regions(PnvPHB4 *phb)
     pnv_phb4_check_all_mbt(phb);
 }
 
-static void pnv_pec_stk_update_map(PnvPHB4 *phb)
+static void pnv_pec_phb_update_map(PnvPHB4 *phb)
 {
     PnvPhb4PecState *pec = phb->pec;
     MemoryRegion *sysmem = get_system_memory();
@@ -1043,7 +1043,7 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
         break;
     case PEC_NEST_STK_BAR_EN:
         phb->nest_regs[reg] = val & 0xf000000000000000ull;
-        pnv_pec_stk_update_map(phb);
+        pnv_pec_phb_update_map(phb);
         break;
     case PEC_NEST_STK_DATA_FRZ_TYPE:
     case PEC_NEST_STK_PBCQ_TUN_BAR:
-- 
2.33.1

Re: [PATCH 17/17] ppc/pnv: rename pnv_pec_stk_update_map()

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> This function does not use 'stack' anymore. Rename it to
> pnv_pec_phb_update_map().
> 
> 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 | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index a9ec42ce2c..d27b62a50a 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -892,7 +892,7 @@ static void pnv_phb4_update_regions(PnvPHB4 *phb)
>       pnv_phb4_check_all_mbt(phb);
>   }
>   
> -static void pnv_pec_stk_update_map(PnvPHB4 *phb)
> +static void pnv_pec_phb_update_map(PnvPHB4 *phb)
>   {
>       PnvPhb4PecState *pec = phb->pec;
>       MemoryRegion *sysmem = get_system_memory();
> @@ -1043,7 +1043,7 @@ static void pnv_pec_stk_nest_xscom_write(void *opaque, hwaddr addr,
>           break;
>       case PEC_NEST_STK_BAR_EN:
>           phb->nest_regs[reg] = val & 0xf000000000000000ull;
> -        pnv_pec_stk_update_map(phb);
> +        pnv_pec_phb_update_map(phb);
>           break;
>       case PEC_NEST_STK_DATA_FRZ_TYPE:
>       case PEC_NEST_STK_PBCQ_TUN_BAR:
> 

Re: [PATCH 00/17] remove PnvPhb4PecStack from Powernv9

[Cédric Le Goater]

On 1/13/22 20:29, Daniel Henrique Barboza wrote:
> Hi,
> 
> After all the done enabling pnv-phb4 user devices, it became clear that
> the stack object is just a container of the PHB and its resources than
> something that needs to be maintained by its own. Removing the
> PnvPhb4PecStack object promotes a simpler code where we're dealing only
> with PECs and PHB4s.
> 
> One thing that isn't handled in this series is the nested regs names.
> There are 30+ nested per-stack registers with names such as
> 'PEC_NEST_STK*' or 'PEC_PCI_STK*' that are left as is. Renaming them to
> remove the 'STK' reference can be done in a follow up when we're
> satisfied with what it is presented here.

I think that's fine. The name identifies the sub-unit logic to which
the register belongs to.

Thanks,

C.
  
> 
> No functional change is intended with this series. The series is based
> on top of current master (at f8d75e10d3),
> 
> Daniel Henrique Barboza (17):
>    ppc/pnv: use PHB4 obj in pnv_pec_stk_pci_xscom_ops
>    ppc/pnv: move PCI registers to PnvPHB4
>    ppc/pnv: move phbbar to PnvPHB4
>    ppc/pnv: move intbar to PnvPHB4
>    ppc/pnv: change pnv_phb4_update_regions() to use PnvPHB4
>    ppc/pnv: move mmbar0/mmbar1 and friends to PnvPHB4
>    ppc/pnv: move nest_regs[] to PnvPHB4
>    ppc/pnv: change pnv_pec_stk_update_map() to use PnvPHB4
>    ppc/pnv: move nest_regs_mr to PnvPHB4
>    ppc/pnv: move phb_regs_mr to PnvPHB4
>    ppc/pnv: introduce PnvPHB4 'phb_number' property
>    ppc/pnv: introduce PnvPHB4 'pec' property
>    ppc/pnv: remove stack pointer from PnvPHB4
>    ppc/pnv: move default_phb_realize() to pec_realize()
>    ppc/pnv: convert pec->stacks[] into pec->phbs[]
>    ppc/pnv: remove PnvPhb4PecStack object
>    ppc/pnv: rename pnv_pec_stk_update_map()
> 
>   hw/pci-host/pnv_phb4.c         | 271 ++++++++++++++++-----------------
>   hw/pci-host/pnv_phb4_pec.c     | 122 ++++-----------
>   include/hw/pci-host/pnv_phb4.h |  84 +++++-----
>   3 files changed, 200 insertions(+), 277 deletions(-)
>