On Tue, Jan 28, 2020 at 03:00:16PM -0700, Jordan Crouse wrote: > Add support to enable TTBR1 if the domain requests it via the > DOMAIN_ATTR_SPLIT_TABLES attribute. If enabled by the hardware > and pagetable configuration the driver will configure the TTBR1 region > and program the domain pagetable on TTBR1. TTBR0 will be disabled. > > After attaching the device the value of he domain attribute can > be queried to see if the split pagetables were successfully programmed. > The domain geometry will be updated as well so that the caller can > determine the active region for the pagetable that was programmed. > > Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx> > --- > > drivers/iommu/arm-smmu.c | 48 +++++++++++++++++++++++++++++++++++++++++------- > drivers/iommu/arm-smmu.h | 22 ++++++++++++++++------ > 2 files changed, 57 insertions(+), 13 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 16c4b87..23b22fa 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -557,11 +557,17 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, > cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr; > cb->ttbr[1] = 0; > } else { > - cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr; > - cb->ttbr[0] |= FIELD_PREP(ARM_SMMU_TTBRn_ASID, > - cfg->asid); > - cb->ttbr[1] = FIELD_PREP(ARM_SMMU_TTBRn_ASID, > - cfg->asid); I think it would be clearer if you set the ASID in TTBR0 unconditionally here... > + if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) { > + cb->ttbr[0] = FIELD_PREP(ARM_SMMU_TTBRn_ASID, > + cfg->asid); > + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr; > + } else { > + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr; > + cb->ttbr[0] |= FIELD_PREP(ARM_SMMU_TTBRn_ASID, > + cfg->asid); ... and then OR'd in the TTBR base here. > + cb->ttbr[1] = FIELD_PREP(ARM_SMMU_TTBRn_ASID, > + cfg->asid); > + } > } > } else { > cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr; > @@ -675,6 +681,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, > enum io_pgtable_fmt fmt; > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_cfg *cfg = &smmu_domain->cfg; > + unsigned long quirks = 0; > > mutex_lock(&smmu_domain->init_mutex); > if (smmu_domain->smmu) > @@ -743,6 +750,14 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, > oas = smmu->ipa_size; > if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) { > fmt = ARM_64_LPAE_S1; > + > + /* > + * We are assuming that split pagetables will always use > + * SEP_UPSTREAM so we don't need to reduce the size of > + * the ias to account for the sign extension bit > + */ > + if (smmu_domain->split_pagetables) > + quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1; > } else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) { > fmt = ARM_32_LPAE_S1; > ias = min(ias, 32UL); > @@ -812,6 +827,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, > .coherent_walk = smmu->features & ARM_SMMU_FEAT_COHERENT_WALK, > .tlb = smmu_domain->flush_ops, > .iommu_dev = smmu->dev, > + .quirks = quirks, > }; > > if (smmu_domain->non_strict) > @@ -825,8 +841,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, > > /* Update the domain's page sizes to reflect the page table format */ > domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap; > - domain->geometry.aperture_end = (1UL << ias) - 1; > - domain->geometry.force_aperture = true; > + > + if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) { > + domain->geometry.aperture_start = ~0UL << ias; > + domain->geometry.aperture_end = ~0UL; > + } else { > + domain->geometry.aperture_end = (1UL << ias) - 1; > + domain->geometry.force_aperture = true; > + smmu_domain->split_pagetables = false; > + } Why do you only force the aperture for TTBR0? > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h > index 8d1cd54..53053fd 100644 > --- a/drivers/iommu/arm-smmu.h > +++ b/drivers/iommu/arm-smmu.h > @@ -172,6 +172,7 @@ enum arm_smmu_cbar_type { > #define ARM_SMMU_TCR_SH0 GENMASK(13, 12) > #define ARM_SMMU_TCR_ORGN0 GENMASK(11, 10) > #define ARM_SMMU_TCR_IRGN0 GENMASK(9, 8) > +#define ARM_SMMU_TCR_EPD0 BIT(7) > #define ARM_SMMU_TCR_T0SZ GENMASK(5, 0) > > #define ARM_SMMU_VTCR_RES1 BIT(31) > @@ -343,16 +344,25 @@ struct arm_smmu_domain { > struct mutex init_mutex; /* Protects smmu pointer */ > spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */ > struct iommu_domain domain; > + bool split_pagetables; > }; > > static inline u32 arm_smmu_lpae_tcr(struct io_pgtable_cfg *cfg) > { > - return ARM_SMMU_TCR_EPD1 | > - FIELD_PREP(ARM_SMMU_TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg) | > - FIELD_PREP(ARM_SMMU_TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) | > - FIELD_PREP(ARM_SMMU_TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) | > - FIELD_PREP(ARM_SMMU_TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) | > - FIELD_PREP(ARM_SMMU_TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz); > + u32 tcr = FIELD_PREP(ARM_SMMU_TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg) | > + FIELD_PREP(ARM_SMMU_TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) | > + FIELD_PREP(ARM_SMMU_TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) | > + FIELD_PREP(ARM_SMMU_TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) | > + FIELD_PREP(ARM_SMMU_TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz); > + > + /* > + * When TTBR1 is selected shift the TCR fields by 16 bits and disable > + * translation in TTBR0 > + */ > + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) > + return (tcr << 16) | ARM_SMMU_TCR_EPD0; > + > + return tcr | ARM_SMMU_TCR_EPD1; > } Please give this a single return statement, i.e. if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) tcr = (tcr << 16) | ARM_SMMU_TCR_EPD0; else tcr |= ARM_SMMU_TCR_EPD1; return tcr; Will