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 |
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
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.
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
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 --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 {
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(-)