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

[Frederic Barrat]

On 11/08/2022 18:39, 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.
>
> While we're at it, change pnv_phb_attach_root_port() to receive a PCIBus
> instead of a PCIHostState to make it clear that the function does not
> make use of the PHB.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb.c | 35 +++++++++++++++--------------------
>   1 file changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 17d9960aa1..65ed1f9eb4 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -51,27 +51,11 @@ static void pnv_parent_bus_fixup(DeviceState *parent, 
> DeviceState *child,
>       }
>   }
>   
> -/*
> - * 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)
> +static void pnv_phb_attach_root_port(PCIBus *bus)
>   {
>       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);
> +    pci_realize_and_unref(root, bus, &error_fatal);
>   }
>   
>   /*
> @@ -171,7 +155,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    pnv_phb_attach_root_port(pci);
> +    pnv_phb_attach_root_port(pci->bus);
>   }
>   
>   static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
> @@ -240,12 +224,18 @@ static void pnv_phb_root_port_realize(DeviceState *dev, 
> Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PnvPHBRootPort *phb_rp = PNV_PHB_ROOT_PORT(dev);
> -    PCIBus *bus = PCI_BUS(qdev_get_parent_bus(dev));
> +    BusState *qbus = qdev_get_parent_bus(dev);
> +    PCIBus *bus = PCI_BUS(qbus);
>       PCIDevice *pci = PCI_DEVICE(dev);
>       uint16_t device_id = 0;
>       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);
>   
> @@ -253,6 +243,11 @@ 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);
>   
> +    pnv_parent_qom_fixup(OBJECT(bus), OBJECT(dev), index);
> +    if (!qdev_set_parent_bus(dev, qbus, &error_fatal)) {


That line looks surprising at first, because we got qbus from 
qdev_get_parent_bus() just above, so it looks like a noop. I talked to 
Daniel about it: the device<->bus relationship is correct when entering 
the function but the call to pnv_parent_qom_fixup() interferes with the 
bus relationship, so it needs to be re-established.
Short of a better suggestion, it probably need a comment.

  Fred



> +        return;
> +    }
> +
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);