[PATCH 0/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[PATCH 0/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bartosz Pawlowski]

This patch series addresses the problem with A an B steppings of
Intel IPU E2000 which expects incorrect endianness in data field of ATS
invalidation request TLP by disabling ATS capability for vulnerable
devices.

Bartosz Pawlowski (2):
  PCI: Extract ATS disabling to a helper function
  PCI: Disable ATS for specific Intel IPU E2000 devices

 drivers/pci/quirks.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

-- 
2.41.0

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

[PATCH 1/2] PCI: Extract ATS disabling to a helper function

[Bartosz Pawlowski]

Introduce quirk_no_ats() helper function to provide a standard way to
disable ATS capability in PCI quirks.

Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pci/quirks.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 321156ca273d..a900546d8d45 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5505,6 +5505,12 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0420, quirk_no_ext_tags);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0422, quirk_no_ext_tags);
 
 #ifdef CONFIG_PCI_ATS
+static void quirk_no_ats(struct pci_dev *pdev)
+{
+	pci_info(pdev, "disabling ATS\n");
+	pdev->ats_cap = 0;
+}
+
 /*
  * Some devices require additional driver setup to enable ATS.  Don't use
  * ATS for those devices as ATS will be enabled before the driver has had a
@@ -5518,14 +5524,10 @@ static void quirk_amd_harvest_no_ats(struct pci_dev *pdev)
 		    (pdev->subsystem_device == 0xce19 ||
 		     pdev->subsystem_device == 0xcc10 ||
 		     pdev->subsystem_device == 0xcc08))
-			goto no_ats;
-		else
-			return;
+			quirk_no_ats(pdev);
+	} else {
+		quirk_no_ats(pdev);
 	}
-
-no_ats:
-	pci_info(pdev, "disabling ATS\n");
-	pdev->ats_cap = 0;
 }
 
 /* AMD Stoney platform GPU */
-- 
2.41.0

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

[PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bartosz Pawlowski]

There is a HW issue in A and B steppings of Intel IPU E2000 that it
expects wrong endianness in ATS invalidation message body. This problem
can lead to outdated translations being returned as valid and finally
cause system instability.

In order to prevent such issues introduce quirk_intel_e2000_no_ats()
function to disable ATS for vulnerable IPU E2000 devices.

Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 drivers/pci/quirks.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a900546d8d45..9aa1e0148ed2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5550,6 +5550,28 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7347, quirk_amd_harvest_no_ats);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x734f, quirk_amd_harvest_no_ats);
 /* AMD Raven platform iGPU */
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
+
+/*
+ * Intel IPU E2000 revisions before C0 implement incorrect endianness
+ * in ATS Invalidate Request message body. Although there is existing software
+ * workaround for this issue, it is not functionally complete (no 5-lvl paging
+ * support) and it requires changes in all IOMMU implementations supporting
+ * ATS. Therefore, disabling ATS seems to be more reasonable.
+ */
+static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
+{
+	if (pdev->revision < 0x20)
+		quirk_no_ats(pdev);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1451, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1452, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1453, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1454, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1455, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
 #endif /* CONFIG_PCI_ATS */
 
 /* Freescale PCIe doesn't support MSI in RC mode */
-- 
2.41.0

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bjorn Helgaas]

On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
> There is a HW issue in A and B steppings of Intel IPU E2000 that it
> expects wrong endianness in ATS invalidation message body. This problem
> can lead to outdated translations being returned as valid and finally
> cause system instability.
> 
> In order to prevent such issues introduce quirk_intel_e2000_no_ats()
> function to disable ATS for vulnerable IPU E2000 devices.
> 
> Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Andy, Alexander, would you please reiterate your reviewed-by on the
mailing list?  I try to avoid relying on internal reviews collected by
the author.  Those are great and I'm glad they happen, but it's good
if they also appear as part of the public conversation on the mailing
list.

> ---
>  drivers/pci/quirks.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a900546d8d45..9aa1e0148ed2 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5550,6 +5550,28 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7347, quirk_amd_harvest_no_ats);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x734f, quirk_amd_harvest_no_ats);
>  /* AMD Raven platform iGPU */
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> +
> +/*
> + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> + * in ATS Invalidate Request message body. Although there is existing software
> + * workaround for this issue, it is not functionally complete (no 5-lvl paging
> + * support) and it requires changes in all IOMMU implementations supporting
> + * ATS. Therefore, disabling ATS seems to be more reasonable.
> + */
> +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> +{
> +	if (pdev->revision < 0x20)
> +		quirk_no_ats(pdev);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1451, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1452, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1453, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1454, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1455, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
>  #endif /* CONFIG_PCI_ATS */
>  
>  /* Freescale PCIe doesn't support MSI in RC mode */
> -- 
> 2.41.0
> 
> ---------------------------------------------------------------------
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
> 
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
> 

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Andy Shevchenko]

On Wed, Aug 16, 2023 at 12:39:06PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
> > There is a HW issue in A and B steppings of Intel IPU E2000 that it
> > expects wrong endianness in ATS invalidation message body. This problem
> > can lead to outdated translations being returned as valid and finally
> > cause system instability.
> > 
> > In order to prevent such issues introduce quirk_intel_e2000_no_ats()
> > function to disable ATS for vulnerable IPU E2000 devices.
> > 
> > Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> 
> Andy, Alexander, would you please reiterate your reviewed-by on the
> mailing list?  I try to avoid relying on internal reviews collected by
> the author.  Those are great and I'm glad they happen, but it's good
> if they also appear as part of the public conversation on the mailing
> list.

They are legit for my name, I dunno what exactly you want to hear from me.
The internal process of review requires these tags by default, it's hard
to deviate from maintainer to maintainer.

> > Intel Technology Poland sp. z o.o.
> > ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> > Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
> > 
> > Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
> > This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

What really needs to be dropped is the footer.
Bartosz, consult with the internal resources on how to get rid of it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bartosz Pawlowski]

On 17.08.2023 11:52, Andy Shevchenko wrote:
> On Wed, Aug 16, 2023 at 12:39:06PM -0500, Bjorn Helgaas wrote:
>> On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
>>> There is a HW issue in A and B steppings of Intel IPU E2000 that it
>>> expects wrong endianness in ATS invalidation message body. This problem
>>> can lead to outdated translations being returned as valid and finally
>>> cause system instability.
>>>
>>> In order to prevent such issues introduce quirk_intel_e2000_no_ats()
>>> function to disable ATS for vulnerable IPU E2000 devices.
>>>
>>> Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>> Andy, Alexander, would you please reiterate your reviewed-by on the
>> mailing list?  I try to avoid relying on internal reviews collected by
>> the author.  Those are great and I'm glad they happen, but it's good
>> if they also appear as part of the public conversation on the mailing
>> list.
> They are legit for my name, I dunno what exactly you want to hear from me.
> The internal process of review requires these tags by default, it's hard
> to deviate from maintainer to maintainer.
>
>>> Intel Technology Poland sp. z o.o.
>>> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
>>> Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
>>>
>>> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
>>> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
> What really needs to be dropped is the footer.
> Bartosz, consult with the internal resources on how to get rid of it.
Requested already and I hope it won't appear anymore.

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Alexander Lobakin]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Thu, 17 Aug 2023 12:52:25 +0300

> On Wed, Aug 16, 2023 at 12:39:06PM -0500, Bjorn Helgaas wrote:
>> On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
>>> There is a HW issue in A and B steppings of Intel IPU E2000 that it
>>> expects wrong endianness in ATS invalidation message body. This problem
>>> can lead to outdated translations being returned as valid and finally
>>> cause system instability.
>>>
>>> In order to prevent such issues introduce quirk_intel_e2000_no_ats()
>>> function to disable ATS for vulnerable IPU E2000 devices.
>>>
>>> Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>
>> Andy, Alexander, would you please reiterate your reviewed-by on the
>> mailing list?  I try to avoid relying on internal reviews collected by
>> the author.  Those are great and I'm glad they happen, but it's good
>> if they also appear as part of the public conversation on the mailing
>> list.
> 
> They are legit for my name, I dunno what exactly you want to hear from me.
> The internal process of review requires these tags by default, it's hard
> to deviate from maintainer to maintainer.

+1 under everything, it works the same way for me.

[...]

Thanks,
Olek

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bjorn Helgaas]

On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
> There is a HW issue in A and B steppings of Intel IPU E2000 that it
> expects wrong endianness in ATS invalidation message body. This problem
> can lead to outdated translations being returned as valid and finally
> cause system instability.

Is there a published erratum for this?  Or at least a number to
identify it?

> In order to prevent such issues introduce quirk_intel_e2000_no_ats()
> function to disable ATS for vulnerable IPU E2000 devices.
> 
> Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>  drivers/pci/quirks.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a900546d8d45..9aa1e0148ed2 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5550,6 +5550,28 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7347, quirk_amd_harvest_no_ats);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x734f, quirk_amd_harvest_no_ats);
>  /* AMD Raven platform iGPU */
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> +
> +/*
> + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> + * in ATS Invalidate Request message body. Although there is existing software
> + * workaround for this issue, it is not functionally complete (no 5-lvl paging
> + * support) and it requires changes in all IOMMU implementations supporting
> + * ATS. Therefore, disabling ATS seems to be more reasonable.

Can we reference the commit that added the existing software
workaround?

It sounds like systems that (a) don't require 5-level paging and (b)
use an IOMMU implementation that include the appropriate changes might
still be able to use ATS?  Is there a way for them to do that?

> + */
> +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> +{
> +	if (pdev->revision < 0x20)
> +		quirk_no_ats(pdev);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1451, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1452, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1453, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1454, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1455, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
>  #endif /* CONFIG_PCI_ATS */
>  
>  /* Freescale PCIe doesn't support MSI in RC mode */
> -- 
> 2.41.0
> 
> ---------------------------------------------------------------------
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
> 
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
> 

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 02:06:23PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:

...

> > +/*
> > + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> > + * in ATS Invalidate Request message body. Although there is existing software
> > + * workaround for this issue, it is not functionally complete (no 5-lvl paging
> > + * support) and it requires changes in all IOMMU implementations supporting
> > + * ATS. Therefore, disabling ATS seems to be more reasonable.
> 
> Can we reference the commit that added the existing software
> workaround?

See below.

> It sounds like systems that (a) don't require 5-level paging and (b)
> use an IOMMU implementation that include the appropriate changes might
> still be able to use ATS?  Is there a way for them to do that?
> 
> > + */
> > +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> > +{

> > +	if (pdev->revision < 0x20)

Isn't it the answer to your question?

> > +		quirk_no_ats(pdev);
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 10:59:20PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 02:06:23PM -0500, Bjorn Helgaas wrote:
> > On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:

...

> > > +/*
> > > + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> > > + * in ATS Invalidate Request message body. Although there is existing software
> > > + * workaround for this issue, it is not functionally complete (no 5-lvl paging
> > > + * support) and it requires changes in all IOMMU implementations supporting
> > > + * ATS. Therefore, disabling ATS seems to be more reasonable.
> > 
> > Can we reference the commit that added the existing software
> > workaround?

> See below.

Oh, I meant the second question here, i.e.

> > It sounds like systems that (a) don't require 5-level paging and (b)
> > use an IOMMU implementation that include the appropriate changes might
> > still be able to use ATS?  Is there a way for them to do that?

^^^ this one.

> > > + */
> > > +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> > > +{
> 
> > > +	if (pdev->revision < 0x20)
> 
> Isn't it the answer to your question?
> 
> > > +		quirk_no_ats(pdev);
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bjorn Helgaas]

On Wed, Sep 06, 2023 at 11:00:19PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 10:59:20PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 02:06:23PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
> 
> ...
> 
> > > > +/*
> > > > + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> > > > + * in ATS Invalidate Request message body. Although there is existing software
> > > > + * workaround for this issue, it is not functionally complete (no 5-lvl paging
> > > > + * support) and it requires changes in all IOMMU implementations supporting
> > > > + * ATS. Therefore, disabling ATS seems to be more reasonable.
> > > 
> > > Can we reference the commit that added the existing software
> > > workaround?
> 
> > See below.
> 
> Oh, I meant the second question here, i.e.
> 
> > > It sounds like systems that (a) don't require 5-level paging and (b)
> > > use an IOMMU implementation that include the appropriate changes might
> > > still be able to use ATS?  Is there a way for them to do that?
> 
> ^^^ this one.

Sorry, I'm missing your point here.

The comment mentions an existing partial software workaround.
Presumably that was added by some commit, and it would be helpful to
know which one.

The comment also suggests that if the software workaround were
completed (or if a system didn't require 5-level paging) and it had
related changes to its IOMMU driver, we could still use ATS even on
hardware with this defect.

So I'm wondering if there's a way for an IOMMU driver that has the
required changes and can tell that we're not using 5-level paging can
override this quirk that disables ATS.

Maybe we want to unconditionally disable ATS on these broken devices.
In that case, I think we should just completely drop the comments
about the software workaround and IOMMU driver changes because they
wouldn't be relevant.

> > > > + */
> > > > +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> > > > +{
> > 
> > > > +	if (pdev->revision < 0x20)
> > 
> > Isn't it the answer to your question?
> > 
> > > > +		quirk_no_ats(pdev);
> > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 03:13:29PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 06, 2023 at 11:00:19PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 10:59:20PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 06, 2023 at 02:06:23PM -0500, Bjorn Helgaas wrote:
> > > > On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:

...

> > > > > +/*
> > > > > + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> > > > > + * in ATS Invalidate Request message body. Although there is existing software
> > > > > + * workaround for this issue, it is not functionally complete (no 5-lvl paging
> > > > > + * support) and it requires changes in all IOMMU implementations supporting
> > > > > + * ATS. Therefore, disabling ATS seems to be more reasonable.
> > > > 
> > > > Can we reference the commit that added the existing software
> > > > workaround?
> > 
> > > See below.
> > 
> > Oh, I meant the second question here, i.e.
> > 
> > > > It sounds like systems that (a) don't require 5-level paging and (b)
> > > > use an IOMMU implementation that include the appropriate changes might
> > > > still be able to use ATS?  Is there a way for them to do that?
> > 
> > ^^^ this one.
> 
> Sorry, I'm missing your point here.
> 
> The comment mentions an existing partial software workaround.
> Presumably that was added by some commit, and it would be helpful to
> know which one.
> 
> The comment also suggests that if the software workaround were
> completed (or if a system didn't require 5-level paging) and it had
> related changes to its IOMMU driver, we could still use ATS even on
> hardware with this defect.
> 
> So I'm wondering if there's a way for an IOMMU driver that has the
> required changes and can tell that we're not using 5-level paging can
> override this quirk that disables ATS.
> 
> Maybe we want to unconditionally disable ATS on these broken devices.
> In that case, I think we should just completely drop the comments
> about the software workaround and IOMMU driver changes because they
> wouldn't be relevant.

I see now what you are for. Yes, I agree that if we can't provide a workaround
then probably comment is a bit bogus.

> > > > > + */
> > > > > +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> > > > > +{
> > > 
> > > > > +	if (pdev->revision < 0x20)
> > > 
> > > Isn't it the answer to your question?
> > > 
> > > > > +		quirk_no_ats(pdev);
> > > > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bartosz Pawlowski]

On 06.09.2023 22:13, Bjorn Helgaas wrote:
> On Wed, Sep 06, 2023 at 11:00:19PM +0300, Andy Shevchenko wrote:
>> On Wed, Sep 06, 2023 at 10:59:20PM +0300, Andy Shevchenko wrote:
>>> On Wed, Sep 06, 2023 at 02:06:23PM -0500, Bjorn Helgaas wrote:
>>>> On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
>> ...
>>
>>>>> +/*
>>>>> + * Intel IPU E2000 revisions before C0 implement incorrect endianness
>>>>> + * in ATS Invalidate Request message body. Although there is existing software
>>>>> + * workaround for this issue, it is not functionally complete (no 5-lvl paging
>>>>> + * support) and it requires changes in all IOMMU implementations supporting
>>>>> + * ATS. Therefore, disabling ATS seems to be more reasonable.
>>>> Can we reference the commit that added the existing software
>>>> workaround?
>>> See below.
>> Oh, I meant the second question here, i.e.
>>
>>>> It sounds like systems that (a) don't require 5-level paging and (b)
>>>> use an IOMMU implementation that include the appropriate changes might
>>>> still be able to use ATS?  Is there a way for them to do that?
>> ^^^ this one.
> Sorry, I'm missing your point here.
>
> The comment mentions an existing partial software workaround.
> Presumably that was added by some commit, and it would be helpful to
> know which one.
>
> The comment also suggests that if the software workaround were
> completed (or if a system didn't require 5-level paging) and it had
> related changes to its IOMMU driver, we could still use ATS even on
> hardware with this defect.
>
> So I'm wondering if there's a way for an IOMMU driver that has the
> required changes and can tell that we're not using 5-level paging can
> override this quirk that disables ATS.
>
> Maybe we want to unconditionally disable ATS on these broken devices.
> In that case, I think we should just completely drop the comments
> about the software workaround and IOMMU driver changes because they
> wouldn't be relevant.

This software workaround was shared only with E2000 customers and not publicly due to it's complexity and tricky design, so I think we cannot refer to it and I'll drop this comment.

Regards,
Bartosz

Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bjorn Helgaas]

On Wed, Aug 16, 2023 at 12:50:28PM -0700, Sheena Mohan wrote:
> It is good to have the SW fix for backward compatibility rather than
> disabling ATS on previous revisions. Maybe adding a flag to toggle the
> feature will be useful.

FYI, I received this (and probably the other direct recipients did,
too), but I think the mailing lists rejected it, probably because it
was a multi-part message (see
http://vger.kernel.org/majordomo-info.html).

The list archive is https://lore.kernel.org/linux-pci/20230816172115.1375716-1-bartosz.pawlowski@intel.com/

> On Wed, Aug 16, 2023 at 10:39 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
> > > There is a HW issue in A and B steppings of Intel IPU E2000 that it
> > > expects wrong endianness in ATS invalidation message body. This problem
> > > can lead to outdated translations being returned as valid and finally
> > > cause system instability.
> > >
> > > In order to prevent such issues introduce quirk_intel_e2000_no_ats()
> > > function to disable ATS for vulnerable IPU E2000 devices.
> > >
> > > Signed-off-by: Bartosz Pawlowski <bartosz.pawlowski@intel.com>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> >
> > Andy, Alexander, would you please reiterate your reviewed-by on the
> > mailing list?  I try to avoid relying on internal reviews collected by
> > the author.  Those are great and I'm glad they happen, but it's good
> > if they also appear as part of the public conversation on the mailing
> > list.
> >
> > > ---
> > >  drivers/pci/quirks.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index a900546d8d45..9aa1e0148ed2 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -5550,6 +5550,28 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI,
> > 0x7347, quirk_amd_harvest_no_ats);
> > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x734f,
> > quirk_amd_harvest_no_ats);
> > >  /* AMD Raven platform iGPU */
> > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8,
> > quirk_amd_harvest_no_ats);
> > > +
> > > +/*
> > > + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> > > + * in ATS Invalidate Request message body. Although there is existing
> > software
> > > + * workaround for this issue, it is not functionally complete (no 5-lvl
> > paging
> > > + * support) and it requires changes in all IOMMU implementations
> > supporting
> > > + * ATS. Therefore, disabling ATS seems to be more reasonable.
> > > + */
> > > +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> > > +{
> > > +     if (pdev->revision < 0x20)
> > > +             quirk_no_ats(pdev);
> > > +}
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1451,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1452,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1453,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1454,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1455,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a,
> > quirk_intel_e2000_no_ats);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c,
> > quirk_intel_e2000_no_ats);
> > >  #endif /* CONFIG_PCI_ATS */
> > >
> > >  /* Freescale PCIe doesn't support MSI in RC mode */
> > > --
> > > 2.41.0
> > >
> > > ---------------------------------------------------------------------
> > > Intel Technology Poland sp. z o.o.
> > > ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII
> > Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP
> > 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> > > Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu
> > ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w
> > transakcjach handlowych.
> > >
> > > Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego
> > adresata i moze zawierac informacje poufne. W razie przypadkowego
> > otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej
> > usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
> > > This e-mail and any attachments may contain confidential material for
> > the sole use of the intended recipient(s). If you are not the intended
> > recipient, please contact the sender and delete all copies; any review or
> > distribution by others is strictly prohibited.
> > >
> >