Re: [v5 PATCH 1/2] hugetlb: arm64: add mte support

[David Hildenbrand <david@redhat.com>]

On 13.09.24 20:34, Yang Shi wrote:
> Enable MTE support for hugetlb.
> 
> The MTE page flags will be set on the folio only.  When copying
> hugetlb folio (for example, CoW), the tags for all subpages will be copied
> when copying the first subpage.
> 
> When freeing hugetlb folio, the MTE flags will be cleared.
> 
> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
> ---
>   arch/arm64/include/asm/hugetlb.h |  8 ++++
>   arch/arm64/include/asm/mman.h    |  3 +-
>   arch/arm64/include/asm/mte.h     | 67 ++++++++++++++++++++++++++++++++
>   arch/arm64/kernel/hibernate.c    |  6 +++
>   arch/arm64/kernel/mte.c          | 27 ++++++++++++-
>   arch/arm64/kvm/guest.c           | 16 ++++++--
>   arch/arm64/kvm/mmu.c             | 11 ++++++
>   arch/arm64/mm/copypage.c         | 27 ++++++++++++-
>   fs/hugetlbfs/inode.c             |  2 +-
>   9 files changed, 159 insertions(+), 8 deletions(-)
> 
> v5: * Indentation fix and renaming per Catalin.
> v4: * Fixed the comment from David.
> v3: * Fixed the build error when !CONFIG_ARM64_MTE.
>      * Incorporated the comment from David to have hugetlb folio
>        specific APIs for manipulating the page flags.
>      * Don't assume the first page is the head page since huge page copy
>        can start from any subpage.
> v2: * Reimplemented the patch to fix the comments from Catalin.
>      * Added test cases (patch #2) per Catalin.
> 
> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
> index 293f880865e8..c6dff3e69539 100644
> --- a/arch/arm64/include/asm/hugetlb.h
> +++ b/arch/arm64/include/asm/hugetlb.h
> @@ -11,6 +11,7 @@
>   #define __ASM_HUGETLB_H
>   
>   #include <asm/cacheflush.h>
> +#include <asm/mte.h>
>   #include <asm/page.h>
>   
>   #ifdef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
> @@ -21,6 +22,13 @@ extern bool arch_hugetlb_migration_supported(struct hstate *h);
>   static inline void arch_clear_hugetlb_flags(struct folio *folio)
>   {
>   	clear_bit(PG_dcache_clean, &folio->flags);
> +
> +#ifdef CONFIG_ARM64_MTE
> +	if (system_supports_mte()) {
> +		clear_bit(PG_mte_tagged, &folio->flags);
> +		clear_bit(PG_mte_lock, &folio->flags);
> +	}
> +#endif
>   }
>   #define arch_clear_hugetlb_flags arch_clear_hugetlb_flags
>   
> diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> index 5966ee4a6154..304dfc499e68 100644
> --- a/arch/arm64/include/asm/mman.h
> +++ b/arch/arm64/include/asm/mman.h
> @@ -28,7 +28,8 @@ static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags)
>   	 * backed by tags-capable memory. The vm_flags may be overridden by a
>   	 * filesystem supporting MTE (RAM-based).
>   	 */
> -	if (system_supports_mte() && (flags & MAP_ANONYMOUS))
> +	if (system_supports_mte() &&
> +	    (flags & (MAP_ANONYMOUS | MAP_HUGETLB)))
>   		return VM_MTE_ALLOWED;
>   
>   	return 0;
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 0f84518632b4..03dc43636aba 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -41,6 +41,8 @@ void mte_free_tag_storage(char *storage);
>   
>   static inline void set_page_mte_tagged(struct page *page)
>   {
> +	VM_WARN_ON_ONCE(folio_test_hugetlb(page_folio(page)));
> +
>   	/*
>   	 * Ensure that the tags written prior to this function are visible
>   	 * before the page flags update.
> @@ -51,6 +53,8 @@ static inline void set_page_mte_tagged(struct page *page)
>   
>   static inline bool page_mte_tagged(struct page *page)
>   {
> +	VM_WARN_ON_ONCE(folio_test_hugetlb(page_folio(page)));
> +
>   	bool ret = test_bit(PG_mte_tagged, &page->flags);
>   
>   	/*
> @@ -76,6 +80,8 @@ static inline bool page_mte_tagged(struct page *page)
>    */
>   static inline bool try_page_mte_tagging(struct page *page)
>   {
> +	VM_WARN_ON_ONCE(folio_test_hugetlb(page_folio(page)));
> +
>   	if (!test_and_set_bit(PG_mte_lock, &page->flags))
>   		return true;
>   
> @@ -157,6 +163,67 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
>   
>   #endif /* CONFIG_ARM64_MTE */
>   
> +#if defined(CONFIG_HUGETLB_PAGE) && defined(CONFIG_ARM64_MTE)
> +static inline void folio_set_hugetlb_mte_tagged(struct folio *folio)
> +{
> +	VM_WARN_ON_ONCE(!folio_test_hugetlb(folio));
> +
> +	/*
> +	 * Ensure that the tags written prior to this function are visible
> +	 * before the folio flags update.
> +	 */
> +	smp_wmb();
> +	set_bit(PG_mte_tagged, &folio->flags);
> +
> +}
> +
> +static inline bool folio_test_hugetlb_mte_tagged(struct folio *folio)
> +{
> +	VM_WARN_ON_ONCE(!folio_test_hugetlb(folio));
> +
> +	bool ret = test_bit(PG_mte_tagged, &folio->flags);

Nit: VM_WARN_ should come after "bool ret" ...

> +
> +	/*
> +	 * If the folio is tagged, ensure ordering with a likely subsequent
> +	 * read of the tags.
> +	 */
> +	if (ret)
> +		smp_rmb();
> +	return ret;
> +}
> +

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb