PCI: Update ACS quirk for more Intel 10G NICs
Subsystems (3)
PATCH 1/1 PCI: Update ACS quirk for more Intel 10G NICs
From: Roland Dreier <[email protected]>
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.
--- drivers/pci/quirks.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
drivers/pci/quirks.c +21 -0
@@ -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 },
Signature · 1 line
2.11.0
Quoted 8 lines · On Thu, 20 Jul 2017 14:41:01 -0700 Roland Dreier <[email protected]> wrote:
From: Roland Dreier <[email protected]>
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
Quoted 44 lines
Signed-off-by Roland Dreier <[email protected]>--- drivers/pci/quirks.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)drivers/pci/quirks.c +21
-0@@ -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 },
Quoted 4 lines · Alex Williamson
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.
Quoted 14 lines · On Thu, 20 Jul 2017 15:53:04 -0700 Roland Dreier <[email protected]>…
Quoted 4 lines · Alex Williamson
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.
Quoted 7 lines
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
Quoted 23 lines
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);
Quoted 6 lines
<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.
Quoted 8 lines
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.
Quoted 35 lines · On Mon, 24 Jul 2017 10:18:56 -0700 Roland Dreier <[email protected]>…
Quoted 23 lines
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);Quoted 6 lines
<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:
Quoted 8 lines
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.
Quoted 9 lines
Quoted 8 lines
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,
Quoted 7 lines
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
Quoted 9 lines
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.
Quoted 10 lines · On Mon, 24 Jul 2017 12:31:39 -0700 Roland Dreier <[email protected]>…
Quoted 9 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
It's not clear to me what the use case for checking for
PCI_ACS_ECenabled 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
Quoted 18 lines
-----Original Message-----
From: Roland Dreier [mailto:[email protected]] On Behalf Of Roland Dreier
Sent: Thursday, July 20, 2017 2:41 PM
To: Bjorn Helgaas <[email protected]>
Cc: [email protected]; [email protected]; Tantilov, Emil S <[email protected]>
Subject: [PATCH] PCI: Update ACS quirk for more Intel 10G NICsFrom: Roland Dreier <[email protected]>
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.
Quoted 4 lines
Signed-off-by Roland Dreier <[email protected]>---
drivers/pci/quirks.c| 21 +++++++++++++++++++++1 file changed, 21 insertions(+)
Thanks,
Emil
Quoted 10 lines · Roland Dreier
From: Roland Dreier <[email protected]>
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 <[email protected]>
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?
Quoted 45 lines
--- drivers/pci/quirks.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)drivers/pci/quirks.c +21
-0@@ -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 },Signature · 1 line
2.11.0
Quoted 10 lines · On Thu, 3 Aug 2017 16:49:11 -0500 Bjorn Helgaas <[email protected]> wrote:
Quoted 9 lines · Roland Dreier
From: Roland Dreier <[email protected]>
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.
Quoted 5 lines
Quoted 1 line
Signed-off-by Roland Dreier <[email protected]>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
Quoted 45 lines
Quoted 45 lines
--- drivers/pci/quirks.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)drivers/pci/quirks.c +21
-0@@ -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 },Signature · 1 line
2.11.0
Quoted 5 lines
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.
From: Bjorn Helgaas
Quoted 6 lines
Sent: 03 August 2017 22:49
Quoted 4 lines · Roland Dreier
From: Roland Dreier <[email protected]>
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
Quoted 1 line
Quoted 1 line
between functions. The X540 and X550 have added ACS capabilities in th=
eir
Quoted 1 line
Quoted 1 line
PCI config space, but the ACS control register is hard-wired to 0 for b=
oth
Quoted 1 line
Quoted 1 line
devices, so we still need the quirk for IOMMU grouping to allow assignm=
ent
Quoted 1 line
Quoted 1 line
of individual SR-IOV functions.
...
Quoted 9 lines
drivers/pci/quirks.c static const struct pci_dev_acs_enabled { · line 4340
Quoted 9 lines
drivers/pci/quirks.c +2
-0@@ -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) =3D=3D value test??
David
Quoted 23 lines · On Fri, 4 Aug 2017 13:28:32 +0000 David Laight <[email protected]> wrote:
From: Bjorn Helgaas
Quoted 10 lines
Sent: 03 August 2017 22:49
Quoted 8 lines · Roland Dreier
From: Roland Dreier <[email protected]>
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.
...
Quoted 9 lines
Quoted 9 lines
drivers/pci/quirks.c +2
-0@@ -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 },...
Quoted 4 lines
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