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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220615141411.5119-1-yf.wang@mediatek.com>
Date:   Wed, 15 Jun 2022 22:14:11 +0800
From:   <yf.wang@...iatek.com>
To:     <will@...nel.org>
CC:     <Libo.Kang@...iatek.com>, <Yong.Wu@...iatek.com>,
        <iommu@...ts.linux-foundation.org>, <isaacm@...eaurora.org>,
        <joro@...tes.org>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <matthias.bgg@...il.com>,
        <miles.chen@...iatek.com>, <ning.li@...iatek.com>,
        <quic_c_gdjako@...cinc.com>, <robin.murphy@....com>,
        <sven@...npeter.dev>, <wsd_upstream@...iatek.com>,
        <yf.wang@...iatek.com>
Subject: 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 :/
> 
> 
> > +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.