[v4,01/11] x86/startup_64: Simplify global variable accesses in GDT/IDT programming

  From: Ard Biesheuvel <ardb@kernel.org>

There are two code paths in the startup code to program an IDT: one that
runs from the 1:1 mapping and one that runs from the virtual kernel
mapping. Currently, these are strictly separate because fixup_pointer()
is used on the 1:1 path, which will produce the wrong value when used
while executing from the virtual kernel mapping.

Switch to RIP_REL_REF() so that the two code paths can be merged. Also,
move the GDT and IDT descriptors to the stack so that they can be
referenced directly, rather than via RIP_REL_REF().

Rename startup_64_setup_env() to startup_64_setup_gdt_idt() while at it,
to make the call from assembler self-documenting.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/setup.h |  2 +-
 arch/x86/kernel/head64.c     | 56 +++++++-------------
 arch/x86/kernel/head_64.S    |  4 +-
 3 files changed, 22 insertions(+), 40 deletions(-)
  

On Tue, Feb 13, 2024 at 01:41:45PM +0100, Ard Biesheuvel wrote:
> @@ -632,5 +616,5 @@ void __head startup_64_setup_env(unsigned long physbase)
>  		     "movl %%eax, %%ss\n"
>  		     "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory");
>  
> -	startup_64_load_idt(physbase);
> +	startup_64_load_idt(&RIP_REL_REF(vc_no_ghcb));

It took me a while to figure out that even if we pass in one of the two
GHCB handler pointers, we only set it if CONFIG_AMD_MEM_ENCRYPT.

I think this ontop of yours is a bit more readable as it makes it
perfectly clear *when* the pointer is valid.

Yeah, if handler is set, we set it for the X86_TRAP_VC vector
unconditionally but that can be changed later, if really needed.

But this way it is clear by having the callers select the pointer. I.e.,
it is a more common coding pattern this way, I'd say.

Thx.

---

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 9d7f12829f2d..e39114c348e2 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -582,8 +582,8 @@ static void __head startup_64_load_idt(void *handler)
 	};
 	gate_desc *idt = (gate_desc *)desc.address;
 
-	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
-		/* VMM Communication Exception */
+	/* @handler is set only for a VMM Communication Exception */
+	if (handler)
 		set_bringup_idt_handler(idt, X86_TRAP_VC, handler);
 
 	native_load_idt(&desc);
@@ -592,10 +592,14 @@ static void __head startup_64_load_idt(void *handler)
 /* This is used when running on kernel addresses */
 void early_setup_idt(void)
 {
-	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
+	void *handler = NULL;
+
+	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
 		setup_ghcb();
+		handler = vc_boot_ghcb;
+	}
 
-	startup_64_load_idt(vc_boot_ghcb);
+	startup_64_load_idt(handler);
 }
 
 /*
@@ -603,6 +607,8 @@ void early_setup_idt(void)
  */
 void __head startup_64_setup_gdt_idt(void)
 {
+	void *handler = NULL;
+
 	struct desc_ptr startup_gdt_descr = {
 		.address	= (unsigned long)&RIP_REL_REF(startup_gdt),
 		.size		= sizeof(startup_gdt) - 1,
@@ -616,5 +622,8 @@ void __head startup_64_setup_gdt_idt(void)
 		     "movl %%eax, %%ss\n"
 		     "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory");
 
-	startup_64_load_idt(&RIP_REL_REF(vc_no_ghcb));
+	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
+		handler = &RIP_REL_REF(vc_no_ghcb);
+
+	startup_64_load_idt(handler);
 }
  

On Tue, 13 Feb 2024 at 21:06, Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Feb 13, 2024 at 01:41:45PM +0100, Ard Biesheuvel wrote:
> > @@ -632,5 +616,5 @@ void __head startup_64_setup_env(unsigned long physbase)
> >                    "movl %%eax, %%ss\n"
> >                    "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory");
> >
> > -     startup_64_load_idt(physbase);
> > +     startup_64_load_idt(&RIP_REL_REF(vc_no_ghcb));
>
> It took me a while to figure out that even if we pass in one of the two
> GHCB handler pointers, we only set it if CONFIG_AMD_MEM_ENCRYPT.
>
> I think this ontop of yours is a bit more readable as it makes it
> perfectly clear *when* the pointer is valid.
>

Looks fine to me.

> Yeah, if handler is set, we set it for the X86_TRAP_VC vector
> unconditionally but that can be changed later, if really needed.
>

We might call the parameter 'vc_handler' to make this clearer.
  

On Tue, 13 Feb 2024 at 22:53, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 13 Feb 2024 at 21:06, Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Tue, Feb 13, 2024 at 01:41:45PM +0100, Ard Biesheuvel wrote:
> > > @@ -632,5 +616,5 @@ void __head startup_64_setup_env(unsigned long physbase)
> > >                    "movl %%eax, %%ss\n"
> > >                    "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory");
> > >
> > > -     startup_64_load_idt(physbase);
> > > +     startup_64_load_idt(&RIP_REL_REF(vc_no_ghcb));
> >
> > It took me a while to figure out that even if we pass in one of the two
> > GHCB handler pointers, we only set it if CONFIG_AMD_MEM_ENCRYPT.
> >
> > I think this ontop of yours is a bit more readable as it makes it
> > perfectly clear *when* the pointer is valid.
> >
>
> Looks fine to me.
>
> > Yeah, if handler is set, we set it for the X86_TRAP_VC vector
> > unconditionally but that can be changed later, if really needed.
> >
>
> We might call the parameter 'vc_handler' to make this clearer.

Actually, we can merge set_bringup_idt_handler() into its caller as well:

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index aee99cfda4eb..804ba9a2214f 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -501,30 +501,22 @@ void __init __noreturn
x86_64_start_reservations(char *real_mode_data)
  */
 static gate_desc bringup_idt_table[NUM_EXCEPTION_VECTORS] __page_aligned_data;

-static void __head set_bringup_idt_handler(gate_desc *idt, int n,
void *handler)
-{
-#ifdef CONFIG_AMD_MEM_ENCRYPT
-       struct idt_data data;
-       gate_desc desc;
-
-       init_idt_data(&data, n, handler);
-       idt_init_desc(&desc, &data);
-       native_write_idt_entry(idt, n, &desc);
-#endif
-}
-
 /* This may run while still in the direct mapping */
-static void __head startup_64_load_idt(void *handler)
+static void __head startup_64_load_idt(void *vc_handler)
 {
        struct desc_ptr desc = {
                .address        = (unsigned
long)&RIP_REL_REF(bringup_idt_table),
                .size           = sizeof(bringup_idt_table) - 1,
        };
-       gate_desc *idt = (gate_desc *)desc.address;
+       struct idt_data data;
+       gate_desc idt_desc;

-       if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
-               /* VMM Communication Exception */
-               set_bringup_idt_handler(idt, X86_TRAP_VC, handler);
+       if (vc_handler) {
+               init_idt_data(&data, X86_TRAP_VC, vc_handler);
+               idt_init_desc(&idt_desc, &data);
+               native_write_idt_entry((gate_desc *)desc.address,
+                                      X86_TRAP_VC, &idt_desc);
+       }

        native_load_idt(&desc);
 }


(^^^ plus your changes boot tested on SEV-SNP)
  

On Wed, Feb 14, 2024 at 08:28:41AM +0100, Ard Biesheuvel wrote:
> Actually, we can merge set_bringup_idt_handler() into its caller as well:

Yap, here's the final version I have here and yes, it boots fine as
a SNP guest:

From: Ard Biesheuvel <ardb@kernel.org>
Date: Tue, 13 Feb 2024 13:41:45 +0100
Subject: [PATCH] x86/startup_64: Simplify global variable accesses in GDT/IDT
 programming

There are two code paths in the startup code to program an IDT: one that
runs from the 1:1 mapping and one that runs from the virtual kernel
mapping. Currently, these are strictly separate because fixup_pointer()
is used on the 1:1 path, which will produce the wrong value when used
while executing from the virtual kernel mapping.

Switch to RIP_REL_REF() so that the two code paths can be merged. Also,
move the GDT and IDT descriptors to the stack so that they can be
referenced directly, rather than via RIP_REL_REF().

Rename startup_64_setup_env() to startup_64_setup_gdt_idt() while at it,
to make the call from assembler self-documenting.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20240213124143.1484862-14-ardb+git@google.com
---
 arch/x86/include/asm/setup.h |  2 +-
 arch/x86/kernel/head64.c     | 75 +++++++++++++++---------------------
 arch/x86/kernel/head_64.S    |  4 +-
 3 files changed, 32 insertions(+), 49 deletions(-)

diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
index 5c83729c8e71..e61e68d71cba 100644
--- a/arch/x86/include/asm/setup.h
+++ b/arch/x86/include/asm/setup.h
@@ -48,7 +48,7 @@ extern unsigned long saved_video_mode;
 extern void reserve_standard_io_resources(void);
 extern void i386_reserve_resources(void);
 extern unsigned long __startup_64(unsigned long physaddr, struct boot_params *bp);
-extern void startup_64_setup_env(unsigned long physbase);
+extern void startup_64_setup_gdt_idt(void);
 extern void early_setup_idt(void);
 extern void __init do_early_exception(struct pt_regs *regs, int trapnr);
 
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index dc0956067944..cdff748bf5cb 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -22,6 +22,7 @@
 #include <linux/cc_platform.h>
 #include <linux/pgtable.h>
 
+#include <asm/asm.h>
 #include <asm/processor.h>
 #include <asm/proto.h>
 #include <asm/smp.h>
@@ -76,15 +77,6 @@ static struct desc_struct startup_gdt[GDT_ENTRIES] __initdata = {
 	[GDT_ENTRY_KERNEL_DS]           = GDT_ENTRY_INIT(DESC_DATA64, 0, 0xfffff),
 };
 
-/*
- * Address needs to be set at runtime because it references the startup_gdt
- * while the kernel still uses a direct mapping.
- */
-static struct desc_ptr startup_gdt_descr __initdata = {
-	.size = sizeof(startup_gdt)-1,
-	.address = 0,
-};
-
 static void __head *fixup_pointer(void *ptr, unsigned long physaddr)
 {
 	return ptr - (void *)_text + (void *)physaddr;
@@ -569,62 +561,52 @@ void __init __noreturn x86_64_start_reservations(char *real_mode_data)
  */
 static gate_desc bringup_idt_table[NUM_EXCEPTION_VECTORS] __page_aligned_data;
 
-static struct desc_ptr bringup_idt_descr = {
-	.size		= (NUM_EXCEPTION_VECTORS * sizeof(gate_desc)) - 1,
-	.address	= 0, /* Set at runtime */
-};
-
-static void set_bringup_idt_handler(gate_desc *idt, int n, void *handler)
+/* This may run while still in the direct mapping */
+static void __head startup_64_load_idt(void *vc_handler)
 {
-#ifdef CONFIG_AMD_MEM_ENCRYPT
+	struct desc_ptr desc = {
+		.address = (unsigned long)&RIP_REL_REF(bringup_idt_table),
+		.size	 = sizeof(bringup_idt_table) - 1,
+	};
 	struct idt_data data;
-	gate_desc desc;
-
-	init_idt_data(&data, n, handler);
-	idt_init_desc(&desc, &data);
-	native_write_idt_entry(idt, n, &desc);
-#endif
-}
+	gate_desc idt_desc;
 
-/* This runs while still in the direct mapping */
-static void __head startup_64_load_idt(unsigned long physbase)
-{
-	struct desc_ptr *desc = fixup_pointer(&bringup_idt_descr, physbase);
-	gate_desc *idt = fixup_pointer(bringup_idt_table, physbase);
-
-
-	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
-		void *handler;
-
-		/* VMM Communication Exception */
-		handler = fixup_pointer(vc_no_ghcb, physbase);
-		set_bringup_idt_handler(idt, X86_TRAP_VC, handler);
+	/* @vc_handler is set only for a VMM Communication Exception */
+	if (vc_handler) {
+		init_idt_data(&data, X86_TRAP_VC, vc_handler);
+		idt_init_desc(&idt_desc, &data);
+		native_write_idt_entry((gate_desc *)desc.address, X86_TRAP_VC, &idt_desc);
 	}
 
-	desc->address = (unsigned long)idt;
-	native_load_idt(desc);
+	native_load_idt(&desc);
 }
 
 /* This is used when running on kernel addresses */
 void early_setup_idt(void)
 {
-	/* VMM Communication Exception */
+	void *handler = NULL;
+
 	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
 		setup_ghcb();
-		set_bringup_idt_handler(bringup_idt_table, X86_TRAP_VC, vc_boot_ghcb);
+		handler = vc_boot_ghcb;
 	}
 
-	bringup_idt_descr.address = (unsigned long)bringup_idt_table;
-	native_load_idt(&bringup_idt_descr);
+	startup_64_load_idt(handler);
 }
 
 /*
  * Setup boot CPU state needed before kernel switches to virtual addresses.
  */
-void __head startup_64_setup_env(unsigned long physbase)
+void __head startup_64_setup_gdt_idt(void)
 {
+	void *handler = NULL;
+
+	struct desc_ptr startup_gdt_descr = {
+		.address	= (unsigned long)&RIP_REL_REF(startup_gdt),
+		.size		= sizeof(startup_gdt) - 1,
+	};
+
 	/* Load GDT */
-	startup_gdt_descr.address = (unsigned long)fixup_pointer(startup_gdt, physbase);
 	native_load_gdt(&startup_gdt_descr);
 
 	/* New GDT is live - reload data segment registers */
@@ -632,5 +614,8 @@ void __head startup_64_setup_env(unsigned long physbase)
 		     "movl %%eax, %%ss\n"
 		     "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory");
 
-	startup_64_load_idt(physbase);
+	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
+		handler = &RIP_REL_REF(vc_no_ghcb);
+
+	startup_64_load_idt(handler);
 }
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index a8eaecbd5c81..bcbebab2cc03 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -68,8 +68,6 @@ SYM_CODE_START_NOALIGN(startup_64)
 	/* Set up the stack for verify_cpu() */
 	leaq	(__end_init_task - PTREGS_SIZE)(%rip), %rsp
 
-	leaq	_text(%rip), %rdi
-
 	/* Setup GSBASE to allow stack canary access for C code */
 	movl	$MSR_GS_BASE, %ecx
 	leaq	INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
@@ -77,7 +75,7 @@ SYM_CODE_START_NOALIGN(startup_64)
 	shrq	$32,  %rdx
 	wrmsr
 
-	call	startup_64_setup_env
+	call	startup_64_setup_gdt_idt
 
 	/* Now switch to __KERNEL_CS so IRET works reliably */
 	pushq	$__KERNEL_CS