Re: [PATCH v2 08/16] ppc/pnv: user created pnv-phb for powernv9

[Frederic Barrat]

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)