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

[Daniel Henrique Barboza]

On 8/12/22 13:13, Frederic Barrat wrote:
> 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.

The more detailed version: when adding a child property
using object_property_add_child(), object_property_try_add_child() is called.
This function calls internally a object_property_try_add() function with lots
of parameters, including a release callback named 
'object_finalize_child_property'.

This release callback is implemented like this:

static void object_finalize_child_property(Object *obj, const char *name,
                                           void *opaque)
{
    Object *child = opaque;

    if (child->class->unparent) {
        (child->class->unparent)(child);
    }
    child->parent = NULL;
    object_unref(child);
}

If you're adding a device as a child, which is our case here,
child->class->unparent is device_unparent(). Note that device_unparent()
removes the device from its parent_bus:

static void device_unparent(Object *obj)
{
    DeviceState *dev = DEVICE(obj);
    BusState *bus;

    if (dev->realized) {
        qdev_unrealize(dev);
    }
    while (dev->num_child_bus) {
        bus = QLIST_FIRST(&dev->child_bus);
        object_unparent(OBJECT(bus));
    }
    if (dev->parent_bus) {
        bus_remove_child(dev->parent_bus, dev);
        object_unref(OBJECT(dev->parent_bus));
        dev->parent_bus = NULL;
    }
}

So, back in our qom_fixup() helper, we're doing an object_unparent(). This
function calls object_property_del_child(), which in turn calls the
property release callback prop->release if it's defined. It is defined,
and in our case it's the object_finalize_child_property() from above that will
end up calling device_unparent(), which will remove the device from the bus.

This is why I'm having to do a qdev_set_parent_bus() to assign the root port
to the same parent bus it had before. We want to fixup it's QOM parent without
changing its parent bus.

Also, note that we're doing the same thing for the user created pnv-phbs in
pnv_phb_user_device_init():

    pnv_parent_qom_fixup(parent, OBJECT(phb), phb->phb_id);
    pnv_parent_bus_fixup(DEVICE(chip), DEVICE(phb));

The difference here is that the QOM parent is not the same as the parent bus,
so it's less apparent what we're doing here.

All that said, since we're always calling these 2 fixup helpers together, I 
think
it's a good idea to create a single helper that handles both the QOM parenting 
and
the parent bus. I can also add some comments about what I've said here to 
explain
why we need to set the parent bus after doing an object_unparent().

> Short of a better suggestion, it probably need a comment.

I don't think we have wiggle room since we're dealing with deep QOM and qdev 
internals
in this case. I'm all ears for suggestions though.


Thanks,


Daniel

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