Re: [RFC v2-fix 1/1] x86/boot: Avoid #VE during boot for TDX platforms

[Dave Hansen]

On 5/17/21 5:59 PM, Kuppuswamy Sathyanarayanan wrote:
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> 
> Avoid operations which will inject #VE during boot process,
> which is obviously fatal for TDX platforms.

It's not "obviously fatal".  We actually have early exception handlers.
 Please give an actual reason.  "They're easy to avoid, and that sure
beats handling the exceptions" is a perfectly fine reason.

> Details are,
> 
> 1. TDX module injects #VE if a TDX guest attempts to write
>    EFER.
>    
>    Boot code updates EFER in following cases:
>    
>    * When enabling Long Mode configuration, EFER.LME bit will
>      be set. Since TDX forces EFER.LME=1, we can skip updating
>      it again. Check for EFER.LME before updating it and skip
>      it if it is already set.
> 
>    * EFER is also updated to enable support for features like
>      System call and No Execute page setting. In TDX, these
>      features are set up by the TDX module. So check whether
>      it is already enabled, and skip enabling it again.
>    
> 2. TDX module also injects a #VE if the guest attempts to clear
>    CR0.NE. Ensure CR0.NE is set when loading CR0 during compressed
>    boot. The Setting CR0.NE should be a nop on all CPUs that
>    support 64-bit mode.
>    
> 3. The TDX-Module (effectively part of the hypervisor) requires

So, after we've mentioned the TDX module a few times, *NOW* we feel the
need to explain what it is?  I'm also baffled by this little aside.
Literally the WHOLE POINT FOR SEAM TO EXIST is that it is NOT PART OF
THE HYPERVISOR.  The whole point.  Literally.

>    CR4.MCE to be set at all times and injects a #VE if the guest
>    attempts to clear CR4.MCE. So, preserve CR4.MCE instead of
>    clearing it during boot to avoid #VE.

This is a good example of a changelog run amok.  It doesn't need to be
an English language reproduction of the code.  This is getting close.

This can all be replaced and improved with a high-level discussion of
what is going on:

	There are a few MSRs and control register bits which the kernel
	normally needs to modify during boot.  But, TDX disallows
	modification of these registers to help provide consistent
	security guarantees.  Fortunately, TDX ensures that these are
	all in the correct state before the kernel loads, which means
	the kernel has no need to modify them.

	The conditions we need to avoid are:
	1. Any writes to the EFER MSR
	2. Clearing CR0.NE
	3. Clearing CR3.MCE

> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index e94874f4bbc1..2d79e5f97360 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -616,12 +616,16 @@ SYM_CODE_START(trampoline_32bit_src)
>  	movl	$MSR_EFER, %ecx
>  	rdmsr
>  	btsl	$_EFER_LME, %eax
> +	jc	1f
>  	wrmsr
> -	popl	%edx
> +1:	popl	%edx

A comment would be nice:

	/* Avoid writing EFER if no change was made (for TDX guest) */

>  	popl	%ecx
>  
>  	/* Enable PAE and LA57 (if required) paging modes */
> -	movl	$X86_CR4_PAE, %eax
> +	movl	%cr4, %eax
> +	/* Clearing CR4.MCE will #VE on TDX guests.  Leave it alone. */
> +	andl	$X86_CR4_MCE, %eax

Maybe I'm just dense today, but I was boggling about what this 'andl' is
actually doing.  This would help:

	/*
	 * Clear all bits except CR4.MCE, which is preserved.
	 * Clearing CR4.MCE will #VE in TDX guests.
	 */

> +	orl	$X86_CR4_PAE, %eax
>  	testl	%edx, %edx
>  	jz	1f
>  	orl	$X86_CR4_LA57, %eax
> @@ -636,7 +640,7 @@ SYM_CODE_START(trampoline_32bit_src)
>  	pushl	%eax
>  
>  	/* Enable paging again */
> -	movl	$(X86_CR0_PG | X86_CR0_PE), %eax
> +	movl	$(X86_CR0_PG | X86_CR0_NE | X86_CR0_PE), %eax
>  	movl	%eax, %cr0

Shouldn't we also comment the X86_CR0_NE?

	/* Enable paging again.  Avoid clearing X86_CR0_NE for TDX. */

>  	lret
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 04bddaaba8e2..92c77cf75542 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -141,7 +141,10 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
>  1:
>  
>  	/* Enable PAE mode, PGE and LA57 */
> -	movl	$(X86_CR4_PAE | X86_CR4_PGE), %ecx
> +	movq	%cr4, %rcx
> +	/* Clearing CR4.MCE will #VE on TDX guests.  Leave it alone. */
> +	andl	$X86_CR4_MCE, %ecx
> +	orl	$(X86_CR4_PAE | X86_CR4_PGE), %ecx

Ditto on the comment from above about clearing/preserving bits.

>  #ifdef CONFIG_X86_5LEVEL
>  	testl	$1, __pgtable_l5_enabled(%rip)
>  	jz	1f
> @@ -229,13 +232,19 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
>  	/* Setup EFER (Extended Feature Enable Register) */
>  	movl	$MSR_EFER, %ecx
>  	rdmsr
> +	movl    %eax, %edx

Comment, please.

>  	btsl	$_EFER_SCE, %eax	/* Enable System Call */
>  	btl	$20,%edi		/* No Execute supported? */
>  	jnc     1f
>  	btsl	$_EFER_NX, %eax
>  	btsq	$_PAGE_BIT_NX,early_pmd_flags(%rip)
> -1:	wrmsr				/* Make changes effective */
>  
> +	/* Skip the WRMSR if the current value matches the desired value. */

If I read this comment in 5 years, I'm going to ask "Why bother?".
Please mention TDX.

> +1:	cmpl	%edx, %eax
> +	je	1f
> +	xor	%edx, %edx
> +	wrmsr				/* Make changes effective */
> +1:
>  	/* Setup cr0 */
>  	movl	$CR0_STATE, %eax
>  	/* Make changes effective */
> diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
> index 754f8d2ac9e8..12b734b1da8b 100644
> --- a/arch/x86/realmode/rm/trampoline_64.S
> +++ b/arch/x86/realmode/rm/trampoline_64.S
> @@ -143,13 +143,20 @@ SYM_CODE_START(startup_32)
>  	movl	%eax, %cr3
>  
>  	# Set up EFER
> +	movl	$MSR_EFER, %ecx
> +	rdmsr
> +	cmp	pa_tr_efer, %eax
> +	jne	.Lwrite_efer
> +	cmp	pa_tr_efer + 4, %edx

Comment, please:

	# Skip EFER writes to avoid faults in TDX guests

> +	je	.Ldone_efer
> +.Lwrite_efer:
>  	movl	pa_tr_efer, %eax
>  	movl	pa_tr_efer + 4, %edx
> -	movl	$MSR_EFER, %ecx
>  	wrmsr
>  
> +.Ldone_efer:
>  	# Enable paging and in turn activate Long Mode
> -	movl	$(X86_CR0_PG | X86_CR0_WP | X86_CR0_PE), %eax
> +	movl	$(X86_CR0_PG | X86_CR0_WP | X86_CR0_NE | X86_CR0_PE), %eax
>  	movl	%eax, %cr0
>  
>  	/*
>