| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9e4e5536-05bb-4309-8d08-7158f9e7e8af@arm.com>
Date: Thu, 5 Oct 2023 15:41:01 +0100
From: Steven Price <steven.price@....com>
To: Ryan Roberts <ryan.roberts@....com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Peter Collingbourne <pcc@...gle.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64/mm: Hoist synchronization out of set_ptes() loop
On 05/10/2023 15:07, Ryan Roberts wrote:
> set_ptes() sets a physically contiguous block of memory (which all
> belongs to the same folio) to a contiguous block of ptes. The arm64
> implementation of this previously just looped, operating on each
> individual pte. But the __sync_icache_dcache() and mte_sync_tags()
> operations can both be hoisted out of the loop so that they are
> performed once for the contiguous set of pages (which may be less than
> the whole folio). This should result in minor performance gains.
>
> __sync_icache_dcache() already acts on the whole folio, and sets a flag
> in the folio so that it skips duplicate calls. But by hoisting the call,
> all the pte testing is done only once.
>
> mte_sync_tags() operates on each individual page with its own loop. But
> by passing the number of pages explicitly, we can rely solely on its
> loop and do the checks only once. This approach also makes it robust for
> the future, rather than assuming if a head page of a compound page is
> being mapped, then the whole compound page is being mapped, instead we
> explicitly know how many pages are being mapped. The old assumption may
> not continue to hold once the "anonymous large folios" feature is
> merged.
>
> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
Reviewed-by: Steven Price <steven.price@....com>
> ---
> v2 fixes the invocations of __set_pte_at() to pass nr_pages rather than order.
> Thanks to Steven Price for pointing that out.
>
> arch/arm64/include/asm/mte.h | 4 ++--
> arch/arm64/include/asm/pgtable.h | 27 +++++++++++++++++----------
> arch/arm64/kernel/mte.c | 4 ++--
> 3 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 4cedbaa16f41..91fbd5c8a391 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -90,7 +90,7 @@ static inline bool try_page_mte_tagging(struct page *page)
> }
>
> void mte_zero_clear_page_tags(void *addr);
> -void mte_sync_tags(pte_t pte);
> +void mte_sync_tags(pte_t pte, unsigned int nr_pages);
> void mte_copy_page_tags(void *kto, const void *kfrom);
> void mte_thread_init_user(void);
> void mte_thread_switch(struct task_struct *next);
> @@ -122,7 +122,7 @@ static inline bool try_page_mte_tagging(struct page *page)
> static inline void mte_zero_clear_page_tags(void *addr)
> {
> }
> -static inline void mte_sync_tags(pte_t pte)
> +static inline void mte_sync_tags(pte_t pte, unsigned int nr_pages)
> {
> }
> static inline void mte_copy_page_tags(void *kto, const void *kfrom)
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 7f7d9b1df4e5..68984ba9ce2a 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -325,8 +325,7 @@ static inline void __check_safe_pte_update(struct mm_struct *mm, pte_t *ptep,
> __func__, pte_val(old_pte), pte_val(pte));
> }
>
> -static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
> - pte_t *ptep, pte_t pte)
> +static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
> {
> if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte))
> __sync_icache_dcache(pte);
> @@ -339,20 +338,18 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
> */
> if (system_supports_mte() && pte_access_permitted(pte, false) &&
> !pte_special(pte) && pte_tagged(pte))
> - mte_sync_tags(pte);
> -
> - __check_safe_pte_update(mm, ptep, pte);
> -
> - set_pte(ptep, pte);
> + mte_sync_tags(pte, nr_pages);
> }
>
> static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, pte_t pte, unsigned int nr)
> {
> page_table_check_ptes_set(mm, ptep, pte, nr);
> + __sync_cache_and_tags(pte, nr);
>
> for (;;) {
> - __set_pte_at(mm, addr, ptep, pte);
> + __check_safe_pte_update(mm, ptep, pte);
> + set_pte(ptep, pte);
> if (--nr == 0)
> break;
> ptep++;
> @@ -531,18 +528,28 @@ static inline pmd_t pmd_mkdevmap(pmd_t pmd)
> #define pud_pfn(pud) ((__pud_to_phys(pud) & PUD_MASK) >> PAGE_SHIFT)
> #define pfn_pud(pfn,prot) __pud(__phys_to_pud_val((phys_addr_t)(pfn) << PAGE_SHIFT) | pgprot_val(prot))
>
> +static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
> + pte_t *ptep, pte_t pte, unsigned int nr)
> +{
> + __sync_cache_and_tags(pte, nr);
> + __check_safe_pte_update(mm, ptep, pte);
> + set_pte(ptep, pte);
> +}
> +
> static inline void set_pmd_at(struct mm_struct *mm, unsigned long addr,
> pmd_t *pmdp, pmd_t pmd)
> {
> page_table_check_pmd_set(mm, pmdp, pmd);
> - return __set_pte_at(mm, addr, (pte_t *)pmdp, pmd_pte(pmd));
> + return __set_pte_at(mm, addr, (pte_t *)pmdp, pmd_pte(pmd),
> + PMD_SIZE >> PAGE_SHIFT);
> }
>
> static inline void set_pud_at(struct mm_struct *mm, unsigned long addr,
> pud_t *pudp, pud_t pud)
> {
> page_table_check_pud_set(mm, pudp, pud);
> - return __set_pte_at(mm, addr, (pte_t *)pudp, pud_pte(pud));
> + return __set_pte_at(mm, addr, (pte_t *)pudp, pud_pte(pud),
> + PUD_SIZE >> PAGE_SHIFT);
> }
>
> #define __p4d_to_phys(p4d) __pte_to_phys(p4d_pte(p4d))
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 4edecaac8f91..2fb5e7a7a4d5 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -35,10 +35,10 @@ DEFINE_STATIC_KEY_FALSE(mte_async_or_asymm_mode);
> EXPORT_SYMBOL_GPL(mte_async_or_asymm_mode);
> #endif
>
> -void mte_sync_tags(pte_t pte)
> +void mte_sync_tags(pte_t pte, unsigned int nr_pages)
> {
> struct page *page = pte_page(pte);
> - long i, nr_pages = compound_nr(page);
> + unsigned int i;
>
> /* if PG_mte_tagged is set, tags have already been initialised */
> for (i = 0; i < nr_pages; i++, page++) {
> --
> 2.25.1
>
Powered by blists - more mailing lists