From: Bjorn Helgaas <[email protected]>
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
From: Bjorn Helgaas <[email protected]>
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 <[email protected]>
---
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
From: Bjorn Helgaas <[email protected]>
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 <[email protected]>
---
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
On 10/9/19 3:45 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan
<[email protected]>
> ---
> 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
On 10/9/19 3:45 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> 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 <[email protected]>
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan
<[email protected]>
> ---
> 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
On Wed Oct 09 19, Bjorn Helgaas wrote:
>From: Bjorn Helgaas <[email protected]>
>
>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
>[email protected]
>https://lists.linuxfoundation.org/mailman/listinfo/iommu
Reviewed-by: Jerry Snitselaar <[email protected]>
Hey Bjorn,
On Wed, Oct 09, 2019 at 05:45:49PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> 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 <[email protected]>
Acked-by: Joerg Roedel <[email protected]>
[+cc Jerry]
On Wed, Oct 09, 2019 at 05:45:49PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> 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!