[PATCH 0/4] iommu/io-pgtable: Cleanup and prep for split tables

[PATCH 0/4] iommu/io-pgtable: Cleanup and prep for split tables

[Robin Murphy]

Hi all,

Although the io-pgtable-arm formats started out with the notion of being
able to provide a complete ready-to-use context for VMSA-compliant users
to consume, the reality is that users inevitably still have to make their
own adjustments to that context anyway. Worse, though, is that some of
that cruft starts actively getting in the way of future work like
supporting split tables using both TTBRs.

These patches clean things up by stripping io-pgatble's context back to
just the parts directly relevant to what it abstracts, and formalising
the expectation that this only forms a basis from which users can
construct their own complete context.

Series based on v2 of "Arm SMMU refactoring" here:
https://patchwork.kernel.org/patch/11096263/

Robin.


Robin Murphy (4):
  iommu/io-pgtable-arm: Rationalise MAIR handling
  iommu/io-pgtable-arm: Rationalise TTBRn handling
  iommu/io-pgtable-arm: Rationalise TCR handling
  iommu/io-pgtable-arm: Prepare for TTBR1 usage

 drivers/iommu/arm-smmu-v3.c        | 11 +++--------
 drivers/iommu/arm-smmu.c           | 14 +++++++-------
 drivers/iommu/arm-smmu.h           |  2 ++
 drivers/iommu/io-pgtable-arm-v7s.c | 22 +++++++++-------------
 drivers/iommu/io-pgtable-arm.c     | 23 +++++++++--------------
 drivers/iommu/ipmmu-vmsa.c         |  4 ++--
 drivers/iommu/msm_iommu.c          |  4 ++--
 drivers/iommu/mtk_iommu.c          |  4 ++--
 drivers/iommu/qcom_iommu.c         |  9 ++++-----
 include/linux/io-pgtable.h         |  6 +++---
 10 files changed, 43 insertions(+), 56 deletions(-)

-- 
2.21.0.dirty

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/4] iommu/io-pgtable-arm: Rationalise MAIR handling

[Robin Murphy]

Between VMSAv8-64 and the various 32-bit formats, there is either one
64-bit MAIR or a pair of 32-bit MAIR0/MAIR1 or NMRR/PMRR registers.
As such, keeping two 64-bit values in io_pgtable_cfg has always been
overkill.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu-v3.c    | 2 +-
 drivers/iommu/arm-smmu.c       | 4 ++--
 drivers/iommu/io-pgtable-arm.c | 5 ++---
 drivers/iommu/ipmmu-vmsa.c     | 2 +-
 drivers/iommu/qcom_iommu.c     | 4 ++--
 include/linux/io-pgtable.h     | 2 +-
 6 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index a9a9fabd3968..2a8db896d698 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1724,7 +1724,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	cfg->cd.asid	= (u16)asid;
 	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
 	cfg->cd.tcr	= pgtbl_cfg->arm_lpae_s1_cfg.tcr;
-	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
+	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
 	return 0;
 
 out_free_asid:
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 523a88842e7f..184ca41e9de7 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -491,8 +491,8 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 			cb->mair[0] = pgtbl_cfg->arm_v7s_cfg.prrr;
 			cb->mair[1] = pgtbl_cfg->arm_v7s_cfg.nmrr;
 		} else {
-			cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
-			cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
+			cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair;
+			cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair >> 32;
 		}
 	}
 }
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 161a7d56264d..c588ba0dd4d1 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -859,8 +859,7 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 	      (ARM_LPAE_MAIR_ATTR_INC_OWBRWA
 	       << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE));
 
-	cfg->arm_lpae_s1_cfg.mair[0] = reg;
-	cfg->arm_lpae_s1_cfg.mair[1] = 0;
+	cfg->arm_lpae_s1_cfg.mair = reg;
 
 	/* Looking good; allocate a pgd */
 	data->pgd = __arm_lpae_alloc_pages(data->pgd_size, GFP_KERNEL, cfg);
@@ -1024,7 +1023,7 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
 		u64 mair, ttbr;
 
 		/* Copy values as union fields overlap */
-		mair = cfg->arm_lpae_s1_cfg.mair[0];
+		mair = cfg->arm_lpae_s1_cfg.mair;
 		ttbr = cfg->arm_lpae_s1_cfg.ttbr[0];
 
 		cfg->arm_mali_lpae_cfg.memattr = mair;
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index ad0098c0c87c..9075e53a748b 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -436,7 +436,7 @@ static void ipmmu_domain_setup_context(struct ipmmu_vmsa_domain *domain)
 
 	/* MAIR0 */
 	ipmmu_ctx_write_root(domain, IMMAIR0,
-			     domain->cfg.arm_lpae_s1_cfg.mair[0]);
+			     domain->cfg.arm_lpae_s1_cfg.mair);
 
 	/* IMBUSCR */
 	if (domain->mmu->features->setup_imbuscr)
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index a2062d13584f..34bb357b3cfa 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -262,9 +262,9 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 
 		/* MAIRs (stage-1 only) */
 		iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR0,
-				pgtbl_cfg.arm_lpae_s1_cfg.mair[0]);
+				pgtbl_cfg.arm_lpae_s1_cfg.mair);
 		iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR1,
-				pgtbl_cfg.arm_lpae_s1_cfg.mair[1]);
+				pgtbl_cfg.arm_lpae_s1_cfg.mair >> 32);
 
 		/* SCTLR */
 		reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE |
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index b5a450a3bb47..a6c8aa204733 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -92,7 +92,7 @@ struct io_pgtable_cfg {
 		struct {
 			u64	ttbr[2];
 			u64	tcr;
-			u64	mair[2];
+			u64	mair;
 		} arm_lpae_s1_cfg;
 
 		struct {
-- 
2.21.0.dirty

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/4] iommu/io-pgtable-arm: Rationalise TTBRn handling

[Robin Murphy]

TTBR1 values have so far been redundant since no users implement any
support for split address spaces. Crucially, though, one of the main
reasons for wanting to do so is to be able to manage each half entirely
independently, e.g. context-switching one set of mappings without
disturbing the other. Thus it seems unlikely that tying two tables
together in a single io_pgtable_cfg would ever be particularly desirable
or useful.

Streamline the configs to just a single conceptual TTBR value
representing the allocated table. This paves the way for future users to
support split address spaces by simply allocating a table and dealing
with the detailed TTBRn logistics themselves.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu-v3.c        |  2 +-
 drivers/iommu/arm-smmu.c           |  9 ++++-----
 drivers/iommu/io-pgtable-arm-v7s.c | 16 +++++++---------
 drivers/iommu/io-pgtable-arm.c     |  7 +++----
 drivers/iommu/ipmmu-vmsa.c         |  2 +-
 drivers/iommu/msm_iommu.c          |  4 ++--
 drivers/iommu/mtk_iommu.c          |  4 ++--
 drivers/iommu/qcom_iommu.c         |  3 +--
 include/linux/io-pgtable.h         |  4 ++--
 9 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2a8db896d698..2e50cf49c3c4 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1722,7 +1722,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	}
 
 	cfg->cd.asid	= (u16)asid;
-	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
 	cfg->cd.tcr	= pgtbl_cfg->arm_lpae_s1_cfg.tcr;
 	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
 	return 0;
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 184ca41e9de7..19030c4b5904 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -473,13 +473,12 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 	/* TTBRs */
 	if (stage1) {
 		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
-			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
-			cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
+			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
+			cb->ttbr[1] = 0;
 		} else {
-			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
 			cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
-			cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-			cb->ttbr[1] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
+			cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
 		}
 	} else {
 		cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index 0fc8dfab2abf..7c80e9d00f73 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -775,15 +775,13 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
 	/* Ensure the empty pgd is visible before any actual TTBR write */
 	wmb();
 
-	/* TTBRs */
-	cfg->arm_v7s_cfg.ttbr[0] = virt_to_phys(data->pgd) |
-				   ARM_V7S_TTBR_S | ARM_V7S_TTBR_NOS |
-				   (cfg->coherent_walk ?
-				   (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)));
-	cfg->arm_v7s_cfg.ttbr[1] = 0;
+	/* TTBR */
+	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)));
 	return &data->iop;
 
 out_free_data:
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index c588ba0dd4d1..56e0921331c3 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -869,9 +869,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 	/* Ensure the empty pgd is visible before any actual TTBR write */
 	wmb();
 
-	/* TTBRs */
-	cfg->arm_lpae_s1_cfg.ttbr[0] = virt_to_phys(data->pgd);
-	cfg->arm_lpae_s1_cfg.ttbr[1] = 0;
+	/* TTBR */
+	cfg->arm_lpae_s1_cfg.ttbr = virt_to_phys(data->pgd);
 	return &data->iop;
 
 out_free_data:
@@ -1024,7 +1023,7 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
 
 		/* Copy values as union fields overlap */
 		mair = cfg->arm_lpae_s1_cfg.mair;
-		ttbr = cfg->arm_lpae_s1_cfg.ttbr[0];
+		ttbr = cfg->arm_lpae_s1_cfg.ttbr;
 
 		cfg->arm_mali_lpae_cfg.memattr = mair;
 		cfg->arm_mali_lpae_cfg.transtab = ttbr |
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9075e53a748b..180ab71d8542 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -416,7 +416,7 @@ static void ipmmu_domain_setup_context(struct ipmmu_vmsa_domain *domain)
 	u32 tmp;
 
 	/* TTBR0 */
-	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
+	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr;
 	ipmmu_ctx_write_root(domain, IMTTLBR0, ttbr);
 	ipmmu_ctx_write_root(domain, IMTTUBR0, ttbr >> 32);
 
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index b25e2eb9e038..fb6d406e0c52 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -270,8 +270,8 @@ static void __program_context(void __iomem *base, int ctx,
 	SET_V2PCFG(base, ctx, 0x3);
 
 	SET_TTBCR(base, ctx, priv->cfg.arm_v7s_cfg.tcr);
-	SET_TTBR0(base, ctx, priv->cfg.arm_v7s_cfg.ttbr[0]);
-	SET_TTBR1(base, ctx, priv->cfg.arm_v7s_cfg.ttbr[1]);
+	SET_TTBR0(base, ctx, priv->cfg.arm_v7s_cfg.ttbr);
+	SET_TTBR1(base, ctx, 0);
 
 	/* Set prrr and nmrr */
 	SET_PRRR(base, ctx, priv->cfg.arm_v7s_cfg.prrr);
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 82e4be4dfdaf..ad644631780d 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -336,7 +336,7 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
 	/* Update the pgtable base address register of the M4U HW */
 	if (!data->m4u_dom) {
 		data->m4u_dom = dom;
-		writel(dom->cfg.arm_v7s_cfg.ttbr[0],
+		writel(dom->cfg.arm_v7s_cfg.ttbr,
 		       data->base + REG_MMU_PT_BASE_ADDR);
 	}
 
@@ -723,7 +723,7 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
 	writel_relaxed(reg->int_main_control, base + REG_MMU_INT_MAIN_CONTROL);
 	writel_relaxed(reg->ivrp_paddr, base + REG_MMU_IVRP_PADDR);
 	if (data->m4u_dom)
-		writel(data->m4u_dom->cfg.arm_v7s_cfg.ttbr[0],
+		writel(data->m4u_dom->cfg.arm_v7s_cfg.ttbr,
 		       base + REG_MMU_PT_BASE_ADDR);
 	return 0;
 }
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index 34bb357b3cfa..de55b6d82ef1 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -247,10 +247,9 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 
 		/* TTBRs */
 		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
-				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[0] |
+				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
 				FIELD_PREP(TTBRn_ASID, ctx->asid));
 		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1,
-				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[1] |
 				FIELD_PREP(TTBRn_ASID, ctx->asid));
 
 		/* TCR */
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index a6c8aa204733..7a0905d7a006 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -90,7 +90,7 @@ struct io_pgtable_cfg {
 	/* Low-level data specific to the table format */
 	union {
 		struct {
-			u64	ttbr[2];
+			u64	ttbr;
 			u64	tcr;
 			u64	mair;
 		} arm_lpae_s1_cfg;
@@ -101,7 +101,7 @@ struct io_pgtable_cfg {
 		} arm_lpae_s2_cfg;
 
 		struct {
-			u32	ttbr[2];
+			u32	ttbr;
 			u32	tcr;
 			u32	nmrr;
 			u32	prrr;
-- 
2.21.0.dirty

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu/io-pgtable-arm: Rationalise TTBRn handling

[Will Deacon]

On Mon, Aug 19, 2019 at 07:19:29PM +0100, Robin Murphy wrote:
> TTBR1 values have so far been redundant since no users implement any
> support for split address spaces. Crucially, though, one of the main
> reasons for wanting to do so is to be able to manage each half entirely
> independently, e.g. context-switching one set of mappings without
> disturbing the other. Thus it seems unlikely that tying two tables
> together in a single io_pgtable_cfg would ever be particularly desirable
> or useful.
> 
> Streamline the configs to just a single conceptual TTBR value
> representing the allocated table. This paves the way for future users to
> support split address spaces by simply allocating a table and dealing
> with the detailed TTBRn logistics themselves.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu-v3.c        |  2 +-
>  drivers/iommu/arm-smmu.c           |  9 ++++-----
>  drivers/iommu/io-pgtable-arm-v7s.c | 16 +++++++---------
>  drivers/iommu/io-pgtable-arm.c     |  7 +++----
>  drivers/iommu/ipmmu-vmsa.c         |  2 +-
>  drivers/iommu/msm_iommu.c          |  4 ++--
>  drivers/iommu/mtk_iommu.c          |  4 ++--
>  drivers/iommu/qcom_iommu.c         |  3 +--
>  include/linux/io-pgtable.h         |  4 ++--
>  9 files changed, 23 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 2a8db896d698..2e50cf49c3c4 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -1722,7 +1722,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
>  	}
>  
>  	cfg->cd.asid	= (u16)asid;
> -	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> +	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
>  	cfg->cd.tcr	= pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>  	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
>  	return 0;
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 184ca41e9de7..19030c4b5904 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -473,13 +473,12 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>  	/* TTBRs */
>  	if (stage1) {
>  		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> -			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> -			cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> +			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
> +			cb->ttbr[1] = 0;
>  		} else {
> -			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> +			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
>  			cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> -			cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> -			cb->ttbr[1] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> +			cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);

Why do you continue to put the ASID in here?

> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> index 34bb357b3cfa..de55b6d82ef1 100644
> --- a/drivers/iommu/qcom_iommu.c
> +++ b/drivers/iommu/qcom_iommu.c
> @@ -247,10 +247,9 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  
>  		/* TTBRs */
>  		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[0] |
> +				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>  				FIELD_PREP(TTBRn_ASID, ctx->asid));
>  		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1,
> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[1] |
>  				FIELD_PREP(TTBRn_ASID, ctx->asid));

Same here.

> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index a6c8aa204733..7a0905d7a006 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -90,7 +90,7 @@ struct io_pgtable_cfg {
>  	/* Low-level data specific to the table format */
>  	union {
>  		struct {
> -			u64	ttbr[2];
> +			u64	ttbr;
>  			u64	tcr;
>  			u64	mair;
>  		} arm_lpae_s1_cfg;
> @@ -101,7 +101,7 @@ struct io_pgtable_cfg {
>  		} arm_lpae_s2_cfg;
>  
>  		struct {
> -			u32	ttbr[2];
> +			u32	ttbr;

We could probably do with a comment for these 'ttbr' field now saying that
they refer to ttbr0 (since the tcr will have EPD1 set).

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu/io-pgtable-arm: Rationalise TTBRn handling

[Robin Murphy]

On 20/08/2019 11:19, Will Deacon wrote:
> On Mon, Aug 19, 2019 at 07:19:29PM +0100, Robin Murphy wrote:
>> TTBR1 values have so far been redundant since no users implement any
>> support for split address spaces. Crucially, though, one of the main
>> reasons for wanting to do so is to be able to manage each half entirely
>> independently, e.g. context-switching one set of mappings without
>> disturbing the other. Thus it seems unlikely that tying two tables
>> together in a single io_pgtable_cfg would ever be particularly desirable
>> or useful.
>>
>> Streamline the configs to just a single conceptual TTBR value
>> representing the allocated table. This paves the way for future users to
>> support split address spaces by simply allocating a table and dealing
>> with the detailed TTBRn logistics themselves.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c        |  2 +-
>>   drivers/iommu/arm-smmu.c           |  9 ++++-----
>>   drivers/iommu/io-pgtable-arm-v7s.c | 16 +++++++---------
>>   drivers/iommu/io-pgtable-arm.c     |  7 +++----
>>   drivers/iommu/ipmmu-vmsa.c         |  2 +-
>>   drivers/iommu/msm_iommu.c          |  4 ++--
>>   drivers/iommu/mtk_iommu.c          |  4 ++--
>>   drivers/iommu/qcom_iommu.c         |  3 +--
>>   include/linux/io-pgtable.h         |  4 ++--
>>   9 files changed, 23 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 2a8db896d698..2e50cf49c3c4 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -1722,7 +1722,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
>>   	}
>>   
>>   	cfg->cd.asid	= (u16)asid;
>> -	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> +	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
>>   	cfg->cd.tcr	= pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>>   	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
>>   	return 0;
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 184ca41e9de7..19030c4b5904 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -473,13 +473,12 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>>   	/* TTBRs */
>>   	if (stage1) {
>>   		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> -			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> -			cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> +			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
>> +			cb->ttbr[1] = 0;
>>   		} else {
>> -			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> +			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
>>   			cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
>> -			cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> -			cb->ttbr[1] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
>> +			cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> 
> Why do you continue to put the ASID in here?

For the same reason we put it there before ;)

Although I guess if TCR.A1 were ever to get flipped accidentally then 
we're still cool.

>> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
>> index 34bb357b3cfa..de55b6d82ef1 100644
>> --- a/drivers/iommu/qcom_iommu.c
>> +++ b/drivers/iommu/qcom_iommu.c
>> @@ -247,10 +247,9 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>>   
>>   		/* TTBRs */
>>   		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[0] |
>> +				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>>   				FIELD_PREP(TTBRn_ASID, ctx->asid));
>>   		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1,
>> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[1] |
>>   				FIELD_PREP(TTBRn_ASID, ctx->asid));
> 
> Same here.
> 
>> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
>> index a6c8aa204733..7a0905d7a006 100644
>> --- a/include/linux/io-pgtable.h
>> +++ b/include/linux/io-pgtable.h
>> @@ -90,7 +90,7 @@ struct io_pgtable_cfg {
>>   	/* Low-level data specific to the table format */
>>   	union {
>>   		struct {
>> -			u64	ttbr[2];
>> +			u64	ttbr;
>>   			u64	tcr;
>>   			u64	mair;
>>   		} arm_lpae_s1_cfg;
>> @@ -101,7 +101,7 @@ struct io_pgtable_cfg {
>>   		} arm_lpae_s2_cfg;
>>   
>>   		struct {
>> -			u32	ttbr[2];
>> +			u32	ttbr;
> 
> We could probably do with a comment for these 'ttbr' field now saying that
> they refer to ttbr0 (since the tcr will have EPD1 set).

Yeah, I did wonder whether this might want elaboration, or whether the 
commit messages plus the code consuming it made the idea sufficiently 
clear - I guess that's my answer...

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu/io-pgtable-arm: Rationalise TTBRn handling

[Will Deacon]

On Tue, Aug 20, 2019 at 03:17:19PM +0100, Robin Murphy wrote:
> On 20/08/2019 11:19, Will Deacon wrote:
> > On Mon, Aug 19, 2019 at 07:19:29PM +0100, Robin Murphy wrote:
> > > TTBR1 values have so far been redundant since no users implement any
> > > support for split address spaces. Crucially, though, one of the main
> > > reasons for wanting to do so is to be able to manage each half entirely
> > > independently, e.g. context-switching one set of mappings without
> > > disturbing the other. Thus it seems unlikely that tying two tables
> > > together in a single io_pgtable_cfg would ever be particularly desirable
> > > or useful.
> > > 
> > > Streamline the configs to just a single conceptual TTBR value
> > > representing the allocated table. This paves the way for future users to
> > > support split address spaces by simply allocating a table and dealing
> > > with the detailed TTBRn logistics themselves.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/iommu/arm-smmu-v3.c        |  2 +-
> > >   drivers/iommu/arm-smmu.c           |  9 ++++-----
> > >   drivers/iommu/io-pgtable-arm-v7s.c | 16 +++++++---------
> > >   drivers/iommu/io-pgtable-arm.c     |  7 +++----
> > >   drivers/iommu/ipmmu-vmsa.c         |  2 +-
> > >   drivers/iommu/msm_iommu.c          |  4 ++--
> > >   drivers/iommu/mtk_iommu.c          |  4 ++--
> > >   drivers/iommu/qcom_iommu.c         |  3 +--
> > >   include/linux/io-pgtable.h         |  4 ++--
> > >   9 files changed, 23 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > index 2a8db896d698..2e50cf49c3c4 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -1722,7 +1722,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> > >   	}
> > >   	cfg->cd.asid	= (u16)asid;
> > > -	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> > > +	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> > >   	cfg->cd.tcr	= pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> > >   	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
> > >   	return 0;
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index 184ca41e9de7..19030c4b5904 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -473,13 +473,12 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> > >   	/* TTBRs */
> > >   	if (stage1) {
> > >   		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> > > -			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> > > -			cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> > > +			cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
> > > +			cb->ttbr[1] = 0;
> > >   		} else {
> > > -			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> > > +			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> > >   			cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> > > -			cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> > > -			cb->ttbr[1] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> > > +			cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> > 
> > Why do you continue to put the ASID in here?
> 
> For the same reason we put it there before ;)
> 
> Although I guess if TCR.A1 were ever to get flipped accidentally then we're
> still cool.

Hmm, but we don't do this for other drivers, so I'd be inclined to zap it
with 0 for consistency.

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Robin Murphy]

Although it's conceptually nice for the io_pgtable_cfg to provide a
standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
looks exactly like an Arm CPU, and they all have various other TCR
controls which io-pgtable can't be expected to understand. Thus since
there is an expectation that drivers will have to add to the given TCR
value anyway, let's strip it down to just the essentials that are
directly relevant to io-pgatble's inner workings - namely the address
sizes, walk attributes, and where appropriate, format selection.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu-v3.c        | 7 +------
 drivers/iommu/arm-smmu.c           | 1 +
 drivers/iommu/arm-smmu.h           | 2 ++
 drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
 drivers/iommu/io-pgtable-arm.c     | 4 ----
 drivers/iommu/qcom_iommu.c         | 2 +-
 6 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2e50cf49c3c4..c8e83f72893f 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -270,9 +270,7 @@
 #define CTXDESC_CD_0_TCR_SH0		GENMASK_ULL(13, 12)
 #define ARM64_TCR_SH0			GENMASK_ULL(13, 12)
 #define CTXDESC_CD_0_TCR_EPD0		(1ULL << 14)
-#define ARM64_TCR_EPD0			(1ULL << 7)
 #define CTXDESC_CD_0_TCR_EPD1		(1ULL << 30)
-#define ARM64_TCR_EPD1			(1ULL << 23)
 
 #define CTXDESC_CD_0_ENDI		(1UL << 15)
 #define CTXDESC_CD_0_V			(1UL << 31)
@@ -280,7 +278,6 @@
 #define CTXDESC_CD_0_TCR_IPS		GENMASK_ULL(34, 32)
 #define ARM64_TCR_IPS			GENMASK_ULL(34, 32)
 #define CTXDESC_CD_0_TCR_TBI0		(1ULL << 38)
-#define ARM64_TCR_TBI0			(1ULL << 37)
 
 #define CTXDESC_CD_0_AA64		(1UL << 41)
 #define CTXDESC_CD_0_S			(1UL << 44)
@@ -1064,8 +1061,6 @@ static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr)
 	val |= ARM_SMMU_TCR2CD(tcr, IRGN0);
 	val |= ARM_SMMU_TCR2CD(tcr, ORGN0);
 	val |= ARM_SMMU_TCR2CD(tcr, SH0);
-	val |= ARM_SMMU_TCR2CD(tcr, EPD0);
-	val |= ARM_SMMU_TCR2CD(tcr, EPD1);
 	val |= ARM_SMMU_TCR2CD(tcr, IPS);
 
 	return val;
@@ -1086,7 +1081,7 @@ static void arm_smmu_write_ctx_desc(struct arm_smmu_device *smmu,
 #endif
 	      CTXDESC_CD_0_R | CTXDESC_CD_0_A | CTXDESC_CD_0_ASET |
 	      CTXDESC_CD_0_AA64 | FIELD_PREP(CTXDESC_CD_0_ASID, cfg->cd.asid) |
-	      CTXDESC_CD_0_V;
+	      CTXDESC_CD_0_V | CTXDESC_CD_0_TCR_EPD1;
 
 	/* STALL_MODEL==0b10 && CD.S==0 is ILLEGAL */
 	if (smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 19030c4b5904..49c734a3814b 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -461,6 +461,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 			cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
 		} else {
 			cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
+			cb->tcr[0] |= TCR_EPD1;
 			cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
 			cb->tcr[1] |= FIELD_PREP(TCR2_SEP, TCR2_SEP_UPSTREAM);
 			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index ac9eac966cf5..7b0e4d238558 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -162,6 +162,8 @@ enum arm_smmu_cbar_type {
 #define TTBRn_ASID			GENMASK_ULL(63, 48)
 
 #define ARM_SMMU_CB_TCR			0x30
+#define TCR_EPD1			BIT(23)
+
 #define ARM_SMMU_CB_CONTEXTIDR		0x34
 #define ARM_SMMU_CB_S1_MAIR0		0x38
 #define ARM_SMMU_CB_S1_MAIR1		0x3c
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index 7c80e9d00f73..a53f8d904f66 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -147,8 +147,6 @@
 #define ARM_V7S_TTBR_IRGN_ATTR(attr)					\
 	((((attr) & 0x1) << 6) | (((attr) & 0x2) >> 1))
 
-#define ARM_V7S_TCR_PD1			BIT(5)
-
 #ifdef CONFIG_ZONE_DMA32
 #define ARM_V7S_TABLE_GFP_DMA GFP_DMA32
 #define ARM_V7S_TABLE_SLAB_FLAGS SLAB_CACHE_DMA32
@@ -751,8 +749,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
 	 */
 	cfg->pgsize_bitmap &= SZ_4K | SZ_64K | SZ_1M | SZ_16M;
 
-	/* TCR: T0SZ=0, disable TTBR1 */
-	cfg->arm_v7s_cfg.tcr = ARM_V7S_TCR_PD1;
+	/* TCR: T0SZ=0, EAE=0 (if applicable) */
+	cfg->arm_v7s_cfg.tcr = 0;
 
 	/*
 	 * TEX remap: the indices used map to the closest equivalent types
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 56e0921331c3..09cb20671fbb 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -112,8 +112,6 @@
 #define ARM_32_LPAE_TCR_EAE		(1 << 31)
 #define ARM_64_LPAE_S2_TCR_RES1		(1 << 31)
 
-#define ARM_LPAE_TCR_EPD1		(1 << 23)
-
 #define ARM_LPAE_TCR_TG0_4K		(0 << 14)
 #define ARM_LPAE_TCR_TG0_64K		(1 << 14)
 #define ARM_LPAE_TCR_TG0_16K		(2 << 14)
@@ -845,8 +843,6 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 
 	reg |= (64ULL - cfg->ias) << ARM_LPAE_TCR_T0SZ_SHIFT;
 
-	/* Disable speculative walks through TTBR1 */
-	reg |= ARM_LPAE_TCR_EPD1;
 	cfg->arm_lpae_s1_cfg.tcr = reg;
 
 	/* MAIRs */
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index de55b6d82ef1..c58237bb2a86 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -257,7 +257,7 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 				(pgtbl_cfg.arm_lpae_s1_cfg.tcr >> 32) |
 				FIELD_PREP(TCR2_SEP, TCR2_SEP_UPSTREAM));
 		iommu_writel(ctx, ARM_SMMU_CB_TCR,
-				pgtbl_cfg.arm_lpae_s1_cfg.tcr);
+				pgtbl_cfg.arm_lpae_s1_cfg.tcr | TCR_EPD1);
 
 		/* MAIRs (stage-1 only) */
 		iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR0,
-- 
2.21.0.dirty

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Will Deacon]

On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> Although it's conceptually nice for the io_pgtable_cfg to provide a
> standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> looks exactly like an Arm CPU, and they all have various other TCR
> controls which io-pgtable can't be expected to understand. Thus since
> there is an expectation that drivers will have to add to the given TCR
> value anyway, let's strip it down to just the essentials that are
> directly relevant to io-pgatble's inner workings - namely the address
> sizes, walk attributes, and where appropriate, format selection.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu-v3.c        | 7 +------
>  drivers/iommu/arm-smmu.c           | 1 +
>  drivers/iommu/arm-smmu.h           | 2 ++
>  drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
>  drivers/iommu/io-pgtable-arm.c     | 4 ----
>  drivers/iommu/qcom_iommu.c         | 2 +-
>  6 files changed, 7 insertions(+), 15 deletions(-)

Hmm, so I'm a bit nervous about this one since I think we really should
be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
on the driver to do this worries me. See my comments on the next patch.

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Robin Murphy]

On 20/08/2019 11:31, Will Deacon wrote:
> On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
>> Although it's conceptually nice for the io_pgtable_cfg to provide a
>> standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
>> looks exactly like an Arm CPU, and they all have various other TCR
>> controls which io-pgtable can't be expected to understand. Thus since
>> there is an expectation that drivers will have to add to the given TCR
>> value anyway, let's strip it down to just the essentials that are
>> directly relevant to io-pgatble's inner workings - namely the address
>> sizes, walk attributes, and where appropriate, format selection.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c        | 7 +------
>>   drivers/iommu/arm-smmu.c           | 1 +
>>   drivers/iommu/arm-smmu.h           | 2 ++
>>   drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
>>   drivers/iommu/io-pgtable-arm.c     | 4 ----
>>   drivers/iommu/qcom_iommu.c         | 2 +-
>>   6 files changed, 7 insertions(+), 15 deletions(-)
> 
> Hmm, so I'm a bit nervous about this one since I think we really should
> be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> on the driver to do this worries me. See my comments on the next patch.

The whole idea is that we already know we can't provide a *complete* TCR 
value (not least because anything above bit 31 is the wild west), thus 
there's really no point in io-pgtable trying to provide anything other 
than the parts it definitely controls. It makes sense to provide this 
partial TCR value "as if" for TTBR0, since that's the most common case, 
but ultimately io-pgatble doesn't know (or need to) which TTBR the 
caller intends to actually use for this table. Even if the caller *is* 
allocating it for TTBR0, io-pgtable doesn't know that they haven't got 
something live in TTBR1 already, so it still wouldn't be in a position 
to make the EPD1 call either way.

Ultimately, it's the IOMMU drivers who decide what they put in which 
TTBR, so it's the IOMMU drivers which have to take responsibility for 
EPD*. Sure you can worry about it, but you can equally worry about them 
them misprogramming the ASID or anything else...

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Will Deacon]

On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
> On 20/08/2019 11:31, Will Deacon wrote:
> > On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> > > Although it's conceptually nice for the io_pgtable_cfg to provide a
> > > standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> > > looks exactly like an Arm CPU, and they all have various other TCR
> > > controls which io-pgtable can't be expected to understand. Thus since
> > > there is an expectation that drivers will have to add to the given TCR
> > > value anyway, let's strip it down to just the essentials that are
> > > directly relevant to io-pgatble's inner workings - namely the address
> > > sizes, walk attributes, and where appropriate, format selection.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/iommu/arm-smmu-v3.c        | 7 +------
> > >   drivers/iommu/arm-smmu.c           | 1 +
> > >   drivers/iommu/arm-smmu.h           | 2 ++
> > >   drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
> > >   drivers/iommu/io-pgtable-arm.c     | 4 ----
> > >   drivers/iommu/qcom_iommu.c         | 2 +-
> > >   6 files changed, 7 insertions(+), 15 deletions(-)
> > 
> > Hmm, so I'm a bit nervous about this one since I think we really should
> > be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> > on the driver to do this worries me. See my comments on the next patch.
> 
> The whole idea is that we already know we can't provide a *complete* TCR
> value (not least because anything above bit 31 is the wild west), thus
> there's really no point in io-pgtable trying to provide anything other than
> the parts it definitely controls. It makes sense to provide this partial TCR
> value "as if" for TTBR0, since that's the most common case, but ultimately
> io-pgatble doesn't know (or need to) which TTBR the caller intends to
> actually use for this table. Even if the caller *is* allocating it for
> TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
> already, so it still wouldn't be in a position to make the EPD1 call either
> way.

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
an alternative would be scrapped the 'u64 tcr' and instead having a bunch
of named bitfields for the stuff we're actually providing, although I'd
still like EPDx to be in there.

> Ultimately, it's the IOMMU drivers who decide what they put in which TTBR,
> so it's the IOMMU drivers which have to take responsibility for EPD*. Sure
> you can worry about it, but you can equally worry about them them
> misprogramming the ASID or anything else...

I find the EPDx bits particularly dangerous because:

  - They're easily overlooked
  - Clobbering TTBR1 with 0x0 doesn't disable walks via TTBR1 as you might
    reasonably expect
  - If you do the above without EPD, the breakage will be subtle

and given that I don't see any real downsides to us providing a default TCR
value with EPD set appropriately, then I think we should do that. I'd be
happy to revisit the decision later on if it's getting the way of a real
use-case, but it feels like we're throwing the baby out with the bathwater
at the moment and I'd rather do this incrementally based on actual need.

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Robin Murphy]

On 20/08/2019 17:07, Will Deacon wrote:
> On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
>> On 20/08/2019 11:31, Will Deacon wrote:
>>> On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
>>>> Although it's conceptually nice for the io_pgtable_cfg to provide a
>>>> standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
>>>> looks exactly like an Arm CPU, and they all have various other TCR
>>>> controls which io-pgtable can't be expected to understand. Thus since
>>>> there is an expectation that drivers will have to add to the given TCR
>>>> value anyway, let's strip it down to just the essentials that are
>>>> directly relevant to io-pgatble's inner workings - namely the address
>>>> sizes, walk attributes, and where appropriate, format selection.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>    drivers/iommu/arm-smmu-v3.c        | 7 +------
>>>>    drivers/iommu/arm-smmu.c           | 1 +
>>>>    drivers/iommu/arm-smmu.h           | 2 ++
>>>>    drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
>>>>    drivers/iommu/io-pgtable-arm.c     | 4 ----
>>>>    drivers/iommu/qcom_iommu.c         | 2 +-
>>>>    6 files changed, 7 insertions(+), 15 deletions(-)
>>>
>>> Hmm, so I'm a bit nervous about this one since I think we really should
>>> be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
>>> on the driver to do this worries me. See my comments on the next patch.
>>
>> The whole idea is that we already know we can't provide a *complete* TCR
>> value (not least because anything above bit 31 is the wild west), thus
>> there's really no point in io-pgtable trying to provide anything other than
>> the parts it definitely controls. It makes sense to provide this partial TCR
>> value "as if" for TTBR0, since that's the most common case, but ultimately
>> io-pgatble doesn't know (or need to) which TTBR the caller intends to
>> actually use for this table. Even if the caller *is* allocating it for
>> TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
>> already, so it still wouldn't be in a position to make the EPD1 call either
>> way.
> 
> Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
> an alternative would be scrapped the 'u64 tcr' and instead having a bunch
> of named bitfields for the stuff we're actually providing, although I'd
> still like EPDx to be in there.

I like the bitfield idea; it would certainly emphasise the "you have to 
do something more with this" angle that I'm pushing towards here, but 
still leave things framed in TCR terms without having to go to some more 
general abstraction. It really doesn't play into your EPD argument 
though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but 
EPDy, for y = !x. For a driver to understand that and do the right thing 
with it is even more involved than for the driver to just set EPD1 by 
itself anyway.

>> Ultimately, it's the IOMMU drivers who decide what they put in which TTBR,
>> so it's the IOMMU drivers which have to take responsibility for EPD*. Sure
>> you can worry about it, but you can equally worry about them them
>> misprogramming the ASID or anything else...
> 
> I find the EPDx bits particularly dangerous because:
> 
>    - They're easily overlooked
>    - Clobbering TTBR1 with 0x0 doesn't disable walks via TTBR1 as you might
>      reasonably expect

(FWIW I'm not sure that that is a reasonable expectation, at least for 
anyone savvy enough to be programming an MMU in the first place. There 
are plenty of systems with RAM at 0x0)

>    - If you do the above without EPD, the breakage will be subtle
> 
> and given that I don't see any real downsides to us providing a default TCR
> value with EPD set appropriately, then I think we should do that. I'd be
> happy to revisit the decision later on if it's getting the way of a real
> use-case, but it feels like we're throwing the baby out with the bathwater
> at the moment and I'd rather do this incrementally based on actual need.
The downside is maintaining extra complexity for the sake of a 
theoretical concern which hasn't been borne out in practice, with a 
promise of yet more complexity down the line. Moving the 3 babies which 
anyone acknowledges into their own private baths takes a whole -8 lines 
of code to implement. That said, if we don't go down the bitfield route, 
then I *can* leave the default TCR having EPD1 set if that makes you 
feel warm and safe, but it will still be resoundingly ignored.

If only LPAE had created these bits as enables rather than disables then 
things would be logical and we could all be happy, but here we are...

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Will Deacon]

On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote:
> On 20/08/2019 17:07, Will Deacon wrote:
> > On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
> > > On 20/08/2019 11:31, Will Deacon wrote:
> > > > On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> > > > > Although it's conceptually nice for the io_pgtable_cfg to provide a
> > > > > standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> > > > > looks exactly like an Arm CPU, and they all have various other TCR
> > > > > controls which io-pgtable can't be expected to understand. Thus since
> > > > > there is an expectation that drivers will have to add to the given TCR
> > > > > value anyway, let's strip it down to just the essentials that are
> > > > > directly relevant to io-pgatble's inner workings - namely the address
> > > > > sizes, walk attributes, and where appropriate, format selection.
> > > > > 
> > > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > > ---
> > > > >    drivers/iommu/arm-smmu-v3.c        | 7 +------
> > > > >    drivers/iommu/arm-smmu.c           | 1 +
> > > > >    drivers/iommu/arm-smmu.h           | 2 ++
> > > > >    drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
> > > > >    drivers/iommu/io-pgtable-arm.c     | 4 ----
> > > > >    drivers/iommu/qcom_iommu.c         | 2 +-
> > > > >    6 files changed, 7 insertions(+), 15 deletions(-)
> > > > 
> > > > Hmm, so I'm a bit nervous about this one since I think we really should
> > > > be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> > > > on the driver to do this worries me. See my comments on the next patch.
> > > 
> > > The whole idea is that we already know we can't provide a *complete* TCR
> > > value (not least because anything above bit 31 is the wild west), thus
> > > there's really no point in io-pgtable trying to provide anything other than
> > > the parts it definitely controls. It makes sense to provide this partial TCR
> > > value "as if" for TTBR0, since that's the most common case, but ultimately
> > > io-pgatble doesn't know (or need to) which TTBR the caller intends to
> > > actually use for this table. Even if the caller *is* allocating it for
> > > TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
> > > already, so it still wouldn't be in a position to make the EPD1 call either
> > > way.
> > 
> > Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
> > an alternative would be scrapped the 'u64 tcr' and instead having a bunch
> > of named bitfields for the stuff we're actually providing, although I'd
> > still like EPDx to be in there.
> 
> I like the bitfield idea; it would certainly emphasise the "you have to do
> something more with this" angle that I'm pushing towards here, but still
> leave things framed in TCR terms without having to go to some more general
> abstraction. It really doesn't play into your EPD argument though - such a
> config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x.
> For a driver to understand that and do the right thing with it is even more
> involved than for the driver to just set EPD1 by itself anyway.

Having considered the bitfield idea some more, I'm less attached to EPDx
because we simply wouldn't be making a statement about them, rather than a
(dangerous) zero value and expecting it to be ignored. So I think we're in
agreement on that.

The only part I'm still stuck to is that I think io-pgtable should know
whether it's targetting TTBR0 or TTBR1 so that it can sanitise input
addresses correctly. Doing this in the driver code is possible, but I'd
rather not start from that position, particularly as it would require things
like sign-extension in the TLBI callbacks.

> If only LPAE had created these bits as enables rather than disables then
> things would be logical and we could all be happy, but here we are...

I'm happy! :D:D:D

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Robin Murphy]

On 21/08/2019 13:11, Will Deacon wrote:
> On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote:
>> On 20/08/2019 17:07, Will Deacon wrote:
>>> On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
>>>> On 20/08/2019 11:31, Will Deacon wrote:
>>>>> On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
>>>>>> Although it's conceptually nice for the io_pgtable_cfg to provide a
>>>>>> standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
>>>>>> looks exactly like an Arm CPU, and they all have various other TCR
>>>>>> controls which io-pgtable can't be expected to understand. Thus since
>>>>>> there is an expectation that drivers will have to add to the given TCR
>>>>>> value anyway, let's strip it down to just the essentials that are
>>>>>> directly relevant to io-pgatble's inner workings - namely the address
>>>>>> sizes, walk attributes, and where appropriate, format selection.
>>>>>>
>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>> ---
>>>>>>     drivers/iommu/arm-smmu-v3.c        | 7 +------
>>>>>>     drivers/iommu/arm-smmu.c           | 1 +
>>>>>>     drivers/iommu/arm-smmu.h           | 2 ++
>>>>>>     drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
>>>>>>     drivers/iommu/io-pgtable-arm.c     | 4 ----
>>>>>>     drivers/iommu/qcom_iommu.c         | 2 +-
>>>>>>     6 files changed, 7 insertions(+), 15 deletions(-)
>>>>>
>>>>> Hmm, so I'm a bit nervous about this one since I think we really should
>>>>> be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
>>>>> on the driver to do this worries me. See my comments on the next patch.
>>>>
>>>> The whole idea is that we already know we can't provide a *complete* TCR
>>>> value (not least because anything above bit 31 is the wild west), thus
>>>> there's really no point in io-pgtable trying to provide anything other than
>>>> the parts it definitely controls. It makes sense to provide this partial TCR
>>>> value "as if" for TTBR0, since that's the most common case, but ultimately
>>>> io-pgatble doesn't know (or need to) which TTBR the caller intends to
>>>> actually use for this table. Even if the caller *is* allocating it for
>>>> TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
>>>> already, so it still wouldn't be in a position to make the EPD1 call either
>>>> way.
>>>
>>> Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
>>> an alternative would be scrapped the 'u64 tcr' and instead having a bunch
>>> of named bitfields for the stuff we're actually providing, although I'd
>>> still like EPDx to be in there.
>>
>> I like the bitfield idea; it would certainly emphasise the "you have to do
>> something more with this" angle that I'm pushing towards here, but still
>> leave things framed in TCR terms without having to go to some more general
>> abstraction. It really doesn't play into your EPD argument though - such a
>> config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x.
>> For a driver to understand that and do the right thing with it is even more
>> involved than for the driver to just set EPD1 by itself anyway.
> 
> Having considered the bitfield idea some more, I'm less attached to EPDx
> because we simply wouldn't be making a statement about them, rather than a
> (dangerous) zero value and expecting it to be ignored. So I think we're in
> agreement on that.

Cool, I'll give bitfields a go for v2.

> The only part I'm still stuck to is that I think io-pgtable should know
> whether it's targetting TTBR0 or TTBR1 so that it can sanitise input
> addresses correctly. Doing this in the driver code is possible, but I'd
> rather not start from that position, particularly as it would require things
> like sign-extension in the TLBI callbacks.

Good point, and thanks for the prod that the way I end up propagating 
masked-off IOVAs through to the TLBI calls here is busted either way. 
I'm OK with introducing an explicit TTBR1 quirk to begin with, as it 
should make things a little easier to reason about, and we can always 
revisit later if and when we do find a need for more flexibility.

>> If only LPAE had created these bits as enables rather than disables then
>> things would be logical and we could all be happy, but here we are...
> 
> I'm happy! :D:D:D

Yeah, but I refuse to believe it's ever because of pagetables ;)

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Jordan Crouse]

On Wed, Aug 21, 2019 at 01:56:20PM +0100, Robin Murphy wrote:
> On 21/08/2019 13:11, Will Deacon wrote:
> >On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote:
> >>On 20/08/2019 17:07, Will Deacon wrote:
> >>>On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
> >>>>On 20/08/2019 11:31, Will Deacon wrote:
> >>>>>On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> >>>>>>Although it's conceptually nice for the io_pgtable_cfg to provide a
> >>>>>>standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> >>>>>>looks exactly like an Arm CPU, and they all have various other TCR
> >>>>>>controls which io-pgtable can't be expected to understand. Thus since
> >>>>>>there is an expectation that drivers will have to add to the given TCR
> >>>>>>value anyway, let's strip it down to just the essentials that are
> >>>>>>directly relevant to io-pgatble's inner workings - namely the address
> >>>>>>sizes, walk attributes, and where appropriate, format selection.
> >>>>>>
> >>>>>>Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >>>>>>---
> >>>>>>    drivers/iommu/arm-smmu-v3.c        | 7 +------
> >>>>>>    drivers/iommu/arm-smmu.c           | 1 +
> >>>>>>    drivers/iommu/arm-smmu.h           | 2 ++
> >>>>>>    drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
> >>>>>>    drivers/iommu/io-pgtable-arm.c     | 4 ----
> >>>>>>    drivers/iommu/qcom_iommu.c         | 2 +-
> >>>>>>    6 files changed, 7 insertions(+), 15 deletions(-)
> >>>>>
> >>>>>Hmm, so I'm a bit nervous about this one since I think we really should
> >>>>>be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> >>>>>on the driver to do this worries me. See my comments on the next patch.
> >>>>
> >>>>The whole idea is that we already know we can't provide a *complete* TCR
> >>>>value (not least because anything above bit 31 is the wild west), thus
> >>>>there's really no point in io-pgtable trying to provide anything other than
> >>>>the parts it definitely controls. It makes sense to provide this partial TCR
> >>>>value "as if" for TTBR0, since that's the most common case, but ultimately
> >>>>io-pgatble doesn't know (or need to) which TTBR the caller intends to
> >>>>actually use for this table. Even if the caller *is* allocating it for
> >>>>TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
> >>>>already, so it still wouldn't be in a position to make the EPD1 call either
> >>>>way.
> >>>
> >>>Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
> >>>an alternative would be scrapped the 'u64 tcr' and instead having a bunch
> >>>of named bitfields for the stuff we're actually providing, although I'd
> >>>still like EPDx to be in there.
> >>
> >>I like the bitfield idea; it would certainly emphasise the "you have to do
> >>something more with this" angle that I'm pushing towards here, but still
> >>leave things framed in TCR terms without having to go to some more general
> >>abstraction. It really doesn't play into your EPD argument though - such a
> >>config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x.
> >>For a driver to understand that and do the right thing with it is even more
> >>involved than for the driver to just set EPD1 by itself anyway.
> >
> >Having considered the bitfield idea some more, I'm less attached to EPDx
> >because we simply wouldn't be making a statement about them, rather than a
> >(dangerous) zero value and expecting it to be ignored. So I think we're in
> >agreement on that.
> 
> Cool, I'll give bitfields a go for v2.
> 
> >The only part I'm still stuck to is that I think io-pgtable should know
> >whether it's targetting TTBR0 or TTBR1 so that it can sanitise input
> >addresses correctly. Doing this in the driver code is possible, but I'd
> >rather not start from that position, particularly as it would require things
> >like sign-extension in the TLBI callbacks.

Bumping this as is our tradition in the -rc1 time frame before we get all
distracted with other stuff. It sounds like the last agreement was for a
TTBR1 hint for the EDP and the sign extension in the functions.

Let me know if you need any help. I've got a little time and more than a little
motivation to keep slogging ahead toward a glorious arm-smmu-v2
per-context pagetable future.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Will Deacon]

On Thu, Oct 03, 2019 at 11:33:52AM -0600, Jordan Crouse wrote:
> On Wed, Aug 21, 2019 at 01:56:20PM +0100, Robin Murphy wrote:
> > On 21/08/2019 13:11, Will Deacon wrote:
> > >On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote:
> > >>On 20/08/2019 17:07, Will Deacon wrote:
> > >>>On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
> > >>>>On 20/08/2019 11:31, Will Deacon wrote:
> > >>>>>On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> > >>>>>>Although it's conceptually nice for the io_pgtable_cfg to provide a
> > >>>>>>standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> > >>>>>>looks exactly like an Arm CPU, and they all have various other TCR
> > >>>>>>controls which io-pgtable can't be expected to understand. Thus since
> > >>>>>>there is an expectation that drivers will have to add to the given TCR
> > >>>>>>value anyway, let's strip it down to just the essentials that are
> > >>>>>>directly relevant to io-pgatble's inner workings - namely the address
> > >>>>>>sizes, walk attributes, and where appropriate, format selection.
> > >>>>>>
> > >>>>>>Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > >>>>>>---
> > >>>>>>    drivers/iommu/arm-smmu-v3.c        | 7 +------
> > >>>>>>    drivers/iommu/arm-smmu.c           | 1 +
> > >>>>>>    drivers/iommu/arm-smmu.h           | 2 ++
> > >>>>>>    drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
> > >>>>>>    drivers/iommu/io-pgtable-arm.c     | 4 ----
> > >>>>>>    drivers/iommu/qcom_iommu.c         | 2 +-
> > >>>>>>    6 files changed, 7 insertions(+), 15 deletions(-)
> > >>>>>
> > >>>>>Hmm, so I'm a bit nervous about this one since I think we really should
> > >>>>>be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> > >>>>>on the driver to do this worries me. See my comments on the next patch.
> > >>>>
> > >>>>The whole idea is that we already know we can't provide a *complete* TCR
> > >>>>value (not least because anything above bit 31 is the wild west), thus
> > >>>>there's really no point in io-pgtable trying to provide anything other than
> > >>>>the parts it definitely controls. It makes sense to provide this partial TCR
> > >>>>value "as if" for TTBR0, since that's the most common case, but ultimately
> > >>>>io-pgatble doesn't know (or need to) which TTBR the caller intends to
> > >>>>actually use for this table. Even if the caller *is* allocating it for
> > >>>>TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
> > >>>>already, so it still wouldn't be in a position to make the EPD1 call either
> > >>>>way.
> > >>>
> > >>>Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
> > >>>an alternative would be scrapped the 'u64 tcr' and instead having a bunch
> > >>>of named bitfields for the stuff we're actually providing, although I'd
> > >>>still like EPDx to be in there.
> > >>
> > >>I like the bitfield idea; it would certainly emphasise the "you have to do
> > >>something more with this" angle that I'm pushing towards here, but still
> > >>leave things framed in TCR terms without having to go to some more general
> > >>abstraction. It really doesn't play into your EPD argument though - such a
> > >>config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x.
> > >>For a driver to understand that and do the right thing with it is even more
> > >>involved than for the driver to just set EPD1 by itself anyway.
> > >
> > >Having considered the bitfield idea some more, I'm less attached to EPDx
> > >because we simply wouldn't be making a statement about them, rather than a
> > >(dangerous) zero value and expecting it to be ignored. So I think we're in
> > >agreement on that.
> > 
> > Cool, I'll give bitfields a go for v2.
> > 
> > >The only part I'm still stuck to is that I think io-pgtable should know
> > >whether it's targetting TTBR0 or TTBR1 so that it can sanitise input
> > >addresses correctly. Doing this in the driver code is possible, but I'd
> > >rather not start from that position, particularly as it would require things
> > >like sign-extension in the TLBI callbacks.
> 
> Bumping this as is our tradition in the -rc1 time frame before we get all
> distracted with other stuff. It sounds like the last agreement was for a
> TTBR1 hint for the EDP and the sign extension in the functions.

If somebody respins this using bitfields and an explicit TTBR1 quirk then
I'll merge it.

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Robin Murphy]

On 2019-10-24 11:51 am, Will Deacon wrote:
> On Thu, Oct 03, 2019 at 11:33:52AM -0600, Jordan Crouse wrote:
>> On Wed, Aug 21, 2019 at 01:56:20PM +0100, Robin Murphy wrote:
>>> On 21/08/2019 13:11, Will Deacon wrote:
>>>> On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote:
>>>>> On 20/08/2019 17:07, Will Deacon wrote:
>>>>>> On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
>>>>>>> On 20/08/2019 11:31, Will Deacon wrote:
>>>>>>>> On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
>>>>>>>>> Although it's conceptually nice for the io_pgtable_cfg to provide a
>>>>>>>>> standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
>>>>>>>>> looks exactly like an Arm CPU, and they all have various other TCR
>>>>>>>>> controls which io-pgtable can't be expected to understand. Thus since
>>>>>>>>> there is an expectation that drivers will have to add to the given TCR
>>>>>>>>> value anyway, let's strip it down to just the essentials that are
>>>>>>>>> directly relevant to io-pgatble's inner workings - namely the address
>>>>>>>>> sizes, walk attributes, and where appropriate, format selection.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/iommu/arm-smmu-v3.c        | 7 +------
>>>>>>>>>     drivers/iommu/arm-smmu.c           | 1 +
>>>>>>>>>     drivers/iommu/arm-smmu.h           | 2 ++
>>>>>>>>>     drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
>>>>>>>>>     drivers/iommu/io-pgtable-arm.c     | 4 ----
>>>>>>>>>     drivers/iommu/qcom_iommu.c         | 2 +-
>>>>>>>>>     6 files changed, 7 insertions(+), 15 deletions(-)
>>>>>>>>
>>>>>>>> Hmm, so I'm a bit nervous about this one since I think we really should
>>>>>>>> be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
>>>>>>>> on the driver to do this worries me. See my comments on the next patch.
>>>>>>>
>>>>>>> The whole idea is that we already know we can't provide a *complete* TCR
>>>>>>> value (not least because anything above bit 31 is the wild west), thus
>>>>>>> there's really no point in io-pgtable trying to provide anything other than
>>>>>>> the parts it definitely controls. It makes sense to provide this partial TCR
>>>>>>> value "as if" for TTBR0, since that's the most common case, but ultimately
>>>>>>> io-pgatble doesn't know (or need to) which TTBR the caller intends to
>>>>>>> actually use for this table. Even if the caller *is* allocating it for
>>>>>>> TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
>>>>>>> already, so it still wouldn't be in a position to make the EPD1 call either
>>>>>>> way.
>>>>>>
>>>>>> Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
>>>>>> an alternative would be scrapped the 'u64 tcr' and instead having a bunch
>>>>>> of named bitfields for the stuff we're actually providing, although I'd
>>>>>> still like EPDx to be in there.
>>>>>
>>>>> I like the bitfield idea; it would certainly emphasise the "you have to do
>>>>> something more with this" angle that I'm pushing towards here, but still
>>>>> leave things framed in TCR terms without having to go to some more general
>>>>> abstraction. It really doesn't play into your EPD argument though - such a
>>>>> config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x.
>>>>> For a driver to understand that and do the right thing with it is even more
>>>>> involved than for the driver to just set EPD1 by itself anyway.
>>>>
>>>> Having considered the bitfield idea some more, I'm less attached to EPDx
>>>> because we simply wouldn't be making a statement about them, rather than a
>>>> (dangerous) zero value and expecting it to be ignored. So I think we're in
>>>> agreement on that.
>>>
>>> Cool, I'll give bitfields a go for v2.
>>>
>>>> The only part I'm still stuck to is that I think io-pgtable should know
>>>> whether it's targetting TTBR0 or TTBR1 so that it can sanitise input
>>>> addresses correctly. Doing this in the driver code is possible, but I'd
>>>> rather not start from that position, particularly as it would require things
>>>> like sign-extension in the TLBI callbacks.
>>
>> Bumping this as is our tradition in the -rc1 time frame before we get all
>> distracted with other stuff. It sounds like the last agreement was for a
>> TTBR1 hint for the EDP and the sign extension in the functions.
> 
> If somebody respins this using bitfields and an explicit TTBR1 quirk then
> I'll merge it.

Oops, the ping did register, I just didn't react outwardly ;)

I have been working on v2, and plan to have something ready next week - 
the holdup was that I started refactoring all the argument passing since 
the number of things we have to carry through from one end of map/unmap 
to the other is getting a bit silly, but I think I can still finish the 
TTBR1 quirk without that, so if I don't get it cracked imminently then 
I'll put it aside to revisit later.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Will Deacon]

On Thu, Oct 24, 2019 at 12:23:46PM +0100, Robin Murphy wrote:
> On 2019-10-24 11:51 am, Will Deacon wrote:
> > On Thu, Oct 03, 2019 at 11:33:52AM -0600, Jordan Crouse wrote:
> > > On Wed, Aug 21, 2019 at 01:56:20PM +0100, Robin Murphy wrote:
> > > > On 21/08/2019 13:11, Will Deacon wrote:
> > > > > On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote:
> > > > > > On 20/08/2019 17:07, Will Deacon wrote:
> > > > > > > On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
> > > > > > > > On 20/08/2019 11:31, Will Deacon wrote:
> > > > > > > > > On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> > > > > > > > > > Although it's conceptually nice for the io_pgtable_cfg to provide a
> > > > > > > > > > standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> > > > > > > > > > looks exactly like an Arm CPU, and they all have various other TCR
> > > > > > > > > > controls which io-pgtable can't be expected to understand. Thus since
> > > > > > > > > > there is an expectation that drivers will have to add to the given TCR
> > > > > > > > > > value anyway, let's strip it down to just the essentials that are
> > > > > > > > > > directly relevant to io-pgatble's inner workings - namely the address
> > > > > > > > > > sizes, walk attributes, and where appropriate, format selection.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > > > > > > > ---
> > > > > > > > > >     drivers/iommu/arm-smmu-v3.c        | 7 +------
> > > > > > > > > >     drivers/iommu/arm-smmu.c           | 1 +
> > > > > > > > > >     drivers/iommu/arm-smmu.h           | 2 ++
> > > > > > > > > >     drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
> > > > > > > > > >     drivers/iommu/io-pgtable-arm.c     | 4 ----
> > > > > > > > > >     drivers/iommu/qcom_iommu.c         | 2 +-
> > > > > > > > > >     6 files changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > > 
> > > > > > > > > Hmm, so I'm a bit nervous about this one since I think we really should
> > > > > > > > > be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> > > > > > > > > on the driver to do this worries me. See my comments on the next patch.
> > > > > > > > 
> > > > > > > > The whole idea is that we already know we can't provide a *complete* TCR
> > > > > > > > value (not least because anything above bit 31 is the wild west), thus
> > > > > > > > there's really no point in io-pgtable trying to provide anything other than
> > > > > > > > the parts it definitely controls. It makes sense to provide this partial TCR
> > > > > > > > value "as if" for TTBR0, since that's the most common case, but ultimately
> > > > > > > > io-pgatble doesn't know (or need to) which TTBR the caller intends to
> > > > > > > > actually use for this table. Even if the caller *is* allocating it for
> > > > > > > > TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
> > > > > > > > already, so it still wouldn't be in a position to make the EPD1 call either
> > > > > > > > way.
> > > > > > > 
> > > > > > > Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
> > > > > > > an alternative would be scrapped the 'u64 tcr' and instead having a bunch
> > > > > > > of named bitfields for the stuff we're actually providing, although I'd
> > > > > > > still like EPDx to be in there.
> > > > > > 
> > > > > > I like the bitfield idea; it would certainly emphasise the "you have to do
> > > > > > something more with this" angle that I'm pushing towards here, but still
> > > > > > leave things framed in TCR terms without having to go to some more general
> > > > > > abstraction. It really doesn't play into your EPD argument though - such a
> > > > > > config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x.
> > > > > > For a driver to understand that and do the right thing with it is even more
> > > > > > involved than for the driver to just set EPD1 by itself anyway.
> > > > > 
> > > > > Having considered the bitfield idea some more, I'm less attached to EPDx
> > > > > because we simply wouldn't be making a statement about them, rather than a
> > > > > (dangerous) zero value and expecting it to be ignored. So I think we're in
> > > > > agreement on that.
> > > > 
> > > > Cool, I'll give bitfields a go for v2.
> > > > 
> > > > > The only part I'm still stuck to is that I think io-pgtable should know
> > > > > whether it's targetting TTBR0 or TTBR1 so that it can sanitise input
> > > > > addresses correctly. Doing this in the driver code is possible, but I'd
> > > > > rather not start from that position, particularly as it would require things
> > > > > like sign-extension in the TLBI callbacks.
> > > 
> > > Bumping this as is our tradition in the -rc1 time frame before we get all
> > > distracted with other stuff. It sounds like the last agreement was for a
> > > TTBR1 hint for the EDP and the sign extension in the functions.
> > 
> > If somebody respins this using bitfields and an explicit TTBR1 quirk then
> > I'll merge it.
> 
> Oops, the ping did register, I just didn't react outwardly ;)
> 
> I have been working on v2, and plan to have something ready next week - the
> holdup was that I started refactoring all the argument passing since the
> number of things we have to carry through from one end of map/unmap to the
> other is getting a bit silly, but I think I can still finish the TTBR1 quirk
> without that, so if I don't get it cracked imminently then I'll put it aside
> to revisit later.

Cheers, Robin! I'll keep an eye out for it.

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling

[Jordan Crouse]

On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
> On 20/08/2019 11:31, Will Deacon wrote:
> >On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> >>Although it's conceptually nice for the io_pgtable_cfg to provide a
> >>standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> >>looks exactly like an Arm CPU, and they all have various other TCR
> >>controls which io-pgtable can't be expected to understand. Thus since
> >>there is an expectation that drivers will have to add to the given TCR
> >>value anyway, let's strip it down to just the essentials that are
> >>directly relevant to io-pgatble's inner workings - namely the address
> >>sizes, walk attributes, and where appropriate, format selection.
> >>
> >>Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >>---
> >>  drivers/iommu/arm-smmu-v3.c        | 7 +------
> >>  drivers/iommu/arm-smmu.c           | 1 +
> >>  drivers/iommu/arm-smmu.h           | 2 ++
> >>  drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
> >>  drivers/iommu/io-pgtable-arm.c     | 4 ----
> >>  drivers/iommu/qcom_iommu.c         | 2 +-
> >>  6 files changed, 7 insertions(+), 15 deletions(-)
> >
> >Hmm, so I'm a bit nervous about this one since I think we really should
> >be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> >on the driver to do this worries me. See my comments on the next patch.
> 
> The whole idea is that we already know we can't provide a *complete* TCR
> value (not least because anything above bit 31 is the wild west), thus
> there's really no point in io-pgtable trying to provide anything other than
> the parts it definitely controls. It makes sense to provide this partial TCR
> value "as if" for TTBR0, since that's the most common case, but ultimately
> io-pgatble doesn't know (or need to) which TTBR the caller intends to
> actually use for this table. Even if the caller *is* allocating it for
> TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
> already, so it still wouldn't be in a position to make the EPD1 call either
> way.
> 
> Ultimately, it's the IOMMU drivers who decide what they put in which TTBR,
> so it's the IOMMU drivers which have to take responsibility for EPD*. Sure
> you can worry about it, but you can equally worry about them them
> misprogramming the ASID or anything else...

I agree. If the driver *does* want to use TTBR1 then it gets a bit uglier to
realize that the io-pgtable set the bit and mask it off. To me it is a lot
clearer if this is done explicitly in the driver especially if the driver is
where we are making the choice to use either ttbr0 or ttbr1.

It would probably be easier to see in actual code, and I'll have a patch
shortly on top of this series.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/4] iommu/io-pgtable-arm: Prepare for TTBR1 usage

[Robin Murphy]

Now that callers are free to use a given table for TTBR1 if they wish
(all they need do is shift the provided attributes when constructing
their final TCR value), the only remaining impediment is the address
validation on map/unmap. The fact that the LPAE address space split is
symmetric makes this easy to accommodate - by simplifying the current
range checks into explicit tests that address bits above IAS are all
zero, it then follows straightforwardly to add the inverse test to
allow the all-ones case as well.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/io-pgtable-arm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 09cb20671fbb..f39c50356351 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
 	arm_lpae_iopte *ptep = data->pgd;
 	int ret, lvl = ARM_LPAE_START_LVL(data);
 	arm_lpae_iopte prot;
+	long iaext = (long)iova >> data->iop.cfg.ias;
 
 	/* If no access, then nothing to do */
 	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
 		return 0;
 
-	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
-		    paddr >= (1ULL << data->iop.cfg.oas)))
+	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))
 		return -ERANGE;
 
 	prot = arm_lpae_prot_to_pte(data, iommu_prot);
@@ -647,8 +647,9 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
 	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
 	arm_lpae_iopte *ptep = data->pgd;
 	int lvl = ARM_LPAE_START_LVL(data);
+	long iaext = (long)iova >> data->iop.cfg.ias;
 
-	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias)))
+	if (WARN_ON(iaext && ~iaext))
 		return 0;
 
 	return __arm_lpae_unmap(data, iova, size, lvl, ptep);
-- 
2.21.0.dirty

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/io-pgtable-arm: Prepare for TTBR1 usage

[Jordan Crouse]

On Mon, Aug 19, 2019 at 07:19:31PM +0100, Robin Murphy wrote:
> Now that callers are free to use a given table for TTBR1 if they wish
> (all they need do is shift the provided attributes when constructing
> their final TCR value), the only remaining impediment is the address
> validation on map/unmap. The fact that the LPAE address space split is
> symmetric makes this easy to accommodate - by simplifying the current
> range checks into explicit tests that address bits above IAS are all
> zero, it then follows straightforwardly to add the inverse test to
> allow the all-ones case as well.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/io-pgtable-arm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 09cb20671fbb..f39c50356351 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>  	arm_lpae_iopte *ptep = data->pgd;
>  	int ret, lvl = ARM_LPAE_START_LVL(data);
>  	arm_lpae_iopte prot;
> +	long iaext = (long)iova >> data->iop.cfg.ias;
>  
>  	/* If no access, then nothing to do */
>  	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
>  		return 0;
>  
> -	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
> -		    paddr >= (1ULL << data->iop.cfg.oas)))
> +	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))
>  		return -ERANGE;
>  
>  	prot = arm_lpae_prot_to_pte(data, iommu_prot);

We'll want to cast away the sign extended bits before mapping the iova, this
might be a good patch for that too as long as we are calculating the iaext.

> @@ -647,8 +647,9 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
>  	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>  	arm_lpae_iopte *ptep = data->pgd;
>  	int lvl = ARM_LPAE_START_LVL(data);
> +	long iaext = (long)iova >> data->iop.cfg.ias;
>  
> -	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias)))
> +	if (WARN_ON(iaext && ~iaext))
>  		return 0;
>  
>  	return __arm_lpae_unmap(data, iova, size, lvl, ptep);

And here too.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/io-pgtable-arm: Prepare for TTBR1 usage

[Robin Murphy]

On 19/08/2019 23:34, Jordan Crouse wrote:
> On Mon, Aug 19, 2019 at 07:19:31PM +0100, Robin Murphy wrote:
>> Now that callers are free to use a given table for TTBR1 if they wish
>> (all they need do is shift the provided attributes when constructing
>> their final TCR value), the only remaining impediment is the address
>> validation on map/unmap. The fact that the LPAE address space split is
>> symmetric makes this easy to accommodate - by simplifying the current
>> range checks into explicit tests that address bits above IAS are all
>> zero, it then follows straightforwardly to add the inverse test to
>> allow the all-ones case as well.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/io-pgtable-arm.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
>> index 09cb20671fbb..f39c50356351 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>>   	arm_lpae_iopte *ptep = data->pgd;
>>   	int ret, lvl = ARM_LPAE_START_LVL(data);
>>   	arm_lpae_iopte prot;
>> +	long iaext = (long)iova >> data->iop.cfg.ias;
>>   
>>   	/* If no access, then nothing to do */
>>   	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
>>   		return 0;
>>   
>> -	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
>> -		    paddr >= (1ULL << data->iop.cfg.oas)))
>> +	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))
>>   		return -ERANGE;
>>   
>>   	prot = arm_lpae_prot_to_pte(data, iommu_prot);
> 
> We'll want to cast away the sign extended bits before mapping the iova, this
> might be a good patch for that too as long as we are calculating the iaext.

Ah good point, I'd forgotten that ARM_LPAE_LVL_IDX() doesn't actually 
cap to IAS if the top level is smaller than bits_per_level (I suppose we 
*could* make it do so for purity, but that's bound to hurt efficiency 
far more than just zeroing out the offending bits here).

Thanks,
Robin.

> 
>> @@ -647,8 +647,9 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
>>   	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>>   	arm_lpae_iopte *ptep = data->pgd;
>>   	int lvl = ARM_LPAE_START_LVL(data);
>> +	long iaext = (long)iova >> data->iop.cfg.ias;
>>   
>> -	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias)))
>> +	if (WARN_ON(iaext && ~iaext))
>>   		return 0;
>>   
>>   	return __arm_lpae_unmap(data, iova, size, lvl, ptep);
> 
> And here too.
> 
> Jordan
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/io-pgtable-arm: Prepare for TTBR1 usage

[Will Deacon]

On Mon, Aug 19, 2019 at 07:19:31PM +0100, Robin Murphy wrote:
> Now that callers are free to use a given table for TTBR1 if they wish
> (all they need do is shift the provided attributes when constructing
> their final TCR value), the only remaining impediment is the address
> validation on map/unmap. The fact that the LPAE address space split is
> symmetric makes this easy to accommodate - by simplifying the current
> range checks into explicit tests that address bits above IAS are all
> zero, it then follows straightforwardly to add the inverse test to
> allow the all-ones case as well.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/io-pgtable-arm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 09cb20671fbb..f39c50356351 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>  	arm_lpae_iopte *ptep = data->pgd;
>  	int ret, lvl = ARM_LPAE_START_LVL(data);
>  	arm_lpae_iopte prot;
> +	long iaext = (long)iova >> data->iop.cfg.ias;
>  
>  	/* If no access, then nothing to do */
>  	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
>  		return 0;
>  
> -	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
> -		    paddr >= (1ULL << data->iop.cfg.oas)))
> +	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))

I had to read that '&&' twice, but I see what you're doing now :)

>  		return -ERANGE;

This doesn't seem sufficient to prevent a mixture of TTBR1 and TTBR0
addresses from being mapped in the same TTBR. Perhaps we need a quirk for
TTBR1, which could then take care of setting EPDx appropriately?

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/io-pgtable-arm: Prepare for TTBR1 usage

[Robin Murphy]

On 20/08/2019 11:30, Will Deacon wrote:
> On Mon, Aug 19, 2019 at 07:19:31PM +0100, Robin Murphy wrote:
>> Now that callers are free to use a given table for TTBR1 if they wish
>> (all they need do is shift the provided attributes when constructing
>> their final TCR value), the only remaining impediment is the address
>> validation on map/unmap. The fact that the LPAE address space split is
>> symmetric makes this easy to accommodate - by simplifying the current
>> range checks into explicit tests that address bits above IAS are all
>> zero, it then follows straightforwardly to add the inverse test to
>> allow the all-ones case as well.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/io-pgtable-arm.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
>> index 09cb20671fbb..f39c50356351 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>>   	arm_lpae_iopte *ptep = data->pgd;
>>   	int ret, lvl = ARM_LPAE_START_LVL(data);
>>   	arm_lpae_iopte prot;
>> +	long iaext = (long)iova >> data->iop.cfg.ias;
>>   
>>   	/* If no access, then nothing to do */
>>   	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
>>   		return 0;
>>   
>> -	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
>> -		    paddr >= (1ULL << data->iop.cfg.oas)))
>> +	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))
> 
> I had to read that '&&' twice, but I see what you're doing now :)
> 
>>   		return -ERANGE;
> 
> This doesn't seem sufficient to prevent a mixture of TTBR1 and TTBR0
> addresses from being mapped in the same TTBR. Perhaps we need a quirk for
> TTBR1, which could then take care of setting EPDx appropriately?

Right, that's the one downside of going for the minimalist "io-pgtable 
doesn't even have to know" approach. On reflection, though, in that 
paradigm it should probably be the caller's responsibility to convert 
TTBR1 addresses to preserve the "as if TTBR0" illusion anyway :/

The advantage of not having a quirk is that it allows split address 
spaces to fit more closely with the aux_domain idea, i.e. we could 
allocate and initialise a domain without having to assume, or even care, 
whether it will end up attached as a primary or aux domain. It *might* 
even be potentially useful to have a domain attached to TTBR0 of one 
device's context and TTBR1 of another's at the same time, although 
that's pretty niche.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/io-pgtable-arm: Prepare for TTBR1 usage

[Will Deacon]

On Tue, Aug 20, 2019 at 03:51:45PM +0100, Robin Murphy wrote:
> On 20/08/2019 11:30, Will Deacon wrote:
> > On Mon, Aug 19, 2019 at 07:19:31PM +0100, Robin Murphy wrote:
> > > Now that callers are free to use a given table for TTBR1 if they wish
> > > (all they need do is shift the provided attributes when constructing
> > > their final TCR value), the only remaining impediment is the address
> > > validation on map/unmap. The fact that the LPAE address space split is
> > > symmetric makes this easy to accommodate - by simplifying the current
> > > range checks into explicit tests that address bits above IAS are all
> > > zero, it then follows straightforwardly to add the inverse test to
> > > allow the all-ones case as well.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/iommu/io-pgtable-arm.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > > index 09cb20671fbb..f39c50356351 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
> > >   	arm_lpae_iopte *ptep = data->pgd;
> > >   	int ret, lvl = ARM_LPAE_START_LVL(data);
> > >   	arm_lpae_iopte prot;
> > > +	long iaext = (long)iova >> data->iop.cfg.ias;
> > >   	/* If no access, then nothing to do */
> > >   	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> > >   		return 0;
> > > -	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
> > > -		    paddr >= (1ULL << data->iop.cfg.oas)))
> > > +	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))
> > 
> > I had to read that '&&' twice, but I see what you're doing now :)
> > 
> > >   		return -ERANGE;
> > 
> > This doesn't seem sufficient to prevent a mixture of TTBR1 and TTBR0
> > addresses from being mapped in the same TTBR. Perhaps we need a quirk for
> > TTBR1, which could then take care of setting EPDx appropriately?
> 
> Right, that's the one downside of going for the minimalist "io-pgtable
> doesn't even have to know" approach. On reflection, though, in that paradigm
> it should probably be the caller's responsibility to convert TTBR1 addresses
> to preserve the "as if TTBR0" illusion anyway :/

Right, and I'd rather not push stuff into the caller for the common case.
It's not exactly onerous to support this in io-pgtable. It's also why I'd
still like to keep the EPDx in there, because the callers that care can
rewrite the stuff, but at least we provided a default.

> The advantage of not having a quirk is that it allows split address spaces
> to fit more closely with the aux_domain idea, i.e. we could allocate and
> initialise a domain without having to assume, or even care, whether it will
> end up attached as a primary or aux domain. It *might* even be potentially
> useful to have a domain attached to TTBR0 of one device's context and TTBR1
> of another's at the same time, although that's pretty niche.

That sounds pretty theoretical to me at the moment.

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu