x86: Confine early 1:1 mapped startup code

Review

Quoted 3 lines · Ard Biesheuvel

(quoted diff) +3 -0

+     /*
+      * Create a mask of CR4 bits to preserve. Omit PGE in order to clean
+      * global 1:1 translations from the TLBs.

Brian raised this question when exactly global entries get flushed and I was looking for the exact definition in the SDM, here's what I'll do ontop:

arch/x86/kernel/head_64.S +6 -2

index 2d8762887c6a..24df91535062 100644
@@ -186,8 +186,13 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 1:
 
        /*
-        * Create a mask of CR4 bits to preserve. Omit PGE in order to clean
+        * Create a mask of CR4 bits to preserve. Omit PGE in order to flush
         * global 1:1 translations from the TLBs.
+        *
+        * From the SDM:
+        * "If CR4.PGE is changing from 0 to 1, there were no global TLB
+        *  entries before the execution; if CR4.PGE is changing from 1 to 0,
+        *  there will be no global TLB entries after the execution."
         */
        movl    $(X86_CR4_PAE | X86_CR4_LA57), %edx
 #ifdef CONFIG_X86_MCE
---

And how it is perfectly clear.

From: Ard Biesheuvel <[email protected]>

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 <[email protected]>

---
 arch/x86/include/asm/pgtable_64_types.h |  2 +-
 arch/x86/kernel/head64.c                | 44 +++++++-------------
 2 files changed, 15 insertions(+), 31 deletions(-)

Quoted 9 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

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.

Quoted 5 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/include/asm/pgtable_64_types.h |  2 +-
 arch/x86/kernel/head64.c                | 44 +++++++-------------
 2 files changed, 15 insertions(+), 31 deletions(-)

Reviewed-by Borislav Petkov (AMD) <[email protected]>

Those should probably be tested on a 5level machine, just in case.

The scenario that I have not managed to test is entering from EFI with 5 levels of paging enabled, and switching back to 4 levels (which should work regardless of CONFIG_X86_5LEVEL). However, no firmware in existence actually supports that today, and I am pretty sure that this code has never been tested under those conditions to begin with. (OVMF patches are under review atm to allow 5-level paging to be enabled in the firmware)

I currently don't have access to real hardware with LA57 support so any additional coverage there is highly appreciated (same for the last patch in this series)

Quoted 7 lines · Ard Biesheuvel

The scenario that I have not managed to test is entering from EFI with 5 levels of paging enabled, and switching back to 4 levels (which should work regardless of CONFIG_X86_5LEVEL). However, no firmware in existence actually supports that today, and I am pretty sure that this code has never been tested under those conditions to begin with. (OVMF patches are under review atm to allow 5-level paging to be enabled in the firmware)

Aha.

Quoted 10 lines · Borislav Petkov

Quoted 7 lines · Ard Biesheuvel

The scenario that I have not managed to test is entering from EFI with 5 levels of paging enabled, and switching back to 4 levels (which should work regardless of CONFIG_X86_5LEVEL). However, no firmware in existence actually supports that today, and I am pretty sure that this code has never been tested under those conditions to begin with. (OVMF patches are under review atm to allow 5-level paging to be enabled in the firmware)

Quoted 2 lines

Aha.

I've built a debug OVMF image using the latest version of the series, and put it at [0]

Run like this

qemu-system-x86_64 -M q35 \
-cpu qemu64,+la57 -smp 4 \
-bios OVMF-5level.fd \
-kernel arch/x86/boot/bzImage \
-append console=ttyS0\ earlyprintk=ttyS0 \
-vga none -nographic -m 1g \
-initrd <initrd.img>

and you will get loads of DEBUG output from the firmware first, and then boot into Linux. (initrd can be omitted)

Right before entering, it will print

CpuDxe: 5-Level Paging = 1

which confirms that the firmware is running with 5 levels of paging.

I've confirmed that this boots happily with this series applied, including when using 'no5lvl' on the command line, or when disabling CONFIG_X86_5LEVEL [confirmed by inspecting /sys/kernel/debug/page_tables/kernel].

[0] http://files.workofard.com/OVMF-5level.fd.gz

Quoted 42 lines · Ard Biesheuvel

Quoted 10 lines · Borislav Petkov

Quoted 7 lines · Ard Biesheuvel

The scenario that I have not managed to test is entering from EFI with 5 levels of paging enabled, and switching back to 4 levels (which should work regardless of CONFIG_X86_5LEVEL). However, no firmware in existence actually supports that today, and I am pretty sure that this code has never been tested under those conditions to begin with. (OVMF patches are under review atm to allow 5-level paging to be enabled in the firmware)

Quoted 2 lines

Aha.

I've built a debug OVMF image using the latest version of the series, and put it at [0]

Run like this

qemu-system-x86_64 -M q35 \
   -cpu qemu64,+la57 -smp 4 \
   -bios OVMF-5level.fd \
   -kernel arch/x86/boot/bzImage \
   -append console=ttyS0\ earlyprintk=ttyS0 \
   -vga none -nographic -m 1g \
   -initrd <initrd.img>

and you will get loads of DEBUG output from the firmware first, and then boot into Linux. (initrd can be omitted)

Right before entering, it will print

CpuDxe: 5-Level Paging = 1

which confirms that the firmware is running with 5 levels of paging.

I've confirmed that this boots happily with this series applied, including when using 'no5lvl' on the command line, or when disabling CONFIG_X86_5LEVEL [confirmed by inspecting /sys/kernel/debug/page_tables/kernel].

Quoted 1 line

[0] http://files.workofard.com/OVMF-5level.fd.gz

Nice, that might come in handy for other testing too.

Thx.

Quoted 28 lines

I've built a debug OVMF image using the latest version of the series, and put it at [0]

Run like this

qemu-system-x86_64 -M q35 \
-cpu qemu64,+la57 -smp 4 \
-bios OVMF-5level.fd \
-kernel arch/x86/boot/bzImage \
-append console=ttyS0\ earlyprintk=ttyS0 \
-vga none -nographic -m 1g \
-initrd <initrd.img>

and you will get loads of DEBUG output from the firmware first, and then boot into Linux. (initrd can be omitted)

Right before entering, it will print

CpuDxe: 5-Level Paging = 1

which confirms that the firmware is running with 5 levels of paging.

I've confirmed that this boots happily with this series applied, including when using 'no5lvl' on the command line, or when disabling CONFIG_X86_5LEVEL [confirmed by inspecting /sys/kernel/debug/page_tables/kernel].

Quoted 1 line

[0] http://files.workofard.com/OVMF-5level.fd.gz

Nice, that might come in handy for other testing too.

Btw, on a semi-related note, do you have an idea whether a normal guest kernel using OVMF istead of seabios would be even able to boot a kernel supplied with -kernel like above but without an -initrd?

I have everything builtin and the same kernel boots fine in a guest with a

[    0.000000] SMBIOS 3.0.0 present.
[    0.000000] DMI: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014

but if I try to boot the respective guest installed with the OVMF BIOS from the debian package:

[    0.000000] efi: EFI v2.7 by Debian distribution of EDK II
[    0.000000] efi: SMBIOS=0x7f788000 SMBIOS 3.0=0x7f786000 ACPI=0x7f97e000 ACPI 2.0=0x7f97e014 MEMATTR=0x7ddfe018

it fails looking up the /dev/root device major/minor deep in the bowels of the vfs:

[    2.565651] do_new_mount:
[    2.566380] vfs_get_tree: fc->root: 0000000000000000
[    2.567298] kern_path: filename: ffff88800d666000 of name: /dev/root
[    2.568418] kern_path: ret: 0
[    2.569009] lookup_bdev: kern_path(/dev/root, , path: ffff88800e537380), error: 0
[    2.571645] lookup_bdev: inode->i_rdev: 0x0
[    2.572417] get_tree_bdev: lookup_bdev(/dev/root, dev: 0x0), error: 0
                                                     ^^^^^^^^^

That dev_t should be 0x800002 - the major and minor of /dev/sda2 but it looks like something else is missing in this case...

How are you passing the root device to the kernel? Via root= on the command line?

and this is OVMF.

I have tried both of these, with i440fx as well as q35, and they all work happily with my Debian guest image passed via -hda to QEMU, and with root=/dev/sda2 on the kernel command line.

How did you get this output? Are these debug printk()s you added yourself?

From: Ard Biesheuvel <[email protected]>

Determining the address of the initial page table to program into CR3 involves:
- taking the physical address
- adding the SME encryption mask

On the primary entry path, the code is mapped using a 1:1 virtual to physical translation, so the physical address can be taken directly using a RIP-relative LEA instruction.

On the secondary entry path, the address can be obtained by taking the offset from the virtual kernel base (__START_kernel_map) and adding the physical kernel base.

This is implemented in a slightly confusing way, so clean this up.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/kernel/head_64.S | 25 ++++++--------------
 1 file changed, 7 insertions(+), 18 deletions(-)

First

Reviewed-by Borislav Petkov (AMD) <[email protected]>

for the patch.

arch/x86/kernel/head_64.S SYM_CODE_START_NOALIGN(startup_64) · line 130

Quoted 3 lines

arch/x86/kernel/head_64.S +3 -0

+     /*
+      * Switch to early_top_pgt which still has the identity mappings
+      * present.

I was wondering why we've had this "discrepancy" forever - the boot CPU would have early_top_pgt *with* the ident mappings while the APs would do init_top_pgt.

But we end up loading init_top_pgt on the BSP too in init_mem_mapping() so there's a short time during boot where we have this difference. I haven't found a reason to have it yet except "why bother"...

And now some details just for future reference:

On the BSP:

=> 0x10000a0: mov %rax,%cr3

cr3 0x9922000
111850: ffffffff89922000 8192 OBJECT GLOBAL DEFAULT 22 early_top_pgt

(gdb) p/x early_top_pgt
$3 = {{pgd = 0x9924063}, {pgd = 0x9924063}, {pgd = 0x0} <repeats 509 times>, {pgd = 0x2418067}}

first two PGDs and the last one are populated.

On the AP:

cr3 0x2416000
104747: ffffffff82416000 8192 OBJECT GLOBAL DEFAULT 12 init_top_pgt

(gdb) p/x (long[512])*0xffffffff82416000
$8 = {0x0 <repeats 273 times>, 0xbe01067, 0x0 <repeats 128 times>, 0xc000067, 0xc001067, 0xc002067, 0xc003067, 0xc004067, 0xc005067,
0xc006067, 0xc007067, 0xc008067, 0xc009067, 0xc00a067, 0xc00b067, 0xc00c067, 0xc00d067, 0xc00e067, 0xc00f067, 0xc010067, 0xc011067, 0xc012067, 0xc013067, 0xc014067, 0xc015067, 0xc016067, 0xc017067, 0xc018067, 0xc019067, 0xc01a067, 0xc01b067, 0xc01c067, 0xc01d067, 0xc01e067, 0xc01f067, 0xc020067, 0xc021067, 0xc022067, 0xc023067, 0xc024067, 0xc025067, 0xc026067, 0xc027067, 0xc028067, 0xc029067, 0xc02a067, 0xc02b067, 0xc02c067, 0xc02d067, 0xc02e067, 0xc02f067, 0xc030067, 0xc031067, 0xc032067, 0xc033067, 0xc034067, 0xc035067, 0xc036067, 0xc037067, 0xc038067, 0xc039067, 0xc03a067, 0xc03b067, 0xc03c067, 0xc03d067, 0xc03e067, 0xc03f067, 0x0, 0x0, 0x7ffd3067, 0x0 <repeats 39 times>, 0x7ffd1067, 0x0, 0x9b11067, 0x2418067}

and that one becomes the swapper_pg_dir which is the kernel pagetable we use.

PTI then does two separate ones, which is a whole different topic.

:-)

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Because we enter with a 1:1 mapping, and so we can only switch to another set of page tables that also includes this 1:1 mapping. Once we are running from the kernel mapping, we can drop the 1:1 mapping but we still need it.

What we could do for robustness is reduce this 1:1 mapping to text + rodata, and make it read-only, but I'm not sure it's worth the churn.

From: Ard Biesheuvel <[email protected]>

Parse the mem_encrypt= command line parameter from the EFI stub if CONFIG_ARCH_HAS_MEM_ENCRYPT=y, so that it can be passed to the early boot code by the arch code in the stub.

This avoids the need for the core kernel to do any string parsing very early in the boot.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 8 ++++++++
 drivers/firmware/efi/libstub/efistub.h         | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

arch/x86/include/uapi/asm/bootparam.h line 41

Quoted 9 lines · Ard Biesheuvel

arch/x86/include/uapi/asm/bootparam.h +1 -0

index 01d19fc22346..eeea058cf602 100644
@@ -38,6 +38,7 @@
 #define XLF_EFI_KEXEC                        (1<<4)
 #define XLF_5LEVEL                   (1<<5)
 #define XLF_5LEVEL_ENABLED           (1<<6)
+#define XLF_MEM_ENCRYPTION           (1<<7)

Needs documenting in Documentation/arch/x86/boot.rst.

And yes, those 5LEVEL things are not documented either but I'm even questioning the justification for their existence. We'll see...

Yeah that seems unnecessary to me. They are only used by kexec, and only for cases where you want to kexec a [much] older kernel that cannot deal with 5-level paging at all. AFAICT 5-level support was added in v4.13.

So I think we might be able to drop these entirely, no?

arch/x86/include/asm/pgtable_64_types.h typedef unsigned long pgprotval_t; · line 43

Quoted 9 lines · Ard Biesheuvel

arch/x86/include/asm/pgtable_64_types.h +9 -0

+     asm(ALTERNATIVE_TERNARY(
+             "movq %%cr4, %[reg] \n\t btl %[la57], %k[reg]" CC_SET(c),
+             %P[feat], "stc", "clc")
+             : [reg] "=&r" (r), CC_OUT(c) (ret)
+             : [feat] "i"  (X86_FEATURE_LA57),
+               [la57] "i"  (X86_CR4_LA57_BIT)
+             : "cc");
+
+     return ret;

Yeah, I said this is creative but an alternative here looks like an overkill.

Can we use a RIP_REL_REF(global var) instead pls?

Today, pgtable_l5_enabled() is used in many places, most of which resolve to cpu_feature_enabled(), and I don't think you are suggesting to replace all of those with a variable load, right?

So that means we'll have to stick with early and late variants of pgtable_l5_enabled() like we have today,

and we should just drop this patch instead - I put it at the end of the series for a reason.

Quoted 3 lines · Ard Biesheuvel

Today, pgtable_l5_enabled() is used in many places, most of which resolve to cpu_feature_enabled(), and I don't think you are suggesting to replace all of those with a variable load, right?

pgtable_l5_enabled() is the odd one out, special thing which should have been cpu_feature_enabled() as latter is the default interface we use to query what features the CPU supports. But 5level got done long ago and we hadn't decided upon cpu_feature_enabled() back then.

So should we replace it with it?

Yap, eventually.

Quoted 2 lines

So that means we'll have to stick with early and late variants of pgtable_l5_enabled() like we have today,

I don't mind at all if we had a

early_pgtable_l5_enabled()

which does the RIP_REL_REF() thing and it should be used only in 1:1 mapping code and the late stuff should start to get replaced with cpu_feature_enabled() until pgtable_l5_enabled() is completely gone.

And then we even see whether we can opencode the handful places.

Quoted 2 lines

and we should just drop this patch instead - I put it at the end of the series for a reason.

Yeah, we can leave that one aside for now but use it to start a cleanup series.

If you're bored and feel like doing it, be my guest but for the next cycle. Or I'll put it on my evergrowing TODO and get to it eventually.

For now, lemme test your set without this one and see whether we can queue them even now.

That is not the issue here. The issue is that cpu_feature_enabled() will produce the wrong value if you call it too early.

From: Ard Biesheuvel <[email protected]>

This is resend #2 of v5 [0] with some touchups applied.

Changes since v6:
- Drop flawed patch to move some SME/SEV related calls out of the early boot path to avoid the potential need for backporting patches #6/#7 to kernels where SEV support may not be crucial. This problem will be dealt with if/when it arises while doing those backports.

Changes since v5:
- drop patches that have been merged
- rebase onto latest tip/x86/boot
- fix comment regarding CR4.PGE wrt flushing of global TLB entries
- avoid adding startup code to .noinstr.text as it triggers objtool warnings

[0] https://lore.kernel.org/all/[email protected]/

Cc Kevin Loughlin <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc Dionna Glaze <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Brian Gerst <[email protected]>

Ard Biesheuvel (9):
x86/startup_64: Simplify CR4 handling in startup code x86/startup_64: Defer assignment of 5-level paging global variables x86/startup_64: Simplify calculation of initial page table address x86/startup_64: Simplify virtual switch on primary boot efi/libstub: Add generic support for parsing mem_encrypt= x86/boot: Move mem_encrypt= parsing to the decompressor x86/sme: Move early SME kernel encryption handling into .head.text x86/sev: Move early startup code into .head.text section x86/startup_64: Drop global variables keeping track of LA57 state

  arch/x86/boot/compressed/misc.c                | 15 ++++
  arch/x86/boot/compressed/misc.h                |  4 -
  arch/x86/boot/compressed/pgtable_64.c          | 12 ---
  arch/x86/boot/compressed/sev.c                 |  3 +
  arch/x86/boot/compressed/vmlinux.lds.S         |  1 +
  arch/x86/include/asm/mem_encrypt.h             |  8 +-
  arch/x86/include/asm/pgtable_64_types.h        | 43 ++++-----
  arch/x86/include/asm/sev.h                     | 10 +--
  arch/x86/include/uapi/asm/bootparam.h          |  1 +
  arch/x86/kernel/cpu/common.c                   |  2 -
  arch/x86/kernel/head64.c                       | 61 ++-----------
  arch/x86/kernel/head_64.S                      | 93 ++++++++------------
  arch/x86/kernel/sev-shared.c                   | 23 +++--
  arch/x86/kernel/sev.c                          | 14 +--
  arch/x86/lib/Makefile                          | 13 ---
  arch/x86/mm/kasan_init_64.c                    |  3 -
  arch/x86/mm/mem_encrypt_identity.c             | 83 +++++------------
  drivers/firmware/efi/libstub/efi-stub-helper.c |  8 ++
  drivers/firmware/efi/libstub/efistub.h         |  2 +-
  drivers/firmware/efi/libstub/x86-stub.c        |  3 +
  20 files changed, 147 insertions(+), 255 deletions(-)

Quoted 19 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

This is resend #2 of v5 [0] with some touchups applied.

Changes since v6:
- Drop flawed patch to move some SME/SEV related calls out of the early boot path to avoid the potential need for backporting patches #6/#7 to kernels where SEV support may not be crucial. This problem will be dealt with if/when it arises while doing those backports.

Changes since v5:
- drop patches that have been merged
- rebase onto latest tip/x86/boot
- fix comment regarding CR4.PGE wrt flushing of global TLB entries
- avoid adding startup code to .noinstr.text as it triggers objtool warnings

[0] https://lore.kernel.org/all/[email protected]/

Quoted 9 lines

Cc Kevin Loughlin <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc Dionna Glaze <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Brian Gerst <[email protected]>

For the series, tested bare metal boots with mem_encrypt=on / mem_encrypt=off and boots of SVM, SEV, SEV-ES and SEV-SNP guests.

Tested-by Tom Lendacky <[email protected]>

Quoted 31 lines · Tom Lendacky

Quoted 28 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

This is resend #2 of v5 [0] with some touchups applied.

Changes since v6:
- Drop flawed patch to move some SME/SEV related calls out of the early boot path to avoid the potential need for backporting patches #6/#7 to kernels where SEV support may not be crucial. This problem will be dealt with if/when it arises while doing those backports.

Changes since v5:
- drop patches that have been merged
- rebase onto latest tip/x86/boot
- fix comment regarding CR4.PGE wrt flushing of global TLB entries
- avoid adding startup code to .noinstr.text as it triggers objtool warnings

[0] https://lore.kernel.org/all/[email protected]/

Cc Kevin Loughlin <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc Dionna Glaze <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Brian Gerst <[email protected]>

Quoted 5 lines

For the series, tested bare metal boots with mem_encrypt=on / mem_encrypt=off and boots of SVM, SEV, SEV-ES and SEV-SNP guests.

Tested-by Tom Lendacky <[email protected]>

Thanks a lot! I take it this was a kernel built with GCC?

Quoted 35 lines · Ard Biesheuvel

Quoted 34 lines · Tom Lendacky

Quoted 19 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

This is resend #2 of v5 [0] with some touchups applied.

Changes since v6:
- Drop flawed patch to move some SME/SEV related calls out of the early boot path to avoid the potential need for backporting patches #6/#7 to kernels where SEV support may not be crucial. This problem will be dealt with if/when it arises while doing those backports.

Changes since v5:
- drop patches that have been merged
- rebase onto latest tip/x86/boot
- fix comment regarding CR4.PGE wrt flushing of global TLB entries
- avoid adding startup code to .noinstr.text as it triggers objtool warnings

[0] https://lore.kernel.org/all/[email protected]/

Quoted 9 lines

Cc Kevin Loughlin <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc Dionna Glaze <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Brian Gerst <[email protected]>

For the series, tested bare metal boots with mem_encrypt=on / mem_encrypt=off and boots of SVM, SEV, SEV-ES and SEV-SNP guests.

Quoted 4 lines

Quoted 2 lines

Tested-by Tom Lendacky <[email protected]>

Thanks a lot! I take it this was a kernel built with GCC?

Yes, it was a GCC build, so I went back and rebuilt with Clang-14 and everything was fine there, too.