[PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Roland Dreier]

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(+)

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Alex Williamson]

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 },

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Roland Dreier]

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.

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Alex Williamson]

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Roland Dreier]

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Alex Williamson]

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Roland Dreier]

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Alex Williamson]

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

RE: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Tantilov, Emil S]

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Bjorn Helgaas]

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
> 

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Alex Williamson]

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Roland Dreier]

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

RE: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[David Laight]

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

Re: [PATCH] PCI: Update ACS quirk for more Intel 10G NICs

[Alex Williamson]

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