Re: [PATCH v2 17/18] ppc/pnv: Introduce user creatable pnv-phb4 devices

["Cédric Le Goater" <clg@kaod.org>]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> This patch introduces pnv-phb4 user creatable devices that are created
> in a similar manner as pnv-phb3 devices, allowing the user to interact
> with the PHBs directly instead of creating PCI Express Controllers that
> will create a certain amount of PHBs per controller index.
> 
> First thing we need is to discover which stack will host the created
> PHB, which is done by the new pnv_phb4_get_stack() function. During
> pnv_phb4_realize() we'll inspect phb->stack to see if we're dealing with
> an user creatable device or not. When using default settings, the
> automatically created PHB4 devices will be realized with phb->stack
> already assigned beforehand during PEC realize. In case we're dealing
> with an user device, find its stack, set the PHB attributes based on the
> stack it belongs and assign the PHB to the stack.
> 
> The xscom stack initialization takes place in pnv_pec_stk_realize() when
> using default settings, but at that point we aren't aware of any user
> PHB4 devices that will belong to the stack. In that case we'll postpone
> xscom stack init until the the end of pnv_phb4_realize(), after all the
> memory mappings of the PHB are done.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c     | 84 +++++++++++++++++++++++++++++++++++++-
>   hw/pci-host/pnv_phb4_pec.c | 12 +++---
>   hw/ppc/pnv.c               |  2 +
>   3 files changed, 90 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 430a5c10f4..1c2334d491 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1236,6 +1236,41 @@ static void pnv_phb4_instance_init(Object *obj)
>       object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
>   }
>   
> +static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
> +                                           Error **errp)
> +{
> +    Pnv9Chip *chip9 = NULL;
> +    int chip_id = phb->chip_id;
> +    int index = phb->phb_id;
> +    int i, j;
> +
> +    if (chip->num_pecs == 0) {

assert() maybe ?


> +        /* Something weird happened. Bail out */
> +        error_setg(errp, "chip id %d has no PCIE controllers", chip_id);
> +        return NULL;
> +    }
> +
> +    chip9 = PNV9_CHIP(chip);
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
> +        /*
> +         * For each PEC, check the amount of stacks it supports
> +         * and see if the given phb4 index matches a stack.
> +         */
> +        PnvPhb4PecState *pec = &chip9->pecs[i];
> +
> +        for (j = 0; j < pec->num_stacks; j++) {
> +            if (index == pnv_phb4_pec_get_phb_id(pec, j)) {
> +                return &pec->stacks[j];
> +            }
> +        }
> +    }
> +
> +    error_setg(errp, "pnv-phb4 index %d didn't match any existing PEC",
> +               chip_id);
> +    return NULL;
> +}
> +
>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB4 *phb = PNV_PHB4(dev);
> @@ -1243,8 +1278,49 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       XiveSource *xsrc = &phb->xsrc;
>       int nr_irqs;
>       char name[32];
> +    PnvPhb4PecStack *stack = NULL;
> +    bool stack_init_xscom = false;
> +    Error *local_err = NULL;
>   
> -    assert(phb->stack);
> +    /* User created PHB */
> +    if (!phb->stack) {
> +        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +        PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
> +        BusState *s;
> +
> +        if (!chip) {
> +            error_setg(errp, "invalid chip id: %d", phb->chip_id);
> +            return;
> +        }
> +
> +        stack = pnv_phb4_get_stack(chip, phb, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +
> +        object_property_set_int(OBJECT(phb), "index",
> +                                phb->phb_id, &error_abort);
> +
> +        pnv_phb4_set_stack_phb_props(stack, phb);
> +
> +        /* Assign the phb to the stack */
> +        stack->phb = phb;

The stack object should check the validity of the stack->phb pointer always.

> +
> +        /*
> +         * Reparent user created devices to the chip to build
> +         * correctly the device tree.
> +         */
> +        pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
> +
> +        s = qdev_get_parent_bus(DEVICE(chip));
> +        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +
> +        stack_init_xscom = true;

This looks wrong.

> +    }
>   
>       /* Set the "big_phb" flag */
>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
> @@ -1298,6 +1374,10 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       pnv_phb4_update_xsrc(phb);
>   
>       phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
> +
> +    if (stack_init_xscom) {
> +        pnv_pec_init_stack_xscom(stack);
> +    }

Isn't it done under the pnv_pec_stk_realize() routine already ?

>   }
>   
>   static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
> @@ -1347,7 +1427,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data)
>       dc->realize         = pnv_phb4_realize;
>       device_class_set_props(dc, pnv_phb4_properties);
>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    dc->user_creatable  = false;
> +    dc->user_creatable  = true;
>   
>       xfc->notify         = pnv_phb4_xive_notify;
>   }
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 41c79d24c4..4417beb97d 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -573,13 +573,13 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>                             &pnv_pec_stk_pci_xscom_ops, stack, name,
>                             PHB4_PEC_PCI_STK_REGS_COUNT);
>   
> -    /* PHB pass-through */
> -    pnv_phb4_set_stack_phb_props(stack, stack->phb);
> -    if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
> -        return;
> +    /*
> +     * There is no guarantee that stack->phb will be available
> +     * at this point.
> +     */
> +    if (stack->phb) {
> +        pnv_pec_init_stack_xscom(stack);
>       }
> -
> -    pnv_pec_init_stack_xscom(stack);


This looks wrong also. I don't understand why.

Thanks,

C.


>   }
>   
>   static Property pnv_pec_stk_properties[] = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index fe7e67e73a..837146a2fb 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1960,6 +1960,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_PHB4);
>   }
>   
>   static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
>