[PATCH] arm64: mm: Pass physical address explicitly in map_range

[Pingfan Liu]

On Mon, Mar 18, 2024 at 8:31 PM Ard Biesheuvel <ardb at kernel.org> wrote:
>
> On Mon, 18 Mar 2024 at 12:12, Pingfan Liu <piliu at redhat.com> wrote:
> >
> > 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(-)
> > > >
> ...
> > >
> > > 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.
> >
>
> There are basically two kinds of position independent code:
> a) relocatable code, which contains statically initialized pointer
> variables (which are absolute) but with metadata that allows a loader
> to fix up those references to be correct for the actual placement of
> the code;
> b) true position independent code, which does not use any absolute
> references at all, and can therefore execute at any offset
>
> Toolchains always generate a), even with -fPIC/-fPIE etc, which mostly

OK, here I get your point. Appreciate your patient explanation so that
I can have a fresh viewpoint of the whole stuff.

> control optimizations/codegen restrictions that allow shared libraries
> and PIE executables to be placed at arbitrary offsets in memory
> without the need for all code and r/o data sections to be updatable
> (for relocation fixups).
>
> In the kernel startup code, we use b), i.e., we generate code using
> PC-relative symbol references, and explicitly forbid the use of
> R_AARCH64_ABS64 relocations so that fixups are never needed. Since
> toolchains generate a), we need to use a special tool (relacheck) for
> this. The resulting code can execute anywhere in memory, and variable
> reads and writes will work as expected, given that they are
> PC-relative.
>
> This code is in charge of creating the initial 1:1 mapping of memory,
> and therefore, it needs the 1:1 mapped addresses of _text and _end,
> etc. Given that we only permit PC-relative references in this code,
> the only way it can produce the correct 1:1 address of those symbols
> is if the code itself is mapped 1:1 too. In other words, calling the
> same code again from the kernel's normal virtual mapping would produce
> a bogus mapping. This does not matter, though: this code has a
> specific purpose at early boot, and shouldn't be used afterwards.
>

A convincing viewpoint. I agree.

>
> > > 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?
> >
>
> I don't understand this question.
>

I tried to guess your comment "Those routines are essentially position
dependent, just not in the conventional way." between lines. And now,
you have explained it completely. I have total understand it.

> > > 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.
> >
>
> Yes, but paddr() suggests that the result is the physical address. But
> this is only the case if paddr() itself runs from a 1:1 mapping of
> memory, otherwise it will be based on whatever other translation is
> being used.
>
> > > 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.
> >
>
> I cannot speak for other developers. I will note that this code is
> highly bespoke for the early execution context where the startup code
> lives. Good documentation is always better, but that doesn't mean this
> code could or should be used more widely.
>

OK, thanks. I will send some documents about it if there is anybody
puzzled by this code later.

Best regards,

Pingfan