| Message ID | 173006777218859d1671ae517c70592c6c02f630.1460391217.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Robin, On Wed, Apr 13, 2016 at 06:13:02PM +0100, Robin Murphy wrote: > The way the driver currently forces an AArch32 or AArch64 context format > based on the kernel config and SMMU architecture version is suboptimal, > in that it makes it very hard to support oddball mix-and-match cases > like the SMMUv1 64KB supplement, or situations where the reduced table > depth of an AArch32 short descriptor context may be desirable under an > AArch64 kernel. It also only happens to work on current implementations > which do support all the relevant formats. > > Introduce an explicit notion of context format, so we can manage that > independently and get rid of the inflexible #ifdeffery. Thanks for doing all of this! One comment on the page size stuff: > + if (IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) { > + switch (PAGE_SIZE) { > + case SZ_64K: > + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_64K) { > + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; > + break; > + } /* else fall through */ > + case SZ_16K: > + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_16K) { > + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; > + break; > + } /* else fall through */ > + case SZ_4K: > + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_4K) > + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; > + } > + } The io-pgtable code (arm_lpae_restrict_pgsizes) already does something *very* similar to this, using the pgsize_bitmap as input. Can you not just choose the ARM_SMMU_CTX_FMT_AARCH64 here and set the pgsize_bitmap to represent all page sizes supported by the hardware? That way, you should end up with the best option coming back from the pgtable code (I do this in the v3 driver, fwiw). Will
On 21/04/16 17:30, Will Deacon wrote: > Hi Robin, > > On Wed, Apr 13, 2016 at 06:13:02PM +0100, Robin Murphy wrote: >> The way the driver currently forces an AArch32 or AArch64 context format >> based on the kernel config and SMMU architecture version is suboptimal, >> in that it makes it very hard to support oddball mix-and-match cases >> like the SMMUv1 64KB supplement, or situations where the reduced table >> depth of an AArch32 short descriptor context may be desirable under an >> AArch64 kernel. It also only happens to work on current implementations >> which do support all the relevant formats. >> >> Introduce an explicit notion of context format, so we can manage that >> independently and get rid of the inflexible #ifdeffery. > > Thanks for doing all of this! One comment on the page size stuff: > >> + if (IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) { >> + switch (PAGE_SIZE) { >> + case SZ_64K: >> + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_64K) { >> + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; >> + break; >> + } /* else fall through */ >> + case SZ_16K: >> + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_16K) { >> + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; >> + break; >> + } /* else fall through */ >> + case SZ_4K: >> + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_4K) >> + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; >> + } >> + } > > The io-pgtable code (arm_lpae_restrict_pgsizes) already does something > *very* similar to this, using the pgsize_bitmap as input. Can you not > just choose the ARM_SMMU_CTX_FMT_AARCH64 here and set the pgsize_bitmap > to represent all page sizes supported by the hardware? That way, you should > end up with the best option coming back from the pgtable code (I do this > in the v3 driver, fwiw). It took a while to come back to me, but the problem is that we can't call alloc_io_pgtable_ops(fmt...) before choosing either ARM_64_LPAE_Sx or ARM_32_LPAE_Sx for fmt, but if we commit to 64-bit we'll get stuck later without the possibility of falling back to AArch32 if it turns out we don't have a viable AArch64 granule. Plus we'd have to remove the LPAE page sizes from the bitmap beforehand in the case we don't support AARCH64_4K lest we end up with a phantom format the hardware doesn't actually do, and it all starts getting rather horrible... Robin. > Will >
On Fri, Apr 22, 2016 at 06:38:04PM +0100, Robin Murphy wrote: > On 21/04/16 17:30, Will Deacon wrote: > >Hi Robin, > > > >On Wed, Apr 13, 2016 at 06:13:02PM +0100, Robin Murphy wrote: > >>The way the driver currently forces an AArch32 or AArch64 context format > >>based on the kernel config and SMMU architecture version is suboptimal, > >>in that it makes it very hard to support oddball mix-and-match cases > >>like the SMMUv1 64KB supplement, or situations where the reduced table > >>depth of an AArch32 short descriptor context may be desirable under an > >>AArch64 kernel. It also only happens to work on current implementations > >>which do support all the relevant formats. > >> > >>Introduce an explicit notion of context format, so we can manage that > >>independently and get rid of the inflexible #ifdeffery. > > > >Thanks for doing all of this! One comment on the page size stuff: > > > >>+ if (IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) { > >>+ switch (PAGE_SIZE) { > >>+ case SZ_64K: > >>+ if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_64K) { > >>+ cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; > >>+ break; > >>+ } /* else fall through */ > >>+ case SZ_16K: > >>+ if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_16K) { > >>+ cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; > >>+ break; > >>+ } /* else fall through */ > >>+ case SZ_4K: > >>+ if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_4K) > >>+ cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; > >>+ } > >>+ } > > > >The io-pgtable code (arm_lpae_restrict_pgsizes) already does something > >*very* similar to this, using the pgsize_bitmap as input. Can you not > >just choose the ARM_SMMU_CTX_FMT_AARCH64 here and set the pgsize_bitmap > >to represent all page sizes supported by the hardware? That way, you should > >end up with the best option coming back from the pgtable code (I do this > >in the v3 driver, fwiw). > > It took a while to come back to me, but the problem is that we can't call > alloc_io_pgtable_ops(fmt...) before choosing either ARM_64_LPAE_Sx or > ARM_32_LPAE_Sx for fmt, but if we commit to 64-bit we'll get stuck later > without the possibility of falling back to AArch32 if it turns out we don't > have a viable AArch64 granule. In what case would you not have a viable AArch64 granule, but the option of falling back to AArch32 makes things work? > Plus we'd have to remove the LPAE page sizes > from the bitmap beforehand in the case we don't support AARCH64_4K lest we > end up with a phantom format the hardware doesn't actually do, and it all > starts getting rather horrible... I'm not following. The io-pgtable code shouldn't give you back a phanton format. Will
On 25/04/16 12:02, Will Deacon wrote: > On Fri, Apr 22, 2016 at 06:38:04PM +0100, Robin Murphy wrote: >> On 21/04/16 17:30, Will Deacon wrote: >>> Hi Robin, >>> >>> On Wed, Apr 13, 2016 at 06:13:02PM +0100, Robin Murphy wrote: >>>> The way the driver currently forces an AArch32 or AArch64 context format >>>> based on the kernel config and SMMU architecture version is suboptimal, >>>> in that it makes it very hard to support oddball mix-and-match cases >>>> like the SMMUv1 64KB supplement, or situations where the reduced table >>>> depth of an AArch32 short descriptor context may be desirable under an >>>> AArch64 kernel. It also only happens to work on current implementations >>>> which do support all the relevant formats. >>>> >>>> Introduce an explicit notion of context format, so we can manage that >>>> independently and get rid of the inflexible #ifdeffery. >>> >>> Thanks for doing all of this! One comment on the page size stuff: >>> >>>> + if (IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) { >>>> + switch (PAGE_SIZE) { >>>> + case SZ_64K: >>>> + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_64K) { >>>> + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; >>>> + break; >>>> + } /* else fall through */ >>>> + case SZ_16K: >>>> + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_16K) { >>>> + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; >>>> + break; >>>> + } /* else fall through */ >>>> + case SZ_4K: >>>> + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_4K) >>>> + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; >>>> + } >>>> + } >>> >>> The io-pgtable code (arm_lpae_restrict_pgsizes) already does something >>> *very* similar to this, using the pgsize_bitmap as input. Can you not >>> just choose the ARM_SMMU_CTX_FMT_AARCH64 here and set the pgsize_bitmap >>> to represent all page sizes supported by the hardware? That way, you should >>> end up with the best option coming back from the pgtable code (I do this >>> in the v3 driver, fwiw). >> >> It took a while to come back to me, but the problem is that we can't call >> alloc_io_pgtable_ops(fmt...) before choosing either ARM_64_LPAE_Sx or >> ARM_32_LPAE_Sx for fmt, but if we commit to 64-bit we'll get stuck later >> without the possibility of falling back to AArch32 if it turns out we don't >> have a viable AArch64 granule. > > In what case would you not have a viable AArch64 granule, but the option > of falling back to AArch32 makes things work? A 4KB page kernel with MMU-401, whose only supported AArch64 granule is supposedly 64KB. >> Plus we'd have to remove the LPAE page sizes >> from the bitmap beforehand in the case we don't support AARCH64_4K lest we >> end up with a phantom format the hardware doesn't actually do, and it all >> starts getting rather horrible... > > I'm not following. The io-pgtable code shouldn't give you back a phanton > format. If you call alloc_io_pgtable_ops() with ARM_64_LPAE_S2 and an unmolested MMU-401 pgsize_bitmap, you get back a 4K granule v8 pagetable, which per a strict reading of the TRM isn't supported (although does appear to work in practice...) Consider also a stage 1 SMMU supporting all formats but only with 4K granules: with a 64K page arm64 kernel, the presence of AARCH64_4K support would lead us into arm_64_lpae_alloc_pgtable_s1(), which would be tricked into giving us back a 64K granule v8 format by the short descriptor large page size matching PAGE_SIZE, and things would definitely go downhill from there. Given that, I think the only truly safe thing to do is to pass an explicit granule in the io_pgtable_cfg and just get rid of the bitmap-based guessing in arm_lpae_restrict_pgsizes() - otherwise, we'd still have to pre-restrict the bitmap, making it pretty much redundant anyway. I'll have a hack at that - in the meantime please feel free to queue patches 1-3 if you're happy with them, as that part is still feature-complete without all this context stuff. Robin. > > Will >
On Mon, Apr 25, 2016 at 02:14:57PM +0100, Robin Murphy wrote: > On 25/04/16 12:02, Will Deacon wrote: > >In what case would you not have a viable AArch64 granule, but the option > >of falling back to AArch32 makes things work? > > A 4KB page kernel with MMU-401, whose only supported AArch64 granule is > supposedly 64KB. That doesn't sound right at all! > >>Plus we'd have to remove the LPAE page sizes > >>from the bitmap beforehand in the case we don't support AARCH64_4K lest we > >>end up with a phantom format the hardware doesn't actually do, and it all > >>starts getting rather horrible... > > > >I'm not following. The io-pgtable code shouldn't give you back a phanton > >format. > > If you call alloc_io_pgtable_ops() with ARM_64_LPAE_S2 and an unmolested > MMU-401 pgsize_bitmap, you get back a 4K granule v8 pagetable, which per a > strict reading of the TRM isn't supported (although does appear to work in > practice...) I suspect that's a badly worded/incorrect TRM. In reality, I'd be happy to assume that a given granule is supported across all formats (in as much as it can be) if it's supported by one of them, but acknowledge that's not guaranteed by the architecture. > Consider also a stage 1 SMMU supporting all formats but only with 4K > granules: with a 64K page arm64 kernel, the presence of AARCH64_4K support > would lead us into arm_64_lpae_alloc_pgtable_s1(), which would be tricked > into giving us back a 64K granule v8 format by the short descriptor large > page size matching PAGE_SIZE, and things would definitely go downhill from > there. We could improve this by having a separate pgsize_bitmap per format, and passing the relevant one to the relevant page table constructor. > Given that, I think the only truly safe thing to do is to pass an explicit > granule in the io_pgtable_cfg and just get rid of the bitmap-based guessing > in arm_lpae_restrict_pgsizes() - otherwise, we'd still have to pre-restrict > the bitmap, making it pretty much redundant anyway. I'll have a hack at that Actually, I think I prefer to go the other way. Let's try moving more of this common logic into the io-pgtable code. For example, an IOMMU driver could say "I support these formats, and these options (page sizes, quirks, etc) for each format" and the io-pgtable code can choose the most appropriate thing. > - in the meantime please feel free to queue patches 1-3 if you're happy with > them, as that part is still feature-complete without all this context stuff. Queued those, thanks! Will
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index acff332..1d4285f 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -117,6 +117,8 @@ #define ID0_NTS (1 << 28) #define ID0_SMS (1 << 27) #define ID0_ATOSNS (1 << 26) +#define ID0_PTFS_NO_AARCH32 (1 << 25) +#define ID0_PTFS_NO_AARCH32S (1 << 24) #define ID0_CTTW (1 << 14) #define ID0_NUMIRPT_SHIFT 16 #define ID0_NUMIRPT_MASK 0xff @@ -317,6 +319,11 @@ struct arm_smmu_device { #define ARM_SMMU_FEAT_TRANS_NESTED (1 << 4) #define ARM_SMMU_FEAT_TRANS_OPS (1 << 5) #define ARM_SMMU_FEAT_VMID16 (1 << 6) +#define ARM_SMMU_FEAT_FMT_AARCH64_4K (1 << 7) +#define ARM_SMMU_FEAT_FMT_AARCH64_16K (1 << 8) +#define ARM_SMMU_FEAT_FMT_AARCH64_64K (1 << 9) +#define ARM_SMMU_FEAT_FMT_AARCH32_L (1 << 10) +#define ARM_SMMU_FEAT_FMT_AARCH32_S (1 << 11) u32 features; #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0) @@ -346,10 +353,18 @@ struct arm_smmu_device { u32 cavium_id_base; /* Specific to Cavium */ }; +enum arm_smmu_context_fmt { + ARM_SMMU_CTX_FMT_NONE, + ARM_SMMU_CTX_FMT_AARCH64, + ARM_SMMU_CTX_FMT_AARCH32_L, + ARM_SMMU_CTX_FMT_AARCH32_S, +}; + struct arm_smmu_cfg { u8 cbndx; u8 irptndx; u32 cbar; + enum arm_smmu_context_fmt fmt; }; #define INVALID_IRPTNDX 0xff @@ -619,14 +634,13 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size, reg = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx); reg += leaf ? ARM_SMMU_CB_S1_TLBIVAL : ARM_SMMU_CB_S1_TLBIVA; - if (!IS_ENABLED(CONFIG_64BIT) || smmu->version == ARM_SMMU_V1) { + if (cfg->fmt != ARM_SMMU_CTX_FMT_AARCH64) { iova &= ~12UL; iova |= ARM_SMMU_CB_ASID(smmu, cfg); do { writel_relaxed(iova, reg); iova += granule; } while (size -= granule); -#ifdef CONFIG_64BIT } else { iova >>= 12; iova |= (u64)ARM_SMMU_CB_ASID(smmu, cfg) << 48; @@ -634,9 +648,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size, writeq_relaxed(iova, reg); iova += granule >> 12; } while (size -= granule); -#endif } -#ifdef CONFIG_64BIT } else if (smmu->version == ARM_SMMU_V2) { reg = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx); reg += leaf ? ARM_SMMU_CB_S2_TLBIIPAS2L : @@ -646,7 +658,6 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size, smmu_write_atomic_lq(iova, reg); iova += granule >> 12; } while (size -= granule); -#endif } else { reg = ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_TLBIVMID; writel_relaxed(ARM_SMMU_CB_VMID(smmu, cfg), reg); @@ -745,11 +756,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx); if (smmu->version > ARM_SMMU_V1) { -#ifdef CONFIG_64BIT - reg = CBA2R_RW64_64BIT; -#else - reg = CBA2R_RW64_32BIT; -#endif + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) + reg = CBA2R_RW64_64BIT; + else + reg = CBA2R_RW64_32BIT; /* 16-bit VMIDs live in CBA2R */ if (smmu->features & ARM_SMMU_FEAT_VMID16) reg |= ARM_SMMU_CB_VMID(smmu, cfg) << CBA2R_VMID_SHIFT; @@ -860,16 +870,48 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) smmu_domain->stage = ARM_SMMU_DOMAIN_S1; + /* + * Choosing a suitable context format is even more fiddly. Until we + * grow some way for the caller to express a preference, just aim for + * the closest match to the CPU format out of what we might have. + */ + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_L) + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH32_L; + if (IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) { + switch (PAGE_SIZE) { + case SZ_64K: + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_64K) { + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; + break; + } /* else fall through */ + case SZ_16K: + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_16K) { + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; + break; + } /* else fall through */ + case SZ_4K: + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_4K) + cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64; + } + } + if (cfg->fmt == ARM_SMMU_CTX_FMT_NONE) { + ret = -EINVAL; + goto out_unlock; + } + switch (smmu_domain->stage) { case ARM_SMMU_DOMAIN_S1: cfg->cbar = CBAR_TYPE_S1_TRANS_S2_BYPASS; start = smmu->num_s2_context_banks; ias = smmu->va_size; oas = smmu->ipa_size; - if (IS_ENABLED(CONFIG_64BIT)) + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) { fmt = ARM_64_LPAE_S1; - else + } else { fmt = ARM_32_LPAE_S1; + ias = min(ias, 32UL); + oas = min(oas, 40UL); + } break; case ARM_SMMU_DOMAIN_NESTED: /* @@ -881,10 +923,13 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, start = 0; ias = smmu->ipa_size; oas = smmu->pa_size; - if (IS_ENABLED(CONFIG_64BIT)) + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) { fmt = ARM_64_LPAE_S2; - else + } else { fmt = ARM_32_LPAE_S2; + ias = min(ias, 40UL); + oas = min(oas, 40UL); + } break; default: ret = -EINVAL; @@ -1670,6 +1715,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) ID0_NUMSIDB_MASK; } + if (smmu->version < ARM_SMMU_V2 || !(id & ID0_PTFS_NO_AARCH32)) { + smmu->features |= ARM_SMMU_FEAT_FMT_AARCH32_L; + if (!(id & ID0_PTFS_NO_AARCH32S)) + smmu->features |= ARM_SMMU_FEAT_FMT_AARCH32_S; + } + /* ID1 */ id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID1); smmu->pgshift = (id & ID1_PAGESIZE) ? 16 : 12; @@ -1725,7 +1776,6 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) if (smmu->version == ARM_SMMU_V1) { smmu->va_size = smmu->ipa_size; - size = SZ_4K | SZ_2M | SZ_1G; } else { size = (id >> ID2_UBS_SHIFT) & ID2_UBS_MASK; smmu->va_size = arm_smmu_id_size_to_bits(size); @@ -1734,13 +1784,25 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) #endif size = 0; if (id & ID2_PTFS_4K) - size |= SZ_4K | SZ_2M | SZ_1G; + smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_4K; if (id & ID2_PTFS_16K) - size |= SZ_16K | SZ_32M; + smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_16K; if (id & ID2_PTFS_64K) - size |= SZ_64K | SZ_512M; + smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_64K; } + /* Now we've corralled the various formats, what'll it do? */ + size = 0; + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_S) + size |= SZ_4K | SZ_64K | SZ_1M | SZ_16M; + if (smmu->features & + (ARM_SMMU_FEAT_FMT_AARCH32_L | ARM_SMMU_FEAT_FMT_AARCH64_4K)) + size |= SZ_4K | SZ_2M | SZ_1G; + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_16K) + size |= SZ_16K | SZ_32M; + if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_64K) + size |= SZ_64K | SZ_512M; + arm_smmu_ops.pgsize_bitmap &= size; dev_notice(smmu->dev, "\tSupported page sizes: 0x%08lx\n", size);
The way the driver currently forces an AArch32 or AArch64 context format based on the kernel config and SMMU architecture version is suboptimal, in that it makes it very hard to support oddball mix-and-match cases like the SMMUv1 64KB supplement, or situations where the reduced table depth of an AArch32 short descriptor context may be desirable under an AArch64 kernel. It also only happens to work on current implementations which do support all the relevant formats. Introduce an explicit notion of context format, so we can manage that independently and get rid of the inflexible #ifdeffery. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/arm-smmu.c | 98 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 80 insertions(+), 18 deletions(-)