The boot trampolines from trampoline_64.S have code flow like:
16bit BIOS SEV-ES 64bit EFI
trampoline_start() sev_es_trampoline_start() trampoline_start_64()
verify_cpu() | |
switch_to_protected: <---------------' v
| pa_trampoline_compat()
v |
startup_32() <-----------------------------------------------'
|
v
startup_64()
|
v
tr_start() := head_64.S:secondary_startup_64()
Since AP bringup always goes through the 16bit BIOS path (EFI doesn't
touch the APs), there is already a verify_cpu() invocation.
Removing the verify_cpu() invocation from secondary_startup_64()
renders the whole secondary_startup_64_no_verify() thing moot, so
remove that too.
Cc: [email protected]
Cc: [email protected]
Fixes: e81dc127ef69 ("x86/callthunks: Add call patching for call depth tracking")
Reported-by: Joan Bruguera <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/realmode.h | 1 -
arch/x86/kernel/head_64.S | 16 ----------------
arch/x86/realmode/init.c | 6 ------
3 files changed, 23 deletions(-)
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -73,7 +73,6 @@ extern unsigned char startup_32_smp[];
extern unsigned char boot_gdt[];
#else
extern unsigned char secondary_startup_64[];
-extern unsigned char secondary_startup_64_no_verify[];
#endif
static inline size_t real_mode_size_needed(void)
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -143,22 +143,6 @@ SYM_CODE_START(secondary_startup_64)
* after the boot processor executes this code.
*/
- /* Sanitize CPU configuration */
- call verify_cpu
-
- /*
- * The secondary_startup_64_no_verify entry point is only used by
- * SEV-ES guests. In those guests the call to verify_cpu() would cause
- * #VC exceptions which can not be handled at this stage of secondary
- * CPU bringup.
- *
- * All non SEV-ES systems, especially Intel systems, need to execute
- * verify_cpu() above to make sure NX is enabled.
- */
-SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
- UNWIND_HINT_EMPTY
- ANNOTATE_NOENDBR
-
/*
* Retrieve the modifier (SME encryption mask if SME is active) to be
* added to the initial pgdir entry that will be programmed into CR3.
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -74,12 +74,6 @@ static void __init sme_sev_setup_real_mo
th->flags |= TH_FLAGS_SME_ACTIVE;
if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
- /*
- * Skip the call to verify_cpu() in secondary_startup_64 as it
- * will cause #VC exceptions when the AP can't handle them yet.
- */
- th->start = (u64) secondary_startup_64_no_verify;
-
if (sev_es_setup_ap_jump_table(real_mode_header))
panic("Failed to get/update SEV-ES AP Jump Table");
}
* Peter Zijlstra <[email protected]> wrote:
> The boot trampolines from trampoline_64.S have code flow like:
>
> 16bit BIOS SEV-ES 64bit EFI
>
> trampoline_start() sev_es_trampoline_start() trampoline_start_64()
> verify_cpu() | |
> switch_to_protected: <---------------' v
> | pa_trampoline_compat()
> v |
> startup_32() <-----------------------------------------------'
> |
> v
> startup_64()
> |
> v
> tr_start() := head_64.S:secondary_startup_64()
oh ... this nice flow chart should move into a prominent C comment I think,
it's far too good to be forgotten in an Git commit changelog.
> Since AP bringup always goes through the 16bit BIOS path (EFI doesn't
> touch the APs), there is already a verify_cpu() invocation.
>
> Removing the verify_cpu() invocation from secondary_startup_64()
> renders the whole secondary_startup_64_no_verify() thing moot, so
> remove that too.
>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: e81dc127ef69 ("x86/callthunks: Add call patching for call depth tracking")
> Reported-by: Joan Bruguera <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
Thanks,
Ingo
On Mon, Jan 16, 2023 at 03:25:34PM +0100, Peter Zijlstra wrote:
> The boot trampolines from trampoline_64.S have code flow like:
>
> 16bit BIOS SEV-ES 64bit EFI
>
> trampoline_start() sev_es_trampoline_start() trampoline_start_64()
> verify_cpu() | |
> switch_to_protected: <---------------' v
> | pa_trampoline_compat()
> v |
> startup_32() <-----------------------------------------------'
> |
> v
> startup_64()
> |
> v
> tr_start() := head_64.S:secondary_startup_64()
>
> Since AP bringup always goes through the 16bit BIOS path (EFI doesn't
> touch the APs), there is already a verify_cpu() invocation.
So supposedly TDX/ACPI-6.4 comes in on trampoline_startup64() for APs --
can any of the TDX capable folks tell me if we need verify_cpu() on
these?
Aside from checking for LM, it seems to clear XD_DISABLE on Intel and
force enable SSE on AMD/K7. Surely none of that is needed for these
shiny new chips?
I mean, I can hack up a patch that adds verify_cpu() to the 64bit entry
point, but it seems really sad to need that on modern systems.
On Wed, Jan 18, 2023 at 10:45:44AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 16, 2023 at 03:25:34PM +0100, Peter Zijlstra wrote:
> > The boot trampolines from trampoline_64.S have code flow like:
> >
> > 16bit BIOS SEV-ES 64bit EFI
> >
> > trampoline_start() sev_es_trampoline_start() trampoline_start_64()
> > verify_cpu() | |
> > switch_to_protected: <---------------' v
> > | pa_trampoline_compat()
> > v |
> > startup_32() <-----------------------------------------------'
> > |
> > v
> > startup_64()
> > |
> > v
> > tr_start() := head_64.S:secondary_startup_64()
> >
> > Since AP bringup always goes through the 16bit BIOS path (EFI doesn't
> > touch the APs), there is already a verify_cpu() invocation.
>
> So supposedly TDX/ACPI-6.4 comes in on trampoline_startup64() for APs --
> can any of the TDX capable folks tell me if we need verify_cpu() on
> these?
>
> Aside from checking for LM, it seems to clear XD_DISABLE on Intel and
> force enable SSE on AMD/K7. Surely none of that is needed for these
> shiny new chips?
TDX has no XD_DISABLE set and it doesn't allow to write to
IA32_MISC_ENABLE MSR (triggers #VE), so we should be safe.
--
Kiryl Shutsemau / Kirill A. Shutemov
On January 18, 2023 1:45:44 AM PST, Peter Zijlstra <[email protected]> wrote:
>On Mon, Jan 16, 2023 at 03:25:34PM +0100, Peter Zijlstra wrote:
>> The boot trampolines from trampoline_64.S have code flow like:
>>
>> 16bit BIOS SEV-ES 64bit EFI
>>
>> trampoline_start() sev_es_trampoline_start() trampoline_start_64()
>> verify_cpu() | |
>> switch_to_protected: <---------------' v
>> | pa_trampoline_compat()
>> v |
>> startup_32() <-----------------------------------------------'
>> |
>> v
>> startup_64()
>> |
>> v
>> tr_start() := head_64.S:secondary_startup_64()
>>
>> Since AP bringup always goes through the 16bit BIOS path (EFI doesn't
>> touch the APs), there is already a verify_cpu() invocation.
>
>So supposedly TDX/ACPI-6.4 comes in on trampoline_startup64() for APs --
>can any of the TDX capable folks tell me if we need verify_cpu() on
>these?
>
>Aside from checking for LM, it seems to clear XD_DISABLE on Intel and
>force enable SSE on AMD/K7. Surely none of that is needed for these
>shiny new chips?
>
>I mean, I can hack up a patch that adds verify_cpu() to the 64bit entry
>point, but it seems really sad to need that on modern systems.
Sad, perhaps, but really better for orthogonality – fewer special cases.
On Thu, Jan 19, 2023 at 11:35:06AM -0800, H. Peter Anvin wrote:
> On January 18, 2023 1:45:44 AM PST, Peter Zijlstra <[email protected]> wrote:
> >On Mon, Jan 16, 2023 at 03:25:34PM +0100, Peter Zijlstra wrote:
> >> The boot trampolines from trampoline_64.S have code flow like:
> >>
> >> 16bit BIOS SEV-ES 64bit EFI
> >>
> >> trampoline_start() sev_es_trampoline_start() trampoline_start_64()
> >> verify_cpu() | |
> >> switch_to_protected: <---------------' v
> >> | pa_trampoline_compat()
> >> v |
> >> startup_32() <-----------------------------------------------'
> >> |
> >> v
> >> startup_64()
> >> |
> >> v
> >> tr_start() := head_64.S:secondary_startup_64()
> >>
> >> Since AP bringup always goes through the 16bit BIOS path (EFI doesn't
> >> touch the APs), there is already a verify_cpu() invocation.
> >
> >So supposedly TDX/ACPI-6.4 comes in on trampoline_startup64() for APs --
> >can any of the TDX capable folks tell me if we need verify_cpu() on
> >these?
> >
> >Aside from checking for LM, it seems to clear XD_DISABLE on Intel and
> >force enable SSE on AMD/K7. Surely none of that is needed for these
> >shiny new chips?
> >
> >I mean, I can hack up a patch that adds verify_cpu() to the 64bit entry
> >point, but it seems really sad to need that on modern systems.
>
> Sad, perhaps, but really better for orthogonality – fewer special cases.
I'd argue more, but whatever. XD_DISABLE is an abomination and 64bit
entry points should care about it just as much as having LM. And this
way we have 2/3 instead of 1/3 entry points do 'special' nonsense.
I ended up with this trainwreck, it adds verify_cpu to
pa_trampoline_compat() because for some raisin it doesn't want to
assemble when placed in trampoline_start64().
Is this really what we want?
---
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -689,9 +689,14 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lno_longmo
jmp 1b
SYM_FUNC_END(.Lno_longmode)
- .globl verify_cpu
#include "../../kernel/verify_cpu.S"
+ .globl verify_cpu
+SYM_FUNC_START_LOCAL(verify_cpu)
+ VERIFY_CPU
+ RET
+SYM_FUNC_END(verify_cpu)
+
.data
SYM_DATA_START_LOCAL(gdt64)
.word gdt_end - gdt - 1
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -321,6 +321,11 @@ SYM_FUNC_END(startup_32_smp)
#include "verify_cpu.S"
+SYM_FUNC_START_LOCAL(verify_cpu)
+ VERIFY_CPU
+ RET
+SYM_FUNC_END(verify_cpu)
+
__INIT
SYM_FUNC_START(early_idt_handler_array)
# 36(%esp) %eflags
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -345,6 +345,12 @@ SYM_CODE_START(secondary_startup_64)
SYM_CODE_END(secondary_startup_64)
#include "verify_cpu.S"
+
+SYM_FUNC_START_LOCAL(verify_cpu)
+ VERIFY_CPU
+ RET
+SYM_FUNC_END(verify_cpu)
+
#include "sev_verify_cbit.S"
#ifdef CONFIG_HOTPLUG_CPU
--- a/arch/x86/kernel/verify_cpu.S
+++ b/arch/x86/kernel/verify_cpu.S
@@ -31,7 +31,7 @@
#include <asm/cpufeatures.h>
#include <asm/msr-index.h>
-SYM_FUNC_START_LOCAL(verify_cpu)
+.macro VERIFY_CPU
pushf # Save caller passed flags
push $0 # Kill any dangerous flags
popf
@@ -46,31 +46,31 @@ SYM_FUNC_START_LOCAL(verify_cpu)
pushfl
popl %eax
cmpl %eax,%ebx
- jz .Lverify_cpu_no_longmode # cpu has no cpuid
+ jz .Lverify_cpu_no_longmode_\@ # cpu has no cpuid
#endif
movl $0x0,%eax # See if cpuid 1 is implemented
cpuid
cmpl $0x1,%eax
- jb .Lverify_cpu_no_longmode # no cpuid 1
+ jb .Lverify_cpu_no_longmode_\@ # no cpuid 1
xor %di,%di
cmpl $0x68747541,%ebx # AuthenticAMD
- jnz .Lverify_cpu_noamd
+ jnz .Lverify_cpu_noamd_\@
cmpl $0x69746e65,%edx
- jnz .Lverify_cpu_noamd
+ jnz .Lverify_cpu_noamd_\@
cmpl $0x444d4163,%ecx
- jnz .Lverify_cpu_noamd
+ jnz .Lverify_cpu_noamd_\@
mov $1,%di # cpu is from AMD
- jmp .Lverify_cpu_check
+ jmp .Lverify_cpu_check_\@
-.Lverify_cpu_noamd:
+.Lverify_cpu_noamd_\@:
cmpl $0x756e6547,%ebx # GenuineIntel?
- jnz .Lverify_cpu_check
+ jnz .Lverify_cpu_check_\@
cmpl $0x49656e69,%edx
- jnz .Lverify_cpu_check
+ jnz .Lverify_cpu_check_\@
cmpl $0x6c65746e,%ecx
- jnz .Lverify_cpu_check
+ jnz .Lverify_cpu_check_\@
# only call IA32_MISC_ENABLE when:
# family > 6 || (family == 6 && model >= 0xd)
@@ -81,60 +81,62 @@ SYM_FUNC_START_LOCAL(verify_cpu)
andl $0x0ff00f00, %eax # mask family and extended family
shrl $8, %eax
cmpl $6, %eax
- ja .Lverify_cpu_clear_xd # family > 6, ok
- jb .Lverify_cpu_check # family < 6, skip
+ ja .Lverify_cpu_clear_xd_\@ # family > 6, ok
+ jb .Lverify_cpu_check_\@ # family < 6, skip
andl $0x000f00f0, %ecx # mask model and extended model
shrl $4, %ecx
cmpl $0xd, %ecx
- jb .Lverify_cpu_check # family == 6, model < 0xd, skip
+ jb .Lverify_cpu_check_\@ # family == 6, model < 0xd, skip
-.Lverify_cpu_clear_xd:
+.Lverify_cpu_clear_xd_\@:
movl $MSR_IA32_MISC_ENABLE, %ecx
rdmsr
btrl $2, %edx # clear MSR_IA32_MISC_ENABLE_XD_DISABLE
- jnc .Lverify_cpu_check # only write MSR if bit was changed
+ jnc .Lverify_cpu_check_\@ # only write MSR if bit was changed
wrmsr
-.Lverify_cpu_check:
+.Lverify_cpu_check_\@:
movl $0x1,%eax # Does the cpu have what it takes
cpuid
andl $REQUIRED_MASK0,%edx
xorl $REQUIRED_MASK0,%edx
- jnz .Lverify_cpu_no_longmode
+ jnz .Lverify_cpu_no_longmode_\@
movl $0x80000000,%eax # See if extended cpuid is implemented
cpuid
cmpl $0x80000001,%eax
- jb .Lverify_cpu_no_longmode # no extended cpuid
+ jb .Lverify_cpu_no_longmode_\@ # no extended cpuid
movl $0x80000001,%eax # Does the cpu have what it takes
cpuid
andl $REQUIRED_MASK1,%edx
xorl $REQUIRED_MASK1,%edx
- jnz .Lverify_cpu_no_longmode
+ jnz .Lverify_cpu_no_longmode_\@
-.Lverify_cpu_sse_test:
+.Lverify_cpu_sse_test_\@:
movl $1,%eax
cpuid
andl $SSE_MASK,%edx
cmpl $SSE_MASK,%edx
- je .Lverify_cpu_sse_ok
+ je .Lverify_cpu_sse_ok_\@
test %di,%di
- jz .Lverify_cpu_no_longmode # only try to force SSE on AMD
+ jz .Lverify_cpu_no_longmode_\@ # only try to force SSE on AMD
movl $MSR_K7_HWCR,%ecx
rdmsr
btr $15,%eax # enable SSE
wrmsr
xor %di,%di # don't loop
- jmp .Lverify_cpu_sse_test # try again
+ jmp .Lverify_cpu_sse_test_\@ # try again
-.Lverify_cpu_no_longmode:
+.Lverify_cpu_no_longmode_\@:
popf # Restore caller passed flags
movl $1,%eax
- RET
-.Lverify_cpu_sse_ok:
+ jmp .Lverify_cpu_ret_\@
+
+.Lverify_cpu_sse_ok_\@:
popf # Restore caller passed flags
xorl %eax, %eax
- RET
-SYM_FUNC_END(verify_cpu)
+
+.Lverify_cpu_ret_\@:
+.endm
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -34,6 +34,8 @@
#include <asm/realmode.h>
#include "realmode.h"
+#include "../kernel/verify_cpu.S"
+
.text
.code16
@@ -52,7 +54,8 @@ SYM_CODE_START(trampoline_start)
# Setup stack
movl $rm_stack_end, %esp
- call verify_cpu # Verify the cpu supports long mode
+ VERIFY_CPU # Verify the cpu supports long mode
+
testl %eax, %eax # Check for return code
jnz no_longmode
@@ -100,8 +103,6 @@ SYM_CODE_START(sev_es_trampoline_start)
SYM_CODE_END(sev_es_trampoline_start)
#endif /* CONFIG_AMD_MEM_ENCRYPT */
-#include "../kernel/verify_cpu.S"
-
.section ".text32","ax"
.code32
.balign 4
@@ -180,6 +181,8 @@ SYM_CODE_START(pa_trampoline_compat)
movl $rm_stack_end, %esp
movw $__KERNEL_DS, %dx
+ VERIFY_CPU
+
movl $(CR0_STATE & ~X86_CR0_PG), %eax
movl %eax, %cr0
ljmpl $__KERNEL32_CS, $pa_startup_32