| 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: <c6450dbe-b280-4096-b666-191311d957c3@arm.com>
Date: Thu, 11 Apr 2024 14:39:35 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Mark Rutland <mark.rutland@....com>
Cc: Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Ard Biesheuvel <ardb@...nel.org>, David Hildenbrand <david@...hat.com>,
Donald Dutile <ddutile@...hat.com>, Eric Chanudet <echanude@...hat.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Itaru Kitayama <itaru.kitayama@...itsu.com>
Subject: Re: [PATCH v2 4/4] arm64: mm: Lazily clear pte table mappings from
fixmap
On 11/04/2024 14:24, Mark Rutland wrote:
> On Thu, Apr 04, 2024 at 03:33:08PM +0100, Ryan Roberts wrote:
>> With the pgtable operations abstracted into `struct pgtable_ops`, the
>> early pgtable alloc, map and unmap operations are nicely centralized. So
>> let's enhance the implementation to speed up the clearing of pte table
>> mappings in the fixmap.
>>
>> Extend FIX_MAP so that we now have 16 slots in the fixmap dedicated for
>> pte tables. At alloc/map time, we select the next slot in the series and
>> map it. Or if we are at the end and no more slots are available, clear
>> down all of the slots and start at the beginning again. Batching the
>> clear like this means we can issue tlbis more efficiently.
>>
>> Due to the batching, there may still be some slots mapped at the end, so
>> address this by adding an optional cleanup() function to `struct
>> pgtable_ops` to handle this for us.
>>
>> Execution time of map_mem(), which creates the kernel linear map page
>> tables, was measured on different machines with different RAM configs:
>>
>> | Apple M2 VM | Ampere Altra| Ampere Altra| Ampere Altra
>> | VM, 16G | VM, 64G | VM, 256G | Metal, 512G
>> ---------------|-------------|-------------|-------------|-------------
>> | ms (%) | ms (%) | ms (%) | ms (%)
>> ---------------|-------------|-------------|-------------|-------------
>> before | 11 (0%) | 109 (0%) | 449 (0%) | 1257 (0%)
>> after | 6 (-46%) | 61 (-44%) | 257 (-43%) | 838 (-33%)
>
> I'd prefer to leave this as-is for now, as compared to the baseline this is the
> last 2-3%, and (assuming my comments on patch 3 hold) that way we don't need
> the pgtable_ops indirection, which'll keep the code fairly simple to understand.
>
> So unless Catalin or Will feel otherwise, I'd suggest that we take patches 1
> and 2, drop 3 and 4 for now, and maybe try the alternative approach I've
> commented on patch 3.
>
> Does that sound ok to you?
In principle, yes. Let me do some benchmarking with your proposed set of patches
to confim :)
>
> Mark.
>
>> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
>> Tested-by: Itaru Kitayama <itaru.kitayama@...itsu.com>
>> Tested-by: Eric Chanudet <echanude@...hat.com>
>> ---
>> arch/arm64/include/asm/fixmap.h | 5 +++-
>> arch/arm64/include/asm/pgtable.h | 4 ---
>> arch/arm64/mm/fixmap.c | 11 ++++++++
>> arch/arm64/mm/mmu.c | 44 +++++++++++++++++++++++++++++---
>> 4 files changed, 56 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
>> index 87e307804b99..91fcd7c5c513 100644
>> --- a/arch/arm64/include/asm/fixmap.h
>> +++ b/arch/arm64/include/asm/fixmap.h
>> @@ -84,7 +84,9 @@ enum fixed_addresses {
>> * Used for kernel page table creation, so unmapped memory may be used
>> * for tables.
>> */
>> - FIX_PTE,
>> +#define NR_PTE_SLOTS 16
>> + FIX_PTE_END,
>> + FIX_PTE_BEGIN = FIX_PTE_END + NR_PTE_SLOTS - 1,
>> FIX_PMD,
>> FIX_PUD,
>> FIX_P4D,
>> @@ -108,6 +110,7 @@ void __init early_fixmap_init(void);
>> #define __late_clear_fixmap(idx) __set_fixmap((idx), 0, FIXMAP_PAGE_CLEAR)
>>
>> extern void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot);
>> +void __init clear_fixmap_nosync(enum fixed_addresses idx);
>>
>> #include <asm-generic/fixmap.h>
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 92c9aed5e7af..4c7114d49697 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -691,10 +691,6 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>> /* Find an entry in the third-level page table. */
>> #define pte_offset_phys(dir,addr) (pmd_page_paddr(READ_ONCE(*(dir))) + pte_index(addr) * sizeof(pte_t))
>>
>> -#define pte_set_fixmap(addr) ((pte_t *)set_fixmap_offset(FIX_PTE, addr))
>> -#define pte_set_fixmap_offset(pmd, addr) pte_set_fixmap(pte_offset_phys(pmd, addr))
>> -#define pte_clear_fixmap() clear_fixmap(FIX_PTE)
>> -
>> #define pmd_page(pmd) phys_to_page(__pmd_to_phys(pmd))
>>
>> /* use ONLY for statically allocated translation tables */
>> diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c
>> index de1e09d986ad..0cb09bedeeec 100644
>> --- a/arch/arm64/mm/fixmap.c
>> +++ b/arch/arm64/mm/fixmap.c
>> @@ -131,6 +131,17 @@ void __set_fixmap(enum fixed_addresses idx,
>> }
>> }
>>
>> +void __init clear_fixmap_nosync(enum fixed_addresses idx)
>> +{
>> + unsigned long addr = __fix_to_virt(idx);
>> + pte_t *ptep;
>> +
>> + BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
>> +
>> + ptep = fixmap_pte(addr);
>> + __pte_clear(&init_mm, addr, ptep);
>> +}
>> +
>> void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
>> {
>> const u64 dt_virt_base = __fix_to_virt(FIX_FDT);
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 90bf822859b8..2e3b594aa23c 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -66,11 +66,14 @@ enum pgtable_type {
>> * mapped either as a result of a previous call to alloc() or
>> * map(). The page's virtual address must be considered invalid
>> * after this call returns.
>> + * @cleanup: (Optional) Called at the end of a set of operations to cleanup
>> + * any lazy state.
>> */
>> struct pgtable_ops {
>> void *(*alloc)(int type, phys_addr_t *pa);
>> void *(*map)(int type, void *parent, unsigned long addr);
>> void (*unmap)(int type);
>> + void (*cleanup)(void);
>> };
>>
>> #define NO_BLOCK_MAPPINGS BIT(0)
>> @@ -139,9 +142,33 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>> }
>> EXPORT_SYMBOL(phys_mem_access_prot);
>>
>> +static int pte_slot_next __initdata = FIX_PTE_BEGIN;
>> +
>> +static void __init clear_pte_fixmap_slots(void)
>> +{
>> + unsigned long start = __fix_to_virt(FIX_PTE_BEGIN);
>> + unsigned long end = __fix_to_virt(pte_slot_next);
>> + int i;
>> +
>> + for (i = FIX_PTE_BEGIN; i > pte_slot_next; i--)
>> + clear_fixmap_nosync(i);
>> +
>> + flush_tlb_kernel_range(start, end);
>> + pte_slot_next = FIX_PTE_BEGIN;
>> +}
>> +
>> +static int __init pte_fixmap_slot(void)
>> +{
>> + if (pte_slot_next < FIX_PTE_END)
>> + clear_pte_fixmap_slots();
>> +
>> + return pte_slot_next--;
>> +}
>> +
>> static void *__init early_pgtable_alloc(int type, phys_addr_t *pa)
>> {
>> void *va;
>> + int slot;
>>
>> *pa = memblock_phys_alloc_range(PAGE_SIZE, PAGE_SIZE, 0,
>> MEMBLOCK_ALLOC_NOLEAKTRACE);
>> @@ -159,7 +186,9 @@ static void *__init early_pgtable_alloc(int type, phys_addr_t *pa)
>> va = pmd_set_fixmap(*pa);
>> break;
>> case TYPE_PTE:
>> - va = pte_set_fixmap(*pa);
>> + slot = pte_fixmap_slot();
>> + set_fixmap(slot, *pa);
>> + va = (pte_t *)__fix_to_virt(slot);
>> break;
>> default:
>> BUG();
>> @@ -174,7 +203,9 @@ static void *__init early_pgtable_alloc(int type, phys_addr_t *pa)
>>
>> static void *__init early_pgtable_map(int type, void *parent, unsigned long addr)
>> {
>> + phys_addr_t pa;
>> void *entry;
>> + int slot;
>>
>> switch (type) {
>> case TYPE_P4D:
>> @@ -187,7 +218,10 @@ static void *__init early_pgtable_map(int type, void *parent, unsigned long addr
>> entry = pmd_set_fixmap_offset((pud_t *)parent, addr);
>> break;
>> case TYPE_PTE:
>> - entry = pte_set_fixmap_offset((pmd_t *)parent, addr);
>> + slot = pte_fixmap_slot();
>> + pa = pte_offset_phys((pmd_t *)parent, addr);
>> + set_fixmap(slot, pa);
>> + entry = (pte_t *)(__fix_to_virt(slot) + (pa & (PAGE_SIZE - 1)));
>> break;
>> default:
>> BUG();
>> @@ -209,7 +243,7 @@ static void __init early_pgtable_unmap(int type)
>> pmd_clear_fixmap();
>> break;
>> case TYPE_PTE:
>> - pte_clear_fixmap();
>> + // Unmap lazily: see clear_pte_fixmap_slots().
>> break;
>> default:
>> BUG();
>> @@ -220,6 +254,7 @@ static struct pgtable_ops early_pgtable_ops __initdata = {
>> .alloc = early_pgtable_alloc,
>> .map = early_pgtable_map,
>> .unmap = early_pgtable_unmap,
>> + .cleanup = clear_pte_fixmap_slots,
>> };
>>
>> bool pgattr_change_is_safe(u64 old, u64 new)
>> @@ -538,6 +573,9 @@ static void __create_pgd_mapping_locked(pgd_t *pgdir, phys_addr_t phys,
>> alloc_init_p4d(pgdp, addr, next, phys, prot, ops, flags);
>> phys += next - addr;
>> } while (pgdp++, addr = next, addr != end);
>> +
>> + if (ops->cleanup)
>> + ops->cleanup();
>> }
>>
>> static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>> --
>> 2.25.1
>>
Powered by blists - more mailing lists