[PATCH 1/2] iommu/arm-smmu: Track context bank state

[PATCH 1/2] iommu/arm-smmu: Track context bank state

[Robin Murphy]

Echoing what we do for Stream Map Entries, maintain a software shadow
state for context bank configuration. With this in place, we are mere
moments away from blissfully easy suspend/resume support.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

Since the runtime PM discussion has come back again, I figured it was
probably about time to finish off my plan for system PM. Lightly tested
on Juno (MMU-401) with hibernation.

Robin.

 drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
 1 file changed, 97 insertions(+), 62 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index bc89b4d6c043..86897b7b81d8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -338,6 +338,13 @@ struct arm_smmu_smr {
 	bool				valid;
 };
 
+struct arm_smmu_cb {
+	u64				ttbr[2];
+	u32				tcr[2];
+	u32				mair[2];
+	struct arm_smmu_cfg		*cfg;
+};
+
 struct arm_smmu_master_cfg {
 	struct arm_smmu_device		*smmu;
 	s16				smendx[];
@@ -380,6 +387,7 @@ struct arm_smmu_device {
 	u32				num_context_banks;
 	u32				num_s2_context_banks;
 	DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
+	struct arm_smmu_cb		*cbs;
 	atomic_t			irptndx;
 
 	u32				num_mapping_groups;
@@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
-	u32 reg, reg2;
-	u64 reg64;
-	bool stage1;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
+	bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
+
+	cb->cfg = cfg;
+
+	/* TTBCR */
+	if (stage1) {
+		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+			cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
+		} else {
+			cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
+			cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
+			cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
+			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
+				cb->tcr[1] |= TTBCR2_AS;
+		}
+	} else {
+		cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
+	}
+
+	/* 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];
+		} else {
+			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+		}
+	} else {
+		cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
+	}
+
+	/* MAIRs (stage-1 only) */
+	if (stage1) {
+		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+			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];
+		}
+	}
+}
+
+static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
+{
+	u32 reg;
+	bool stage1;
+	struct arm_smmu_cb *cb = &smmu->cbs[idx];
+	struct arm_smmu_cfg *cfg = cb->cfg;
+	struct arm_smmu_cfg default_cfg = {0};
 	void __iomem *cb_base, *gr1_base;
 
+	if (!cfg)
+		cfg = &default_cfg;
+
 	gr1_base = ARM_SMMU_GR1(smmu);
 	stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
-	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
+	cb_base = ARM_SMMU_CB(smmu, idx);
 
+	/* CBA2R */
 	if (smmu->version > ARM_SMMU_V1) {
 		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
 			reg = CBA2R_RW64_64BIT;
@@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 		if (smmu->features & ARM_SMMU_FEAT_VMID16)
 			reg |= cfg->vmid << CBA2R_VMID_SHIFT;
 
-		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
+		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
 	}
 
 	/* CBAR */
@@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 		/* 8-bit VMIDs live in CBAR */
 		reg |= cfg->vmid << CBAR_VMID_SHIFT;
 	}
-	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
+	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
 
 	/*
 	 * TTBCR
 	 * We must write this before the TTBRs, since it determines the
 	 * access behaviour of some fields (in particular, ASID[15:8]).
 	 */
-	if (stage1) {
-		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
-			reg = pgtbl_cfg->arm_v7s_cfg.tcr;
-			reg2 = 0;
-		} else {
-			reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
-			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
-			reg2 |= TTBCR2_SEP_UPSTREAM;
-			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
-				reg2 |= TTBCR2_AS;
-		}
-		if (smmu->version > ARM_SMMU_V1)
-			writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
-	} else {
-		reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
-	}
-	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
+	if (stage1 && smmu->version > ARM_SMMU_V1)
+		writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
+	writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
 
 	/* TTBRs */
-	if (stage1) {
-		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
-			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
-			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
-			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
-			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
-			writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
-		} else {
-			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
-			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
-			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
-			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
-			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
-		}
+	if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+		writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
+		writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
+		writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
 	} else {
-		reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
-		writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
+		writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
+		writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
 	}
 
 	/* MAIRs (stage-1 only) */
 	if (stage1) {
-		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
-			reg = pgtbl_cfg->arm_v7s_cfg.prrr;
-			reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
-		} else {
-			reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
-			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
-		}
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
-		writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
+		writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
+		writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
 	}
 
 	/* SCTLR */
-	reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
-	if (stage1)
-		reg |= SCTLR_S1_ASIDPNE;
-#ifdef __BIG_ENDIAN
-	reg |= SCTLR_E;
-#endif
+	if (!cb->cfg) {
+		reg = 0;
+	} else {
+		reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
+		if (stage1)
+			reg |= SCTLR_S1_ASIDPNE;
+		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+			reg |= SCTLR_E;
+	}
 	writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
 }
 
@@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 
 	/* Initialise the context bank with our page table cfg */
 	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
+	arm_smmu_write_context_bank(smmu, cfg->cbndx);
 
 	/*
 	 * Request context fault interrupt. Do this last to avoid the
@@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
-	void __iomem *cb_base;
 	int irq;
 
 	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
@@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	 * Disable the context bank and free the page tables before freeing
 	 * it.
 	 */
-	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
-	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+	smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
+	arm_smmu_write_context_bank(smmu, cfg->cbndx);
 
 	if (cfg->irptndx != INVALID_IRPTNDX) {
 		irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
@@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 {
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
-	void __iomem *cb_base;
 	int i;
 	u32 reg, major;
 
@@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 
 	/* Make sure all context banks are disabled and clear CB_FSR  */
 	for (i = 0; i < smmu->num_context_banks; ++i) {
-		cb_base = ARM_SMMU_CB(smmu, i);
-		writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+		void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
+
+		arm_smmu_write_context_bank(smmu, i);
 		writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
 		/*
 		 * Disable MMU-500's not-particularly-beneficial next-page
@@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		smmu->cavium_id_base -= smmu->num_context_banks;
 		dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
 	}
+	smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
+				 sizeof(*smmu->cbs), GFP_KERNEL);
+	if (!smmu->cbs)
+		return -ENOMEM;
 
 	/* ID2 */
 	id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
-- 
2.12.2.dirty

[PATCH 2/2] iommu/arm-smmu: Add system PM support

[Robin Murphy]

With all our hardware state tracked in such a way that we can naturally
restore it as part of the necessary reset, resuming is trivial, and
there's nothing to do on suspend at all.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 86897b7b81d8..0f5f06e9abfa 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	arm_smmu_device_reset(smmu);
+	return 0;
+}
+
+
+static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
+
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {
 		.name		= "arm-smmu",
 		.of_match_table	= of_match_ptr(arm_smmu_of_match),
+		.pm = &arm_smmu_pm_ops,
 	},
 	.probe	= arm_smmu_device_probe,
 	.remove	= arm_smmu_device_remove,
-- 
2.12.2.dirty

Re: [PATCH 2/2] iommu/arm-smmu: Add system PM support

[Will Deacon]

On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
> With all our hardware state tracked in such a way that we can naturally
> restore it as part of the necessary reset, resuming is trivial, and
> there's nothing to do on suspend at all.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 86897b7b81d8..0f5f06e9abfa 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> +{

Did you actually get a warning here without the __maybe_unused annotation?
It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.

> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> +	arm_smmu_device_reset(smmu);
> +	return 0;
> +}
> +
> +
> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +
>  static struct platform_driver arm_smmu_driver = {
>  	.driver	= {
>  		.name		= "arm-smmu",
>  		.of_match_table	= of_match_ptr(arm_smmu_of_match),
> +		.pm = &arm_smmu_pm_ops,

Cosmetic: can you tab-align this assignment please?

Will

Re: [PATCH 2/2] iommu/arm-smmu: Add system PM support

[Robin Murphy]

On 08/08/17 12:18, Will Deacon wrote:
> On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
>> With all our hardware state tracked in such a way that we can naturally
>> restore it as part of the necessary reset, resuming is trivial, and
>> there's nothing to do on suspend at all.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/iommu/arm-smmu.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 86897b7b81d8..0f5f06e9abfa 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>> +{
> 
> Did you actually get a warning here without the __maybe_unused annotation?
> It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.

I'm under the impression that the annotation is preferred over #ifdefs
for new code (for the sake of coverage, I guess).

>> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> +
>> +	arm_smmu_device_reset(smmu);
>> +	return 0;
>> +}
>> +
>> +
>> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
>> +
>>  static struct platform_driver arm_smmu_driver = {
>>  	.driver	= {
>>  		.name		= "arm-smmu",
>>  		.of_match_table	= of_match_ptr(arm_smmu_of_match),
>> +		.pm = &arm_smmu_pm_ops,
> 
> Cosmetic: can you tab-align this assignment please?

Oops, I missed that - will do.

Robin.

Re: [PATCH 2/2] iommu/arm-smmu: Add system PM support

[Jonathan Cameron]

On Tue, 8 Aug 2017 13:14:18 +0100
Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:

> On 08/08/17 12:18, Will Deacon wrote:
> > On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:  
> >> With all our hardware state tracked in such a way that we can naturally
> >> restore it as part of the necessary reset, resuming is trivial, and
> >> there's nothing to do on suspend at all.
> >>
> >> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >> ---
> >>  drivers/iommu/arm-smmu.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 86897b7b81d8..0f5f06e9abfa 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> >>  	return 0;
> >>  }
> >>  
> >> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> >> +{  
> > 
> > Did you actually get a warning here without the __maybe_unused annotation?
> > It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.  
> 
> I'm under the impression that the annotation is preferred over #ifdefs
> for new code (for the sake of coverage, I guess).
https://patchwork.kernel.org/patch/9734367/

Is a good thread discussing this.  Both coverage and to avoid common
pitfalls of the ifdef fun.

Jonathan
> 
> >> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> >> +
> >> +	arm_smmu_device_reset(smmu);
> >> +	return 0;
> >> +}
> >> +
> >> +
> >> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> >> +
> >>  static struct platform_driver arm_smmu_driver = {
> >>  	.driver	= {
> >>  		.name		= "arm-smmu",
> >>  		.of_match_table	= of_match_ptr(arm_smmu_of_match),
> >> +		.pm = &arm_smmu_pm_ops,  
> > 
> > Cosmetic: can you tab-align this assignment please?  
> 
> Oops, I missed that - will do.
> 
> Robin.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state

[Sricharan R]

Hi Robin,

On 7/18/2017 6:14 PM, Robin Murphy wrote:
> Echoing what we do for Stream Map Entries, maintain a software shadow
> state for context bank configuration. With this in place, we are mere
> moments away from blissfully easy suspend/resume support.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> Since the runtime PM discussion has come back again, I figured it was
> probably about time to finish off my plan for system PM. Lightly tested
> on Juno (MMU-401) with hibernation.
> 
> Robin.
> 
>  drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>  1 file changed, 97 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index bc89b4d6c043..86897b7b81d8 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -338,6 +338,13 @@ struct arm_smmu_smr {
>  	bool				valid;
>  };
>  
> +struct arm_smmu_cb {
> +	u64				ttbr[2];
> +	u32				tcr[2];
> +	u32				mair[2];
> +	struct arm_smmu_cfg		*cfg;
> +};
> +

 When i was trying this sometime back [1], was
 saving and using the pgtbl->cfg and domain->cfg for
 restoring the context. But this looks correct to make
 a actual shadow once and use it later all the time.
 Will also try some testing today.

 Reviewed-by: sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org

[1] https://patchwork.kernel.org/patch/9389717/

Regards,
 Sricharan


>  struct arm_smmu_master_cfg {
>  	struct arm_smmu_device		*smmu;
>  	s16				smendx[];
> @@ -380,6 +387,7 @@ struct arm_smmu_device {
>  	u32				num_context_banks;
>  	u32				num_s2_context_banks;
>  	DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
> +	struct arm_smmu_cb		*cbs;
>  	atomic_t			irptndx;
>  
>  	u32				num_mapping_groups;
> @@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>  static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>  				       struct io_pgtable_cfg *pgtbl_cfg)
>  {
> -	u32 reg, reg2;
> -	u64 reg64;
> -	bool stage1;
>  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> -	struct arm_smmu_device *smmu = smmu_domain->smmu;
> +	struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
> +	bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> +
> +	cb->cfg = cfg;
> +
> +	/* TTBCR */
> +	if (stage1) {
> +		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> +			cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
> +		} else {
> +			cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> +			cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> +			cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
> +			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> +				cb->tcr[1] |= TTBCR2_AS;
> +		}
> +	} else {
> +		cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> +	}
> +
> +	/* 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];
> +		} else {
> +			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> +			cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> +		}
> +	} else {
> +		cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> +	}
> +
> +	/* MAIRs (stage-1 only) */
> +	if (stage1) {
> +		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> +			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];
> +		}
> +	}
> +}
> +
> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> +{
> +	u32 reg;
> +	bool stage1;
> +	struct arm_smmu_cb *cb = &smmu->cbs[idx];
> +	struct arm_smmu_cfg *cfg = cb->cfg;
> +	struct arm_smmu_cfg default_cfg = {0};
>  	void __iomem *cb_base, *gr1_base;
>  
> +	if (!cfg)
> +		cfg = &default_cfg;
> +
>  	gr1_base = ARM_SMMU_GR1(smmu);
>  	stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> +	cb_base = ARM_SMMU_CB(smmu, idx);
>  
> +	/* CBA2R */
>  	if (smmu->version > ARM_SMMU_V1) {
>  		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>  			reg = CBA2R_RW64_64BIT;
> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>  		if (smmu->features & ARM_SMMU_FEAT_VMID16)
>  			reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>  
> -		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
> +		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>  	}
>  
>  	/* CBAR */
> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>  		/* 8-bit VMIDs live in CBAR */
>  		reg |= cfg->vmid << CBAR_VMID_SHIFT;
>  	}
> -	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
> +	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>  
>  	/*
>  	 * TTBCR
>  	 * We must write this before the TTBRs, since it determines the
>  	 * access behaviour of some fields (in particular, ASID[15:8]).
>  	 */
> -	if (stage1) {
> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> -			reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> -			reg2 = 0;
> -		} else {
> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> -			reg2 |= TTBCR2_SEP_UPSTREAM;
> -			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> -				reg2 |= TTBCR2_AS;
> -		}
> -		if (smmu->version > ARM_SMMU_V1)
> -			writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> -	} else {
> -		reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> -	}
> -	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> +	if (stage1 && smmu->version > ARM_SMMU_V1)
> +		writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
> +	writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>  
>  	/* TTBRs */
> -	if (stage1) {
> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
> -			writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> -		} else {
> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
> -		}
> +	if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> +		writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> +		writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> +		writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>  	} else {
> -		reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> -		writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> +		writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> +		writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>  	}
>  
>  	/* MAIRs (stage-1 only) */
>  	if (stage1) {
> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> -			reg = pgtbl_cfg->arm_v7s_cfg.prrr;
> -			reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
> -		} else {
> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> -		}
> -		writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
> -		writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
> +		writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
> +		writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>  	}
>  
>  	/* SCTLR */
> -	reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> -	if (stage1)
> -		reg |= SCTLR_S1_ASIDPNE;
> -#ifdef __BIG_ENDIAN
> -	reg |= SCTLR_E;
> -#endif
> +	if (!cb->cfg) {
> +		reg = 0;
> +	} else {
> +		reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> +		if (stage1)
> +			reg |= SCTLR_S1_ASIDPNE;
> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +			reg |= SCTLR_E;
> +	}
>  	writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>  }
>  
> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  
>  	/* Initialise the context bank with our page table cfg */
>  	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);
>  
>  	/*
>  	 * Request context fault interrupt. Do this last to avoid the
> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
>  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> -	void __iomem *cb_base;
>  	int irq;
>  
>  	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	 * Disable the context bank and free the page tables before freeing
>  	 * it.
>  	 */
> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> -	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> +	smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);
>  
>  	if (cfg->irptndx != INVALID_IRPTNDX) {
>  		irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
> @@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
>  static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  {
>  	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> -	void __iomem *cb_base;
>  	int i;
>  	u32 reg, major;
>  
> @@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  
>  	/* Make sure all context banks are disabled and clear CB_FSR  */
>  	for (i = 0; i < smmu->num_context_banks; ++i) {
> -		cb_base = ARM_SMMU_CB(smmu, i);
> -		writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> +		void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
> +
> +		arm_smmu_write_context_bank(smmu, i);
>  		writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
>  		/*
>  		 * Disable MMU-500's not-particularly-beneficial next-page
> @@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  		smmu->cavium_id_base -= smmu->num_context_banks;
>  		dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
>  	}
> +	smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
> +				 sizeof(*smmu->cbs), GFP_KERNEL);
> +	if (!smmu->cbs)
> +		return -ENOMEM;
>  
>  	/* ID2 */
>  	id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
> 

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state

[Robin Murphy]

On 19/07/17 03:40, Sricharan R wrote:
> Hi Robin,
> 
> On 7/18/2017 6:14 PM, Robin Murphy wrote:
>> Echoing what we do for Stream Map Entries, maintain a software shadow
>> state for context bank configuration. With this in place, we are mere
>> moments away from blissfully easy suspend/resume support.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> Since the runtime PM discussion has come back again, I figured it was
>> probably about time to finish off my plan for system PM. Lightly tested
>> on Juno (MMU-401) with hibernation.
>>
>> Robin.
>>
>>  drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 97 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index bc89b4d6c043..86897b7b81d8 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -338,6 +338,13 @@ struct arm_smmu_smr {
>>  	bool				valid;
>>  };
>>  
>> +struct arm_smmu_cb {
>> +	u64				ttbr[2];
>> +	u32				tcr[2];
>> +	u32				mair[2];
>> +	struct arm_smmu_cfg		*cfg;
>> +};
>> +
> 
>  When i was trying this sometime back [1], was
>  saving and using the pgtbl->cfg and domain->cfg for
>  restoring the context. But this looks correct to make
>  a actual shadow once and use it later all the time.

Yeah, I did have a go at using the io_pgtable_cfg, but given the
variable format of the registers involved it turned out simpler to
shadow the raw register state directly.

>  Will also try some testing today.
> 
>  Reviewed-by: sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org

Thanks!

Robin.

> [1] https://patchwork.kernel.org/patch/9389717/
> 
> Regards,
>  Sricharan
> 
> 
>>  struct arm_smmu_master_cfg {
>>  	struct arm_smmu_device		*smmu;
>>  	s16				smendx[];
>> @@ -380,6 +387,7 @@ struct arm_smmu_device {
>>  	u32				num_context_banks;
>>  	u32				num_s2_context_banks;
>>  	DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
>> +	struct arm_smmu_cb		*cbs;
>>  	atomic_t			irptndx;
>>  
>>  	u32				num_mapping_groups;
>> @@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>>  static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>>  				       struct io_pgtable_cfg *pgtbl_cfg)
>>  {
>> -	u32 reg, reg2;
>> -	u64 reg64;
>> -	bool stage1;
>>  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> -	struct arm_smmu_device *smmu = smmu_domain->smmu;
>> +	struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
>> +	bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> +
>> +	cb->cfg = cfg;
>> +
>> +	/* TTBCR */
>> +	if (stage1) {
>> +		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> +			cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
>> +		} else {
>> +			cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> +			cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> +			cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
>> +			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> +				cb->tcr[1] |= TTBCR2_AS;
>> +		}
>> +	} else {
>> +		cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> +	}
>> +
>> +	/* 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];
>> +		} else {
>> +			cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> +			cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> +		}
>> +	} else {
>> +		cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> +	}
>> +
>> +	/* MAIRs (stage-1 only) */
>> +	if (stage1) {
>> +		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> +			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];
>> +		}
>> +	}
>> +}
>> +
>> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>> +{
>> +	u32 reg;
>> +	bool stage1;
>> +	struct arm_smmu_cb *cb = &smmu->cbs[idx];
>> +	struct arm_smmu_cfg *cfg = cb->cfg;
>> +	struct arm_smmu_cfg default_cfg = {0};
>>  	void __iomem *cb_base, *gr1_base;
>>  
>> +	if (!cfg)
>> +		cfg = &default_cfg;
>> +
>>  	gr1_base = ARM_SMMU_GR1(smmu);
>>  	stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> +	cb_base = ARM_SMMU_CB(smmu, idx);
>>  
>> +	/* CBA2R */
>>  	if (smmu->version > ARM_SMMU_V1) {
>>  		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>>  			reg = CBA2R_RW64_64BIT;
>> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>>  		if (smmu->features & ARM_SMMU_FEAT_VMID16)
>>  			reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>>  
>> -		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
>> +		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>>  	}
>>  
>>  	/* CBAR */
>> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>>  		/* 8-bit VMIDs live in CBAR */
>>  		reg |= cfg->vmid << CBAR_VMID_SHIFT;
>>  	}
>> -	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
>> +	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>>  
>>  	/*
>>  	 * TTBCR
>>  	 * We must write this before the TTBRs, since it determines the
>>  	 * access behaviour of some fields (in particular, ASID[15:8]).
>>  	 */
>> -	if (stage1) {
>> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> -			reg = pgtbl_cfg->arm_v7s_cfg.tcr;
>> -			reg2 = 0;
>> -		} else {
>> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> -			reg2 |= TTBCR2_SEP_UPSTREAM;
>> -			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> -				reg2 |= TTBCR2_AS;
>> -		}
>> -		if (smmu->version > ARM_SMMU_V1)
>> -			writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
>> -	} else {
>> -		reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> -	}
>> -	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>> +	if (stage1 && smmu->version > ARM_SMMU_V1)
>> +		writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
>> +	writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>>  
>>  	/* TTBRs */
>> -	if (stage1) {
>> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
>> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
>> -			writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> -		} else {
>> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
>> -		}
>> +	if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> +		writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> +		writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> +		writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>>  	} else {
>> -		reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> -		writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> +		writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> +		writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>>  	}
>>  
>>  	/* MAIRs (stage-1 only) */
>>  	if (stage1) {
>> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> -			reg = pgtbl_cfg->arm_v7s_cfg.prrr;
>> -			reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> -		} else {
>> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> -		}
>> -		writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
>> -		writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
>> +		writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
>> +		writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>>  	}
>>  
>>  	/* SCTLR */
>> -	reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> -	if (stage1)
>> -		reg |= SCTLR_S1_ASIDPNE;
>> -#ifdef __BIG_ENDIAN
>> -	reg |= SCTLR_E;
>> -#endif
>> +	if (!cb->cfg) {
>> +		reg = 0;
>> +	} else {
>> +		reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> +		if (stage1)
>> +			reg |= SCTLR_S1_ASIDPNE;
>> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> +			reg |= SCTLR_E;
>> +	}
>>  	writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>>  }
>>  
>> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>  
>>  	/* Initialise the context bank with our page table cfg */
>>  	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>  
>>  	/*
>>  	 * Request context fault interrupt. Do this last to avoid the
>> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
>>  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> -	void __iomem *cb_base;
>>  	int irq;
>>  
>>  	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>  	 * Disable the context bank and free the page tables before freeing
>>  	 * it.
>>  	 */
>> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> -	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> +	smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
>> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>  
>>  	if (cfg->irptndx != INVALID_IRPTNDX) {
>>  		irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
>> @@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
>>  static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>>  {
>>  	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>> -	void __iomem *cb_base;
>>  	int i;
>>  	u32 reg, major;
>>  
>> @@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>>  
>>  	/* Make sure all context banks are disabled and clear CB_FSR  */
>>  	for (i = 0; i < smmu->num_context_banks; ++i) {
>> -		cb_base = ARM_SMMU_CB(smmu, i);
>> -		writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> +		void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
>> +
>> +		arm_smmu_write_context_bank(smmu, i);
>>  		writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
>>  		/*
>>  		 * Disable MMU-500's not-particularly-beneficial next-page
>> @@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>  		smmu->cavium_id_base -= smmu->num_context_banks;
>>  		dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
>>  	}
>> +	smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
>> +				 sizeof(*smmu->cbs), GFP_KERNEL);
>> +	if (!smmu->cbs)
>> +		return -ENOMEM;
>>  
>>  	/* ID2 */
>>  	id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
>>
> 

Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state

[Will Deacon]

Hi Robin,

I like the idea, but I think there are a few minor issues with the patch.
Comments below.

On Tue, Jul 18, 2017 at 01:44:41PM +0100, Robin Murphy wrote:
> Echoing what we do for Stream Map Entries, maintain a software shadow
> state for context bank configuration. With this in place, we are mere
> moments away from blissfully easy suspend/resume support.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> Since the runtime PM discussion has come back again, I figured it was
> probably about time to finish off my plan for system PM. Lightly tested
> on Juno (MMU-401) with hibernation.
> 
> Robin.
> 
>  drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>  1 file changed, 97 insertions(+), 62 deletions(-)

[...]

> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> +{
> +	u32 reg;
> +	bool stage1;
> +	struct arm_smmu_cb *cb = &smmu->cbs[idx];
> +	struct arm_smmu_cfg *cfg = cb->cfg;
> +	struct arm_smmu_cfg default_cfg = {0};
>  	void __iomem *cb_base, *gr1_base;
>  
> +	if (!cfg)
> +		cfg = &default_cfg;
> +
>  	gr1_base = ARM_SMMU_GR1(smmu);
>  	stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> +	cb_base = ARM_SMMU_CB(smmu, idx);
>  
> +	/* CBA2R */
>  	if (smmu->version > ARM_SMMU_V1) {
>  		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>  			reg = CBA2R_RW64_64BIT;
> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>  		if (smmu->features & ARM_SMMU_FEAT_VMID16)
>  			reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>  
> -		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
> +		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>  	}
>  
>  	/* CBAR */
> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>  		/* 8-bit VMIDs live in CBAR */
>  		reg |= cfg->vmid << CBAR_VMID_SHIFT;
>  	}
> -	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
> +	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>  
>  	/*
>  	 * TTBCR
>  	 * We must write this before the TTBRs, since it determines the
>  	 * access behaviour of some fields (in particular, ASID[15:8]).
>  	 */
> -	if (stage1) {
> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> -			reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> -			reg2 = 0;
> -		} else {
> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> -			reg2 |= TTBCR2_SEP_UPSTREAM;
> -			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> -				reg2 |= TTBCR2_AS;
> -		}
> -		if (smmu->version > ARM_SMMU_V1)
> -			writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> -	} else {
> -		reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> -	}
> -	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> +	if (stage1 && smmu->version > ARM_SMMU_V1)
> +		writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
> +	writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>  
>  	/* TTBRs */
> -	if (stage1) {
> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
> -			writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> -		} else {
> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
> -		}
> +	if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> +		writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> +		writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> +		writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>  	} else {
> -		reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> -		writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> +		writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> +		writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);

This doesn't look right to me. For the LPAE S1 case, we don't set the ASID
afaict, and for the S2 case we write to a RESERVED register (since we only
have one TTBR).
>  	}
>  
>  	/* MAIRs (stage-1 only) */
>  	if (stage1) {
> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> -			reg = pgtbl_cfg->arm_v7s_cfg.prrr;
> -			reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
> -		} else {
> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> -		}
> -		writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
> -		writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
> +		writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
> +		writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>  	}
>  
>  	/* SCTLR */
> -	reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> -	if (stage1)
> -		reg |= SCTLR_S1_ASIDPNE;
> -#ifdef __BIG_ENDIAN
> -	reg |= SCTLR_E;
> -#endif
> +	if (!cb->cfg) {
> +		reg = 0;

I think this might be too late. See below.

> +	} else {
> +		reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> +		if (stage1)
> +			reg |= SCTLR_S1_ASIDPNE;
> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +			reg |= SCTLR_E;
> +	}
>  	writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>  }
>  
> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  
>  	/* Initialise the context bank with our page table cfg */
>  	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);
>  
>  	/*
>  	 * Request context fault interrupt. Do this last to avoid the
> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
>  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> -	void __iomem *cb_base;
>  	int irq;
>  
>  	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	 * Disable the context bank and free the page tables before freeing
>  	 * it.
>  	 */
> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> -	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> +	smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);

So here, we end up passing a zeroed structure to
arm_smmu_write_context_bank, but afaict that will write to the TTBR and TCR
before SCTLR, which worries me. Couldn't we get whacky speculative table
walks through physical address 0 with ASID 0 before we kill the SCTLR?

If the only time we pass something with a NULL cfg is on the destroy path,
perhaps just do:

  if (!cfg) {
	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
	return;
  }

instead?

Will

Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state

[Robin Murphy]

On 08/08/17 12:11, Will Deacon wrote:
> Hi Robin,
> 
> I like the idea, but I think there are a few minor issues with the patch.
> Comments below.
> 
> On Tue, Jul 18, 2017 at 01:44:41PM +0100, Robin Murphy wrote:
>> Echoing what we do for Stream Map Entries, maintain a software shadow
>> state for context bank configuration. With this in place, we are mere
>> moments away from blissfully easy suspend/resume support.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> Since the runtime PM discussion has come back again, I figured it was
>> probably about time to finish off my plan for system PM. Lightly tested
>> on Juno (MMU-401) with hibernation.
>>
>> Robin.
>>
>>  drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 97 insertions(+), 62 deletions(-)
> 
> [...]
> 
>> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>> +{
>> +	u32 reg;
>> +	bool stage1;
>> +	struct arm_smmu_cb *cb = &smmu->cbs[idx];
>> +	struct arm_smmu_cfg *cfg = cb->cfg;
>> +	struct arm_smmu_cfg default_cfg = {0};
>>  	void __iomem *cb_base, *gr1_base;
>>  
>> +	if (!cfg)
>> +		cfg = &default_cfg;
>> +
>>  	gr1_base = ARM_SMMU_GR1(smmu);
>>  	stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> +	cb_base = ARM_SMMU_CB(smmu, idx);
>>  
>> +	/* CBA2R */
>>  	if (smmu->version > ARM_SMMU_V1) {
>>  		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>>  			reg = CBA2R_RW64_64BIT;
>> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>>  		if (smmu->features & ARM_SMMU_FEAT_VMID16)
>>  			reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>>  
>> -		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
>> +		writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>>  	}
>>  
>>  	/* CBAR */
>> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>>  		/* 8-bit VMIDs live in CBAR */
>>  		reg |= cfg->vmid << CBAR_VMID_SHIFT;
>>  	}
>> -	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
>> +	writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>>  
>>  	/*
>>  	 * TTBCR
>>  	 * We must write this before the TTBRs, since it determines the
>>  	 * access behaviour of some fields (in particular, ASID[15:8]).
>>  	 */
>> -	if (stage1) {
>> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> -			reg = pgtbl_cfg->arm_v7s_cfg.tcr;
>> -			reg2 = 0;
>> -		} else {
>> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> -			reg2 |= TTBCR2_SEP_UPSTREAM;
>> -			if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> -				reg2 |= TTBCR2_AS;
>> -		}
>> -		if (smmu->version > ARM_SMMU_V1)
>> -			writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
>> -	} else {
>> -		reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> -	}
>> -	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>> +	if (stage1 && smmu->version > ARM_SMMU_V1)
>> +		writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
>> +	writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>>  
>>  	/* TTBRs */
>> -	if (stage1) {
>> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
>> -			reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> -			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
>> -			writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> -		} else {
>> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> -			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> -			reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> -			writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
>> -		}
>> +	if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> +		writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> +		writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> +		writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>>  	} else {
>> -		reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> -		writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> +		writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> +		writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
> 
> This doesn't look right to me. For the LPAE S1 case, we don't set the ASID
> afaict, and for the S2 case we write to a RESERVED register (since we only
> have one TTBR).

Oops, yes, arm_smmu_init_context_bank() has indeed forgotten to munge
the ASID into cb->ttbr[0] for that case.

TTBR1, though, is not an oversight - architecturally it is UNK/SBZP in
stage 2 contexts, so since we never read it and cb->ttbr[1] will always
be zero for stage 2, this is a valid thing to do and helps keep the code
simple.

>>  	}
>>  
>>  	/* MAIRs (stage-1 only) */
>>  	if (stage1) {
>> -		if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> -			reg = pgtbl_cfg->arm_v7s_cfg.prrr;
>> -			reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> -		} else {
>> -			reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> -			reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> -		}
>> -		writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
>> -		writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
>> +		writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
>> +		writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>>  	}
>>  
>>  	/* SCTLR */
>> -	reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> -	if (stage1)
>> -		reg |= SCTLR_S1_ASIDPNE;
>> -#ifdef __BIG_ENDIAN
>> -	reg |= SCTLR_E;
>> -#endif
>> +	if (!cb->cfg) {
>> +		reg = 0;
> 
> I think this might be too late. See below.

Ha, this bit did feel slightly awkward, and you are indeed quite right...

>> +	} else {
>> +		reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> +		if (stage1)
>> +			reg |= SCTLR_S1_ASIDPNE;
>> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> +			reg |= SCTLR_E;
>> +	}
>>  	writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>>  }
>>  
>> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>  
>>  	/* Initialise the context bank with our page table cfg */
>>  	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>  
>>  	/*
>>  	 * Request context fault interrupt. Do this last to avoid the
>> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
>>  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> -	void __iomem *cb_base;
>>  	int irq;
>>  
>>  	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>  	 * Disable the context bank and free the page tables before freeing
>>  	 * it.
>>  	 */
>> -	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> -	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> +	smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
>> +	arm_smmu_write_context_bank(smmu, cfg->cbndx);
> 
> So here, we end up passing a zeroed structure to
> arm_smmu_write_context_bank, but afaict that will write to the TTBR and TCR
> before SCTLR, which worries me. Couldn't we get whacky speculative table
> walks through physical address 0 with ASID 0 before we kill the SCTLR?
> 
> If the only time we pass something with a NULL cfg is on the destroy path,
> perhaps just do:
> 
>   if (!cfg) {
> 	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> 	return;
>   }
> 
> instead?

We also hit all banks with a NULL cfg in the initial reset from
arm_smmu_device_probe(), but that still has the same semantics as
teardown - if the context isn't assigned to a domain, there's no point
even touching any hardware registers other than clearing SCTLR, so
there's no need for the whole default_cfg silliness at all. Thanks for
helping me see the obvious!

Robin.