Re: [PATCH for-7.2 06/10] ppc/pnv: enable user created pnv-phb for powernv8

[Daniel Henrique Barboza]

On 8/5/22 07:50, Frederic Barrat wrote:
> > diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> > index 077f391d59..953c384bf6 100644
> > --- a/hw/pci-host/pnv_phb.c
> > +++ b/hw/pci-host/pnv_phb.c
> > @@ -17,6 +17,7 @@
> >   #include "hw/ppc/pnv.h"
> >   #include "hw/qdev-properties.h"
> >   #include "qom/object.h"
> > +#include "sysemu/sysemu.h"
> >   /*
> > @@ -171,6 +172,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
> >           pnv_phb4_bus_init(dev, PNV_PHB4(phb->backend));
> >       }
> > +    if (phb->version == 3 && !defaults_enabled()) {
> > +        return;
> > +    }
> > +
> >       pnv_phb_attach_root_port(pci);
> >   }
>
>
> So we skip the call to pnv_phb_attach_root_port() for the user created PHBs.
>
> I seem to remember that when the user creates a PHB on the command line, they 
> also need to create the root port and we can't just create one by default with 
> the PHB. It seems heavy when just using the qemu CLI but that was a requirement 
> from libvirt. Did I get that right?

libvirt requires all pci devices to be declared in the XML. If we create the
root ports automatically by just adding PHBs I would need to reflect that
in the domain XML by either automatically add root ports with each PHB, which
is easier said than done, or tell the user to do so.

libvirt also allows for custom naming of the buses, e.g the PHB can be named
'phb-root0' and its root port 'my-rootport.1'. There's no way we can name
root ports buses by adding them automatically with each PHB like we do
for default PHBs.


> In any case, I can see a problem. pnv_phb_attach_root_port() creates the root port 
> object, so if the above is correct, we want to skip that. But it also creates the 
> PHB->root port relationship in QOM. That we don't do anywhere for the 
> user-created case.
>
> Calling qemu with "-nodefaults -device pnv-phb -device pnv-phb-root-port", here 
> is what I see in QOM, which shows the wrong parenting of the root port:
>    /peripheral-anon (container)
>      /device[1] (pnv-phb-root-port)
>        /bus master container[0] (memory-region)
>        /bus master[0] (memory-region)
>        /pci_bridge_io[0] (memory-region)
>        /pci_bridge_io[1] (memory-region)
>        /pci_bridge_mem[0] (memory-region)
>        /pci_bridge_pci[0] (memory-region)
>        /pci_bridge_pref_mem[0] (memory-region)
>        /pci_bridge_vga_io_hi[0] (memory-region)
>        /pci_bridge_vga_io_lo[0] (memory-region)
>        /pci_bridge_vga_mem[0] (memory-region)
>        /pcie.0 (PCIE)
>
>
> With the full series, same problem on P8/P9/P10

I forgot to mention it in the cover letter. This is intended. The reason
is that we can't access the PHB from the root-port due to QOM best
practices, i.e. we can't do qdev_get_parent_bus()->parent to parent
the root port with the PHB. The reason why we're able to do that for
default devices is because we have access to the PHB regardless of QOM
when creating the root port.


If we want QOM consistency between user created devices and default
devices, one alternative is to make the root-port a child of the
root bus. That can be achievable in both cases.


Thanks,


Daniel


>    Fred