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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
