Re: [PATCH 10/10] x86/virt/tdx: Allow SEAMCALL to handle #UD and #GP

[Huang, Kai]

On Thu, 2023-07-13 at 10:07 +0200, Peter Zijlstra wrote:
> On Wed, Jul 12, 2023 at 08:55:24PM +1200, Kai Huang wrote:
> > @@ -85,6 +86,7 @@
> >  	.endif	/* \saved */
> >  
> >  	.if \host
> > +1:
> >  	seamcall
> >  	/*
> >  	 * SEAMCALL instruction is essentially a VMExit from VMX root
> > @@ -99,6 +101,7 @@
> >  	 */
> >  	mov $TDX_SEAMCALL_VMFAILINVALID, %rdi
> >  	cmovc %rdi, %rax
> > +2:
> >  	.else
> >  	tdcall
> >  	.endif
> 
> This is just wrong, if the thing traps you should not do the return
> registers. And at this point the mov/cmovc thing doesn't make much sense
> either.

OK will do.  Yes "do return registers" isn't necessary.  I thought to keep code
simple we can just do it.  The trap/VMFAILINVALID code path isn't in performance
path anyway.

This is a problem in the current upstream code too.  I'll fix it first in a
separate patch.

> 
> > @@ -185,4 +188,21 @@
> >  
> >  	FRAME_END
> >  	RET
> > +
> > +	.if \host
> > +3:
> > +	/*
> > +	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
> > +	 * the trap number.  Convert the trap number to the TDX error
> > +	 * code by setting TDX_SW_ERROR to the high 32-bits of %rax.
> > +	 *
> > +	 * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction
> > +	 * only accepts 32-bit immediate at most.
> > +	 */
> > +	movq $TDX_SW_ERROR, %r12
> > +	orq  %r12, %rax
> > +	jmp  2b
> > +
> > +	_ASM_EXTABLE_FAULT(1b, 3b)
> > +	.endif	/* \host */
> >  .endm
> 
> Also, please used named labels where possible and *please* keep asm
> directives unindented.

Yes will do.

> 
> 
> --- a/arch/x86/virt/vmx/tdx/tdxcall.S
> +++ b/arch/x86/virt/vmx/tdx/tdxcall.S
> @@ -56,7 +56,7 @@
>  	movq	TDX_MODULE_r10(%rsi), %r10
>  	movq	TDX_MODULE_r11(%rsi), %r11
>  
> -	.if \saved
> +.if \saved
>  	/*
>  	 * Move additional input regs from the structure.  For simplicity
>  	 * assume that anything needs the callee-saved regs also tramples
> @@ -75,18 +75,18 @@
>  	movq	TDX_MODULE_r15(%rsi), %r15
>  	movq	TDX_MODULE_rbx(%rsi), %rbx
>  
> -	.if \ret
> +.if \ret
>  	/* Save the structure pointer as %rsi is about to be clobbered */
>  	pushq	%rsi
> -	.endif
> +.endif
>  
>  	movq	TDX_MODULE_rdi(%rsi), %rdi
>  	/* %rsi needs to be done at last */
>  	movq	TDX_MODULE_rsi(%rsi), %rsi
> -	.endif	/* \saved */
> +.endif	/* \saved */
>  
> -	.if \host
> -1:
> +.if \host
> +.Lseamcall:
>  	seamcall
>  	/*
>  	 * SEAMCALL instruction is essentially a VMExit from VMX root
> @@ -99,15 +99,13 @@
>  	 * This value will never be used as actual SEAMCALL error code as
>  	 * it is from the Reserved status code class.
>  	 */
> -	mov $TDX_SEAMCALL_VMFAILINVALID, %rdi
> -	cmovc %rdi, %rax
> -2:
> -	.else
> +	jc	.Lseamfail
> +.else
>  	tdcall
> -	.endif
> +.endif
>  
> -	.if \ret
> -	.if \saved
> +.if \ret
> +.if \saved
>  	/*
>  	 * Restore the structure from stack to saved the output registers
>  	 *
> @@ -136,7 +134,7 @@
>  	movq %r15, TDX_MODULE_r15(%rsi)
>  	movq %rbx, TDX_MODULE_rbx(%rsi)
>  	movq %rdi, TDX_MODULE_rdi(%rsi)
> -	.endif	/* \saved */
> +.endif	/* \saved */
>  
>  	/* Copy output regs to the structure */
>  	movq %rcx, TDX_MODULE_rcx(%rsi)
> @@ -145,10 +143,11 @@
>  	movq %r9,  TDX_MODULE_r9(%rsi)
>  	movq %r10, TDX_MODULE_r10(%rsi)
>  	movq %r11, TDX_MODULE_r11(%rsi)
> -	.endif	/* \ret */
> +.endif	/* \ret */
>  
> -	.if \saved
> -	.if \ret
> +.Lout:
> +.if \saved
> +.if \ret
>  	/*
>  	 * Clear registers shared by guest for VP.ENTER and VP.VMCALL to
>  	 * prevent speculative use of values from guest/VMM, including
> @@ -170,13 +169,8 @@
>  	xorq %r9,  %r9
>  	xorq %r10, %r10
>  	xorq %r11, %r11
> -	xorq %r12, %r12
> -	xorq %r13, %r13
> -	xorq %r14, %r14
> -	xorq %r15, %r15
> -	xorq %rbx, %rbx
>  	xorq %rdi, %rdi
> -	.endif	/* \ret */
> +.endif	/* \ret */
>  
>  	/* Restore callee-saved GPRs as mandated by the x86_64 ABI */
>  	popq	%r15
> @@ -184,13 +178,17 @@
>  	popq	%r13
>  	popq	%r12
>  	popq	%rbx
> -	.endif	/* \saved */
> +.endif	/* \saved */
>  
>  	FRAME_END
>  	RET
>  
> -	.if \host
> -3:
> +.if \host
> +.Lseamfail:
> +	mov	$TDX_SEAMCALL_VMFAILINVALID, %rax
> +	jmp	.Lout
> +
> +.Lseamtrap:
>  	/*
>  	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
>  	 * the trap number.  Convert the trap number to the TDX error
> @@ -201,8 +199,8 @@
>  	 */
>  	movq $TDX_SW_ERROR, %r12
>  	orq  %r12, %rax
> -	jmp  2b
> +	jmp .Lout

Thanks for the code.

There might be stack balancing issue here.  I'll double check when updating this
patch.

Thanks!

>  
> -	_ASM_EXTABLE_FAULT(1b, 3b)
> -	.endif	/* \host */
> +	_ASM_EXTABLE_FAULT(.Lseamcall, .Lseamtrap)
> +.endif	/* \host */
>  .endm