Re: [PATCH 0/4] Generic IOMMU page table framework

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

On Sun, Dec 14, 2014 at 11:49:30PM +0000, Laurent Pinchart wrote:
> Hi Will,

Hi Laurent,

> On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
> > This series introduces a generic IOMMU page table allocation framework,
> > implements support for ARM long-descriptors and then ports the arm-smmu
> > driver over to the new code.

[...]

> > All feedback welcome.
> 
> I've successfully tested the patch set with the Renesas IPMMU-VMSA driver with
> the following extension to the allocator.
> 
> Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Wahey, that's really cool, thanks! I have a few minor comments on your patch
below. If you don't object, then I can make them locally and include your
patch on top of my v2 series?

> From 4bebb7f3a5a48541d4c89ce7c61e6ff66686c3a9 Mon Sep 17 00:00:00 2001
> From: Laurent Pinchart <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> Date: Sun, 14 Dec 2014 23:34:50 +0200
> Subject: [PATCH] iommu: io-pgtable-arm: Add Non-Secure quirk
> 
> The quirk causes the Non-Secure bit to be set in all page table entries.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>  drivers/iommu/io-pgtable-arm.c | 7 +++++++
>  drivers/iommu/io-pgtable.h     | 3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 669e322a83a4..b6910e142734 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -80,11 +80,13 @@
>  #define ARM_LPAE_PTE_TYPE_TABLE		3
>  #define ARM_LPAE_PTE_TYPE_PAGE		3
>  
> +#define ARM_LPAE_PTE_NSTABLE		(((arm_lpae_iopte)1) << 63)
>  #define ARM_LPAE_PTE_XN			(((arm_lpae_iopte)3) << 53)
>  #define ARM_LPAE_PTE_AF			(((arm_lpae_iopte)1) << 10)
>  #define ARM_LPAE_PTE_SH_NS		(((arm_lpae_iopte)0) << 8)
>  #define ARM_LPAE_PTE_SH_OS		(((arm_lpae_iopte)2) << 8)
>  #define ARM_LPAE_PTE_SH_IS		(((arm_lpae_iopte)3) << 8)
> +#define ARM_LPAE_PTE_NS			(((arm_lpae_iopte)1) << 5)
>  #define ARM_LPAE_PTE_VALID		(((arm_lpae_iopte)1) << 0)
>  
>  #define ARM_LPAE_PTE_ATTR_LO_MASK	(((arm_lpae_iopte)0x3ff) << 2)
> @@ -201,6 +203,9 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>  	if (iopte_leaf(*ptep, lvl))
>  		return -EEXIST;
>  
> +	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE)
> +		pte |= ARM_LPAE_PTE_NS;
> +
>  	if (lvl == ARM_LPAE_MAX_LEVELS - 1)
>  		pte |= ARM_LPAE_PTE_TYPE_PAGE;
>  	else
> @@ -244,6 +249,8 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
>  		data->iop.cfg.tlb->flush_pgtable(cptep, 1UL << data->pg_shift,
>  						 cookie);
>  		pte = __pa(cptep) | ARM_LPAE_PTE_TYPE_TABLE;
> +		if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE)
> +			pte |= ARM_LPAE_PTE_NSTABLE;
>  		*ptep = pte;
>  		data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), cookie);
>  	} else {
> diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
> index c1cff3d045db..a41a15d30596 100644
> --- a/drivers/iommu/io-pgtable.h
> +++ b/drivers/iommu/io-pgtable.h
> @@ -24,6 +24,9 @@ struct iommu_gather_ops {
>  	void (*flush_pgtable)(void *ptr, size_t size, void *cookie);
>  };
>  
> +/* Set the Non-Secure bit in the PTEs */
> +#define IO_PGTABLE_QUIRK_NON_SECURE	(1 << 0)

I think I'd stick an _ARM_ somewhere in here, so maybe
IO_PGTABLE_QUIRK_ARM_NS?

> +
>  struct io_pgtable_cfg {

and I'd put the #define here, next to the member.

>  	int			quirks; /* IO_PGTABLE_QUIRK_* */
>  	unsigned long		pgsize_bitmap;
> -- 

Will