Re: [PATCH] arm64: mm: Move KPTI helpers to mmu.c

[Kevin Brodsky <kevin.brodsky@arm.com>]

On 11/09/2025 08:24, Ard Biesheuvel wrote:
> On Thu, 11 Sept 2025 at 08:18, Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
>>
>>
>> On 11/09/25 11:38 AM, Ard Biesheuvel wrote:
>>> On Thu, 11 Sept 2025 at 07:13, Anshuman Khandual
>>> <anshuman.khandual@arm.com> wrote:
>>>>
>>>>
>>>> On 10/09/25 4:14 PM, Kevin Brodsky wrote:
>>>>> create_kpti_ng_temp_pgd() is currently defined (as an alias) in
>>>>> mmu.c without matching declaration in a header; instead cpufeature.c
>>>>> makes its own declaration. This is clearly not pretty, and as commit
>>>>> ceca927c86e6 ("arm64: mm: Fix CFI failure due to kpti_ng_pgd_alloc
>>>>> function signature") showed, it also makes it very easy for the
>>>>> prototypes to go out of sync.
>>>>>
>>>>> All this would be much simpler if kpti_install_ng_mappings() and
>>>>> associated functions lived in mmu.c, where they logically belong.
>>>>> This is what this patch does:
>>>>> - Move kpti_install_ng_mappings() and associated functions from
>>>>>   cpufeature.c to mmu.c, add a declaration to <asm/mmu.h>
>>>>> - Make create_kpti_ng_temp_pgd() a static function that simply calls
>>>>>   __create_pgd_mapping_locked() instead of aliasing it
>>>>> - Mark all these functions __init
>>>>> - Move __initdata after kpti_ng_temp_alloc (as suggested by
>>>>>   checkpatch)
>>>>>
>>>>> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
>>>>> ---
>>>>> Note: as things stand, create_kpti_ng_temp_pgd() could be removed,
>>>>> but a separate patch [1] will make use of it to add an
>>>>> assertion.
>>>>>
>>>>> [1] https://lore.kernel.org/all/20250813145607.1612234-3-chaitanyas.prakash@arm.com/
>>>>> ---
>>>>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>> Cc: Kees Cook <kees@kernel.org>,
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> Cc: Will Deacon <will@kernel.org>
>>>>> Cc: Yeoreum Yun <yeoreum.yun@arm.com>
>>>>> ---
>>>>>  arch/arm64/include/asm/mmu.h   |   6 ++
>>>>>  arch/arm64/kernel/cpufeature.c |  97 ------------------------------
>>>>>  arch/arm64/mm/mmu.c            | 106 ++++++++++++++++++++++++++++++---
>>>>>  3 files changed, 103 insertions(+), 106 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
>>>>> index 49f1a810df16..624edd6c4964 100644
>>>>> --- a/arch/arm64/include/asm/mmu.h
>>>>> +++ b/arch/arm64/include/asm/mmu.h
>>>>> @@ -104,5 +104,11 @@ static inline bool kaslr_requires_kpti(void)
>>>>>       return true;
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_UNMAP_KERNEL_AT_EL0
>>>>> +void kpti_install_ng_mappings(void);
>>>> Could the declarations be moved here instead ?
>>> Why?
>> To avoid both typedef and external instance declaration in the C
>> code even though there is just a single call site in there.
> But why would we want to avoid those in the first place?
>
> Moving these into mmu.h pollutes the global namespace with
> declarations that must never be used outside of
> __kpti_install_ng_mappings() to begin with.

That makes sense to me - no need for these declarations to be in a
header as they're implementation details of
__kpti_install_ng_mappings(). What is more common is to have them at the
top-level in the .c file instead of inside a function, but I think
either way is fine.

- Kevin