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

[Michael S. Tsirkin]

On Mon, Jan 24, 2011 at 08:39:57PM +0900, Isaku Yamahata wrote:
> On Sun, Jan 23, 2011 at 05:57:53PM +0200, Michael S. Tsirkin wrote:
> > On Sat, Jan 22, 2011 at 01:39:57AM +0900, Isaku Yamahata wrote:
> > > On Fri, Jan 21, 2011 at 04:29:41PM +0200, Michael S. Tsirkin wrote:
> > > > 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.
> > > 
> > > It seems that I should have added the comment to refer the spec.
> > > I'd like to implement ARI enable bit correctly.
> > > 
> > > Downstream port(and root port) doesn't forward pci transaction for
> > > function > 7 by default for compatibility, 
> > > Only when ARI forwarding enable bit of downstream/root port is set,
> > > the virtual p2p bridge forwards pci transaction for
> > > function > 7 (i.e. slot > 0).
> > 
> > Oh, I see, yes, function > 7 gets described as slot >0.
> > I think this is what I missed.
> > Hmm, it'd pretty confusing. Should we fix this,
> > pass devfn all over?
> 
> Sounds to make sense.
> Although it seems only pci_find_device() will be affected at a glance, 
> I'll look into it more closely.
> 
> 
> > I now understand what the code does, it just needs
> > a good comment that explains that at the moment
> > slot encodes the high bits of the device id.
> > 
> > Also, let's replace pcie_cap_is_ari_enabled
> > with an inline that does all the relevant logic
> > E.g.
> > 
> > /* With PCI Express Endpoints, there's a single device behind
> >    each downstream port bus, and bits 3:7 of the function number get
> >    encoded in the slot number (the Express spec calls it the Device
> >    Number). This allows > 8 functions, but
> >    these extended functions are only accessible when the
> >    Alternative routing-ID Interpretation (ARI)
> >    capability is enabled in the downstream port. With that capability
> >    disabled the port enforces the Device Number field being 0.*/
> > static inline
> > bool pcie_check_slot(PCIDevice *dev)
> > {
> >     return !pci_is_express(dev) || !slot ||
> >             pcie_cap_get_type(dev) != PCI_EXP_TYPE_DOWNSTREAM ||
> >             (pci_get_long(dev->config + dev->exp.exp_cap + PCI_EXP_DEVCTL2) 
> > &
> >             PCI_EXP_DEVCTL2_ARI);
> > }
> 
> Okay.
> 
> 
> > >   6.13 Alternative routing-ID Interpretation(ARI)
> > >   7.8.15 Device capabilites 2 register
> > >   ARI forwarding supproted
> > >   7.8.16 Device control 2 register
> > >   ARI forwarding Enable
> > >     5 ARI Forwarding Enable When set, the Downstream Port
> > >     disables its traditional Device Number field being 0 enforcement
> > >     when turning a Type 1 Configuration Request into a Type 0
> > >     Configuration Request, permitting access to Extended Functions
> > >     in an ARI Device immediately below the Port. See Section 6.13.
> > >     Default value of this bit is 0b. Must be hardwired to 0b if the ARI
> > >     Forwarding Supported bit is 0b.
> > > Oh, the patch should check root port in addition to downstream port.
> > 
> > It should? Where does it say so?
> 
> I wasn't clear enough.
> pcie_check_slot() above should include something like
>   !((type == downstream) ||
>     (type == root && the below is endpoint))

So:
(pcie_cap_get_type(dev) != PCI_EXP_TYPE_DOWNSTREAM &&
pcie_cap_get_type(dev) != PCI_EXP_ROOT) 

I think the endpoint check is not needed: the rule really
applies to any device besides the donwstream port itself.
and that is never below root, is it?

> 
> > > > 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.
> > > 
> > > I think there are only two issues to address.
> > > Configuration space and hot plug.
> > 
> > info pci, maybe others (firmware path?)
> 
> Oh yes, maybe.
> 
> 
> > > As you already described it, ARI is defined such that APIs can stay same,
> > > just interpret slot bits as part of function number.
> > > This patch addresses configuration space access.
> > > 
> > > Multifunction hot plug hasn't been addressed even for conventional pci 
> > > case.
> > 
> > But at least we learned to deny hotplug attempts for functions > 0...
> > 
> > > Some kind of refactoring would be necessary for it, I think.
> > 
> > It's probably just a matter of figuring out what works with acpi.
> > Express has a native hotplug which likely works ok already ...
> > 
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata