Re: [PATCH 01/17] pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port

[Daniel Henrique Barboza]

On 1/3/22 05:24, Cédric Le Goater wrote:
> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> > When creating a pnv_phb3_root_port using the command line, the first
> > root port is created successfully, but the second fails with the
> > following error:
> >
> > qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3:
> > Can't add chassis slot, error -16
> >
> > This error comes from the realize() function of its parent type,
> > rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails
> > with -EBUSY if there's an existing PCIESlot that has the same
> > chassis/slot value, regardless of being in a different bus.
> >
> > One way to prevent this error is simply set chassis and slot values in
> > the command line. However, since phb3 root buses only supports a single
> > root port, we can just get an unique chassis/slot value by checking
> > which root bus the pnv_phb3_root_port is going to be attached, get the
> > equivalent phb3 device and use its chip-id and index values, which are
> > guaranteed to be unique.
>
> I guess parent_realize() will fail if we add 2 root port devices under
> the same phb ?

Yes. It will fail with an error similar to the one that happens with phb4 root
ports:


qemu-system-ppc64: -device pnv-phb4-root-port,bus=pnv-phb4-root-bus.0,slot=8: 
Bus 'pnv-phb4-root-bus.0' is full

This happens because we're setting max_dev = 1 in the root bus class, in both 
cases:


static void pnv_phb4_root_bus_class_init(ObjectClass *klass, void *data)
{
    BusClass *k = BUS_CLASS(klass);

    /*
     * PHB4 has only a single root complex. Enforce the limit on the
     * parent bus
     */
    k->max_dev = 1;
}


I also did a quick experiment with both. I set their root bus class max_dev to 
2, then changed
the code to allow adding a second root port in the PHB. The machine boots, 
which is good, but the
OS doesn't see the extra root port in the output of 'lspci'. Same result for 
both.


I can't see if this has to do with skiboot lacking support for it or something 
that we need
to do in QEMU. For PHB4s, seeing how we've tied together the stack->phb->root 
port relationship
throughout the code I can't say I'm surprised. Even if skiboot starts to allow 
multiple pci devices
attached directly in the root complex we'll have to do some rework in the QEMU 
side to allow it.


Thanks,


Daniel

> Thanks,
>
> C.
>
>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> > index 4e2d680d44..130d392b3e 100644
> > --- a/hw/pci-host/pnv_phb3.c
> > +++ b/hw/pci-host/pnv_phb3.c
> > @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = {
> >   static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
> >   {
> >       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
> > +    PCIDevice *pci = PCI_DEVICE(dev);
> > +    PCIBus *bus = pci_get_bus(pci);
> > +    PnvPHB3 *phb = NULL;
> >       Error *local_err = NULL;
> > +    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> > +                                          TYPE_PNV_PHB3);
> > +
> > +    if (!phb) {
> > +        error_setg(errp,
> > +"pnv_phb3_root_port devices must be connected to pnv-phb3 buses");
> > +        return;
> > +    }
> > +
> > +    /* Set unique chassis/slot values for the root port */
> > +    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
> > +    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
> > +
> >       rpc->parent_realize(dev, &local_err);
> >       if (local_err) {
> >           error_propagate(errp, local_err);