Messages in this thread | | | From | <> | Subject | Re: [PATCH v8 1/3] iommu/io-pgtable-arm-v7s: Add a quirk to allow pgtable PA up to 35bit | Date | Wed, 15 Jun 2022 22:14:11 +0800 |
| |
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 :/ > > > > +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.
> > 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.
> > 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.
| |