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

[Will Deacon <will@kernel.org>]

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