On 25/04/16 14:41, Will Deacon wrote:
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.
Agreed - that sounds like the best approach in the long run, and I'd be
happy to put some thought into designing it once the to-do list shrinks
a bit. Right now, though, how about a quick fix like I just hacked up
below? (diff on top of this series because laziness)
Robin.
--->8---
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a917ad0e949f..0798926f28f1 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -784,28 +784,20 @@ static int arm_smmu_init_domain_context(struct
iommu_domain *domain,
/*
* 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.
+ * grow some way for the caller to express a preference, and/or move
+ * the decision into the io-pgtable code where it arguably belongs,
+ * just aim for the closest thing to the rest of the system, and hope
+ * that the hardware isn't mental enough that we can't assume AArch64
+ * support to be a superset of AArch32 support...
*/
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 ((IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) &&
+ (smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_64K |
+ ARM_SMMU_FEAT_FMT_AARCH64_16K |
+ 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;
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
