[PATCH 00/11] pnv-phb related cleanups

[PATCH 00/11] pnv-phb related cleanups

[Daniel Henrique Barboza]

Hi,

I decided to make some cleanups/fixes after the feedback received in the
v2 of the pnv-phb proxy series [1]. A lot of the reviews and comment were
related to the current state of the code instead of what was being done
there.

With this series we want to:

- provide a better base for proxy pnv-phb series, fixing and changing code
that is already in place;
- make a common logic that applies to both default and user created devices.
There's no reason to make distinctions between them aside from what we will
already have to deal with.


After these cleanups we'll be able to simplify the work required in [1].

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg06254.html

Daniel Henrique Barboza (11):
  ppc/pnv: move root port attach to pnv_phb4_realize()
  ppc/pnv: attach phb3/phb4 root ports in QOM tree
  ppc/pnv: use dev->parent_bus->parent to get the PHB
  ppc/pnv: use dev instead of pci->qdev in root_port_realize()
  ppc/pnv: make pnv_ics_get() use the chip8->phbs[] array
  ppc/pnv: make pnv_ics_resend() use chip8->phbs[]
  ppc/pnv: make pnv_chip_power8_pic_print_info() use chip8->phbs[]
  ppc/pnv: turn chip8->phbs[] into a PnvPHB3* array
  ppc/pnv: add PHB object/bus parenting helpers
  ppc/pnv: move PHB3 initialization to realize time
  ppc/pnv: move PHB4 parent fixup to phb4_realize()

 hw/pci-host/pnv_phb3.c     |  30 +++++++--
 hw/pci-host/pnv_phb4.c     |  31 +++++++--
 hw/pci-host/pnv_phb4_pec.c |   4 --
 hw/ppc/pnv.c               | 127 +++++++++++++++++++++----------------
 include/hw/ppc/pnv.h       |  10 ++-
 5 files changed, 128 insertions(+), 74 deletions(-)

-- 
2.36.1

[PATCH 01/11] ppc/pnv: move root port attach to pnv_phb4_realize()

[Daniel Henrique Barboza]

Creating a root port is something related to the PHB, not the PEC. It
also makes the logic more in line with what pnv-phb3 does.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/pci-host/pnv_phb4.c     | 4 ++++
 hw/pci-host/pnv_phb4_pec.c | 3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 6594016121..23ad8de7ee 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1547,6 +1547,7 @@ static void pnv_phb4_instance_init(Object *obj)
 static void pnv_phb4_realize(DeviceState *dev, Error **errp)
 {
     PnvPHB4 *phb = PNV_PHB4(dev);
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
     PCIHostState *pci = PCI_HOST_BRIDGE(dev);
     XiveSource *xsrc = &phb->xsrc;
     int nr_irqs;
@@ -1583,6 +1584,9 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
     pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
 
+    /* Add a single Root port if running with defaults */
+    pnv_phb_attach_root_port(pci, pecc->rp_model);
+
     /* Setup XIVE Source */
     if (phb->big_phb) {
         nr_irqs = PNV_PHB4_MAX_INTs;
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 8b7e823fa5..c9aaf1c28e 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -130,9 +130,6 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
     if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
         return;
     }
-
-    /* Add a single Root port if running with defaults */
-    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), pecc->rp_model);
 }
 
 static void pnv_pec_realize(DeviceState *dev, Error **errp)
-- 
2.36.1

Re: [PATCH 01/11] ppc/pnv: move root port attach to pnv_phb4_realize()

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> Creating a root port is something related to the PHB, not the PEC. It
> also makes the logic more in line with what pnv-phb3 does.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---

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

   Fred


>   hw/pci-host/pnv_phb4.c     | 4 ++++
>   hw/pci-host/pnv_phb4_pec.c | 3 ---
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 6594016121..23ad8de7ee 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1547,6 +1547,7 @@ static void pnv_phb4_instance_init(Object *obj)
>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB4 *phb = PNV_PHB4(dev);
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>       XiveSource *xsrc = &phb->xsrc;
>       int nr_irqs;
> @@ -1583,6 +1584,9 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   
> +    /* Add a single Root port if running with defaults */
> +    pnv_phb_attach_root_port(pci, pecc->rp_model);
> +
>       /* Setup XIVE Source */
>       if (phb->big_phb) {
>           nr_irqs = PNV_PHB4_MAX_INTs;
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 8b7e823fa5..c9aaf1c28e 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -130,9 +130,6 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>           return;
>       }
> -
> -    /* Add a single Root port if running with defaults */
> -    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), pecc->rp_model);
>   }
>   
>   static void pnv_pec_realize(DeviceState *dev, Error **errp)

Re: [PATCH 01/11] ppc/pnv: move root port attach to pnv_phb4_realize()

[Cédric Le Goater]

On 6/13/22 17:44, Daniel Henrique Barboza wrote:
> Creating a root port is something related to the PHB, not the PEC. It
> also makes the logic more in line with what pnv-phb3 does.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>

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

So the root port is back where it was.

Could we avoid the pci_new() and use object_initialize_child() instead ?

Thanks,

C.


> ---
>   hw/pci-host/pnv_phb4.c     | 4 ++++
>   hw/pci-host/pnv_phb4_pec.c | 3 ---
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 6594016121..23ad8de7ee 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1547,6 +1547,7 @@ static void pnv_phb4_instance_init(Object *obj)
>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB4 *phb = PNV_PHB4(dev);
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>       XiveSource *xsrc = &phb->xsrc;
>       int nr_irqs;
> @@ -1583,6 +1584,9 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   
> +    /* Add a single Root port if running with defaults */
> +    pnv_phb_attach_root_port(pci, pecc->rp_model);
> +
>       /* Setup XIVE Source */
>       if (phb->big_phb) {
>           nr_irqs = PNV_PHB4_MAX_INTs;
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 8b7e823fa5..c9aaf1c28e 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -130,9 +130,6 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>           return;
>       }
> -
> -    /* Add a single Root port if running with defaults */
> -    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), pecc->rp_model);
>   }
>   
>   static void pnv_pec_realize(DeviceState *dev, Error **errp)

Re: [PATCH 01/11] ppc/pnv: move root port attach to pnv_phb4_realize()

[Daniel Henrique Barboza]

On 6/14/22 09:02, Cédric Le Goater wrote:
> On 6/13/22 17:44, Daniel Henrique Barboza wrote:
>> Creating a root port is something related to the PHB, not the PEC. It
>> also makes the logic more in line with what pnv-phb3 does.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> So the root port is back where it was.
> 
> Could we avoid the pci_new() and use object_initialize_child() instead ?


We could but then we would need to deal with yet another difference with
default versus user created devices, given that for user devices we can't
initialize_child(). And since we're also unifying the root ports later on
I'd rather wait to see how it turns out when everything is finished.


Tanks,

Daniel

> 
> Thanks,
> 
> C.
> 
> 
>> ---
>>   hw/pci-host/pnv_phb4.c     | 4 ++++
>>   hw/pci-host/pnv_phb4_pec.c | 3 ---
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 6594016121..23ad8de7ee 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1547,6 +1547,7 @@ static void pnv_phb4_instance_init(Object *obj)
>>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvPHB4 *phb = PNV_PHB4(dev);
>> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(phb->pec);
>>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>>       XiveSource *xsrc = &phb->xsrc;
>>       int nr_irqs;
>> @@ -1583,6 +1584,9 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>       pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>> +    /* Add a single Root port if running with defaults */
>> +    pnv_phb_attach_root_port(pci, pecc->rp_model);
>> +
>>       /* Setup XIVE Source */
>>       if (phb->big_phb) {
>>           nr_irqs = PNV_PHB4_MAX_INTs;
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 8b7e823fa5..c9aaf1c28e 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -130,9 +130,6 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>>           return;
>>       }
>> -
>> -    /* Add a single Root port if running with defaults */
>> -    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), pecc->rp_model);
>>   }
>>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
> 

[PATCH 02/11] ppc/pnv: attach phb3/phb4 root ports in QOM tree

[Daniel Henrique Barboza]

At this moment we leave the pnv-phb3(4)-root-port unattached in QOM:

  /unattached (container)
(...)
    /device[2] (pnv-phb3-root-port)
      /bus master container[0] (memory-region)
      /bus master[0] (memory-region)
      /pci_bridge_io[0] (memory-region)
      /pci_bridge_io[1] (memory-region)
      /pci_bridge_mem[0] (memory-region)
      /pci_bridge_pci[0] (memory-region)
      /pci_bridge_pref_mem[0] (memory-region)
      /pci_bridge_vga_io_hi[0] (memory-region)
      /pci_bridge_vga_io_lo[0] (memory-region)
      /pci_bridge_vga_mem[0] (memory-region)
      /pcie.0 (PCIE)

Let's make changes in pnv_phb_attach_root_port() to attach the created
root ports to its corresponding PHB.

This is the result afterwards:

    /pnv-phb3[0] (pnv-phb3)
      /lsi (ics)
      /msi (phb3-msi)
      /msi32[0] (memory-region)
      /msi64[0] (memory-region)
      /pbcq (pnv-pbcq)
    (...)
      /phb3_iommu[0] (pnv-phb3-iommu-memory-region)
      /pnv-phb3-root.0 (pnv-phb3-root)
        /pnv-phb3-root-port[0] (pnv-phb3-root-port)
          /bus master container[0] (memory-region)
          /bus master[0] (memory-region)
          /pci_bridge_io[0] (memory-region)
          /pci_bridge_io[1] (memory-region)
          /pci_bridge_mem[0] (memory-region)
          /pci_bridge_pci[0] (memory-region)
          /pci_bridge_pref_mem[0] (memory-region)
          /pci_bridge_vga_io_hi[0] (memory-region)
          /pci_bridge_vga_io_lo[0] (memory-region)
          /pci_bridge_vga_mem[0] (memory-region)
          /pcie.0 (PCIE)

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/pci-host/pnv_phb3.c | 2 +-
 hw/pci-host/pnv_phb4.c | 2 +-
 hw/ppc/pnv.c           | 7 ++++++-
 include/hw/ppc/pnv.h   | 2 +-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 26ac9b7123..4ba660f8b9 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1052,7 +1052,7 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
 
     pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
 
-    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT);
+    pnv_phb_attach_root_port(pci, TYPE_PNV_PHB3_ROOT_PORT, phb->phb_id);
 }
 
 void pnv_phb3_update_regions(PnvPHB3 *phb)
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 23ad8de7ee..ffd9d8a947 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1585,7 +1585,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
 
     /* Add a single Root port if running with defaults */
-    pnv_phb_attach_root_port(pci, pecc->rp_model);
+    pnv_phb_attach_root_port(pci, pecc->rp_model, phb->phb_id);
 
     /* Setup XIVE Source */
     if (phb->big_phb) {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 7c08a78d6c..40e0cbd84d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1190,9 +1190,14 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
 }
 
 /* Attach a root port device */
-void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
+void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index)
 {
     PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
+    g_autofree char *default_id = g_strdup_printf("%s[%d]", name, index);
+    const char *dev_id = DEVICE(root)->id;
+
+    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
+                              OBJECT(root));
 
     pci_realize_and_unref(root, pci->bus, &error_fatal);
 }
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 86cb7d7f97..033890a23f 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -189,7 +189,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
                          TYPE_PNV_CHIP_POWER10)
 
 PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
-void pnv_phb_attach_root_port(PCIHostState *pci, const char *name);
+void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index);
 
 #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
 typedef struct PnvMachineClass PnvMachineClass;
-- 
2.36.1

Re: [PATCH 02/11] ppc/pnv: attach phb3/phb4 root ports in QOM tree

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> At this moment we leave the pnv-phb3(4)-root-port unattached in QOM:
> 
>    /unattached (container)
> (...)
>      /device[2] (pnv-phb3-root-port)
>        /bus master container[0] (memory-region)
>        /bus master[0] (memory-region)
>        /pci_bridge_io[0] (memory-region)
>        /pci_bridge_io[1] (memory-region)
>        /pci_bridge_mem[0] (memory-region)
>        /pci_bridge_pci[0] (memory-region)
>        /pci_bridge_pref_mem[0] (memory-region)
>        /pci_bridge_vga_io_hi[0] (memory-region)
>        /pci_bridge_vga_io_lo[0] (memory-region)
>        /pci_bridge_vga_mem[0] (memory-region)
>        /pcie.0 (PCIE)
> 
> Let's make changes in pnv_phb_attach_root_port() to attach the created
> root ports to its corresponding PHB.
> 
> This is the result afterwards:
> 
>      /pnv-phb3[0] (pnv-phb3)
>        /lsi (ics)
>        /msi (phb3-msi)
>        /msi32[0] (memory-region)
>        /msi64[0] (memory-region)
>        /pbcq (pnv-pbcq)
>      (...)
>        /phb3_iommu[0] (pnv-phb3-iommu-memory-region)
>        /pnv-phb3-root.0 (pnv-phb3-root)
>          /pnv-phb3-root-port[0] (pnv-phb3-root-port)
>            /bus master container[0] (memory-region)
>            /bus master[0] (memory-region)
>            /pci_bridge_io[0] (memory-region)
>            /pci_bridge_io[1] (memory-region)
>            /pci_bridge_mem[0] (memory-region)
>            /pci_bridge_pci[0] (memory-region)
>            /pci_bridge_pref_mem[0] (memory-region)
>            /pci_bridge_vga_io_hi[0] (memory-region)
>            /pci_bridge_vga_io_lo[0] (memory-region)
>            /pci_bridge_vga_mem[0] (memory-region)
>            /pcie.0 (PCIE)
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---


I've always wondered if there was a good reason to have them detached. 
Glad to see there was none :-)

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

   Fred


>   hw/pci-host/pnv_phb3.c | 2 +-
>   hw/pci-host/pnv_phb4.c | 2 +-
>   hw/ppc/pnv.c           | 7 ++++++-
>   include/hw/ppc/pnv.h   | 2 +-
>   4 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 26ac9b7123..4ba660f8b9 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1052,7 +1052,7 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>   
>       pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
>   
> -    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT);
> +    pnv_phb_attach_root_port(pci, TYPE_PNV_PHB3_ROOT_PORT, phb->phb_id);
>   }
>   
>   void pnv_phb3_update_regions(PnvPHB3 *phb)
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 23ad8de7ee..ffd9d8a947 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1585,7 +1585,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   
>       /* Add a single Root port if running with defaults */
> -    pnv_phb_attach_root_port(pci, pecc->rp_model);
> +    pnv_phb_attach_root_port(pci, pecc->rp_model, phb->phb_id);
>   
>       /* Setup XIVE Source */
>       if (phb->big_phb) {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7c08a78d6c..40e0cbd84d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1190,9 +1190,14 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
>   }
>   
>   /* Attach a root port device */
> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index)
>   {
>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
> +    g_autofree char *default_id = g_strdup_printf("%s[%d]", name, index);
> +    const char *dev_id = DEVICE(root)->id;
> +
> +    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
> +                              OBJECT(root));
>   
>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>   }
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 86cb7d7f97..033890a23f 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -189,7 +189,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
>                            TYPE_PNV_CHIP_POWER10)
>   
>   PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name);
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index);
>   
>   #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>   typedef struct PnvMachineClass PnvMachineClass;

Re: [PATCH 02/11] ppc/pnv: attach phb3/phb4 root ports in QOM tree

[Cédric Le Goater]

On 6/14/22 11:09, Frederic Barrat wrote:
> 
> 
> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>> At this moment we leave the pnv-phb3(4)-root-port unattached in QOM:
>>
>>    /unattached (container)
>> (...)
>>      /device[2] (pnv-phb3-root-port)
>>        /bus master container[0] (memory-region)
>>        /bus master[0] (memory-region)
>>        /pci_bridge_io[0] (memory-region)
>>        /pci_bridge_io[1] (memory-region)
>>        /pci_bridge_mem[0] (memory-region)
>>        /pci_bridge_pci[0] (memory-region)
>>        /pci_bridge_pref_mem[0] (memory-region)
>>        /pci_bridge_vga_io_hi[0] (memory-region)
>>        /pci_bridge_vga_io_lo[0] (memory-region)
>>        /pci_bridge_vga_mem[0] (memory-region)
>>        /pcie.0 (PCIE)
>>
>> Let's make changes in pnv_phb_attach_root_port() to attach the created
>> root ports to its corresponding PHB.
>>
>> This is the result afterwards:
>>
>>      /pnv-phb3[0] (pnv-phb3)
>>        /lsi (ics)
>>        /msi (phb3-msi)
>>        /msi32[0] (memory-region)
>>        /msi64[0] (memory-region)
>>        /pbcq (pnv-pbcq)
>>      (...)
>>        /phb3_iommu[0] (pnv-phb3-iommu-memory-region)
>>        /pnv-phb3-root.0 (pnv-phb3-root)
>>          /pnv-phb3-root-port[0] (pnv-phb3-root-port)
>>            /bus master container[0] (memory-region)
>>            /bus master[0] (memory-region)
>>            /pci_bridge_io[0] (memory-region)
>>            /pci_bridge_io[1] (memory-region)
>>            /pci_bridge_mem[0] (memory-region)
>>            /pci_bridge_pci[0] (memory-region)
>>            /pci_bridge_pref_mem[0] (memory-region)
>>            /pci_bridge_vga_io_hi[0] (memory-region)
>>            /pci_bridge_vga_io_lo[0] (memory-region)
>>            /pci_bridge_vga_mem[0] (memory-region)
>>            /pcie.0 (PCIE)
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
> 
> 
> I've always wondered if there was a good reason to have them detached. Glad to see there was none :-)

Wasn't it for libvirt integration ?

C.


> 
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> 
>    Fred
> 
> 
>>   hw/pci-host/pnv_phb3.c | 2 +-
>>   hw/pci-host/pnv_phb4.c | 2 +-
>>   hw/ppc/pnv.c           | 7 ++++++-
>>   include/hw/ppc/pnv.h   | 2 +-
>>   4 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index 26ac9b7123..4ba660f8b9 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -1052,7 +1052,7 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>>       pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
>> -    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT);
>> +    pnv_phb_attach_root_port(pci, TYPE_PNV_PHB3_ROOT_PORT, phb->phb_id);
>>   }
>>   void pnv_phb3_update_regions(PnvPHB3 *phb)
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 23ad8de7ee..ffd9d8a947 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1585,7 +1585,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>>       /* Add a single Root port if running with defaults */
>> -    pnv_phb_attach_root_port(pci, pecc->rp_model);
>> +    pnv_phb_attach_root_port(pci, pecc->rp_model, phb->phb_id);
>>       /* Setup XIVE Source */
>>       if (phb->big_phb) {
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7c08a78d6c..40e0cbd84d 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1190,9 +1190,14 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
>>   }
>>   /* Attach a root port device */
>> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
>> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index)
>>   {
>>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
>> +    g_autofree char *default_id = g_strdup_printf("%s[%d]", name, index);
>> +    const char *dev_id = DEVICE(root)->id;
>> +
>> +    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
>> +                              OBJECT(root));
>>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>>   }
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 86cb7d7f97..033890a23f 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -189,7 +189,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
>>                            TYPE_PNV_CHIP_POWER10)
>>   PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
>> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name);
>> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index);
>>   #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>>   typedef struct PnvMachineClass PnvMachineClass;

Re: [PATCH 02/11] ppc/pnv: attach phb3/phb4 root ports in QOM tree

[Cédric Le Goater]

On 6/13/22 17:44, Daniel Henrique Barboza wrote:
> At this moment we leave the pnv-phb3(4)-root-port unattached in QOM:
> 
>    /unattached (container)
> (...)
>      /device[2] (pnv-phb3-root-port)
>        /bus master container[0] (memory-region)
>        /bus master[0] (memory-region)
>        /pci_bridge_io[0] (memory-region)
>        /pci_bridge_io[1] (memory-region)
>        /pci_bridge_mem[0] (memory-region)
>        /pci_bridge_pci[0] (memory-region)
>        /pci_bridge_pref_mem[0] (memory-region)
>        /pci_bridge_vga_io_hi[0] (memory-region)
>        /pci_bridge_vga_io_lo[0] (memory-region)
>        /pci_bridge_vga_mem[0] (memory-region)
>        /pcie.0 (PCIE)
> 
> Let's make changes in pnv_phb_attach_root_port() to attach the created
> root ports to its corresponding PHB.
> 
> This is the result afterwards:
> 
>      /pnv-phb3[0] (pnv-phb3)
>        /lsi (ics)
>        /msi (phb3-msi)
>        /msi32[0] (memory-region)
>        /msi64[0] (memory-region)
>        /pbcq (pnv-pbcq)
>      (...)
>        /phb3_iommu[0] (pnv-phb3-iommu-memory-region)
>        /pnv-phb3-root.0 (pnv-phb3-root)
>          /pnv-phb3-root-port[0] (pnv-phb3-root-port)
>            /bus master container[0] (memory-region)
>            /bus master[0] (memory-region)
>            /pci_bridge_io[0] (memory-region)
>            /pci_bridge_io[1] (memory-region)
>            /pci_bridge_mem[0] (memory-region)
>            /pci_bridge_pci[0] (memory-region)
>            /pci_bridge_pref_mem[0] (memory-region)
>            /pci_bridge_vga_io_hi[0] (memory-region)
>            /pci_bridge_vga_io_lo[0] (memory-region)
>            /pci_bridge_vga_mem[0] (memory-region)
>            /pcie.0 (PCIE)
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>


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

Thanks,

C.


> ---
>   hw/pci-host/pnv_phb3.c | 2 +-
>   hw/pci-host/pnv_phb4.c | 2 +-
>   hw/ppc/pnv.c           | 7 ++++++-
>   include/hw/ppc/pnv.h   | 2 +-
>   4 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 26ac9b7123..4ba660f8b9 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1052,7 +1052,7 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>   
>       pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
>   
> -    pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), TYPE_PNV_PHB3_ROOT_PORT);
> +    pnv_phb_attach_root_port(pci, TYPE_PNV_PHB3_ROOT_PORT, phb->phb_id);
>   }
>   
>   void pnv_phb3_update_regions(PnvPHB3 *phb)
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 23ad8de7ee..ffd9d8a947 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1585,7 +1585,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   
>       /* Add a single Root port if running with defaults */
> -    pnv_phb_attach_root_port(pci, pecc->rp_model);
> +    pnv_phb_attach_root_port(pci, pecc->rp_model, phb->phb_id);
>   
>       /* Setup XIVE Source */
>       if (phb->big_phb) {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7c08a78d6c..40e0cbd84d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1190,9 +1190,14 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
>   }
>   
>   /* Attach a root port device */
> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index)
>   {
>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
> +    g_autofree char *default_id = g_strdup_printf("%s[%d]", name, index);
> +    const char *dev_id = DEVICE(root)->id;
> +
> +    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
> +                              OBJECT(root));
>   
>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>   }
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 86cb7d7f97..033890a23f 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -189,7 +189,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
>                            TYPE_PNV_CHIP_POWER10)
>   
>   PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name);
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index);
>   
>   #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>   typedef struct PnvMachineClass PnvMachineClass;

[PATCH 03/11] ppc/pnv: use dev->parent_bus->parent to get the PHB

[Daniel Henrique Barboza]

It is not advisable to execute an object_dynamic_cast() to poke into
bus->qbus.parent and follow it up with a C cast into the PnvPHB type we
think we got.

A better way is to access the PnvPHB object via a QOM macro accessing
the existing parent links of the DeviceState. For a given
pnv-phb3/4-root-port 'dev', dev->parent_bus will give us the PHB bus,
and dev->parent_bus->parent is the PHB. Use the adequate QOM macro to
assert the type, and keep the NULL check in case we didn't get the
object we were expecting.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/pci-host/pnv_phb3.c | 10 +++++++---
 hw/pci-host/pnv_phb4.c | 10 +++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 4ba660f8b9..7901d8172c 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1139,12 +1139,16 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
 {
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
     PCIDevice *pci = PCI_DEVICE(dev);
-    PCIBus *bus = pci_get_bus(pci);
     PnvPHB3 *phb = NULL;
     Error *local_err = NULL;
 
-    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
-                                          TYPE_PNV_PHB3);
+    /*
+     * dev->parent_bus gives access to the pnv-phb-root bus.
+     * The PnvPHB3 is the owner (parent) of the bus.
+     */
+    if (dev && dev->parent_bus) {
+        phb = PNV_PHB3(dev->parent_bus->parent);
+    }
 
     if (!phb) {
         error_setg(errp,
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index ffd9d8a947..bae9398d86 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1782,12 +1782,16 @@ static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
 {
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
     PCIDevice *pci = PCI_DEVICE(dev);
-    PCIBus *bus = pci_get_bus(pci);
     PnvPHB4 *phb = NULL;
     Error *local_err = NULL;
 
-    phb = (PnvPHB4 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
-                                          TYPE_PNV_PHB4);
+    /*
+     * dev->parent_bus gives access to the pnv-phb-root bus.
+     * The PnvPHB4 is the owner (parent) of the bus.
+     */
+    if (dev && dev->parent_bus) {
+        phb = PNV_PHB4(dev->parent_bus->parent);
+    }
 
     if (!phb) {
         error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);
-- 
2.36.1

Re: [PATCH 03/11] ppc/pnv: use dev->parent_bus->parent to get the PHB

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> It is not advisable to execute an object_dynamic_cast() to poke into
> bus->qbus.parent and follow it up with a C cast into the PnvPHB type we
> think we got.
> 
> A better way is to access the PnvPHB object via a QOM macro accessing
> the existing parent links of the DeviceState. For a given
> pnv-phb3/4-root-port 'dev', dev->parent_bus will give us the PHB bus,
> and dev->parent_bus->parent is the PHB. Use the adequate QOM macro to
> assert the type, and keep the NULL check in case we didn't get the
> object we were expecting.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/pci-host/pnv_phb3.c | 10 +++++++---
>   hw/pci-host/pnv_phb4.c | 10 +++++++---
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 4ba660f8b9..7901d8172c 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1139,12 +1139,16 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PCIDevice *pci = PCI_DEVICE(dev);
> -    PCIBus *bus = pci_get_bus(pci);
>       PnvPHB3 *phb = NULL;
>       Error *local_err = NULL;
>   
> -    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> -                                          TYPE_PNV_PHB3);
> +    /*
> +     * dev->parent_bus gives access to the pnv-phb-root bus.
> +     * The PnvPHB3 is the owner (parent) of the bus.
> +     */
> +    if (dev && dev->parent_bus) {
> +        phb = PNV_PHB3(dev->parent_bus->parent);
> +    }
>   
>       if (!phb) {
>           error_setg(errp,
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index ffd9d8a947..bae9398d86 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1782,12 +1782,16 @@ static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PCIDevice *pci = PCI_DEVICE(dev);
> -    PCIBus *bus = pci_get_bus(pci);
>       PnvPHB4 *phb = NULL;
>       Error *local_err = NULL;
>   
> -    phb = (PnvPHB4 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> -                                          TYPE_PNV_PHB4);
> +    /*
> +     * dev->parent_bus gives access to the pnv-phb-root bus.
> +     * The PnvPHB4 is the owner (parent) of the bus.
> +     */
> +    if (dev && dev->parent_bus) {


Does it make sense to test 'dev' first when it's the device being realized?

   Fred




> +        phb = PNV_PHB4(dev->parent_bus->parent);
> +    }
>   
>       if (!phb) {
>           error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);

Re: [PATCH 03/11] ppc/pnv: use dev->parent_bus->parent to get the PHB

[Daniel Henrique Barboza]

On 6/14/22 06:10, Frederic Barrat wrote:
> 
> 
> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>> It is not advisable to execute an object_dynamic_cast() to poke into
>> bus->qbus.parent and follow it up with a C cast into the PnvPHB type we
>> think we got.
>>
>> A better way is to access the PnvPHB object via a QOM macro accessing
>> the existing parent links of the DeviceState. For a given
>> pnv-phb3/4-root-port 'dev', dev->parent_bus will give us the PHB bus,
>> and dev->parent_bus->parent is the PHB. Use the adequate QOM macro to
>> assert the type, and keep the NULL check in case we didn't get the
>> object we were expecting.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
>>   hw/pci-host/pnv_phb3.c | 10 +++++++---
>>   hw/pci-host/pnv_phb4.c | 10 +++++++---
>>   2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index 4ba660f8b9..7901d8172c 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -1139,12 +1139,16 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
>>   {
>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>>       PCIDevice *pci = PCI_DEVICE(dev);
>> -    PCIBus *bus = pci_get_bus(pci);
>>       PnvPHB3 *phb = NULL;
>>       Error *local_err = NULL;
>> -    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
>> -                                          TYPE_PNV_PHB3);
>> +    /*
>> +     * dev->parent_bus gives access to the pnv-phb-root bus.
>> +     * The PnvPHB3 is the owner (parent) of the bus.
>> +     */
>> +    if (dev && dev->parent_bus) {
>> +        phb = PNV_PHB3(dev->parent_bus->parent);
>> +    }
>>       if (!phb) {
>>           error_setg(errp,
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index ffd9d8a947..bae9398d86 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1782,12 +1782,16 @@ static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
>>   {
>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>>       PCIDevice *pci = PCI_DEVICE(dev);
>> -    PCIBus *bus = pci_get_bus(pci);
>>       PnvPHB4 *phb = NULL;
>>       Error *local_err = NULL;
>> -    phb = (PnvPHB4 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
>> -                                          TYPE_PNV_PHB4);
>> +    /*
>> +     * dev->parent_bus gives access to the pnv-phb-root bus.
>> +     * The PnvPHB4 is the owner (parent) of the bus.
>> +     */
>> +    if (dev && dev->parent_bus) {
> 
> 
> Does it make sense to test 'dev' first when it's the device being realized?

Hmmm not really. I got overzealous here it seems.

I'll keep just the check for dev->parent in v2.


Thanks,


Daniel

> 
>    Fred
> 
> 
> 
> 
>> +        phb = PNV_PHB4(dev->parent_bus->parent);
>> +    }
>>       if (!phb) {
>>           error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);
> 

Re: [PATCH 03/11] ppc/pnv: use dev->parent_bus->parent to get the PHB

[Cédric Le Goater]

On 6/13/22 17:44, Daniel Henrique Barboza wrote:
> It is not advisable to execute an object_dynamic_cast() to poke into
> bus->qbus.parent and follow it up with a C cast into the PnvPHB type we
> think we got.
> 
> A better way is to access the PnvPHB object via a QOM macro accessing
> the existing parent links of the DeviceState. For a given
> pnv-phb3/4-root-port 'dev', dev->parent_bus will give us the PHB bus,
> and dev->parent_bus->parent is the PHB. Use the adequate QOM macro to
> assert the type, and keep the NULL check in case we didn't get the
> object we were expecting.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/pci-host/pnv_phb3.c | 10 +++++++---
>   hw/pci-host/pnv_phb4.c | 10 +++++++---
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 4ba660f8b9..7901d8172c 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1139,12 +1139,16 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PCIDevice *pci = PCI_DEVICE(dev);
> -    PCIBus *bus = pci_get_bus(pci);
>       PnvPHB3 *phb = NULL;
>       Error *local_err = NULL;
>   
> -    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> -                                          TYPE_PNV_PHB3);
> +    /*
> +     * dev->parent_bus gives access to the pnv-phb-root bus.
> +     * The PnvPHB3 is the owner (parent) of the bus.
> +     */
> +    if (dev && dev->parent_bus) {
> +        phb = PNV_PHB3(dev->parent_bus->parent);
> +    }
>

Couldn't we simply use :

       phb = PNV_PHB3(bus);

?

Thanks,

C.

>       if (!phb) {
>           error_setg(errp,
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index ffd9d8a947..bae9398d86 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1782,12 +1782,16 @@ static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PCIDevice *pci = PCI_DEVICE(dev);
> -    PCIBus *bus = pci_get_bus(pci);
>       PnvPHB4 *phb = NULL;
>       Error *local_err = NULL;
>   
> -    phb = (PnvPHB4 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> -                                          TYPE_PNV_PHB4);
> +    /*
> +     * dev->parent_bus gives access to the pnv-phb-root bus.
> +     * The PnvPHB4 is the owner (parent) of the bus.
> +     */
> +    if (dev && dev->parent_bus) {
> +        phb = PNV_PHB4(dev->parent_bus->parent);
> +    }
>   
>       if (!phb) {
>           error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);

Re: [PATCH 03/11] ppc/pnv: use dev->parent_bus->parent to get the PHB

[Daniel Henrique Barboza]

On 6/14/22 09:10, Cédric Le Goater wrote:
> On 6/13/22 17:44, Daniel Henrique Barboza wrote:
>> It is not advisable to execute an object_dynamic_cast() to poke into
>> bus->qbus.parent and follow it up with a C cast into the PnvPHB type we
>> think we got.
>>
>> A better way is to access the PnvPHB object via a QOM macro accessing
>> the existing parent links of the DeviceState. For a given
>> pnv-phb3/4-root-port 'dev', dev->parent_bus will give us the PHB bus,
>> and dev->parent_bus->parent is the PHB. Use the adequate QOM macro to
>> assert the type, and keep the NULL check in case we didn't get the
>> object we were expecting.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
>>   hw/pci-host/pnv_phb3.c | 10 +++++++---
>>   hw/pci-host/pnv_phb4.c | 10 +++++++---
>>   2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index 4ba660f8b9..7901d8172c 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -1139,12 +1139,16 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
>>   {
>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>>       PCIDevice *pci = PCI_DEVICE(dev);
>> -    PCIBus *bus = pci_get_bus(pci);
>>       PnvPHB3 *phb = NULL;
>>       Error *local_err = NULL;
>> -    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
>> -                                          TYPE_PNV_PHB3);
>> +    /*
>> +     * dev->parent_bus gives access to the pnv-phb-root bus.
>> +     * The PnvPHB3 is the owner (parent) of the bus.
>> +     */
>> +    if (dev && dev->parent_bus) {
>> +        phb = PNV_PHB3(dev->parent_bus->parent);
>> +    }
>>
> 
> Couldn't we simply use :
> 
>        phb = PNV_PHB3(bus);
> 
> ?

No. This will give us a reference to a pnv-phb3-root object.


Getting a reference to the PHB by using bus->parent happens in other parts of
code, such as:


hw/pci-host/gpex-acpi.c:            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
hw/pci-bridge/pci_expander_bridge.c:    main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);


So I believe we're not out of line here.


Thanks,


Daniel


> 
> Thanks,
> 
> C.
> 
>>       if (!phb) {
>>           error_setg(errp,
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index ffd9d8a947..bae9398d86 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1782,12 +1782,16 @@ static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
>>   {
>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>>       PCIDevice *pci = PCI_DEVICE(dev);
>> -    PCIBus *bus = pci_get_bus(pci);
>>       PnvPHB4 *phb = NULL;
>>       Error *local_err = NULL;
>> -    phb = (PnvPHB4 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
>> -                                          TYPE_PNV_PHB4);
>> +    /*
>> +     * dev->parent_bus gives access to the pnv-phb-root bus.
>> +     * The PnvPHB4 is the owner (parent) of the bus.
>> +     */
>> +    if (dev && dev->parent_bus) {
>> +        phb = PNV_PHB4(dev->parent_bus->parent);
>> +    }
>>       if (!phb) {
>>           error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);
> 
> 

[PATCH 04/11] ppc/pnv: use dev instead of pci->qdev in root_port_realize()

[Daniel Henrique Barboza]

We already have access to the 'dev' object.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/pci-host/pnv_phb3.c | 4 ++--
 hw/pci-host/pnv_phb4.c | 5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 7901d8172c..bda23fd20b 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1157,8 +1157,8 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
     }
 
     /* Set unique chassis/slot values for the root port */
-    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
-    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
+    qdev_prop_set_uint8(dev, "chassis", phb->chip_id);
+    qdev_prop_set_uint16(dev, "slot", phb->phb_id);
 
     rpc->parent_realize(dev, &local_err);
     if (local_err) {
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index bae9398d86..bfec8b9f6d 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1781,7 +1781,6 @@ static void pnv_phb4_root_port_reset(DeviceState *dev)
 static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
 {
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
-    PCIDevice *pci = PCI_DEVICE(dev);
     PnvPHB4 *phb = NULL;
     Error *local_err = NULL;
 
@@ -1799,8 +1798,8 @@ static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
     }
 
     /* Set unique chassis/slot values for the root port */
-    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
-    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
+    qdev_prop_set_uint8(dev, "chassis", phb->chip_id);
+    qdev_prop_set_uint16(dev, "slot", phb->phb_id);
 
     rpc->parent_realize(dev, &local_err);
     if (local_err) {
-- 
2.36.1

Re: [PATCH 04/11] ppc/pnv: use dev instead of pci->qdev in root_port_realize()

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> We already have access to the 'dev' object.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---


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

   Fred


>   hw/pci-host/pnv_phb3.c | 4 ++--
>   hw/pci-host/pnv_phb4.c | 5 ++---
>   2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 7901d8172c..bda23fd20b 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1157,8 +1157,8 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
>       }
>   
>       /* Set unique chassis/slot values for the root port */
> -    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
> -    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
> +    qdev_prop_set_uint8(dev, "chassis", phb->chip_id);
> +    qdev_prop_set_uint16(dev, "slot", phb->phb_id);
>   
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index bae9398d86..bfec8b9f6d 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1781,7 +1781,6 @@ static void pnv_phb4_root_port_reset(DeviceState *dev)
>   static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
> -    PCIDevice *pci = PCI_DEVICE(dev);
>       PnvPHB4 *phb = NULL;
>       Error *local_err = NULL;
>   
> @@ -1799,8 +1798,8 @@ static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
>       }
>   
>       /* Set unique chassis/slot values for the root port */
> -    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
> -    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
> +    qdev_prop_set_uint8(dev, "chassis", phb->chip_id);
> +    qdev_prop_set_uint16(dev, "slot", phb->phb_id);
>   
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {

[PATCH 05/11] ppc/pnv: make pnv_ics_get() use the chip8->phbs[] array

[Daniel Henrique Barboza]

The function is working today by getting all the child objects of the
chip, interacting with each of them to check whether the child is a PHB,
and then doing what needs to be done.

We have all the chip PHBs in the phbs[] array so interacting with all
child objects is unneeded.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/ppc/pnv.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 40e0cbd84d..05a8d5034f 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1944,41 +1944,39 @@ typedef struct ForeachPhb3Args {
     ICSState *ics;
 } ForeachPhb3Args;
 
-static int pnv_ics_get_child(Object *child, void *opaque)
+static void pnv_ics_get_phb_ics(PnvPHB3 *phb3, ForeachPhb3Args *args)
 {
-    ForeachPhb3Args *args = opaque;
-    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
+    if (ics_valid_irq(&phb3->lsis, args->irq)) {
+        args->ics = &phb3->lsis;
+    }
 
-    if (phb3) {
-        if (ics_valid_irq(&phb3->lsis, args->irq)) {
-            args->ics = &phb3->lsis;
-        }
-        if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
-            args->ics = ICS(&phb3->msis);
-        }
+    if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
+        args->ics = ICS(&phb3->msis);
     }
-    return args->ics ? 1 : 0;
 }
 
 static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
 {
     PnvMachineState *pnv = PNV_MACHINE(xi);
     ForeachPhb3Args args = { irq, NULL };
-    int i;
+    int i, j;
 
     for (i = 0; i < pnv->num_chips; i++) {
-        PnvChip *chip = pnv->chips[i];
         Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
 
         if (ics_valid_irq(&chip8->psi.ics, irq)) {
             return &chip8->psi.ics;
         }
 
-        object_child_foreach(OBJECT(chip), pnv_ics_get_child, &args);
-        if (args.ics) {
-            return args.ics;
+        for (j = 0; j < chip8->num_phbs; j++) {
+            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
+
+            if (args.ics) {
+                return args.ics;
+            }
         }
     }
+
     return NULL;
 }
 
-- 
2.36.1

Re: [PATCH 05/11] ppc/pnv: make pnv_ics_get() use the chip8->phbs[] array

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> The function is working today by getting all the child objects of the
> chip, interacting with each of them to check whether the child is a PHB,
> and then doing what needs to be done.
> 
> We have all the chip PHBs in the phbs[] array so interacting with all
> child objects is unneeded.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/ppc/pnv.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 40e0cbd84d..05a8d5034f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1944,41 +1944,39 @@ typedef struct ForeachPhb3Args {
>       ICSState *ics;
>   } ForeachPhb3Args;
>   
> -static int pnv_ics_get_child(Object *child, void *opaque)
> +static void pnv_ics_get_phb_ics(PnvPHB3 *phb3, ForeachPhb3Args *args)
>   {
> -    ForeachPhb3Args *args = opaque;
> -    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
> +    if (ics_valid_irq(&phb3->lsis, args->irq)) {
> +        args->ics = &phb3->lsis;
> +    }
>   
> -    if (phb3) {
> -        if (ics_valid_irq(&phb3->lsis, args->irq)) {
> -            args->ics = &phb3->lsis;
> -        }
> -        if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
> -            args->ics = ICS(&phb3->msis);
> -        }
> +    if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
> +        args->ics = ICS(&phb3->msis);
>       }
> -    return args->ics ? 1 : 0;
>   }


It seems that we could gain in readability by dropping the 
ForeachPhb3Args structure completely.
The 'irq' member can just be an input argument to the function instead 
of the full structure.
The 'ics' member is no longer needed, it can be the returned value of 
the function (instead of void)

   Fred


>   
>   static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>   {
>       PnvMachineState *pnv = PNV_MACHINE(xi);
>       ForeachPhb3Args args = { irq, NULL };
> -    int i;
> +    int i, j;
>   
>       for (i = 0; i < pnv->num_chips; i++) {
> -        PnvChip *chip = pnv->chips[i];
>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>   
>           if (ics_valid_irq(&chip8->psi.ics, irq)) {
>               return &chip8->psi.ics;
>           }
>   
> -        object_child_foreach(OBJECT(chip), pnv_ics_get_child, &args);
> -        if (args.ics) {
> -            return args.ics;
> +        for (j = 0; j < chip8->num_phbs; j++) {
> +            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
> +
> +            if (args.ics) {
> +                return args.ics;
> +            }
>           }
>       }
> +
>       return NULL;
>   }
>   

Re: [PATCH 05/11] ppc/pnv: make pnv_ics_get() use the chip8->phbs[] array

[Cédric Le Goater]

On 6/13/22 17:44, Daniel Henrique Barboza wrote:
> The function is working today by getting all the child objects of the
> chip, interacting with each of them to check whether the child is a PHB,
> and then doing what needs to be done.
> 
> We have all the chip PHBs in the phbs[] array so interacting with all
> child objects is unneeded.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/ppc/pnv.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 40e0cbd84d..05a8d5034f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1944,41 +1944,39 @@ typedef struct ForeachPhb3Args {
>       ICSState *ics;
>   } ForeachPhb3Args;
>   
> -static int pnv_ics_get_child(Object *child, void *opaque)
> +static void pnv_ics_get_phb_ics(PnvPHB3 *phb3, ForeachPhb3Args *args)
>   {
> -    ForeachPhb3Args *args = opaque;
> -    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
> +    if (ics_valid_irq(&phb3->lsis, args->irq)) {
> +        args->ics = &phb3->lsis;
> +    }
>   
> -    if (phb3) {
> -        if (ics_valid_irq(&phb3->lsis, args->irq)) {
> -            args->ics = &phb3->lsis;
> -        }
> -        if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
> -            args->ics = ICS(&phb3->msis);
> -        }
> +    if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
> +        args->ics = ICS(&phb3->msis);
>       }
> -    return args->ics ? 1 : 0;
>   }
>   
>   static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>   {
>       PnvMachineState *pnv = PNV_MACHINE(xi);
>       ForeachPhb3Args args = { irq, NULL };
> -    int i;
> +    int i, j;
>   
>       for (i = 0; i < pnv->num_chips; i++) {
> -        PnvChip *chip = pnv->chips[i];
>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>   
>           if (ics_valid_irq(&chip8->psi.ics, irq)) {
>               return &chip8->psi.ics;
>           }
>   
> -        object_child_foreach(OBJECT(chip), pnv_ics_get_child, &args);
> -        if (args.ics) {
> -            return args.ics;
> +        for (j = 0; j < chip8->num_phbs; j++) {
> +            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);

If we don't need this function elsewhere, why keep it ?

Thanks,

C.

> +
> +            if (args.ics) {
> +                return args.ics;
> +            }
>           }
>       }
> +
>       return NULL;
>   }
>   

[PATCH 06/11] ppc/pnv: make pnv_ics_resend() use chip8->phbs[]

[Daniel Henrique Barboza]

pnv_ics_resend() is scrolling through all the child objects of the chip
to search for the PHBs. It's faster and simpler to just use the phbs[]
array.

pnv_ics_resend_child() was folded into pnv_ics_resend() since it's too
simple to justify its own function.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/ppc/pnv.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 05a8d5034f..d70deffa1d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1993,28 +1993,20 @@ PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
     return NULL;
 }
 
-static int pnv_ics_resend_child(Object *child, void *opaque)
-{
-    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
-
-    if (phb3) {
-        ics_resend(&phb3->lsis);
-        ics_resend(ICS(&phb3->msis));
-    }
-    return 0;
-}
-
 static void pnv_ics_resend(XICSFabric *xi)
 {
     PnvMachineState *pnv = PNV_MACHINE(xi);
-    int i;
+    int i, j;
 
     for (i = 0; i < pnv->num_chips; i++) {
-        PnvChip *chip = pnv->chips[i];
         Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
 
-        ics_resend(&chip8->psi.ics);
-        object_child_foreach(OBJECT(chip), pnv_ics_resend_child, NULL);
+        for (j = 0; j < chip8->num_phbs; j++) {
+            PnvPHB3 *phb3 = &chip8->phbs[j];
+
+            ics_resend(&phb3->lsis);
+            ics_resend(ICS(&phb3->msis));
+        }
     }
 }
 
-- 
2.36.1

Re: [PATCH 06/11] ppc/pnv: make pnv_ics_resend() use chip8->phbs[]

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> pnv_ics_resend() is scrolling through all the child objects of the chip
> to search for the PHBs. It's faster and simpler to just use the phbs[]
> array.
> 
> pnv_ics_resend_child() was folded into pnv_ics_resend() since it's too
> simple to justify its own function.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/ppc/pnv.c | 22 +++++++---------------
>   1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 05a8d5034f..d70deffa1d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1993,28 +1993,20 @@ PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
>       return NULL;
>   }
>   
> -static int pnv_ics_resend_child(Object *child, void *opaque)
> -{
> -    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
> -
> -    if (phb3) {
> -        ics_resend(&phb3->lsis);
> -        ics_resend(ICS(&phb3->msis));
> -    }
> -    return 0;
> -}
> -
>   static void pnv_ics_resend(XICSFabric *xi)
>   {
>       PnvMachineState *pnv = PNV_MACHINE(xi);
> -    int i;
> +    int i, j;
>   
>       for (i = 0; i < pnv->num_chips; i++) {
> -        PnvChip *chip = pnv->chips[i];
>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>   
> -        ics_resend(&chip8->psi.ics);


That line shouldn't be dropped, right?

   Fred


> -        object_child_foreach(OBJECT(chip), pnv_ics_resend_child, NULL);
> +        for (j = 0; j < chip8->num_phbs; j++) {
> +            PnvPHB3 *phb3 = &chip8->phbs[j];
> +
> +            ics_resend(&phb3->lsis);
> +            ics_resend(ICS(&phb3->msis));
> +        }
>       }
>   }
>   

Re: [PATCH 06/11] ppc/pnv: make pnv_ics_resend() use chip8->phbs[]

[Cédric Le Goater]

On 6/14/22 11:24, Frederic Barrat wrote:
> 
> 
> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>> pnv_ics_resend() is scrolling through all the child objects of the chip
>> to search for the PHBs. It's faster and simpler to just use the phbs[]
>> array.
>>
>> pnv_ics_resend_child() was folded into pnv_ics_resend() since it's too
>> simple to justify its own function.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
>>   hw/ppc/pnv.c | 22 +++++++---------------
>>   1 file changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 05a8d5034f..d70deffa1d 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1993,28 +1993,20 @@ PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
>>       return NULL;
>>   }
>> -static int pnv_ics_resend_child(Object *child, void *opaque)
>> -{
>> -    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
>> -
>> -    if (phb3) {
>> -        ics_resend(&phb3->lsis);
>> -        ics_resend(ICS(&phb3->msis));
>> -    }
>> -    return 0;
>> -}
>> -
>>   static void pnv_ics_resend(XICSFabric *xi)
>>   {
>>       PnvMachineState *pnv = PNV_MACHINE(xi);
>> -    int i;
>> +    int i, j;
>>       for (i = 0; i < pnv->num_chips; i++) {
>> -        PnvChip *chip = pnv->chips[i];
>>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>> -        ics_resend(&chip8->psi.ics);
> 
> 
> That line shouldn't be dropped, right?

yes. that's a typo, which should break the console.

C.


> 
>    Fred
> 
> 
>> -        object_child_foreach(OBJECT(chip), pnv_ics_resend_child, NULL);
>> +        for (j = 0; j < chip8->num_phbs; j++) {
>> +            PnvPHB3 *phb3 = &chip8->phbs[j];
>> +
>> +            ics_resend(&phb3->lsis);
>> +            ics_resend(ICS(&phb3->msis));
>> +        }
>>       }
>>   }

Re: [PATCH 06/11] ppc/pnv: make pnv_ics_resend() use chip8->phbs[]

[Daniel Henrique Barboza]

On 6/14/22 06:24, Frederic Barrat wrote:
> 
> 
> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>> pnv_ics_resend() is scrolling through all the child objects of the chip
>> to search for the PHBs. It's faster and simpler to just use the phbs[]
>> array.
>>
>> pnv_ics_resend_child() was folded into pnv_ics_resend() since it's too
>> simple to justify its own function.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
>>   hw/ppc/pnv.c | 22 +++++++---------------
>>   1 file changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 05a8d5034f..d70deffa1d 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1993,28 +1993,20 @@ PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
>>       return NULL;
>>   }
>> -static int pnv_ics_resend_child(Object *child, void *opaque)
>> -{
>> -    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
>> -
>> -    if (phb3) {
>> -        ics_resend(&phb3->lsis);
>> -        ics_resend(ICS(&phb3->msis));
>> -    }
>> -    return 0;
>> -}
>> -
>>   static void pnv_ics_resend(XICSFabric *xi)
>>   {
>>       PnvMachineState *pnv = PNV_MACHINE(xi);
>> -    int i;
>> +    int i, j;
>>       for (i = 0; i < pnv->num_chips; i++) {
>> -        PnvChip *chip = pnv->chips[i];
>>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>> -        ics_resend(&chip8->psi.ics);
> 
> 
> That line shouldn't be dropped, right?

oooffff. It shouldn't. I'll fix it in the v2.


It didn't break anything I could see though. OS boots with network
with ping ....



Daniel

> 
>    Fred
> 
> 
>> -        object_child_foreach(OBJECT(chip), pnv_ics_resend_child, NULL);
>> +        for (j = 0; j < chip8->num_phbs; j++) {
>> +            PnvPHB3 *phb3 = &chip8->phbs[j];
>> +
>> +            ics_resend(&phb3->lsis);
>> +            ics_resend(ICS(&phb3->msis));
>> +        }
>>       }
>>   }
> 

[PATCH 07/11] ppc/pnv: make pnv_chip_power8_pic_print_info() use chip8->phbs[]

[Daniel Henrique Barboza]

It's inneficient to scroll all child objects when we have all PHBs
available in chip8->phbs[].

pnv_chip_power8_pic_print_info_child() ended up folded into
pic_print_info() for simplicity.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/ppc/pnv.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d70deffa1d..5e3323e950 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -652,25 +652,19 @@ static ISABus *pnv_isa_create(PnvChip *chip, Error **errp)
     return PNV_CHIP_GET_CLASS(chip)->isa_create(chip, errp);
 }
 
-static int pnv_chip_power8_pic_print_info_child(Object *child, void *opaque)
-{
-    Monitor *mon = opaque;
-    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
-
-    if (phb3) {
-        pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
-        ics_pic_print_info(&phb3->lsis, mon);
-    }
-    return 0;
-}
-
 static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
 {
     Pnv8Chip *chip8 = PNV8_CHIP(chip);
+    int i;
 
     ics_pic_print_info(&chip8->psi.ics, mon);
-    object_child_foreach(OBJECT(chip),
-                         pnv_chip_power8_pic_print_info_child, mon);
+
+    for (i = 0; i < chip8->num_phbs; i++) {
+        PnvPHB3 *phb3 = &chip8->phbs[i];
+
+        pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
+        ics_pic_print_info(&phb3->lsis, mon);
+    }
 }
 
 static int pnv_chip_power9_pic_print_info_child(Object *child, void *opaque)
-- 
2.36.1

Re: [PATCH 07/11] ppc/pnv: make pnv_chip_power8_pic_print_info() use chip8->phbs[]

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> It's inneficient to scroll all child objects when we have all PHBs
> available in chip8->phbs[].
> 
> pnv_chip_power8_pic_print_info_child() ended up folded into
> pic_print_info() for simplicity.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---


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

   Fred


>   hw/ppc/pnv.c | 22 ++++++++--------------
>   1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d70deffa1d..5e3323e950 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -652,25 +652,19 @@ static ISABus *pnv_isa_create(PnvChip *chip, Error **errp)
>       return PNV_CHIP_GET_CLASS(chip)->isa_create(chip, errp);
>   }
>   
> -static int pnv_chip_power8_pic_print_info_child(Object *child, void *opaque)
> -{
> -    Monitor *mon = opaque;
> -    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
> -
> -    if (phb3) {
> -        pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
> -        ics_pic_print_info(&phb3->lsis, mon);
> -    }
> -    return 0;
> -}
> -
>   static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>   {
>       Pnv8Chip *chip8 = PNV8_CHIP(chip);
> +    int i;
>   
>       ics_pic_print_info(&chip8->psi.ics, mon);
> -    object_child_foreach(OBJECT(chip),
> -                         pnv_chip_power8_pic_print_info_child, mon);
> +
> +    for (i = 0; i < chip8->num_phbs; i++) {
> +        PnvPHB3 *phb3 = &chip8->phbs[i];
> +
> +        pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
> +        ics_pic_print_info(&phb3->lsis, mon);
> +    }
>   }
>   
>   static int pnv_chip_power9_pic_print_info_child(Object *child, void *opaque)

[PATCH 08/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB3* array

[Daniel Henrique Barboza]

When enabling user created PHBs (a change reverted by commit 9c10d86fee)
we were handling PHBs created by default versus by the user in different
manners. The only difference between these PHBs is that one will have a
valid phb3->chip that is assigned during pnv_chip_power8_realize(),
while the user created needs to search which chip it belongs to.

Aside from that there shouldn't be any difference. Making the default
PHBs behave in line with the user created ones will make it easier to
re-introduce them later on. It will also make the code easier to follow
since we are dealing with them in equal manner.

The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
This will allow us to assign user created PHBs into it later on. The way
we initilize the default case is now more in line with that would happen
with the user created case: the object is created, parented by the chip
because pnv_xscom_dt() relies on it, and then assigned to the array.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/ppc/pnv.c         | 19 ++++++++++++++-----
 include/hw/ppc/pnv.h |  6 +++++-
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 5e3323e950..6ce9e94e05 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
     ics_pic_print_info(&chip8->psi.ics, mon);
 
     for (i = 0; i < chip8->num_phbs; i++) {
-        PnvPHB3 *phb3 = &chip8->phbs[i];
+        PnvPHB3 *phb3 = chip8->phbs[i];
 
         pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
         ics_pic_print_info(&phb3->lsis, mon);
@@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
     chip8->num_phbs = pcc->num_phbs;
 
     for (i = 0; i < chip8->num_phbs; i++) {
-        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
+        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
+
+        /*
+         * We need the chip to parent the PHB to allow the DT
+         * to build correctly (via pnv_xscom_dt()).
+         *
+         * TODO: the PHB should be parented by a PEC device.
+         */
+        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
+        chip8->phbs[i] = phb3;
     }
 
 }
@@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
 
     /* PHB3 controllers */
     for (i = 0; i < chip8->num_phbs; i++) {
-        PnvPHB3 *phb = &chip8->phbs[i];
+        PnvPHB3 *phb = chip8->phbs[i];
 
         object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
         object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
@@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
         }
 
         for (j = 0; j < chip8->num_phbs; j++) {
-            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
+            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
 
             if (args.ics) {
                 return args.ics;
@@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
         Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
 
         for (j = 0; j < chip8->num_phbs; j++) {
-            PnvPHB3 *phb3 = &chip8->phbs[j];
+            PnvPHB3 *phb3 = chip8->phbs[j];
 
             ics_resend(&phb3->lsis);
             ics_resend(ICS(&phb3->msis));
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 033890a23f..11f1089289 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -80,7 +80,11 @@ struct Pnv8Chip {
     PnvHomer     homer;
 
 #define PNV8_CHIP_PHB3_MAX 4
-    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
+    /*
+     * The array is used to allow quick access to the phbs by
+     * pnv_ics_get_child() and pnv_ics_resend_child().
+     */
+    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
     uint32_t     num_phbs;
 
     XICSFabric    *xics;
-- 
2.36.1

Re: [PATCH 08/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB3* array

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> When enabling user created PHBs (a change reverted by commit 9c10d86fee)
> we were handling PHBs created by default versus by the user in different
> manners. The only difference between these PHBs is that one will have a
> valid phb3->chip that is assigned during pnv_chip_power8_realize(),
> while the user created needs to search which chip it belongs to.
> 
> Aside from that there shouldn't be any difference. Making the default
> PHBs behave in line with the user created ones will make it easier to
> re-introduce them later on. It will also make the code easier to follow
> since we are dealing with them in equal manner.
> 
> The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
> This will allow us to assign user created PHBs into it later on. The way
> we initilize the default case is now more in line with that would happen
> with the user created case: the object is created, parented by the chip
> because pnv_xscom_dt() relies on it, and then assigned to the array.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---


This patch is more prep work for the user-created device instead of 
general cleanup like the previous ones, but I don't see anything wrong 
with it. So:

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

   Fred



>   hw/ppc/pnv.c         | 19 ++++++++++++++-----
>   include/hw/ppc/pnv.h |  6 +++++-
>   2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 5e3323e950..6ce9e94e05 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>       ics_pic_print_info(&chip8->psi.ics, mon);
>   
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb3 = &chip8->phbs[i];
> +        PnvPHB3 *phb3 = chip8->phbs[i];
>   
>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
>           ics_pic_print_info(&phb3->lsis, mon);
> @@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
>       chip8->num_phbs = pcc->num_phbs;
>   
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
> +        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
> +
> +        /*
> +         * We need the chip to parent the PHB to allow the DT
> +         * to build correctly (via pnv_xscom_dt()).
> +         *
> +         * TODO: the PHB should be parented by a PEC device.
> +         */
> +        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
> +        chip8->phbs[i] = phb3;
>       }
>   
>   }
> @@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>   
>       /* PHB3 controllers */
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb = &chip8->phbs[i];
> +        PnvPHB3 *phb = chip8->phbs[i];
>   
>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
> @@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>           }
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
> +            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
>   
>               if (args.ics) {
>                   return args.ics;
> @@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            PnvPHB3 *phb3 = &chip8->phbs[j];
> +            PnvPHB3 *phb3 = chip8->phbs[j];
>   
>               ics_resend(&phb3->lsis);
>               ics_resend(ICS(&phb3->msis));
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 033890a23f..11f1089289 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -80,7 +80,11 @@ struct Pnv8Chip {
>       PnvHomer     homer;
>   
>   #define PNV8_CHIP_PHB3_MAX 4
> -    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
> +    /*
> +     * The array is used to allow quick access to the phbs by
> +     * pnv_ics_get_child() and pnv_ics_resend_child().
> +     */
> +    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
>       uint32_t     num_phbs;
>   
>       XICSFabric    *xics;

Re: [PATCH 08/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB3* array

[Daniel Henrique Barboza]

On 6/14/22 06:53, Frederic Barrat wrote:
> 
> 
> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>> When enabling user created PHBs (a change reverted by commit 9c10d86fee)
>> we were handling PHBs created by default versus by the user in different
>> manners. The only difference between these PHBs is that one will have a
>> valid phb3->chip that is assigned during pnv_chip_power8_realize(),
>> while the user created needs to search which chip it belongs to.
>>
>> Aside from that there shouldn't be any difference. Making the default
>> PHBs behave in line with the user created ones will make it easier to
>> re-introduce them later on. It will also make the code easier to follow
>> since we are dealing with them in equal manner.
>>
>> The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
>> This will allow us to assign user created PHBs into it later on. The way
>> we initilize the default case is now more in line with that would happen
>> with the user created case: the object is created, parented by the chip
>> because pnv_xscom_dt() relies on it, and then assigned to the array.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
> 
> 
> This patch is more prep work for the user-created device instead of general cleanup like the previous ones, but I don't see anything wrong with it. So:
> 
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


I've been thinking about it and I guess I could do better with this
and the proxy pnv-phb series that is already in v2. What I'm thinking
is:

- crop patches 8-11 from this series. Patches 1-7 would be the prep cleanup
series;

- split the pnv-phb series in two:

   - first series will just introduce the pnv-phb devices and consolidate the
root ports. We're going to deal just with default devices. No consideration
about future user-created devices will be made;

   - a second series would then re-introduce user creatable phbs and root ports.
Patches 8-11 of this series would be handled in this second patch set since it's
closely related to user devices.


Does that sound fair?


Thanks,


Daniel




> 
>    Fred
> 
> 
> 
>>   hw/ppc/pnv.c         | 19 ++++++++++++++-----
>>   include/hw/ppc/pnv.h |  6 +++++-
>>   2 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 5e3323e950..6ce9e94e05 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>>       ics_pic_print_info(&chip8->psi.ics, mon);
>>       for (i = 0; i < chip8->num_phbs; i++) {
>> -        PnvPHB3 *phb3 = &chip8->phbs[i];
>> +        PnvPHB3 *phb3 = chip8->phbs[i];
>>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
>>           ics_pic_print_info(&phb3->lsis, mon);
>> @@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
>>       chip8->num_phbs = pcc->num_phbs;
>>       for (i = 0; i < chip8->num_phbs; i++) {
>> -        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
>> +        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
>> +
>> +        /*
>> +         * We need the chip to parent the PHB to allow the DT
>> +         * to build correctly (via pnv_xscom_dt()).
>> +         *
>> +         * TODO: the PHB should be parented by a PEC device.
>> +         */
>> +        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
>> +        chip8->phbs[i] = phb3;
>>       }
>>   }
>> @@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>>       /* PHB3 controllers */
>>       for (i = 0; i < chip8->num_phbs; i++) {
>> -        PnvPHB3 *phb = &chip8->phbs[i];
>> +        PnvPHB3 *phb = chip8->phbs[i];
>>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
>> @@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>>           }
>>           for (j = 0; j < chip8->num_phbs; j++) {
>> -            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
>> +            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
>>               if (args.ics) {
>>                   return args.ics;
>> @@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>>           for (j = 0; j < chip8->num_phbs; j++) {
>> -            PnvPHB3 *phb3 = &chip8->phbs[j];
>> +            PnvPHB3 *phb3 = chip8->phbs[j];
>>               ics_resend(&phb3->lsis);
>>               ics_resend(ICS(&phb3->msis));
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 033890a23f..11f1089289 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -80,7 +80,11 @@ struct Pnv8Chip {
>>       PnvHomer     homer;
>>   #define PNV8_CHIP_PHB3_MAX 4
>> -    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>> +    /*
>> +     * The array is used to allow quick access to the phbs by
>> +     * pnv_ics_get_child() and pnv_ics_resend_child().
>> +     */
>> +    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
>>       uint32_t     num_phbs;
>>       XICSFabric    *xics;

Re: [PATCH 08/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB3* array

[Frederic Barrat]

On 14/06/2022 17:39, Daniel Henrique Barboza wrote:
> I've been thinking about it and I guess I could do better with this
> and the proxy pnv-phb series that is already in v2. What I'm thinking
> is:
> 
> - crop patches 8-11 from this series. Patches 1-7 would be the prep cleanup
> series;
> 
> - split the pnv-phb series in two:
> 
>    - first series will just introduce the pnv-phb devices and 
> consolidate the
> root ports. We're going to deal just with default devices. No consideration
> about future user-created devices will be made;
> 
>    - a second series would then re-introduce user creatable phbs and 
> root ports.
> Patches 8-11 of this series would be handled in this second patch set 
> since it's
> closely related to user devices.
> 
> 
> Does that sound fair?


Sounds good to me. That should keep series smaller and easier to review 
and merge.

   Fred

Re: [PATCH 08/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB3* array

[Cédric Le Goater]

On 6/14/22 17:39, Daniel Henrique Barboza wrote:
> 
> 
> On 6/14/22 06:53, Frederic Barrat wrote:
>>
>>
>> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>>> When enabling user created PHBs (a change reverted by commit 9c10d86fee)
>>> we were handling PHBs created by default versus by the user in different
>>> manners. The only difference between these PHBs is that one will have a
>>> valid phb3->chip that is assigned during pnv_chip_power8_realize(),
>>> while the user created needs to search which chip it belongs to.
>>>
>>> Aside from that there shouldn't be any difference. Making the default
>>> PHBs behave in line with the user created ones will make it easier to
>>> re-introduce them later on. It will also make the code easier to follow
>>> since we are dealing with them in equal manner.
>>>
>>> The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
>>> This will allow us to assign user created PHBs into it later on. The way
>>> we initilize the default case is now more in line with that would happen
>>> with the user created case: the object is created, parented by the chip
>>> because pnv_xscom_dt() relies on it, and then assigned to the array.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>>> ---
>>
>>
>> This patch is more prep work for the user-created device instead of general cleanup like the previous ones, but I don't see anything wrong with it. So:
>>
>> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> 
> 
> I've been thinking about it and I guess I could do better with this
> and the proxy pnv-phb series that is already in v2. What I'm thinking
> is:
> 
> - crop patches 8-11 from this series. Patches 1-7 would be the prep cleanup
> series;
> 
> - split the pnv-phb series in two:
> 
>    - first series will just introduce the pnv-phb devices and consolidate the
> root ports. We're going to deal just with default devices. No consideration
> about future user-created devices will be made;

Yes. From what I have read, this looks very feasible with a v2.

Thanks,

C.

> 
>    - a second series would then re-introduce user creatable phbs and root ports.
> Patches 8-11 of this series would be handled in this second patch set since it's
> closely related to user devices.
> 
> 
> Does that sound fair?
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
>>
>>    Fred
>>
>>
>>
>>>   hw/ppc/pnv.c         | 19 ++++++++++++++-----
>>>   include/hw/ppc/pnv.h |  6 +++++-
>>>   2 files changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 5e3323e950..6ce9e94e05 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>>>       ics_pic_print_info(&chip8->psi.ics, mon);
>>>       for (i = 0; i < chip8->num_phbs; i++) {
>>> -        PnvPHB3 *phb3 = &chip8->phbs[i];
>>> +        PnvPHB3 *phb3 = chip8->phbs[i];
>>>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
>>>           ics_pic_print_info(&phb3->lsis, mon);
>>> @@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
>>>       chip8->num_phbs = pcc->num_phbs;
>>>       for (i = 0; i < chip8->num_phbs; i++) {
>>> -        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
>>> +        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
>>> +
>>> +        /*
>>> +         * We need the chip to parent the PHB to allow the DT
>>> +         * to build correctly (via pnv_xscom_dt()).
>>> +         *
>>> +         * TODO: the PHB should be parented by a PEC device.
>>> +         */
>>> +        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
>>> +        chip8->phbs[i] = phb3;
>>>       }
>>>   }
>>> @@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>>>       /* PHB3 controllers */
>>>       for (i = 0; i < chip8->num_phbs; i++) {
>>> -        PnvPHB3 *phb = &chip8->phbs[i];
>>> +        PnvPHB3 *phb = chip8->phbs[i];
>>>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>>>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
>>> @@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>>>           }
>>>           for (j = 0; j < chip8->num_phbs; j++) {
>>> -            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
>>> +            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
>>>               if (args.ics) {
>>>                   return args.ics;
>>> @@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>>>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>>>           for (j = 0; j < chip8->num_phbs; j++) {
>>> -            PnvPHB3 *phb3 = &chip8->phbs[j];
>>> +            PnvPHB3 *phb3 = chip8->phbs[j];
>>>               ics_resend(&phb3->lsis);
>>>               ics_resend(ICS(&phb3->msis));
>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>>> index 033890a23f..11f1089289 100644
>>> --- a/include/hw/ppc/pnv.h
>>> +++ b/include/hw/ppc/pnv.h
>>> @@ -80,7 +80,11 @@ struct Pnv8Chip {
>>>       PnvHomer     homer;
>>>   #define PNV8_CHIP_PHB3_MAX 4
>>> -    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>>> +    /*
>>> +     * The array is used to allow quick access to the phbs by
>>> +     * pnv_ics_get_child() and pnv_ics_resend_child().
>>> +     */
>>> +    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
>>>       uint32_t     num_phbs;
>>>       XICSFabric    *xics;

[PATCH 09/11] ppc/pnv: add PHB object/bus parenting helpers

[Daniel Henrique Barboza]

These helpers are inspired by the changes that were reverted in commit
9c10d86fee "ppc/pnv: Remove user-created PHB{3,4,5} devices". We'll use
them to handle the default case we already support.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/ppc/pnv.c         | 31 +++++++++++++++++++++++++++++++
 include/hw/ppc/pnv.h |  2 ++
 2 files changed, 33 insertions(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 6ce9e94e05..d77c90d64a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1983,6 +1983,37 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
     return NULL;
 }
 
+
+/*
+ * Set the QOM parent of an object child. If the device state
+ * associated with the child has an id, use it as QOM id. Otherwise
+ * use object_typename[index] as QOM id.
+ */
+void pnv_parent_qom_fixup(Object *parent, Object *child, int index)
+{
+    g_autofree char *default_id =
+        g_strdup_printf("%s[%d]", object_get_typename(child), index);
+    const char *dev_id = DEVICE(child)->id;
+
+    if (child->parent == parent) {
+        return;
+    }
+
+    object_ref(child);
+    object_unparent(child);
+    object_property_add_child(parent, dev_id ? dev_id : default_id, child);
+    object_unref(child);
+}
+
+void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *phb_dev)
+{
+    BusState *s = qdev_get_parent_bus(parent);
+
+    if (!qdev_set_parent_bus(phb_dev, s, &error_fatal)) {
+        return;
+    }
+}
+
 PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
 {
     int i;
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 11f1089289..c091f23039 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -194,6 +194,8 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
 
 PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
 void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index);
+void pnv_parent_qom_fixup(Object *parent, Object *child, int index);
+void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *child);
 
 #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
 typedef struct PnvMachineClass PnvMachineClass;
-- 
2.36.1

[PATCH 10/11] ppc/pnv: move PHB3 initialization to realize time

[Daniel Henrique Barboza]

There's nothing special that is being done in
pnv_chip_power8_instance_init() that can't be done during
pnv_chip_power8_realize(). Move the PHB creating and phbs[] assignment
to power8_realize().

We also need to assign a proper phb->chip parent and bus. This is done
by the PHB itself, in pnv_phb3_realize(), in a similar fashion that user
created PHB3s are going to do.

After all this we're left with logic that, aside from phb chip
assignment that are still being done in power8_realize(), behaves the
same for default and user created PHB3s.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/pci-host/pnv_phb3.c | 14 ++++++++++++++
 hw/ppc/pnv.c           | 24 +++++-------------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index bda23fd20b..c1c73fb88d 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -998,6 +998,20 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /*
+     * We need the chip to parent the PHB to allow the DT
+     * to build correctly (via pnv_xscom_dt()).
+     *
+     * TODO: the PHB should be parented by a PHB3 PEC device.
+     */
+    pnv_parent_qom_fixup(OBJECT(phb->chip), OBJECT(phb), phb->phb_id);
+
+    /*
+     * pnv-phb3 buses are child of the main-system-bus, same as
+     * the chip.
+     */
+    pnv_parent_bus_fixup(DEVICE(phb->chip), dev);
+
     /* LSI sources */
     object_property_set_link(OBJECT(&phb->lsis), "xics", OBJECT(pnv),
                              &error_abort);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d77c90d64a..e4080a98e1 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1130,8 +1130,6 @@ static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
 static void pnv_chip_power8_instance_init(Object *obj)
 {
     Pnv8Chip *chip8 = PNV8_CHIP(obj);
-    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
-    int i;
 
     object_property_add_link(obj, "xics", TYPE_XICS_FABRIC,
                              (Object **)&chip8->xics,
@@ -1145,22 +1143,6 @@ static void pnv_chip_power8_instance_init(Object *obj)
     object_initialize_child(obj, "occ", &chip8->occ, TYPE_PNV8_OCC);
 
     object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
-
-    chip8->num_phbs = pcc->num_phbs;
-
-    for (i = 0; i < chip8->num_phbs; i++) {
-        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
-
-        /*
-         * We need the chip to parent the PHB to allow the DT
-         * to build correctly (via pnv_xscom_dt()).
-         *
-         * TODO: the PHB should be parented by a PEC device.
-         */
-        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
-        chip8->phbs[i] = phb3;
-    }
-
 }
 
 static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
@@ -1286,8 +1268,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
                                 &chip8->homer.regs);
 
     /* PHB3 controllers */
+    chip8->num_phbs = pcc->num_phbs;
+
     for (i = 0; i < chip8->num_phbs; i++) {
-        PnvPHB3 *phb = chip8->phbs[i];
+        PnvPHB3 *phb = PNV_PHB3(object_new(TYPE_PNV_PHB3));
+
+        chip8->phbs[i] = phb;
 
         object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
         object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
-- 
2.36.1

Re: [PATCH 10/11] ppc/pnv: move PHB3 initialization to realize time

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> There's nothing special that is being done in
> pnv_chip_power8_instance_init() that can't be done during
> pnv_chip_power8_realize(). Move the PHB creating and phbs[] assignment
> to power8_realize().
> 
> We also need to assign a proper phb->chip parent and bus. This is done
> by the PHB itself, in pnv_phb3_realize(), in a similar fashion that user
> created PHB3s are going to do.
> 
> After all this we're left with logic that, aside from phb chip
> assignment that are still being done in power8_realize(), behaves the
> same for default and user created PHB3s.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/pci-host/pnv_phb3.c | 14 ++++++++++++++
>   hw/ppc/pnv.c           | 24 +++++-------------------
>   2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index bda23fd20b..c1c73fb88d 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -998,6 +998,20 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> +    /*
> +     * We need the chip to parent the PHB to allow the DT
> +     * to build correctly (via pnv_xscom_dt()).
> +     *
> +     * TODO: the PHB should be parented by a PHB3 PEC device.
> +     */
> +    pnv_parent_qom_fixup(OBJECT(phb->chip), OBJECT(phb), phb->phb_id);
> +


Wouldn't we get the same result in a cleaner way by adding the phb as a 
child of the chip in pnv_chip_power8_realize() ? Right next to when the 
PnvPHB3 object pointer is added to the chip8->phbs array

   Fred


> +    /*
> +     * pnv-phb3 buses are child of the main-system-bus, same as
> +     * the chip.
> +     */
> +    pnv_parent_bus_fixup(DEVICE(phb->chip), dev);
> +
>       /* LSI sources */
>       object_property_set_link(OBJECT(&phb->lsis), "xics", OBJECT(pnv),
>                                &error_abort);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d77c90d64a..e4080a98e1 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1130,8 +1130,6 @@ static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
>   static void pnv_chip_power8_instance_init(Object *obj)
>   {
>       Pnv8Chip *chip8 = PNV8_CHIP(obj);
> -    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
> -    int i;
>   
>       object_property_add_link(obj, "xics", TYPE_XICS_FABRIC,
>                                (Object **)&chip8->xics,
> @@ -1145,22 +1143,6 @@ static void pnv_chip_power8_instance_init(Object *obj)
>       object_initialize_child(obj, "occ", &chip8->occ, TYPE_PNV8_OCC);
>   
>       object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
> -
> -    chip8->num_phbs = pcc->num_phbs;
> -
> -    for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
> -
> -        /*
> -         * We need the chip to parent the PHB to allow the DT
> -         * to build correctly (via pnv_xscom_dt()).
> -         *
> -         * TODO: the PHB should be parented by a PEC device.
> -         */
> -        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
> -        chip8->phbs[i] = phb3;
> -    }
> -
>   }
>   
>   static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
> @@ -1286,8 +1268,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>                                   &chip8->homer.regs);
>   
>       /* PHB3 controllers */
> +    chip8->num_phbs = pcc->num_phbs;
> +
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb = chip8->phbs[i];
> +        PnvPHB3 *phb = PNV_PHB3(object_new(TYPE_PNV_PHB3));
> +
> +        chip8->phbs[i] = phb;
>   
>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,

[PATCH 11/11] ppc/pnv: move PHB4 parent fixup to phb4_realize()

[Daniel Henrique Barboza]

After the code cleanups done in the past, default PHB4 code is already
fairly close to user created PHB4s. What we need to do to make it equal
is move the QOM and bus parenting changes from
pnv_pec_default_phb_realize() to pnv_phb4_realize().

Using the same logic for both cases (aside from PEC assigning) will make
our lives easier when re-enabling user created PHB4s.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/pci-host/pnv_phb4.c     | 12 ++++++++++++
 hw/pci-host/pnv_phb4_pec.c |  1 -
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index bfec8b9f6d..fd6fac21ac 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1553,6 +1553,18 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     int nr_irqs;
     char name[32];
 
+    /*
+     * We need the PEC to parent the PHB to allow the DT
+     * to build correctly (via pnv_xscom_dt()).
+     */
+    pnv_parent_qom_fixup(OBJECT(phb->pec), OBJECT(phb), phb->phb_id);
+
+    /*
+     * pnv-phb4 buses are child of the main-system-bus, same as
+     * the chip.
+     */
+    pnv_parent_bus_fixup(DEVICE(phb->pec->chip), dev);
+
     /* Set the "big_phb" flag */
     phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
 
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index c9aaf1c28e..eb47b50737 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -119,7 +119,6 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
     PnvPHB4 *phb = PNV_PHB4(qdev_new(pecc->phb_type));
     int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
 
-    object_property_add_child(OBJECT(pec), "phb[*]", OBJECT(phb));
     object_property_set_link(OBJECT(phb), "pec", OBJECT(pec),
                              &error_abort);
     object_property_set_int(OBJECT(phb), "chip-id", pec->chip_id,
-- 
2.36.1