[PATCH] arm64: mm: Pass physical address explicitly in map_range
Pingfan Liu
piliu at redhat.com
Mon Mar 18 04:12:19 PDT 2024
On Fri, Mar 15, 2024 at 7:39 PM Ard Biesheuvel <ardb at kernel.org> wrote:
>
> 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.
>
>From what angle, can it be considered as position dependent code?
Could you enlighten me so I can have a refreshed understanding of this
stuff.
> 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.
>
Is the 1:1 mapping the angle?
> 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.
>
Exactly, it ensures only PC-relative references.
> If there is confusion about this, feel free to propose improved
> documentation. But these code changes only make things worse imo.
>
Does this look so straightforward for senior developers? Or I can send
out some documents for it.
Thanks,
Pingfan
More information about the linux-arm-kernel
mailing list