Re: [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
On Wed Oct 09 19, Bjorn Helgaas wrote: From: Bjorn Helgaas 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
On 10/9/19 3:45 PM, Bjorn Helgaas wrote: From: Bjorn Helgaas 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 Looks good to me. Reviewed-by: Kuppuswamy Sathyanarayanan --- 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
Re: [PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI
On 10/9/19 3:45 PM, Bjorn Helgaas wrote:
From: Bjorn Helgaas
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
Reviewed-by: Kuppuswamy Sathyanarayanan
---
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, );
+
+ 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, );
-
- if (status & PCI_PRI_STATUS_PASID)
- return 1;
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
-
#define PASID_NUMBER_SHIFT8
#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
[PATCH 1/3] PCI/ATS: Remove unused PRI and PASID stubs
From: Bjorn Helgaas
The following functions are only used by amd_iommu.c and intel-iommu.c
(when CONFIG_INTEL_IOMMU_SVM is enabled). CONFIG_PCI_PRI and
CONFIG_PCI_PASID are always defined in those cases, so there's no need for
the stubs.
pci_enable_pri()
pci_disable_pri()
pci_reset_pri()
pci_prg_resp_pasid_required()
pci_enable_pasid()
pci_disable_pasid()
Remove the unused stubs.
Signed-off-by: Bjorn Helgaas
---
include/linux/pci-ats.h | 10 --
1 file changed, 10 deletions(-)
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 67de3a9499bb..963c11f7c56b 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -27,14 +27,7 @@ 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 */
-static inline int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
-{ return -ENODEV; }
-static inline void pci_disable_pri(struct pci_dev *pdev) { }
static inline void pci_restore_pri_state(struct pci_dev *pdev) { }
-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
@@ -44,9 +37,6 @@ 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);
#else /* CONFIG_PCI_PASID */
-static inline int pci_enable_pasid(struct pci_dev *pdev, int features)
-{ return -EINVAL; }
-static inline void pci_disable_pasid(struct pci_dev *pdev) { }
static inline void pci_restore_pasid_state(struct pci_dev *pdev) { }
static inline int pci_pasid_features(struct pci_dev *pdev)
{ return -EINVAL; }
--
2.23.0.581.g78d2f28ef7-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/3] PCI/ATS: Remove unnecessary EXPORT_SYMBOL_GPL()
From: Bjorn Helgaas
The following functions are only used by the PCI core or by IOMMU drivers
that cannot be modular, so there's no need to export them at all:
pci_enable_ats()
pci_disable_ats()
pci_restore_ats_state()
pci_ats_queue_depth()
pci_ats_page_aligned()
pci_enable_pri()
pci_restore_pri_state()
pci_reset_pri()
pci_prg_resp_pasid_required()
pci_enable_pasid()
pci_disable_pasid()
pci_restore_pasid_state()
pci_pasid_features()
pci_max_pasids()
Remove the unnecessary EXPORT_SYMBOL_GPL()s.
Signed-off-by: Bjorn Helgaas
---
drivers/pci/ats.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 76ae518d55f4..982b46f0a54d 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -69,7 +69,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
dev->ats_enabled = 1;
return 0;
}
-EXPORT_SYMBOL_GPL(pci_enable_ats);
/**
* pci_disable_ats - disable the ATS capability
@@ -88,7 +87,6 @@ void pci_disable_ats(struct pci_dev *dev)
dev->ats_enabled = 0;
}
-EXPORT_SYMBOL_GPL(pci_disable_ats);
void pci_restore_ats_state(struct pci_dev *dev)
{
@@ -102,7 +100,6 @@ void pci_restore_ats_state(struct pci_dev *dev)
ctrl |= PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
}
-EXPORT_SYMBOL_GPL(pci_restore_ats_state);
/**
* pci_ats_queue_depth - query the ATS Invalidate Queue Depth
@@ -129,7 +126,6 @@ int pci_ats_queue_depth(struct pci_dev *dev)
pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, );
return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
}
-EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
/**
* pci_ats_page_aligned - Return Page Aligned Request bit status.
@@ -156,7 +152,6 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
return 0;
}
-EXPORT_SYMBOL_GPL(pci_ats_page_aligned);
#ifdef CONFIG_PCI_PRI
void pci_pri_init(struct pci_dev *pdev)
@@ -218,7 +213,6 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
return 0;
}
-EXPORT_SYMBOL_GPL(pci_enable_pri);
/**
* pci_disable_pri - Disable PRI capability
@@ -271,7 +265,6 @@ void pci_restore_pri_state(struct pci_dev *pdev)
pci_write_config_dword(pdev, pri + PCI_PRI_ALLOC_REQ, reqs);
pci_write_config_word(pdev, pri + PCI_PRI_CTRL, control);
}
-EXPORT_SYMBOL_GPL(pci_restore_pri_state);
/**
* pci_reset_pri - Resets device's PRI state
@@ -299,7 +292,6 @@ 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
@@ -315,7 +307,6 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
return pdev->pasid_required;
}
-EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
#endif /* CONFIG_PCI_PRI */
#ifdef CONFIG_PCI_PASID
@@ -373,7 +364,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
return 0;
}
-EXPORT_SYMBOL_GPL(pci_enable_pasid);
/**
* pci_disable_pasid - Disable the PASID capability
@@ -398,7 +388,6 @@ void pci_disable_pasid(struct pci_dev *pdev)
pdev->pasid_enabled = 0;
}
-EXPORT_SYMBOL_GPL(pci_disable_pasid);
/**
* pci_restore_pasid_state - Restore PASID capabilities
@@ -421,7 +410,6 @@ void pci_restore_pasid_state(struct pci_dev *pdev)
control = PCI_PASID_CTRL_ENABLE | pdev->pasid_features;
pci_write_config_word(pdev, pasid + PCI_PASID_CTRL, control);
}
-EXPORT_SYMBOL_GPL(pci_restore_pasid_state);
/**
* pci_pasid_features - Check which PASID features are supported
@@ -450,7 +438,6 @@ int pci_pasid_features(struct pci_dev *pdev)
return supported;
}
-EXPORT_SYMBOL_GPL(pci_pasid_features);
#define PASID_NUMBER_SHIFT 8
#define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
@@ -478,5 +465,4 @@ int pci_max_pasids(struct pci_dev *pdev)
return (1 << supported);
}
-EXPORT_SYMBOL_GPL(pci_max_pasids);
#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
[PATCH 3/3] PCI/ATS: Make pci_restore_pri_state(), pci_restore_pasid_state() private
From: Bjorn Helgaas
These interfaces:
void pci_restore_pri_state(struct pci_dev *pdev);
void pci_restore_pasid_state(struct pci_dev *pdev);
are only used in drivers/pci and do not need to be seen by the rest of the
kernel. Most them to drivers/pci/pci.h so they're private to the PCI
subsystem.
Signed-off-by: Bjorn Helgaas
---
drivers/pci/pci.h | 4
include/linux/pci-ats.h | 5 -
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ae84d28ba03a..e6b46d2b9846 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -458,14 +458,18 @@ static inline void pci_restore_ats_state(struct pci_dev
*dev) { }
#ifdef CONFIG_PCI_PRI
void pci_pri_init(struct pci_dev *dev);
+void pci_restore_pri_state(struct pci_dev *pdev);
#else
static inline void pci_pri_init(struct pci_dev *dev) { }
+static inline void pci_restore_pri_state(struct pci_dev *pdev) { }
#endif
#ifdef CONFIG_PCI_PASID
void pci_pasid_init(struct pci_dev *dev);
+void pci_restore_pasid_state(struct pci_dev *pdev);
#else
static inline void pci_pasid_init(struct pci_dev *dev) { }
+static inline void pci_restore_pasid_state(struct pci_dev *pdev) { }
#endif
#ifdef CONFIG_PCI_IOV
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 963c11f7c56b..5d62e78946a3 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -23,21 +23,16 @@ static inline int pci_ats_page_aligned(struct pci_dev *dev)
#ifdef CONFIG_PCI_PRI
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 */
-static inline void pci_restore_pri_state(struct pci_dev *pdev) { }
#endif /* CONFIG_PCI_PRI */
#ifdef CONFIG_PCI_PASID
int pci_enable_pasid(struct pci_dev *pdev, int features);
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);
#else /* CONFIG_PCI_PASID */
-static inline void pci_restore_pasid_state(struct pci_dev *pdev) { }
static inline int pci_pasid_features(struct pci_dev *pdev)
{ return -EINVAL; }
static inline int pci_max_pasids(struct pci_dev *pdev)
--
2.23.0.581.g78d2f28ef7-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 0/3] PCI/ATS: Clean up unnecessary stubs and exports
From: Bjorn Helgaas Most of the ATS/PRI/PASID interfaces are only used by IOMMU drivers that can only be built statically, not as modules. A couple are only used by the PCI core and don't need to be visible outside at all. These are intended to be cleanup only, but let me know if they would break something. Bjorn Helgaas (3): PCI/ATS: Remove unused PRI and PASID stubs PCI/ATS: Remove unnecessary EXPORT_SYMBOL_GPL() PCI/ATS: Make pci_restore_pri_state(), pci_restore_pasid_state() private drivers/pci/ats.c | 14 -- drivers/pci/pci.h | 4 include/linux/pci-ats.h | 15 --- 3 files changed, 4 insertions(+), 29 deletions(-) -- 2.23.0.581.g78d2f28ef7-goog
[PATCH 1/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
From: Bjorn Helgaas 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 --- 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
[PATCH 2/2] PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI
From: Bjorn Helgaas
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
---
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, );
+
+ 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, );
-
- 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
[PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
From: Bjorn Helgaas 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
Re: [PATCH 3/3] iommu/ipmmu-vmsa: Add utlb_offset_base
Hi Shimoda-san,
Thanks for your patch.
On 2019-10-09 17:26:49 +0900, Yoshihiro Shimoda wrote:
> Since we will have changed memory mapping of the IPMMU in the future,
> this patch adds a utlb_offset_base into struct ipmmu_features
> for IMUCTR and IMUASID registers.
> No behavior change.
>
> Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Niklas Söderlund
> ---
> drivers/iommu/ipmmu-vmsa.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 76fb250..bc00e58 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -52,6 +52,7 @@ struct ipmmu_features {
> bool cache_snoop;
> u32 ctx_offset_base;
> u32 ctx_offset_stride;
> + u32 utlb_offset_base;
> };
>
> struct ipmmu_vmsa_device {
> @@ -285,6 +286,11 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain
> *domain,
> ipmmu_ctx_write_root(domain, reg, data);
> }
>
> +static u32 ipmmu_utlb_reg(struct ipmmu_vmsa_device *mmu, unsigned int reg)
> +{
> + return mmu->features->utlb_offset_base + reg;
> +}
> +
> /*
> -
> * TLB and microTLB Management
> */
> @@ -330,9 +336,9 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain
> *domain,
>*/
>
> /* TODO: What should we set the ASID to ? */
> - ipmmu_write(mmu, IMUASID(utlb), 0);
> + ipmmu_write(mmu, ipmmu_utlb_reg(mmu, IMUASID(utlb)), 0);
> /* TODO: Do we need to flush the microTLB ? */
> - ipmmu_write(mmu, IMUCTR(utlb),
> + ipmmu_write(mmu, ipmmu_utlb_reg(mmu, IMUCTR(utlb)),
> IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
> IMUCTR_MMUEN);
> mmu->utlb_ctx[utlb] = domain->context_id;
> @@ -346,7 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain
> *domain,
> {
> struct ipmmu_vmsa_device *mmu = domain->mmu;
>
> - ipmmu_write(mmu, IMUCTR(utlb), 0);
> + ipmmu_write(mmu, ipmmu_utlb_reg(mmu, IMUCTR(utlb)), 0);
> mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
> }
>
> @@ -995,6 +1001,7 @@ static const struct ipmmu_features
> ipmmu_features_default = {
> .cache_snoop = true,
> .ctx_offset_base = 0,
> .ctx_offset_stride = 0x40,
> + .utlb_offset_base = 0,
> };
>
> static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
> @@ -1008,6 +1015,7 @@ static const struct ipmmu_features
> ipmmu_features_rcar_gen3 = {
> .cache_snoop = false,
> .ctx_offset_base = 0,
> .ctx_offset_stride = 0x40,
> + .utlb_offset_base = 0,
> };
>
> static const struct of_device_id ipmmu_of_ids[] = {
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH 2/3] iommu/ipmmu-vmsa: Calculate context registers' offset instead of a macro
Hi Shimoda-san,
Thanks for your patch.
On 2019-10-09 17:26:48 +0900, Yoshihiro Shimoda wrote:
> Since we will have changed memory mapping of the IPMMU in the future,
> this patch uses ipmmu_features values instead of a macro to
> calculate context registers offset. No behavior change.
>
> Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Niklas Söderlund
> ---
> drivers/iommu/ipmmu-vmsa.c | 27 +++
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index dd554c2..76fb250 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -50,6 +50,8 @@ struct ipmmu_features {
> bool twobit_imttbcr_sl0;
> bool reserved_context;
> bool cache_snoop;
> + u32 ctx_offset_base;
> + u32 ctx_offset_stride;
> };
>
> struct ipmmu_vmsa_device {
> @@ -99,8 +101,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device
> *dev)
>
> #define IM_NS_ALIAS_OFFSET 0x800
>
> -#define IM_CTX_SIZE 0x40
> -
> #define IMCTR0x
> #define IMCTR_TRE(1 << 17)
> #define IMCTR_AFE(1 << 16)
> @@ -253,18 +253,25 @@ static void ipmmu_write(struct ipmmu_vmsa_device *mmu,
> unsigned int offset,
> iowrite32(data, mmu->base + offset);
> }
>
> +static u32 ipmmu_ctx_reg(struct ipmmu_vmsa_device *mmu, unsigned int
> context_id,
> + unsigned int reg)
> +{
> + return mmu->features->ctx_offset_base +
> +context_id * mmu->features->ctx_offset_stride + reg;
> +}
> +
> static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain,
> unsigned int reg)
> {
> return ipmmu_read(domain->mmu->root,
> - domain->context_id * IM_CTX_SIZE + reg);
> + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg));
> }
>
> static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain,
>unsigned int reg, u32 data)
> {
> ipmmu_write(domain->mmu->root,
> - domain->context_id * IM_CTX_SIZE + reg, data);
> + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), data);
> }
>
> static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain,
> @@ -272,10 +279,10 @@ static void ipmmu_ctx_write_all(struct
> ipmmu_vmsa_domain *domain,
> {
> if (domain->mmu != domain->mmu->root)
> ipmmu_write(domain->mmu,
> - domain->context_id * IM_CTX_SIZE + reg, data);
> + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg),
> + data);
>
> - ipmmu_write(domain->mmu->root,
> - domain->context_id * IM_CTX_SIZE + reg, data);
> + ipmmu_ctx_write_root(domain, reg, data);
> }
>
> /*
> -
> @@ -974,7 +981,7 @@ static void ipmmu_device_reset(struct ipmmu_vmsa_device
> *mmu)
>
> /* Disable all contexts. */
> for (i = 0; i < mmu->num_ctx; ++i)
> - ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
> + ipmmu_write(mmu, ipmmu_ctx_reg(mmu, i, IMCTR), 0);
> }
>
> static const struct ipmmu_features ipmmu_features_default = {
> @@ -986,6 +993,8 @@ static const struct ipmmu_features ipmmu_features_default
> = {
> .twobit_imttbcr_sl0 = false,
> .reserved_context = false,
> .cache_snoop = true,
> + .ctx_offset_base = 0,
> + .ctx_offset_stride = 0x40,
> };
>
> static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
> @@ -997,6 +1006,8 @@ static const struct ipmmu_features
> ipmmu_features_rcar_gen3 = {
> .twobit_imttbcr_sl0 = true,
> .reserved_context = true,
> .cache_snoop = false,
> + .ctx_offset_base = 0,
> + .ctx_offset_stride = 0x40,
> };
>
> static const struct of_device_id ipmmu_of_ids[] = {
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH 1/3] iommu/ipmmu-vmsa: Remove some unused register declarations
Hi Shimoda-san, Thanks for your patch. On 2019-10-09 17:26:47 +0900, Yoshihiro Shimoda wrote: > To support different registers memory mapping hardware easily > in the future, this patch removes some unused register > declarations. > > Signed-off-by: Yoshihiro Shimoda Reviewed-by: Niklas Söderlund > --- > drivers/iommu/ipmmu-vmsa.c | 11 --- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c > index 9da8309..dd554c2 100644 > --- a/drivers/iommu/ipmmu-vmsa.c > +++ b/drivers/iommu/ipmmu-vmsa.c > @@ -104,8 +104,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device > *dev) > #define IMCTR0x > #define IMCTR_TRE(1 << 17) > #define IMCTR_AFE(1 << 16) > -#define IMCTR_RTSEL_MASK (3 << 4) > -#define IMCTR_RTSEL_SHIFT4 > #define IMCTR_TREN (1 << 3) > #define IMCTR_INTEN (1 << 2) > #define IMCTR_FLUSH (1 << 1) > @@ -115,7 +113,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device > *dev) > > #define IMTTBCR 0x0008 > #define IMTTBCR_EAE (1 << 31) > -#define IMTTBCR_PMB (1 << 30) > #define IMTTBCR_SH1_NON_SHAREABLE(0 << 28) /* R-Car Gen2 only */ > #define IMTTBCR_SH1_OUTER_SHAREABLE (2 << 28) /* R-Car Gen2 only */ > #define IMTTBCR_SH1_INNER_SHAREABLE (3 << 28) /* R-Car Gen2 only */ > @@ -193,12 +190,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device > *dev) > #define IMELAR 0x0030 /* IMEAR on R-Car Gen2 > */ > #define IMEUAR 0x0034 /* R-Car Gen3 only */ > > -#define IMPCTR 0x0200 > -#define IMPSTR 0x0208 > -#define IMPEAR 0x020c > -#define IMPMBA(n)(0x0280 + ((n) * 4)) > -#define IMPMBD(n)(0x02c0 + ((n) * 4)) > - > #define IMUCTR(n)((n) < 32 ? IMUCTR0(n) : IMUCTR32(n)) > #define IMUCTR0(n) (0x0300 + ((n) * 16)) > #define IMUCTR32(n) (0x0600 + (((n) - 32) * 16)) > @@ -206,8 +197,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device > *dev) > #define IMUCTR_FIXADD_MASK (0xff << 16) > #define IMUCTR_FIXADD_SHIFT 16 > #define IMUCTR_TTSEL_MMU(n) ((n) << 4) > -#define IMUCTR_TTSEL_PMB (8 << 4) > -#define IMUCTR_TTSEL_MASK(15 << 4) > #define IMUCTR_FLUSH (1 << 1) > #define IMUCTR_MMUEN (1 << 0) > > -- > 2.7.4 > -- Regards, Niklas Söderlund
Re: [PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()
On 2019-10-09 12:57 a.m., Christoph Hellwig wrote:
> On Tue, Oct 08, 2019 at 04:18:35PM -0600, Logan Gunthorpe wrote:
>> From: Kit Chow
>>
>> Currently the Intel IOMMU uses the default dma_[un]map_resource()
>
> s/Intel/AMD/ ?
Oops, yes, my mistake.
>> +static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
>> +size_t size, enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +struct protection_domain *domain;
>> +struct dma_ops_domain *dma_dom;
>> +
>> +domain = get_domain(dev);
>> +if (PTR_ERR(domain) == -EINVAL)
>> +return (dma_addr_t)paddr;
>
> I thought that case can't happen anymore?
>
> Also note that Joerg just applied the patch to convert the AMD iommu
> driver to use the dma-iommu ops. Can you test that series and check
> it does the right thing for your use case? From looking at the code
> I think it should.
Yes, looking at the new code, it looks like this patch will not be
needed. So we can drop it. We'll test it to make sure.
I believe the other two patches in this series are still needed though.
Thanks,
Logan
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/mediatek: Move the tlb_sync into tlb_flush
On Wed, Oct 9, 2019 at 10:38 PM Yong Wu wrote:
>
> On Wed, 2019-10-09 at 16:56 +0900, Tomasz Figa wrote:
> > On Tue, Oct 8, 2019 at 5:09 PM Yong Wu wrote:
> > >
> > > Hi Tomasz,
> > >
> > > Sorry for reply late.
> > >
> > > On Wed, 2019-10-02 at 14:18 +0900, Tomasz Figa wrote:
> > > > Hi Yong,
> > > >
> > > > On Mon, Sep 30, 2019 at 2:42 PM Yong Wu wrote:
> > > > >
> > > > > The commit 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU
> > > > > API
> > > > > TLB sync") help move the tlb_sync of unmap from v7s into the iommu
> > > > > framework. It helps add a new function "mtk_iommu_iotlb_sync", But it
> > > > > lacked the dom->pgtlock, then it will cause the variable
> > > > > "tlb_flush_active" may be changed unexpectedly, we could see this
> > > > > warning
> > > > > log randomly:
> > > > >
> > > >
> > > > Thanks for the patch! Please see my comments inline.
> > > >
> > > > > mtk-iommu 10205000.iommu: Partial TLB flush timed out, falling back to
> > > > > full flush
> > > > >
> > > > > To fix this issue, we can add dom->pgtlock in the
> > > > > "mtk_iommu_iotlb_sync".
> > > > > And when checking this issue, we find that __arm_v7s_unmap call
> > > > > io_pgtable_tlb_add_flush consecutively when it is
> > > > > supersection/largepage,
> > > > > this also is potential unsafe for us. There is no tlb flush queue in
> > > > > the
> > > > > MediaTek M4U HW. The HW always expect the tlb_flush/tlb_sync one by
> > > > > one.
> > > > > If v7s don't always gurarantee the sequence, Thus, In this patch I
> > > > > move
> > > > > the tlb_sync into tlb_flush(also rename the function deleting
> > > > > "_nosync").
> > > > > and we don't care if it is leaf, rearrange the callback functions.
> > > > > Also,
> > > > > the tlb flush/sync was already finished in v7s, then iotlb_sync and
> > > > > iotlb_sync_all is unnecessary.
> > > >
> > > > Performance-wise, we could do much better. Instead of synchronously
> > > > syncing at the end of mtk_iommu_tlb_add_flush(), we could sync at the
> > > > beginning, if there was any previous flush still pending. We would
> > > > also have to keep the .iotlb_sync() callback, to take care of waiting
> > > > for the last flush. That would allow better pipelining with CPU in
> > > > cases like this:
> > > >
> > > > for (all pages in range) {
> > > >change page table();
> > > >flush();
> > > > }
> > > >
> > > > "change page table()" could execute while the IOMMU is flushing the
> > > > previous change.
> > >
> > > Do you mean adding a new tlb_sync before tlb_flush_no_sync, like below:
> > >
> > > mtk_iommu_tlb_add_flush_nosync {
> > >+ mtk_iommu_tlb_sync();
> > >tlb_flush_no_sync();
> > >data->tlb_flush_active = true;
> > > }
> > >
> > > mtk_iommu_tlb_sync {
> > > if (!data->tlb_flush_active)
> > > return;
> > > tlb_sync();
> > > data->tlb_flush_active = false;
> > > }
> > >
> > > This way look improve the flow, But adjusting the flow is not the root
> > > cause of this issue. the problem is "data->tlb_flush_active" may be
> > > changed from mtk_iommu_iotlb_sync which don't have a dom->pglock.
> >
> > That was not the only problem with existing code. Existing code also
> > assumed that add_flush and sync always go in pairs, but that's not
> > true.
>
> Yes. Thus I put the tlb_flush always followed by tlb_sync to make sure
> they always go in pairs.
>
> >
> > My suggestion is to fix the locking in the driver and keep the sync
> > deferred as much as possible, so that performance is not degraded. I
>
> I really didn't get this timeout warning log in previous kernel(Many
> tlb_flush followed by one tlb_sync),
Locking issues typically lead to timing problems (race conditions), so
it might just be that the sequence or timing of calls changed between
kernel versions, enough to trigger the issue.
> But deferring the sync is not
> suggested by our DE, thus I still would like to fix the sequence in this
> patch with putting them together.
>
What's the reason it's not suggested? From my understanding, there
shouldn't be any dependency on hardware design here, it's just a
simple software optimization - we can pipeline other CPU operations
while the IOMMU is flushing the TLB.
Basically, right now:
CPU writes page tables 1
IOMMU flushes 1 | CPU busy waits
CPU writes page tables 2
IOMMU flushes 2 | CPU busy waits
CPU writes page tables 3
IOMMU flushes 3 | CPU busy waits
...
With my suggestion that could be:
CPU writes page tables 1 I
IOMMU flushes 1 | CPU writes page tables 2
IOMMU flushes 1 | CPU busy waits less time
IOMMU flushes 2 | CPU writes page tables 3
IOMMU flushes 2 | CPU busy waits less time
IOMMU flushes 3 | CPU busy waits
It reduces the time the CPU spends on busy waiting rather than doing
something useful. It also reduces the total time of maps and unmaps.
Actually, we can optimize even more. Please consider the case below.
CPU writes PTE for IOVA 0x1000.
IOMMU flushes 0x1000 | CPU busy waits
CPU
Re: [PATCH] iommu/mediatek: Move the tlb_sync into tlb_flush
On Wed, 2019-10-09 at 16:56 +0900, Tomasz Figa wrote:
> On Tue, Oct 8, 2019 at 5:09 PM Yong Wu wrote:
> >
> > Hi Tomasz,
> >
> > Sorry for reply late.
> >
> > On Wed, 2019-10-02 at 14:18 +0900, Tomasz Figa wrote:
> > > Hi Yong,
> > >
> > > On Mon, Sep 30, 2019 at 2:42 PM Yong Wu wrote:
> > > >
> > > > The commit 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API
> > > > TLB sync") help move the tlb_sync of unmap from v7s into the iommu
> > > > framework. It helps add a new function "mtk_iommu_iotlb_sync", But it
> > > > lacked the dom->pgtlock, then it will cause the variable
> > > > "tlb_flush_active" may be changed unexpectedly, we could see this
> > > > warning
> > > > log randomly:
> > > >
> > >
> > > Thanks for the patch! Please see my comments inline.
> > >
> > > > mtk-iommu 10205000.iommu: Partial TLB flush timed out, falling back to
> > > > full flush
> > > >
> > > > To fix this issue, we can add dom->pgtlock in the
> > > > "mtk_iommu_iotlb_sync".
> > > > And when checking this issue, we find that __arm_v7s_unmap call
> > > > io_pgtable_tlb_add_flush consecutively when it is
> > > > supersection/largepage,
> > > > this also is potential unsafe for us. There is no tlb flush queue in the
> > > > MediaTek M4U HW. The HW always expect the tlb_flush/tlb_sync one by one.
> > > > If v7s don't always gurarantee the sequence, Thus, In this patch I move
> > > > the tlb_sync into tlb_flush(also rename the function deleting
> > > > "_nosync").
> > > > and we don't care if it is leaf, rearrange the callback functions. Also,
> > > > the tlb flush/sync was already finished in v7s, then iotlb_sync and
> > > > iotlb_sync_all is unnecessary.
> > >
> > > Performance-wise, we could do much better. Instead of synchronously
> > > syncing at the end of mtk_iommu_tlb_add_flush(), we could sync at the
> > > beginning, if there was any previous flush still pending. We would
> > > also have to keep the .iotlb_sync() callback, to take care of waiting
> > > for the last flush. That would allow better pipelining with CPU in
> > > cases like this:
> > >
> > > for (all pages in range) {
> > >change page table();
> > >flush();
> > > }
> > >
> > > "change page table()" could execute while the IOMMU is flushing the
> > > previous change.
> >
> > Do you mean adding a new tlb_sync before tlb_flush_no_sync, like below:
> >
> > mtk_iommu_tlb_add_flush_nosync {
> >+ mtk_iommu_tlb_sync();
> >tlb_flush_no_sync();
> >data->tlb_flush_active = true;
> > }
> >
> > mtk_iommu_tlb_sync {
> > if (!data->tlb_flush_active)
> > return;
> > tlb_sync();
> > data->tlb_flush_active = false;
> > }
> >
> > This way look improve the flow, But adjusting the flow is not the root
> > cause of this issue. the problem is "data->tlb_flush_active" may be
> > changed from mtk_iommu_iotlb_sync which don't have a dom->pglock.
>
> That was not the only problem with existing code. Existing code also
> assumed that add_flush and sync always go in pairs, but that's not
> true.
Yes. Thus I put the tlb_flush always followed by tlb_sync to make sure
they always go in pairs.
>
> My suggestion is to fix the locking in the driver and keep the sync
> deferred as much as possible, so that performance is not degraded. I
I really didn't get this timeout warning log in previous kernel(Many
tlb_flush followed by one tlb_sync), But deferring the sync is not
suggested by our DE, thus I still would like to fix the sequence in this
patch with putting them together.
> changed my mind, though. I think we would need to make more changes to
> the driver to make it implement the flushing efficiently, so let's go
> with the current simple approach for now and improve incrementally.
>
> >
> > Currently the synchronisation of the tlb_flush/tlb_sync flow are
> > controlled by the variable "data->tlb_flush_active".
> >
> > In this patch putting the tlb_flush/tlb_sync together looks make
> > the flow simpler:
> > a) Don't need the sensitive variable "tlb_flush_active".
> > b) Remove mtk_iommu_iotlb_sync, Don't need add lock in it.
> > c) Simplify the tlb_flush_walk/tlb_flush_leaf.
> > is it ok?
> >
>
> Okay, let's do so as a first step to fix the issue. Then we can
> optimize in follow up patches.
Thanks the confirm, I have sent a quick v2.
>
> > >
> > > >
> > > > Besides, there are two minor changes:
> > > > a) Use writel for the register F_MMU_INV_RANGE which is for triggering
> > > > the
> > > > HW work. We expect all the setting(iova_start/iova_end...) have already
> > > > been finished before F_MMU_INV_RANGE.
> > > > b) Reduce the tlb timeout value from 10us to 1000us. the original
> > > > value
> > > > is so long that affect the multimedia performance.
> > >
> > > By definition, timeout is something that should not normally happen.
> > > Too long timeout affecting multimedia performance would suggest that
> > > the timeout was actually happening, which is the core problem, not the
> > > length of
[PATCH v2 4/4] iommu/mediatek: Reduce the tlb flush timeout value
Reduce the tlb timeout value from 10us to 1000us. the original value
is so long that affect the multimedia performance. This is only a minor
improvement rather than fixing a issue.
Signed-off-by: Yong Wu
---
drivers/iommu/mtk_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 607f92c..1e9ff25 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -195,7 +195,7 @@ static void mtk_iommu_tlb_add_flush(unsigned long iova,
size_t size,
* follows tlb_flush to avoid break the sequence.
*/
ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
- tmp, tmp != 0, 10, 10);
+ tmp, tmp != 0, 10, 1000);
if (ret) {
dev_warn(data->dev,
"Partial TLB flush timed out, falling back to
full flush\n");
--
1.9.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/4] iommu/mediatek: Use writel for TLB range invalidation
Use writel for the register F_MMU_INV_RANGE which is for triggering the HW work. We expect all the setting(iova_start/iova_end...) have already been finished before F_MMU_INV_RANGE. Signed-off-by: Anan.Sun Signed-off-by: Yong Wu --- This is a improvement rather than fixing a issue. --- drivers/iommu/mtk_iommu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c index 24a13a6..607f92c 100644 --- a/drivers/iommu/mtk_iommu.c +++ b/drivers/iommu/mtk_iommu.c @@ -187,8 +187,7 @@ static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size, writel_relaxed(iova, data->base + REG_MMU_INVLD_START_A); writel_relaxed(iova + size - 1, data->base + REG_MMU_INVLD_END_A); - writel_relaxed(F_MMU_INV_RANGE, - data->base + REG_MMU_INVALIDATE); + writel(F_MMU_INV_RANGE, data->base + REG_MMU_INVALIDATE); /* * There is no tlb flush queue in the HW, the HW always expect -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/4] iommu/mediatek: Move the tlb_sync into tlb_flush
The commit 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API
TLB sync") help move the tlb_sync of unmap from v7s into the iommu
framework. It helps add a new function "mtk_iommu_iotlb_sync", But it
lacked the dom->pgtlock, then it will cause the variable
"tlb_flush_active" may be changed unexpectedly, we could see this warning
log randomly:
mtk-iommu 10205000.iommu: Partial TLB flush timed out, falling back to
full flush
To fix this issue, we can add dom->pgtlock in the "mtk_iommu_iotlb_sync".
In addition, when checking this issue, we find that __arm_v7s_unmap call
io_pgtable_tlb_add_flush consecutively when it is supersection/largepage,
this also is potential unsafe for us. There is no tlb flush queue in the
MediaTek M4U HW. The HW always expect the tlb_flush/tlb_sync one by one.
If v7s don't always gurarantee the sequence, Thus, In this patch I move
the tlb_sync into tlb_flush(also rename the function deleting "_nosync").
and we don't care if it is leaf, rearrange the callback functions. Also,
the tlb flush/sync was already finished in v7s, then iotlb_sync is
unnecessary.
Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB sync")
Signed-off-by: Chao Hao
Signed-off-by: Yong Wu
---
drivers/iommu/mtk_iommu.c | 63 +--
drivers/iommu/mtk_iommu.h | 1 -
2 files changed, 17 insertions(+), 47 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 76b9388..24a13a6 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -173,11 +173,12 @@ static void mtk_iommu_tlb_flush_all(void *cookie)
}
}
-static void mtk_iommu_tlb_add_flush_nosync(unsigned long iova, size_t size,
- size_t granule, bool leaf,
- void *cookie)
+static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size,
+ size_t granule, void *cookie)
{
struct mtk_iommu_data *data = cookie;
+ int ret;
+ u32 tmp;
for_each_m4u(data) {
writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0,
@@ -188,21 +189,12 @@ static void mtk_iommu_tlb_add_flush_nosync(unsigned long
iova, size_t size,
data->base + REG_MMU_INVLD_END_A);
writel_relaxed(F_MMU_INV_RANGE,
data->base + REG_MMU_INVALIDATE);
- data->tlb_flush_active = true;
- }
-}
-
-static void mtk_iommu_tlb_sync(void *cookie)
-{
- struct mtk_iommu_data *data = cookie;
- int ret;
- u32 tmp;
-
- for_each_m4u(data) {
- /* Avoid timing out if there's nothing to wait for */
- if (!data->tlb_flush_active)
- return;
+ /*
+* There is no tlb flush queue in the HW, the HW always expect
+* tlb_flush and tlb_sync in pair strictly. Here tlb_sync always
+* follows tlb_flush to avoid break the sequence.
+*/
ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
tmp, tmp != 0, 10, 10);
if (ret) {
@@ -212,36 +204,21 @@ static void mtk_iommu_tlb_sync(void *cookie)
}
/* Clear the CPE status */
writel_relaxed(0, data->base + REG_MMU_CPE_DONE);
- data->tlb_flush_active = false;
}
}
-static void mtk_iommu_tlb_flush_walk(unsigned long iova, size_t size,
-size_t granule, void *cookie)
-{
- mtk_iommu_tlb_add_flush_nosync(iova, size, granule, false, cookie);
- mtk_iommu_tlb_sync(cookie);
-}
-
-static void mtk_iommu_tlb_flush_leaf(unsigned long iova, size_t size,
-size_t granule, void *cookie)
+static void mtk_iommu_tlb_flush_page(struct iommu_iotlb_gather *gather,
+unsigned long iova, size_t granule,
+void *cookie)
{
- mtk_iommu_tlb_add_flush_nosync(iova, size, granule, true, cookie);
- mtk_iommu_tlb_sync(cookie);
-}
-
-static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
- unsigned long iova, size_t granule,
- void *cookie)
-{
- mtk_iommu_tlb_add_flush_nosync(iova, granule, granule, true, cookie);
+ mtk_iommu_tlb_add_flush(iova, granule, granule, cookie);
}
static const struct iommu_flush_ops mtk_iommu_flush_ops = {
.tlb_flush_all = mtk_iommu_tlb_flush_all,
- .tlb_flush_walk = mtk_iommu_tlb_flush_walk,
- .tlb_flush_leaf = mtk_iommu_tlb_flush_leaf,
- .tlb_add_page = mtk_iommu_tlb_flush_page_nosync,
+ .tlb_flush_walk = mtk_iommu_tlb_add_flush,
+ .tlb_flush_leaf = mtk_iommu_tlb_add_flush,
+ .tlb_add_page =
[PATCH v2 1/4] iommu/mediatek: Correct the flush_iotlb_all callback
Use the correct tlb_flush_all instead of the original one.
Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB sync")
Signed-off-by: Yong Wu
---
1. rebase on v5.4-rc1
2. v1:
https://lore.kernel.org/linux-iommu/CAAFQd5C3U7pZo4SSUJ52Q7E+0FaUoORQFbQC5RhCHBhi=nf...@mail.gmail.com/T/#t
---
drivers/iommu/mtk_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 67a483c..76b9388 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -447,7 +447,7 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
{
- mtk_iommu_tlb_sync(mtk_iommu_get_m4u_data());
+ mtk_iommu_tlb_flush_all(mtk_iommu_get_m4u_data());
}
static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
--
1.9.1
[PATCH] memory: mtk-smi: Add PM suspend and resume ops
In the commit 4f0a1a1ae351 ("memory: mtk-smi: Invoke pm runtime_callback
to enable clocks"), we use pm_runtime callback to enable/disable the smi
larb clocks. It will cause the larb's clock may not be disabled when
suspend. That is because device_prepare will call pm_runtime_get_noresume
which will keep the larb's PM runtime status still is active when suspend,
then it won't enter our pm_runtime suspend callback to disable the
corresponding clocks.
This patch adds suspend pm_ops to force disable the clocks, Use "LATE" to
make sure it disable the larb's clocks after the multimedia devices.
Fixes: 4f0a1a1ae351 ("memory: mtk-smi: Invoke pm runtime_callback to enable
clocks")
Signed-off-by: Anan Sun
Signed-off-by: Yong Wu
---
base on v5.4-rc1.
---
drivers/memory/mtk-smi.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index 439d7d8..a113e81 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -366,6 +366,8 @@ static int __maybe_unused mtk_smi_larb_suspend(struct
device *dev)
static const struct dev_pm_ops smi_larb_pm_ops = {
SET_RUNTIME_PM_OPS(mtk_smi_larb_suspend, mtk_smi_larb_resume, NULL)
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+pm_runtime_force_resume)
};
static struct platform_driver mtk_smi_larb_driver = {
@@ -507,6 +509,8 @@ static int __maybe_unused mtk_smi_common_suspend(struct
device *dev)
static const struct dev_pm_ops smi_common_pm_ops = {
SET_RUNTIME_PM_OPS(mtk_smi_common_suspend, mtk_smi_common_resume, NULL)
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+pm_runtime_force_resume)
};
static struct platform_driver mtk_smi_common_driver = {
--
1.9.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu: fix "hang" when games exit
On 2019-10-07 9:49 pm, Rob Clark wrote:
From: Rob Clark
When games, browser, or anything using a lot of GPU buffers exits, there
can be many hundreds or thousands of buffers to unmap and free. If the
GPU is otherwise suspended, this can cause arm-smmu to resume/suspend
for each buffer, resulting 5-10 seconds worth of reprogramming the
context bank (arm_smmu_write_context_bank()/arm_smmu_write_s2cr()/etc).
To the user it would appear that the system just locked up.
A simple solution is to use pm_runtime_put_autosuspend() instead, so we
don't immediately suspend the SMMU device.
Reviewed-by: Robin Murphy
Signed-off-by: Rob Clark
---
v1: original
v2: unconditionally use autosuspend, rather than deciding based on what
consumer does
drivers/iommu/arm-smmu.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 3f1d55fb43c4..b7b41f5001bc 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -289,7 +289,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device
*smmu)
static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
{
if (pm_runtime_enabled(smmu->dev))
- pm_runtime_put(smmu->dev);
+ pm_runtime_put_autosuspend(smmu->dev);
}
static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
@@ -1445,6 +1445,9 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
/* Looks ok, so add the device to the domain */
ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
+ pm_runtime_set_autosuspend_delay(smmu->dev, 20);
+ pm_runtime_use_autosuspend(smmu->dev);
+
rpm_put:
arm_smmu_rpm_put(smmu);
return ret;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/3] iommu/ipmmu-vmsa: Calculate context registers' offset instead of a macro
Since we will have changed memory mapping of the IPMMU in the future,
this patch uses ipmmu_features values instead of a macro to
calculate context registers offset. No behavior change.
Signed-off-by: Yoshihiro Shimoda
---
drivers/iommu/ipmmu-vmsa.c | 27 +++
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index dd554c2..76fb250 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -50,6 +50,8 @@ struct ipmmu_features {
bool twobit_imttbcr_sl0;
bool reserved_context;
bool cache_snoop;
+ u32 ctx_offset_base;
+ u32 ctx_offset_stride;
};
struct ipmmu_vmsa_device {
@@ -99,8 +101,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
#define IM_NS_ALIAS_OFFSET 0x800
-#define IM_CTX_SIZE0x40
-
#define IMCTR 0x
#define IMCTR_TRE (1 << 17)
#define IMCTR_AFE (1 << 16)
@@ -253,18 +253,25 @@ static void ipmmu_write(struct ipmmu_vmsa_device *mmu,
unsigned int offset,
iowrite32(data, mmu->base + offset);
}
+static u32 ipmmu_ctx_reg(struct ipmmu_vmsa_device *mmu, unsigned int
context_id,
+unsigned int reg)
+{
+ return mmu->features->ctx_offset_base +
+ context_id * mmu->features->ctx_offset_stride + reg;
+}
+
static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain,
unsigned int reg)
{
return ipmmu_read(domain->mmu->root,
- domain->context_id * IM_CTX_SIZE + reg);
+ ipmmu_ctx_reg(domain->mmu, domain->context_id, reg));
}
static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain,
unsigned int reg, u32 data)
{
ipmmu_write(domain->mmu->root,
- domain->context_id * IM_CTX_SIZE + reg, data);
+ ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), data);
}
static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain,
@@ -272,10 +279,10 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain
*domain,
{
if (domain->mmu != domain->mmu->root)
ipmmu_write(domain->mmu,
- domain->context_id * IM_CTX_SIZE + reg, data);
+ ipmmu_ctx_reg(domain->mmu, domain->context_id, reg),
+ data);
- ipmmu_write(domain->mmu->root,
- domain->context_id * IM_CTX_SIZE + reg, data);
+ ipmmu_ctx_write_root(domain, reg, data);
}
/*
-
@@ -974,7 +981,7 @@ static void ipmmu_device_reset(struct ipmmu_vmsa_device
*mmu)
/* Disable all contexts. */
for (i = 0; i < mmu->num_ctx; ++i)
- ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
+ ipmmu_write(mmu, ipmmu_ctx_reg(mmu, i, IMCTR), 0);
}
static const struct ipmmu_features ipmmu_features_default = {
@@ -986,6 +993,8 @@ static const struct ipmmu_features ipmmu_features_default =
{
.twobit_imttbcr_sl0 = false,
.reserved_context = false,
.cache_snoop = true,
+ .ctx_offset_base = 0,
+ .ctx_offset_stride = 0x40,
};
static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
@@ -997,6 +1006,8 @@ static const struct ipmmu_features
ipmmu_features_rcar_gen3 = {
.twobit_imttbcr_sl0 = true,
.reserved_context = true,
.cache_snoop = false,
+ .ctx_offset_base = 0,
+ .ctx_offset_stride = 0x40,
};
static const struct of_device_id ipmmu_of_ids[] = {
--
2.7.4
[PATCH 1/3] iommu/ipmmu-vmsa: Remove some unused register declarations
To support different registers memory mapping hardware easily in the future, this patch removes some unused register declarations. Signed-off-by: Yoshihiro Shimoda --- drivers/iommu/ipmmu-vmsa.c | 11 --- 1 file changed, 11 deletions(-) diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c index 9da8309..dd554c2 100644 --- a/drivers/iommu/ipmmu-vmsa.c +++ b/drivers/iommu/ipmmu-vmsa.c @@ -104,8 +104,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev) #define IMCTR 0x #define IMCTR_TRE (1 << 17) #define IMCTR_AFE (1 << 16) -#define IMCTR_RTSEL_MASK (3 << 4) -#define IMCTR_RTSEL_SHIFT 4 #define IMCTR_TREN (1 << 3) #define IMCTR_INTEN(1 << 2) #define IMCTR_FLUSH(1 << 1) @@ -115,7 +113,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev) #define IMTTBCR0x0008 #define IMTTBCR_EAE(1 << 31) -#define IMTTBCR_PMB(1 << 30) #define IMTTBCR_SH1_NON_SHAREABLE (0 << 28) /* R-Car Gen2 only */ #define IMTTBCR_SH1_OUTER_SHAREABLE(2 << 28) /* R-Car Gen2 only */ #define IMTTBCR_SH1_INNER_SHAREABLE(3 << 28) /* R-Car Gen2 only */ @@ -193,12 +190,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev) #define IMELAR 0x0030 /* IMEAR on R-Car Gen2 */ #define IMEUAR 0x0034 /* R-Car Gen3 only */ -#define IMPCTR 0x0200 -#define IMPSTR 0x0208 -#define IMPEAR 0x020c -#define IMPMBA(n) (0x0280 + ((n) * 4)) -#define IMPMBD(n) (0x02c0 + ((n) * 4)) - #define IMUCTR(n) ((n) < 32 ? IMUCTR0(n) : IMUCTR32(n)) #define IMUCTR0(n) (0x0300 + ((n) * 16)) #define IMUCTR32(n)(0x0600 + (((n) - 32) * 16)) @@ -206,8 +197,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev) #define IMUCTR_FIXADD_MASK (0xff << 16) #define IMUCTR_FIXADD_SHIFT16 #define IMUCTR_TTSEL_MMU(n)((n) << 4) -#define IMUCTR_TTSEL_PMB (8 << 4) -#define IMUCTR_TTSEL_MASK (15 << 4) #define IMUCTR_FLUSH (1 << 1) #define IMUCTR_MMUEN (1 << 0) -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/3] iommu/ipmmu-vmsa: Add utlb_offset_base
Since we will have changed memory mapping of the IPMMU in the future,
this patch adds a utlb_offset_base into struct ipmmu_features
for IMUCTR and IMUASID registers.
No behavior change.
Signed-off-by: Yoshihiro Shimoda
---
drivers/iommu/ipmmu-vmsa.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 76fb250..bc00e58 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -52,6 +52,7 @@ struct ipmmu_features {
bool cache_snoop;
u32 ctx_offset_base;
u32 ctx_offset_stride;
+ u32 utlb_offset_base;
};
struct ipmmu_vmsa_device {
@@ -285,6 +286,11 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain
*domain,
ipmmu_ctx_write_root(domain, reg, data);
}
+static u32 ipmmu_utlb_reg(struct ipmmu_vmsa_device *mmu, unsigned int reg)
+{
+ return mmu->features->utlb_offset_base + reg;
+}
+
/*
-
* TLB and microTLB Management
*/
@@ -330,9 +336,9 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain
*domain,
*/
/* TODO: What should we set the ASID to ? */
- ipmmu_write(mmu, IMUASID(utlb), 0);
+ ipmmu_write(mmu, ipmmu_utlb_reg(mmu, IMUASID(utlb)), 0);
/* TODO: Do we need to flush the microTLB ? */
- ipmmu_write(mmu, IMUCTR(utlb),
+ ipmmu_write(mmu, ipmmu_utlb_reg(mmu, IMUCTR(utlb)),
IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
IMUCTR_MMUEN);
mmu->utlb_ctx[utlb] = domain->context_id;
@@ -346,7 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain
*domain,
{
struct ipmmu_vmsa_device *mmu = domain->mmu;
- ipmmu_write(mmu, IMUCTR(utlb), 0);
+ ipmmu_write(mmu, ipmmu_utlb_reg(mmu, IMUCTR(utlb)), 0);
mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
}
@@ -995,6 +1001,7 @@ static const struct ipmmu_features ipmmu_features_default
= {
.cache_snoop = true,
.ctx_offset_base = 0,
.ctx_offset_stride = 0x40,
+ .utlb_offset_base = 0,
};
static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
@@ -1008,6 +1015,7 @@ static const struct ipmmu_features
ipmmu_features_rcar_gen3 = {
.cache_snoop = false,
.ctx_offset_base = 0,
.ctx_offset_stride = 0x40,
+ .utlb_offset_base = 0,
};
static const struct of_device_id ipmmu_of_ids[] = {
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 0/3] iommu/ipmmu-vmsa: minor updates
This patch series is based on the latest iommu.git / next branch to modify the driver in the future's new hardware. Yoshihiro Shimoda (3): iommu/ipmmu-vmsa: Remove some unused register declarations iommu/ipmmu-vmsa: Calculate context registers' offset instead of a macro iommu/ipmmu-vmsa: Add utlb_offset_base drivers/iommu/ipmmu-vmsa.c | 52 ++ 1 file changed, 30 insertions(+), 22 deletions(-) -- 2.7.4
Re: [PATCH] iommu/rockchip: don't use platform_get_irq to implicitly count irqs
Hi,
Missatge de Heiko Stuebner del dia dc., 25 de set.
2019 a les 20:44:
>
> Till now the Rockchip iommu driver walked through the irq list via
> platform_get_irq() until it encountered an ENXIO error. With the
> recent change to add a central error message, this always results
> in such an error for each iommu on probe and shutdown.
>
> To not confuse people, switch to platform_count_irqs() to get the
> actual number of interrupts before walking through them.
>
> Fixes: 7723f4c5ecdb ("driver core: platform: Add an error message to
> platform_get_irq*()")
> Signed-off-by: Heiko Stuebner
> ---
This patch definitely removes the annoying messages on my Samsung
Chromebook Plus like:
rk_iommu ff924000.iommu: IRQ index 1 not found
rk_iommu ff914000.iommu: IRQ index 1 not found
rk_iommu ff903f00.iommu: IRQ index 1 not found
rk_iommu ff8f3f00.iommu: IRQ index 1 not found
rk_iommu ff650800.iommu: IRQ index 1 not found
FWIW, I sent a similar patch [1] to fix this, but can be rejected in
favour of the Heiko's patch. So,
Tested-by: Enric Balletbo i Serra
Thanks,
Enric
[1] https://lkml.org/lkml/2019/10/8/551
> drivers/iommu/rockchip-iommu.c | 19 ++-
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 26290f310f90..4dcbf68dfda4 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -100,6 +100,7 @@ struct rk_iommu {
> struct device *dev;
> void __iomem **bases;
> int num_mmu;
> + int num_irq;
> struct clk_bulk_data *clocks;
> int num_clocks;
> bool reset_disabled;
> @@ -1136,7 +1137,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
> struct rk_iommu *iommu;
> struct resource *res;
> int num_res = pdev->num_resources;
> - int err, i, irq;
> + int err, i;
>
> iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> if (!iommu)
> @@ -1163,6 +1164,10 @@ static int rk_iommu_probe(struct platform_device *pdev)
> if (iommu->num_mmu == 0)
> return PTR_ERR(iommu->bases[0]);
>
> + iommu->num_irq = platform_irq_count(pdev);
> + if (iommu->num_irq < 0)
> + return iommu->num_irq;
> +
> iommu->reset_disabled = device_property_read_bool(dev,
> "rockchip,disable-mmu-reset");
>
> @@ -1219,8 +1224,9 @@ static int rk_iommu_probe(struct platform_device *pdev)
>
> pm_runtime_enable(dev);
>
> - i = 0;
> - while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
> + for (i = 0; i < iommu->num_irq; i++) {
> + int irq = platform_get_irq(pdev, i);
> +
> if (irq < 0)
> return irq;
>
> @@ -1245,10 +1251,13 @@ static int rk_iommu_probe(struct platform_device
> *pdev)
> static void rk_iommu_shutdown(struct platform_device *pdev)
> {
> struct rk_iommu *iommu = platform_get_drvdata(pdev);
> - int i = 0, irq;
> + int i;
> +
> + for (i = 0; i < iommu->num_irq; i++) {
> + int irq = platform_get_irq(pdev, i);
>
> - while ((irq = platform_get_irq(pdev, i++)) != -ENXIO)
> devm_free_irq(iommu->dev, irq, iommu);
> + }
>
> pm_runtime_force_suspend(>dev);
> }
> --
> 2.23.0
>
>
> ___
> Linux-rockchip mailing list
> linux-rockc...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/mediatek: Move the tlb_sync into tlb_flush
On Tue, Oct 8, 2019 at 5:09 PM Yong Wu wrote:
>
> Hi Tomasz,
>
> Sorry for reply late.
>
> On Wed, 2019-10-02 at 14:18 +0900, Tomasz Figa wrote:
> > Hi Yong,
> >
> > On Mon, Sep 30, 2019 at 2:42 PM Yong Wu wrote:
> > >
> > > The commit 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API
> > > TLB sync") help move the tlb_sync of unmap from v7s into the iommu
> > > framework. It helps add a new function "mtk_iommu_iotlb_sync", But it
> > > lacked the dom->pgtlock, then it will cause the variable
> > > "tlb_flush_active" may be changed unexpectedly, we could see this warning
> > > log randomly:
> > >
> >
> > Thanks for the patch! Please see my comments inline.
> >
> > > mtk-iommu 10205000.iommu: Partial TLB flush timed out, falling back to
> > > full flush
> > >
> > > To fix this issue, we can add dom->pgtlock in the "mtk_iommu_iotlb_sync".
> > > And when checking this issue, we find that __arm_v7s_unmap call
> > > io_pgtable_tlb_add_flush consecutively when it is supersection/largepage,
> > > this also is potential unsafe for us. There is no tlb flush queue in the
> > > MediaTek M4U HW. The HW always expect the tlb_flush/tlb_sync one by one.
> > > If v7s don't always gurarantee the sequence, Thus, In this patch I move
> > > the tlb_sync into tlb_flush(also rename the function deleting "_nosync").
> > > and we don't care if it is leaf, rearrange the callback functions. Also,
> > > the tlb flush/sync was already finished in v7s, then iotlb_sync and
> > > iotlb_sync_all is unnecessary.
> >
> > Performance-wise, we could do much better. Instead of synchronously
> > syncing at the end of mtk_iommu_tlb_add_flush(), we could sync at the
> > beginning, if there was any previous flush still pending. We would
> > also have to keep the .iotlb_sync() callback, to take care of waiting
> > for the last flush. That would allow better pipelining with CPU in
> > cases like this:
> >
> > for (all pages in range) {
> >change page table();
> >flush();
> > }
> >
> > "change page table()" could execute while the IOMMU is flushing the
> > previous change.
>
> Do you mean adding a new tlb_sync before tlb_flush_no_sync, like below:
>
> mtk_iommu_tlb_add_flush_nosync {
>+ mtk_iommu_tlb_sync();
>tlb_flush_no_sync();
>data->tlb_flush_active = true;
> }
>
> mtk_iommu_tlb_sync {
> if (!data->tlb_flush_active)
> return;
> tlb_sync();
> data->tlb_flush_active = false;
> }
>
> This way look improve the flow, But adjusting the flow is not the root
> cause of this issue. the problem is "data->tlb_flush_active" may be
> changed from mtk_iommu_iotlb_sync which don't have a dom->pglock.
That was not the only problem with existing code. Existing code also
assumed that add_flush and sync always go in pairs, but that's not
true.
My suggestion is to fix the locking in the driver and keep the sync
deferred as much as possible, so that performance is not degraded. I
changed my mind, though. I think we would need to make more changes to
the driver to make it implement the flushing efficiently, so let's go
with the current simple approach for now and improve incrementally.
>
> Currently the synchronisation of the tlb_flush/tlb_sync flow are
> controlled by the variable "data->tlb_flush_active".
>
> In this patch putting the tlb_flush/tlb_sync together looks make
> the flow simpler:
> a) Don't need the sensitive variable "tlb_flush_active".
> b) Remove mtk_iommu_iotlb_sync, Don't need add lock in it.
> c) Simplify the tlb_flush_walk/tlb_flush_leaf.
> is it ok?
>
Okay, let's do so as a first step to fix the issue. Then we can
optimize in follow up patches.
> >
> > >
> > > Besides, there are two minor changes:
> > > a) Use writel for the register F_MMU_INV_RANGE which is for triggering the
> > > HW work. We expect all the setting(iova_start/iova_end...) have already
> > > been finished before F_MMU_INV_RANGE.
> > > b) Reduce the tlb timeout value from 10us to 1000us. the original
> > > value
> > > is so long that affect the multimedia performance.
> >
> > By definition, timeout is something that should not normally happen.
> > Too long timeout affecting multimedia performance would suggest that
> > the timeout was actually happening, which is the core problem, not the
> > length of the timeout. Could you provide more details on this?
>
> As description above, this issue is because there is no dom->pgtlock in
> the mtk_iommu_iotlb_sync. I have tried that the issue will disappear
> after adding lock in it.
>
> Although the issue is fixed after this patch, I still would like to
> reduce the timeout value for somehow error happen in the future. 100ms
> is unnecessary for us. It looks a minor improvement rather than fixing
> the issue. I will use a new patch for it.
>
Okay, makes sense.
Best regards,
Tomasz
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()
On Tue, Oct 08, 2019 at 04:18:35PM -0600, Logan Gunthorpe wrote:
> From: Kit Chow
>
> Currently the Intel IOMMU uses the default dma_[un]map_resource()
s/Intel/AMD/ ?
> +static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
> + size_t size, enum dma_data_direction dir, unsigned long attrs)
> +{
> + struct protection_domain *domain;
> + struct dma_ops_domain *dma_dom;
> +
> + domain = get_domain(dev);
> + if (PTR_ERR(domain) == -EINVAL)
> + return (dma_addr_t)paddr;
I thought that case can't happen anymore?
Also note that Joerg just applied the patch to convert the AMD iommu
driver to use the dma-iommu ops. Can you test that series and check
it does the right thing for your use case? From looking at the code
I think it should.
