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 <[email protected]>
---
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
On Tue, 12 Sep 2017 04:55:16 -0700
Vadim Lomovtsev <[email protected]> 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 <[email protected]>
> ---
> 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
On Tue, Sep 12, 2017 at 10:15:45AM -0600, Alex Williamson wrote:
> On Tue, 12 Sep 2017 04:55:16 -0700
> Vadim Lomovtsev <[email protected]> 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 <[email protected]>
> > ---
> > 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
To my previous email, please ignore this for now:
> Would the resetting of RR/CR/UF/SV bits be more correct here ?
WBR,
Vadim
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 <[email protected]>
---
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
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: [email protected]; [email protected]; [email protected]; [email protected]
Cc: Snyder, Wilson; Daney, David; [email protected]; 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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
On Mon, 18 Sep 2017 01:48:01 -0700
Vadim Lomovtsev <[email protected]> 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 <[email protected]>
> ---
> 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)
On Wed, Sep 20, 2017 at 10:31:51AM -0600, Alex Williamson wrote:
> On Mon, 18 Sep 2017 01:48:01 -0700
> Vadim Lomovtsev <[email protected]> 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 <[email protected]>
> > ---
> > 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)
>
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
On Mon, 25 Sep 2017 06:08:40 -0700
Vadim Lomovtsev <[email protected]> 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 <[email protected]>
> ---
> drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
Reviewed-by: Alex Williamson <[email protected]>
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)
On Tue, Sep 26, 2017 at 09:43:43AM -0600, Alex Williamson wrote:
> On Mon, 25 Sep 2017 06:08:40 -0700
> Vadim Lomovtsev <[email protected]> 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 <[email protected]>
> > ---
> > drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> > 1 file changed, 21 insertions(+), 8 deletions(-)
>
>
> Reviewed-by: Alex Williamson <[email protected]>
>
> 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)
>
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 <[email protected]> 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 <[email protected]>
> > > ---
> > > drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> > > 1 file changed, 21 insertions(+), 8 deletions(-)
> >
> >
> > Reviewed-by: Alex Williamson <[email protected]>
> >
> > 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)
> >
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
On Wed, 27 Sep 2017 11:20:39 -0700
Vadim Lomovtsev <[email protected]> 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 <[email protected]>
> ---
> v5 -> v6: comment typo fix: change 0xa00 to 0xa000
Reviewed-by: Alex Williamson <[email protected]>
> 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)
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 <[email protected]> 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 <[email protected]>
> > ---
> > v5 -> v6: comment typo fix: change 0xa00 to 0xa000
>
> Reviewed-by: Alex Williamson <[email protected]>
>
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)
>