| 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: <eb57601d-abac-492a-8e62-dc1c406af572@arm.com>
Date: Wed, 21 May 2025 15:14:20 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Dev Jain <dev.jain@....com>, akpm@...ux-foundation.org
Cc: david@...hat.com, willy@...radead.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, catalin.marinas@....com, will@...nel.org,
Liam.Howlett@...cle.com, lorenzo.stoakes@...cle.com, vbabka@...e.cz,
jannh@...gle.com, anshuman.khandual@....com, peterx@...hat.com,
joey.gouly@....com, ioworker0@...il.com, baohua@...nel.org,
kevin.brodsky@....com, quic_zhenhuah@...cinc.com,
christophe.leroy@...roup.eu, yangyicong@...ilicon.com,
linux-arm-kernel@...ts.infradead.org, hughd@...gle.com,
yang@...amperecomputing.com, ziy@...dia.com
Subject: Re: [PATCH v3 4/5] arm64: Add batched version of
ptep_modify_prot_start
On 19/05/2025 08:48, Dev Jain wrote:
> Override the generic definition to use get_and_clear_full_ptes(). This helper
> does a TLBI only for the starting and ending contpte block of the range, whereas
> the current implementation will call ptep_get_and_clear() for every contpte block,
> thus doing a TLBI on every contpte block. Therefore, we have a performance win.
> The arm64 definition of pte_accessible() allows us to batch around it in clear_flush_ptes():
> #define pte_accessible(mm, pte) \
> (mm_tlb_flush_pending(mm) ? pte_present(pte) : pte_valid(pte))
>
> All ptes are obviously present in the folio batch, and they are also valid.
>
> Signed-off-by: Dev Jain <dev.jain@....com>
Please squash this with the
> ---
> arch/arm64/include/asm/pgtable.h | 5 +++++
> arch/arm64/mm/mmu.c | 12 +++++++++---
> include/linux/pgtable.h | 4 ++++
> mm/pgtable-generic.c | 16 +++++++++++-----
> 4 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 2a77f11b78d5..8872ea5f0642 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -1553,6 +1553,11 @@ extern void ptep_modify_prot_commit(struct vm_area_struct *vma,
> unsigned long addr, pte_t *ptep,
> pte_t old_pte, pte_t new_pte);
>
> +#define modify_prot_start_ptes modify_prot_start_ptes
> +extern pte_t modify_prot_start_ptes(struct vm_area_struct *vma,
> + unsigned long addr, pte_t *ptep,
> + unsigned int nr);
> +
> #ifdef CONFIG_ARM64_CONTPTE
>
> /*
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 8fcf59ba39db..fe60be8774f4 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1523,7 +1523,8 @@ static int __init prevent_bootmem_remove_init(void)
> early_initcall(prevent_bootmem_remove_init);
> #endif
>
> -pte_t ptep_modify_prot_start(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep)
> +pte_t modify_prot_start_ptes(struct vm_area_struct *vma, unsigned long addr,
> + pte_t *ptep, unsigned int nr)
> {
> if (alternative_has_cap_unlikely(ARM64_WORKAROUND_2645198)) {
> /*
> @@ -1532,9 +1533,14 @@ pte_t ptep_modify_prot_start(struct vm_area_struct *vma, unsigned long addr, pte
> * in cases where cpu is affected with errata #2645198.
> */
> if (pte_user_exec(ptep_get(ptep)))
> - return ptep_clear_flush(vma, addr, ptep);
> + return clear_flush_ptes(vma, addr, ptep, nr);
> }
> - return ptep_get_and_clear(vma->vm_mm, addr, ptep);
> + return get_and_clear_full_ptes(vma->vm_mm, addr, ptep, nr, 0);
> +}
I think we can do this more precisely with respect to tlbis and also without
needing to create a new clear_flush_ptes() helper:
pte_t modify_prot_start_ptes(struct vm_area_struct *vma, unsigned long addr,
pte_t *ptep, unsigned int nr)
{
pte_t pte = get_and_clear_full_ptes(vma->vm_mm, addr, ptep, nr, 0);
if (alternative_has_cap_unlikely(ARM64_WORKAROUND_2645198)) {
/*
* Break-before-make (BBM) is required for all user space mappings
* when the permission changes from executable to non-executable
* in cases where cpu is affected with errata #2645198.
*/
if (pte_accessible(vma->vm_mm, pte) && pte_user_exec(pte))
__flush_tlb_range(vma, addr, nr * PAGE_SIZE,
PAGE_SIZE, true, 3);
}
return pte;
}
> +
> +pte_t ptep_modify_prot_start(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep)
> +{
> + return modify_prot_start_ptes(vma, addr, ptep, 1);
> }
>
> void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep,
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index e40ed57e034d..41f4a8de5c28 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -828,6 +828,10 @@ extern pte_t ptep_clear_flush(struct vm_area_struct *vma,
> pte_t *ptep);
> #endif
>
> +extern pte_t clear_flush_ptes(struct vm_area_struct *vma,
> + unsigned long address,
> + pte_t *ptep, unsigned int nr);
> +
> #ifndef __HAVE_ARCH_PMDP_HUGE_CLEAR_FLUSH
> extern pmd_t pmdp_huge_clear_flush(struct vm_area_struct *vma,
> unsigned long address,
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index 5a882f2b10f9..e238f88c3cac 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -90,17 +90,23 @@ int ptep_clear_flush_young(struct vm_area_struct *vma,
> }
> #endif
>
> -#ifndef __HAVE_ARCH_PTEP_CLEAR_FLUSH
> -pte_t ptep_clear_flush(struct vm_area_struct *vma, unsigned long address,
> - pte_t *ptep)
> +pte_t clear_flush_ptes(struct vm_area_struct *vma, unsigned long address,
> + pte_t *ptep, unsigned int nr)
> {
> struct mm_struct *mm = (vma)->vm_mm;
> pte_t pte;
> - pte = ptep_get_and_clear(mm, address, ptep);
> + pte = get_and_clear_full_ptes(mm, address, ptep, nr, 0);
> if (pte_accessible(mm, pte))
> - flush_tlb_page(vma, address);
> + flush_tlb_range(vma, address, address + nr * PAGE_SIZE);
> return pte;
> }
Let's not create a new generic helper if only arm64 is using it. We would
also want to add a doc header to describe this helper. My proposal avoids
this.
Thanks,
Ryan
> +
> +#ifndef __HAVE_ARCH_PTEP_CLEAR_FLUSH
> +pte_t ptep_clear_flush(struct vm_area_struct *vma, unsigned long address,
> + pte_t *ptep)
> +{
> + return clear_flush_ptes(vma, address, ptep, 1);
> +}
> #endif
>
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
Powered by blists - more mailing lists