Re: [PATCH v5 10/16] x86/startup_64: Simplify virtual switch on primary boot

[Ard Biesheuvel <ardb@kernel.org>]

On Wed, 21 Feb 2024 at 12:36, Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> The secondary startup code is used on the primary boot path as well, but
> in this case, the initial part runs from a 1:1 mapping, until an
> explicit cross-jump is made to the kernel virtual mapping of the same
> code.
>
> On the secondary boot path, this jump is pointless as the code already
> executes from the mapping targeted by the jump. So combine this
> cross-jump with the jump from startup_64() into the common boot path.
> This simplifies the execution flow, and clearly separates code that runs
> from a 1:1 mapping from code that runs from the kernel virtual mapping.
>
> Note that this requires a page table switch, so hoist the CR3 assignment
> into startup_64() as well. And since absolute symbol references will no
> longer be permitted in .head.text once we enable the associated build
> time checks, a RIP-relative memory operand is used in the JMP
> instruction, referring to an absolute constant in the .init.rodata
> section.
>
> Given that the secondary startup code does not require a special
> placement inside the executable, move it to the .noinstr.text section.

This should be the .text section, or we get

vmlinux.o: warning: objtool: early_setup_idt+0x4: call to
startup_64_load_idt() leaves .noinstr.text section

It would be better to have the secondary startup code in
.noinstr.text, but it is only a very minor improvement. I'll be
looking into teaching objtool to be strict about .head.text code and
reject references that use absolute addressing, so I might be able to
tweak it to permit this use case as well but at this point we should
probably just move it into ordinary .text


> This requires the use of a subsection so that the payload is placed
> after the page aligned Xen hypercall page, as otherwise, objtool will
> complain about the resulting JMP instruction emitted by the assembler
> being unreachable.
>

^^^ this bit can be dropped, and the following hunk applied (apologies
if my gmail mangles this - i can resend the patch or the series)


diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 4bee33d8e1dc..e16df01791be 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -515,7 +515,7 @@
 }

 /* This is used when running on kernel addresses */
-void noinstr early_setup_idt(void)
+void early_setup_idt(void)
 {
        void *handler = NULL;

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 32fefb23f4df..c9ee92550508 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -139,8 +139,7 @@
        __INITRODATA
 0:     .quad   common_startup_64

-       .section .noinstr.text, "ax"
-       .subsection 1
+       .text
 SYM_CODE_START(secondary_startup_64)
        UNWIND_HINT_END_OF_STACK
        ANNOTATE_NOENDBR