Re: [PATCH v2 1/2] x86/boot: Move early kernel mapping code into startup/

[Ingo Molnar <mingo@kernel.org>]

* Ard Biesheuvel <ardb@kernel.org> wrote:

> On Mon, 7 Apr 2025 at 19:44, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Ard Biesheuvel <ardb+git@google.com> wrote:
> >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > The startup code that constructs the kernel virtual mapping runs from
> > > the 1:1 mapping of memory itself, and therefore, cannot use absolute
> > > symbol references. Move this code into a separate source file under
> > > arch/x86/boot/startup/ where all such code will be kept from now on.
> > >
> > > Since all code here is constructed in a manner that ensures that it
> > > tolerates running from the 1:1 mapping of memory, any uses of the
> > > RIP_REL_REF() macro can be dropped, along with __head annotations for
> > > placing this code in a dedicated startup section.
> >
> > So would it be possible to do this in ~3 steps: first the mechanic
> > movement of code, with very few changes (if the result builds & boots),
> > then drop the RIP_REL_REF() uses and __head annotations in two separate
> > patches?
> >
> > Bisectability, ease of review, etc.
> >
> > (The tiny bird gets the worm, but I might have butchered that proverb.)
> >
> 
> Yes.
> 
> And actually, the Clang boot regression that was reported indicates 
> that this statement it not 100% true to begin with. While it is no 
> longer necessary to use RIP_REL_REF() for accesses to global 
> variables, it may still be needed when explicitly taking the address 
> of a global variable and storing it in a stack allocated struct, 
> e.g.,
> 
> void __init startup_64_setup_gdt_idt(void)
> {
>         void *handler = NULL;
> 
>         struct desc_ptr startup_gdt_descr = {
>                 .address = (__force unsigned long)gdt_page.gdt,
>                 .size    = GDT_SIZE - 1,
>         };
> 
> In this case, even -fPIC may produce an absolute reference to
> gdt_page.gdt, but from .rodata not from .text, and this is equally
> broken at early boot.

OK.

> Once all this code has been moved into place, I'll propose the
> validation (similar to arm64 and EFI stub) which just greps the output
> of readelf -r and checks for occurrences of R_X86_64_64; that way, we
> will detect early and precisely whether the codegen is ok.

Yeah, that sounds good!

> Please let me know which of these patches you are intending to keep 
> in tip/x86/boot, and I will respin on top of that.

I'd like to merge all of them as long as they don't intentionally regress.
All of this seemed like a step forward, and having them in one place
will enable a new type of debugging check - which is a win too in my book.

So no fundamental worries from me, just the request to have more 
careful iterations.

Thanks,

	Ingo