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

From: Bjorn Helgaas
Date: Tue Jun 18 2013 - 22:30:45 EST


On Tue, Jun 18, 2013 at 4:47 PM, Alex Williamson
<alex.williamson@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/