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

From: Michal Orzel <michal.orzel@amd.com>
To: Julien Grall <julien@xen.org>, <xen-devel@lists.xenproject.org>
Cc: <Luca.Fancellu@arm.com>, Julien Grall <jgrall@amazon.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v3 01/18] xen/arm64: flushtlb: Reduce scope of barrier for local TLB flush
Date: Tue, 13 Dec 2022 10:11:06 +0100	[thread overview]
Message-ID: <650bc040-63a9-8950-e2ff-6829c9a452a8@amd.com> (raw)
In-Reply-To: <20221212095523.52683-2-julien@xen.org>

Hi Julien,

On 12/12/2022 10:55, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> 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@amazon.com>
> 
> ---
> 
>     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.

>   * 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.

>              "tlbi "  # tlbop  ";",               \
>              ARM64_WORKAROUND_REPEAT_TLBI,        \
>              CONFIG_ARM64_WORKAROUND_REPEAT_TLBI) \
> -        "dsb  ish;"                              \
> +        "dsb  "  # sh  ";"                       \
>          "isb;"                                   \
>          : : : "memory");                         \
>  }
> 
>  /* Flush local TLBs, current VMID only. */
> -TLB_HELPER(flush_guest_tlb_local, vmalls12e1);
> +TLB_HELPER(flush_guest_tlb_local, vmalls12e1, nsh);
> 
>  /* Flush innershareable TLBs, current VMID only */
> -TLB_HELPER(flush_guest_tlb, vmalls12e1is);
> +TLB_HELPER(flush_guest_tlb, vmalls12e1is, ish);
> 
>  /* Flush local TLBs, all VMIDs, non-hypervisor mode */
> -TLB_HELPER(flush_all_guests_tlb_local, alle1);
> +TLB_HELPER(flush_all_guests_tlb_local, alle1, nsh);
> 
>  /* Flush innershareable TLBs, all VMIDs, non-hypervisor mode */
> -TLB_HELPER(flush_all_guests_tlb, alle1is);
> +TLB_HELPER(flush_all_guests_tlb, alle1is, ish);
> 
>  /* Flush all hypervisor mappings from the TLB of the local processor. */
> -TLB_HELPER(flush_xen_tlb_local, alle2);
> +TLB_HELPER(flush_xen_tlb_local, alle2, nsh);
> 
>  /* Flush TLB of local processor for address va. */
>  static inline void  __flush_xen_tlb_one_local(vaddr_t va)
> --
> 2.38.1
> 

With the remarks fixed:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal