IOMMU Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
@ 2019-10-09 22:45 Bjorn Helgaas
  2019-10-09 22:45 ` [PATCH 1/2] " Bjorn Helgaas
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2019-10-09 22:45 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan, David Woodhouse, Joerg Roedel
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

I think intel-iommu.c depends on CONFIG_AMD_IOMMU in an undesirable way:

When CONFIG_INTEL_IOMMU_SVM=y, iommu_enable_dev_iotlb() calls PRI
interfaces (pci_reset_pri() and pci_enable_pri()), but those are only
implemented when CONFIG_PCI_PRI is enabled.  If CONFIG_PCI_PRI is not
enabled, there are stubs that just return failure.

The INTEL_IOMMU_SVM Kconfig does nothing with PCI_PRI, but AMD_IOMMU
selects PCI_PRI.  So if AMD_IOMMU is enabled, intel-iommu.c gets the full
PRI interfaces.  If AMD_IOMMU is not enabled, it gets the PRI stubs.

This seems wrong.  The first patch here makes INTEL_IOMMU_SVM select
PCI_PRI so intel-iommu.c always gets the full PRI interfaces.

The second patch moves pci_prg_resp_pasid_required(), which simply returns
a bit from the PCI capability, from #ifdef CONFIG_PCI_PASID to #ifdef
CONFIG_PCI_PRI.  This is related because INTEL_IOMMU_SVM already *does*
select PCI_PASID, so it previously always got pci_prg_resp_pasid_required()
even though it got stubs for other PRI things.

Since these are related and I have several follow-on ATS-related patches in
the queue, I'd like to take these both via the PCI tree.

Bjorn Helgaas (2):
  iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
  PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI

 drivers/iommu/Kconfig   |  1 +
 drivers/pci/ats.c       | 55 +++++++++++++++++++----------------------
 include/linux/pci-ats.h | 11 ++++-----
 3 files changed, 31 insertions(+), 36 deletions(-)

-- 
2.23.0.581.g78d2f28ef7-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
  2019-10-09 22:45 [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Bjorn Helgaas
@ 2019-10-09 22:45 ` " Bjorn Helgaas
  2019-10-09 23:42   ` Kuppuswamy Sathyanarayanan
  2019-10-09 22:45 ` [PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI Bjorn Helgaas
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2019-10-09 22:45 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan, David Woodhouse, Joerg Roedel
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

When CONFIG_INTEL_IOMMU_SVM=y, iommu_enable_dev_iotlb() calls PRI
interfaces (pci_reset_pri() and pci_enable_pri()), but those are only
implemented when CONFIG_PCI_PRI is enabled.

Previously INTEL_IOMMU_SVM selected PCI_PASID but not PCI_PRI, so the state
of PCI_PRI depended on whether AMD_IOMMU (which selects PCI_PRI) was
enabled or PCI_PRI was enabled explicitly.

The behavior of iommu_enable_dev_iotlb() should not depend on whether
AMD_IOMMU is enabled.  Make it predictable by having INTEL_IOMMU_SVM select
PCI_PRI so iommu_enable_dev_iotlb() always uses the full implementations of
PRI interfaces.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/iommu/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index e3842eabcfdd..b183c9f916b0 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -207,6 +207,7 @@ config INTEL_IOMMU_SVM
 	bool "Support for Shared Virtual Memory with Intel IOMMU"
 	depends on INTEL_IOMMU && X86
 	select PCI_PASID
+	select PCI_PRI
 	select MMU_NOTIFIER
 	help
 	  Shared Virtual Memory (SVM) provides a facility for devices
-- 
2.23.0.581.g78d2f28ef7-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI
  2019-10-09 22:45 [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Bjorn Helgaas
  2019-10-09 22:45 ` [PATCH 1/2] " Bjorn Helgaas
@ 2019-10-09 22:45 ` Bjorn Helgaas
  2019-10-09 22:55   ` Kuppuswamy Sathyanarayanan
  2019-10-09 23:50 ` [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Jerry Snitselaar
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2019-10-09 22:45 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan, David Woodhouse, Joerg Roedel
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

pci_prg_resp_pasid_required() returns the value of the "PRG Response PASID
Required" bit from the PRI capability, but the interface was previously
defined under #ifdef CONFIG_PCI_PASID.

Move it from CONFIG_PCI_PASID to CONFIG_PCI_PRI so it's with the other
PRI-related things.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c       | 55 +++++++++++++++++++----------------------
 include/linux/pci-ats.h | 11 ++++-----
 2 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index e18499243f84..0d06177252c7 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -280,6 +280,31 @@ int pci_reset_pri(struct pci_dev *pdev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_reset_pri);
+
+/**
+ * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
+ *				 status.
+ * @pdev: PCI device structure
+ *
+ * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
+ */
+int pci_prg_resp_pasid_required(struct pci_dev *pdev)
+{
+	u16 status;
+	int pos;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
+	if (!pos)
+		return 0;
+
+	pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
+
+	if (status & PCI_PRI_STATUS_PASID)
+		return 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
 #endif /* CONFIG_PCI_PRI */
 
 #ifdef CONFIG_PCI_PASID
@@ -395,36 +420,6 @@ int pci_pasid_features(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL_GPL(pci_pasid_features);
 
-/**
- * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
- *				 status.
- * @pdev: PCI device structure
- *
- * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
- *
- * Even though the PRG response PASID status is read from PRI Status
- * Register, since this API will mainly be used by PASID users, this
- * function is defined within #ifdef CONFIG_PCI_PASID instead of
- * CONFIG_PCI_PRI.
- */
-int pci_prg_resp_pasid_required(struct pci_dev *pdev)
-{
-	u16 status;
-	int pos;
-
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
-	if (!pos)
-		return 0;
-
-	pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
-
-	if (status & PCI_PRI_STATUS_PASID)
-		return 1;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
-
 #define PASID_NUMBER_SHIFT	8
 #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
 /**
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 1ebb88e7c184..a7a2b3d94fcc 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -10,6 +10,7 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs);
 void pci_disable_pri(struct pci_dev *pdev);
 void pci_restore_pri_state(struct pci_dev *pdev);
 int pci_reset_pri(struct pci_dev *pdev);
+int pci_prg_resp_pasid_required(struct pci_dev *pdev);
 
 #else /* CONFIG_PCI_PRI */
 
@@ -31,6 +32,10 @@ static inline int pci_reset_pri(struct pci_dev *pdev)
 	return -ENODEV;
 }
 
+static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
+{
+	return 0;
+}
 #endif /* CONFIG_PCI_PRI */
 
 #ifdef CONFIG_PCI_PASID
@@ -40,7 +45,6 @@ void pci_disable_pasid(struct pci_dev *pdev);
 void pci_restore_pasid_state(struct pci_dev *pdev);
 int pci_pasid_features(struct pci_dev *pdev);
 int pci_max_pasids(struct pci_dev *pdev);
-int pci_prg_resp_pasid_required(struct pci_dev *pdev);
 
 #else  /* CONFIG_PCI_PASID */
 
@@ -66,11 +70,6 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
 {
 	return -EINVAL;
 }
-
-static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
-{
-	return 0;
-}
 #endif /* CONFIG_PCI_PASID */
 
 
-- 
2.23.0.581.g78d2f28ef7-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI
  2019-10-09 22:45 ` [PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI Bjorn Helgaas
@ 2019-10-09 22:55   ` Kuppuswamy Sathyanarayanan
  0 siblings, 0 replies; 8+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2019-10-09 22:55 UTC (permalink / raw)
  To: Bjorn Helgaas, David Woodhouse, Joerg Roedel
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu, Bjorn Helgaas


On 10/9/19 3:45 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> pci_prg_resp_pasid_required() returns the value of the "PRG Response PASID
> Required" bit from the PRI capability, but the interface was previously
> defined under #ifdef CONFIG_PCI_PASID.
>
> Move it from CONFIG_PCI_PASID to CONFIG_PCI_PRI so it's with the other
> PRI-related things.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>   drivers/pci/ats.c       | 55 +++++++++++++++++++----------------------
>   include/linux/pci-ats.h | 11 ++++-----
>   2 files changed, 30 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index e18499243f84..0d06177252c7 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -280,6 +280,31 @@ int pci_reset_pri(struct pci_dev *pdev)
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(pci_reset_pri);
> +
> +/**
> + * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
> + *				 status.
> + * @pdev: PCI device structure
> + *
> + * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
> + */
> +int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> +{
> +	u16 status;
> +	int pos;
> +
> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
> +	if (!pos)
> +		return 0;
> +
> +	pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
> +
> +	if (status & PCI_PRI_STATUS_PASID)
> +		return 1;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
>   #endif /* CONFIG_PCI_PRI */
>   
>   #ifdef CONFIG_PCI_PASID
> @@ -395,36 +420,6 @@ int pci_pasid_features(struct pci_dev *pdev)
>   }
>   EXPORT_SYMBOL_GPL(pci_pasid_features);
>   
> -/**
> - * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
> - *				 status.
> - * @pdev: PCI device structure
> - *
> - * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
> - *
> - * Even though the PRG response PASID status is read from PRI Status
> - * Register, since this API will mainly be used by PASID users, this
> - * function is defined within #ifdef CONFIG_PCI_PASID instead of
> - * CONFIG_PCI_PRI.
> - */
> -int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> -{
> -	u16 status;
> -	int pos;
> -
> -	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
> -	if (!pos)
> -		return 0;
> -
> -	pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
> -
> -	if (status & PCI_PRI_STATUS_PASID)
> -		return 1;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
> -
>   #define PASID_NUMBER_SHIFT	8
>   #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
>   /**
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index 1ebb88e7c184..a7a2b3d94fcc 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -10,6 +10,7 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs);
>   void pci_disable_pri(struct pci_dev *pdev);
>   void pci_restore_pri_state(struct pci_dev *pdev);
>   int pci_reset_pri(struct pci_dev *pdev);
> +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
>   
>   #else /* CONFIG_PCI_PRI */
>   
> @@ -31,6 +32,10 @@ static inline int pci_reset_pri(struct pci_dev *pdev)
>   	return -ENODEV;
>   }
>   
> +static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> +{
> +	return 0;
> +}
>   #endif /* CONFIG_PCI_PRI */
>   
>   #ifdef CONFIG_PCI_PASID
> @@ -40,7 +45,6 @@ void pci_disable_pasid(struct pci_dev *pdev);
>   void pci_restore_pasid_state(struct pci_dev *pdev);
>   int pci_pasid_features(struct pci_dev *pdev);
>   int pci_max_pasids(struct pci_dev *pdev);
> -int pci_prg_resp_pasid_required(struct pci_dev *pdev);
>   
>   #else  /* CONFIG_PCI_PASID */
>   
> @@ -66,11 +70,6 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
>   {
>   	return -EINVAL;
>   }
> -
> -static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> -{
> -	return 0;
> -}
>   #endif /* CONFIG_PCI_PASID */
>   
>   

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
  2019-10-09 22:45 ` [PATCH 1/2] " Bjorn Helgaas
@ 2019-10-09 23:42   ` Kuppuswamy Sathyanarayanan
  0 siblings, 0 replies; 8+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2019-10-09 23:42 UTC (permalink / raw)
  To: Bjorn Helgaas, David Woodhouse, Joerg Roedel
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu, Bjorn Helgaas


On 10/9/19 3:45 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> When CONFIG_INTEL_IOMMU_SVM=y, iommu_enable_dev_iotlb() calls PRI
> interfaces (pci_reset_pri() and pci_enable_pri()), but those are only
> implemented when CONFIG_PCI_PRI is enabled.
>
> Previously INTEL_IOMMU_SVM selected PCI_PASID but not PCI_PRI, so the state
> of PCI_PRI depended on whether AMD_IOMMU (which selects PCI_PRI) was
> enabled or PCI_PRI was enabled explicitly.
>
> The behavior of iommu_enable_dev_iotlb() should not depend on whether
> AMD_IOMMU is enabled.  Make it predictable by having INTEL_IOMMU_SVM select
> PCI_PRI so iommu_enable_dev_iotlb() always uses the full implementations of
> PRI interfaces.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>

> ---
>   drivers/iommu/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index e3842eabcfdd..b183c9f916b0 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -207,6 +207,7 @@ config INTEL_IOMMU_SVM
>   	bool "Support for Shared Virtual Memory with Intel IOMMU"
>   	depends on INTEL_IOMMU && X86
>   	select PCI_PASID
> +	select PCI_PRI
>   	select MMU_NOTIFIER
>   	help
>   	  Shared Virtual Memory (SVM) provides a facility for devices

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
  2019-10-09 22:45 [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Bjorn Helgaas
  2019-10-09 22:45 ` [PATCH 1/2] " Bjorn Helgaas
  2019-10-09 22:45 ` [PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI Bjorn Helgaas
@ 2019-10-09 23:50 ` Jerry Snitselaar
  2019-10-15 11:46 ` Joerg Roedel
  2019-10-15 21:40 ` Bjorn Helgaas
  4 siblings, 0 replies; 8+ messages in thread
From: Jerry Snitselaar @ 2019-10-09 23:50 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu,
	Bjorn Helgaas, David Woodhouse

On Wed Oct 09 19, Bjorn Helgaas wrote:
>From: Bjorn Helgaas <bhelgaas@google.com>
>
>I think intel-iommu.c depends on CONFIG_AMD_IOMMU in an undesirable way:
>
>When CONFIG_INTEL_IOMMU_SVM=y, iommu_enable_dev_iotlb() calls PRI
>interfaces (pci_reset_pri() and pci_enable_pri()), but those are only
>implemented when CONFIG_PCI_PRI is enabled.  If CONFIG_PCI_PRI is not
>enabled, there are stubs that just return failure.
>
>The INTEL_IOMMU_SVM Kconfig does nothing with PCI_PRI, but AMD_IOMMU
>selects PCI_PRI.  So if AMD_IOMMU is enabled, intel-iommu.c gets the full
>PRI interfaces.  If AMD_IOMMU is not enabled, it gets the PRI stubs.
>
>This seems wrong.  The first patch here makes INTEL_IOMMU_SVM select
>PCI_PRI so intel-iommu.c always gets the full PRI interfaces.
>
>The second patch moves pci_prg_resp_pasid_required(), which simply returns
>a bit from the PCI capability, from #ifdef CONFIG_PCI_PASID to #ifdef
>CONFIG_PCI_PRI.  This is related because INTEL_IOMMU_SVM already *does*
>select PCI_PASID, so it previously always got pci_prg_resp_pasid_required()
>even though it got stubs for other PRI things.
>
>Since these are related and I have several follow-on ATS-related patches in
>the queue, I'd like to take these both via the PCI tree.
>
>Bjorn Helgaas (2):
>  iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
>  PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI
>
> drivers/iommu/Kconfig   |  1 +
> drivers/pci/ats.c       | 55 +++++++++++++++++++----------------------
> include/linux/pci-ats.h | 11 ++++-----
> 3 files changed, 31 insertions(+), 36 deletions(-)
>
>-- 
>2.23.0.581.g78d2f28ef7-goog
>
>_______________________________________________
>iommu mailing list
>iommu@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
  2019-10-09 22:45 [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Bjorn Helgaas
                   ` (2 preceding siblings ...)
  2019-10-09 23:50 ` [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Jerry Snitselaar
@ 2019-10-15 11:46 ` Joerg Roedel
  2019-10-15 21:40 ` Bjorn Helgaas
  4 siblings, 0 replies; 8+ messages in thread
From: Joerg Roedel @ 2019-10-15 11:46 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu,
	Bjorn Helgaas, David Woodhouse

Hey Bjorn,

On Wed, Oct 09, 2019 at 05:45:49PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think intel-iommu.c depends on CONFIG_AMD_IOMMU in an undesirable way:
> 
> When CONFIG_INTEL_IOMMU_SVM=y, iommu_enable_dev_iotlb() calls PRI
> interfaces (pci_reset_pri() and pci_enable_pri()), but those are only
> implemented when CONFIG_PCI_PRI is enabled.  If CONFIG_PCI_PRI is not
> enabled, there are stubs that just return failure.
> 
> The INTEL_IOMMU_SVM Kconfig does nothing with PCI_PRI, but AMD_IOMMU
> selects PCI_PRI.  So if AMD_IOMMU is enabled, intel-iommu.c gets the full
> PRI interfaces.  If AMD_IOMMU is not enabled, it gets the PRI stubs.
> 
> This seems wrong.  The first patch here makes INTEL_IOMMU_SVM select
> PCI_PRI so intel-iommu.c always gets the full PRI interfaces.

Indeed, this is very wrong, thanks for fixing it. Feel free to apply
this series to your tree with my:

Reviewed-by: Joerg Roedel <jroedel@suse.de>
Acked-by: Joerg Roedel <jroedel@suse.de>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
  2019-10-09 22:45 [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Bjorn Helgaas
                   ` (3 preceding siblings ...)
  2019-10-15 11:46 ` Joerg Roedel
@ 2019-10-15 21:40 ` Bjorn Helgaas
  4 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2019-10-15 21:40 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan, David Woodhouse, Joerg Roedel
  Cc: Ashok Raj, linux-pci, linux-kernel, Keith Busch, iommu

[+cc Jerry]

On Wed, Oct 09, 2019 at 05:45:49PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think intel-iommu.c depends on CONFIG_AMD_IOMMU in an undesirable way:
> 
> When CONFIG_INTEL_IOMMU_SVM=y, iommu_enable_dev_iotlb() calls PRI
> interfaces (pci_reset_pri() and pci_enable_pri()), but those are only
> implemented when CONFIG_PCI_PRI is enabled.  If CONFIG_PCI_PRI is not
> enabled, there are stubs that just return failure.
> 
> The INTEL_IOMMU_SVM Kconfig does nothing with PCI_PRI, but AMD_IOMMU
> selects PCI_PRI.  So if AMD_IOMMU is enabled, intel-iommu.c gets the full
> PRI interfaces.  If AMD_IOMMU is not enabled, it gets the PRI stubs.
> 
> This seems wrong.  The first patch here makes INTEL_IOMMU_SVM select
> PCI_PRI so intel-iommu.c always gets the full PRI interfaces.
> 
> The second patch moves pci_prg_resp_pasid_required(), which simply returns
> a bit from the PCI capability, from #ifdef CONFIG_PCI_PASID to #ifdef
> CONFIG_PCI_PRI.  This is related because INTEL_IOMMU_SVM already *does*
> select PCI_PASID, so it previously always got pci_prg_resp_pasid_required()
> even though it got stubs for other PRI things.
> 
> Since these are related and I have several follow-on ATS-related patches in
> the queue, I'd like to take these both via the PCI tree.
> 
> Bjorn Helgaas (2):
>   iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
>   PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI
> 
>  drivers/iommu/Kconfig   |  1 +
>  drivers/pci/ats.c       | 55 +++++++++++++++++++----------------------
>  include/linux/pci-ats.h | 11 ++++-----
>  3 files changed, 31 insertions(+), 36 deletions(-)

I applied these to pci/virtualization for v5.5 with Kuppuswamy's
and Joerg's Reviewed-by on both and Jerry's on the first.  Thank you
all for checking this over!
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 22:45 [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Bjorn Helgaas
2019-10-09 22:45 ` [PATCH 1/2] " Bjorn Helgaas
2019-10-09 23:42   ` Kuppuswamy Sathyanarayanan
2019-10-09 22:45 ` [PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI Bjorn Helgaas
2019-10-09 22:55   ` Kuppuswamy Sathyanarayanan
2019-10-09 23:50 ` [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM Jerry Snitselaar
2019-10-15 11:46 ` Joerg Roedel
2019-10-15 21:40 ` Bjorn Helgaas

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org iommu@archiver.kernel.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/ public-inbox