On 04/11/2019 18:36, Will Deacon wrote:
On Mon, Oct 28, 2019 at 06:51:55PM +0000, Robin Murphy wrote:
On 28/10/2019 15:09, Steven Price wrote:
[...]
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -822,15 +822,13 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct
io_pgtable_cfg *cfg,
/* Ensure the empty pgd is visible before any actual TTBR write */
wmb();
- /* TTBRs */
- cfg->arm_v7s_cfg.ttbr[0] = virt_to_phys(data->pgd) |
- ARM_V7S_TTBR_S | ARM_V7S_TTBR_NOS |
- (cfg->coherent_walk ?
- (ARM_V7S_TTBR_IRGN_ATTR(ARM_V7S_RGN_WBWA) |
- ARM_V7S_TTBR_ORGN_ATTR(ARM_V7S_RGN_WBWA)) :
- (ARM_V7S_TTBR_IRGN_ATTR(ARM_V7S_RGN_NC) |
- ARM_V7S_TTBR_ORGN_ATTR(ARM_V7S_RGN_NC)));
- cfg->arm_v7s_cfg.ttbr[1] = 0;
+ /* TTBR */
+ cfg->arm_v7s_cfg.ttbr = virt_to_phys(data->pgd) | ARM_V7S_TTBR_S |
+ (cfg->coherent_walk ? (ARM_V7S_TTBR_NOS |
+ ARM_V7S_TTBR_IRGN_ATTR(ARM_V7S_RGN_WBWA) |
+ ARM_V7S_TTBR_ORGN_ATTR(ARM_V7S_RGN_WBWA)) :
+ (ARM_V7S_TTBR_IRGN_ATTR(ARM_V7S_RGN_NC) |
+ ARM_V7S_TTBR_ORGN_ATTR(ARM_V7S_RGN_NC)));
ARM_V7S_TTBR_NOS seems to have sneaked into the cfg->coherent_walk
condition here - which you haven't mentioned in the commit log, so it
doesn't look like it should be in this commit.
Ah, yes, it's taken a while to remember whether this was something important
that got muddled up in rebasing, but it's actually just trivial cleanup. For
!coherent_walk, the non-cacheable output attribute makes shareable accesses
implicitly outer-shareable, so setting TTBR.NOS for that case actually does
nothing except look misleading. Thus this is essentially just a cosmetic
change included in the reformatting for clarity and consistency with the
LPAE version. I'll call that out in the commit message, thanks for spotting!
I vaguely remember a case where you had to mark non-cacheable accesses as
outer-shareable explicitly to avoid unpredictable behaviour. Hmm.
/me looks at the Arm ARM
Ok, it looks like this changed between ARMv7 and ARMv8. The ARMv7 ARM
states:
| A memory region with a resultant memory type attribute of Normal, and a
| resultant cacheability attribute of Inner Non-cacheable, Outer
| Non-cacheable, must have a resultant shareability attribute of Outer
| Shareable, otherwise shareability is UNPREDICTABLE.
Although, SMMUv2 does go a bit further in saying:
"In SMMUv2, the SMMU treats final attributes that are Normal Inner
Non-cacheable or Normal Outer Non-cacheable as Outer Shareable. In
SMMUv1, it is IMPLEMENTATION DEFINED how the SMMU treats such attributes."
and SMMUv3 follows similar lines:
"The SMMU does not output inconsistent attributes as a result of
misconfiguration. Outer Shareable is used as the effective Shareability
when Device or Normal Inner Non-cacheable Outer Non-cacheable types are
configured."
Although this only seems to be the case for LPAE! The short descriptor docs are
less clear, but I think it might be wise to ensure that non-cacheable mappings
are always outer-shareable for consistency.
Agreed, despite the above I think it does make sense to be explicit and
not rely on subtleties. Between 9e6ea59f3ff3 and this patch we should
have walks covered, so I can spin a followup to fix actual mappings as well.
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
