[PATCH] arm64: mm: Pass physical address explicitly in map_range
Ard Biesheuvel
ardb at kernel.org
Fri Mar 15 04:38:48 PDT 2024
On Thu, 14 Mar 2024 at 13:53, Pingfan Liu <piliu at redhat.com> wrote:
>
> This patch is not a fix, just a improvement in code reading.
>
> At the present, the deduction of a symbol's physical address hides in
> the compiler's trick. Introduce a function paddr(), which make the
> process explicitly at the sacrifice of one sub and one add instruction.
>
> Signed-off-by: Pingfan Liu <piliu at redhat.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Will Deacon <will at kernel.org>
> Cc: Ard Biesheuvel <ardb at kernel.org>
> Cc: Kees Cook <keescook at chromium.org>
> To: linux-arm-kernel at lists.infradead.org
> ---
> arch/arm64/kernel/pi/map_kernel.c | 30 +++++++++++++++---------------
> arch/arm64/kernel/pi/map_range.c | 21 +++++++++++++++++++--
> arch/arm64/kernel/pi/pi.h | 1 +
> 3 files changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm64/kernel/pi/map_kernel.c b/arch/arm64/kernel/pi/map_kernel.c
> index cac1e1f63c44..f0bad9ff3cce 100644
> --- a/arch/arm64/kernel/pi/map_kernel.c
> +++ b/arch/arm64/kernel/pi/map_kernel.c
> @@ -78,16 +78,16 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
> twopass |= enable_scs;
> prot = twopass ? data_prot : text_prot;
>
> - map_segment(init_pg_dir, &pgdp, va_offset, _stext, _etext, prot,
> + map_segment(init_pg_dir, &pgdp, va_offset, paddr(_stext), paddr(_etext), prot,
> !twopass, root_level);
> - map_segment(init_pg_dir, &pgdp, va_offset, __start_rodata,
> - __inittext_begin, data_prot, false, root_level);
> - map_segment(init_pg_dir, &pgdp, va_offset, __inittext_begin,
> - __inittext_end, prot, false, root_level);
> - map_segment(init_pg_dir, &pgdp, va_offset, __initdata_begin,
> - __initdata_end, data_prot, false, root_level);
> - map_segment(init_pg_dir, &pgdp, va_offset, _data, _end, data_prot,
> - true, root_level);
> + map_segment(init_pg_dir, &pgdp, va_offset, paddr(__start_rodata),
> + paddr(__inittext_begin), data_prot, false, root_level);
> + map_segment(init_pg_dir, &pgdp, va_offset, paddr(__inittext_begin),
> + paddr(__inittext_end), prot, false, root_level);
> + map_segment(init_pg_dir, &pgdp, va_offset, paddr(__initdata_begin),
> + paddr(__initdata_end), data_prot, false, root_level);
> + map_segment(init_pg_dir, &pgdp, va_offset, paddr(_data), paddr(_end),
> + data_prot, true, root_level);
> dsb(ishst);
>
> idmap_cpu_replace_ttbr1(init_pg_dir);
> @@ -109,7 +109,7 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
> * potential TLB conflicts when creating the contiguous
> * descriptors.
> */
> - unmap_segment(init_pg_dir, va_offset, _stext, _etext,
> + unmap_segment(init_pg_dir, va_offset, paddr(_stext), paddr(_etext),
> root_level);
> dsb(ishst);
> isb();
> @@ -120,10 +120,10 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
> * Remap these segments with different permissions
> * No new page table allocations should be needed
> */
> - map_segment(init_pg_dir, NULL, va_offset, _stext, _etext,
> - text_prot, true, root_level);
> - map_segment(init_pg_dir, NULL, va_offset, __inittext_begin,
> - __inittext_end, text_prot, false, root_level);
> + map_segment(init_pg_dir, NULL, va_offset, paddr(_stext),
> + paddr(_etext), text_prot, true, root_level);
> + map_segment(init_pg_dir, NULL, va_offset, paddr(__inittext_begin),
> + paddr(__inittext_end), text_prot, false, root_level);
> }
>
> /* Copy the root page table to its final location */
> @@ -223,7 +223,7 @@ static void __init map_fdt(u64 fdt)
> asmlinkage void __init early_map_kernel(u64 boot_status, void *fdt)
> {
> static char const chosen_str[] __initconst = "/chosen";
> - u64 va_base, pa_base = (u64)&_text;
> + u64 va_base, pa_base = paddr(_text);
> u64 kaslr_offset = pa_base % MIN_KIMG_ALIGN;
> int root_level = 4 - CONFIG_PGTABLE_LEVELS;
> int va_bits = VA_BITS;
> diff --git a/arch/arm64/kernel/pi/map_range.c b/arch/arm64/kernel/pi/map_range.c
> index 5410b2cac590..b5a68dcf3cf5 100644
> --- a/arch/arm64/kernel/pi/map_range.c
> +++ b/arch/arm64/kernel/pi/map_range.c
> @@ -87,6 +87,23 @@ void __init map_range(u64 *pte, u64 start, u64 end, u64 pa, pgprot_t prot,
> }
> }
>
> +u64 __init paddr(char *symbol)
> +{
> + u64 _text_paddr;
> + u64 delta;
> +
> + asm volatile(
> + "adrp %0, _text;"
> + "add %0, %0, #:lo12:_text;"
> + : "=r" (_text_paddr)
> + :
> + :
> + );
> +
> + delta = symbol - _text;
> + return _text_paddr + delta;
> +}
> +
> asmlinkage u64 __init create_init_idmap(pgd_t *pg_dir, pteval_t clrmask)
> {
> u64 ptep = (u64)pg_dir + PAGE_SIZE;
> @@ -96,9 +113,9 @@ asmlinkage u64 __init create_init_idmap(pgd_t *pg_dir, pteval_t clrmask)
> pgprot_val(text_prot) &= ~clrmask;
> pgprot_val(data_prot) &= ~clrmask;
>
> - map_range(&ptep, (u64)_stext, (u64)__initdata_begin, (u64)_stext,
> + map_range(&ptep, paddr(_stext), paddr(__initdata_begin), paddr(_stext),
> text_prot, IDMAP_ROOT_LEVEL, (pte_t *)pg_dir, false, 0);
> - map_range(&ptep, (u64)__initdata_begin, (u64)_end, (u64)__initdata_begin,
> + map_range(&ptep, paddr(__initdata_begin), paddr(_end), paddr(__initdata_begin),
> data_prot, IDMAP_ROOT_LEVEL, (pte_t *)pg_dir, false, 0);
>
> return ptep;
> diff --git a/arch/arm64/kernel/pi/pi.h b/arch/arm64/kernel/pi/pi.h
> index c91e5e965cd3..7ffba712da06 100644
> --- a/arch/arm64/kernel/pi/pi.h
> +++ b/arch/arm64/kernel/pi/pi.h
> @@ -28,6 +28,7 @@ u64 kaslr_early_init(void *fdt, int chosen);
> void relocate_kernel(u64 offset);
> int scs_patch(const u8 eh_frame[], int size);
>
> +u64 paddr(char *symbol);
> void map_range(u64 *pgd, u64 start, u64 end, u64 pa, pgprot_t prot,
> int level, pte_t *tbl, bool may_use_cont, u64 va_offset);
>
Hello Pingfan,
I struggle to see the point of this patch. create_init_idmap() and
map_kernel() are only called from the early startup code, where the
code is mapped 1:1. Those routines are essentially position dependent,
just not in the conventional way.
You are adding a function that has the same properties - your paddr()
function is position dependent, and will only return the correct PA of
its argument if it is called from a 1:1 mapping of the code.
Both existing routines call map_range(), which is position
independent, and can (and is) used from other contexts as well. All
code built under the pi/ subdirectory is built with instrumentation
disabled and without absolute symbol references, making it safe for
this particular routine to be called from elsewhere. This is also what
guarantees that the references to _text are using the correct
translation, as only PC-relative references are permitted.
If there is confusion about this, feel free to propose improved
documentation. But these code changes only make things worse imo.
More information about the linux-arm-kernel
mailing list