| Message ID | 20170720214101.7449-1-roland@kernel.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 20 Jul 2017 14:41:01 -0700 Roland Dreier <roland@kernel.org> wrote: > From: Roland Dreier <roland@purestorage.com> > > Add one more variant of the 82599 plus the device IDs for X540 and X550 > variants. Intel has confirmed that none of these devices does peer-to-peer > between functions. The X540 and X550 have added ACS capabilities in their > PCI config space, but the ACS control register is hard-wired to 0 for both > devices, so we still need the quirk for IOMMU grouping to allow assignment > of individual SR-IOV functions. Most of the ACS capabilities are worded as "Must be implemented by devices that implement ..." Shouldn't a hard-wired ACS capability sufficiently describe that, or is there something wrong with how they're hard wired? Thanks, Alex > > Signed-off-by: Roland Dreier <roland@purestorage.com> > --- > drivers/pci/quirks.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 6967c6b4cf6b..b939db671326 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4335,12 +4335,33 @@ static const struct pci_dev_acs_enabled { > { PCI_VENDOR_ID_INTEL, 0x1507, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1514, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x151C, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x1528, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1529, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x154A, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x152A, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x154D, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x154F, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1551, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1558, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x1560, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x1563, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AA, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AC, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AD, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AE, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15B0, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C2, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C3, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C4, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C6, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C7, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C8, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15CE, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15E4, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15E5, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15D1, pci_quirk_mf_endpoint_acs }, > /* 82580 */ > { PCI_VENDOR_ID_INTEL, 0x1509, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x150E, pci_quirk_mf_endpoint_acs },
>-----Original Message----- >From: Roland Dreier [mailto:roland@purestorage.com] On Behalf Of Roland >Dreier >Sent: Thursday, July 20, 2017 2:41 PM >To: Bjorn Helgaas <bhelgaas@google.com> >Cc: linux-pci@vger.kernel.org; netdev@vger.kernel.org; Tantilov, Emil S ><emil.s.tantilov@intel.com> >Subject: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs > >From: Roland Dreier <roland@purestorage.com> > >Add one more variant of the 82599 plus the device IDs for X540 and X550 >variants. Intel has confirmed that none of these devices does peer-to-peer >between functions. The X540 and X550 have added ACS capabilities in their >PCI config space, but the ACS control register is hard-wired to 0 for both >devices, so we still need the quirk for IOMMU grouping to allow assignment >of individual SR-IOV functions. > >Signed-off-by: Roland Dreier <roland@purestorage.com> >--- > drivers/pci/quirks.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) Acked-by: Emil Tantilov <emil.s.tantilov@intel.com> Thanks, Emil
On Thu, Jul 20, 2017 at 3:15 PM, Alex Williamson <alex.williamson@redhat.com> wrote: > Most of the ACS capabilities are worded as "Must be implemented by > devices that implement ..." Shouldn't a hard-wired ACS capability > sufficiently describe that, or is there something wrong with how > they're hard wired? Interesting question. I just looked at what the PCI spec says about the various bits for ACS functions in multi-function devices. Many of the functions are "must not be implemented." Of the ones that are "must be implemented if..." the key is that the if is for devices that support peer-to-peer traffic. I think the Intel NICs are compliant with the spec - they don't support any ACS mechanisms for controlling peer-to-peer traffic, but they also don't implement peer-to-peer traffic. This means that the PCI standard way of knowing that it is safe to assign individual functions does not prove it is safe - but with device-specific knowledge we do know it is safe. Hence a quirk to give that device-specific information to the kernel. - R.
On Thu, 20 Jul 2017 15:53:04 -0700 Roland Dreier <roland.dreier@gmail.com> wrote: > On Thu, Jul 20, 2017 at 3:15 PM, Alex Williamson > <alex.williamson@redhat.com> wrote: > > > Most of the ACS capabilities are worded as "Must be implemented by > > devices that implement ..." Shouldn't a hard-wired ACS capability > > sufficiently describe that, or is there something wrong with how > > they're hard wired? > > Interesting question. I just looked at what the PCI spec says about > the various bits for ACS functions in multi-function devices. Many of > the functions are "must not be implemented." Of the ones that are > "must be implemented if..." the key is that the if is for devices that > support peer-to-peer traffic. > > I think the Intel NICs are compliant with the spec - they don't > support any ACS mechanisms for controlling peer-to-peer traffic, but > they also don't implement peer-to-peer traffic. This means that the > PCI standard way of knowing that it is safe to assign individual > functions does not prove it is safe - but with device-specific > knowledge we do know it is safe. Hence a quirk to give that > device-specific information to the kernel. I think that the device implementing ACS and not implementing certain features that are "must be implemented if..." is a positive indication that the device does not have that behavior and therefore the ability to control that behavior is unnecessary. pci_acs_flags_enabled() is coded with this intention: static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) { int pos; u16 cap, ctrl; pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS); if (!pos) return false; /* * Except for egress control, capabilities are either required * or only required if controllable. Features missing from the * capability field can therefore be assumed as hard-wired enabled. */ pci_read_config_word(pdev, pos + PCI_ACS_CAP, &cap); acs_flags &= (cap | PCI_ACS_EC); <Remove any requested ACS flags which are not implemented, except EC> pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl); return (ctrl & acs_flags) == acs_flags; <Compare what remains> } For example, from the spec (rev 3.1a 6.12.1.2): ACS P2P Request Redirect: must be implemented by Functions that support peer-to-peer traffic with other Functions. <Therefore if this capability within ACS is not implemented, p2p is not supported.> When ACS P2P Request Redirect is enabled in a multi-Function device, peer-to-peer Requests (between Functions of the device) must be redirected Upstream towards the RC. <If p2p is not supported, there's no need to control this, the behavior is the same is if p2p were supported and this control bit is enabled> At least that's my interpretation and how I've indicated to folks at Intel how things should work for a device that does not support p2p. Of course this all hinges on having an ACS capability. Without an ACS capability we must assume any sort of p2p is possible unless we have a quirk to tell us otherwise. With ACS, the absence of capabilities is a positive indication that those forms of p2p are not possible (depending on the spec wording for that capability). If the code doesn't behave that way, let's fix it. Thanks, Alex
> I think that the device implementing ACS and not implementing certain > features that are "must be implemented if..." is a positive indication > that the device does not have that behavior and therefore the ability > to control that behavior is unnecessary. pci_acs_flags_enabled() is > coded with this intention: > > static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) > { > int pos; > u16 cap, ctrl; > > pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS); > if (!pos) > return false; > > /* > * Except for egress control, capabilities are either required > * or only required if controllable. Features missing from the > * capability field can therefore be assumed as hard-wired enabled. > */ > pci_read_config_word(pdev, pos + PCI_ACS_CAP, &cap); > acs_flags &= (cap | PCI_ACS_EC); > > <Remove any requested ACS flags which are not implemented, except EC> > > pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl); > return (ctrl & acs_flags) == acs_flags; > > <Compare what remains> I'm not sure it makes sense to look for the EC bit in the control register if the capability bit says it is not supported. In fact I think it would violate the PCI spec to set the bit in the control register if it is zero in the capability register - the spec says: Default value of this bit is 0b. Must be hardwired to 0b if the ACS P2P Egress Control functionality is not implemented. I don't understand the reasoning behind forcing the EC bit in commit 83db7e0bdb70. I think the only reason this works is that nowhere in the kernel uses this function to check PCI_ACS_EC. > At least that's my interpretation and how I've indicated to folks at > Intel how things should work for a device that does not support p2p. > Of course this all hinges on having an ACS capability. Without an ACS > capability we must assume any sort of p2p is possible unless we have a > quirk to tell us otherwise. With ACS, the absence of capabilities is a > positive indication that those forms of p2p are not possible (depending > on the spec wording for that capability). If the code doesn't behave > that way, let's fix it. Thanks, Thanks - I'm looking more closely at what goes wrong with X550, since from reading the code it looks like it should work, but people have observed that it doesn't on our system. However the issue might be elsewhere. However I'll send a patch removing that "| PCI_ACS_EC" from the check if you agree - maybe I'm misunderstanding the logic but I don't see how it could work if it ever became relevant. - R.
On Mon, 24 Jul 2017 10:18:56 -0700 Roland Dreier <roland.dreier@gmail.com> wrote: > > I think that the device implementing ACS and not implementing certain > > features that are "must be implemented if..." is a positive indication > > that the device does not have that behavior and therefore the ability > > to control that behavior is unnecessary. pci_acs_flags_enabled() is > > coded with this intention: > > > > static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) > > { > > int pos; > > u16 cap, ctrl; > > > > pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS); > > if (!pos) > > return false; > > > > /* > > * Except for egress control, capabilities are either required > > * or only required if controllable. Features missing from the > > * capability field can therefore be assumed as hard-wired enabled. > > */ > > pci_read_config_word(pdev, pos + PCI_ACS_CAP, &cap); > > acs_flags &= (cap | PCI_ACS_EC); > > > > <Remove any requested ACS flags which are not implemented, except EC> > > > > pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl); > > return (ctrl & acs_flags) == acs_flags; > > > > <Compare what remains> > > I'm not sure it makes sense to look for the EC bit in the control register if > the capability bit says it is not supported. In fact I think it would > violate the PCI spec to set the bit in the control register if it is zero in the > capability register - the spec says: > > Default value of this bit is 0b. Must be hardwired to 0b if the ACS P2P > Egress Control functionality is not implemented. > > I don't understand the reasoning behind forcing the EC bit in commit > 83db7e0bdb70. > > I think the only reason this works is that nowhere in the kernel uses > this function to check PCI_ACS_EC. Is there a misunderstanding of the code flow here? We're never setting EC. In the first code block we're just masking out requested capabilities where unimplemented capabilities is the same as implemented + enabled. We're not adding EC to the request, we're just not removing it based on the implemented capabilities because we don't interpret it the same way. Thus if someone wants to test a device for EC, it really needs to implement EC, we cannot assume it based on lack of support for EC in the ACS capability. As you point out, nobody really cares about EC yet though. > > At least that's my interpretation and how I've indicated to folks at > > Intel how things should work for a device that does not support p2p. > > Of course this all hinges on having an ACS capability. Without an ACS > > capability we must assume any sort of p2p is possible unless we have a > > quirk to tell us otherwise. With ACS, the absence of capabilities is a > > positive indication that those forms of p2p are not possible (depending > > on the spec wording for that capability). If the code doesn't behave > > that way, let's fix it. Thanks, > > Thanks - I'm looking more closely at what goes wrong with X550, since > from reading the code it looks like it should work, but people have observed > that it doesn't on our system. However the issue might be elsewhere. > > However I'll send a patch removing that "| PCI_ACS_EC" from the check if > you agree - maybe I'm misunderstanding the logic but I don't see how it > could work if it ever became relevant. I don't yet see anything wrong with the EC handling, but please explain further if I'm just being dense. Thanks, Alex
> Is there a misunderstanding of the code flow here? We're never setting > EC. In the first code block we're just masking out requested > capabilities where unimplemented capabilities is the same as > implemented + enabled. We're not adding EC to the request, we're > just not removing it based on the implemented capabilities because we > don't interpret it the same way. Thus if someone wants to test a > device for EC, it really needs to implement EC, we cannot assume it > based on lack of support for EC in the ACS capability. As you point > out, nobody really cares about EC yet though. I guess I find the semantics confusing. For every other bit, pci_acs_enabled() returns true if the bit is either enabled or not implemented. For EC, it returns false if the bit is not implemented. It's not clear to me what the use case for checking for PCI_ACS_EC enabled would be. Seems like checking for EC in the capabilities register would be more useful. - R.
On Mon, 24 Jul 2017 12:31:39 -0700 Roland Dreier <roland.dreier@gmail.com> wrote: > > Is there a misunderstanding of the code flow here? We're never setting > > EC. In the first code block we're just masking out requested > > capabilities where unimplemented capabilities is the same as > > implemented + enabled. We're not adding EC to the request, we're > > just not removing it based on the implemented capabilities because we > > don't interpret it the same way. Thus if someone wants to test a > > device for EC, it really needs to implement EC, we cannot assume it > > based on lack of support for EC in the ACS capability. As you point > > out, nobody really cares about EC yet though. > > I guess I find the semantics confusing. For every other bit, > pci_acs_enabled() returns true if the bit is either enabled or not > implemented. For EC, it returns false if the bit is not implemented. EC is a bit more complicated than the other bits, it has both an enable bit and a control vector and I'd need to stare at the spec for a while to understand it better and likely decide that it needs a separate interface from the rest of the capabilities within ACS. Therefore we take the conservative approach of requiring the device to legitimately support it if anyone asks for it. > It's not clear to me what the use case for checking for PCI_ACS_EC > enabled would be. Seems like checking for EC in the capabilities > register would be more useful. Some sort of interface for manipulating the control vector would be necessary to fully support it and maybe the interface today just doesn't make much sense for it. Thanks, Alex
On Thu, Jul 20, 2017 at 02:41:01PM -0700, Roland Dreier wrote: > From: Roland Dreier <roland@purestorage.com> > > Add one more variant of the 82599 plus the device IDs for X540 and X550 > variants. Intel has confirmed that none of these devices does peer-to-peer > between functions. The X540 and X550 have added ACS capabilities in their > PCI config space, but the ACS control register is hard-wired to 0 for both > devices, so we still need the quirk for IOMMU grouping to allow assignment > of individual SR-IOV functions. > > Signed-off-by: Roland Dreier <roland@purestorage.com> I haven't seen a real conclusion to the discussion yet, so I'm waiting on that and hopefully an ack from Alex. Can you please repost with that, since I'm dropping it from patchwork for now? > --- > drivers/pci/quirks.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 6967c6b4cf6b..b939db671326 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4335,12 +4335,33 @@ static const struct pci_dev_acs_enabled { > { PCI_VENDOR_ID_INTEL, 0x1507, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1514, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x151C, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x1528, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1529, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x154A, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x152A, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x154D, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x154F, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1551, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x1558, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x1560, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x1563, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AA, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AC, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AD, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AE, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15B0, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C2, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C3, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C4, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C6, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C7, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15C8, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15CE, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15E4, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15E5, pci_quirk_mf_endpoint_acs }, > + { PCI_VENDOR_ID_INTEL, 0x15D1, pci_quirk_mf_endpoint_acs }, > /* 82580 */ > { PCI_VENDOR_ID_INTEL, 0x1509, pci_quirk_mf_endpoint_acs }, > { PCI_VENDOR_ID_INTEL, 0x150E, pci_quirk_mf_endpoint_acs }, > -- > 2.11.0 >
On Thu, 3 Aug 2017 16:49:11 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Thu, Jul 20, 2017 at 02:41:01PM -0700, Roland Dreier wrote: > > From: Roland Dreier <roland@purestorage.com> > > > > Add one more variant of the 82599 plus the device IDs for X540 and X550 > > variants. Intel has confirmed that none of these devices does peer-to-peer > > between functions. The X540 and X550 have added ACS capabilities in their > > PCI config space, but the ACS control register is hard-wired to 0 for both > > devices, so we still need the quirk for IOMMU grouping to allow assignment > > of individual SR-IOV functions. > > > > Signed-off-by: Roland Dreier <roland@purestorage.com> > > I haven't seen a real conclusion to the discussion yet, so I'm waiting on > that and hopefully an ack from Alex. Can you please repost with that, > since I'm dropping it from patchwork for now? I think the conclusion is that a hard-wired ACS capability is a positive indication of isolation for a multifunction device, the code is intended to support this and appears to do so, and Roland was going to investigate the sightings that inspired this patch in more detail. Dropping for now is appropriate. Thanks, Alex > > --- > > drivers/pci/quirks.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index 6967c6b4cf6b..b939db671326 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -4335,12 +4335,33 @@ static const struct pci_dev_acs_enabled { > > { PCI_VENDOR_ID_INTEL, 0x1507, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x1514, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x151C, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x1528, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x1529, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x154A, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x152A, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x154D, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x154F, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x1551, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x1558, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x1560, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x1563, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15AA, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15AC, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15AD, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15AE, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15B0, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15C2, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15C3, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15C4, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15C6, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15C7, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15C8, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15CE, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15E4, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15E5, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x15D1, pci_quirk_mf_endpoint_acs }, > > /* 82580 */ > > { PCI_VENDOR_ID_INTEL, 0x1509, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x150E, pci_quirk_mf_endpoint_acs }, > > -- > > 2.11.0 > >
From: Bjorn Helgaas > Sent: 03 August 2017 22:49 > On Thu, Jul 20, 2017 at 02:41:01PM -0700, Roland Dreier wrote: > > From: Roland Dreier <roland@purestorage.com> > > > > Add one more variant of the 82599 plus the device IDs for X540 and X550 > > variants. Intel has confirmed that none of these devices does peer-to-peer > > between functions. The X540 and X550 have added ACS capabilities in their > > PCI config space, but the ACS control register is hard-wired to 0 for both > > devices, so we still need the quirk for IOMMU grouping to allow assignment > > of individual SR-IOV functions. ... > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -4335,12 +4335,33 @@ static const struct pci_dev_acs_enabled { > > { PCI_VENDOR_ID_INTEL, 0x1507, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x1514, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x151C, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x1528, pci_quirk_mf_endpoint_acs }, > > { PCI_VENDOR_ID_INTEL, 0x1529, pci_quirk_mf_endpoint_acs }, > > + { PCI_VENDOR_ID_INTEL, 0x154A, pci_quirk_mf_endpoint_acs }, ... That list is looking a bit long. Is it possible to run-time determine that the ACS control register is hard wired to zero, and apply the quirk to all such devices. Or even changing to a (device & mask) == value test?? David
On Fri, 4 Aug 2017 13:28:32 +0000 David Laight <David.Laight@ACULAB.COM> wrote: > From: Bjorn Helgaas > > Sent: 03 August 2017 22:49 > > On Thu, Jul 20, 2017 at 02:41:01PM -0700, Roland Dreier wrote: > > > From: Roland Dreier <roland@purestorage.com> > > > > > > Add one more variant of the 82599 plus the device IDs for X540 and X550 > > > variants. Intel has confirmed that none of these devices does peer-to-peer > > > between functions. The X540 and X550 have added ACS capabilities in their > > > PCI config space, but the ACS control register is hard-wired to 0 for both > > > devices, so we still need the quirk for IOMMU grouping to allow assignment > > > of individual SR-IOV functions. > ... > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -4335,12 +4335,33 @@ static const struct pci_dev_acs_enabled { > > > { PCI_VENDOR_ID_INTEL, 0x1507, pci_quirk_mf_endpoint_acs }, > > > { PCI_VENDOR_ID_INTEL, 0x1514, pci_quirk_mf_endpoint_acs }, > > > { PCI_VENDOR_ID_INTEL, 0x151C, pci_quirk_mf_endpoint_acs }, > > > + { PCI_VENDOR_ID_INTEL, 0x1528, pci_quirk_mf_endpoint_acs }, > > > { PCI_VENDOR_ID_INTEL, 0x1529, pci_quirk_mf_endpoint_acs }, > > > + { PCI_VENDOR_ID_INTEL, 0x154A, pci_quirk_mf_endpoint_acs }, > ... > > That list is looking a bit long. > Is it possible to run-time determine that the ACS control register is hard wired > to zero, and apply the quirk to all such devices. > Or even changing to a (device & mask) == value test?? In fact, hard-wired ACS doesn't need a quirk at all, please see the other thread of the discussion. Thanks, Alex
> I think the conclusion is that a hard-wired ACS capability is a > positive indication of isolation for a multifunction device, the code > is intended to support this and appears to do so, and Roland was going > to investigate the sightings that inspired this patch in more detail. > Dropping for now is appropriate. Thanks, That's right. I confirmed that the issue we found was due to another PCI quirk. It may make sense to add more 82599 variants to the table, but the X540 and X550 work without a quirk. Sorry for the noise. - R.
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 6967c6b4cf6b..b939db671326 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -4335,12 +4335,33 @@ static const struct pci_dev_acs_enabled { { PCI_VENDOR_ID_INTEL, 0x1507, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x1514, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x151C, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x1528, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x1529, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x154A, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x152A, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x154D, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x154F, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x1551, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x1558, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x1560, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x1563, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15AA, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15AC, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15AD, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15AE, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15B0, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15AB, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15C2, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15C3, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15C4, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15C6, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15C7, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15C8, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15CE, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15E4, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15E5, pci_quirk_mf_endpoint_acs }, + { PCI_VENDOR_ID_INTEL, 0x15D1, pci_quirk_mf_endpoint_acs }, /* 82580 */ { PCI_VENDOR_ID_INTEL, 0x1509, pci_quirk_mf_endpoint_acs }, { PCI_VENDOR_ID_INTEL, 0x150E, pci_quirk_mf_endpoint_acs },