[PATCH] PCI: quirks: update cavium ACS quirk implementation

[PATCH] PCI: quirks: update cavium ACS quirk implementation

[Vadim Lomovtsev]

This commit makes PIC ACS quirk applicable only to Cavium PCIE devices
and Cavium PCIE Root Ports which has limited PCI capabilities in terms
of no ACS support. Match function checks for ACS support and exact ACS
bits set at the device capabilities.
Also by this commit we get rid off device ID range values checkings.

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
---
 drivers/pci/quirks.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d3361..11ca951 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,6 +4211,29 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
+#define CAVIUM_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | \
+			  PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT)
+
+static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
+{
+	int pos = 0;
+	u32 caps = 0;
+
+	/* Filter out a few obvious non-matches first */
+	if (!pci_is_pcie(dev) || pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+		return false;
+
+	/* Get the ACS caps offset */
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	if (pos) {
+		pci_read_config_dword(dev, pos + PCI_ACS_CAP, &caps);
+		/* If we have no such bits set, then we will need a quirk */
+		return ((caps & CAVIUM_ACS_FLAGS) != CAVIUM_ACS_FLAGS);
+	}
+
+	return true;
+}
+
 static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 {
 	/*
@@ -4218,13 +4241,10 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 	 * with other functions, allowing masking out these bits as if they
 	 * were unimplemented in the ACS capability.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
-
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
-	return acs_flags ? 0 : 1;
+	return acs_flags & ~(CAVIUM_ACS_FLAGS) ? 0 : 1;
 }
 
 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
-- 
2.9.5

Re: [PATCH] PCI: quirks: update cavium ACS quirk implementation

[Alex Williamson]

On Tue, 12 Sep 2017 04:55:16 -0700
Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:

> This commit makes PIC ACS quirk applicable only to Cavium PCIE devices
> and Cavium PCIE Root Ports which has limited PCI capabilities in terms
> of no ACS support. Match function checks for ACS support and exact ACS
> bits set at the device capabilities.
> Also by this commit we get rid off device ID range values checkings.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
>  drivers/pci/quirks.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..11ca951 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,6 +4211,29 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> +#define CAVIUM_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | \
> +			  PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> +{
> +	int pos = 0;
> +	u32 caps = 0;
> +
> +	/* Filter out a few obvious non-matches first */
> +	if (!pci_is_pcie(dev) || pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> +		return false;
> +
> +	/* Get the ACS caps offset */
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> +	if (pos) {
> +		pci_read_config_dword(dev, pos + PCI_ACS_CAP, &caps);
> +		/* If we have no such bits set, then we will need a quirk */
> +		return ((caps & CAVIUM_ACS_FLAGS) != CAVIUM_ACS_FLAGS);
> +	}
> +
> +	return true;
> +}
> +
>  static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  {
>  	/*
> @@ -4218,13 +4241,10 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * with other functions, allowing masking out these bits as if they
>  	 * were unimplemented in the ACS capability.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> -
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)

No please.  As I read it, this is assuming that any Cavium PCIe root
port supports the equivalent isolation flags.  Do you have a crystal
ball to know about all the future PCIe root ports that Cavium is going
to ship?  Quirk the devices you can verify support the equivalent
isolation capabilities and solve this problem automatically for future
devices by implementing ACS in hardware.  No free pass for all future
hardware, especially not one that overrides the hardware potentially
implementing ACS in the future and ignoring it if it's not sufficient.
We're actually trying to be diligent to test for isolation and this
entirely ignores that.

Also, as we've been through with APM, how do you justify each of these
ACS flags?  Claiming that a device does not support peer-to-peer does
not automatically justify Source Validation.  What feature of your
hardware allows you to claim that?  How does a root port that does not
support P2P imply anything about Transaction Blocking?  What about
Direct Translated P2P?  If the device doesn't support P2P, doesn't that
mean it shouldn't claim DT?  Like the attempted APM quirk, I think this
original quirk here has just taken and misapplied the mask we use for
multifunction devices where downstream ports have much different
requirements for ACS.  Thanks,

Alex

Re: [PATCH] PCI: quirks: update cavium ACS quirk implementation

[Vadim Lomovtsev]

On Tue, Sep 12, 2017 at 10:15:45AM -0600, Alex Williamson wrote:
> On Tue, 12 Sep 2017 04:55:16 -0700
> Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> 
> > This commit makes PIC ACS quirk applicable only to Cavium PCIE devices
> > and Cavium PCIE Root Ports which has limited PCI capabilities in terms
> > of no ACS support. Match function checks for ACS support and exact ACS
> > bits set at the device capabilities.
> > Also by this commit we get rid off device ID range values checkings.
> > 
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > ---
> >  drivers/pci/quirks.c | 30 +++++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..11ca951 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,6 +4211,29 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > +#define CAVIUM_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | \
> > +			  PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> > +{
> > +	int pos = 0;
> > +	u32 caps = 0;
> > +
> > +	/* Filter out a few obvious non-matches first */
> > +	if (!pci_is_pcie(dev) || pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> > +		return false;
> > +
> > +	/* Get the ACS caps offset */
> > +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> > +	if (pos) {
> > +		pci_read_config_dword(dev, pos + PCI_ACS_CAP, &caps);
> > +		/* If we have no such bits set, then we will need a quirk */
> > +		return ((caps & CAVIUM_ACS_FLAGS) != CAVIUM_ACS_FLAGS);
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> >  {
> >  	/*
> > @@ -4218,13 +4241,10 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> >  	 * with other functions, allowing masking out these bits as if they
> >  	 * were unimplemented in the ACS capability.
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > -
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> 
> No please.  As I read it, this is assuming that any Cavium PCIe root
> port supports the equivalent isolation flags.  Do you have a crystal
> ball to know about all the future PCIe root ports that Cavium is going
> to ship?

Well, yes, my bad then.
Would the check for exact device id (or some range) of pcie device/root
port be more suitable here (as it is implemented for other vendors) ?

> Quirk the devices you can verify support the equivalent
> isolation capabilities and solve this problem automatically for future
> devices by implementing ACS in hardware.  No free pass for all future
> hardware, especially not one that overrides the hardware potentially
> implementing ACS in the future and ignoring it if it's not sufficient.
> We're actually trying to be diligent to test for isolation and this
> entirely ignores that.
> 
> Also, as we've been through with APM, how do you justify each of these
> ACS flags?  Claiming that a device does not support peer-to-peer does
> not automatically justify Source Validation.  What feature of your
> hardware allows you to claim that?  How does a root port that does not
> support P2P imply anything about Transaction Blocking?  What about
> Direct Translated P2P?  If the device doesn't support P2P, doesn't that
> mean it shouldn't claim DT?  Like the attempted APM quirk, I think this
> original quirk here has just taken and misapplied the mask we use for
> multifunction devices where downstream ports have much different
> requirements for ACS.  Thanks,

My understanding that CN81xx/83xx/88xx pcie bridges/root ports has no ACS support.
And the original mask was constructed in that way erroneously copied I guess.

Would the resetting of RR/CR/UF/SV bits be more correct here ?

> 
> Alex

WBR,
Vadim

Re: [PATCH] PCI: quirks: update cavium ACS quirk implementation

[Vadim Lomovtsev]

To my previous email, please ignore this for now:

> Would the resetting of RR/CR/UF/SV bits be more correct here ?

WBR,
Vadim

[PATCH v3] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

This commit makes Cavium PCI ACS quirk applicable only to Cavium
ThunderX (CN81/83/88XX) PCIE Root Ports which has limited PCI capabilities
in terms of no ACS support advertisement. However, the RTL internally
implements similar protection as if ACS had completion redirection,
blocking and validation features enabled.

Following settings are enforced by hardware on 8xxx although it does not have ACS:

$PCCBR_XXX_ACS_CAP_CTL (as if it present)
---------------------------------------------------------------------------------------------------------
 Bit     Field Field   Reset      Typical    Field
 Pos     Name  Type    Value      Value      Description
---------------------------------------------------------------------------------------------------------
 <31:23> --    RAZ     --         --         Reserved.
 <22>    DTE   R/W     0          --         ACS direct translated P2P enable. Value ignored by hardware.
 <21>    ECE   RO      0          0          ACS P2P egress control enable. Always clear.
 <20>    UFE   R/W     0          --         ACS upstream forwarding enable. Value ignored by hardware.
 <19>    CRE   R/W     0          --         ACS completion redirect enable. Value ignored by hardware.
 <18>    RRE   R/W     0          --         ACS P2P request redirect enable. Value ignored by hardware.
 <17>    TBE   R/W     0          --         ACS transaction blocking enable. Value ignored by hardware.
 <16>    SVE   R/W     0          --         ACS source validation enable. Value ignored by hardware.
 <15:8>  ECVS  RO      0x0        0x0        Egress control vector size. Always zero.
 <7>     --    RAZ     --         --         Reserved.
 <6>     DT    RO      1          1          ACS direct translated P2P. Always set.
 <5>     EC    RO      0          0          ACS P2P egress control. Always clear.
 <4>     UF    RO      1          1          ACS upstream forwarding. Always set.
 <3>     CR    RO      1          1          ACS completion redirect. Always set.
 <2>     RR    RO      1          1          ACS P2P request redirect. Always set.
 <1>     TB    RO      1          1          ACS transaction blocking. Always set.
 <0>     SV    RO      1          1          ACS source validation. Always set.
---------------------------------------------------------------------------------------------------------

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
---
 drivers/pci/quirks.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d3361..f1786a5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,20 +4211,28 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
-static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+/*
+ * The CN8XXX on-chip devices' PCCBR's do not advertise
+ * ACS, although the RTL internally implements similar protections as to
+ * if ACS had completion redirection, blocking and validation features
+ * enabled.
+ */
+#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | \
+				 PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT)
+
+static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
 {
-	/*
-	 * Cavium devices matching this quirk do not perform peer-to-peer
-	 * with other functions, allowing masking out these bits as if they
-	 * were unimplemented in the ACS capability.
-	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+	return (pci_is_pcie(dev) &&
+		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+		((dev->device & 0xf800) == 0xa000));
+}
 
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+{
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
-	return acs_flags ? 0 : 1;
+	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
 }
 
 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
-- 
2.9.5

Re: [PATCH v3] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Lomovtsev, Vadim]

Hi guys,

Sorry for wasting your time by getting back to this.

Please correct me if I'm wrong,...

So far it might fall into same discussion as happened here:
https://lkml.org/lkml/2017/8/1/559

And to provided ACS mask I've already got a comment before for initial patch that it is not correct because it violates the spec.

> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | \
> +                                PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT)

So from that point the proper mask is expected to be as following:

> (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV)

which should be enough to provide proper device isolation.

and based on that it appears to got self-nack here.

Are there any other comments on this ?
Going to prepare and test v4 of patch.

Sorry again for inconvenience.

WBR,
Vadim
________________________________________
From: Lomovtsev, Vadim
Sent: Friday, September 15, 2017 3:57:13 PM
To: bhelgaas@google.com; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; alex.williamson@redhat.com
Cc: Snyder, Wilson; Daney, David; jcm@redhat.com; Lomovtsev, Vadim
Subject: [PATCH v3] PCI: quirks: update Cavium ThunderX ACS quirk implementation

This commit makes Cavium PCI ACS quirk applicable only to Cavium
ThunderX (CN81/83/88XX) PCIE Root Ports which has limited PCI capabilities
in terms of no ACS support advertisement. However, the RTL internally
implements similar protection as if ACS had completion redirection,
blocking and validation features enabled.

Following settings are enforced by hardware on 8xxx although it does not have ACS:

$PCCBR_XXX_ACS_CAP_CTL (as if it present)
---------------------------------------------------------------------------------------------------------
 Bit     Field Field   Reset      Typical    Field
 Pos     Name  Type    Value      Value      Description
---------------------------------------------------------------------------------------------------------
 <31:23> --    RAZ     --         --         Reserved.
 <22>    DTE   R/W     0          --         ACS direct translated P2P enable. Value ignored by hardware.
 <21>    ECE   RO      0          0          ACS P2P egress control enable. Always clear.
 <20>    UFE   R/W     0          --         ACS upstream forwarding enable. Value ignored by hardware.
 <19>    CRE   R/W     0          --         ACS completion redirect enable. Value ignored by hardware.
 <18>    RRE   R/W     0          --         ACS P2P request redirect enable. Value ignored by hardware.
 <17>    TBE   R/W     0          --         ACS transaction blocking enable. Value ignored by hardware.
 <16>    SVE   R/W     0          --         ACS source validation enable. Value ignored by hardware.
 <15:8>  ECVS  RO      0x0        0x0        Egress control vector size. Always zero.
 <7>     --    RAZ     --         --         Reserved.
 <6>     DT    RO      1          1          ACS direct translated P2P. Always set.
 <5>     EC    RO      0          0          ACS P2P egress control. Always clear.
 <4>     UF    RO      1          1          ACS upstream forwarding. Always set.
 <3>     CR    RO      1          1          ACS completion redirect. Always set.
 <2>     RR    RO      1          1          ACS P2P request redirect. Always set.
 <1>     TB    RO      1          1          ACS transaction blocking. Always set.
 <0>     SV    RO      1          1          ACS source validation. Always set.
---------------------------------------------------------------------------------------------------------

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
---
 drivers/pci/quirks.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d3361..f1786a5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,20 +4211,28 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }

-static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+/*
+ * The CN8XXX on-chip devices' PCCBR's do not advertise
+ * ACS, although the RTL internally implements similar protections as to
+ * if ACS had completion redirection, blocking and validation features
+ * enabled.
+ */
+#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | \
+                                PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT)
+
+static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
 {
-       /*
-        * Cavium devices matching this quirk do not perform peer-to-peer
-        * with other functions, allowing masking out these bits as if they
-        * were unimplemented in the ACS capability.
-        */
-       acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-                      PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+       return (pci_is_pcie(dev) &&
+               (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+               ((dev->device & 0xf800) == 0xa000));
+}

-       if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+{
+       if (!pci_quirk_cavium_acs_match(dev))
                return -ENOTTY;

-       return acs_flags ? 0 : 1;
+       return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
 }

 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
--
2.9.5

[PATCH v4] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

This commit makes Cavium PCI ACS quirk applicable only to Cavium
ThunderX (CN81/83/88XX) PCIE Root Ports which has limited PCI capabilities
in terms of no ACS support advertisement. However, the RTL internally
implements similar protection as if ACS had completion/request redirection,
upstream forwarding and validation features enabled.

Current quirk implementation doesn't take into account PCIERCs which
also needs to be quirked. So the pci device id check mask is updated
and check of device ID moved into separate function.

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
---
	v1	: put device check into separate function and extend it to all
		  Cavium PCIERC/PCCBR devices;
	v1 -> v2: update match function in order to filter only ThunderX devices by device
	      	  ids to properly filter CN8XXX devices, update subject & description with
		  ACS register info (rejected by maillist due to triple X in subject);
	v2 -> v3: update subject: remove CN8XXX from subject line, replace it with ThunderX;
	v3 -> v4: update ACS mask (remove TB and TD bits), update commit message (remove
	      	  ACS register printout);

 drivers/pci/quirks.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d3361..e6b904a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,20 +4211,26 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
-static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+/*
+ * Cavium devices matching this quirk do not perform peer-to-peer
+ * with other functions, allowing masking out these bits as if they
+ * were unimplemented in the ACS capability.
+ */
+#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
+
+static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
 {
-	/*
-	 * Cavium devices matching this quirk do not perform peer-to-peer
-	 * with other functions, allowing masking out these bits as if they
-	 * were unimplemented in the ACS capability.
-	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+	return (pci_is_pcie(dev) &&
+		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+		((dev->device & 0xf800) == 0xa000));
+}
 
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+{
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
-	return acs_flags ? 0 : 1;
+	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
 }
 
 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
-- 
2.9.5

Re: [PATCH v4] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

Hi guys,

Sorry for annoying topic ..
Could you please provide your comments/objections on this if any ?

WBR,
Vadim

On Mon, Sep 18, 2017 at 01:48:01AM -0700, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN81/83/88XX) PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
> 	v1	: put device check into separate function and extend it to all
> 		  Cavium PCIERC/PCCBR devices;
> 	v1 -> v2: update match function in order to filter only ThunderX devices by device
> 	      	  ids to properly filter CN8XXX devices, update subject & description with
> 		  ACS register info (rejected by maillist due to triple X in subject);
> 	v2 -> v3: update subject: remove CN8XXX from subject line, replace it with ThunderX;
> 	v3 -> v4: update ACS mask (remove TB and TD bits), update commit message (remove
> 	      	  ACS register printout);
> 
>  drivers/pci/quirks.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..e6b904a 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,26 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * Cavium devices matching this quirk do not perform peer-to-peer
> + * with other functions, allowing masking out these bits as if they
> + * were unimplemented in the ACS capability.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
> -	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> -	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> -- 
> 2.9.5
> 

Re: [PATCH v4] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Alex Williamson]

On Mon, 18 Sep 2017 01:48:01 -0700
Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:

> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN81/83/88XX) PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
> 	v1	: put device check into separate function and extend it to all
> 		  Cavium PCIERC/PCCBR devices;
> 	v1 -> v2: update match function in order to filter only ThunderX devices by device
> 	      	  ids to properly filter CN8XXX devices, update subject & description with
> 		  ACS register info (rejected by maillist due to triple X in subject);
> 	v2 -> v3: update subject: remove CN8XXX from subject line, replace it with ThunderX;
> 	v3 -> v4: update ACS mask (remove TB and TD bits), update commit message (remove
> 	      	  ACS register printout);
> 
>  drivers/pci/quirks.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..e6b904a 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,26 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * Cavium devices matching this quirk do not perform peer-to-peer
> + * with other functions, allowing masking out these bits as if they
> + * were unimplemented in the ACS capability.

nit, the description here still steals too much from the multifunction
quirk.  Multifunction devices can often support ACS with unimplemented
capabilities, which indicate that the device does not support the
behavior described by that capability bit.  However, downstream ports
are required to implement certain ACS capabilities if they implement
ACS at all.  So the code is actually asserting that the hardware
implements *and* enables equivalent ACS functionality for these flags.

> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
> -	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> -	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));

That's effectively 2k device IDs, 0xa000-0xa7ff that you and Cavium are
vouching for ACS equivalent isolation.  How many of these actually
exist?  The PCI IDs database gets really sparse after the first 64
entries.  Internally are these device IDs allocated to programs based on
the same ASICs or is this just a slightly more restricted crystal ball
(ie. wishful thinking)?  Thanks,

Alex

> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)

Re: [PATCH v4] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

On Wed, Sep 20, 2017 at 10:31:51AM -0600, Alex Williamson wrote:
> On Mon, 18 Sep 2017 01:48:01 -0700
> Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> 
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN81/83/88XX) PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> > 
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > ---
> > 	v1	: put device check into separate function and extend it to all
> > 		  Cavium PCIERC/PCCBR devices;
> > 	v1 -> v2: update match function in order to filter only ThunderX devices by device
> > 	      	  ids to properly filter CN8XXX devices, update subject & description with
> > 		  ACS register info (rejected by maillist due to triple X in subject);
> > 	v2 -> v3: update subject: remove CN8XXX from subject line, replace it with ThunderX;
> > 	v3 -> v4: update ACS mask (remove TB and TD bits), update commit message (remove
> > 	      	  ACS register printout);
> > 
> >  drivers/pci/quirks.c | 26 ++++++++++++++++----------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..e6b904a 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,26 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * Cavium devices matching this quirk do not perform peer-to-peer
> > + * with other functions, allowing masking out these bits as if they
> > + * were unimplemented in the ACS capability.
> 
> nit,

put this down for later use..

> the description here still steals too much from the multifunction
> quirk.

description was just moved (and  needs to be rewrited) since the orignal idea was
just to extend quirk for all ThunderX family which has that limitation.

Would the following be more suitable here:

"The Cavium downstream ports doesn't advertise their ACS capability registers.
However, the RTL internally implements similar protection as if ACS had completion redirection,
forwarding and validation features enabled." ?

> Multifunction devices can often support ACS with unimplemented
> capabilities, which indicate that the device does not support the
> behavior described by that capability bit. However, downstream ports
> are required to implement certain ACS capabilities if they implement
> ACS at all.  So the code is actually asserting that the hardware
> implements *and* enables equivalent ACS functionality for these flags.

Yes it is. The hardware doesn't advertise ACS caps which is desing limitation,
however it implements similar functionality to ACS provided flags which allows code to assert this.

> 
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> >  {
> > -	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > -	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> 
> That's effectively 2k device IDs, 0xa000-0xa7ff that you and Cavium are
> vouching for ACS equivalent isolation.  How many of these actually
> exist?  The PCI IDs database gets really sparse after the first 64
> entries.  Internally are these device IDs allocated to programs based on
> the same ASICs or is this just a slightly more restricted crystal ball
> (ie. wishful thinking)?  Thanks,

The latter; this represents 8 SoCs, the lower 8 bits of the DEVID are used
to indicate which subdevice is used within the SoC.

Vadim

> 
> Alex
> 
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> 

[PATCH v5] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

This commit makes Cavium PCI ACS quirk applicable only to Cavium
ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
in terms of no ACS support advertisement. However, the RTL internally
implements similar protection as if ACS had completion/request redirection,
upstream forwarding and validation features enabled.

Current quirk implementation doesn't take into account PCIERCs which
also needs to be quirked. So the pci device id check mask is updated
and check of device ID moved into separate function.

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
---
 drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d3361..0fd2e15 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
-static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+/*
+ * The Cavium downstream ports doesn't advertise their ACS capability registers.
+ * However, the RTL internally implements similar protection as if
+ * ACS had completion redirection, forwarding and validation features enabled.
+ * So by this flags we're asserting that the hardware implements and
+ * enables equivalent ACS functionality for these flags.
+ */
+#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
+
+static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
 {
 	/*
-	 * Cavium devices matching this quirk do not perform peer-to-peer
-	 * with other functions, allowing masking out these bits as if they
-	 * were unimplemented in the ACS capability.
+	 * Effectively selects all downstream ports for whole ThunderX 1 family
+	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID
+	 * are used to indicate which subdevice is used within the SoC.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+	return (pci_is_pcie(dev) &&
+		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+		((dev->device & 0xf800) == 0xa000));
+}
 
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+{
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
-	return acs_flags ? 0 : 1;
+	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
 }
 
 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
-- 
2.9.5

Re: [PATCH v5] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

Hi guys,

Could you please consider to review following patch?

For v5 changes:
- ACS mask comment has been updated.
- comment has been added for device id filtering mask at match
  function to provide explantion of CN8xxx devid scheme.

WBR,
Vadim

On Mon, Sep 25, 2017 at 06:08:40AM -0700, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..0fd2e15 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> -- 
> 2.9.5
> 

Re: [PATCH v5] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Alex Williamson]

On Mon, 25 Sep 2017 06:08:40 -0700
Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:

> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)


Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Thanks for making the updates


> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..0fd2e15 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)

Re: [PATCH v5] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

On Tue, Sep 26, 2017 at 09:43:43AM -0600, Alex Williamson wrote:
> On Mon, 25 Sep 2017 06:08:40 -0700
> Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> 
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> > 
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > ---
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Thanks for making the updates
>

Thank you for your patience and for your time, Alex.



Bjorn,

Would you mind to pick up this patch ?

WBR,
Vadim

> 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..0fd2e15 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> >  {
> >  	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> 

Re: [PATCH v5] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

On Tue, Sep 26, 2017 at 09:00:26AM -0700, Vadim Lomovtsev wrote:
> On Tue, Sep 26, 2017 at 09:43:43AM -0600, Alex Williamson wrote:
> > On Mon, 25 Sep 2017 06:08:40 -0700
> > Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> > 
> > > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > > in terms of no ACS support advertisement. However, the RTL internally
> > > implements similar protection as if ACS had completion/request redirection,
> > > upstream forwarding and validation features enabled.
> > > 
> > > Current quirk implementation doesn't take into account PCIERCs which
> > > also needs to be quirked. So the pci device id check mask is updated
> > > and check of device ID moved into separate function.
> > > 
> > > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > > ---
> > >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> > >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > 
> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > Thanks for making the updates
> >
> 
> Thank you for your patience and for your time, Alex.
> 
> 
> 
> Bjorn,
> 
> Would you mind to pick up this patch ?
> 
> WBR,
> Vadim
> 
> > 
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index a4d3361..0fd2e15 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> > >  #endif
> > >  }
> > >  
> > > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > > +/*
> > > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > > + * However, the RTL internally implements similar protection as if
> > > + * ACS had completion redirection, forwarding and validation features enabled.
> > > + * So by this flags we're asserting that the hardware implements and
> > > + * enables equivalent ACS functionality for these flags.
> > > + */
> > > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > > +
> > > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> > >  {
> > >  	/*
> > > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > > -	 * with other functions, allowing masking out these bits as if they
> > > -	 * were unimplemented in the ACS capability.
> > > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > > +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID

Bjorn,

Please ignore this since I found one more typo here: it should be 0xa000, sorry. Will post v6.
Sorry for wasting your time, guys.

WBR,
Vadim


> > > +	 * are used to indicate which subdevice is used within the SoC.
> > >  	 */
> > > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > > +	return (pci_is_pcie(dev) &&
> > > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > > +		((dev->device & 0xf800) == 0xa000));
> > > +}
> > >  
> > > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > > +{
> > > +	if (!pci_quirk_cavium_acs_match(dev))
> > >  		return -ENOTTY;
> > >  
> > > -	return acs_flags ? 0 : 1;
> > > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> > >  }
> > >  
> > >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> > 

[PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

This commit makes Cavium PCI ACS quirk applicable only to Cavium
ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
in terms of no ACS support advertisement. However, the RTL internally
implements similar protection as if ACS had completion/request redirection,
upstream forwarding and validation features enabled.

Current quirk implementation doesn't take into account PCIERCs which
also needs to be quirked. So the pci device id check mask is updated
and check of device ID moved into separate function.

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
---
	v5 -> v6: comment typo fix: change 0xa00 to 0xa000

 drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d3361..ed6c76d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
-static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+/*
+ * The Cavium downstream ports doesn't advertise their ACS capability registers.
+ * However, the RTL internally implements similar protection as if
+ * ACS had completion redirection, forwarding and validation features enabled.
+ * So by this flags we're asserting that the hardware implements and
+ * enables equivalent ACS functionality for these flags.
+ */
+#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
+
+static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
 {
 	/*
-	 * Cavium devices matching this quirk do not perform peer-to-peer
-	 * with other functions, allowing masking out these bits as if they
-	 * were unimplemented in the ACS capability.
+	 * Effectively selects all downstream ports for whole ThunderX 1 family
+	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
+	 * are used to indicate which subdevice is used within the SoC.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+	return (pci_is_pcie(dev) &&
+		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+		((dev->device & 0xf800) == 0xa000));
+}
 
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+{
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
-	return acs_flags ? 0 : 1;
+	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
 }
 
 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
-- 
2.4.11

Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

Hi guys,

I've found one typo (0xa00 instead of 0xa000 at code comment) and v6 has it fixed.
I bring my apologies for that, could you please review this patch once again.

WBR,
Vadim

On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..ed6c76d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> -- 
> 2.4.11
> 

Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Alex Williamson]

On Wed, 27 Sep 2017 11:20:39 -0700
Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:

> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..ed6c76d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)

Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

Hi Bjorn,

On Wed, Sep 27, 2017 at 02:18:54PM -0600, Alex Williamson wrote:
> On Wed, 27 Sep 2017 11:20:39 -0700
> Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> 
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> > 
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > ---
> > 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
>

If there is no any objections, could you please take this patch ?

WBR,
Vadim

>  
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..ed6c76d 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> >  {
> >  	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> 

Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

Hi Bjorn,

Would you please consider to take this patch ?
It is required to provide correct ACS mask for CN8xxx PCI root ports.

WBR,
Vadim

On Wed, Sep 27, 2017 at 02:18:54PM -0600, Alex Williamson wrote:
> On Wed, 27 Sep 2017 11:20:39 -0700
> Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> 
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> > 
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > ---
> > 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 
>  
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..ed6c76d 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> >  {
> >  	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> 

Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Robert Richter]

Bjorn,

On 27.09.17 11:20:39, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>

ping on this patch too for pci/host-thunder. Do you think we can mark
this stable for 4.11? Since there is a dependent patch not yet in
stable (b77d537d00, should be added to stable too) there might be
conflicts. But maybe this could be figured out once the patch is
considered for inclusion into stable.

Thanks,

-Robert

> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)

Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Bjorn Helgaas]

[+cc David, Manish]

Please use a subject line that tells more about what's going on.
"Update quirk" doesn't really convey any useful information.
Something like "Apply Cavium ThunderX ACS quirk only to Root Ports".

On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.

s/PCIE/PCIe/ above

s/PCIERCs/PCIe Root Ports/ (I assume, since usually a Root Complex
itself doesn't appear as a pci_dev)

> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..ed6c76d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)

You are changing the set of flags, which isn't mentioned in the changelog.
I think the best thing would be to have two patches: one that changes the
set of flags and a second that changes the set of affected devices.

This set of flags was used twice in the original patch, so a #define made
sense.  But now you only use it once, so there's no benefit in adding the
#define, and adding it makes the change in the set of flags harder to see.

> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)

No need to use "__inline__" here.  This isn't performance critical,
and the compiler will probably inline it anyway because there's only
one use.

>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.

Please wrap your comments so they fit in 80 columns.

>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));

I'm just a little confused by the 0xa000 mask you refer to in the
comment, because the mask in the code is 0xf800.

Previously the quirk applied to all Cavium device IDs in the range
0xa000-0xa0ff.  Now it applies to device IDs in the range
0xa000-0xa7ff, but only if they are PCIe Root Ports.  Right?

> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> -- 
> 2.4.11
> 

Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

[Vadim Lomovtsev]

On Mon, Oct 16, 2017 at 04:23:20PM -0500, Bjorn Helgaas wrote:
Hi Bjorn,

> [+cc David, Manish]
> 
> Please use a subject line that tells more about what's going on.
> "Update quirk" doesn't really convey any useful information.
> Something like "Apply Cavium ThunderX ACS quirk only to Root Ports".
> 
> On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote:
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> 
> s/PCIE/PCIe/ above
> 
> s/PCIERCs/PCIe Root Ports/ (I assume, since usually a Root Complex
> itself doesn't appear as a pci_dev)
> 
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > ---
> > 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> > 
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..ed6c76d 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> 
> You are changing the set of flags, which isn't mentioned in the changelog.
> I think the best thing would be to have two patches: one that changes the
> set of flags and a second that changes the set of affected devices.
> 
> This set of flags was used twice in the original patch, so a #define made
> sense.  But now you only use it once, so there's no benefit in adding the
> #define, and adding it makes the change in the set of flags harder to see.
> 
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> 
> No need to use "__inline__" here.  This isn't performance critical,
> and the compiler will probably inline it anyway because there's only
> one use.
>
> >  {
> >  	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> 
> Please wrap your comments so they fit in 80 columns.
> 
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> 
> I'm just a little confused by the 0xa000 mask you refer to in the
> comment, because the mask in the code is 0xf800.

Sorry for confusion, will correct it to 0xf800 for comment.

> 
> Previously the quirk applied to all Cavium device IDs in the range
> 0xa000-0xa0ff.  Now it applies to device IDs in the range
> 0xa000-0xa7ff, but only if they are PCIe Root Ports.  Right?

Correct. Since this quirk is necessary for Root Ports only.

Thanks for reply, I'll re-work this one accordingly to your comments and re-send v7.

WBR,
Vadim

> 
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> > -- 
> > 2.4.11
> > 

[PATCH v7 0/2] PCI: quirks: Cavium ThunderX ACS quirk update

[Vadim Lomovtsev]

From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>

version 7:
- update patch description accordingly to review comments;
- split for two patches: for ACS mask change and device id match change;
- remove macro #define of ACS flags, put it back to quirk function;
- remove '__inline_' from device id matching function;
- wrap code comments to fit into 80 columns;
- comment fix: change 0xa00 to 0xf800 for matching function description;

version 6:
- comment typo fix: change 0xa00 to 0xa000;

version 5:
- update code comments accordingly to review comments;

version 4:
- update ACS mask (remove TB and TD bits), update commit message (remove
  ACS register printout);

version 3:
- update subject: remove CN8XXX from subject line, replace it with ThunderX;

version 2:
- update match function in order to filter only ThunderX devices by device
  ids to properly filter CN8XXX devices, update subject & description with
  ACS register info (v2 was rejected by maillist due to triple X in subject);
  
Vadim Lomovtsev (2):
  PCI: quirks: Set Cavium ACS capability quirk flags to assert
    RR/CR/SV/UF.
  PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports

 drivers/pci/quirks.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

-- 
2.13.6

[PATCH v7 1/2] PCI: quirks: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF.

[Vadim Lomovtsev]

From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>

The Cavium ThunderX (CN8XXX) family PCIe Root Ports has limited PCI
capabilities in terms of no ACS support advertisement. However,
the RTL internally implements similar protection as if ACS had
completion/request redirection, upstream forwarding and validation
features enabled.

This commit changes Cavium ACS capabilities quirk flags accordingly.

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
 drivers/pci/quirks.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d33619a7bb..5e0e83304fda 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4214,12 +4214,13 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 {
 	/*
-	 * Cavium devices matching this quirk do not perform peer-to-peer
-	 * with other functions, allowing masking out these bits as if they
-	 * were unimplemented in the ACS capability.
+	 * The Cavium downstream ports doesn't advertise their ACS capability
+	 * registers. However, the RTL internally implements similar protection
+	 * as if ACS had completion redirection, forwarding and validation
+	 * features enabled. So by this flags we're asserting that the hardware
+	 * implements and enables equivalent ACS functionality for these flags.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
 
 	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
 		return -ENOTTY;
-- 
2.13.6

[PATCH v7 2/2] PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports

[Vadim Lomovtsev]

From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>

This commit changes the Cavium PCI ACS quirk appliance only to
Cavium CN8xxx PICe Root Ports.

Current quirk implementation doesn't take into account PCIe Root Ports
which needs to be quirked. So the pci device id check mask is updated
accordingly to CN8xxx device identification scheme and check of device
ID moved into separate function.

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
 drivers/pci/quirks.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 5e0e83304fda..cfefb2431348 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,6 +4211,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
+static bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
+{
+	/*
+	 * Effectively selects all downstream ports for whole ThunderX 1 
+	 * family by 0xf800 mask (which represents 8 SoCs), while the lower
+	 * bits of device ID are used to indicate which subdevice is used 
+	 * within the SoC.
+	 */
+	return (pci_is_pcie(dev) &&
+		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+		((dev->device & 0xf800) == 0xa000));
+}
+
 static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 {
 	/*
@@ -4222,7 +4235,7 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 	 */
 	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
 
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
 	return acs_flags ? 0 : 1;
-- 
2.13.6

Re: [PATCH v7 0/2] PCI: quirks: Cavium ThunderX ACS quirk update

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 05:47:37AM -0700, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> 
> version 7:
> - update patch description accordingly to review comments;
> - split for two patches: for ACS mask change and device id match change;
> - remove macro #define of ACS flags, put it back to quirk function;
> - remove '__inline_' from device id matching function;
> - wrap code comments to fit into 80 columns;
> - comment fix: change 0xa00 to 0xf800 for matching function description;
> 
> version 6:
> - comment typo fix: change 0xa00 to 0xa000;
> 
> version 5:
> - update code comments accordingly to review comments;
> 
> version 4:
> - update ACS mask (remove TB and TD bits), update commit message (remove
>   ACS register printout);
> 
> version 3:
> - update subject: remove CN8XXX from subject line, replace it with ThunderX;
> 
> version 2:
> - update match function in order to filter only ThunderX devices by device
>   ids to properly filter CN8XXX devices, update subject & description with
>   ACS register info (v2 was rejected by maillist due to triple X in subject);
>   
> Vadim Lomovtsev (2):
>   PCI: quirks: Set Cavium ACS capability quirk flags to assert
>     RR/CR/SV/UF.
>   PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports
> 
>  drivers/pci/quirks.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)

If I'm reading this correctly, the first patch is basically fixing a bug in
the original Cavium ACS quirk (b404bcfbf035 ("PCI: Add ACS quirk for all
Cavium devices")).  Right?

I put them on pci/virtualization for v4.15 as follows (patches unchanged,
changelogs wordsmithed).

You mentioned stable backports.  b404bcfbf035 ("PCI: Add ACS quirk for all
Cavium devices") appeared in v4.6.  b77d537d00d0 ("PCI: Apply Cavium ACS
quirk only to CN81xx/CN83xx/CN88xx devices") appeared in v4.12.  I assume
you would ideally want all this backported as far as v4.6?  We'd have to
figure out how to express that in stable tags.


commit 60f6d5f7ebc4bba83d73115f58b805f2f2618667
Author: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
Date:   Tue Oct 17 05:47:38 2017 -0700

    PCI: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF
    
    The Cavium ThunderX (CN8XXX) family of PCIe Root Ports does not advertise
    an ACS capability.  However, the RTL internally implements similar
    protection as if ACS had Request Redirection, Completion Redirection,
    Source Validation, and Upstream Forwarding features enabled.
    
    Change Cavium ACS capabilities quirk flags accordingly.
    
    Fixes: b404bcfbf035 ("PCI: Add ACS quirk for all Cavium devices")
    Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d33619a7bb..c23650cfd5a1 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4214,12 +4214,14 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 {
 	/*
-	 * Cavium devices matching this quirk do not perform peer-to-peer
-	 * with other functions, allowing masking out these bits as if they
-	 * were unimplemented in the ACS capability.
+	 * Cavium root ports don't advertise an ACS capability.  However,
+	 * the RTL internally implements similar protection as if ACS had
+	 * Request Redirection, Completion Redirection, Source Validation,
+	 * and Upstream Forwarding features enabled.  Assert that the
+	 * hardware implements and enables equivalent ACS functionality for
+	 * these flags.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
 
 	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
 		return -ENOTTY;

commit 35da518fcad24f7be515976238a4667b5ccd1711
Author: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
Date:   Tue Oct 17 05:47:39 2017 -0700

    PCI: Apply Cavium ThunderX ACS quirk to more Root Ports
    
    Extend the Cavium ThunderX ACS quirk to cover more device IDs and restrict
    it to only Root Ports.
    
    Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
    [bhelgaas: changelog]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c23650cfd5a1..0e22cce05742 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,6 +4211,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
+static bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
+{
+	/*
+	 * Effectively selects all downstream ports for whole ThunderX 1
+	 * family by 0xf800 mask (which represents 8 SoCs), while the lower
+	 * bits of device ID are used to indicate which subdevice is used
+	 * within the SoC.
+	 */
+	return (pci_is_pcie(dev) &&
+		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+		((dev->device & 0xf800) == 0xa000));
+}
+
 static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 {
 	/*
@@ -4223,7 +4236,7 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 	 */
 	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
 
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
 	return acs_flags ? 0 : 1;

Re: [PATCH v7 0/2] PCI: quirks: Cavium ThunderX ACS quirk update

[Vadim Lomovtsev]

On Thu, Oct 19, 2017 at 06:26:45AM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 17, 2017 at 05:47:37AM -0700, Vadim Lomovtsev wrote:
> > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > 
> > version 7:
> > - update patch description accordingly to review comments;
> > - split for two patches: for ACS mask change and device id match change;
> > - remove macro #define of ACS flags, put it back to quirk function;
> > - remove '__inline_' from device id matching function;
> > - wrap code comments to fit into 80 columns;
> > - comment fix: change 0xa00 to 0xf800 for matching function description;
> > 
> > version 6:
> > - comment typo fix: change 0xa00 to 0xa000;
> > 
> > version 5:
> > - update code comments accordingly to review comments;
> > 
> > version 4:
> > - update ACS mask (remove TB and TD bits), update commit message (remove
> >   ACS register printout);
> > 
> > version 3:
> > - update subject: remove CN8XXX from subject line, replace it with ThunderX;
> > 
> > version 2:
> > - update match function in order to filter only ThunderX devices by device
> >   ids to properly filter CN8XXX devices, update subject & description with
> >   ACS register info (v2 was rejected by maillist due to triple X in subject);
> >   
> > Vadim Lomovtsev (2):
> >   PCI: quirks: Set Cavium ACS capability quirk flags to assert
> >     RR/CR/SV/UF.
> >   PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports
> > 
> >  drivers/pci/quirks.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> If I'm reading this correctly, the first patch is basically fixing a bug in
> the original Cavium ACS quirk (b404bcfbf035 ("PCI: Add ACS quirk for all
> Cavium devices")).  Right?

Yes, first patch fixes improper ACS flags in the patch you mentioned.

The second one fixes devid check from another patch
(b77d537d00d0 PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices).

> 
> I put them on pci/virtualization for v4.15 as follows (patches unchanged,
> changelogs wordsmithed).
>

Good, thank you.

> You mentioned stable backports.  b404bcfbf035 ("PCI: Add ACS quirk for all
> Cavium devices") appeared in v4.6.  b77d537d00d0 ("PCI: Apply Cavium ACS
> quirk only to CN81xx/CN83xx/CN88xx devices") appeared in v4.12.  I assume
> you would ideally want all this backported as far as v4.6?  We'd have to
> figure out how to express that in stable tags.

Ideally yes, I think we would need to have them backported onto stable(s),
but so far I didn't see any bug-reports which could require these fixes.

Anyway, in term of backporting them - are there anything I can help with ?

WBR,
Vadim

> 
> 
> commit 60f6d5f7ebc4bba83d73115f58b805f2f2618667
> Author: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> Date:   Tue Oct 17 05:47:38 2017 -0700
> 
>     PCI: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF
>     
>     The Cavium ThunderX (CN8XXX) family of PCIe Root Ports does not advertise
>     an ACS capability.  However, the RTL internally implements similar
>     protection as if ACS had Request Redirection, Completion Redirection,
>     Source Validation, and Upstream Forwarding features enabled.
>     
>     Change Cavium ACS capabilities quirk flags accordingly.
>     
>     Fixes: b404bcfbf035 ("PCI: Add ACS quirk for all Cavium devices")
>     Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d33619a7bb..c23650cfd5a1 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4214,12 +4214,14 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Cavium root ports don't advertise an ACS capability.  However,
> +	 * the RTL internally implements similar protection as if ACS had
> +	 * Request Redirection, Completion Redirection, Source Validation,
> +	 * and Upstream Forwarding features enabled.  Assert that the
> +	 * hardware implements and enables equivalent ACS functionality for
> +	 * these flags.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
>  
>  	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
>  		return -ENOTTY;
> 
> commit 35da518fcad24f7be515976238a4667b5ccd1711
> Author: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> Date:   Tue Oct 17 05:47:39 2017 -0700
> 
>     PCI: Apply Cavium ThunderX ACS quirk to more Root Ports
>     
>     Extend the Cavium ThunderX ACS quirk to cover more device IDs and restrict
>     it to only Root Ports.
>     
>     Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>     [bhelgaas: changelog]
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index c23650cfd5a1..0e22cce05742 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,6 +4211,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> +static bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> +{
> +	/*
> +	 * Effectively selects all downstream ports for whole ThunderX 1
> +	 * family by 0xf800 mask (which represents 8 SoCs), while the lower
> +	 * bits of device ID are used to indicate which subdevice is used
> +	 * within the SoC.
> +	 */
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
> +
>  static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  {
>  	/*
> @@ -4223,7 +4236,7 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  	 */
>  	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
>  	return acs_flags ? 0 : 1;

Re: [PATCH v7 0/2] PCI: quirks: Cavium ThunderX ACS quirk update

[Bjorn Helgaas]

On Thu, Oct 19, 2017 at 04:59:21AM -0700, Vadim Lomovtsev wrote:
> On Thu, Oct 19, 2017 at 06:26:45AM -0500, Bjorn Helgaas wrote:
> > On Tue, Oct 17, 2017 at 05:47:37AM -0700, Vadim Lomovtsev wrote:
> > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > > 
> > > version 7:
> > > - update patch description accordingly to review comments;
> > > - split for two patches: for ACS mask change and device id match change;
> > > - remove macro #define of ACS flags, put it back to quirk function;
> > > - remove '__inline_' from device id matching function;
> > > - wrap code comments to fit into 80 columns;
> > > - comment fix: change 0xa00 to 0xf800 for matching function description;
> > > 
> > > version 6:
> > > - comment typo fix: change 0xa00 to 0xa000;
> > > 
> > > version 5:
> > > - update code comments accordingly to review comments;
> > > 
> > > version 4:
> > > - update ACS mask (remove TB and TD bits), update commit message (remove
> > >   ACS register printout);
> > > 
> > > version 3:
> > > - update subject: remove CN8XXX from subject line, replace it with ThunderX;
> > > 
> > > version 2:
> > > - update match function in order to filter only ThunderX devices by device
> > >   ids to properly filter CN8XXX devices, update subject & description with
> > >   ACS register info (v2 was rejected by maillist due to triple X in subject);
> > >   
> > > Vadim Lomovtsev (2):
> > >   PCI: quirks: Set Cavium ACS capability quirk flags to assert
> > >     RR/CR/SV/UF.
> > >   PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports
> > > 
> > >  drivers/pci/quirks.c | 26 ++++++++++++++++++++------
> > >  1 file changed, 20 insertions(+), 6 deletions(-)
> > 
> > If I'm reading this correctly, the first patch is basically fixing a bug in
> > the original Cavium ACS quirk (b404bcfbf035 ("PCI: Add ACS quirk for all
> > Cavium devices")).  Right?
> 
> Yes, first patch fixes improper ACS flags in the patch you mentioned.
> 
> The second one fixes devid check from another patch
> (b77d537d00d0 PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices).
> 
> > 
> > I put them on pci/virtualization for v4.15 as follows (patches unchanged,
> > changelogs wordsmithed).
> >
> 
> Good, thank you.
> 
> > You mentioned stable backports.  b404bcfbf035 ("PCI: Add ACS quirk for all
> > Cavium devices") appeared in v4.6.  b77d537d00d0 ("PCI: Apply Cavium ACS
> > quirk only to CN81xx/CN83xx/CN88xx devices") appeared in v4.12.  I assume
> > you would ideally want all this backported as far as v4.6?  We'd have to
> > figure out how to express that in stable tags.
> 
> Ideally yes, I think we would need to have them backported onto stable(s),
> but so far I didn't see any bug-reports which could require these fixes.
> 
> Anyway, in term of backporting them - are there anything I can help with ?

I added the following tags, which I *think* should be sufficient:

  PCI: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF
  Cc: stable@vger.kernel.org      # v4.6+: b77d537d00d0: PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices

  PCI: Apply Cavium ThunderX ACS quirk to more Root Ports
  Cc: stable@vger.kernel.org      # v4.12+

Re: [PATCH v7 0/2] PCI: quirks: Cavium ThunderX ACS quirk update

[Vadim Lomovtsev]

On Thu, Oct 19, 2017 at 01:50:10PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 19, 2017 at 04:59:21AM -0700, Vadim Lomovtsev wrote:
> > On Thu, Oct 19, 2017 at 06:26:45AM -0500, Bjorn Helgaas wrote:
> > > On Tue, Oct 17, 2017 at 05:47:37AM -0700, Vadim Lomovtsev wrote:
> > > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > > > 
> > > > version 7:
> > > > - update patch description accordingly to review comments;
> > > > - split for two patches: for ACS mask change and device id match change;
> > > > - remove macro #define of ACS flags, put it back to quirk function;
> > > > - remove '__inline_' from device id matching function;
> > > > - wrap code comments to fit into 80 columns;
> > > > - comment fix: change 0xa00 to 0xf800 for matching function description;
> > > > 
> > > > version 6:
> > > > - comment typo fix: change 0xa00 to 0xa000;
> > > > 
> > > > version 5:
> > > > - update code comments accordingly to review comments;
> > > > 
> > > > version 4:
> > > > - update ACS mask (remove TB and TD bits), update commit message (remove
> > > >   ACS register printout);
> > > > 
> > > > version 3:
> > > > - update subject: remove CN8XXX from subject line, replace it with ThunderX;
> > > > 
> > > > version 2:
> > > > - update match function in order to filter only ThunderX devices by device
> > > >   ids to properly filter CN8XXX devices, update subject & description with
> > > >   ACS register info (v2 was rejected by maillist due to triple X in subject);
> > > >   
> > > > Vadim Lomovtsev (2):
> > > >   PCI: quirks: Set Cavium ACS capability quirk flags to assert
> > > >     RR/CR/SV/UF.
> > > >   PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports
> > > > 
> > > >  drivers/pci/quirks.c | 26 ++++++++++++++++++++------
> > > >  1 file changed, 20 insertions(+), 6 deletions(-)
> > > 
> > > If I'm reading this correctly, the first patch is basically fixing a bug in
> > > the original Cavium ACS quirk (b404bcfbf035 ("PCI: Add ACS quirk for all
> > > Cavium devices")).  Right?
> > 
> > Yes, first patch fixes improper ACS flags in the patch you mentioned.
> > 
> > The second one fixes devid check from another patch
> > (b77d537d00d0 PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices).
> > 
> > > 
> > > I put them on pci/virtualization for v4.15 as follows (patches unchanged,
> > > changelogs wordsmithed).
> > >
> > 
> > Good, thank you.
> > 
> > > You mentioned stable backports.  b404bcfbf035 ("PCI: Add ACS quirk for all
> > > Cavium devices") appeared in v4.6.  b77d537d00d0 ("PCI: Apply Cavium ACS
> > > quirk only to CN81xx/CN83xx/CN88xx devices") appeared in v4.12.  I assume
> > > you would ideally want all this backported as far as v4.6?  We'd have to
> > > figure out how to express that in stable tags.
> > 
> > Ideally yes, I think we would need to have them backported onto stable(s),
> > but so far I didn't see any bug-reports which could require these fixes.
> > 
> > Anyway, in term of backporting them - are there anything I can help with ?
> 
> I added the following tags, which I *think* should be sufficient:
> 
>   PCI: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF
>   Cc: stable@vger.kernel.org      # v4.6+: b77d537d00d0: PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices
> 
>   PCI: Apply Cavium ThunderX ACS quirk to more Root Ports
>   Cc: stable@vger.kernel.org      # v4.12+

Looks good. Thank you.

-Vadim