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

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

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel