[PATCH v3 0/2] iommu: rockchip: Fix directory table address encoding

[PATCH v3 0/2] iommu: rockchip: Fix directory table address encoding

[Jonas Karlman]

The address to the directory table is currently encoded using an
incorrect bit layout when configured into the DTE_ADDR reg on IOMMU v2.

This currently do not cause any issue because the directory and page
tables is allocated in memory below 4GB thanks to the use of the
GFP_DMA32 flag.

Testing has shown that the directory table address should be encoded
using the same bit layout as the page table and memory page addresses.

Only removing the GFP_DMA32 on a RK3568 device with 8GB of memory will
result in a page fault similar to:

[    0.907236] rk_iommu fe043e00.iommu: Page fault at 0x00000000ff801000 of type read
[    0.907264] rk_iommu fe043e00.iommu: iova = 0x00000000ff801000: dte_index: 0x3fe pte_index: 0x1 page_offset: 0x0
[    0.907281] rk_iommu fe043e00.iommu: mmu_dte_addr: 0x000000010189a000 dte@0x000000010189aff8: 0x1722101 valid: 1 pte@0x0000000101722004: 0x2c01107 valid: 1 page@0x0000000102c01000 flags: 0x106

This series fixes this by using the existing mk_dtentries instead of the
dma_addr_dte ops to encode the directory table address, removes unused
ops and finally removes the GFP_DMA32 flag to allow for directory and
page tables to be allocated in memory above 4GB on IOMMU v2.

Changes in v3:
- merge patch 1 and 2
- only remove GFP_DMA32 flag for IOMMU v2

Changes in v2:
- no changes, rebased on next-20230615

This series can also be found at [1].

[1] https://github.com/Kwiboo/linux-rockchip/commits/next-20230616-iommu

Jonas Karlman (2):
  iommu: rockchip: Fix directory table address encoding
  iommu: rockchip: Allocate tables from all available memory for IOMMU
    v2

 drivers/iommu/rockchip-iommu.c | 50 +++++++---------------------------
 1 file changed, 10 insertions(+), 40 deletions(-)

-- 
2.40.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 1/2] iommu: rockchip: Fix directory table address encoding

[Jonas Karlman]

The physical address to the directory table is currently encoded using
the following bit layout for IOMMU v2.

 31:12 - Address bit 31:0
 11: 4 - Address bit 39:32

This is also the bit layout used by the vendor kernel.

However, testing has shown that addresses to the directory/page tables
and memory pages are all encoded using the same bit layout.

IOMMU v1:
 31:12 - Address bit 31:0

IOMMU v2:
 31:12 - Address bit 31:0
 11: 8 - Address bit 35:32
  7: 4 - Address bit 39:36

Change to use the mk_dtentries ops to encode the directory table address
correctly. The value written to DTE_ADDR may include the valid bit set,
a bit that is ignored and DTE_ADDR reg read it back as 0.

This also update the bit layout comment for the page address and the
number of nybbles that are read back for DTE_ADDR comment.

These changes render the dte_addr_phys and dma_addr_dte ops unused and
is removed.

Fixes: 227014b33f62 ("iommu: rockchip: Add internal ops to handle variants")
Fixes: c55356c534aa ("iommu: rockchip: Add support for iommu v2")
Fixes: c987b65a574f ("iommu/rockchip: Fix physical address decoding")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v3:
- squash removal of unused ops into this patch
- update commit message

v2:
- replace currently with correctly in commit message

 drivers/iommu/rockchip-iommu.c | 43 ++++------------------------------
 1 file changed, 5 insertions(+), 38 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 4054030c3237..ae42959bc490 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -98,8 +98,6 @@ struct rk_iommu_ops {
 	phys_addr_t (*pt_address)(u32 dte);
 	u32 (*mk_dtentries)(dma_addr_t pt_dma);
 	u32 (*mk_ptentries)(phys_addr_t page, int prot);
-	phys_addr_t (*dte_addr_phys)(u32 addr);
-	u32 (*dma_addr_dte)(dma_addr_t dt_dma);
 	u64 dma_bit_mask;
 };
 
@@ -278,8 +276,8 @@ static u32 rk_mk_pte(phys_addr_t page, int prot)
 /*
  * In v2:
  * 31:12 - Page address bit 31:0
- *  11:9 - Page address bit 34:32
- *   8:4 - Page address bit 39:35
+ * 11: 8 - Page address bit 35:32
+ *  7: 4 - Page address bit 39:36
  *     3 - Security
  *     2 - Writable
  *     1 - Readable
@@ -506,7 +504,7 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu)
 
 	/*
 	 * Check if register DTE_ADDR is working by writing DTE_ADDR_DUMMY
-	 * and verifying that upper 5 nybbles are read back.
+	 * and verifying that upper 5 (v1) or 7 (v2) nybbles are read back.
 	 */
 	for (i = 0; i < iommu->num_mmu; i++) {
 		dte_addr = rk_ops->pt_address(DTE_ADDR_DUMMY);
@@ -531,33 +529,6 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu)
 	return 0;
 }
 
-static inline phys_addr_t rk_dte_addr_phys(u32 addr)
-{
-	return (phys_addr_t)addr;
-}
-
-static inline u32 rk_dma_addr_dte(dma_addr_t dt_dma)
-{
-	return dt_dma;
-}
-
-#define DT_HI_MASK GENMASK_ULL(39, 32)
-#define DTE_BASE_HI_MASK GENMASK(11, 4)
-#define DT_SHIFT   28
-
-static inline phys_addr_t rk_dte_addr_phys_v2(u32 addr)
-{
-	u64 addr64 = addr;
-	return (phys_addr_t)(addr64 & RK_DTE_PT_ADDRESS_MASK) |
-	       ((addr64 & DTE_BASE_HI_MASK) << DT_SHIFT);
-}
-
-static inline u32 rk_dma_addr_dte_v2(dma_addr_t dt_dma)
-{
-	return (dt_dma & RK_DTE_PT_ADDRESS_MASK) |
-	       ((dt_dma & DT_HI_MASK) >> DT_SHIFT);
-}
-
 static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova)
 {
 	void __iomem *base = iommu->bases[index];
@@ -577,7 +548,7 @@ static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova)
 	page_offset = rk_iova_page_offset(iova);
 
 	mmu_dte_addr = rk_iommu_read(base, RK_MMU_DTE_ADDR);
-	mmu_dte_addr_phys = rk_ops->dte_addr_phys(mmu_dte_addr);
+	mmu_dte_addr_phys = rk_ops->pt_address(mmu_dte_addr);
 
 	dte_addr_phys = mmu_dte_addr_phys + (4 * dte_index);
 	dte_addr = phys_to_virt(dte_addr_phys);
@@ -967,7 +938,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu)
 
 	for (i = 0; i < iommu->num_mmu; i++) {
 		rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
-			       rk_ops->dma_addr_dte(rk_domain->dt_dma));
+			       rk_ops->mk_dtentries(rk_domain->dt_dma));
 		rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE);
 		rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK);
 	}
@@ -1405,8 +1376,6 @@ static struct rk_iommu_ops iommu_data_ops_v1 = {
 	.pt_address = &rk_dte_pt_address,
 	.mk_dtentries = &rk_mk_dte,
 	.mk_ptentries = &rk_mk_pte,
-	.dte_addr_phys = &rk_dte_addr_phys,
-	.dma_addr_dte = &rk_dma_addr_dte,
 	.dma_bit_mask = DMA_BIT_MASK(32),
 };
 
@@ -1414,8 +1383,6 @@ static struct rk_iommu_ops iommu_data_ops_v2 = {
 	.pt_address = &rk_dte_pt_address_v2,
 	.mk_dtentries = &rk_mk_dte_v2,
 	.mk_ptentries = &rk_mk_pte_v2,
-	.dte_addr_phys = &rk_dte_addr_phys_v2,
-	.dma_addr_dte = &rk_dma_addr_dte_v2,
 	.dma_bit_mask = DMA_BIT_MASK(40),
 };
 
-- 
2.40.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 1/2] iommu: rockchip: Fix directory table address encoding

[Robin Murphy]

On 17/06/2023 7:25 pm, Jonas Karlman wrote:
> The physical address to the directory table is currently encoded using
> the following bit layout for IOMMU v2.
> 
>   31:12 - Address bit 31:0
>   11: 4 - Address bit 39:32
> 
> This is also the bit layout used by the vendor kernel.
> 
> However, testing has shown that addresses to the directory/page tables
> and memory pages are all encoded using the same bit layout.
> 
> IOMMU v1:
>   31:12 - Address bit 31:0
> 
> IOMMU v2:
>   31:12 - Address bit 31:0
>   11: 8 - Address bit 35:32
>    7: 4 - Address bit 39:36
> 
> Change to use the mk_dtentries ops to encode the directory table address
> correctly. The value written to DTE_ADDR may include the valid bit set,
> a bit that is ignored and DTE_ADDR reg read it back as 0.
> 
> This also update the bit layout comment for the page address and the
> number of nybbles that are read back for DTE_ADDR comment.
> 
> These changes render the dte_addr_phys and dma_addr_dte ops unused and
> is removed.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Fixes: 227014b33f62 ("iommu: rockchip: Add internal ops to handle variants")
> Fixes: c55356c534aa ("iommu: rockchip: Add support for iommu v2")
> Fixes: c987b65a574f ("iommu/rockchip: Fix physical address decoding")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v3:
> - squash removal of unused ops into this patch
> - update commit message
> 
> v2:
> - replace currently with correctly in commit message
> 
>   drivers/iommu/rockchip-iommu.c | 43 ++++------------------------------
>   1 file changed, 5 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 4054030c3237..ae42959bc490 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -98,8 +98,6 @@ struct rk_iommu_ops {
>   	phys_addr_t (*pt_address)(u32 dte);
>   	u32 (*mk_dtentries)(dma_addr_t pt_dma);
>   	u32 (*mk_ptentries)(phys_addr_t page, int prot);
> -	phys_addr_t (*dte_addr_phys)(u32 addr);
> -	u32 (*dma_addr_dte)(dma_addr_t dt_dma);
>   	u64 dma_bit_mask;
>   };
>   
> @@ -278,8 +276,8 @@ static u32 rk_mk_pte(phys_addr_t page, int prot)
>   /*
>    * In v2:
>    * 31:12 - Page address bit 31:0
> - *  11:9 - Page address bit 34:32
> - *   8:4 - Page address bit 39:35
> + * 11: 8 - Page address bit 35:32
> + *  7: 4 - Page address bit 39:36
>    *     3 - Security
>    *     2 - Writable
>    *     1 - Readable
> @@ -506,7 +504,7 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu)
>   
>   	/*
>   	 * Check if register DTE_ADDR is working by writing DTE_ADDR_DUMMY
> -	 * and verifying that upper 5 nybbles are read back.
> +	 * and verifying that upper 5 (v1) or 7 (v2) nybbles are read back.
>   	 */
>   	for (i = 0; i < iommu->num_mmu; i++) {
>   		dte_addr = rk_ops->pt_address(DTE_ADDR_DUMMY);
> @@ -531,33 +529,6 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu)
>   	return 0;
>   }
>   
> -static inline phys_addr_t rk_dte_addr_phys(u32 addr)
> -{
> -	return (phys_addr_t)addr;
> -}
> -
> -static inline u32 rk_dma_addr_dte(dma_addr_t dt_dma)
> -{
> -	return dt_dma;
> -}
> -
> -#define DT_HI_MASK GENMASK_ULL(39, 32)
> -#define DTE_BASE_HI_MASK GENMASK(11, 4)
> -#define DT_SHIFT   28
> -
> -static inline phys_addr_t rk_dte_addr_phys_v2(u32 addr)
> -{
> -	u64 addr64 = addr;
> -	return (phys_addr_t)(addr64 & RK_DTE_PT_ADDRESS_MASK) |
> -	       ((addr64 & DTE_BASE_HI_MASK) << DT_SHIFT);
> -}
> -
> -static inline u32 rk_dma_addr_dte_v2(dma_addr_t dt_dma)
> -{
> -	return (dt_dma & RK_DTE_PT_ADDRESS_MASK) |
> -	       ((dt_dma & DT_HI_MASK) >> DT_SHIFT);
> -}
> -
>   static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova)
>   {
>   	void __iomem *base = iommu->bases[index];
> @@ -577,7 +548,7 @@ static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova)
>   	page_offset = rk_iova_page_offset(iova);
>   
>   	mmu_dte_addr = rk_iommu_read(base, RK_MMU_DTE_ADDR);
> -	mmu_dte_addr_phys = rk_ops->dte_addr_phys(mmu_dte_addr);
> +	mmu_dte_addr_phys = rk_ops->pt_address(mmu_dte_addr);
>   
>   	dte_addr_phys = mmu_dte_addr_phys + (4 * dte_index);
>   	dte_addr = phys_to_virt(dte_addr_phys);
> @@ -967,7 +938,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu)
>   
>   	for (i = 0; i < iommu->num_mmu; i++) {
>   		rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
> -			       rk_ops->dma_addr_dte(rk_domain->dt_dma));
> +			       rk_ops->mk_dtentries(rk_domain->dt_dma));
>   		rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE);
>   		rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK);
>   	}
> @@ -1405,8 +1376,6 @@ static struct rk_iommu_ops iommu_data_ops_v1 = {
>   	.pt_address = &rk_dte_pt_address,
>   	.mk_dtentries = &rk_mk_dte,
>   	.mk_ptentries = &rk_mk_pte,
> -	.dte_addr_phys = &rk_dte_addr_phys,
> -	.dma_addr_dte = &rk_dma_addr_dte,
>   	.dma_bit_mask = DMA_BIT_MASK(32),
>   };
>   
> @@ -1414,8 +1383,6 @@ static struct rk_iommu_ops iommu_data_ops_v2 = {
>   	.pt_address = &rk_dte_pt_address_v2,
>   	.mk_dtentries = &rk_mk_dte_v2,
>   	.mk_ptentries = &rk_mk_pte_v2,
> -	.dte_addr_phys = &rk_dte_addr_phys_v2,
> -	.dma_addr_dte = &rk_dma_addr_dte_v2,
>   	.dma_bit_mask = DMA_BIT_MASK(40),
>   };
>   

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 2/2] iommu: rockchip: Allocate tables from all available memory for IOMMU v2

[Jonas Karlman]

IOMMU v2 found in newer Rockchip SoCs, e.g. RK356x and RK3588, support
placing directory and page tables in up to 40-bit addressable physical
memory.

Remove the use of GFP_DMA32 flag for IOMMU v2 now that the physical
address to the directory table is correctly written to DTE_ADDR.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v3:
- rework to only affect IOMMU v2

v2:
- no change

 drivers/iommu/rockchip-iommu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index ae42959bc490..8ff69fbf9f65 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -99,6 +99,7 @@ struct rk_iommu_ops {
 	u32 (*mk_dtentries)(dma_addr_t pt_dma);
 	u32 (*mk_ptentries)(phys_addr_t page, int prot);
 	u64 dma_bit_mask;
+	gfp_t gfp_flags;
 };
 
 struct rk_iommu {
@@ -727,7 +728,7 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
 	if (rk_dte_is_pt_valid(dte))
 		goto done;
 
-	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | GFP_DMA32);
+	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | rk_ops->gfp_flags);
 	if (!page_table)
 		return ERR_PTR(-ENOMEM);
 
@@ -1076,7 +1077,7 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
 	 * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
 	 * Allocate one 4 KiB page for each table.
 	 */
-	rk_domain->dt = (u32 *)get_zeroed_page(GFP_KERNEL | GFP_DMA32);
+	rk_domain->dt = (u32 *)get_zeroed_page(GFP_KERNEL | rk_ops->gfp_flags);
 	if (!rk_domain->dt)
 		goto err_free_domain;
 
@@ -1377,6 +1378,7 @@ static struct rk_iommu_ops iommu_data_ops_v1 = {
 	.mk_dtentries = &rk_mk_dte,
 	.mk_ptentries = &rk_mk_pte,
 	.dma_bit_mask = DMA_BIT_MASK(32),
+	.gfp_flags = GFP_DMA32,
 };
 
 static struct rk_iommu_ops iommu_data_ops_v2 = {
@@ -1384,6 +1386,7 @@ static struct rk_iommu_ops iommu_data_ops_v2 = {
 	.mk_dtentries = &rk_mk_dte_v2,
 	.mk_ptentries = &rk_mk_pte_v2,
 	.dma_bit_mask = DMA_BIT_MASK(40),
+	.gfp_flags = 0,
 };
 
 static const struct of_device_id rk_iommu_dt_ids[] = {
-- 
2.40.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 2/2] iommu: rockchip: Allocate tables from all available memory for IOMMU v2

[Robin Murphy]

On 17/06/2023 7:25 pm, Jonas Karlman wrote:
> IOMMU v2 found in newer Rockchip SoCs, e.g. RK356x and RK3588, support
> placing directory and page tables in up to 40-bit addressable physical
> memory.
> 
> Remove the use of GFP_DMA32 flag for IOMMU v2 now that the physical
> address to the directory table is correctly written to DTE_ADDR.

FWIW I'd be tempted to refactor a bit harder since this is closely 
coupled to the DMA mask and both could be calculated from a single data 
value, but there's absolutely nothing wrong with this approach either.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

[ In fact if you start down that rabbit-hole, then I think logically it 
leads to an even bigger refactor to convert the whole lot to use 
dma_alloc_pages() instead ]

> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v3:
> - rework to only affect IOMMU v2
> 
> v2:
> - no change
> 
>   drivers/iommu/rockchip-iommu.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index ae42959bc490..8ff69fbf9f65 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -99,6 +99,7 @@ struct rk_iommu_ops {
>   	u32 (*mk_dtentries)(dma_addr_t pt_dma);
>   	u32 (*mk_ptentries)(phys_addr_t page, int prot);
>   	u64 dma_bit_mask;
> +	gfp_t gfp_flags;
>   };
>   
>   struct rk_iommu {
> @@ -727,7 +728,7 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
>   	if (rk_dte_is_pt_valid(dte))
>   		goto done;
>   
> -	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | GFP_DMA32);
> +	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | rk_ops->gfp_flags);
>   	if (!page_table)
>   		return ERR_PTR(-ENOMEM);
>   
> @@ -1076,7 +1077,7 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
>   	 * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
>   	 * Allocate one 4 KiB page for each table.
>   	 */
> -	rk_domain->dt = (u32 *)get_zeroed_page(GFP_KERNEL | GFP_DMA32);
> +	rk_domain->dt = (u32 *)get_zeroed_page(GFP_KERNEL | rk_ops->gfp_flags);
>   	if (!rk_domain->dt)
>   		goto err_free_domain;
>   
> @@ -1377,6 +1378,7 @@ static struct rk_iommu_ops iommu_data_ops_v1 = {
>   	.mk_dtentries = &rk_mk_dte,
>   	.mk_ptentries = &rk_mk_pte,
>   	.dma_bit_mask = DMA_BIT_MASK(32),
> +	.gfp_flags = GFP_DMA32,
>   };
>   
>   static struct rk_iommu_ops iommu_data_ops_v2 = {
> @@ -1384,6 +1386,7 @@ static struct rk_iommu_ops iommu_data_ops_v2 = {
>   	.mk_dtentries = &rk_mk_dte_v2,
>   	.mk_ptentries = &rk_mk_pte_v2,
>   	.dma_bit_mask = DMA_BIT_MASK(40),
> +	.gfp_flags = 0,
>   };
>   
>   static const struct of_device_id rk_iommu_dt_ids[] = {

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 0/2] iommu: rockchip: Fix directory table address encoding

[Joerg Roedel]

On Sat, Jun 17, 2023 at 06:25:43PM +0000, Jonas Karlman wrote:
> Jonas Karlman (2):
>   iommu: rockchip: Fix directory table address encoding
>   iommu: rockchip: Allocate tables from all available memory for IOMMU
>     v2
> 
>  drivers/iommu/rockchip-iommu.c | 50 +++++++---------------------------
>  1 file changed, 10 insertions(+), 40 deletions(-)

Applied, thanks.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip