[PATCH v2 0/3] iommu: rockchip: Fix discovery table address encoding

[PATCH v2 0/3] iommu: rockchip: Fix discovery table address encoding

[Jonas Karlman]

This is a re-send of a series sent out in January, see [1].

The address to the discovery table is currently encoded using an
incorrect bit layout when configured into the MMU_DTE_ADDR reg.

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

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

Removing the GFP_DMA32 on a RK3568 with 8GB of memory will result in:

[    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 discovery table address, removes unused
ops and finally removes the GFP_DMA32 flag to allow for discovery and
page tables to be allocated in memory above 4GB.

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

This series can also be found at [2].

[1] https://lore.kernel.org/all/20230125221809.3275481-1-jonas@kwiboo.se/
[2] https://github.com/Kwiboo/linux-rockchip/commits/next-20230615-iommu

Jonas Karlman (3):
  iommu: rockchip: Fix discovery table address encoding
  iommu: rockchip: Remove unused variant ops
  iommu: rockchip: Allocate tables from all available memory

 drivers/iommu/rockchip-iommu.c | 45 +++++-----------------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] iommu: rockchip: Fix discovery table address encoding

[Jonas Karlman]

The address to the discovery table is currently encoded using the
following bit layout.

 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 discovery/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 discovery table address
correctly. Also update the bit layout comment for the page address.

These changes render the dte_addr_phys and dma_addr_dte ops unused
and will be removed in a following patch.

Fixes: 227014b33f62 ("iommu: rockchip: Add internal ops to handle variants")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- replace currently with correctly in commit message

 drivers/iommu/rockchip-iommu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 4054030c3237..d46319f77e5c 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -278,8 +278,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
@@ -577,7 +577,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 +967,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);
 	}
-- 
2.40.1

[PATCH v2 2/3] iommu: rockchip: Remove unused variant ops

[Jonas Karlman]

The dte_addr_phys and dma_addr_dte ops is now unused by the driver,
let's remove them.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- no change

 drivers/iommu/rockchip-iommu.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index d46319f77e5c..62be9bf42390 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;
 };
 
@@ -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];
@@ -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

[PATCH v2 3/3] iommu: rockchip: Allocate tables from all available memory

[Jonas Karlman]

Newer Rockchip SoCs, RK356x and RK3588, support more then 4GB of memory.
However, the RK IOMMU driver is using the GFP_DMA32 flag to limit
allocation of the discovery and page tables into memory below 4GB.

Let's remove this limitation now that the discovery table address is
correctly configured for addresses above 4GB.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- no change

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

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 62be9bf42390..46498fc382ee 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -727,7 +727,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);
 	if (!page_table)
 		return ERR_PTR(-ENOMEM);
 
@@ -1076,7 +1076,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);
 	if (!rk_domain->dt)
 		goto err_free_domain;
 
-- 
2.40.1

Re: [PATCH v2 1/3] iommu: rockchip: Fix discovery table address encoding

[Robin Murphy]

On 2023-06-15 21:10, Jonas Karlman wrote:
> The address to the discovery table is currently encoded using the
> following bit layout.
> 
>   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 discovery/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 discovery table address

Nit: s/discovery/directory/g

> correctly. Also update the bit layout comment for the page address.
> 
> These changes render the dte_addr_phys and dma_addr_dte ops unused
> and will be removed in a following patch.

TBH I'd just squash that into this patch - we don't gain anything from 
leaving dead code in stable kernels, and at worst it just stands to make 
future fixes harder to backport.

> Fixes: 227014b33f62 ("iommu: rockchip: Add internal ops to handle variants")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v2:
> - replace currently with correctly in commit message
> 
>   drivers/iommu/rockchip-iommu.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 4054030c3237..d46319f77e5c 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -278,8 +278,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
> @@ -577,7 +577,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 +967,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));

Hmm, this writes the RK_DTE_PT_VALID bit into the register as well - 
does that really make sense?

Thanks,
Robin.

>   		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);
>   	}

Re: [PATCH v2 3/3] iommu: rockchip: Allocate tables from all available memory

[Robin Murphy]

On 2023-06-15 21:10, Jonas Karlman wrote:
> Newer Rockchip SoCs, RK356x and RK3588, support more then 4GB of memory.

Older ones supposedly did too, e.g. commit 79db45be2b8b ("ARM: dts: 
rockchip: convert rk3288 device tree files to 64 bits"). Are we certain 
that nobody actually has a system with IOMMUv1 and more than 4GB of RAM?

> However, the RK IOMMU driver is using the GFP_DMA32 flag to limit
> allocation of the discovery and page tables into memory below 4GB.
> 
> Let's remove this limitation now that the discovery table address is

Nit: s/discovery/directory/g again

Thanks,
Robin.

> correctly configured for addresses above 4GB.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v2:
> - no change
> 
>   drivers/iommu/rockchip-iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 62be9bf42390..46498fc382ee 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -727,7 +727,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);
>   	if (!page_table)
>   		return ERR_PTR(-ENOMEM);
>   
> @@ -1076,7 +1076,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);
>   	if (!rk_domain->dt)
>   		goto err_free_domain;
>   

Re: [PATCH v2 1/3] iommu: rockchip: Fix discovery table address encoding

[Jonas Karlman]

On 2023-06-15 23:24, Robin Murphy wrote:
> On 2023-06-15 21:10, Jonas Karlman wrote:
>> The address to the discovery table is currently encoded using the
>> following bit layout.
>>
>>   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 discovery/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 discovery table address
> 
> Nit: s/discovery/directory/g

Oops, will fix in entire series in a v3 :-)

> 
>> correctly. Also update the bit layout comment for the page address.
>>
>> These changes render the dte_addr_phys and dma_addr_dte ops unused
>> and will be removed in a following patch.
> 
> TBH I'd just squash that into this patch - we don't gain anything from 
> leaving dead code in stable kernels, and at worst it just stands to make 
> future fixes harder to backport.

Make sense, will squash them in v3.

> 
>> Fixes: 227014b33f62 ("iommu: rockchip: Add internal ops to handle variants")
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v2:
>> - replace currently with correctly in commit message
>>
>>   drivers/iommu/rockchip-iommu.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>> index 4054030c3237..d46319f77e5c 100644
>> --- a/drivers/iommu/rockchip-iommu.c
>> +++ b/drivers/iommu/rockchip-iommu.c
>> @@ -278,8 +278,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
>> @@ -577,7 +577,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 +967,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));
> 
> Hmm, this writes the RK_DTE_PT_VALID bit into the register as well - 
> does that really make sense?

On v1 bit 11:0 behave as read-only and on v2 bit 3:0 behave read-only.
Writing anything to those bits read back as 0, so was safe to write
RK_DTE_PT_VALID. Should probably mention this in a comment in v3.

Regards,
Jonas

> 
> Thanks,
> Robin.
> 
>>   		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);
>>   	}

Re: [PATCH v2 3/3] iommu: rockchip: Allocate tables from all available memory

[Jonas Karlman]

On 2023-06-15 23:25, Robin Murphy wrote:
> On 2023-06-15 21:10, Jonas Karlman wrote:
>> Newer Rockchip SoCs, RK356x and RK3588, support more then 4GB of memory.
> 
> Older ones supposedly did too, e.g. commit 79db45be2b8b ("ARM: dts: 
> rockchip: convert rk3288 device tree files to 64 bits"). Are we certain 
> that nobody actually has a system with IOMMUv1 and more than 4GB of RAM?

In IOMMU v1 bit 11:0 read back as 0 from MMU_DTE_ADDR reg, so I expect
that the old limit for v1 is 4GB. I will reword this to focus on IOMMU
v1 vs v2 instead of SoCs in v3.

> 
>> However, the RK IOMMU driver is using the GFP_DMA32 flag to limit
>> allocation of the discovery and page tables into memory below 4GB.
>>
>> Let's remove this limitation now that the discovery table address is
> 
> Nit: s/discovery/directory/g again

Will fix in v3 :-)

Regards,
Jonas

> 
> Thanks,
> Robin.
> 
>> correctly configured for addresses above 4GB.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v2:
>> - no change
>>
>>   drivers/iommu/rockchip-iommu.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>> index 62be9bf42390..46498fc382ee 100644
>> --- a/drivers/iommu/rockchip-iommu.c
>> +++ b/drivers/iommu/rockchip-iommu.c
>> @@ -727,7 +727,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);
>>   	if (!page_table)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> @@ -1076,7 +1076,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);
>>   	if (!rk_domain->dt)
>>   		goto err_free_domain;
>>   

Re: [PATCH v2 3/3] iommu: rockchip: Allocate tables from all available memory

[Robin Murphy]

On 2023-06-15 23:26, Jonas Karlman wrote:
> On 2023-06-15 23:25, Robin Murphy wrote:
>> On 2023-06-15 21:10, Jonas Karlman wrote:
>>> Newer Rockchip SoCs, RK356x and RK3588, support more then 4GB of memory.
>>
>> Older ones supposedly did too, e.g. commit 79db45be2b8b ("ARM: dts:
>> rockchip: convert rk3288 device tree files to 64 bits"). Are we certain
>> that nobody actually has a system with IOMMUv1 and more than 4GB of RAM?
> 
> In IOMMU v1 bit 11:0 read back as 0 from MMU_DTE_ADDR reg, so I expect
> that the old limit for v1 is 4GB.

Right, that was my point, sorry if it was unclear - on at least RK3288, 
the *SoC* apparently supports RAM above 4GB, even if it's only the CPUs 
that can access it. GFP_DMA32 technically *is* correct and appropriate 
for IOMMUv1, so it's only safe to unconditionally remove it if we're 
sure that, on all relevant IOMMUv1 SoCs in practice, no RAM above 4GB 
exists such that ZONE_NORMAL is empty and all allocations come from 
ZONE_DMA32 by default anyway.

(in truth this only matters for 64-bit SoCs, since on 32-bit there is no 
ZONE_DMA32, but any >4GB RAM would already have to be in ZONE_HIGHMEM, 
so safely out of scope)

Thanks,
Robin.

> I will reword this to focus on IOMMU
> v1 vs v2 instead of SoCs in v3.
> 
>>
>>> However, the RK IOMMU driver is using the GFP_DMA32 flag to limit
>>> allocation of the discovery and page tables into memory below 4GB.
>>>
>>> Let's remove this limitation now that the discovery table address is
>>
>> Nit: s/discovery/directory/g again
> 
> Will fix in v3 :-)
> 
> Regards,
> Jonas
> 
>>
>> Thanks,
>> Robin.
>>
>>> correctly configured for addresses above 4GB.
>>>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> v2:
>>> - no change
>>>
>>>    drivers/iommu/rockchip-iommu.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>>> index 62be9bf42390..46498fc382ee 100644
>>> --- a/drivers/iommu/rockchip-iommu.c
>>> +++ b/drivers/iommu/rockchip-iommu.c
>>> @@ -727,7 +727,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);
>>>    	if (!page_table)
>>>    		return ERR_PTR(-ENOMEM);
>>>    
>>> @@ -1076,7 +1076,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);
>>>    	if (!rk_domain->dt)
>>>    		goto err_free_domain;
>>>    
> 

Re: [PATCH v2 3/3] iommu: rockchip: Allocate tables from all available memory

[Jonas Karlman]

On 2023-06-16 12:58, Robin Murphy wrote:
> On 2023-06-15 23:26, Jonas Karlman wrote:
>> On 2023-06-15 23:25, Robin Murphy wrote:
>>> On 2023-06-15 21:10, Jonas Karlman wrote:
>>>> Newer Rockchip SoCs, RK356x and RK3588, support more then 4GB of memory.
>>>
>>> Older ones supposedly did too, e.g. commit 79db45be2b8b ("ARM: dts:
>>> rockchip: convert rk3288 device tree files to 64 bits"). Are we certain
>>> that nobody actually has a system with IOMMUv1 and more than 4GB of RAM?
>>
>> In IOMMU v1 bit 11:0 read back as 0 from MMU_DTE_ADDR reg, so I expect
>> that the old limit for v1 is 4GB.
> 
> Right, that was my point, sorry if it was unclear - on at least RK3288, 
> the *SoC* apparently supports RAM above 4GB, even if it's only the CPUs 
> that can access it. GFP_DMA32 technically *is* correct and appropriate 
> for IOMMUv1, so it's only safe to unconditionally remove it if we're 
> sure that, on all relevant IOMMUv1 SoCs in practice, no RAM above 4GB 
> exists such that ZONE_NORMAL is empty and all allocations come from 
> ZONE_DMA32 by default anyway.
> 
> (in truth this only matters for 64-bit SoCs, since on 32-bit there is no 
> ZONE_DMA32, but any >4GB RAM would already have to be in ZONE_HIGHMEM, 
> so safely out of scope)

Thanks for the insights!

I will keep the use of GFP_DMA32 flag on IOMMUv1 to be safe and only
remove it for IOMMUv2 in a v3 series.

Regards,
Jonas

> 
> Thanks,
> Robin.
> 
>> I will reword this to focus on IOMMU
>> v1 vs v2 instead of SoCs in v3.
>>
>>>
>>>> However, the RK IOMMU driver is using the GFP_DMA32 flag to limit
>>>> allocation of the discovery and page tables into memory below 4GB.
>>>>
>>>> Let's remove this limitation now that the discovery table address is
>>>
>>> Nit: s/discovery/directory/g again
>>
>> Will fix in v3 :-)
>>
>> Regards,
>> Jonas
>>
>>>
>>> Thanks,
>>> Robin.
>>>
>>>> correctly configured for addresses above 4GB.
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>> v2:
>>>> - no change
>>>>
>>>>    drivers/iommu/rockchip-iommu.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>>>> index 62be9bf42390..46498fc382ee 100644
>>>> --- a/drivers/iommu/rockchip-iommu.c
>>>> +++ b/drivers/iommu/rockchip-iommu.c
>>>> @@ -727,7 +727,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);
>>>>    	if (!page_table)
>>>>    		return ERR_PTR(-ENOMEM);
>>>>    
>>>> @@ -1076,7 +1076,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);
>>>>    	if (!rk_domain->dt)
>>>>    		goto err_free_domain;
>>>>    
>>