diff mbox series

[v4,04/11] x86/startup_64: Defer assignment of 5-level paging global variables

Message ID 20240213124143.1484862-17-ardb+git@google.com
State New
Headers
Series x86: Confine early 1:1 mapped startup code |

Commit Message

Ard Biesheuvel Feb. 13, 2024, 12:41 p.m. UTC 
  From: Ard Biesheuvel <ardb@kernel.org>

Assigning the 5-level paging related global variables from the earliest
C code using explicit references that use the 1:1 translation of memory
is unnecessary, as the startup code itself does not rely on them to
create the initial page tables, and this is all it should be doing. So
defer these assignments to the primary C entry code that executes via
the ordinary kernel virtual mapping.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/kernel/head64.c | 44 +++++++-------------
 1 file changed, 14 insertions(+), 30 deletions(-)

Comments

Borislav Petkov Feb. 20, 2024, 6:45 p.m. UTC | #1 
On Tue, Feb 13, 2024 at 01:41:48PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Assigning the 5-level paging related global variables from the earliest
> C code using explicit references that use the 1:1 translation of memory
> is unnecessary, as the startup code itself does not rely on them to
> create the initial page tables, and this is all it should be doing. So
> defer these assignments to the primary C entry code that executes via
> the ordinary kernel virtual mapping.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/kernel/head64.c | 44 +++++++-------------
>  1 file changed, 14 insertions(+), 30 deletions(-)

Whoops:

arch/x86/kernel/head64.c: In function ‘x86_64_start_kernel’:
arch/x86/kernel/head64.c:442:17: error: ‘__pgtable_l5_enabled’ undeclared (first use in this function); did you mean ‘pgtable_l5_enabled’?
  442 |                 __pgtable_l5_enabled    = 1;
      |                 ^~~~~~~~~~~~~~~~~~~~
      |                 pgtable_l5_enabled
arch/x86/kernel/head64.c:442:17: note: each undeclared identifier is reported only once for each function it appears in
make[4]: *** [scripts/Makefile.build:243: arch/x86/kernel/head64.o] Error 1
make[3]: *** [scripts/Makefile.build:481: arch/x86/kernel] Error 2
make[2]: *** [scripts/Makefile.build:481: arch/x86] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/mnt/kernel/kernel/2nd/linux/Makefile:1921: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
Ard Biesheuvel Feb. 20, 2024, 11:33 p.m. UTC | #2 
On Tue, 20 Feb 2024 at 19:45, Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Feb 13, 2024 at 01:41:48PM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Assigning the 5-level paging related global variables from the earliest
> > C code using explicit references that use the 1:1 translation of memory
> > is unnecessary, as the startup code itself does not rely on them to
> > create the initial page tables, and this is all it should be doing. So
> > defer these assignments to the primary C entry code that executes via
> > the ordinary kernel virtual mapping.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/kernel/head64.c | 44 +++++++-------------
> >  1 file changed, 14 insertions(+), 30 deletions(-)
>
> Whoops:
>

Right, this is the same issue as in #11 - in both cases, the extern
declaration of __pgtable_l5_enabled needs to be visible regardless of
CONFIG_X86_5LEVEL.

I'll fix both cases for v5.

> arch/x86/kernel/head64.c: In function ‘x86_64_start_kernel’:
> arch/x86/kernel/head64.c:442:17: error: ‘__pgtable_l5_enabled’ undeclared (first use in this function); did you mean ‘pgtable_l5_enabled’?
>   442 |                 __pgtable_l5_enabled    = 1;
>       |                 ^~~~~~~~~~~~~~~~~~~~
>       |                 pgtable_l5_enabled
> arch/x86/kernel/head64.c:442:17: note: each undeclared identifier is reported only once for each function it appears in
> make[4]: *** [scripts/Makefile.build:243: arch/x86/kernel/head64.o] Error 1
> make[3]: *** [scripts/Makefile.build:481: arch/x86/kernel] Error 2
> make[2]: *** [scripts/Makefile.build:481: arch/x86] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/mnt/kernel/kernel/2nd/linux/Makefile:1921: .] Error 2
> make: *** [Makefile:240: __sub-make] Error 2
>
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov Feb. 21, 2024, 10:09 a.m. UTC | #3 
On Wed, Feb 21, 2024 at 12:33:08AM +0100, Ard Biesheuvel wrote:
> Right, this is the same issue as in #11 - in both cases, the extern
> declaration of __pgtable_l5_enabled needs to be visible regardless of
> CONFIG_X86_5LEVEL.

Yap, I don't mind something like below.

5LEVEL will be practically enabled everywhere.

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 158da0fd01d2..eeb1744215f2 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -52,13 +52,11 @@ extern pmd_t early_dynamic_pgts[EARLY_DYNAMIC_PAGE_TABLES][PTRS_PER_PMD];
 static unsigned int __initdata next_early_pgt;
 pmdval_t early_pmd_flags = __PAGE_KERNEL_LARGE & ~(_PAGE_GLOBAL | _PAGE_NX);
 
-#ifdef CONFIG_X86_5LEVEL
 unsigned int __pgtable_l5_enabled __ro_after_init;
 unsigned int pgdir_shift __ro_after_init = 39;
 EXPORT_SYMBOL(pgdir_shift);
 unsigned int ptrs_per_p4d __ro_after_init = 1;
 EXPORT_SYMBOL(ptrs_per_p4d);
-#endif
 
 #ifdef CONFIG_DYNAMIC_MEMORY_LAYOUT
 unsigned long page_offset_base __ro_after_init = __PAGE_OFFSET_BASE_L4;
Ard Biesheuvel Feb. 21, 2024, 10:20 a.m. UTC | #4 
On Wed, 21 Feb 2024 at 11:09, Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Feb 21, 2024 at 12:33:08AM +0100, Ard Biesheuvel wrote:
> > Right, this is the same issue as in #11 - in both cases, the extern
> > declaration of __pgtable_l5_enabled needs to be visible regardless of
> > CONFIG_X86_5LEVEL.
>
> Yap, I don't mind something like below.
>
> 5LEVEL will be practically enabled everywhere.
>
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index 158da0fd01d2..eeb1744215f2 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -52,13 +52,11 @@ extern pmd_t early_dynamic_pgts[EARLY_DYNAMIC_PAGE_TABLES][PTRS_PER_PMD];
>  static unsigned int __initdata next_early_pgt;
>  pmdval_t early_pmd_flags = __PAGE_KERNEL_LARGE & ~(_PAGE_GLOBAL | _PAGE_NX);
>
> -#ifdef CONFIG_X86_5LEVEL
>  unsigned int __pgtable_l5_enabled __ro_after_init;
>  unsigned int pgdir_shift __ro_after_init = 39;
>  EXPORT_SYMBOL(pgdir_shift);
>  unsigned int ptrs_per_p4d __ro_after_init = 1;
>  EXPORT_SYMBOL(ptrs_per_p4d);
> -#endif
>

Just the below should be sufficient

--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -22,7 +22,7 @@ typedef struct { pteval_t pte; } pte_t;
 typedef struct { pmdval_t pmd; } pmd_t;

-#ifdef CONFIG_X86_5LEVEL
 extern unsigned int __pgtable_l5_enabled;

+#ifdef CONFIG_X86_5LEVEL
 #ifdef USE_EARLY_PGTABLE_L5
 /*
Borislav Petkov Feb. 21, 2024, 11:12 a.m. UTC | #5 
On Wed, Feb 21, 2024 at 11:20:13AM +0100, Ard Biesheuvel wrote:
> Just the below should be sufficient
> 
> --- a/arch/x86/include/asm/pgtable_64_types.h
> +++ b/arch/x86/include/asm/pgtable_64_types.h
> @@ -22,7 +22,7 @@ typedef struct { pteval_t pte; } pte_t;
>  typedef struct { pmdval_t pmd; } pmd_t;
> 
> -#ifdef CONFIG_X86_5LEVEL
>  extern unsigned int __pgtable_l5_enabled;
> 
> +#ifdef CONFIG_X86_5LEVEL
>  #ifdef USE_EARLY_PGTABLE_L5

Perhaps but the CONFIG_X86_5LEVEL ifdeffery is just ugly and getting
unnecessary.
Ard Biesheuvel Feb. 21, 2024, 11:21 a.m. UTC | #6 
On Wed, 21 Feb 2024 at 12:13, Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Feb 21, 2024 at 11:20:13AM +0100, Ard Biesheuvel wrote:
> > Just the below should be sufficient
> >
> > --- a/arch/x86/include/asm/pgtable_64_types.h
> > +++ b/arch/x86/include/asm/pgtable_64_types.h
> > @@ -22,7 +22,7 @@ typedef struct { pteval_t pte; } pte_t;
> >  typedef struct { pmdval_t pmd; } pmd_t;
> >
> > -#ifdef CONFIG_X86_5LEVEL
> >  extern unsigned int __pgtable_l5_enabled;
> >
> > +#ifdef CONFIG_X86_5LEVEL
> >  #ifdef USE_EARLY_PGTABLE_L5
>
> Perhaps but the CONFIG_X86_5LEVEL ifdeffery is just ugly and getting
> unnecessary.
>

That all gets ripped out in the last patch.


Btw v5 is good to go, in case you're ok switching to that:

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=x86-pie-for-sev-v5
Borislav Petkov Feb. 21, 2024, 11:23 a.m. UTC | #7 
On Wed, Feb 21, 2024 at 12:21:20PM +0100, Ard Biesheuvel wrote:
> Btw v5 is good to go, in case you're ok switching to that:

Sure, send it on.

Thx.
diff mbox series

Patch

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 4b08e321d168..4bcbd4ae2dc6 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -23,6 +23,7 @@ 
 #include <linux/pgtable.h>
 
 #include <asm/asm.h>
+#include <asm/page_64.h>
 #include <asm/processor.h>
 #include <asm/proto.h>
 #include <asm/smp.h>
@@ -77,24 +78,11 @@  static struct desc_struct startup_gdt[GDT_ENTRIES] __initdata = {
 	[GDT_ENTRY_KERNEL_DS]           = GDT_ENTRY_INIT(DESC_DATA64, 0, 0xfffff),
 };
 
-#ifdef CONFIG_X86_5LEVEL
-static void __head *fixup_pointer(void *ptr, unsigned long physaddr)
-{
-	return ptr - (void *)_text + (void *)physaddr;
-}
-
-static unsigned long __head *fixup_long(void *ptr, unsigned long physaddr)
+static inline bool check_la57_support(void)
 {
-	return fixup_pointer(ptr, physaddr);
-}
-
-static unsigned int __head *fixup_int(void *ptr, unsigned long physaddr)
-{
-	return fixup_pointer(ptr, physaddr);
-}
+	if (!IS_ENABLED(CONFIG_X86_5LEVEL))
+		return false;
 
-static bool __head check_la57_support(unsigned long physaddr)
-{
 	/*
 	 * 5-level paging is detected and enabled at kernel decompression
 	 * stage. Only check if it has been enabled there.
@@ -102,21 +90,8 @@  static bool __head check_la57_support(unsigned long physaddr)
 	if (!(native_read_cr4() & X86_CR4_LA57))
 		return false;
 
-	*fixup_int(&__pgtable_l5_enabled, physaddr) = 1;
-	*fixup_int(&pgdir_shift, physaddr) = 48;
-	*fixup_int(&ptrs_per_p4d, physaddr) = 512;
-	*fixup_long(&page_offset_base, physaddr) = __PAGE_OFFSET_BASE_L5;
-	*fixup_long(&vmalloc_base, physaddr) = __VMALLOC_BASE_L5;
-	*fixup_long(&vmemmap_base, physaddr) = __VMEMMAP_BASE_L5;
-
 	return true;
 }
-#else
-static bool __head check_la57_support(unsigned long physaddr)
-{
-	return false;
-}
-#endif
 
 static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdval_t *pmd)
 {
@@ -180,7 +155,7 @@  unsigned long __head __startup_64(unsigned long physaddr,
 	bool la57;
 	int i;
 
-	la57 = check_la57_support(physaddr);
+	la57 = check_la57_support();
 
 	/* Is the address too large? */
 	if (physaddr >> MAX_PHYSMEM_BITS)
@@ -463,6 +438,15 @@  asmlinkage __visible void __init __noreturn x86_64_start_kernel(char * real_mode
 				(__START_KERNEL & PGDIR_MASK)));
 	BUILD_BUG_ON(__fix_to_virt(__end_of_fixed_addresses) <= MODULES_END);
 
+	if (check_la57_support()) {
+		__pgtable_l5_enabled	= 1;
+		pgdir_shift		= 48;
+		ptrs_per_p4d		= 512;
+		page_offset_base	= __PAGE_OFFSET_BASE_L5;
+		vmalloc_base		= __VMALLOC_BASE_L5;
+		vmemmap_base		= __VMEMMAP_BASE_L5;
+	}
+
 	cr4_init_shadow();
 
 	/* Kill off the identity-map trampoline */