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

[Cédric Le Goater]

On 8/10/22 12:05, 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.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb.c | 76 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 76 insertions(+)
>
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 826c0c144e..18a050a6d3 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -18,6 +18,37 @@
>   #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)
> +{
> +    BusState *parent_bus = qdev_get_parent_bus(parent);
> +
> +    if (!qdev_set_parent_bus(child, parent_bus, &error_fatal)) {
> +        return;
> +    }
> +}
> +
>   /*
>    * Attach a root port device.
>    *
> @@ -41,6 +72,43 @@ 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 void pnv_phb_user_device_init(PnvPHB *phb)

Why not pass an errp to this routine ?

> +{
> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
> +    Object *parent = NULL;
> +
> +    if (!chip) {
> +        error_setg(&error_fatal, "invalid chip id: %d", phb->chip_id);

that you could use here.

> +        return;
> +    }
> +
> +    if (phb->version == 3) {
> +        parent = OBJECT(chip);
> +    } else {
> +        /* phb4 support will be added later */
> +        return;
> +    }
> +
> +    /*
> +     * 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.
> +     *
> +     * 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.
> +     */
> +    pnv_parent_qom_fixup(parent, OBJECT(phb), phb->phb_id);
> +    pnv_parent_bus_fixup(DEVICE(chip), DEVICE(phb));

and in pnv_parent_bus_fixup() also,

> +}
> +
>   static void pnv_phb_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB *phb = PNV_PHB(dev);
> @@ -74,6 +142,14 @@ 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) {
> +        pnv_phb_user_device_init(phb);

and return the error message to the caller of pnv_phb_realize()

C.


> +    }
> +
>       if (phb->version == 3) {
>           object_property_set_link(phb->backend, "chip",
>                                    OBJECT(phb->chip), errp);