To enable user creatable PnvPHB devices for powernv9 we'll revert the
powernv9 related changes made in 9c10d86fee "ppc/pnv: Remove
user-created PHB{3,4,5} devices".

This change alone isn't enough to enable user creatable devices for powernv10
due to how pnv_phb4_get_pec() currently works. For now let's just enable it
for powernv9 alone.

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 22cf1c2a5e..a5c8ae494b 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1571,13 +1571,69 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
     pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
 }
 
+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;
+}
+
 static void pnv_phb4_realize(DeviceState *dev, Error **errp)
 {
     PnvPHB4 *phb = PNV_PHB4(dev);
+    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
+    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
     XiveSource *xsrc = &phb->xsrc;
+    BusState *s;
+    Error *local_err = NULL;
     int nr_irqs;
     char name[32];
 
+    if (!chip) {
+        error_setg(errp, "invalid chip id: %d", phb->chip_id);
+        return;
+    }
+
+    /* User created PHBs need to be assigned to a PEC */
+    if (!phb->pec) {
+        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    /* Reparent the PHB to the chip to build the device tree */
+    pnv_chip_parent_fixup(chip, OBJECT(phb->phb_base), phb->phb_id);
+
+    s = qdev_get_parent_bus(DEVICE(chip));
+    if (!qdev_set_parent_bus(DEVICE(phb->phb_base), s, &local_err)) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     /* Set the "big_phb" flag */
     phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
 
@@ -1803,7 +1859,7 @@ static void pnv_phb4_root_port_class_init(ObjectClass *klass, void *data)
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
 
     dc->desc     = "IBM PHB4 PCIE Root Port";
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 
     device_class_set_parent_realize(dc, pnv_phb4_root_port_realize,
                                     &rpc->parent_realize);
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 888ecbe8f3..0e67f3a338 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 3b0b230e49..697a2b5302 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2186,6 +2186,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

On 31/05/2022 23:49, Daniel Henrique Barboza wrote:
> To enable user creatable PnvPHB devices for powernv9 we'll revert the
> powernv9 related changes made in 9c10d86fee "ppc/pnv: Remove
> user-created PHB{3,4,5} devices".
> 
> This change alone isn't enough to enable user creatable devices for powernv10
> due to how pnv_phb4_get_pec() currently works. For now let's just enable it
> for powernv9 alone.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c     | 58 +++++++++++++++++++++++++++++++++++++-
>   hw/pci-host/pnv_phb4_pec.c |  6 ++--
>   hw/ppc/pnv.c               |  2 ++
>   3 files changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 22cf1c2a5e..a5c8ae494b 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1571,13 +1571,69 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   }
>   
> +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;
> +}
> +
>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB4 *phb = PNV_PHB4(dev);
> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
>       XiveSource *xsrc = &phb->xsrc;
> +    BusState *s;
> +    Error *local_err = NULL;
>       int nr_irqs;
>       char name[32];
>   
> +    if (!chip) {
> +        error_setg(errp, "invalid chip id: %d", phb->chip_id);
> +        return;
> +    }
> +
> +    /* User created PHBs need to be assigned to a PEC */
> +    if (!phb->pec) {
> +        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
> +    /* Reparent the PHB to the chip to build the device tree */
> +    pnv_chip_parent_fixup(chip, OBJECT(phb->phb_base), phb->phb_id);


Didn't you mean to do that only for the user-created case? And why not 
attaching the PHB to the PEC instead of the chip, like in 
pnv_pec_default_phb_realize() ? I think it makes more sense to keep the 
chip->PEC->PHB parenting in the qom tree (and incidentally, that's where 
I'd prefer to have the phb3 model move to).
Also, the comment seems wrong to me. The qom parenting doesn't matter 
when building the device tree. We only iterate over the PHBs found in 
the array of the PEC object (cf. pnv_pec_dt_xscom())



> +    s = qdev_get_parent_bus(DEVICE(chip));
> +    if (!qdev_set_parent_bus(DEVICE(phb->phb_base), s, &local_err)) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }


Same comment, I think that's only desirable for user-created devices. 
We're already calling sysbus_realize() for the default case.


Silly question: where does it break if a user tries to create 2 PHBs 
with the same index?


   Fred




>       /* Set the "big_phb" flag */
>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
>   
> @@ -1803,7 +1859,7 @@ static void pnv_phb4_root_port_class_init(ObjectClass *klass, void *data)
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
>   
>       dc->desc     = "IBM PHB4 PCIE Root Port";
> -    dc->user_creatable = false;
> +    dc->user_creatable = true;
>   
>       device_class_set_parent_realize(dc, pnv_phb4_root_port_realize,
>                                       &rpc->parent_realize);
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 888ecbe8f3..0e67f3a338 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 3b0b230e49..697a2b5302 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -2186,6 +2186,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)

On 6/2/22 13:33, Frederic Barrat wrote:
> 
> 
> On 31/05/2022 23:49, Daniel Henrique Barboza wrote:
>> To enable user creatable PnvPHB devices for powernv9 we'll revert the
>> powernv9 related changes made in 9c10d86fee "ppc/pnv: Remove
>> user-created PHB{3,4,5} devices".
>>
>> This change alone isn't enough to enable user creatable devices for powernv10
>> due to how pnv_phb4_get_pec() currently works. For now let's just enable it
>> for powernv9 alone.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb4.c     | 58 +++++++++++++++++++++++++++++++++++++-
>>   hw/pci-host/pnv_phb4_pec.c |  6 ++--
>>   hw/ppc/pnv.c               |  2 ++
>>   3 files changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 22cf1c2a5e..a5c8ae494b 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1571,13 +1571,69 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
>>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>>   }
>> +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;
>> +}
>> +
>>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvPHB4 *phb = PNV_PHB4(dev);
>> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>> +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
>>       XiveSource *xsrc = &phb->xsrc;
>> +    BusState *s;
>> +    Error *local_err = NULL;
>>       int nr_irqs;
>>       char name[32];
>> +    if (!chip) {
>> +        error_setg(errp, "invalid chip id: %d", phb->chip_id);
>> +        return;
>> +    }
>> +
>> +    /* User created PHBs need to be assigned to a PEC */
>> +    if (!phb->pec) {
>> +        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +    }
>> +
>> +    /* Reparent the PHB to the chip to build the device tree */
>> +    pnv_chip_parent_fixup(chip, OBJECT(phb->phb_base), phb->phb_id);
> 
> 
> Didn't you mean to do that only for the user-created case? 

It's done for both because the default generated PHBs are being created loosely
from the chip starting on 3f4c369ea63e ("ppc/pnv: make PECs create and realize
PHB4s"). We had to amend the code after that commit to fix the QOM hierarchy
of these devices. 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM hierarchy") has
more details.


> And why not attaching the PHB to the PEC instead of the chip, like in pnv_pec_default_phb_realize() ? I think it makes more sense to keep the chip->PEC->PHB parenting in the qom tree (and incidentally, that's where I'd prefer to have the phb3 model move to).

I can only speculate since I didn't participate in the initial design. My
educated guess is that we didn't want to show PECs in qtree, hence we
made the PHB a child of the chip instead. I'll also guess that this can be
due to how PHB3 worked and we just copied the existing design.

> Also, the comment seems wrong to me. The qom parenting doesn't matter when building the device tree. We only iterate over the PHBs found in the array of the PEC object (cf. pnv_pec_dt_xscom())

I believe it refers to the QOM tree, a.k.a qtree. This has no relation to the
actual device tree the kernel uses. This comment can be clearer though.


Thanks,


Daniel

> 
> 
> 
>> +    s = qdev_get_parent_bus(DEVICE(chip));
>> +    if (!qdev_set_parent_bus(DEVICE(phb->phb_base), s, &local_err)) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
> 
> 
> Same comment, I think that's only desirable for user-created devices. We're already calling sysbus_realize() for the default case.
> 
> 
> Silly question: where does it break if a user tries to create 2 PHBs with the same index?
> 
> 
>    Fred
> 
> 
> 
> 
>>       /* Set the "big_phb" flag */
>>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
>> @@ -1803,7 +1859,7 @@ static void pnv_phb4_root_port_class_init(ObjectClass *klass, void *data)
>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
>>       dc->desc     = "IBM PHB4 PCIE Root Port";
>> -    dc->user_creatable = false;
>> +    dc->user_creatable = true;
>>       device_class_set_parent_realize(dc, pnv_phb4_root_port_realize,
>>                                       &rpc->parent_realize);
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 888ecbe8f3..0e67f3a338 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 3b0b230e49..697a2b5302 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -2186,6 +2186,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)

On 03/06/2022 23:00, Daniel Henrique Barboza wrote:
>>>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>>   {
>>>       PnvPHB4 *phb = PNV_PHB4(dev);
>>> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>>> +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
>>>       XiveSource *xsrc = &phb->xsrc;
>>> +    BusState *s;
>>> +    Error *local_err = NULL;
>>>       int nr_irqs;
>>>       char name[32];
>>> +    if (!chip) {
>>> +        error_setg(errp, "invalid chip id: %d", phb->chip_id);
>>> +        return;
>>> +    }
>>> +
>>> +    /* User created PHBs need to be assigned to a PEC */
>>> +    if (!phb->pec) {
>>> +        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
>>> +        if (local_err) {
>>> +            error_propagate(errp, local_err);
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    /* Reparent the PHB to the chip to build the device tree */
>>> +    pnv_chip_parent_fixup(chip, OBJECT(phb->phb_base), phb->phb_id);
>>
>>
>> Didn't you mean to do that only for the user-created case? 
> 
> It's done for both because the default generated PHBs are being created 
> loosely
> from the chip starting on 3f4c369ea63e ("ppc/pnv: make PECs create and 
> realize
> PHB4s"). We had to amend the code after that commit to fix the QOM 
> hierarchy
> of these devices. 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM 
> hierarchy") has
> more details.
> 
> 
>> And why not attaching the PHB to the PEC instead of the chip, like in 
>> pnv_pec_default_phb_realize() ? I think it makes more sense to keep 
>> the chip->PEC->PHB parenting in the qom tree (and incidentally, that's 
>> where I'd prefer to have the phb3 model move to).
> 
> I can only speculate since I didn't participate in the initial design. My
> educated guess is that we didn't want to show PECs in qtree, hence we
> made the PHB a child of the chip instead. I'll also guess that this can be
> due to how PHB3 worked and we just copied the existing design.


I don't believe that's correct though. As Cedric replied, the PECs show 
up in the qom tree on P9/P10, with chip->PEC->PHB, in that order. And I 
think that's fine, that's the model we should tend to (and which would 
require a fix on P8, since there we have chip->phb->pbcq, which is 
backwards. The pbcq object is the equivalent of the PEC).

So I think we should keep the qom relationship as it currently is on 
P9/P10, since it looks good. On P8, we can keep the current status for 
now and, as discussed privately, have another series later to clean 
things up.

   Fred

On 6/3/22 23:00, Daniel Henrique Barboza wrote:
> 
> 
> On 6/2/22 13:33, Frederic Barrat wrote:
>>
>>
>> On 31/05/2022 23:49, Daniel Henrique Barboza wrote:
>>> To enable user creatable PnvPHB devices for powernv9 we'll revert the
>>> powernv9 related changes made in 9c10d86fee "ppc/pnv: Remove
>>> user-created PHB{3,4,5} devices".
>>>
>>> This change alone isn't enough to enable user creatable devices for powernv10
>>> due to how pnv_phb4_get_pec() currently works. For now let's just enable it
>>> for powernv9 alone.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>>   hw/pci-host/pnv_phb4.c     | 58 +++++++++++++++++++++++++++++++++++++-
>>>   hw/pci-host/pnv_phb4_pec.c |  6 ++--
>>>   hw/ppc/pnv.c               |  2 ++
>>>   3 files changed, 63 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>>> index 22cf1c2a5e..a5c8ae494b 100644
>>> --- a/hw/pci-host/pnv_phb4.c
>>> +++ b/hw/pci-host/pnv_phb4.c
>>> @@ -1571,13 +1571,69 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
>>>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>>>   }
>>> +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;
>>> +}
>>> +
>>>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>>   {
>>>       PnvPHB4 *phb = PNV_PHB4(dev);
>>> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>>> +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
>>>       XiveSource *xsrc = &phb->xsrc;
>>> +    BusState *s;
>>> +    Error *local_err = NULL;
>>>       int nr_irqs;
>>>       char name[32];
>>> +    if (!chip) {
>>> +        error_setg(errp, "invalid chip id: %d", phb->chip_id);
>>> +        return;
>>> +    }
>>> +
>>> +    /* User created PHBs need to be assigned to a PEC */
>>> +    if (!phb->pec) {
>>> +        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
>>> +        if (local_err) {
>>> +            error_propagate(errp, local_err);
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    /* Reparent the PHB to the chip to build the device tree */
>>> +    pnv_chip_parent_fixup(chip, OBJECT(phb->phb_base), phb->phb_id);
>>
>>
>> Didn't you mean to do that only for the user-created case? 
> 
> It's done for both because the default generated PHBs are being created loosely
> from the chip starting on 3f4c369ea63e ("ppc/pnv: make PECs create and realize
> PHB4s"). We had to amend the code after that commit to fix the QOM hierarchy
> of these devices. 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM hierarchy") has
> more details.
> 
> 
>> And why not attaching the PHB to the PEC instead of the chip, like in pnv_pec_default_phb_realize() ? I think it makes more sense to keep the chip->PEC->PHB parenting in the qom tree (and incidentally, that's where I'd prefer to have the phb3 model move to).
>
> I can only speculate since I didn't participate in the initial design. My
> educated guess is that we didn't want to show PECs in qtree, 

PECs are low level pieces of logic, that don't need to be exposed
under the monitor. We need the models for the FW though.

> hence we made the PHB a child of the chip instead. 

Did we ? see below.

> I'll also guess that this can be
> due to how PHB3 worked and we just copied the existing design.


hmm, I am not following what you are saying. Here is what we have :

P8:

/machine (powernv8-machine)
   /chip[0] (power8_v2.0-pnv-chip)
     /phb[0] (pnv-phb3)
       /lsi (ics)
       /msi (phb3-msi)
       /pbcq (pnv-pbcq)
         /xscom-pbcq-nest-0.0[0] (memory-region)
         /xscom-pbcq-pci-0.0[0] (memory-region)
         /xscom-pbcq-spci-0.0[0] (memory-region)
       /pnv-phb3-root.0 (pnv-phb3-root)

   /unattached (container)
     /device[1] (pnv-phb3-root-port)

P9:

/machine (powernv9-machine)
   /chip[0] (power9_v2.0-pnv-chip)
     /pec[0] (pnv-phb4-pec)
       /phb[0] (pnv-phb4)
         /pnv-phb4-root.0 (pnv-phb4-root)
         /source (xive-source)
         /xscom-pec-0.0-nest-phb-0[0] (memory-region)
         /xscom-pec-0.0-pci-phb-0[0] (memory-region)
         /xscom-pec-0.0-pci-phb-0[1] (memory-region)
     ...

   /unattached (container)
     /device[1] (pnv-phb4-root-port)


The RP was detached for some libvirt reason I think.

We would like to keep the same QOM hierarchy and possibly fixed
the modeling of PHB3 which is reversed.


> 
>> Also, the comment seems wrong to me. The qom parenting doesn't matter when building the device tree. 

it does. See pnv_dt_xscom()

Thanks,

C.


>> We only iterate over the PHBs found in the array of the PEC object (cf. pnv_pec_dt_xscom())
> 
> I believe it refers to the QOM tree, a.k.a qtree. This has no relation to the
> actual device tree the kernel uses. This comment can be clearer though.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
>>
>>
>>
>>> +    s = qdev_get_parent_bus(DEVICE(chip));
>>> +    if (!qdev_set_parent_bus(DEVICE(phb->phb_base), s, &local_err)) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>
>>
>> Same comment, I think that's only desirable for user-created devices. We're already calling sysbus_realize() for the default case.
>>
>>
>> Silly question: where does it break if a user tries to create 2 PHBs with the same index?
>>
>>
>>    Fred
>>
>>
>>
>>
>>>       /* Set the "big_phb" flag */
>>>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
>>> @@ -1803,7 +1859,7 @@ static void pnv_phb4_root_port_class_init(ObjectClass *klass, void *data)
>>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
>>>       dc->desc     = "IBM PHB4 PCIE Root Port";
>>> -    dc->user_creatable = false;
>>> +    dc->user_creatable = true;
>>>       device_class_set_parent_realize(dc, pnv_phb4_root_port_realize,
>>>                                       &rpc->parent_realize);
>>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>>> index 888ecbe8f3..0e67f3a338 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 3b0b230e49..697a2b5302 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -2186,6 +2186,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)

On 07/06/2022 08:35, Cédric Le Goater wrote:
>>
>>> Also, the comment seems wrong to me. The qom parenting doesn't matter 
>>> when building the device tree. 
> 
> it does. See pnv_dt_xscom()


Yeah, what I meant is that on P9, there's no "dt_scom" method for the 
PHB. The PHBs are added by the dt_scom() of the PEC. So the parenting of 
the PHB doesn't really matter.

I was actually wondering why it was done that way. If we have a clean 
qom tree (again, only on P9/P10 because P8 is wrong), then the PEC could 
add the "pbcq@xxxxxx" layer in the device tree, then call the qom 
children, i.e. the PHBs, and they would each add themselves (each phb 
adds the 'stack@xxxxxx' entry in the device tree).

But then I see your comment about giving headaches for user-created 
devices. So something else to discuss...

   Fred

>>> Also, the comment seems wrong to me. The qom parenting doesn't matter when building the device tree. 
> 
> it does. See pnv_dt_xscom()

And this is the root cause of many headaches for user-created devices.
Could it be done differently ?

Thanks,

C.

On 6/7/22 03:44, Cédric Le Goater wrote:
>>>> Also, the comment seems wrong to me. The qom parenting doesn't matter when building the device tree. 
>>
>> it does. See pnv_dt_xscom()
> 
> And this is the root cause of many headaches for user-created devices.
> Could it be done differently ?

Just tried to do a related change based on the review you gave in patch 07:

----
But it would assert if child is not of the correct type. The routine
above is called from a object_child_foreach() which loops on all
children.

I think it could be improved by using directly &chip*->phbs[i].

C.
-----

This doesn't work out of the gate because, for user creatable devices, we
are not setting the phbs back into the chip->phbs[] array. In fact we're
not even incrementing chip->num_phbs. We were getting away with all of it
because we are parenting the PHBs to the chip during realize.

Considering that, and also how pnv_dt_xscom() works, I´d rather stick with
most of the the QOM usage we already have. Otherwise we would need, for example,
to change xscom_dt_child to go through each device we want in the DT. And
there's no clear benefit aside from using less QOM, but that can be amended
by adding more documentation here and there.

I can make an exception for powernv8 and pnv_ics_get_child()/pnv_ics_resend_child(),
where we're cycling through all child elements every time. For those cases it's worth
to access the phbs directly via chip->phbs[], and for user creatable phb3s I'll add
the created phb in the array.

I also believe that I can do more to make the current handling of default phb3/phb4
closer to what user creatable devices does. This will ease the work to be done
by this series and will also make the design easier to understand. I might also do
some changes that Mark pointed out in the phb3/4 root ports as well. This series can
then be more about the PnvPHB proxy.


Thanks,


Daniel




> 
> Thanks,
> 
> C.
> 
>