Re: [PATCH v8 1/3] iommu/io-pgtable-arm-v7s: Add a quirk to allow pgtable PA up to 35bit

On Tue, 2022-06-14 at 13:56 +0100, Will Deacon wrote:

Hi,

For some reason, this series has landed in my spam folder so
apologies
for the delay :/

quoted

+static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int lvl,
+				    struct io_pgtable_cfg *cfg)
+{
+	arm_v7s_iopte pte = paddr & ARM_V7S_LVL_MASK(lvl);
+
+	if (!arm_v7s_is_mtk_enabled(cfg))
+		return pte;
+
+	return to_iopte_mtk(paddr, pte);

nit, but can we rename and rework this so it reads a bit better,
please?
Something like:


	if (arm_v7s_is_mtk_enabled(cfg))
		return to_mtk_iopte(paddr, pte);

	return pte;

Hi Will,
Thanks for your suggestion, PATCH v9 version will modify it.

quoted

 static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl,
 				  struct io_pgtable_cfg *cfg)
 {

@@ -234,6 +239,7 @@ static arm_v7s_iopte *iopte_deref(arm_v7s_iopte

pte, int lvl,
 static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
 				   struct arm_v7s_io_pgtable *data)
 {
+	gfp_t gfp_l1 = __GFP_ZERO | ARM_V7S_TABLE_GFP_DMA;
 	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 	struct device *dev = cfg->iommu_dev;
 	phys_addr_t phys;

@@ -241,9 +247,11 @@ static void *__arm_v7s_alloc_table(int lvl,

gfp_t gfp,
 	size_t size = ARM_V7S_TABLE_SIZE(lvl, cfg);
 	void *table = NULL;
 
+	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT)
+		gfp_l1 = GFP_KERNEL | __GFP_ZERO;

I think it's a bit grotty to override the flags inline like this
(same for
the slab flag later on). Something like this is a bit cleaner:


	/*
	 * Comment explaining why GFP_KERNEL is desirable here.
	 * I'm assuming it's because the walker can address all of
memory.
	 */
	gfp_l1 = cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT ?
		 GFP_KERNEL : ARM_V7S_TABLE_GFP_DMA;

	...

	__get_free_pages(gfp_l1 | __GFP_ZERO, ...);


and similar for the slab flag.

Hi Will,
Thanks for your suggestion, PATCH v9 version will modify it.

quoted

 	if (lvl == 1)
-		table = (void *)__get_free_pages(
-			__GFP_ZERO | ARM_V7S_TABLE_GFP_DMA,
get_order(size));
+		table = (void *)__get_free_pages(gfp_l1,
get_order(size));
 	else if (lvl == 2)
 		table = kmem_cache_zalloc(data->l2_tables, gfp);
 

@@ -251,7 +259,8 @@ static void *__arm_v7s_alloc_table(int lvl,

gfp_t gfp,
 		return NULL;
 
 	phys = virt_to_phys(table);
-	if (phys != (arm_v7s_iopte)phys) {
+	if (phys != (arm_v7s_iopte)phys &&
+	    !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT)) {
 		/* Doesn't fit in PTE */

Shouldn't we be checking that the address is within 35 bits here?
Perhaps we
should generate a mask from the oas instead of just using the cast.

Hi Will,
Thanks for your suggestion, PATCH v9 version will add checking that the address is within 35 bits:

phys = virt_to_phys(table);
if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT ?
    phys >= (1ULL << cfg->oas) : phys != (arm_v7s_iopte)phys) {
	/* Doesn't fit in PTE */


Thanks,
Yunfei.

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