Re: [PATCH v3 01/18] xen/arm64: flushtlb: Reduce scope of barrier for local TLB flush

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 13/12/2022 10:45, Julien Grall wrote:
> 
> 
> On 13/12/2022 09:11, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 12/12/2022 10:55, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> Per D5-4929 in ARM DDI 0487H.a:
>>> "A DSB NSH is sufficient to ensure completion of TLB maintenance
>>>   instructions that apply to a single PE. A DSB ISH is sufficient to
>>>   ensure completion of TLB maintenance instructions that apply to PEs
>>>   in the same Inner Shareable domain.
>>> "
>>>
>>> This means barrier after local TLB flushes could be reduced to
>>> non-shareable.
>>>
>>> Note that the scope of the barrier in the workaround has not been
>>> changed because Linux v6.1-rc8 is also using 'ish' and I couldn't
>>> find anything in the Neoverse N1 suggesting that a 'nsh' would
>>> be sufficient.
>>>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> ---
>>>
>>>      I have used an older version of the Arm Arm because the explanation
>>>      in the latest (ARM DDI 0487I.a) is less obvious. I reckon the paragraph
>>>      about DSB in D8.13.8 is missing the shareability. But this is implied
>>>      in B2.3.11:
>>>
>>>      "If the required access types of the DSB is reads and writes, the
>>>       following instructions issued by PEe before the DSB are complete for
>>>       the required shareability domain:
>>>
>>>       [...]
>>>
>>>       — All TLB maintenance instructions.
>>>      "
>>>
>>>      Changes in v3:
>>>          - Patch added
>>> ---
>>>   xen/arch/arm/include/asm/arm64/flushtlb.h | 27 ++++++++++++++---------
>>>   1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/include/asm/arm64/flushtlb.h 
>>> b/xen/arch/arm/include/asm/arm64/flushtlb.h
>>> index 7c5431518741..39d429ace552 100644
>>> --- a/xen/arch/arm/include/asm/arm64/flushtlb.h
>>> +++ b/xen/arch/arm/include/asm/arm64/flushtlb.h
>>> @@ -12,8 +12,9 @@
>>>    * ARM64_WORKAROUND_REPEAT_TLBI:
>> Before this line, in the same comment, we state DSB ISHST. This should also 
>> be changed
>> to reflect the change done by this patch.
> 
> This is on purpose. I decided to keep the sequence as-is and instead add
> a paragraph explaining that 'nsh' is sufficient for local TLB flushes.
> 
>>
>>>    * Modification of the translation table for a virtual address might lead 
>>> to
>>>    * read-after-read ordering violation.
>>> - * The workaround repeats TLBI+DSB operation for all the TLB flush 
>>> operations.
>>> - * While this is stricly not necessary, we don't want to take any risk.
>>> + * The workaround repeats TLBI+DSB ISH operation for all the TLB flush
>>> + * operations. While this is stricly not necessary, we don't want to
>> s/stricly/strictly/
>>
>>> + * take any risk.
>>>    *
>>>    * For Xen page-tables the ISB will discard any instructions fetched
>>>    * from the old mappings.
>>> @@ -21,38 +22,42 @@
>>>    * For the Stage-2 page-tables the ISB ensures the completion of the DSB
>>>    * (and therefore the TLB invalidation) before continuing. So we know
>>>    * the TLBs cannot contain an entry for a mapping we may have removed.
>>> + *
>>> + * Note that for local TLB flush, using non-shareable (nsh) is sufficient
>>> + * (see D5-4929 in ARM DDI 0487H.a). Althougth, the memory barrier in
>> s/Althougth/Although/
>>
>>> + * for the workaround is left as inner-shareable to match with Linux.
>> So for the workaround we stay with DSB ISH. But ...
>>
>>>    */
>>> -#define TLB_HELPER(name, tlbop)                  \
>>> +#define TLB_HELPER(name, tlbop, sh)              \
>>>   static inline void name(void)                    \
>>>   {                                                \
>>>       asm volatile(                                \
>>> -        "dsb  ishst;"                            \
>>> +        "dsb  "  # sh  "st;"                     \
>>>           "tlbi "  # tlbop  ";"                    \
>>>           ALTERNATIVE(                             \
>>>               "nop; nop;",                         \
>>> -            "dsb  ish;"                          \
>>> +            "dsb  "  # sh  ";"                   \
>> ... you do not adhere to this.
> 
> This is a leftover from my previous approach. I will drop it.
> 
> [...]
> 
>>
>> With the remarks fixed:
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> I am not planning to fix the first remark. Please let me know if your
> Reviewed-by tag stands.
I'm ok with it so you can keep my tag.

> 
> Cheers,
> 
> --
> Julien Grall

~Michal