[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Michael S. Tsirkin]

On Fri, Jan 21, 2011 at 07:44:16PM +0900, Isaku Yamahata wrote:
> On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> > > make pci_find_device() ARI aware.
> > > 
> > > Signed-off-by: Isaku Yamahata <address@hidden>
> > > ---
> > >  hw/pci.c |    7 +++++++
> > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 8d0e3df..851f350 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > >  
> > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int 
> > > function)
> > >  {
> > > +    PCIDevice *d;
> > >      bus = pci_find_bus(bus, bus_num);
> > >  
> > >      if (!bus)
> > >          return NULL;
> > >  
> > > +    d = bus->parent_dev;
> > > +    if (d && pci_is_express(d) &&
> > > +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> > > +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> > > +        return NULL;
> > > +    }
> > >      return bus->devices[PCI_DEVFN(slot, function)];
> > >  }
> > 
> > I'd like to split this express-specific code out in some way.
> > Further, the downstream port always has a single slot.
> > Maybe it shouldn't be a bus at all, but this needs some thought.
> 
> Yes, it needs consideration.
> 
> 
> > How about we put flag in PCIBus that says that a single
> > slot is supported? Downstream ports would just set it.
> 
> So such a flag must be set/clear by something like pcie_cap_ari_write_config()
> depending on ARI bit at runtime.

Well, to figure it out, could you please describe what is the situation
your patch tries to fix? I would generally would like a reason for the
change to be given in commit logs, please try to avoid just restating
what the patch does.

Are you trying to create a device with > 8 functions?
If that is the case I suspect this is not the best way
to do this at all.

> pcie device can have 256 functions instead of 8.

Only if it's an ARI device. And note that if you have a device with
256 functions and disable ARI in the port, it appears as
multiple devices.

> Maybe we'd like to emulate how p2p bridge transfers pci transaction
> to child pci bus somehow.

To support > 8 functions per device, some refactoring would be needed:
you can not figure out slot and function from the root bus,
it depends on ARI along the way. So APIs that pass in
decoded slot/function do not make sense anymore,
you must pass in devfn all the way.

But since everyone decodes and encodes them in the same way,
many things will work even without decoding.



-- 
MST