PCI: quirks: update cavium ACS quirk implementation

Message ID 1505217316-15702-1-git-send-email-Vadim.Lomovtsev@caviumnetworks.com
State Superseded
Headers show
Series
  • PCI: quirks: update cavium ACS quirk implementation
Related show

Commit Message

Vadim Lomovtsev Sept. 12, 2017, 11:55 a.m.
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(-)

Comments

Alex Williamson Sept. 12, 2017, 4:15 p.m. | #1
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
Vadim Lomovtsev Sept. 13, 2017, 11:37 a.m. | #2
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
Vadim Lomovtsev Sept. 13, 2017, 12:01 p.m. | #3
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

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)