Re: [PATCH for-7.2 v2 2/2] ppc/pnv: fix QOM parenting of user creatable root ports

[Frederic Barrat]

On 19/08/2022 11:47, Daniel Henrique Barboza wrote:
> User creatable root ports are being parented by the 'peripheral' or the
> 'peripheral-anon' container. This happens because this is the regular
> QOM schema for sysbus devices that are added via the command line.
>
> Let's make this QOM hierarchy similar to what we have with default root
> ports, i.e. the root port must be parented by the pnv-root-bus. To do
> that we change the qom and bus parent of the root port during
> root_port_realize(). The realize() is shared by the default root port
> code path, so we can remove the code inside pnv_phb_attach_root_port()
> that was adding the root port as a child of the bus as well.
>
> After all that, remove pnv_phb_attach_root_port() and create the root
> port explictly in the 'default_enabled()' case of pnv_phb_realize().
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   hw/pci-host/pnv_phb.c | 47 ++++++++++++++++++-------------------------
>   1 file changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 4ea33fb6ba..7b11f1e8dd 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -62,29 +62,6 @@ static bool pnv_parent_fixup(Object *parent, BusState 
> *parent_bus,
>       return true;
>   }
>   
> -/*
> - * Attach a root port device.
> - *
> - * 'index' will be used both as a PCIE slot value and to calculate
> - * QOM id. 'chip_id' is going to be used as PCIE chassis for the
> - * root port.
> - */
> -static void pnv_phb_attach_root_port(PCIHostState *pci)
> -{
> -    PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
> -    const char *dev_id = DEVICE(root)->id;
> -    g_autofree char *default_id = NULL;
> -    int index;
> -
> -    index = object_property_get_int(OBJECT(pci->bus), "phb-id", &error_fatal);
> -    default_id = g_strdup_printf("%s[%d]", TYPE_PNV_PHB_ROOT_PORT, index);
> -
> -    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
> -                              OBJECT(root));
> -
> -    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
> @@ -180,11 +157,11 @@ static void pnv_phb_realize(DeviceState *dev, Error 
> **errp)
>           pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
>       }
>   
> -    if (!defaults_enabled()) {
> -        return;
> -    }
> +    if (defaults_enabled()) {
> +        PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
>   
> -    pnv_phb_attach_root_port(pci);
> +        pci_realize_and_unref(root, pci->bus, errp);
> +    }
>   }
>   
>   static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
> @@ -259,6 +236,11 @@ static void pnv_phb_root_port_realize(DeviceState *dev, 
> Error **errp)
>       Error *local_err = NULL;
>       int chip_id, index;
>   
> +    /*
> +     * 'index' will be used both as a PCIE slot value and to calculate
> +     * QOM id. 'chip_id' is going to be used as PCIE chassis for the
> +     * root port.
> +     */
>       chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
>       index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
>   
> @@ -266,6 +248,17 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>       qdev_prop_set_uint8(dev, "chassis", chip_id);
>       qdev_prop_set_uint16(dev, "slot", index);
>   
> +    /*
> +     * User created root ports are QOM parented to one of
> +     * the peripheral containers but it's already at the right
> +     * parent bus. Change the QOM parent to be the same as the
> +     * parent bus it's already assigned to.
> +     */
> +    if (!pnv_parent_fixup(OBJECT(bus), BUS(bus), OBJECT(dev),
> +                          index, errp)) {
> +        return;
> +    }
> +
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);