diff mbox series

[v2,07/10] iommu/io-pgtable-arm: Rationalise MAIR handling

Message ID c6bee9e6de5e7f4aa2293ee5385ffa2dd95600d3.1572024120.git.robin.murphy@arm.com (mailing list archive)
State New, archived
Headers show
Series iommu/io-pgtable: Cleanup and prep for split tables | expand

Commit Message

Robin Murphy Oct. 25, 2019, 6:08 p.m. UTC 
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 | 3 +--
 drivers/iommu/ipmmu-vmsa.c     | 2 +-
 drivers/iommu/qcom_iommu.c     | 4 ++--
 include/linux/io-pgtable.h     | 2 +-
 6 files changed, 8 insertions(+), 9 deletions(-)

Comments

Will Deacon Nov. 4, 2019, 6:20 p.m. UTC | #1 
On Fri, Oct 25, 2019 at 07:08:36PM +0100, Robin Murphy wrote:
> 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 | 3 +--
>  drivers/iommu/ipmmu-vmsa.c     | 2 +-
>  drivers/iommu/qcom_iommu.c     | 4 ++--
>  include/linux/io-pgtable.h     | 2 +-
>  6 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 8da93e730d6f..3f20e548f1ec 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2172,7 +2172,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 080af0326816..2bc3e93b11e6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -552,8 +552,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;

Does this work correctly for big-endian?

Will
Robin Murphy Nov. 4, 2019, 6:43 p.m. UTC | #2 
On 04/11/2019 18:20, Will Deacon wrote:
> On Fri, Oct 25, 2019 at 07:08:36PM +0100, Robin Murphy wrote:
>> 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 | 3 +--
>>   drivers/iommu/ipmmu-vmsa.c     | 2 +-
>>   drivers/iommu/qcom_iommu.c     | 4 ++--
>>   include/linux/io-pgtable.h     | 2 +-
>>   6 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 8da93e730d6f..3f20e548f1ec 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2172,7 +2172,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 080af0326816..2bc3e93b11e6 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -552,8 +552,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;
> 
> Does this work correctly for big-endian?

I don't see why it wouldn't - cfg.mair is read and written as a u64, so 
this should always return its most significant word regardless of the 
storage format. We're not doing anything dodgy like trying to type-pun 
the u64 directly into the u32[2].

Robin.
Will Deacon Nov. 4, 2019, 7:20 p.m. UTC | #3 
On Mon, Nov 04, 2019 at 06:43:06PM +0000, Robin Murphy wrote:
> On 04/11/2019 18:20, Will Deacon wrote:
> > On Fri, Oct 25, 2019 at 07:08:36PM +0100, Robin Murphy wrote:
> > > 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 | 3 +--
> > >   drivers/iommu/ipmmu-vmsa.c     | 2 +-
> > >   drivers/iommu/qcom_iommu.c     | 4 ++--
> > >   include/linux/io-pgtable.h     | 2 +-
> > >   6 files changed, 8 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > index 8da93e730d6f..3f20e548f1ec 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -2172,7 +2172,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 080af0326816..2bc3e93b11e6 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -552,8 +552,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;
> > 
> > Does this work correctly for big-endian?
> 
> I don't see why it wouldn't - cfg.mair is read and written as a u64, so this
> should always return its most significant word regardless of the storage
> format. We're not doing anything dodgy like trying to type-pun the u64
> directly into the u32[2].

Urgh, I need to convince myself about this then. Off to draw those silly
ABCD DCBA diagrams on some paper.

Will
Will Deacon Nov. 4, 2019, 7:57 p.m. UTC | #4 
On Mon, Nov 04, 2019 at 07:20:58PM +0000, Will Deacon wrote:
> On Mon, Nov 04, 2019 at 06:43:06PM +0000, Robin Murphy wrote:
> > On 04/11/2019 18:20, Will Deacon wrote:
> > > On Fri, Oct 25, 2019 at 07:08:36PM +0100, Robin Murphy wrote:
> > > > 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 | 3 +--
> > > >   drivers/iommu/ipmmu-vmsa.c     | 2 +-
> > > >   drivers/iommu/qcom_iommu.c     | 4 ++--
> > > >   include/linux/io-pgtable.h     | 2 +-
> > > >   6 files changed, 8 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > > index 8da93e730d6f..3f20e548f1ec 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -2172,7 +2172,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 080af0326816..2bc3e93b11e6 100644
> > > > --- a/drivers/iommu/arm-smmu.c
> > > > +++ b/drivers/iommu/arm-smmu.c
> > > > @@ -552,8 +552,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;
> > > 
> > > Does this work correctly for big-endian?
> > 
> > I don't see why it wouldn't - cfg.mair is read and written as a u64, so this
> > should always return its most significant word regardless of the storage
> > format. We're not doing anything dodgy like trying to type-pun the u64
> > directly into the u32[2].
> 
> Urgh, I need to convince myself about this then. Off to draw those silly
> ABCD DCBA diagrams on some paper.

Yes, you're right, it's fine. I was worried about explicitly writing
2x32-bit MAIRs and then loading them as one, but that's not what is going on
here.

Will
diff mbox series

Patch

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 8da93e730d6f..3f20e548f1ec 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2172,7 +2172,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 080af0326816..2bc3e93b11e6 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -552,8 +552,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 15b4927ce36b..1795df8f7a51 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -861,8 +861,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(ARM_LPAE_PGD_SIZE(data),
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9da8309f7170..e4da6efbda49 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -438,7 +438,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 c31e7bc4ccbe..66e9b40e9275 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -284,9 +284,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 ec7a13405f10..ee21eedafe98 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -102,7 +102,7 @@  struct io_pgtable_cfg {
 		struct {
 			u64	ttbr[2];
 			u64	tcr;
-			u64	mair[2];
+			u64	mair;
 		} arm_lpae_s1_cfg;
 
 		struct {