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

[Daniel Henrique Barboza]

On 8/19/22 04:19, Cédric Le Goater wrote:
> On 8/19/22 00:45, 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 | 38 +++++++++++++++++++-------------------
> >   1 file changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> > index 4ea33fb6ba..38ec8571b7 100644
> > --- a/hw/pci-host/pnv_phb.c
> > +++ b/hw/pci-host/pnv_phb.c
> > @@ -62,27 +62,11 @@ 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)
> > +static void pnv_phb_attach_root_port(PCIBus *bus)
>
> Do we still need this pnv_phb_attach_root_port routine ?

Good point. We can just add these 2 lines in the 'defaults_enabled' case
in pnv_phb_realize().

I'll re-send. Thanks,


Daniel



> >   {
> >       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);
> >   }
> >   /*
> > @@ -184,7 +168,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,
> > @@ -259,6 +243,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 +255,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);