Re: [PATCH v3 09/10] ppc/pnv: Introduce user creatable pnv-phb4 devices

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

On 1/10/22 15:33, 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.
> 
> We accomplish this by doing the following:
> 
> - add a pnv_phb4_get_stack() helper to retrieve which stack an user
> created phb4 would occupy;
> 
> - if a suitable pec->stack is found, setup the phb attributes in a
> similar fashion as done in pnv_phb4_pec_realize() when defaults are
> enabled;
> 
> - use 'defaults_enabled()' in pnv_pec_stk_instance_init() to avoid
> creating a 'stack->phb' qdev that might be overwritten by an user
> created pnv-phb4 device.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c     | 86 +++++++++++++++++++++++++++++++++++++-
>   hw/pci-host/pnv_phb4_pec.c | 25 ++++++-----
>   hw/ppc/pnv.c               |  2 +
>   3 files changed, 101 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index fc23a96b7f..8c8f5dd0e1 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1229,15 +1229,97 @@ 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;


Change the pnv_phb4_get_stack() prototype to :

   static PnvPhb4PecStack *pnv_phb4_get_stack(int chip_id, int index,
                                              Error **errp);

it will simplify the pnv_phb4_realize() routine.


> +    int i, j;
> +
> +    if (chip->num_pecs == 0) {
> +        /*
> +         * This is expected to happen since chip-id and index are
> +         * being set by the user in the command line. Return an
> +         * informative error instead of asserting.
> +         */

I don't understand how this can happen since num_pecs is initialized :
  
   chip->num_pecs = pcc->num_pecs

and

   k->num_pecs = PNV9_CHIP_MAX_PEC;

> +        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 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);
>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>       XiveSource *xsrc = &phb->xsrc;
> +    PnvPhb4PecStack *stack = NULL;
> +    Error *local_err = NULL;
>       int nr_irqs;
>       char name[32];
>   
> -    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;
> +        }
> +
> +        /*
> +         * Assign the phb to the stack. If pnv_phb4_get_stack() returned
> +         * stack = NULL without an error we're better of aborting.
> +         */
> +        g_assert(stack);

This can not happend if pnv_phb4_get_stack() returns local_err;

> +        stack->phb = phb;
> +
> +        object_property_set_int(OBJECT(phb), "index",
> +                                phb->phb_id, &error_abort);

This is redundant. "index" has already been set by the user if the PHB
is user created or by the PnvPhb4PecStack realize routine.

> +        pnv_phb4_set_stack_phb_props(stack, phb);

I have the feeling this should be called after the 'if (!phb->stack)'
code section and not in pnv_pec_realize() like PATCH 5 does.

> +
> +        /*
> +         * 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;
> +        }
> +    }
>   
>       /* Set the "big_phb" flag */
>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
> @@ -1342,7 +1424,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 5e02a51f04..1e3233e7ec 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -19,6 +19,7 @@
>   #include "hw/pci/pci_bus.h"
>   #include "hw/ppc/pnv.h"
>   #include "hw/qdev-properties.h"
> +#include "sysemu/sysemu.h"
>   
>   #include <libfdt.h>
>   
> @@ -397,15 +398,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
>           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
>   
> -        /*
> -         * stack->phb->index is dependent on the position the
> -         * stack occupies in pec->stacks[]. We have this information
> -         * available here via the 'i' iterator so it's convenient to
> -         * do it now.
> -         */
> -        object_property_set_int(OBJECT(stack->phb), "index", phb_id,
> -                                &error_abort);
> -        pnv_phb4_set_stack_phb_props(stack, stack->phb);
> +        if (defaults_enabled()) {
> +            /*
> +             * stack->phb->index is dependent on the position the
> +             * stack occupies in pec->stacks[]. We have this information
> +             * available here via the 'i' iterator so it's convenient to
> +             * do it now.
> +             */
> +            object_property_set_int(OBJECT(stack->phb), "index", phb_id,
> +                                    &error_abort);
> +            pnv_phb4_set_stack_phb_props(stack, stack->phb);
> +        }

This clearly belongs to PHB4 realize handler.

Thanks,

C.

>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
> @@ -543,7 +546,9 @@ static void pnv_pec_stk_instance_init(Object *obj)
>   {
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
>   
> -    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> +    if (defaults_enabled()) {
> +        stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> +    }
>   }
>   
>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> 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)
>