Re: [PATCH v2] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Robin Murphy <robin.murphy@arm.com>]

On 27/02/2019 23:22, Rob Herring wrote:
> ARM Mali midgard GPU page tables are similar to standard 64-bit stage 1
> page tables, but have a few differences. Add a new format type to
> represent the format. The input address size is 48-bits and the output
> address size is 40-bits (and possibly less?). Note that the later
> bifrost GPUs follow the standard 64-bit stage 1 format.
> 
> The differences in the format compared to 64-bit stage 1 format are:
> 
> The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.
> 
> The access flags are not read-only and unprivileged, but read and write.
> This is similar to stage 2 entries, but the memory attributes field matches
> stage 1 being an index.
> 
> The nG bit is not set by the vendor driver. This one didn't seem to matter,
> but we'll keep it aligned to the vendor driver.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: iommu@lists.linux-foundation.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Robin, Hopefully this is what you had in mind.

Getting there, for sure :)

>   drivers/iommu/io-pgtable-arm.c | 70 +++++++++++++++++++++++++++-------
>   drivers/iommu/io-pgtable.c     |  1 +
>   include/linux/io-pgtable.h     |  2 +
>   3 files changed, 59 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index d3700ec15cbd..84beea1f47a7 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -180,11 +180,6 @@
>   
>   #define iopte_prot(pte)	((pte) & ARM_LPAE_PTE_ATTR_MASK)
>   
> -#define iopte_leaf(pte,l)					\
> -	(l == (ARM_LPAE_MAX_LEVELS - 1) ?			\
> -		(iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE) :	\
> -		(iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK))
> -
>   struct arm_lpae_io_pgtable {
>   	struct io_pgtable	iop;
>   
> @@ -198,6 +193,14 @@ struct arm_lpae_io_pgtable {
>   
>   typedef u64 arm_lpae_iopte;
>   
> +static inline bool iopte_leaf(arm_lpae_iopte pte, int l, enum io_pgtable_fmt fmt)
> +{
> +	if ((l == (ARM_LPAE_MAX_LEVELS - 1)) && (fmt != ARM_MALI_LPAE))
> +		return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE;
> +
> +	return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK;
> +}
> +
>   static arm_lpae_iopte paddr_to_iopte(phys_addr_t paddr,
>   				     struct arm_lpae_io_pgtable *data)
>   {
> @@ -304,11 +307,14 @@ static void __arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>   		pte |= ARM_LPAE_PTE_NS;
>   
>   	if (lvl == ARM_LPAE_MAX_LEVELS - 1)
> -		pte |= ARM_LPAE_PTE_TYPE_PAGE;
> +		pte |= (data->iop.fmt == ARM_MALI_LPAE) ?
> +			ARM_LPAE_PTE_TYPE_BLOCK : ARM_LPAE_PTE_TYPE_PAGE;

Yuck at that ternary... Made worse by the previous hunk which proves you 
already know the nice reasonable way to structure this logic ;)

>   	else
>   		pte |= ARM_LPAE_PTE_TYPE_BLOCK;
>   
> -	pte |= ARM_LPAE_PTE_AF | ARM_LPAE_PTE_SH_IS;
> +	if (data->iop.fmt != ARM_MALI_LPAE)
> +		pte |= ARM_LPAE_PTE_AF;

So ENTRY_ACCESS_BIT is something different from the VMSA access flag then?

> +	pte |= ARM_LPAE_PTE_SH_IS;
>   	pte |= paddr_to_iopte(paddr, data);
>   
>   	__arm_lpae_set_pte(ptep, pte, &data->iop.cfg);
> @@ -321,7 +327,7 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>   {
>   	arm_lpae_iopte pte = *ptep;
>   
> -	if (iopte_leaf(pte, lvl)) {
> +	if (iopte_leaf(pte, lvl, data->iop.fmt)) {
>   		/* We require an unmap first */
>   		WARN_ON(!selftest_running);
>   		return -EEXIST;
> @@ -409,7 +415,7 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
>   		__arm_lpae_sync_pte(ptep, cfg);
>   	}
>   
> -	if (pte && !iopte_leaf(pte, lvl)) {
> +	if (pte && !iopte_leaf(pte, lvl, data->iop.fmt)) {
>   		cptep = iopte_deref(pte, data);
>   	} else if (pte) {
>   		/* We require an unmap first */
> @@ -426,7 +432,22 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
>   {
>   	arm_lpae_iopte pte;
>   
> -	if (data->iop.fmt == ARM_64_LPAE_S1 ||
> +	if (data->iop.fmt == ARM_MALI_LPAE) {
> +		pte = 0;
> +
> +		if (prot & IOMMU_WRITE)
> +			pte |= ARM_LPAE_PTE_AP_RDONLY;

This one in particular I will never be able to look at without 
instinctively thinking "hang on, how did I ever let that bug slip 
through?"...

> +
> +		if (prot & IOMMU_READ)
> +			pte |= ARM_LPAE_PTE_AP_UNPRIV;

...while this one only looks utterly insane. Can we please at least use 
the stage 2 permission definitions for these so that they're actually 
readable.

> +
> +		if (prot & IOMMU_MMIO)
> +			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
> +				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
> +		else if (prot & IOMMU_CACHE)
> +			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
> +				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
> +	} else if (data->iop.fmt == ARM_64_LPAE_S1 ||

In fact, looking at it now, I think it only takes a little (untested) 
refactor to split permissions vs. attributes to get the Mali version for 
free:

----->8-----
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 237cacd4a62b..3cbc08bb7acd 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -430,31 +430,33 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
arm_lpae_io_pgtable *data,
  	if (data->iop.fmt == ARM_64_LPAE_S1 ||
  	    data->iop.fmt == ARM_32_LPAE_S1) {
  		pte = ARM_LPAE_PTE_nG;
-
  		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
  			pte |= ARM_LPAE_PTE_AP_RDONLY;
-
  		if (!(prot & IOMMU_PRIV))
  			pte |= ARM_LPAE_PTE_AP_UNPRIV;
-
-		if (prot & IOMMU_MMIO)
-			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
-				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
-		else if (prot & IOMMU_CACHE)
-			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
-				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
  	} else {
  		pte = ARM_LPAE_PTE_HAP_FAULT;
  		if (prot & IOMMU_READ)
  			pte |= ARM_LPAE_PTE_HAP_READ;
  		if (prot & IOMMU_WRITE)
  			pte |= ARM_LPAE_PTE_HAP_WRITE;
+	}
+
+	if (data->iop.fmt == ARM_64_LPAE_S2 ||
+	    data->iop.fmt == ARM_32_LPAE_S2) {
  		if (prot & IOMMU_MMIO)
  			pte |= ARM_LPAE_PTE_MEMATTR_DEV;
  		else if (prot & IOMMU_CACHE)
  			pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
  		else
  			pte |= ARM_LPAE_PTE_MEMATTR_NC;
+	} else {
+		if (prot & IOMMU_MMIO)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+		else if (prot & IOMMU_CACHE)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
  	}

  	if (prot & IOMMU_NOEXEC)
-----8<-----

>   	    data->iop.fmt == ARM_32_LPAE_S1) {
>   		pte = ARM_LPAE_PTE_nG;
>   
> @@ -511,7 +532,7 @@ static void __arm_lpae_free_pgtable(struct arm_lpae_io_pgtable *data, int lvl,
>   	while (ptep != end) {
>   		arm_lpae_iopte pte = *ptep++;
>   
> -		if (!pte || iopte_leaf(pte, lvl))
> +		if (!pte || iopte_leaf(pte, lvl, data->iop.fmt))
>   			continue;
>   
>   		__arm_lpae_free_pgtable(data, lvl + 1, iopte_deref(pte, data));
> @@ -602,7 +623,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>   	if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) {
>   		__arm_lpae_set_pte(ptep, 0, &iop->cfg);
>   
> -		if (!iopte_leaf(pte, lvl)) {
> +		if (!iopte_leaf(pte, lvl, iop->fmt)) {
>   			/* Also flush any partial walks */
>   			io_pgtable_tlb_add_flush(iop, iova, size,
>   						ARM_LPAE_GRANULE(data), false);
> @@ -621,7 +642,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>   		}
>   
>   		return size;
> -	} else if (iopte_leaf(pte, lvl)) {
> +	} else if (iopte_leaf(pte, lvl, iop->fmt)) {
>   		/*
>   		 * Insert a table at the next level to map the old region,
>   		 * minus the part we want to unmap
> @@ -669,7 +690,7 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
>   			return 0;
>   
>   		/* Leaf entry? */
> -		if (iopte_leaf(pte,lvl))
> +		if (iopte_leaf(pte,lvl, data->iop.fmt))

Super-nit: If you're touching this line anyway, would you mind adding 
the currently-missing space as well?

>   			goto found_translation;
>   
>   		/* Take it to the next level */
> @@ -995,6 +1016,22 @@ arm_32_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
>   	return iop;
>   }
>   
> +static struct io_pgtable *
> +arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
> +{
> +	struct io_pgtable *iop;
> +
> +	if (cfg->ias != 48 || cfg->oas > 40)
> +		return NULL;
> +
> +	cfg->pgsize_bitmap &= (SZ_4K | SZ_2M | SZ_1G);
> +	iop = arm_64_lpae_alloc_pgtable_s1(cfg, cookie);
> +	if (iop)
> +		cfg->arm_lpae_s1_cfg.tcr = 0;

The general design intent is that we return ready-to-go register values 
here (much like mmu_get_as_setup() in mali_kbase, it seems), so I think 
it's worth adding an arm_mali_cfg to the io_pgtable_cfg union and 
transforming the VMSA output into final transtab/transcfg/memattr format 
at this point, rather then callers needing to care (e.g. some of those 
AS_TRANSTAB_LPAE_* bits look reminiscent of the walk attributes we set 
up for a VMSA TCR).

Robin.

> +
> +	return iop;
> +}
> +
>   struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
>   	.alloc	= arm_64_lpae_alloc_pgtable_s1,
>   	.free	= arm_lpae_free_pgtable,
> @@ -1015,6 +1052,11 @@ struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
>   	.free	= arm_lpae_free_pgtable,
>   };
>   
> +struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
> +	.alloc	= arm_mali_lpae_alloc_pgtable,
> +	.free	= arm_lpae_free_pgtable,
> +};
> +
>   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
>   
>   static struct io_pgtable_cfg *cfg_cookie;
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 93f2880be6c6..5227cfdbb65b 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -30,6 +30,7 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
>   	[ARM_32_LPAE_S2] = &io_pgtable_arm_32_lpae_s2_init_fns,
>   	[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
>   	[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
> +	[ARM_MALI_LPAE] = &io_pgtable_arm_mali_lpae_init_fns,
>   #endif
>   #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S
>   	[ARM_V7S] = &io_pgtable_arm_v7s_init_fns,
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index 47d5ae559329..5f0be2471651 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -12,6 +12,7 @@ enum io_pgtable_fmt {
>   	ARM_64_LPAE_S1,
>   	ARM_64_LPAE_S2,
>   	ARM_V7S,
> +	ARM_MALI_LPAE,
>   	IO_PGTABLE_NUM_FMTS,
>   };
>   
> @@ -209,5 +210,6 @@ extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
>   extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
>   extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
>   extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
> +extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
>   
>   #endif /* __IO_PGTABLE_H */
>