[PATCH for-7.2 v4 00/11] enable pnv-phb user created devices

[PATCH for-7.2 v4 00/11] enable pnv-phb user created devices

[Daniel Henrique Barboza]

Hi,

This version contains changes based on Cedric's v3 feedback. The biggest
change was made in patch 4, where a new helper was added to handle the
logic where a PHB is added to a chip. 

Changes from v3:
- patch 4:
  - added Error **errp parameter to pnv_parent_bus_fixup() and pnv_phb_user_device_init()
  - added pnv_chip_add_phb() helper
- patch 5:
  - changed pnv_chip_power8_instance_init() to use an 'Object *phb' pointer
- patch 6:
  - added the default PHBs under an "if (defaults_enabled())" case
- patch 7:
  - pnv_phb4_get_pec() was moved to hw/ppc/pnv.c 
- patch 9:
  - use g_assert_not_reached() in pnv_phb4_get_pec()
- v3 link: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01548.html

Daniel Henrique Barboza (11):
  ppc/pnv: add phb-id/chip-id PnvPHB3RootBus properties
  ppc/pnv: add phb-id/chip-id PnvPHB4RootBus properties
  ppc/pnv: set root port chassis and slot using Bus properties
  ppc/pnv: add helpers for pnv-phb user devices
  ppc/pnv: turn chip8->phbs[] into a PnvPHB* array
  ppc/pnv: enable user created pnv-phb for powernv8
  ppc/pnv: add PHB4 helpers for user created pnv-phb
  ppc/pnv: enable user created pnv-phb for powernv9
  ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs
  ppc/pnv: user creatable pnv-phb for powernv10
  ppc/pnv: fix QOM parenting of user creatable root ports

 hw/pci-host/pnv_phb.c          | 120 +++++++++++++++++++++++++++------
 hw/pci-host/pnv_phb3.c         |  50 ++++++++++++++
 hw/pci-host/pnv_phb4.c         |  51 ++++++++++++++
 hw/pci-host/pnv_phb4_pec.c     |   6 +-
 hw/ppc/pnv.c                   | 103 +++++++++++++++++++++++++---
 include/hw/pci-host/pnv_phb3.h |   9 ++-
 include/hw/pci-host/pnv_phb4.h |  10 +++
 include/hw/ppc/pnv.h           |   7 +-
 8 files changed, 325 insertions(+), 31 deletions(-)

-- 
2.36.1

[PATCH for-7.2 v4 01/11] ppc/pnv: add phb-id/chip-id PnvPHB3RootBus properties

[Daniel Henrique Barboza]

We rely on the phb-id and chip-id, which are PHB properties, to assign
chassis and slot to the root port. For default devices this is no big
deal: the root port is being created under pnv_phb_realize() and the
values are being passed on via the 'index' and 'chip-id' of the
pnv_phb_attach_root_port() helper.

If we want to implement user created root ports we have a problem. The
user created root port will not be aware of which PHB it belongs to,
unless we're willing to violate QOM best practices and access the PHB
via dev->parent_bus->parent. What we can do is to access the root bus
parent bus.

Since we're already assigning the root port as QOM child of the bus, and
the bus is initiated using PHB properties, let's add phb-id and chip-id
as properties of the bus. This will allow us trivial access to them, for
both user-created and default root ports, without doing anything too
shady with QOM.

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

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index d4c04a281a..af8575c007 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1006,6 +1006,11 @@ void pnv_phb3_bus_init(DeviceState *dev, PnvPHB3 *phb)
                                      &phb->pci_mmio, &phb->pci_io,
                                      0, 4, TYPE_PNV_PHB3_ROOT_BUS);
 
+    object_property_set_int(OBJECT(pci->bus), "phb-id", phb->phb_id,
+                            &error_abort);
+    object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
+                            &error_abort);
+
     pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
 }
 
@@ -1105,10 +1110,55 @@ static const TypeInfo pnv_phb3_type_info = {
     .instance_init = pnv_phb3_instance_init,
 };
 
+static void pnv_phb3_root_bus_get_prop(Object *obj, Visitor *v,
+                                       const char *name,
+                                       void *opaque, Error **errp)
+{
+    PnvPHB3RootBus *bus = PNV_PHB3_ROOT_BUS(obj);
+    uint64_t value = 0;
+
+    if (strcmp(name, "phb-id") == 0) {
+        value = bus->phb_id;
+    } else {
+        value = bus->chip_id;
+    }
+
+    visit_type_size(v, name, &value, errp);
+}
+
+static void pnv_phb3_root_bus_set_prop(Object *obj, Visitor *v,
+                                       const char *name,
+                                       void *opaque, Error **errp)
+
+{
+    PnvPHB3RootBus *bus = PNV_PHB3_ROOT_BUS(obj);
+    uint64_t value;
+
+    if (!visit_type_size(v, name, &value, errp)) {
+        return;
+    }
+
+    if (strcmp(name, "phb-id") == 0) {
+        bus->phb_id = value;
+    } else {
+        bus->chip_id = value;
+    }
+}
+
 static void pnv_phb3_root_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *k = BUS_CLASS(klass);
 
+    object_class_property_add(klass, "phb-id", "int",
+                              pnv_phb3_root_bus_get_prop,
+                              pnv_phb3_root_bus_set_prop,
+                              NULL, NULL);
+
+    object_class_property_add(klass, "chip-id", "int",
+                              pnv_phb3_root_bus_get_prop,
+                              pnv_phb3_root_bus_set_prop,
+                              NULL, NULL);
+
     /*
      * PHB3 has only a single root complex. Enforce the limit on the
      * parent bus
diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
index bff69201d9..4854f6d2f6 100644
--- a/include/hw/pci-host/pnv_phb3.h
+++ b/include/hw/pci-host/pnv_phb3.h
@@ -104,9 +104,16 @@ struct PnvPBCQState {
 };
 
 /*
- * PHB3 PCIe Root port
+ * PHB3 PCIe Root Bus
  */
 #define TYPE_PNV_PHB3_ROOT_BUS "pnv-phb3-root"
+struct PnvPHB3RootBus {
+    PCIBus parent;
+
+    uint32_t chip_id;
+    uint32_t phb_id;
+};
+OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB3RootBus, PNV_PHB3_ROOT_BUS)
 
 /*
  * PHB3 PCIe Host Bridge for PowerNV machines (POWER8)
-- 
2.36.1

Re: [PATCH for-7.2 v4 01/11] ppc/pnv: add phb-id/chip-id PnvPHB3RootBus properties

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> We rely on the phb-id and chip-id, which are PHB properties, to assign
> chassis and slot to the root port. For default devices this is no big
> deal: the root port is being created under pnv_phb_realize() and the
> values are being passed on via the 'index' and 'chip-id' of the
> pnv_phb_attach_root_port() helper.
> 
> If we want to implement user created root ports we have a problem. The
> user created root port will not be aware of which PHB it belongs to,
> unless we're willing to violate QOM best practices and access the PHB
> via dev->parent_bus->parent. What we can do is to access the root bus
> parent bus.
> 
> Since we're already assigning the root port as QOM child of the bus, and
> the bus is initiated using PHB properties, let's add phb-id and chip-id
> as properties of the bus. This will allow us trivial access to them, for
> both user-created and default root ports, without doing anything too
> shady with QOM.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


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

   Fred


>   hw/pci-host/pnv_phb3.c         | 50 ++++++++++++++++++++++++++++++++++
>   include/hw/pci-host/pnv_phb3.h |  9 +++++-
>   2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index d4c04a281a..af8575c007 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1006,6 +1006,11 @@ void pnv_phb3_bus_init(DeviceState *dev, PnvPHB3 *phb)
>                                        &phb->pci_mmio, &phb->pci_io,
>                                        0, 4, TYPE_PNV_PHB3_ROOT_BUS);
>   
> +    object_property_set_int(OBJECT(pci->bus), "phb-id", phb->phb_id,
> +                            &error_abort);
> +    object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
> +                            &error_abort);
> +
>       pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
>   }
>   
> @@ -1105,10 +1110,55 @@ static const TypeInfo pnv_phb3_type_info = {
>       .instance_init = pnv_phb3_instance_init,
>   };
>   
> +static void pnv_phb3_root_bus_get_prop(Object *obj, Visitor *v,
> +                                       const char *name,
> +                                       void *opaque, Error **errp)
> +{
> +    PnvPHB3RootBus *bus = PNV_PHB3_ROOT_BUS(obj);
> +    uint64_t value = 0;
> +
> +    if (strcmp(name, "phb-id") == 0) {
> +        value = bus->phb_id;
> +    } else {
> +        value = bus->chip_id;
> +    }
> +
> +    visit_type_size(v, name, &value, errp);
> +}
> +
> +static void pnv_phb3_root_bus_set_prop(Object *obj, Visitor *v,
> +                                       const char *name,
> +                                       void *opaque, Error **errp)
> +
> +{
> +    PnvPHB3RootBus *bus = PNV_PHB3_ROOT_BUS(obj);
> +    uint64_t value;
> +
> +    if (!visit_type_size(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (strcmp(name, "phb-id") == 0) {
> +        bus->phb_id = value;
> +    } else {
> +        bus->chip_id = value;
> +    }
> +}
> +
>   static void pnv_phb3_root_bus_class_init(ObjectClass *klass, void *data)
>   {
>       BusClass *k = BUS_CLASS(klass);
>   
> +    object_class_property_add(klass, "phb-id", "int",
> +                              pnv_phb3_root_bus_get_prop,
> +                              pnv_phb3_root_bus_set_prop,
> +                              NULL, NULL);
> +
> +    object_class_property_add(klass, "chip-id", "int",
> +                              pnv_phb3_root_bus_get_prop,
> +                              pnv_phb3_root_bus_set_prop,
> +                              NULL, NULL);
> +
>       /*
>        * PHB3 has only a single root complex. Enforce the limit on the
>        * parent bus
> diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
> index bff69201d9..4854f6d2f6 100644
> --- a/include/hw/pci-host/pnv_phb3.h
> +++ b/include/hw/pci-host/pnv_phb3.h
> @@ -104,9 +104,16 @@ struct PnvPBCQState {
>   };
>   
>   /*
> - * PHB3 PCIe Root port
> + * PHB3 PCIe Root Bus
>    */
>   #define TYPE_PNV_PHB3_ROOT_BUS "pnv-phb3-root"
> +struct PnvPHB3RootBus {
> +    PCIBus parent;
> +
> +    uint32_t chip_id;
> +    uint32_t phb_id;
> +};
> +OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB3RootBus, PNV_PHB3_ROOT_BUS)
>   
>   /*
>    * PHB3 PCIe Host Bridge for PowerNV machines (POWER8)

[PATCH for-7.2 v4 02/11] ppc/pnv: add phb-id/chip-id PnvPHB4RootBus properties

[Daniel Henrique Barboza]

The same rationale provided in the PHB3 bus case applies here.

Note: we could have merged both buses in a single object, like we did
with the root ports, and spare some boilerplate. The reason we opted to
preserve both buses objects is twofold:

- there's not user side advantage in doing so. Unifying the root ports
presents a clear user QOL change when we enable user created devices back.
The buses objects, aside from having a different QOM name, is transparent
to the user;

- we leave a door opened in case we want to increase the root port limit
for phb4/5 later on without having to deal with phb3 code.

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index b98c394713..824e1a73fb 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1551,6 +1551,12 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
                                      pnv_phb4_set_irq, pnv_phb4_map_irq, phb,
                                      &phb->pci_mmio, &phb->pci_io,
                                      0, 4, TYPE_PNV_PHB4_ROOT_BUS);
+
+    object_property_set_int(OBJECT(pci->bus), "phb-id", phb->phb_id,
+                            &error_abort);
+    object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
+                            &error_abort);
+
     pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
     pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
 }
@@ -1708,10 +1714,55 @@ static const TypeInfo pnv_phb5_type_info = {
     .instance_size = sizeof(PnvPHB4),
 };
 
+static void pnv_phb4_root_bus_get_prop(Object *obj, Visitor *v,
+                                       const char *name,
+                                       void *opaque, Error **errp)
+{
+    PnvPHB4RootBus *bus = PNV_PHB4_ROOT_BUS(obj);
+    uint64_t value = 0;
+
+    if (strcmp(name, "phb-id") == 0) {
+        value = bus->phb_id;
+    } else {
+        value = bus->chip_id;
+    }
+
+    visit_type_size(v, name, &value, errp);
+}
+
+static void pnv_phb4_root_bus_set_prop(Object *obj, Visitor *v,
+                                       const char *name,
+                                       void *opaque, Error **errp)
+
+{
+    PnvPHB4RootBus *bus = PNV_PHB4_ROOT_BUS(obj);
+    uint64_t value;
+
+    if (!visit_type_size(v, name, &value, errp)) {
+        return;
+    }
+
+    if (strcmp(name, "phb-id") == 0) {
+        bus->phb_id = value;
+    } else {
+        bus->chip_id = value;
+    }
+}
+
 static void pnv_phb4_root_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *k = BUS_CLASS(klass);
 
+    object_class_property_add(klass, "phb-id", "int",
+                              pnv_phb4_root_bus_get_prop,
+                              pnv_phb4_root_bus_set_prop,
+                              NULL, NULL);
+
+    object_class_property_add(klass, "chip-id", "int",
+                              pnv_phb4_root_bus_get_prop,
+                              pnv_phb4_root_bus_set_prop,
+                              NULL, NULL);
+
     /*
      * PHB4 has only a single root complex. Enforce the limit on the
      * parent bus
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 20aa4819d3..50d4faa001 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -45,7 +45,17 @@ typedef struct PnvPhb4DMASpace {
     QLIST_ENTRY(PnvPhb4DMASpace) list;
 } PnvPhb4DMASpace;
 
+/*
+ * PHB4 PCIe Root Bus
+ */
 #define TYPE_PNV_PHB4_ROOT_BUS "pnv-phb4-root"
+struct PnvPHB4RootBus {
+    PCIBus parent;
+
+    uint32_t chip_id;
+    uint32_t phb_id;
+};
+OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB4RootBus, PNV_PHB4_ROOT_BUS)
 
 /*
  * PHB4 PCIe Host Bridge for PowerNV machines (POWER9)
-- 
2.36.1

Re: [PATCH for-7.2 v4 02/11] ppc/pnv: add phb-id/chip-id PnvPHB4RootBus properties

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> The same rationale provided in the PHB3 bus case applies here.
> 
> Note: we could have merged both buses in a single object, like we did
> with the root ports, and spare some boilerplate. The reason we opted to
> preserve both buses objects is twofold:
> 
> - there's not user side advantage in doing so. Unifying the root ports
> presents a clear user QOL change when we enable user created devices back.
> The buses objects, aside from having a different QOM name, is transparent
> to the user;
> 
> - we leave a door opened in case we want to increase the root port limit
> for phb4/5 later on without having to deal with phb3 code.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


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

   Fred


>   hw/pci-host/pnv_phb4.c         | 51 ++++++++++++++++++++++++++++++++++
>   include/hw/pci-host/pnv_phb4.h | 10 +++++++
>   2 files changed, 61 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index b98c394713..824e1a73fb 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1551,6 +1551,12 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
>                                        pnv_phb4_set_irq, pnv_phb4_map_irq, phb,
>                                        &phb->pci_mmio, &phb->pci_io,
>                                        0, 4, TYPE_PNV_PHB4_ROOT_BUS);
> +
> +    object_property_set_int(OBJECT(pci->bus), "phb-id", phb->phb_id,
> +                            &error_abort);
> +    object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
> +                            &error_abort);
> +
>       pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   }
> @@ -1708,10 +1714,55 @@ static const TypeInfo pnv_phb5_type_info = {
>       .instance_size = sizeof(PnvPHB4),
>   };
>   
> +static void pnv_phb4_root_bus_get_prop(Object *obj, Visitor *v,
> +                                       const char *name,
> +                                       void *opaque, Error **errp)
> +{
> +    PnvPHB4RootBus *bus = PNV_PHB4_ROOT_BUS(obj);
> +    uint64_t value = 0;
> +
> +    if (strcmp(name, "phb-id") == 0) {
> +        value = bus->phb_id;
> +    } else {
> +        value = bus->chip_id;
> +    }
> +
> +    visit_type_size(v, name, &value, errp);
> +}
> +
> +static void pnv_phb4_root_bus_set_prop(Object *obj, Visitor *v,
> +                                       const char *name,
> +                                       void *opaque, Error **errp)
> +
> +{
> +    PnvPHB4RootBus *bus = PNV_PHB4_ROOT_BUS(obj);
> +    uint64_t value;
> +
> +    if (!visit_type_size(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (strcmp(name, "phb-id") == 0) {
> +        bus->phb_id = value;
> +    } else {
> +        bus->chip_id = value;
> +    }
> +}
> +
>   static void pnv_phb4_root_bus_class_init(ObjectClass *klass, void *data)
>   {
>       BusClass *k = BUS_CLASS(klass);
>   
> +    object_class_property_add(klass, "phb-id", "int",
> +                              pnv_phb4_root_bus_get_prop,
> +                              pnv_phb4_root_bus_set_prop,
> +                              NULL, NULL);
> +
> +    object_class_property_add(klass, "chip-id", "int",
> +                              pnv_phb4_root_bus_get_prop,
> +                              pnv_phb4_root_bus_set_prop,
> +                              NULL, NULL);
> +
>       /*
>        * PHB4 has only a single root complex. Enforce the limit on the
>        * parent bus
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 20aa4819d3..50d4faa001 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -45,7 +45,17 @@ typedef struct PnvPhb4DMASpace {
>       QLIST_ENTRY(PnvPhb4DMASpace) list;
>   } PnvPhb4DMASpace;
>   
> +/*
> + * PHB4 PCIe Root Bus
> + */
>   #define TYPE_PNV_PHB4_ROOT_BUS "pnv-phb4-root"
> +struct PnvPHB4RootBus {
> +    PCIBus parent;
> +
> +    uint32_t chip_id;
> +    uint32_t phb_id;
> +};
> +OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB4RootBus, PNV_PHB4_ROOT_BUS)
>   
>   /*
>    * PHB4 PCIe Host Bridge for PowerNV machines (POWER9)

[PATCH for-7.2 v4 03/11] ppc/pnv: set root port chassis and slot using Bus properties

[Daniel Henrique Barboza]

For default root ports we have a way of accessing chassis and slot,
before root_port_realize(), via pnv_phb_attach_root_port(). For the
future user created root ports this won't be the case: we can't use
this helper because we don't have access to the PHB phb-id/chip-id
values.

In earlier patches we've added phb-id and chip-id to pnv-phb-root-bus
objects. We're now able to use the bus to retrieve them. The bus is
reachable for both user created and default devices, so we're changing
all the code paths. This also allow us to validate these changes with
the existing default devices.

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

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index c47ed92462..826c0c144e 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -25,21 +25,19 @@
  * QOM id. 'chip_id' is going to be used as PCIE chassis for the
  * root port.
  */
-static void pnv_phb_attach_root_port(PCIHostState *pci, int index, int chip_id)
+static void pnv_phb_attach_root_port(PCIHostState *pci)
 {
     PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
-    g_autofree char *default_id = g_strdup_printf("%s[%d]",
-                                                  TYPE_PNV_PHB_ROOT_PORT,
-                                                  index);
     const char *dev_id = DEVICE(root)->id;
+    g_autofree char *default_id = NULL;
+    int index;
+
+    index = object_property_get_int(OBJECT(pci->bus), "phb-id", &error_fatal);
+    default_id = g_strdup_printf("%s[%d]", TYPE_PNV_PHB_ROOT_PORT, index);
 
     object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
                               OBJECT(root));
 
-    /* Set unique chassis/slot values for the root port */
-    qdev_prop_set_uint8(DEVICE(root), "chassis", chip_id);
-    qdev_prop_set_uint16(DEVICE(root), "slot", index);
-
     pci_realize_and_unref(root, pci->bus, &error_fatal);
 }
 
@@ -93,7 +91,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
         pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
     }
 
-    pnv_phb_attach_root_port(pci, phb->phb_id, phb->chip_id);
+    pnv_phb_attach_root_port(pci);
 }
 
 static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
@@ -162,9 +160,18 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
 {
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
     PnvPHBRootPort *phb_rp = PNV_PHB_ROOT_PORT(dev);
+    PCIBus *bus = PCI_BUS(qdev_get_parent_bus(dev));
     PCIDevice *pci = PCI_DEVICE(dev);
     uint16_t device_id = 0;
     Error *local_err = NULL;
+    int chip_id, index;
+
+    chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
+    index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
+
+    /* Set unique chassis/slot values for the root port */
+    qdev_prop_set_uint8(dev, "chassis", chip_id);
+    qdev_prop_set_uint16(dev, "slot", index);
 
     rpc->parent_realize(dev, &local_err);
     if (local_err) {
-- 
2.36.1

Re: [PATCH for-7.2 v4 03/11] ppc/pnv: set root port chassis and slot using Bus properties

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> For default root ports we have a way of accessing chassis and slot,
> before root_port_realize(), via pnv_phb_attach_root_port(). For the
> future user created root ports this won't be the case: we can't use
> this helper because we don't have access to the PHB phb-id/chip-id
> values.
> 
> In earlier patches we've added phb-id and chip-id to pnv-phb-root-bus
> objects. We're now able to use the bus to retrieve them. The bus is
> reachable for both user created and default devices, so we're changing
> all the code paths. This also allow us to validate these changes with
> the existing default devices.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


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

   Fred


>   hw/pci-host/pnv_phb.c | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index c47ed92462..826c0c144e 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -25,21 +25,19 @@
>    * QOM id. 'chip_id' is going to be used as PCIE chassis for the
>    * root port.
>    */
> -static void pnv_phb_attach_root_port(PCIHostState *pci, int index, int chip_id)
> +static void pnv_phb_attach_root_port(PCIHostState *pci)
>   {
>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
> -    g_autofree char *default_id = g_strdup_printf("%s[%d]",
> -                                                  TYPE_PNV_PHB_ROOT_PORT,
> -                                                  index);
>       const char *dev_id = DEVICE(root)->id;
> +    g_autofree char *default_id = NULL;
> +    int index;
> +
> +    index = object_property_get_int(OBJECT(pci->bus), "phb-id", &error_fatal);
> +    default_id = g_strdup_printf("%s[%d]", TYPE_PNV_PHB_ROOT_PORT, index);
>   
>       object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
>                                 OBJECT(root));
>   
> -    /* Set unique chassis/slot values for the root port */
> -    qdev_prop_set_uint8(DEVICE(root), "chassis", chip_id);
> -    qdev_prop_set_uint16(DEVICE(root), "slot", index);
> -
>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>   }
>   
> @@ -93,7 +91,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>           pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
>       }
>   
> -    pnv_phb_attach_root_port(pci, phb->phb_id, phb->chip_id);
> +    pnv_phb_attach_root_port(pci);
>   }
>   
>   static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
> @@ -162,9 +160,18 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PnvPHBRootPort *phb_rp = PNV_PHB_ROOT_PORT(dev);
> +    PCIBus *bus = PCI_BUS(qdev_get_parent_bus(dev));
>       PCIDevice *pci = PCI_DEVICE(dev);
>       uint16_t device_id = 0;
>       Error *local_err = NULL;
> +    int chip_id, index;
> +
> +    chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
> +    index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
> +
> +    /* Set unique chassis/slot values for the root port */
> +    qdev_prop_set_uint8(dev, "chassis", chip_id);
> +    qdev_prop_set_uint16(dev, "slot", index);
>   
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {

[PATCH for-7.2 v4 04/11] ppc/pnv: add helpers for pnv-phb user devices

[Daniel Henrique Barboza]

pnv_parent_qom_fixup() and pnv_parent_bus_fixup() are versions of the
helpers that were reverted by commit 9c10d86fee "ppc/pnv: Remove
user-created PHB{3,4,5} devices". They are needed to amend the QOM and
bus hierarchies of user created pnv-phbs, matching them with default
pnv-phbs.

A new helper pnv_phb_user_device_init() is created to handle
user-created devices setup. We're going to call it inside
pnv_phb_realize() in case we're realizing an user created device. This
will centralize all user device realated in a single spot, leaving the
realize functions of the phb3/phb4 backends untouched.

Another helper called pnv_chip_add_phb() was added to handle the
particularities of each chip version when adding a new PHB.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb.c | 75 +++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/pnv.c          | 20 ++++++++++++
 include/hw/ppc/pnv.h  |  1 +
 3 files changed, 96 insertions(+)

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 826c0c144e..5dc44f45d1 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -18,6 +18,38 @@
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
 
+
+/*
+ * 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.
+ */
+static 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);
+}
+
+static void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *child,
+                                 Error **errp)
+{
+    BusState *parent_bus = qdev_get_parent_bus(parent);
+
+    if (!qdev_set_parent_bus(child, parent_bus, errp)) {
+        return;
+    }
+}
+
 /*
  * Attach a root port device.
  *
@@ -41,6 +73,39 @@ static void pnv_phb_attach_root_port(PCIHostState *pci)
     pci_realize_and_unref(root, pci->bus, &error_fatal);
 }
 
+/*
+ * User created devices won't have the initial setup that default
+ * devices have. This setup consists of assigning a parent device
+ * (chip for PHB3, PEC for PHB4/5) that will be the QOM/bus parent
+ * of the PHB.
+ */
+static bool pnv_phb_user_device_init(PnvPHB *phb, Error **errp)
+{
+    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
+    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
+    Object *parent = NULL;
+
+    if (!chip) {
+        error_setg(errp, "invalid chip id: %d", phb->chip_id);
+        return false;
+    }
+
+    parent = pnv_chip_add_phb(chip, phb, errp);
+    if (!parent) {
+        return false;
+    }
+
+    /*
+     * Reparent user created devices to the chip to build
+     * correctly the device tree. pnv_xscom_dt() needs every
+     * PHB to be a child of the chip to build the DT correctly.
+     */
+    pnv_parent_qom_fixup(parent, OBJECT(phb), phb->phb_id);
+    pnv_parent_bus_fixup(DEVICE(chip), DEVICE(phb), errp);
+
+    return true;
+}
+
 static void pnv_phb_realize(DeviceState *dev, Error **errp)
 {
     PnvPHB *phb = PNV_PHB(dev);
@@ -74,6 +139,16 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
     object_property_set_uint(phb->backend, "chip-id", phb->chip_id, errp);
     object_property_set_link(phb->backend, "phb-base", OBJECT(phb), errp);
 
+    /*
+     * Handle user created devices. User devices will not have a
+     * pointer to a chip (PHB3) and a PEC (PHB4/5).
+     */
+    if (!phb->chip && !phb->pec) {
+        if (!pnv_phb_user_device_init(phb, errp)) {
+            return;
+        }
+    }
+
     if (phb->version == 3) {
         object_property_set_link(phb->backend, "chip",
                                  OBJECT(phb->chip), errp);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index f9e5a3d248..2deaac17f7 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -281,6 +281,26 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
     g_free(reg);
 }
 
+/*
+ * Adds a PnvPHB to the chip. Returns the parent obj of the
+ * PHB which varies with each version (phb version 3 is parented
+ * by the chip, version 4 and 4 are parented by the PEC
+ * device).
+ *
+ * TODO: for version 3 we're still parenting the PHB with the
+ * chip. We should parent with a (so far not implemented)
+ * PHB3 PEC device.
+ */
+Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
+{
+    if (phb->version == 3) {
+        return OBJECT(chip);
+    } else {
+        /* phb4 support will be added later */
+        return NULL;
+    }
+}
+
 static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
 {
     static const char compat[] = "ibm,power8-xscom\0ibm,xscom";
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 033d907287..781d0acffa 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -231,6 +231,7 @@ struct PnvMachineState {
 };
 
 PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
+Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
 
 #define PNV_FDT_ADDR          0x01000000
 #define PNV_TIMEBASE_FREQ     512000000ULL
-- 
2.36.1

Re: [PATCH for-7.2 v4 04/11] ppc/pnv: add helpers for pnv-phb user devices

[Cédric Le Goater]

On 8/11/22 18:39, Daniel Henrique Barboza wrote:
> pnv_parent_qom_fixup() and pnv_parent_bus_fixup() are versions of the
> helpers that were reverted by commit 9c10d86fee "ppc/pnv: Remove
> user-created PHB{3,4,5} devices". They are needed to amend the QOM and
> bus hierarchies of user created pnv-phbs, matching them with default
> pnv-phbs.
> 
> A new helper pnv_phb_user_device_init() is created to handle
> user-created devices setup. We're going to call it inside
> pnv_phb_realize() in case we're realizing an user created device. This
> will centralize all user device realated in a single spot, leaving the
> realize functions of the phb3/phb4 backends untouched.
> 
> Another helper called pnv_chip_add_phb() was added to handle the
> particularities of each chip version when adding a new PHB.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Thanks,

C.

> ---
>   hw/pci-host/pnv_phb.c | 75 +++++++++++++++++++++++++++++++++++++++++++
>   hw/ppc/pnv.c          | 20 ++++++++++++
>   include/hw/ppc/pnv.h  |  1 +
>   3 files changed, 96 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 826c0c144e..5dc44f45d1 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -18,6 +18,38 @@
>   #include "hw/qdev-properties.h"
>   #include "qom/object.h"
>   
> +
> +/*
> + * 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.
> + */
> +static 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);
> +}
> +
> +static void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *child,
> +                                 Error **errp)
> +{
> +    BusState *parent_bus = qdev_get_parent_bus(parent);
> +
> +    if (!qdev_set_parent_bus(child, parent_bus, errp)) {
> +        return;
> +    }
> +}
> +
>   /*
>    * Attach a root port device.
>    *
> @@ -41,6 +73,39 @@ static void pnv_phb_attach_root_port(PCIHostState *pci)
>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>   }
>   
> +/*
> + * User created devices won't have the initial setup that default
> + * devices have. This setup consists of assigning a parent device
> + * (chip for PHB3, PEC for PHB4/5) that will be the QOM/bus parent
> + * of the PHB.
> + */
> +static bool pnv_phb_user_device_init(PnvPHB *phb, Error **errp)
> +{
> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
> +    Object *parent = NULL;
> +
> +    if (!chip) {
> +        error_setg(errp, "invalid chip id: %d", phb->chip_id);
> +        return false;
> +    }
> +
> +    parent = pnv_chip_add_phb(chip, phb, errp);
> +    if (!parent) {
> +        return false;
> +    }
> +
> +    /*
> +     * Reparent user created devices to the chip to build
> +     * correctly the device tree. pnv_xscom_dt() needs every
> +     * PHB to be a child of the chip to build the DT correctly.
> +     */
> +    pnv_parent_qom_fixup(parent, OBJECT(phb), phb->phb_id);
> +    pnv_parent_bus_fixup(DEVICE(chip), DEVICE(phb), errp);
> +
> +    return true;
> +}
> +
>   static void pnv_phb_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB *phb = PNV_PHB(dev);
> @@ -74,6 +139,16 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>       object_property_set_uint(phb->backend, "chip-id", phb->chip_id, errp);
>       object_property_set_link(phb->backend, "phb-base", OBJECT(phb), errp);
>   
> +    /*
> +     * Handle user created devices. User devices will not have a
> +     * pointer to a chip (PHB3) and a PEC (PHB4/5).
> +     */
> +    if (!phb->chip && !phb->pec) {
> +        if (!pnv_phb_user_device_init(phb, errp)) {
> +            return;
> +        }
> +    }
> +
>       if (phb->version == 3) {
>           object_property_set_link(phb->backend, "chip",
>                                    OBJECT(phb->chip), errp);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index f9e5a3d248..2deaac17f7 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -281,6 +281,26 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>       g_free(reg);
>   }
>   
> +/*
> + * Adds a PnvPHB to the chip. Returns the parent obj of the
> + * PHB which varies with each version (phb version 3 is parented
> + * by the chip, version 4 and 4 are parented by the PEC
> + * device).
> + *
> + * TODO: for version 3 we're still parenting the PHB with the
> + * chip. We should parent with a (so far not implemented)
> + * PHB3 PEC device.
> + */
> +Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
> +{
> +    if (phb->version == 3) {
> +        return OBJECT(chip);
> +    } else {
> +        /* phb4 support will be added later */
> +        return NULL;
> +    }
> +}
> +
>   static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
>   {
>       static const char compat[] = "ibm,power8-xscom\0ibm,xscom";
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 033d907287..781d0acffa 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -231,6 +231,7 @@ struct PnvMachineState {
>   };
>   
>   PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
> +Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
>   
>   #define PNV_FDT_ADDR          0x01000000
>   #define PNV_TIMEBASE_FREQ     512000000ULL

Re: [PATCH for-7.2 v4 04/11] ppc/pnv: add helpers for pnv-phb user devices

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> pnv_parent_qom_fixup() and pnv_parent_bus_fixup() are versions of the
> helpers that were reverted by commit 9c10d86fee "ppc/pnv: Remove
> user-created PHB{3,4,5} devices". They are needed to amend the QOM and
> bus hierarchies of user created pnv-phbs, matching them with default
> pnv-phbs.
> 
> A new helper pnv_phb_user_device_init() is created to handle
> user-created devices setup. We're going to call it inside
> pnv_phb_realize() in case we're realizing an user created device. This
> will centralize all user device realated in a single spot, leaving the
> realize functions of the phb3/phb4 backends untouched.
> 
> Another helper called pnv_chip_add_phb() was added to handle the
> particularities of each chip version when adding a new PHB.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


Just a minor typo in a comment below.
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index f9e5a3d248..2deaac17f7 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -281,6 +281,26 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>       g_free(reg);
>   }
>   
> +/*
> + * Adds a PnvPHB to the chip. Returns the parent obj of the
> + * PHB which varies with each version (phb version 3 is parented
> + * by the chip, version 4 and 4 are parented by the PEC


typo-----------------------------^

   Fred


> + * device).
> + *
> + * TODO: for version 3 we're still parenting the PHB with the
> + * chip. We should parent with a (so far not implemented)
> + * PHB3 PEC device.
> + */
> +Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
> +{
> +    if (phb->version == 3) {
> +        return OBJECT(chip);
> +    } else {
> +        /* phb4 support will be added later */
> +        return NULL;
> +    }
> +}
> +
>   static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
>   {
>       static const char compat[] = "ibm,power8-xscom\0ibm,xscom";
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 033d907287..781d0acffa 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -231,6 +231,7 @@ struct PnvMachineState {
>   };
>   
>   PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
> +Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
>   
>   #define PNV_FDT_ADDR          0x01000000
>   #define PNV_TIMEBASE_FREQ     512000000ULL

[PATCH for-7.2 v4 05/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB* 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 <danielhb413@gmail.com>
---
 hw/ppc/pnv.c         | 27 ++++++++++++++++++++++-----
 include/hw/ppc/pnv.h |  6 +++++-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 2deaac17f7..e53e9e297d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -294,6 +294,13 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
 Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
 {
     if (phb->version == 3) {
+        Pnv8Chip *chip8 = PNV8_CHIP(chip);
+
+        phb->chip = chip;
+
+        chip8->phbs[chip8->num_phbs] = phb;
+        chip8->num_phbs++;
+
         return OBJECT(chip);
     } else {
         /* phb4 support will be added later */
@@ -681,7 +688,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++) {
-        PnvPHB *phb = &chip8->phbs[i];
+        PnvPHB *phb = chip8->phbs[i];
         PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
 
         pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
@@ -1174,7 +1181,17 @@ 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_PHB);
+        Object *phb = object_new(TYPE_PNV_PHB);
+
+        /*
+         * 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 that, at
+         * this moment, is not modelled powernv8/phb3.
+         */
+        object_property_add_child(obj, "phb[*]", phb);
+        chip8->phbs[i] = PNV_PHB(phb);
     }
 
 }
@@ -1290,7 +1307,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
 
     /* PHB controllers */
     for (i = 0; i < chip8->num_phbs; i++) {
-        PnvPHB *phb = &chip8->phbs[i];
+        PnvPHB *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,
@@ -1958,7 +1975,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
         }
 
         for (j = 0; j < chip8->num_phbs; j++) {
-            PnvPHB *phb = &chip8->phbs[j];
+            PnvPHB *phb = chip8->phbs[j];
             PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
 
             if (ics_valid_irq(&phb3->lsis, irq)) {
@@ -1997,7 +2014,7 @@ static void pnv_ics_resend(XICSFabric *xi)
         ics_resend(&chip8->psi.ics);
 
         for (j = 0; j < chip8->num_phbs; j++) {
-            PnvPHB *phb = &chip8->phbs[j];
+            PnvPHB *phb = chip8->phbs[j];
             PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
 
             ics_resend(&phb3->lsis);
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 781d0acffa..49433281d7 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -81,7 +81,11 @@ struct Pnv8Chip {
     PnvHomer     homer;
 
 #define PNV8_CHIP_PHB3_MAX 4
-    PnvPHB       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().
+     */
+    PnvPHB       *phbs[PNV8_CHIP_PHB3_MAX];
     uint32_t     num_phbs;
 
     XICSFabric    *xics;
-- 
2.36.1

Re: [PATCH for-7.2 v4 05/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB* array

[Cédric Le Goater]

On 8/11/22 18:39, 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 <danielhb413@gmail.com>

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

Thanks,

C.

> ---
>   hw/ppc/pnv.c         | 27 ++++++++++++++++++++++-----
>   include/hw/ppc/pnv.h |  6 +++++-
>   2 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 2deaac17f7..e53e9e297d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -294,6 +294,13 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>   Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
>   {
>       if (phb->version == 3) {
> +        Pnv8Chip *chip8 = PNV8_CHIP(chip);
> +
> +        phb->chip = chip;
> +
> +        chip8->phbs[chip8->num_phbs] = phb;
> +        chip8->num_phbs++;
> +
>           return OBJECT(chip);
>       } else {
>           /* phb4 support will be added later */
> @@ -681,7 +688,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++) {
> -        PnvPHB *phb = &chip8->phbs[i];
> +        PnvPHB *phb = chip8->phbs[i];
>           PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
>   
>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
> @@ -1174,7 +1181,17 @@ 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_PHB);
> +        Object *phb = object_new(TYPE_PNV_PHB);
> +
> +        /*
> +         * 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 that, at
> +         * this moment, is not modelled powernv8/phb3.
> +         */
> +        object_property_add_child(obj, "phb[*]", phb);
> +        chip8->phbs[i] = PNV_PHB(phb);
>       }
>   
>   }
> @@ -1290,7 +1307,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>   
>       /* PHB controllers */
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB *phb = &chip8->phbs[i];
> +        PnvPHB *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,
> @@ -1958,7 +1975,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>           }
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            PnvPHB *phb = &chip8->phbs[j];
> +            PnvPHB *phb = chip8->phbs[j];
>               PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
>   
>               if (ics_valid_irq(&phb3->lsis, irq)) {
> @@ -1997,7 +2014,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>           ics_resend(&chip8->psi.ics);
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            PnvPHB *phb = &chip8->phbs[j];
> +            PnvPHB *phb = chip8->phbs[j];
>               PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
>   
>               ics_resend(&phb3->lsis);
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 781d0acffa..49433281d7 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -81,7 +81,11 @@ struct Pnv8Chip {
>       PnvHomer     homer;
>   
>   #define PNV8_CHIP_PHB3_MAX 4
> -    PnvPHB       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().
> +     */
> +    PnvPHB       *phbs[PNV8_CHIP_PHB3_MAX];
>       uint32_t     num_phbs;
>   
>       XICSFabric    *xics;

Re: [PATCH for-7.2 v4 05/11] ppc/pnv: turn chip8->phbs[] into a PnvPHB* array

[Frederic Barrat]

On 11/08/2022 18:39, 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 <danielhb413@gmail.com>
> ---


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

   Fred


>   hw/ppc/pnv.c         | 27 ++++++++++++++++++++++-----
>   include/hw/ppc/pnv.h |  6 +++++-
>   2 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 2deaac17f7..e53e9e297d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -294,6 +294,13 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>   Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
>   {
>       if (phb->version == 3) {
> +        Pnv8Chip *chip8 = PNV8_CHIP(chip);
> +
> +        phb->chip = chip;
> +
> +        chip8->phbs[chip8->num_phbs] = phb;
> +        chip8->num_phbs++;
> +
>           return OBJECT(chip);
>       } else {
>           /* phb4 support will be added later */
> @@ -681,7 +688,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++) {
> -        PnvPHB *phb = &chip8->phbs[i];
> +        PnvPHB *phb = chip8->phbs[i];
>           PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
>   
>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
> @@ -1174,7 +1181,17 @@ 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_PHB);
> +        Object *phb = object_new(TYPE_PNV_PHB);
> +
> +        /*
> +         * 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 that, at
> +         * this moment, is not modelled powernv8/phb3.
> +         */
> +        object_property_add_child(obj, "phb[*]", phb);
> +        chip8->phbs[i] = PNV_PHB(phb);
>       }
>   
>   }
> @@ -1290,7 +1307,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>   
>       /* PHB controllers */
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB *phb = &chip8->phbs[i];
> +        PnvPHB *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,
> @@ -1958,7 +1975,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>           }
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            PnvPHB *phb = &chip8->phbs[j];
> +            PnvPHB *phb = chip8->phbs[j];
>               PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
>   
>               if (ics_valid_irq(&phb3->lsis, irq)) {
> @@ -1997,7 +2014,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>           ics_resend(&chip8->psi.ics);
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            PnvPHB *phb = &chip8->phbs[j];
> +            PnvPHB *phb = chip8->phbs[j];
>               PnvPHB3 *phb3 = PNV_PHB3(phb->backend);
>   
>               ics_resend(&phb3->lsis);
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 781d0acffa..49433281d7 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -81,7 +81,11 @@ struct Pnv8Chip {
>       PnvHomer     homer;
>   
>   #define PNV8_CHIP_PHB3_MAX 4
> -    PnvPHB       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().
> +     */
> +    PnvPHB       *phbs[PNV8_CHIP_PHB3_MAX];
>       uint32_t     num_phbs;
>   
>       XICSFabric    *xics;

[PATCH for-7.2 v4 06/11] ppc/pnv: enable user created pnv-phb for powernv8

[Daniel Henrique Barboza]

The bulk of the work was already done by previous patches.

Use defaults_enabled() to determine whether we need to create the
default devices or not.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb.c |  9 +++++++--
 hw/ppc/pnv.c          | 32 ++++++++++++++++++--------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 5dc44f45d1..1f53ff77c5 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -17,6 +17,7 @@
 #include "hw/ppc/pnv.h"
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
+#include "sysemu/sysemu.h"
 
 
 /*
@@ -166,6 +167,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
         pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
     }
 
+    if (phb->version == 3 && !defaults_enabled()) {
+        return;
+    }
+
     pnv_phb_attach_root_port(pci);
 }
 
@@ -201,7 +206,7 @@ static void pnv_phb_class_init(ObjectClass *klass, void *data)
     dc->realize = pnv_phb_realize;
     device_class_set_props(dc, pnv_phb_properties);
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 }
 
 static void pnv_phb_root_port_reset(DeviceState *dev)
@@ -292,7 +297,7 @@ static void pnv_phb_root_port_class_init(ObjectClass *klass, void *data)
     device_class_set_parent_reset(dc, pnv_phb_root_port_reset,
                                   &rpc->parent_reset);
     dc->reset = &pnv_phb_root_port_reset;
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 
     k->vendor_id = PCI_VENDOR_ID_IBM;
     /* device_id will be written during realize() */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index e53e9e297d..e82d6522f0 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1178,20 +1178,22 @@ static void pnv_chip_power8_instance_init(Object *obj)
 
     object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
 
-    chip8->num_phbs = pcc->num_phbs;
-
-    for (i = 0; i < chip8->num_phbs; i++) {
-        Object *phb = object_new(TYPE_PNV_PHB);
-
-        /*
-         * 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 that, at
-         * this moment, is not modelled powernv8/phb3.
-         */
-        object_property_add_child(obj, "phb[*]", phb);
-        chip8->phbs[i] = PNV_PHB(phb);
+    if (defaults_enabled()) {
+        chip8->num_phbs = pcc->num_phbs;
+
+        for (i = 0; i < chip8->num_phbs; i++) {
+            Object *phb = object_new(TYPE_PNV_PHB);
+
+            /*
+             * 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 that, at
+             * this moment, is not modelled powernv8/phb3.
+             */
+            object_property_add_child(obj, "phb[*]", phb);
+            chip8->phbs[i] = PNV_PHB(phb);
+        }
     }
 
 }
@@ -2130,6 +2132,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
 
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
+
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
 }
 
 static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
-- 
2.36.1

Re: [PATCH for-7.2 v4 06/11] ppc/pnv: enable user created pnv-phb for powernv8

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> The bulk of the work was already done by previous patches.
> 
> Use defaults_enabled() to determine whether we need to create the
> default devices or not.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


The QOM relationship for user-created root port is not ideal, but it's 
addressed in the last patch of the series.
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   hw/pci-host/pnv_phb.c |  9 +++++++--
>   hw/ppc/pnv.c          | 32 ++++++++++++++++++--------------
>   2 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 5dc44f45d1..1f53ff77c5 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -17,6 +17,7 @@
>   #include "hw/ppc/pnv.h"
>   #include "hw/qdev-properties.h"
>   #include "qom/object.h"
> +#include "sysemu/sysemu.h"
>   
>   
>   /*
> @@ -166,6 +167,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>           pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
>       }
>   
> +    if (phb->version == 3 && !defaults_enabled()) {
> +        return;
> +    }
> +
>       pnv_phb_attach_root_port(pci);
>   }
>   
> @@ -201,7 +206,7 @@ static void pnv_phb_class_init(ObjectClass *klass, void *data)
>       dc->realize = pnv_phb_realize;
>       device_class_set_props(dc, pnv_phb_properties);
>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    dc->user_creatable = false;
> +    dc->user_creatable = true;
>   }
>   
>   static void pnv_phb_root_port_reset(DeviceState *dev)
> @@ -292,7 +297,7 @@ static void pnv_phb_root_port_class_init(ObjectClass *klass, void *data)
>       device_class_set_parent_reset(dc, pnv_phb_root_port_reset,
>                                     &rpc->parent_reset);
>       dc->reset = &pnv_phb_root_port_reset;
> -    dc->user_creatable = false;
> +    dc->user_creatable = true;
>   
>       k->vendor_id = PCI_VENDOR_ID_IBM;
>       /* device_id will be written during realize() */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e53e9e297d..e82d6522f0 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1178,20 +1178,22 @@ static void pnv_chip_power8_instance_init(Object *obj)
>   
>       object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
>   
> -    chip8->num_phbs = pcc->num_phbs;
> -
> -    for (i = 0; i < chip8->num_phbs; i++) {
> -        Object *phb = object_new(TYPE_PNV_PHB);
> -
> -        /*
> -         * 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 that, at
> -         * this moment, is not modelled powernv8/phb3.
> -         */
> -        object_property_add_child(obj, "phb[*]", phb);
> -        chip8->phbs[i] = PNV_PHB(phb);
> +    if (defaults_enabled()) {
> +        chip8->num_phbs = pcc->num_phbs;
> +
> +        for (i = 0; i < chip8->num_phbs; i++) {
> +            Object *phb = object_new(TYPE_PNV_PHB);
> +
> +            /*
> +             * 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 that, at
> +             * this moment, is not modelled powernv8/phb3.
> +             */
> +            object_property_add_child(obj, "phb[*]", phb);
> +            chip8->phbs[i] = PNV_PHB(phb);
> +        }
>       }
>   
>   }
> @@ -2130,6 +2132,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
>   
>       pmc->compat = compat;
>       pmc->compat_size = sizeof(compat);
> +
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
>   }
>   
>   static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)

[PATCH for-7.2 v4 07/11] ppc/pnv: add PHB4 helpers for user created pnv-phb

[Daniel Henrique Barboza]

The PHB4 backend relies on a link with the corresponding PEC element.
This is trivial to do during machine_init() time for default devices,
but not so much for user created ones.

pnv_phb4_get_pec() is a small variation of the function that was
reverted by commit 9c10d86fee "ppc/pnv: Remove user-created PHB{3,4,5}
devices". We'll use it to determine the appropriate PEC for a given user
created pnv-phb that uses a PHB4 backend.

This is done during realize() time, in pnv_phb_user_device_init().

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

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index e82d6522f0..0644f45a1d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -281,6 +281,34 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
     g_free(reg);
 }
 
+static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
+                                         Error **errp)
+{
+    Pnv9Chip *chip9 = PNV9_CHIP(chip);
+    int chip_id = phb->chip_id;
+    int index = phb->phb_id;
+    int i, j;
+
+    for (i = 0; i < chip->num_pecs; i++) {
+        /*
+         * For each PEC, check the amount of phbs it supports
+         * and see if the given phb4 index matches an index.
+         */
+        PnvPhb4PecState *pec = &chip9->pecs[i];
+
+        for (j = 0; j < pec->num_phbs; j++) {
+            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
+                return pec;
+            }
+        }
+    }
+    error_setg(errp,
+               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
+               chip_id, index);
+
+    return NULL;
+}
+
 /*
  * Adds a PnvPHB to the chip. Returns the parent obj of the
  * PHB which varies with each version (phb version 3 is parented
@@ -302,10 +330,11 @@ Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
         chip8->num_phbs++;
 
         return OBJECT(chip);
-    } else {
-        /* phb4 support will be added later */
-        return NULL;
     }
+
+    phb->pec = pnv_phb4_get_pec(chip, PNV_PHB4(phb->backend), errp);
+
+    return OBJECT(phb->pec);
 }
 
 static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
-- 
2.36.1

Re: [PATCH for-7.2 v4 07/11] ppc/pnv: add PHB4 helpers for user created pnv-phb

[Cédric Le Goater]

On 8/11/22 18:39, Daniel Henrique Barboza wrote:
> The PHB4 backend relies on a link with the corresponding PEC element.
> This is trivial to do during machine_init() time for default devices,
> but not so much for user created ones.
> 
> pnv_phb4_get_pec() is a small variation of the function that was
> reverted by commit 9c10d86fee "ppc/pnv: Remove user-created PHB{3,4,5}
> devices". We'll use it to determine the appropriate PEC for a given user
> created pnv-phb that uses a PHB4 backend.
> 
> This is done during realize() time, in pnv_phb_user_device_init().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Thanks,

C.


> ---
>   hw/ppc/pnv.c | 35 ++++++++++++++++++++++++++++++++---
>   1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e82d6522f0..0644f45a1d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -281,6 +281,34 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>       g_free(reg);
>   }
>   
> +static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
> +                                         Error **errp)
> +{
> +    Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +    int chip_id = phb->chip_id;
> +    int index = phb->phb_id;
> +    int i, j;
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
> +        /*
> +         * For each PEC, check the amount of phbs it supports
> +         * and see if the given phb4 index matches an index.
> +         */
> +        PnvPhb4PecState *pec = &chip9->pecs[i];
> +
> +        for (j = 0; j < pec->num_phbs; j++) {
> +            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> +                return pec;
> +            }
> +        }
> +    }
> +    error_setg(errp,
> +               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
> +               chip_id, index);
> +
> +    return NULL;
> +}
> +
>   /*
>    * Adds a PnvPHB to the chip. Returns the parent obj of the
>    * PHB which varies with each version (phb version 3 is parented
> @@ -302,10 +330,11 @@ Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
>           chip8->num_phbs++;
>   
>           return OBJECT(chip);
> -    } else {
> -        /* phb4 support will be added later */
> -        return NULL;
>       }
> +
> +    phb->pec = pnv_phb4_get_pec(chip, PNV_PHB4(phb->backend), errp);
> +
> +    return OBJECT(phb->pec);
>   }
>   
>   static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)

Re: [PATCH for-7.2 v4 07/11] ppc/pnv: add PHB4 helpers for user created pnv-phb

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> The PHB4 backend relies on a link with the corresponding PEC element.
> This is trivial to do during machine_init() time for default devices,
> but not so much for user created ones.
> 
> pnv_phb4_get_pec() is a small variation of the function that was
> reverted by commit 9c10d86fee "ppc/pnv: Remove user-created PHB{3,4,5}
> devices". We'll use it to determine the appropriate PEC for a given user
> created pnv-phb that uses a PHB4 backend.
> 
> This is done during realize() time, in pnv_phb_user_device_init().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


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

   Fred


>   hw/ppc/pnv.c | 35 ++++++++++++++++++++++++++++++++---
>   1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e82d6522f0..0644f45a1d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -281,6 +281,34 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>       g_free(reg);
>   }
>   
> +static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
> +                                         Error **errp)
> +{
> +    Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +    int chip_id = phb->chip_id;
> +    int index = phb->phb_id;
> +    int i, j;
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
> +        /*
> +         * For each PEC, check the amount of phbs it supports
> +         * and see if the given phb4 index matches an index.
> +         */
> +        PnvPhb4PecState *pec = &chip9->pecs[i];
> +
> +        for (j = 0; j < pec->num_phbs; j++) {
> +            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> +                return pec;
> +            }
> +        }
> +    }
> +    error_setg(errp,
> +               "pnv-phb4 chip-id %d index %d didn't match any existing PEC",
> +               chip_id, index);
> +
> +    return NULL;
> +}
> +
>   /*
>    * Adds a PnvPHB to the chip. Returns the parent obj of the
>    * PHB which varies with each version (phb version 3 is parented
> @@ -302,10 +330,11 @@ Object *pnv_chip_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp)
>           chip8->num_phbs++;
>   
>           return OBJECT(chip);
> -    } else {
> -        /* phb4 support will be added later */
> -        return NULL;
>       }
> +
> +    phb->pec = pnv_phb4_get_pec(chip, PNV_PHB4(phb->backend), errp);
> +
> +    return OBJECT(phb->pec);
>   }
>   
>   static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)

[PATCH for-7.2 v4 08/11] ppc/pnv: enable user created pnv-phb for powernv9

[Daniel Henrique Barboza]

Enable pnv-phb user created devices for powernv9 now that we have
everything in place.

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

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 1f53ff77c5..17d9960aa1 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -167,7 +167,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
         pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
     }
 
-    if (phb->version == 3 && !defaults_enabled()) {
+    if (!defaults_enabled()) {
         return;
     }
 
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 8dc363d69c..9871f462cd 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -146,8 +146,10 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
     pec->num_phbs = pecc->num_phbs[pec->index];
 
     /* Create PHBs if running with defaults */
-    for (i = 0; i < pec->num_phbs; i++) {
-        pnv_pec_default_phb_realize(pec, i, errp);
+    if (defaults_enabled()) {
+        for (i = 0; i < pec->num_phbs; i++) {
+            pnv_pec_default_phb_realize(pec, i, errp);
+        }
     }
 
     /* Initialize the XSCOM regions for the PEC registers */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0644f45a1d..ec0558ed1c 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2188,6 +2188,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
     pmc->dt_power_mgt = pnv_dt_power_mgt;
+
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
 }
 
 static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
-- 
2.36.1

Re: [PATCH for-7.2 v4 08/11] ppc/pnv: enable user created pnv-phb for powernv9

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> Enable pnv-phb user created devices for powernv9 now that we have
> everything in place.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


Same comment as in patch 6 regarding the QOM relationship of the 
user-created root port.
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   hw/pci-host/pnv_phb.c      | 2 +-
>   hw/pci-host/pnv_phb4_pec.c | 6 ++++--
>   hw/ppc/pnv.c               | 2 ++
>   3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 1f53ff77c5..17d9960aa1 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -167,7 +167,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>           pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
>       }
>   
> -    if (phb->version == 3 && !defaults_enabled()) {
> +    if (!defaults_enabled()) {
>           return;
>       }
>   
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 8dc363d69c..9871f462cd 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -146,8 +146,10 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       pec->num_phbs = pecc->num_phbs[pec->index];
>   
>       /* Create PHBs if running with defaults */
> -    for (i = 0; i < pec->num_phbs; i++) {
> -        pnv_pec_default_phb_realize(pec, i, errp);
> +    if (defaults_enabled()) {
> +        for (i = 0; i < pec->num_phbs; i++) {
> +            pnv_pec_default_phb_realize(pec, i, errp);
> +        }
>       }
>   
>       /* Initialize the XSCOM regions for the PEC registers */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0644f45a1d..ec0558ed1c 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -2188,6 +2188,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>       pmc->compat = compat;
>       pmc->compat_size = sizeof(compat);
>       pmc->dt_power_mgt = pnv_dt_power_mgt;
> +
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
>   }
>   
>   static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)

[PATCH for-7.2 v4 09/11] ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs

[Daniel Henrique Barboza]

The function assumes that we're always dealing with a PNV9_CHIP()
object. This is not the case when the pnv-phb device belongs to a
powernv10 machine.

Change pnv_phb4_get_pec() to be able to work with PNV10_CHIP() if
necessary.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/pnv.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index ec0558ed1c..e6c14fe231 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -284,17 +284,30 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
 static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
                                          Error **errp)
 {
-    Pnv9Chip *chip9 = PNV9_CHIP(chip);
+    PnvPHB *phb_base = phb->phb_base;
+    PnvPhb4PecState *pecs = NULL;
     int chip_id = phb->chip_id;
     int index = phb->phb_id;
     int i, j;
 
+    if (phb_base->version == 4) {
+        Pnv9Chip *chip9 = PNV9_CHIP(chip);
+
+        pecs = chip9->pecs;
+    } else if (phb_base->version == 5) {
+        Pnv10Chip *chip10 = PNV10_CHIP(chip);
+
+        pecs = chip10->pecs;
+    } else {
+        g_assert_not_reached();
+    }
+
     for (i = 0; i < chip->num_pecs; i++) {
         /*
          * For each PEC, check the amount of phbs it supports
          * and see if the given phb4 index matches an index.
          */
-        PnvPhb4PecState *pec = &chip9->pecs[i];
+        PnvPhb4PecState *pec = &pecs[i];
 
         for (j = 0; j < pec->num_phbs; j++) {
             if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
-- 
2.36.1

Re: [PATCH for-7.2 v4 09/11] ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs

[Cédric Le Goater]

On 8/11/22 18:39, Daniel Henrique Barboza wrote:
> The function assumes that we're always dealing with a PNV9_CHIP()
> object. This is not the case when the pnv-phb device belongs to a
> powernv10 machine.
> 
> Change pnv_phb4_get_pec() to be able to work with PNV10_CHIP() if
> necessary.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Thanks,

C.

> ---
>   hw/ppc/pnv.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index ec0558ed1c..e6c14fe231 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -284,17 +284,30 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>   static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
>                                            Error **errp)
>   {
> -    Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +    PnvPHB *phb_base = phb->phb_base;
> +    PnvPhb4PecState *pecs = NULL;
>       int chip_id = phb->chip_id;
>       int index = phb->phb_id;
>       int i, j;
>   
> +    if (phb_base->version == 4) {
> +        Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +
> +        pecs = chip9->pecs;
> +    } else if (phb_base->version == 5) {
> +        Pnv10Chip *chip10 = PNV10_CHIP(chip);
> +
> +        pecs = chip10->pecs;
> +    } else {
> +        g_assert_not_reached();
> +    }
> +
>       for (i = 0; i < chip->num_pecs; i++) {
>           /*
>            * For each PEC, check the amount of phbs it supports
>            * and see if the given phb4 index matches an index.
>            */
> -        PnvPhb4PecState *pec = &chip9->pecs[i];
> +        PnvPhb4PecState *pec = &pecs[i];
>   
>           for (j = 0; j < pec->num_phbs; j++) {
>               if (index == pnv_phb4_pec_get_phb_id(pec, j)) {

Re: [PATCH for-7.2 v4 09/11] ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> The function assumes that we're always dealing with a PNV9_CHIP()
> object. This is not the case when the pnv-phb device belongs to a
> powernv10 machine.
> 
> Change pnv_phb4_get_pec() to be able to work with PNV10_CHIP() if
> necessary.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


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

   Fred


>   hw/ppc/pnv.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index ec0558ed1c..e6c14fe231 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -284,17 +284,30 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>   static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb,
>                                            Error **errp)
>   {
> -    Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +    PnvPHB *phb_base = phb->phb_base;
> +    PnvPhb4PecState *pecs = NULL;
>       int chip_id = phb->chip_id;
>       int index = phb->phb_id;
>       int i, j;
>   
> +    if (phb_base->version == 4) {
> +        Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +
> +        pecs = chip9->pecs;
> +    } else if (phb_base->version == 5) {
> +        Pnv10Chip *chip10 = PNV10_CHIP(chip);
> +
> +        pecs = chip10->pecs;
> +    } else {
> +        g_assert_not_reached();
> +    }
> +
>       for (i = 0; i < chip->num_pecs; i++) {
>           /*
>            * For each PEC, check the amount of phbs it supports
>            * and see if the given phb4 index matches an index.
>            */
> -        PnvPhb4PecState *pec = &chip9->pecs[i];
> +        PnvPhb4PecState *pec = &pecs[i];
>   
>           for (j = 0; j < pec->num_phbs; j++) {
>               if (index == pnv_phb4_pec_get_phb_id(pec, j)) {

[PATCH for-7.2 v4 10/11] ppc/pnv: user creatable pnv-phb for powernv10

[Daniel Henrique Barboza]

Given that powernv9 and powernv10 uses the same pnv-phb backend, the
logic to allow user created pnv-phbs for powernv10 is already in place.
Let's flip the switch.

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

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index e6c14fe231..9bf35ca9d6 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2226,6 +2226,8 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
     pmc->dt_power_mgt = pnv_dt_power_mgt;
 
     xfc->match_nvt = pnv10_xive_match_nvt;
+
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
 }
 
 static bool pnv_machine_get_hb(Object *obj, Error **errp)
-- 
2.36.1

Re: [PATCH for-7.2 v4 10/11] ppc/pnv: user creatable pnv-phb for powernv10

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> Given that powernv9 and powernv10 uses the same pnv-phb backend, the
> logic to allow user created pnv-phbs for powernv10 is already in place.
> Let's flip the switch.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


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

   Fred


>   hw/ppc/pnv.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e6c14fe231..9bf35ca9d6 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -2226,6 +2226,8 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
>       pmc->dt_power_mgt = pnv_dt_power_mgt;
>   
>       xfc->match_nvt = pnv10_xive_match_nvt;
> +
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
>   }
>   
>   static bool pnv_machine_get_hb(Object *obj, Error **errp)

[PATCH for-7.2 v4 11/11] ppc/pnv: fix QOM parenting of user creatable root ports

[Daniel Henrique Barboza]

User creatable root ports are being parented by the 'peripheral' or the
'peripheral-anon' container. This happens because this is the regular
QOM schema for sysbus devices that are added via the command line.

Let's make this QOM hierarchy similar to what we have with default root
ports, i.e. the root port must be parented by the pnv-root-bus. To do
that we change the qom and bus parent of the root port during
root_port_realize(). The realize() is shared by the default root port
code path, so we can remove the code inside pnv_phb_attach_root_port()
that was adding the root port as a child of the bus as well.

While we're at it, change pnv_phb_attach_root_port() to receive a PCIBus
instead of a PCIHostState to make it clear that the function does not
make use of the PHB.

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

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 17d9960aa1..65ed1f9eb4 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -51,27 +51,11 @@ static void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *child,
     }
 }
 
-/*
- * Attach a root port device.
- *
- * 'index' will be used both as a PCIE slot value and to calculate
- * QOM id. 'chip_id' is going to be used as PCIE chassis for the
- * root port.
- */
-static void pnv_phb_attach_root_port(PCIHostState *pci)
+static void pnv_phb_attach_root_port(PCIBus *bus)
 {
     PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
-    const char *dev_id = DEVICE(root)->id;
-    g_autofree char *default_id = NULL;
-    int index;
-
-    index = object_property_get_int(OBJECT(pci->bus), "phb-id", &error_fatal);
-    default_id = g_strdup_printf("%s[%d]", TYPE_PNV_PHB_ROOT_PORT, index);
-
-    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
-                              OBJECT(root));
 
-    pci_realize_and_unref(root, pci->bus, &error_fatal);
+    pci_realize_and_unref(root, bus, &error_fatal);
 }
 
 /*
@@ -171,7 +155,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    pnv_phb_attach_root_port(pci);
+    pnv_phb_attach_root_port(pci->bus);
 }
 
 static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
@@ -240,12 +224,18 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
 {
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
     PnvPHBRootPort *phb_rp = PNV_PHB_ROOT_PORT(dev);
-    PCIBus *bus = PCI_BUS(qdev_get_parent_bus(dev));
+    BusState *qbus = qdev_get_parent_bus(dev);
+    PCIBus *bus = PCI_BUS(qbus);
     PCIDevice *pci = PCI_DEVICE(dev);
     uint16_t device_id = 0;
     Error *local_err = NULL;
     int chip_id, index;
 
+    /*
+     * 'index' will be used both as a PCIE slot value and to calculate
+     * QOM id. 'chip_id' is going to be used as PCIE chassis for the
+     * root port.
+     */
     chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
     index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
 
@@ -253,6 +243,11 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
     qdev_prop_set_uint8(dev, "chassis", chip_id);
     qdev_prop_set_uint16(dev, "slot", index);
 
+    pnv_parent_qom_fixup(OBJECT(bus), OBJECT(dev), index);
+    if (!qdev_set_parent_bus(dev, qbus, &error_fatal)) {
+        return;
+    }
+
     rpc->parent_realize(dev, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-- 
2.36.1

Re: [PATCH for-7.2 v4 11/11] ppc/pnv: fix QOM parenting of user creatable root ports

[Frederic Barrat]

On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
> User creatable root ports are being parented by the 'peripheral' or the
> 'peripheral-anon' container. This happens because this is the regular
> QOM schema for sysbus devices that are added via the command line.
> 
> Let's make this QOM hierarchy similar to what we have with default root
> ports, i.e. the root port must be parented by the pnv-root-bus. To do
> that we change the qom and bus parent of the root port during
> root_port_realize(). The realize() is shared by the default root port
> code path, so we can remove the code inside pnv_phb_attach_root_port()
> that was adding the root port as a child of the bus as well.
> 
> While we're at it, change pnv_phb_attach_root_port() to receive a PCIBus
> instead of a PCIHostState to make it clear that the function does not
> make use of the PHB.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb.c | 35 +++++++++++++++--------------------
>   1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 17d9960aa1..65ed1f9eb4 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -51,27 +51,11 @@ static void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *child,
>       }
>   }
>   
> -/*
> - * Attach a root port device.
> - *
> - * 'index' will be used both as a PCIE slot value and to calculate
> - * QOM id. 'chip_id' is going to be used as PCIE chassis for the
> - * root port.
> - */
> -static void pnv_phb_attach_root_port(PCIHostState *pci)
> +static void pnv_phb_attach_root_port(PCIBus *bus)
>   {
>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
> -    const char *dev_id = DEVICE(root)->id;
> -    g_autofree char *default_id = NULL;
> -    int index;
> -
> -    index = object_property_get_int(OBJECT(pci->bus), "phb-id", &error_fatal);
> -    default_id = g_strdup_printf("%s[%d]", TYPE_PNV_PHB_ROOT_PORT, index);
> -
> -    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
> -                              OBJECT(root));
>   
> -    pci_realize_and_unref(root, pci->bus, &error_fatal);
> +    pci_realize_and_unref(root, bus, &error_fatal);
>   }
>   
>   /*
> @@ -171,7 +155,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    pnv_phb_attach_root_port(pci);
> +    pnv_phb_attach_root_port(pci->bus);
>   }
>   
>   static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
> @@ -240,12 +224,18 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PnvPHBRootPort *phb_rp = PNV_PHB_ROOT_PORT(dev);
> -    PCIBus *bus = PCI_BUS(qdev_get_parent_bus(dev));
> +    BusState *qbus = qdev_get_parent_bus(dev);
> +    PCIBus *bus = PCI_BUS(qbus);
>       PCIDevice *pci = PCI_DEVICE(dev);
>       uint16_t device_id = 0;
>       Error *local_err = NULL;
>       int chip_id, index;
>   
> +    /*
> +     * 'index' will be used both as a PCIE slot value and to calculate
> +     * QOM id. 'chip_id' is going to be used as PCIE chassis for the
> +     * root port.
> +     */
>       chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
>       index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
>   
> @@ -253,6 +243,11 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>       qdev_prop_set_uint8(dev, "chassis", chip_id);
>       qdev_prop_set_uint16(dev, "slot", index);
>   
> +    pnv_parent_qom_fixup(OBJECT(bus), OBJECT(dev), index);
> +    if (!qdev_set_parent_bus(dev, qbus, &error_fatal)) {


That line looks surprising at first, because we got qbus from 
qdev_get_parent_bus() just above, so it looks like a noop. I talked to 
Daniel about it: the device<->bus relationship is correct when entering 
the function but the call to pnv_parent_qom_fixup() interferes with the 
bus relationship, so it needs to be re-established.
Short of a better suggestion, it probably need a comment.

   Fred



> +        return;
> +    }
> +
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);

Re: [PATCH for-7.2 v4 11/11] ppc/pnv: fix QOM parenting of user creatable root ports

[Daniel Henrique Barboza]

On 8/12/22 13:13, Frederic Barrat wrote:
> 
> 
> On 11/08/2022 18:39, Daniel Henrique Barboza wrote:
>> User creatable root ports are being parented by the 'peripheral' or the
>> 'peripheral-anon' container. This happens because this is the regular
>> QOM schema for sysbus devices that are added via the command line.
>>
>> Let's make this QOM hierarchy similar to what we have with default root
>> ports, i.e. the root port must be parented by the pnv-root-bus. To do
>> that we change the qom and bus parent of the root port during
>> root_port_realize(). The realize() is shared by the default root port
>> code path, so we can remove the code inside pnv_phb_attach_root_port()
>> that was adding the root port as a child of the bus as well.
>>
>> While we're at it, change pnv_phb_attach_root_port() to receive a PCIBus
>> instead of a PCIHostState to make it clear that the function does not
>> make use of the PHB.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb.c | 35 +++++++++++++++--------------------
>>   1 file changed, 15 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
>> index 17d9960aa1..65ed1f9eb4 100644
>> --- a/hw/pci-host/pnv_phb.c
>> +++ b/hw/pci-host/pnv_phb.c
>> @@ -51,27 +51,11 @@ static void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *child,
>>       }
>>   }
>> -/*
>> - * Attach a root port device.
>> - *
>> - * 'index' will be used both as a PCIE slot value and to calculate
>> - * QOM id. 'chip_id' is going to be used as PCIE chassis for the
>> - * root port.
>> - */
>> -static void pnv_phb_attach_root_port(PCIHostState *pci)
>> +static void pnv_phb_attach_root_port(PCIBus *bus)
>>   {
>>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
>> -    const char *dev_id = DEVICE(root)->id;
>> -    g_autofree char *default_id = NULL;
>> -    int index;
>> -
>> -    index = object_property_get_int(OBJECT(pci->bus), "phb-id", &error_fatal);
>> -    default_id = g_strdup_printf("%s[%d]", TYPE_PNV_PHB_ROOT_PORT, index);
>> -
>> -    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
>> -                              OBJECT(root));
>> -    pci_realize_and_unref(root, pci->bus, &error_fatal);
>> +    pci_realize_and_unref(root, bus, &error_fatal);
>>   }
>>   /*
>> @@ -171,7 +155,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>>           return;
>>       }
>> -    pnv_phb_attach_root_port(pci);
>> +    pnv_phb_attach_root_port(pci->bus);
>>   }
>>   static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
>> @@ -240,12 +224,18 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>>   {
>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>>       PnvPHBRootPort *phb_rp = PNV_PHB_ROOT_PORT(dev);
>> -    PCIBus *bus = PCI_BUS(qdev_get_parent_bus(dev));
>> +    BusState *qbus = qdev_get_parent_bus(dev);
>> +    PCIBus *bus = PCI_BUS(qbus);
>>       PCIDevice *pci = PCI_DEVICE(dev);
>>       uint16_t device_id = 0;
>>       Error *local_err = NULL;
>>       int chip_id, index;
>> +    /*
>> +     * 'index' will be used both as a PCIE slot value and to calculate
>> +     * QOM id. 'chip_id' is going to be used as PCIE chassis for the
>> +     * root port.
>> +     */
>>       chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
>>       index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
>> @@ -253,6 +243,11 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>>       qdev_prop_set_uint8(dev, "chassis", chip_id);
>>       qdev_prop_set_uint16(dev, "slot", index);
>> +    pnv_parent_qom_fixup(OBJECT(bus), OBJECT(dev), index);
>> +    if (!qdev_set_parent_bus(dev, qbus, &error_fatal)) {
> 
> 
> That line looks surprising at first, because we got qbus from qdev_get_parent_bus() just above, so it looks like a noop. I talked to Daniel about it: the device<->bus relationship is correct when entering the function but the call to pnv_parent_qom_fixup() interferes with the bus relationship, so it needs to be re-established.

The more detailed version: when adding a child property
using object_property_add_child(), object_property_try_add_child() is called.
This function calls internally a object_property_try_add() function with lots
of parameters, including a release callback named 'object_finalize_child_property'.

This release callback is implemented like this:

static void object_finalize_child_property(Object *obj, const char *name,
                                            void *opaque)
{
     Object *child = opaque;

     if (child->class->unparent) {
         (child->class->unparent)(child);
     }
     child->parent = NULL;
     object_unref(child);
}

If you're adding a device as a child, which is our case here,
child->class->unparent is device_unparent(). Note that device_unparent()
removes the device from its parent_bus:

static void device_unparent(Object *obj)
{
     DeviceState *dev = DEVICE(obj);
     BusState *bus;

     if (dev->realized) {
         qdev_unrealize(dev);
     }
     while (dev->num_child_bus) {
         bus = QLIST_FIRST(&dev->child_bus);
         object_unparent(OBJECT(bus));
     }
     if (dev->parent_bus) {
         bus_remove_child(dev->parent_bus, dev);
         object_unref(OBJECT(dev->parent_bus));
         dev->parent_bus = NULL;
     }
}

So, back in our qom_fixup() helper, we're doing an object_unparent(). This
function calls object_property_del_child(), which in turn calls the
property release callback prop->release if it's defined. It is defined,
and in our case it's the object_finalize_child_property() from above that will
end up calling device_unparent(), which will remove the device from the bus.

This is why I'm having to do a qdev_set_parent_bus() to assign the root port
to the same parent bus it had before. We want to fixup it's QOM parent without
changing its parent bus.

Also, note that we're doing the same thing for the user created pnv-phbs in
pnv_phb_user_device_init():

     pnv_parent_qom_fixup(parent, OBJECT(phb), phb->phb_id);
     pnv_parent_bus_fixup(DEVICE(chip), DEVICE(phb));

The difference here is that the QOM parent is not the same as the parent bus,
so it's less apparent what we're doing here.

All that said, since we're always calling these 2 fixup helpers together, I think
it's a good idea to create a single helper that handles both the QOM parenting and
the parent bus. I can also add some comments about what I've said here to explain
why we need to set the parent bus after doing an object_unparent().

> Short of a better suggestion, it probably need a comment.

I don't think we have wiggle room since we're dealing with deep QOM and qdev internals
in this case. I'm all ears for suggestions though.


Thanks,


Daniel

> 
>    Fred
> 
> 
> 
>> +        return;
>> +    }
>> +
>>       rpc->parent_realize(dev, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);