Message-ID: <CAErSpo7Z9v1QPQZLpfNAUPiq3w4ZfmzS1PStJW+SgJ_DmjjFvg@mail.gmail.com>
Date:	Tue, 18 Jun 2013 20:30:17 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Alex Williamson <alex.williamson@...hat.com>
Cc:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Don Dutile <ddutile@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] pci: Fix flaw in pci_acs_enabled()

On Tue, Jun 18, 2013 at 4:47 PM, Alex Williamson
<alex.williamson@...hat.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@...hat.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.

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."

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists