This patchset mainly fixes a tlb flush timeout issue and use the new
iommu_gather to re-implement the tlb flush flow. and several clean up
patches about the tlb_flush.
change note:
v3:
1. Use the gather to implement the tlb_flush suggested from Tomasz.
2. add some clean up patches.
v2:
https://lore.kernel.org/linux-iommu/[email protected]/T/#t
1. rebase on v5.4-rc1
2. only split to several patches.
v1:
https://lore.kernel.org/linux-iommu/CAAFQd5C3U7pZo4SSUJ52Q7E+0FaUoORQFbQC5RhCHBhi=NFYTw@mail.gmail.com/T/#t
Yong Wu (7):
iommu/mediatek: Correct the flush_iotlb_all callback
iommu/mediatek: Add pgtlock in the iotlb_sync
iommu/mediatek: Use gather to achieve the tlb range flush
iommu/mediatek: Delete the leaf in the tlb flush
iommu/mediatek: Move the tlb_sync into tlb_flush
iommu/mediatek: Use writel for TLB range invalidation
iommu/mediatek: Reduce the tlb flush timeout value
drivers/iommu/mtk_iommu.c | 77 +++++++++++++++++++++++------------------------
drivers/iommu/mtk_iommu.h | 2 +-
2 files changed, 38 insertions(+), 41 deletions(-)
--
1.9.1
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 <[email protected]>
---
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
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
This patch adds dom->pgtlock in mtk_iommu_iotlb_sync to fix this issue.
Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB sync")
Signed-off-by: Yong Wu <[email protected]>
---
drivers/iommu/mtk_iommu.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 76b9388..5f594d6 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -453,7 +453,12 @@ static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
struct iommu_iotlb_gather *gather)
{
+ struct mtk_iommu_domain *dom = to_mtk_domain(domain);
+ unsigned long flags;
+
+ spin_lock_irqsave(&dom->pgtlock, flags);
mtk_iommu_tlb_sync(mtk_iommu_get_m4u_data());
+ spin_unlock_irqrestore(&dom->pgtlock, flags);
}
static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
--
1.9.1
Use the iommu_gather mechanism to achieve the tlb range flush.
Gather the iova range in the "tlb_add_page", then flush the merged iova
range in iotlb_sync.
Note: If iotlb_sync comes from iommu_iotlb_gather_add_page, we have to
avoid retry the lock since the spinlock have already been acquired.
Suggested-by: Tomasz Figa <[email protected]>
Signed-off-by: Yong Wu <[email protected]>
---
1) This is the special case backtrace:
mtk_iommu_iotlb_sync+0x50/0xa0
mtk_iommu_tlb_flush_page_nosync+0x5c/0xd0
__arm_v7s_unmap+0x174/0x598
arm_v7s_unmap+0x30/0x48
mtk_iommu_unmap+0x50/0x78
__iommu_unmap+0xa4/0xf8
2) The checking "if (gather->start == ULONG_MAX) return;" also is
necessary. It will happened when unmap only go to _flush_walk, then
enter this tlb_sync.
---
drivers/iommu/mtk_iommu.c | 29 +++++++++++++++++++++++++----
drivers/iommu/mtk_iommu.h | 1 +
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 5f594d6..8712afc 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -234,7 +234,12 @@ 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);
+ struct mtk_iommu_data *data = cookie;
+ struct iommu_domain *domain = &data->m4u_dom->domain;
+
+ data->is_in_tlb_gather_add_page = true;
+ iommu_iotlb_gather_add_page(domain, gather, iova, granule);
+ data->is_in_tlb_gather_add_page = false;
}
static const struct iommu_flush_ops mtk_iommu_flush_ops = {
@@ -453,12 +458,28 @@ static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
struct iommu_iotlb_gather *gather)
{
+ struct mtk_iommu_data *data = mtk_iommu_get_m4u_data();
struct mtk_iommu_domain *dom = to_mtk_domain(domain);
+ bool is_in_gather = data->is_in_tlb_gather_add_page;
+ size_t length = gather->end - gather->start;
unsigned long flags;
- spin_lock_irqsave(&dom->pgtlock, flags);
- mtk_iommu_tlb_sync(mtk_iommu_get_m4u_data());
- spin_unlock_irqrestore(&dom->pgtlock, flags);
+ if (gather->start == ULONG_MAX)
+ return;
+
+ /*
+ * Avoid acquire the lock when it's in gather_add_page since the lock
+ * has already been held.
+ */
+ if (!is_in_gather)
+ spin_lock_irqsave(&dom->pgtlock, flags);
+
+ mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
+ false, data);
+ mtk_iommu_tlb_sync(data);
+
+ if (!is_in_gather)
+ spin_unlock_irqrestore(&dom->pgtlock, flags);
}
static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index fc0f16e..d29af1d 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -58,6 +58,7 @@ struct mtk_iommu_data {
struct iommu_group *m4u_group;
bool enable_4GB;
bool tlb_flush_active;
+ bool is_in_tlb_gather_add_page;
struct iommu_device iommu;
const struct mtk_iommu_plat_data *plat_data;
--
1.9.1
In our tlb range flush, we don't care the "leaf". Remove it to simplify
the code. no functional change.
Signed-off-by: Yong Wu <[email protected]>
---
drivers/iommu/mtk_iommu.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 8712afc..19f936c 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -174,8 +174,7 @@ 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)
+ size_t granule, void *cookie)
{
struct mtk_iommu_data *data = cookie;
@@ -219,14 +218,7 @@ static void mtk_iommu_tlb_sync(void *cookie)
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)
-{
- mtk_iommu_tlb_add_flush_nosync(iova, size, granule, true, cookie);
+ mtk_iommu_tlb_add_flush_nosync(iova, size, granule, cookie);
mtk_iommu_tlb_sync(cookie);
}
@@ -245,7 +237,7 @@ static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
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_flush_leaf = mtk_iommu_tlb_flush_walk,
.tlb_add_page = mtk_iommu_tlb_flush_page_nosync,
};
@@ -475,7 +467,7 @@ static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
spin_lock_irqsave(&dom->pgtlock, flags);
mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
- false, data);
+ data);
mtk_iommu_tlb_sync(data);
if (!is_in_gather)
--
1.9.1
Right now, the tlb_add_flush_nosync and tlb_sync always appear together.
we merge the two functions into one. No functional change.
mtk_iommu_tlb_add_flush_nosync
mtk_iommu_tlb_sync
Signed-off-by: Chao Hao <[email protected]>
Signed-off-by: Yong Wu <[email protected]>
---
drivers/iommu/mtk_iommu.c | 36 ++++++++----------------------------
drivers/iommu/mtk_iommu.h | 1 -
2 files changed, 8 insertions(+), 29 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 19f936c..dbbacc3 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -173,10 +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,
+static void mtk_iommu_tlb_flush_range_sync(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,
@@ -187,21 +189,8 @@ 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;
+ /* tlb sync */
ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
tmp, tmp != 0, 10, 100000);
if (ret) {
@@ -211,17 +200,9 @@ 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, 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)
@@ -236,8 +217,8 @@ static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
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_walk,
+ .tlb_flush_walk = mtk_iommu_tlb_flush_range_sync,
+ .tlb_flush_leaf = mtk_iommu_tlb_flush_range_sync,
.tlb_add_page = mtk_iommu_tlb_flush_page_nosync,
};
@@ -466,9 +447,8 @@ static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
if (!is_in_gather)
spin_lock_irqsave(&dom->pgtlock, flags);
- mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
- data);
- mtk_iommu_tlb_sync(data);
+ mtk_iommu_tlb_flush_range_sync(gather->start, length,
+ gather->pgsize, data);
if (!is_in_gather)
spin_unlock_irqrestore(&dom->pgtlock, flags);
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index d29af1d..9056f73 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -57,7 +57,6 @@ struct mtk_iommu_data {
struct mtk_iommu_domain *m4u_dom;
struct iommu_group *m4u_group;
bool enable_4GB;
- bool tlb_flush_active;
bool is_in_tlb_gather_add_page;
struct iommu_device iommu;
--
1.9.1
Reduce the tlb timeout value from 100000us 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 <[email protected]>
---
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 d285457..c9d49af 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -191,7 +191,7 @@ static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size,
/* tlb sync */
ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
- tmp, tmp != 0, 10, 100000);
+ 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
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 <[email protected]>
Signed-off-by: Yong Wu <[email protected]>
---
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 dbbacc3..d285457 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -187,8 +187,7 @@ static void mtk_iommu_tlb_flush_range_sync(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);
/* tlb sync */
ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
--
1.9.1
On 14/10/2019 07:38, Yong Wu wrote:
> 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.
For Arm CPUs, these registers should be mapped as Device memory,
therefore the same-peripheral rule should implicitly enforce that the
accesses are made in program order, hence you're unlikely to have seen a
problem in reality. However, the logical reasoning for the change seems
valid in general, so I'd argue that it's still worth making if only for
the sake of good practice:
Acked-by: Robin Murphy <[email protected]>
> Signed-off-by: Anan.Sun <[email protected]>
> Signed-off-by: Yong Wu <[email protected]>
> ---
> 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 dbbacc3..d285457 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -187,8 +187,7 @@ static void mtk_iommu_tlb_flush_range_sync(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);
>
> /* tlb sync */
> ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
>
On 14/10/2019 07:38, Yong Wu wrote:
> Use the correct tlb_flush_all instead of the original one.
Reviewed-by: Robin Murphy <[email protected]>
> Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB sync")
> Signed-off-by: Yong Wu <[email protected]>
> ---
> 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,
>
On 14/10/2019 07:38, Yong Wu wrote:
> Use the iommu_gather mechanism to achieve the tlb range flush.
> Gather the iova range in the "tlb_add_page", then flush the merged iova
> range in iotlb_sync.
>
> Note: If iotlb_sync comes from iommu_iotlb_gather_add_page, we have to
> avoid retry the lock since the spinlock have already been acquired.
I think this could probably be even simpler - once the actual
register-poking is all confined to mtk_iommu_tlb_sync(), you should be
able get rid of the per-domain locking in map/unmap and just have a
single per-IOMMU lock to serialise syncs. The io-pgtable code itself
hasn't needed external locking for a while now.
Robin.
> Suggested-by: Tomasz Figa <[email protected]>
> Signed-off-by: Yong Wu <[email protected]>
> ---
> 1) This is the special case backtrace:
>
> mtk_iommu_iotlb_sync+0x50/0xa0
> mtk_iommu_tlb_flush_page_nosync+0x5c/0xd0
> __arm_v7s_unmap+0x174/0x598
> arm_v7s_unmap+0x30/0x48
> mtk_iommu_unmap+0x50/0x78
> __iommu_unmap+0xa4/0xf8
>
> 2) The checking "if (gather->start == ULONG_MAX) return;" also is
> necessary. It will happened when unmap only go to _flush_walk, then
> enter this tlb_sync.
> ---
> drivers/iommu/mtk_iommu.c | 29 +++++++++++++++++++++++++----
> drivers/iommu/mtk_iommu.h | 1 +
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 5f594d6..8712afc 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -234,7 +234,12 @@ 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);
> + struct mtk_iommu_data *data = cookie;
> + struct iommu_domain *domain = &data->m4u_dom->domain;
> +
> + data->is_in_tlb_gather_add_page = true;
> + iommu_iotlb_gather_add_page(domain, gather, iova, granule);
> + data->is_in_tlb_gather_add_page = false;
> }
>
> static const struct iommu_flush_ops mtk_iommu_flush_ops = {
> @@ -453,12 +458,28 @@ static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
> static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
> struct iommu_iotlb_gather *gather)
> {
> + struct mtk_iommu_data *data = mtk_iommu_get_m4u_data();
> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
> + bool is_in_gather = data->is_in_tlb_gather_add_page;
> + size_t length = gather->end - gather->start;
> unsigned long flags;
>
> - spin_lock_irqsave(&dom->pgtlock, flags);
> - mtk_iommu_tlb_sync(mtk_iommu_get_m4u_data());
> - spin_unlock_irqrestore(&dom->pgtlock, flags);
> + if (gather->start == ULONG_MAX)
> + return;
> +
> + /*
> + * Avoid acquire the lock when it's in gather_add_page since the lock
> + * has already been held.
> + */
> + if (!is_in_gather)
> + spin_lock_irqsave(&dom->pgtlock, flags);
> +
> + mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
> + false, data);
> + mtk_iommu_tlb_sync(data);
> +
> + if (!is_in_gather)
> + spin_unlock_irqrestore(&dom->pgtlock, flags);
> }
>
> static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index fc0f16e..d29af1d 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -58,6 +58,7 @@ struct mtk_iommu_data {
> struct iommu_group *m4u_group;
> bool enable_4GB;
> bool tlb_flush_active;
> + bool is_in_tlb_gather_add_page;
>
> struct iommu_device iommu;
> const struct mtk_iommu_plat_data *plat_data;
>
On 14/10/2019 07:38, Yong Wu wrote:
> In our tlb range flush, we don't care the "leaf". Remove it to simplify
> the code. no functional change.
Presumably you don't care about the granule either?
Robin.
> Signed-off-by: Yong Wu <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 8712afc..19f936c 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -174,8 +174,7 @@ 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)
> + size_t granule, void *cookie)
> {
> struct mtk_iommu_data *data = cookie;
>
> @@ -219,14 +218,7 @@ static void mtk_iommu_tlb_sync(void *cookie)
> 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)
> -{
> - mtk_iommu_tlb_add_flush_nosync(iova, size, granule, true, cookie);
> + mtk_iommu_tlb_add_flush_nosync(iova, size, granule, cookie);
> mtk_iommu_tlb_sync(cookie);
> }
>
> @@ -245,7 +237,7 @@ static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
> 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_flush_leaf = mtk_iommu_tlb_flush_walk,
> .tlb_add_page = mtk_iommu_tlb_flush_page_nosync,
> };
>
> @@ -475,7 +467,7 @@ static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
> spin_lock_irqsave(&dom->pgtlock, flags);
>
> mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
> - false, data);
> + data);
> mtk_iommu_tlb_sync(data);
>
> if (!is_in_gather)
>
On Mon, 2019-10-14 at 15:22 +0100, Robin Murphy wrote:
> On 14/10/2019 07:38, Yong Wu wrote:
> > In our tlb range flush, we don't care the "leaf". Remove it to simplify
> > the code. no functional change.
>
> Presumably you don't care about the granule either?
Yes. I only keep "granule" to satisfy the format of "tlb_flush_walk",
then it's no need add a new helper function.
>
> Robin.
>
> > Signed-off-by: Yong Wu <[email protected]>
> > ---
> > drivers/iommu/mtk_iommu.c | 16 ++++------------
> > 1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 8712afc..19f936c 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -174,8 +174,7 @@ 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)
> > + size_t granule, void *cookie)
> > {
> > struct mtk_iommu_data *data = cookie;
> >
> > @@ -219,14 +218,7 @@ static void mtk_iommu_tlb_sync(void *cookie)
> > 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)
> > -{
> > - mtk_iommu_tlb_add_flush_nosync(iova, size, granule, true, cookie);
> > + mtk_iommu_tlb_add_flush_nosync(iova, size, granule, cookie);
> > mtk_iommu_tlb_sync(cookie);
> > }
> >
> > @@ -245,7 +237,7 @@ static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
> > 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_flush_leaf = mtk_iommu_tlb_flush_walk,
> > .tlb_add_page = mtk_iommu_tlb_flush_page_nosync,
> > };
> >
> > @@ -475,7 +467,7 @@ static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
> > spin_lock_irqsave(&dom->pgtlock, flags);
> >
> > mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
> > - false, data);
> > + data);
> > mtk_iommu_tlb_sync(data);
> >
> > if (!is_in_gather)
> >
>
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Mon, 2019-10-14 at 15:04 +0100, Robin Murphy wrote:
> On 14/10/2019 07:38, Yong Wu wrote:
> > 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.
>
> For Arm CPUs, these registers should be mapped as Device memory,
> therefore the same-peripheral rule should implicitly enforce that the
> accesses are made in program order, hence you're unlikely to have seen a
> problem in reality. However, the logical reasoning for the change seems
> valid in general, so I'd argue that it's still worth making if only for
> the sake of good practice:
>
> Acked-by: Robin Murphy <[email protected]>
Thanks very much for the view. If this patch is not so necessary, I will
remove it this time.
>
> > Signed-off-by: Anan.Sun <[email protected]>
> > Signed-off-by: Yong Wu <[email protected]>
> > ---
> > 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 dbbacc3..d285457 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -187,8 +187,7 @@ static void mtk_iommu_tlb_flush_range_sync(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);
> >
> > /* tlb sync */
> > ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
> >
>
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Mon, 2019-10-14 at 15:21 +0100, Robin Murphy wrote:
> On 14/10/2019 07:38, Yong Wu wrote:
> > Use the iommu_gather mechanism to achieve the tlb range flush.
> > Gather the iova range in the "tlb_add_page", then flush the merged iova
> > range in iotlb_sync.
> >
> > Note: If iotlb_sync comes from iommu_iotlb_gather_add_page, we have to
> > avoid retry the lock since the spinlock have already been acquired.
>
> I think this could probably be even simpler - once the actual
> register-poking is all confined to mtk_iommu_tlb_sync(), you should be
> able get rid of the per-domain locking in map/unmap and just have a
> single per-IOMMU lock to serialise syncs. The io-pgtable code itself
> hasn't needed external locking for a while now.
This is more simpler! Thanks very much. I will try this.
The only concern is there is no lock in the iova_to_phys then, maybe use
the new lock instead.
>
> Robin.
>
> > Suggested-by: Tomasz Figa <[email protected]>
> > Signed-off-by: Yong Wu <[email protected]>
> > ---
> > 1) This is the special case backtrace:
> >
> > mtk_iommu_iotlb_sync+0x50/0xa0
> > mtk_iommu_tlb_flush_page_nosync+0x5c/0xd0
> > __arm_v7s_unmap+0x174/0x598
> > arm_v7s_unmap+0x30/0x48
> > mtk_iommu_unmap+0x50/0x78
> > __iommu_unmap+0xa4/0xf8
> >
> > 2) The checking "if (gather->start == ULONG_MAX) return;" also is
> > necessary. It will happened when unmap only go to _flush_walk, then
> > enter this tlb_sync.
> > ---
> > drivers/iommu/mtk_iommu.c | 29 +++++++++++++++++++++++++----
> > drivers/iommu/mtk_iommu.h | 1 +
> > 2 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 5f594d6..8712afc 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -234,7 +234,12 @@ 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);
> > + struct mtk_iommu_data *data = cookie;
> > + struct iommu_domain *domain = &data->m4u_dom->domain;
> > +
> > + data->is_in_tlb_gather_add_page = true;
> > + iommu_iotlb_gather_add_page(domain, gather, iova, granule);
> > + data->is_in_tlb_gather_add_page = false;
> > }
> >
> > static const struct iommu_flush_ops mtk_iommu_flush_ops = {
> > @@ -453,12 +458,28 @@ static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
> > static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
> > struct iommu_iotlb_gather *gather)
> > {
> > + struct mtk_iommu_data *data = mtk_iommu_get_m4u_data();
> > struct mtk_iommu_domain *dom = to_mtk_domain(domain);
> > + bool is_in_gather = data->is_in_tlb_gather_add_page;
> > + size_t length = gather->end - gather->start;
> > unsigned long flags;
> >
> > - spin_lock_irqsave(&dom->pgtlock, flags);
> > - mtk_iommu_tlb_sync(mtk_iommu_get_m4u_data());
> > - spin_unlock_irqrestore(&dom->pgtlock, flags);
> > + if (gather->start == ULONG_MAX)
> > + return;
> > +
> > + /*
> > + * Avoid acquire the lock when it's in gather_add_page since the lock
> > + * has already been held.
> > + */
> > + if (!is_in_gather)
> > + spin_lock_irqsave(&dom->pgtlock, flags);
> > +
> > + mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
> > + false, data);
> > + mtk_iommu_tlb_sync(data);
> > +
> > + if (!is_in_gather)
> > + spin_unlock_irqrestore(&dom->pgtlock, flags);
> > }
> >
> > static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index fc0f16e..d29af1d 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -58,6 +58,7 @@ struct mtk_iommu_data {
> > struct iommu_group *m4u_group;
> > bool enable_4GB;
> > bool tlb_flush_active;
> > + bool is_in_tlb_gather_add_page;
> >
> > struct iommu_device iommu;
> > const struct mtk_iommu_plat_data *plat_data;
> >
On 15/10/2019 06:25, Yong Wu wrote:
> On Mon, 2019-10-14 at 15:22 +0100, Robin Murphy wrote:
>> On 14/10/2019 07:38, Yong Wu wrote:
>>> In our tlb range flush, we don't care the "leaf". Remove it to simplify
>>> the code. no functional change.
>>
>> Presumably you don't care about the granule either?
>
> Yes. I only keep "granule" to satisfy the format of "tlb_flush_walk",
> then it's no need add a new helper function.
Ah, I'd failed to make the connection that it ends up wired in directly
to the callbacks in patch #5 - indeed there's no point churning the
mtk_iommu_tlb_add_flush_nosync() callsites here if they're just getting
removed later anyway. In that case,
Reviewed-by: Robin Murphy <[email protected]>
>>> Signed-off-by: Yong Wu <[email protected]>
>>> ---
>>> drivers/iommu/mtk_iommu.c | 16 ++++------------
>>> 1 file changed, 4 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 8712afc..19f936c 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -174,8 +174,7 @@ 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)
>>> + size_t granule, void *cookie)
>>> {
>>> struct mtk_iommu_data *data = cookie;
>>>
>>> @@ -219,14 +218,7 @@ static void mtk_iommu_tlb_sync(void *cookie)
>>> 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)
>>> -{
>>> - mtk_iommu_tlb_add_flush_nosync(iova, size, granule, true, cookie);
>>> + mtk_iommu_tlb_add_flush_nosync(iova, size, granule, cookie);
>>> mtk_iommu_tlb_sync(cookie);
>>> }
>>>
>>> @@ -245,7 +237,7 @@ static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
>>> 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_flush_leaf = mtk_iommu_tlb_flush_walk,
>>> .tlb_add_page = mtk_iommu_tlb_flush_page_nosync,
>>> };
>>>
>>> @@ -475,7 +467,7 @@ static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
>>> spin_lock_irqsave(&dom->pgtlock, flags);
>>>
>>> mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
>>> - false, data);
>>> + data);
>>> mtk_iommu_tlb_sync(data);
>>>
>>> if (!is_in_gather)
>>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
>
On 15/10/2019 06:26, Yong Wu wrote:
> On Mon, 2019-10-14 at 15:21 +0100, Robin Murphy wrote:
>> On 14/10/2019 07:38, Yong Wu wrote:
>>> Use the iommu_gather mechanism to achieve the tlb range flush.
>>> Gather the iova range in the "tlb_add_page", then flush the merged iova
>>> range in iotlb_sync.
>>>
>>> Note: If iotlb_sync comes from iommu_iotlb_gather_add_page, we have to
>>> avoid retry the lock since the spinlock have already been acquired.
>>
>> I think this could probably be even simpler - once the actual
>> register-poking is all confined to mtk_iommu_tlb_sync(), you should be
>> able get rid of the per-domain locking in map/unmap and just have a
>> single per-IOMMU lock to serialise syncs. The io-pgtable code itself
>> hasn't needed external locking for a while now.
>
> This is more simpler! Thanks very much. I will try this.
>
> The only concern is there is no lock in the iova_to_phys then, maybe use
> the new lock instead.
iova_to_phys isn't issuing any syncs, so you don't need any locking
there - if anyone calls that in a way which races against the given
address being unmapped and remapped they can't expect a meaningful
result anyway.
Robin.
>>> Suggested-by: Tomasz Figa <[email protected]>
>>> Signed-off-by: Yong Wu <[email protected]>
>>> ---
>>> 1) This is the special case backtrace:
>>>
>>> mtk_iommu_iotlb_sync+0x50/0xa0
>>> mtk_iommu_tlb_flush_page_nosync+0x5c/0xd0
>>> __arm_v7s_unmap+0x174/0x598
>>> arm_v7s_unmap+0x30/0x48
>>> mtk_iommu_unmap+0x50/0x78
>>> __iommu_unmap+0xa4/0xf8
>>>
>>> 2) The checking "if (gather->start == ULONG_MAX) return;" also is
>>> necessary. It will happened when unmap only go to _flush_walk, then
>>> enter this tlb_sync.
>>> ---
>>> drivers/iommu/mtk_iommu.c | 29 +++++++++++++++++++++++++----
>>> drivers/iommu/mtk_iommu.h | 1 +
>>> 2 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 5f594d6..8712afc 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -234,7 +234,12 @@ 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);
>>> + struct mtk_iommu_data *data = cookie;
>>> + struct iommu_domain *domain = &data->m4u_dom->domain;
>>> +
>>> + data->is_in_tlb_gather_add_page = true;
>>> + iommu_iotlb_gather_add_page(domain, gather, iova, granule);
>>> + data->is_in_tlb_gather_add_page = false;
>>> }
>>>
>>> static const struct iommu_flush_ops mtk_iommu_flush_ops = {
>>> @@ -453,12 +458,28 @@ static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
>>> static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
>>> struct iommu_iotlb_gather *gather)
>>> {
>>> + struct mtk_iommu_data *data = mtk_iommu_get_m4u_data();
>>> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
>>> + bool is_in_gather = data->is_in_tlb_gather_add_page;
>>> + size_t length = gather->end - gather->start;
>>> unsigned long flags;
>>>
>>> - spin_lock_irqsave(&dom->pgtlock, flags);
>>> - mtk_iommu_tlb_sync(mtk_iommu_get_m4u_data());
>>> - spin_unlock_irqrestore(&dom->pgtlock, flags);
>>> + if (gather->start == ULONG_MAX)
>>> + return;
>>> +
>>> + /*
>>> + * Avoid acquire the lock when it's in gather_add_page since the lock
>>> + * has already been held.
>>> + */
>>> + if (!is_in_gather)
>>> + spin_lock_irqsave(&dom->pgtlock, flags);
>>> +
>>> + mtk_iommu_tlb_add_flush_nosync(gather->start, length, gather->pgsize,
>>> + false, data);
>>> + mtk_iommu_tlb_sync(data);
>>> +
>>> + if (!is_in_gather)
>>> + spin_unlock_irqrestore(&dom->pgtlock, flags);
>>> }
>>>
>>> static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
>>> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
>>> index fc0f16e..d29af1d 100644
>>> --- a/drivers/iommu/mtk_iommu.h
>>> +++ b/drivers/iommu/mtk_iommu.h
>>> @@ -58,6 +58,7 @@ struct mtk_iommu_data {
>>> struct iommu_group *m4u_group;
>>> bool enable_4GB;
>>> bool tlb_flush_active;
>>> + bool is_in_tlb_gather_add_page;
>>>
>>> struct iommu_device iommu;
>>> const struct mtk_iommu_plat_data *plat_data;
>>>
>
>