[PATCH 0/5] iommu/sun50i: Fix various issues

[PATCH 0/5] iommu/sun50i: Fix various issues

[Jernej Skrabec]

Testing IOMMU together with video decoder (Cedrus) exposed many bugs in
sun50i-iommu driver. This series addresses all issues so video decoder
works well with IOMMU.

First two patches address recovery issues in interrupt when either page
faults or permission errors were reported. Third patch fixes permission
domain assignment. Fourth patch fixes dma sync size. Sometimes sync also
touched some other buffers and kernel generated warning in dmesg. Fifth
patch fixes issue with synching PDE and PTE tables. Without it, page
faults were randomly generated even with valid iova addresses.

Please take a look.

Best regards,
Jernej

Jernej Skrabec (5):
  iommu/sun50i: Fix reset release
  iommu/sun50i: Consider all fault sources for reset
  iommu/sun50i: Fix R/W permission check
  iommu/sun50i: Fix flush size
  iommu/sun50i: Invalidate iova in map and unmap callback

 drivers/iommu/sun50i-iommu.c | 66 +++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 5 deletions(-)

--
2.38.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] iommu/sun50i: Fix reset release

[Jernej Skrabec]

Reset signal is asserted by writing 0 to the corresponding locations of
masters we want to reset. So in order to deassert all reset signals, we
should write 1's to all locations.

Current code writes 1's to locations of masters which were just reset
which is good. However, at the same time it also writes 0's to other
locations and thus asserts reset signals of remaining masters. Fix code
by writing all 1's when we want to deassert all reset signals.

This bug was discovered when working with Cedrus (video decoder). When
it faulted, display went blank due to reset signal assertion.

Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/iommu/sun50i-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index a84c63518773..c777882d0ec2 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -27,6 +27,7 @@
 #include <linux/types.h>
 
 #define IOMMU_RESET_REG			0x010
+#define IOMMU_RESET_RELEASE_ALL			0xffffffff
 #define IOMMU_ENABLE_REG		0x020
 #define IOMMU_ENABLE_ENABLE			BIT(0)
 
@@ -893,7 +894,7 @@ static irqreturn_t sun50i_iommu_irq(int irq, void *dev_id)
 	iommu_write(iommu, IOMMU_INT_CLR_REG, status);
 
 	iommu_write(iommu, IOMMU_RESET_REG, ~status);
-	iommu_write(iommu, IOMMU_RESET_REG, status);
+	iommu_write(iommu, IOMMU_RESET_REG, IOMMU_RESET_RELEASE_ALL);
 
 	spin_unlock(&iommu->iommu_lock);
 
-- 
2.38.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] iommu/sun50i: Consider all fault sources for reset

[Jernej Skrabec]

We have to reset masters for all faults - permissions, L1 fault or L2
fault. Currently it's done only for permissions. If other type of fault
happens, master is in locked up state. Fix that by really considering
all fault sources.

Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/iommu/sun50i-iommu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index c777882d0ec2..38d1069cf383 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -869,8 +869,8 @@ static phys_addr_t sun50i_iommu_handle_perm_irq(struct sun50i_iommu *iommu)
 
 static irqreturn_t sun50i_iommu_irq(int irq, void *dev_id)
 {
+	u32 status, l1_status, l2_status, resets;
 	struct sun50i_iommu *iommu = dev_id;
-	u32 status;
 
 	spin_lock(&iommu->iommu_lock);
 
@@ -880,6 +880,9 @@ static irqreturn_t sun50i_iommu_irq(int irq, void *dev_id)
 		return IRQ_NONE;
 	}
 
+	l1_status = iommu_read(iommu, IOMMU_L1PG_INT_REG);
+	l2_status = iommu_read(iommu, IOMMU_L2PG_INT_REG);
+
 	if (status & IOMMU_INT_INVALID_L2PG)
 		sun50i_iommu_handle_pt_irq(iommu,
 					    IOMMU_INT_ERR_ADDR_L2_REG,
@@ -893,7 +896,8 @@ static irqreturn_t sun50i_iommu_irq(int irq, void *dev_id)
 
 	iommu_write(iommu, IOMMU_INT_CLR_REG, status);
 
-	iommu_write(iommu, IOMMU_RESET_REG, ~status);
+	resets = (status | l1_status | l2_status) & IOMMU_INT_MASTER_MASK;
+	iommu_write(iommu, IOMMU_RESET_REG, ~resets);
 	iommu_write(iommu, IOMMU_RESET_REG, IOMMU_RESET_RELEASE_ALL);
 
 	spin_unlock(&iommu->iommu_lock);
-- 
2.38.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] iommu/sun50i: Fix R/W permission check

[Jernej Skrabec]

Because driver has enum type permissions and iommu subsystem has bitmap
type, we have to be careful how check for combined read and write
permissions is done. In such case, we have to mask both permissions and
check that both are set at the same time.

Current code just masks both flags but doesn't check that both are set.
In short, it always sets R/W permission, regardles if requested
permissions were RO, WO or RW. Fix that.

Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/iommu/sun50i-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 38d1069cf383..135df6934a9e 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -271,7 +271,7 @@ static u32 sun50i_mk_pte(phys_addr_t page, int prot)
 	enum sun50i_iommu_aci aci;
 	u32 flags = 0;
 
-	if (prot & (IOMMU_READ | IOMMU_WRITE))
+	if ((prot & (IOMMU_READ | IOMMU_WRITE)) == (IOMMU_READ | IOMMU_WRITE))
 		aci = SUN50I_IOMMU_ACI_RD_WR;
 	else if (prot & IOMMU_READ)
 		aci = SUN50I_IOMMU_ACI_RD;
-- 
2.38.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] iommu/sun50i: Fix flush size

[Jernej Skrabec]

Function sun50i_table_flush() takes number of entries as an argument,
not number of bytes. Fix that mistake in sun50i_dte_get_page_table().

Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/iommu/sun50i-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 135df6934a9e..7c3b2ac552da 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -512,7 +512,7 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
 		sun50i_iommu_free_page_table(iommu, drop_pt);
 	}
 
-	sun50i_table_flush(sun50i_domain, page_table, PT_SIZE);
+	sun50i_table_flush(sun50i_domain, page_table, NUM_PT_ENTRIES);
 	sun50i_table_flush(sun50i_domain, dte_addr, 1);
 
 	return page_table;
-- 
2.38.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] iommu/sun50i: Invalidate iova at map and unmap

[Jernej Skrabec]

Mapped and unmapped iova addresses needs to be invalidated immediately
or otherwise they might or might not work when used by master device.

This was discovered when running video decoder conformity test with
Cedrus. Some videos were now and then decoded incorrectly and generated
page faults.

Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/iommu/sun50i-iommu.c | 51 ++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 7c3b2ac552da..21e47ce6946a 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -518,6 +518,53 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
 	return page_table;
 }
 
+static void sun50i_iommu_zap_iova(struct sun50i_iommu *iommu, unsigned long iova)
+{
+	unsigned long flags;
+	u32 reg;
+	int ret;
+
+	spin_lock_irqsave(&iommu->iommu_lock, flags);
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
+
+	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_REG, iova);
+	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_MASK_REG, GENMASK(11, 0));
+	iommu_write(iommu, IOMMU_TLB_IVLD_ENABLE_REG, IOMMU_TLB_IVLD_ENABLE_ENABLE);
+
+	ret = readl_poll_timeout_atomic(iommu->base + IOMMU_TLB_IVLD_ENABLE_REG,
+					reg, !reg, 1, 2000);
+	if (ret)
+		dev_warn(iommu->dev, "TLB invalidation timed out!\n");
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, IOMMU_AUTO_GATING_ENABLE);
+
+	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
+}
+
+static void sun50i_iommu_zap_ptw_cache(struct sun50i_iommu *iommu, unsigned long iova)
+{
+	unsigned long flags;
+	u32 reg;
+	int ret;
+
+	spin_lock_irqsave(&iommu->iommu_lock, flags);
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
+
+	iommu_write(iommu, IOMMU_PC_IVLD_ADDR_REG, iova);
+	iommu_write(iommu, IOMMU_PC_IVLD_ENABLE_REG, IOMMU_PC_IVLD_ENABLE_ENABLE);
+
+	ret = readl_poll_timeout_atomic(iommu->base + IOMMU_PC_IVLD_ENABLE_REG,
+					reg, !reg, 1, 2000);
+	if (ret)
+		dev_warn(iommu->dev, "PTW cache invalidation timed out!\n");
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, IOMMU_AUTO_GATING_ENABLE);
+
+	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
+}
+
 static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
 {
@@ -546,6 +593,8 @@ static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
 
 	*pte_addr = sun50i_mk_pte(paddr, prot);
 	sun50i_table_flush(sun50i_domain, pte_addr, 1);
+	sun50i_iommu_zap_iova(iommu, iova);
+	sun50i_iommu_zap_ptw_cache(iommu, iova);
 
 out:
 	return ret;
@@ -571,6 +620,8 @@ static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long iova
 
 	memset(pte_addr, 0, sizeof(*pte_addr));
 	sun50i_table_flush(sun50i_domain, pte_addr, 1);
+	sun50i_iommu_zap_iova(sun50i_domain->iommu, iova);
+	sun50i_iommu_zap_ptw_cache(sun50i_domain->iommu, iova);
 
 	return SZ_4K;
 }
-- 
2.38.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] iommu/sun50i: Invalidate iova in map and unmap callback

[Jernej Skrabec]

Mapped and unmapped iova addresses needs to be invalidated immediately
or otherwise they might or might not work when used by master or CPU.

This was discovered when running video decoder conformity test with
Cedrus. Some videos were now and then decoded incorrectly and generated
page faults.

Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/iommu/sun50i-iommu.c | 51 ++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 7c3b2ac552da..21e47ce6946a 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -518,6 +518,53 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
 	return page_table;
 }
 
+static void sun50i_iommu_zap_iova(struct sun50i_iommu *iommu, unsigned long iova)
+{
+	unsigned long flags;
+	u32 reg;
+	int ret;
+
+	spin_lock_irqsave(&iommu->iommu_lock, flags);
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
+
+	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_REG, iova);
+	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_MASK_REG, GENMASK(11, 0));
+	iommu_write(iommu, IOMMU_TLB_IVLD_ENABLE_REG, IOMMU_TLB_IVLD_ENABLE_ENABLE);
+
+	ret = readl_poll_timeout_atomic(iommu->base + IOMMU_TLB_IVLD_ENABLE_REG,
+					reg, !reg, 1, 2000);
+	if (ret)
+		dev_warn(iommu->dev, "TLB invalidation timed out!\n");
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, IOMMU_AUTO_GATING_ENABLE);
+
+	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
+}
+
+static void sun50i_iommu_zap_ptw_cache(struct sun50i_iommu *iommu, unsigned long iova)
+{
+	unsigned long flags;
+	u32 reg;
+	int ret;
+
+	spin_lock_irqsave(&iommu->iommu_lock, flags);
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
+
+	iommu_write(iommu, IOMMU_PC_IVLD_ADDR_REG, iova);
+	iommu_write(iommu, IOMMU_PC_IVLD_ENABLE_REG, IOMMU_PC_IVLD_ENABLE_ENABLE);
+
+	ret = readl_poll_timeout_atomic(iommu->base + IOMMU_PC_IVLD_ENABLE_REG,
+					reg, !reg, 1, 2000);
+	if (ret)
+		dev_warn(iommu->dev, "PTW cache invalidation timed out!\n");
+
+	iommu_write(iommu, IOMMU_AUTO_GATING_REG, IOMMU_AUTO_GATING_ENABLE);
+
+	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
+}
+
 static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
 {
@@ -546,6 +593,8 @@ static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
 
 	*pte_addr = sun50i_mk_pte(paddr, prot);
 	sun50i_table_flush(sun50i_domain, pte_addr, 1);
+	sun50i_iommu_zap_iova(iommu, iova);
+	sun50i_iommu_zap_ptw_cache(iommu, iova);
 
 out:
 	return ret;
@@ -571,6 +620,8 @@ static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long iova
 
 	memset(pte_addr, 0, sizeof(*pte_addr));
 	sun50i_table_flush(sun50i_domain, pte_addr, 1);
+	sun50i_iommu_zap_iova(sun50i_domain->iommu, iova);
+	sun50i_iommu_zap_ptw_cache(sun50i_domain->iommu, iova);
 
 	return SZ_4K;
 }
-- 
2.38.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] iommu/sun50i: Invalidate iova at map and unmap

[Jernej Škrabec]

Dne četrtek, 13. oktober 2022 ob 20:12:20 CEST je Jernej Skrabec napisal(a):
> Mapped and unmapped iova addresses needs to be invalidated immediately
> or otherwise they might or might not work when used by master device.
> 
> This was discovered when running video decoder conformity test with
> Cedrus. Some videos were now and then decoded incorrectly and generated
> page faults.
> 
> Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Please ignore this patch. It's same as next one, with slightly different commit 
message.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] iommu/sun50i: Invalidate iova in map and unmap callback

[Robin Murphy]

On 2022-10-13 19:12, Jernej Skrabec wrote:
> Mapped and unmapped iova addresses needs to be invalidated immediately
> or otherwise they might or might not work when used by master or CPU.
> 
> This was discovered when running video decoder conformity test with
> Cedrus. Some videos were now and then decoded incorrectly and generated
> page faults.
> 
> Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/iommu/sun50i-iommu.c | 51 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> index 7c3b2ac552da..21e47ce6946a 100644
> --- a/drivers/iommu/sun50i-iommu.c
> +++ b/drivers/iommu/sun50i-iommu.c
> @@ -518,6 +518,53 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
>   	return page_table;
>   }
>   
> +static void sun50i_iommu_zap_iova(struct sun50i_iommu *iommu, unsigned long iova)
> +{
> +	unsigned long flags;
> +	u32 reg;
> +	int ret;
> +
> +	spin_lock_irqsave(&iommu->iommu_lock, flags);
> +
> +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
> +
> +	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_REG, iova);
> +	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_MASK_REG, GENMASK(11, 0));
> +	iommu_write(iommu, IOMMU_TLB_IVLD_ENABLE_REG, IOMMU_TLB_IVLD_ENABLE_ENABLE);
> +
> +	ret = readl_poll_timeout_atomic(iommu->base + IOMMU_TLB_IVLD_ENABLE_REG,
> +					reg, !reg, 1, 2000);
> +	if (ret)
> +		dev_warn(iommu->dev, "TLB invalidation timed out!\n");
> +
> +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, IOMMU_AUTO_GATING_ENABLE);
> +
> +	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
> +}
> +
> +static void sun50i_iommu_zap_ptw_cache(struct sun50i_iommu *iommu, unsigned long iova)
> +{
> +	unsigned long flags;
> +	u32 reg;
> +	int ret;
> +
> +	spin_lock_irqsave(&iommu->iommu_lock, flags);
> +
> +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
> +
> +	iommu_write(iommu, IOMMU_PC_IVLD_ADDR_REG, iova);
> +	iommu_write(iommu, IOMMU_PC_IVLD_ENABLE_REG, IOMMU_PC_IVLD_ENABLE_ENABLE);
> +
> +	ret = readl_poll_timeout_atomic(iommu->base + IOMMU_PC_IVLD_ENABLE_REG,
> +					reg, !reg, 1, 2000);
> +	if (ret)
> +		dev_warn(iommu->dev, "PTW cache invalidation timed out!\n");
> +
> +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, IOMMU_AUTO_GATING_ENABLE);
> +
> +	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
> +}
> +
>   static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
>   			    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
>   {
> @@ -546,6 +593,8 @@ static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
>   
>   	*pte_addr = sun50i_mk_pte(paddr, prot);
>   	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> +	sun50i_iommu_zap_iova(iommu, iova);
> +	sun50i_iommu_zap_ptw_cache(iommu, iova);

Consider hooking up .sync_map if you need that behaviour. I'd guess the 
address/mask combination allows invalidating multiple pages at once, 
which would be a heck of a lot more efficient.

In principle we probably shouldn't need walk cache maintenance for just 
changing leaf entries, so that could perhaps be pushed further down into 
sun50i_dte_get_page_table().

>   out:
>   	return ret;
> @@ -571,6 +620,8 @@ static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long iova
>   
>   	memset(pte_addr, 0, sizeof(*pte_addr));
>   	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> +	sun50i_iommu_zap_iova(sun50i_domain->iommu, iova);
> +	sun50i_iommu_zap_ptw_cache(sun50i_domain->iommu, iova);

Hmm, we already have .iotlb_sync hooked up for this, so at best adding 
more maintenance here is simply redundant, but at worst it would be 
papering over some bug in sun50i_iommu_iotlb_sync() - if unmaps really 
aren't working properly then that wants fixing instead. Of course it 
could also be enhanced to use the gather mechanism to perform more 
selective invalidations, but that's another patch in its own right.

Thanks,
Robin.

>   
>   	return SZ_4K;
>   }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Re: [PATCH 5/5] iommu/sun50i: Invalidate iova in map and unmap callback

[Jernej Škrabec]

Dne petek, 14. oktober 2022 ob 12:23:25 CEST je Robin Murphy napisal(a):
> On 2022-10-13 19:12, Jernej Skrabec wrote:
> > Mapped and unmapped iova addresses needs to be invalidated immediately
> > or otherwise they might or might not work when used by master or CPU.
> > 
> > This was discovered when running video decoder conformity test with
> > Cedrus. Some videos were now and then decoded incorrectly and generated
> > page faults.
> > 
> > Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> > 
> >   drivers/iommu/sun50i-iommu.c | 51 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 51 insertions(+)
> > 
> > diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> > index 7c3b2ac552da..21e47ce6946a 100644
> > --- a/drivers/iommu/sun50i-iommu.c
> > +++ b/drivers/iommu/sun50i-iommu.c
> > @@ -518,6 +518,53 @@ static u32 *sun50i_dte_get_page_table(struct
> > sun50i_iommu_domain *sun50i_domain,> 
> >   	return page_table;
> >   
> >   }
> > 
> > +static void sun50i_iommu_zap_iova(struct sun50i_iommu *iommu, unsigned
> > long iova) +{
> > +	unsigned long flags;
> > +	u32 reg;
> > +	int ret;
> > +
> > +	spin_lock_irqsave(&iommu->iommu_lock, flags);
> > +
> > +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
> > +
> > +	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_REG, iova);
> > +	iommu_write(iommu, IOMMU_TLB_IVLD_ADDR_MASK_REG, GENMASK(11, 0));
> > +	iommu_write(iommu, IOMMU_TLB_IVLD_ENABLE_REG,
> > IOMMU_TLB_IVLD_ENABLE_ENABLE); +
> > +	ret = readl_poll_timeout_atomic(iommu->base + 
IOMMU_TLB_IVLD_ENABLE_REG,
> > +					reg, !reg, 1, 2000);
> > +	if (ret)
> > +		dev_warn(iommu->dev, "TLB invalidation timed out!\n");
> > +
> > +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 
IOMMU_AUTO_GATING_ENABLE);
> > +
> > +	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
> > +}
> > +
> > +static void sun50i_iommu_zap_ptw_cache(struct sun50i_iommu *iommu,
> > unsigned long iova) +{
> > +	unsigned long flags;
> > +	u32 reg;
> > +	int ret;
> > +
> > +	spin_lock_irqsave(&iommu->iommu_lock, flags);
> > +
> > +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 0);
> > +
> > +	iommu_write(iommu, IOMMU_PC_IVLD_ADDR_REG, iova);
> > +	iommu_write(iommu, IOMMU_PC_IVLD_ENABLE_REG,
> > IOMMU_PC_IVLD_ENABLE_ENABLE); +
> > +	ret = readl_poll_timeout_atomic(iommu->base + 
IOMMU_PC_IVLD_ENABLE_REG,
> > +					reg, !reg, 1, 2000);
> > +	if (ret)
> > +		dev_warn(iommu->dev, "PTW cache invalidation timed out!
\n");
> > +
> > +	iommu_write(iommu, IOMMU_AUTO_GATING_REG, 
IOMMU_AUTO_GATING_ENABLE);
> > +
> > +	spin_unlock_irqrestore(&iommu->iommu_lock, flags);
> > +}
> > +
> > 
> >   static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long
> >   iova,>   
> >   			    phys_addr_t paddr, size_t size, int 
prot, gfp_t gfp)
> >   
> >   {
> > 
> > @@ -546,6 +593,8 @@ static int sun50i_iommu_map(struct iommu_domain
> > *domain, unsigned long iova,> 
> >   	*pte_addr = sun50i_mk_pte(paddr, prot);
> >   	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> > 
> > +	sun50i_iommu_zap_iova(iommu, iova);
> > +	sun50i_iommu_zap_ptw_cache(iommu, iova);
> 
> Consider hooking up .sync_map if you need that behaviour. I'd guess the
> address/mask combination allows invalidating multiple pages at once,
> which would be a heck of a lot more efficient.
> 
> In principle we probably shouldn't need walk cache maintenance for just
> changing leaf entries, so that could perhaps be pushed further down into
> sun50i_dte_get_page_table().

Note that this is my first foray into iommu and sun50i-iommu documentation is 
confusing to say the least (it has english words in it, but their combination 
often doesn't make sense.) 

I'll try that, thanks. Without this invalidation, handing buffer between two 
iommu supported peripherals works, but CPU access often doesn't. PTW cache can 
be invalidated only one by one. It's TLB invalidation that has mask.

> 
> >   out:
> >   	return ret;
> > 
> > @@ -571,6 +620,8 @@ static size_t sun50i_iommu_unmap(struct iommu_domain
> > *domain, unsigned long iova> 
> >   	memset(pte_addr, 0, sizeof(*pte_addr));
> >   	sun50i_table_flush(sun50i_domain, pte_addr, 1);
> > 
> > +	sun50i_iommu_zap_iova(sun50i_domain->iommu, iova);
> > +	sun50i_iommu_zap_ptw_cache(sun50i_domain->iommu, iova);
> 
> Hmm, we already have .iotlb_sync hooked up for this, so at best adding
> more maintenance here is simply redundant, but at worst it would be
> papering over some bug in sun50i_iommu_iotlb_sync() - if unmaps really
> aren't working properly then that wants fixing instead. Of course it
> could also be enhanced to use the gather mechanism to perform more
> selective invalidations, but that's another patch in its own right.

.iotlb_sync assumes that flush operation will to the same thing as invalidation 
of each entry separately. It obviously doesn't, as my testing shows. I'll 
rewrite .iotlb_sync to do invalidation instead of flush and check if that 
works.

I have two questions:
1. documentation says it's mandatory to do TLB and PTW invalidation in 
interrupt handler when page fault occurs. Do you see a reason for that?
2. vendor driver and other iommu drivers have spin lock guards across whole 
.iova_to_phys, .map and .unmap functions. Should I add them here too?

Best regards,
Jernej

> 
> Thanks,
> Robin.
> 
> >   	return SZ_4K;
> >   
> >   }



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel