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

[yf.wang]

On Wed, 2022-06-15 at 18:03 +0100, Robin Murphy wrote:
> On 2022-06-15 17:12, yf.wang@xxxxxxxxxxxx wrote:
> > 
> >   static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl,
> >   				  struct io_pgtable_cfg *cfg)
> >   {
> > @@ -240,10 +245,17 @@ static void *__arm_v7s_alloc_table(int lvl,
> > gfp_t gfp,
> >   	dma_addr_t dma;
> >   	size_t size = ARM_V7S_TABLE_SIZE(lvl, cfg);
> >   	void *table = NULL;
> > +	gfp_t gfp_l1;
> > +
> > +	/*
> > +	 * ARM_MTK_TTBR_EXT extend the translation table base support
> > all
> > +	 * memory address.
> > +	 */
> > +	gfp_l1 = cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT ?
> > +		 GFP_KERNEL : ARM_V7S_TABLE_GFP_DMA;
> >   
> >   	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 | __GFP_ZERO,
> > get_order(size));
> >   	else if (lvl == 2)
> >   		table = kmem_cache_zalloc(data->l2_tables, gfp);
> >   
> > @@ -251,7 +263,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 (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT ?
> > +	    phys >= (1ULL << cfg->oas) : phys != (arm_v7s_iopte)phys) {
> 
> Given that the comment above says it supports all of memory, how
> would 
> phys >= (1ULL << cfg->oas) ever be true?
> 

Hi Robin,

Since Mediatek IOMMU hardware support at most 35bit PA in pgtable,
so add a quirk to allow the PA of pgtables support up to bit35,
but need to check oas do error hanlde.


> >   		/* Doesn't fit in PTE */
> >   		dev_err(dev, "Page table does not fit in PTE: %pa",
> > &phys);
> >   		goto out_free;
> > arm_v7s_install_table(arm_v7s_iopte *table,
> >   					   arm_v7s_iopte curr,
> >   					   struct io_pgtable_cfg *cfg)

...
 
> > diff --git a/include/linux/io-pgtable.h b/include/linux/io-
> > pgtable.h
> > index 86af6f0a00a2..c9189716f6bd 100644
> > --- a/include/linux/io-pgtable.h
> > +++ b/include/linux/io-pgtable.h
> > @@ -74,17 +74,22 @@ struct io_pgtable_cfg {
> >   	 *	to support up to 35 bits PA where the bit32, bit33 and
> > bit34 are
> >   	 *	encoded in the bit9, bit4 and bit5 of the PTE respectively.
> >   	 *
> > +	 * IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT: (ARM v7s format) MediaTek
> > IOMMUs
> > +	 *	extend the translation table base support up to 35 bits PA,
> > the
> > +	 *	encoding format is same with IO_PGTABLE_QUIRK_ARM_MTK_EXT.
> > +	 *
> >   	 * IO_PGTABLE_QUIRK_ARM_TTBR1: (ARM LPAE format) Configure the
> > table
> >   	 *	for use in the upper half of a split address space.
> >   	 *
> >   	 * IO_PGTABLE_QUIRK_ARM_OUTER_WBWA: Override the outer-
> > cacheability
> >   	 *	attributes set in the TCR for a non-coherent page-table
> > walker.
> >   	 */
> > -	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
> > -	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
> > -	#define IO_PGTABLE_QUIRK_ARM_MTK_EXT	BIT(3)
> > -	#define IO_PGTABLE_QUIRK_ARM_TTBR1	BIT(5)
> > -	#define IO_PGTABLE_QUIRK_ARM_OUTER_WBWA	BIT(6)
> > +	#define IO_PGTABLE_QUIRK_ARM_NS			BIT(0)
> > +	#define IO_PGTABLE_QUIRK_NO_PERMS		BIT(1)
> > +	#define IO_PGTABLE_QUIRK_ARM_MTK_EXT		BIT(3)
> > +	#define IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT	BIT(4)
> > +	#define IO_PGTABLE_QUIRK_ARM_TTBR1		BIT(5)
> > +	#define IO_PGTABLE_QUIRK_ARM_OUTER_WBWA		BIT(6)
> >   	unsigned long			quirks;
> >   	unsigned long			pgsize_bitmap;
> >   	unsigned int			ias;
> > @@ -122,7 +127,7 @@ struct io_pgtable_cfg {
> >   		} arm_lpae_s2_cfg;
> >   
> >   		struct {
> > -			u32	ttbr;
> > +			u64	ttbr;
> 
> The point of this is to return an encoded TTBR register value, not a
> raw 
> base address. I see from the other patches that your register is
> still 
> 32 bits, so I'd prefer to follow the standard pattern and not need
> this 
> change.
> 
> Thanks,
> Robin.
> 

Hi Robin,
Thanks for your suggestion, next version will recovery ttbr to 32 bits,
will modify arm_v7s_alloc_pgtable to return an encoded TTBR, encoded
PA bits[34:32] to to lower bits, as follows:

/* TTBR */
phys_addr_t paddr;
paddr = virt_to_phys(data->pgd);
cfg->arm_v7s_cfg.ttbr = virt_to_phys(data->pgd) | ARM_V7S_TTBR_S |
			(cfg->coherent_walk ? (ARM_V7S_TTBR_NOS |
			 ARM_V7S_TTBR_IRGN_ATTR(ARM_V7S_RGN_WBWA) |
			 ARM_V7S_TTBR_ORGN_ATTR(ARM_V7S_RGN_WBWA)) :
			(ARM_V7S_TTBR_IRGN_ATTR(ARM_V7S_RGN_NC) |
			 ARM_V7S_TTBR_ORGN_ATTR(ARM_V7S_RGN_NC)));

if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT)
	cfg->arm_v7s_cfg.ttbr = (paddr & GENMASK(31, 7)) |
				upper_32_bits(paddr);

Thanks,
Yunfei.

> >   			u32	tcr;
> >   			u32	nmrr;
> >   			u32	prrr;