Re: [PATCH v2 1/2] pci: Fix flaw in pci_acs_enabled()

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2013-06-18 at 20:30 -0600, Bjorn Helgaas wrote:
> On Tue, Jun 18, 2013 at 4:47 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Tue, 2013-06-18 at 16:10 -0600, Bjorn Helgaas wrote:
> >> On Tue, Jun 18, 2013 at 12:38 PM, Alex Williamson
> >> <alex.williamson@redhat.com> wrote:
> >> > On Tue, 2013-06-18 at 11:09 -0600, Bjorn Helgaas wrote:
> >> >> On Fri, Jun 07, 2013 at 10:34:41AM -0600, Alex Williamson wrote:
> > ...
> >> >> >   * pci_acs_enabled - test ACS against required flags for a given device
> >> >> >   * @pdev: device to test
> >> >> > @@ -2364,8 +2377,7 @@ void pci_enable_acs(struct pci_dev *dev)
> >> >> >   */
> >> >> >  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags)
> >> >>
> >> >> I know you didn't change the *name* of this function, but I think it
> >> >> would be easier to follow if you did change the name to something more
> >> >> descriptive, e.g., something to do with the actual property you're
> >> >> interested in, which has to do with routing peer-to-peer DMA.
> >> >>
> >> >> That property makes sense even for the excluded devices, while the
> >> >> idea of an ACS capability that doesn't even exist is implicitly
> >> >> enabled, really doesn't.
> >> >
> >> > I think we also don't want to put the complexity at the caller for
> >> > understanding what capabilities are applicable to a given device.  It's
> >> > also convenient to use the set of ACS flags.  Given that, the current
> >> > naming came about.  It's a little awkward, but it's easy to use.
> >> > Suggestions for a better name?
> >>
> >> 100% agreed the caller shouldn't have to worry about different device
> >> types.  I was thinking something like "pci_enforces_peer_isolation()"
> >> or "pci_peer_dma_routed_upstream()".  Or maybe it should be
> >> "pci_dev_...()".
> >
> > Ok, I'll play with those.  I'm worried that there are nuances to each
> > flag bit that don't all fit under such a broad description.
> 
> It's true that they might be overly broad.  On the other hand, the set
> of flags we look for is always the same: PCI_ACS_SV | PCI_ACS_RR |
> PCI_ACS_CR | PCI_ACS_UF, so what's the point in making a completely
> general-purpose interface?  I'm not sure it's even worth passing the
> flags around if the code would be clearer without that.
> 
> > Do you want
> > to gate this series on a rename of an existing function?
> 
> You put me in a bit of a tight spot :)  My #1 concern is correctness
> and maintainability.  Naming things so they're consistent with other
> code and make sense to other readers is a huge part of that.
> Unfortunately I don't have time to do any work myself, and my only
> tool is to apply patches or not.  But no, I don't want to gate a
> simple bug fix on other "cleanup" rework.
> 
> > ...
> >> >> Maybe something like (pidgin C):
> >> >>
> >> >>     if (PCI_EXP_TYPE_DOWNSTREAM || PCI_EXP_TYPE_ROOT_PORT)
> >> >>       return pci_acs_flags_enabled(pdev, acs_flags);
> >> >>
> >> >>     if (!pdev->multifunction)
> >> >>       return true;
> >> >>
> >> >>     acs_flags &= (PCI_ACS_RR | PCI_ACS_CR | ...);
> >> >>     return pci_acs_flags_enabled(pdev, acs_flags);
> >> >
> >> > ...  Note that
> >> > the above simplification incorrectly handles multifunction bridges or EC
> >> > devices.
> >>
> >> Hmm...  What *is* the correct behavior for a bridge?  You return
> >> "true," i.e., you're saying that a PCIe-to-PCI bridge will always
> >> route peer-to-peer transactions from PCI devices upstream to its PCIe
> >> link.  But that seems wrong: a PCI DMA transaction can target a peer
> >> on the same PCI bus, and it's not even possible for the bridge to
> >> validate the transaction or forward it upstream.
> >>
> >> I suspect the "ACS is never applicable to a PCI Express to PCI Bridge
> >> Function" statement in 6.12.1 just means "it's impossible for ACS to
> >> isolate the devices below the bridge from each other, so it would be
> >> misleading to implement the capability."
> >
> > Note that we never consider ACS to be enabled for a conventional PCI
> > device.  I suppose we could have cases where it's the only device on a
> > bus, but for the most part, it's not worth the trouble (it may be the
> > only device now, then a hotplug occurs).  So really saying the bridge
> > does or doesn't support ACS doesn't matter to the devices behind it.
> 
> > What does matter is the fan-out of that isolation group of the
> > conventional devices beyond the bridge.  If the spec is indicating that
> > a bridge cannot do peer-to-peer with other devices  then all of the
> > conventional devices behind it are in an isolation domain so long as the
> > path between the bridge and the RC supports ACS.  If the bridge can do
> > peer-to-peer and it is a multifunction device, then the isolation domain
> > grows to include the other functions and subordinates of the other
> > functions.  I took the assumption that a bridge probably needs to
> > forward transaction upstream.  Do you have an alternate opinion?
> 
> I think you're talking about a multi-function device with several
> PCIe-to-PCI bridges (e.g., Option A of Figure 1-4, p. 29, of the PCIe
> bridge spec 1.0), and the question is whether the bridge can forward a
> transaction between bus X and bus Y without forwarding it upstream.

Right, or the second function may not be a bridge, it could be anything
that could accept a p2p DMA.

> I don't see any mention of forwarding transactions between functions
> of a multi-ported bridge, so the conservative assumption would be that
> a bridge is allowed to do that.  I would expect a conventional
> multi-port PCI-PCI bridge to forward between functions, because in
> conventional PCI there would be no reason to forward it upstream, so
> I'd think PCIe-PCI bridges, at least those designed pre-ACS, would be
> similar.  And I don't think the ACS capability is expressive enough to
> say "peer-to-peer transactions between functions must be forwarded
> upstream, but peer-to-peer transactions below a single function may
> not be."

The best I can find is the statement that the bridge must forward
transactions from the secondary interface to the primary interface.  We
could infer that that means upstream, but we can't rule out some switch
logic in a multifunction package that could do a re-route from that
statement.  However then why would the PCIe spec forbid a PCIe-to-PCI
bridge from implementing ACS?

ACS doesn't need to be expressive enough for what you're describing.  We
know that there's no protection against p2p on conventional PCI.  The
PCIe bridge spec even indicates that transactions within the bridge
apertures should not be forwarded to the primary interface (which should
really be a big red flag that we shouldn't even attempt to support
assignment of such devices).  The question is whether a transaction
going out the primary interface is necessarily headed upstream or if it
can go directly to a peer device.  In that case, I can't figure out why
ACS is precluded from PCIe-to-PCI bridges.  If anything this is an
example of why we need a user override, then we could it to the more
conservative value pending further information and let the user change
it.  Thanks,

Alex