Re: [PATCH v2 08/10] iommu/io-pgtable-arm: Rationalise TTBRn handling

[Robin Murphy <robin.murphy@arm.com>]

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