Re: [RFC v2 07/32] x86/traps: Add do_general_protection() helper function

[Dave Hansen]

On 4/26/21 11:01 AM, Kuppuswamy Sathyanarayanan wrote:
> TDX guest #VE exception handler treats unsupported exceptions

 ^ The

> as #GP. So to handle the #GP, move the protection fault handler

s/So to/To/

Also, it does not "treat them as #GP".  It handles them in the same way
that a #GP is handled.  There's a difference between literally making
them a #GP and having a similar end result.  This description conflates
them.

> code to out of exc_general_protection() and create new helper
> function for it.

I wouldn't name the functions.  Just say that you want the #GP behavior
from #VE so you need a common helper.

> Also since exception handler is responsible to decide when to

	    ^ an

> turn on/off IRQ, move cond_local_irq_{enable/disable)() calls
> out of do_general_protection().

This paragraph doesn't really say anything meaningful.  Yes, exception
handlers reenable interrupts.  Try to *SAY* something about why they do
this and why you have to move the code around.  Or, just axe it.

> This is a preparatory patch for adding #VE exception handler
> support for TDX guests.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/traps.c | 51 ++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 651e3e508959..213d4aa8e337 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -527,44 +527,28 @@ static enum kernel_gp_hint get_kernel_gp_address(struct pt_regs *regs,
>  
>  #define GPFSTR "general protection fault"
>  
> -DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> +static void do_general_protection(struct pt_regs *regs, long error_code)
>  {
>  	char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;
>  	enum kernel_gp_hint hint = GP_NO_HINT;
> -	struct task_struct *tsk;
> +	struct task_struct *tsk = current;
>  	unsigned long gp_addr;
>  	int ret;
>  
> -	cond_local_irq_enable(regs);
> -
> -	if (static_cpu_has(X86_FEATURE_UMIP)) {
> -		if (user_mode(regs) && fixup_umip_exception(regs))
> -			goto exit;
> -	}
> -
> -	if (v8086_mode(regs)) {
> -		local_irq_enable();
> -		handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code);
> -		local_irq_disable();
> -		return;
> -	}
> -
> -	tsk = current;
> -
>  	if (user_mode(regs)) {
>  		tsk->thread.error_code = error_code;
>  		tsk->thread.trap_nr = X86_TRAP_GP;
>  
>  		if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
> -			goto exit;
> +			return;
>  
>  		show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
>  		force_sig(SIGSEGV);
> -		goto exit;
> +		return;
>  	}
>  
>  	if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
> -		goto exit;
> +		return;
>  
>  	tsk->thread.error_code = error_code;
>  	tsk->thread.trap_nr = X86_TRAP_GP;
> @@ -576,11 +560,11 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  	if (!preemptible() &&
>  	    kprobe_running() &&
>  	    kprobe_fault_handler(regs, X86_TRAP_GP))
> -		goto exit;
> +		return;
>  
>  	ret = notify_die(DIE_GPF, desc, regs, error_code, X86_TRAP_GP, SIGSEGV);

So... We're going to send signals based on #VE which use this bit in the
ABI which is documented as:

#define X86_TRAP_GP             13      /* General Protection Fault */

Considering that there is also a:

#define X86_TRAP_VE             20      /* Virtualization Exception */

this seems like a stretch.

Also, isnt there a lot of truly #GP-specific code in there, like
fixup_exception()?  Why do you need to call that for #VE?  How did you
decide what remains in the handler versus what gets separated out?

>  	if (ret == NOTIFY_STOP)
> -		goto exit;
> +		return;
>  
>  	if (error_code)
>  		snprintf(desc, sizeof(desc), "segment-related " GPFSTR);
> @@ -601,8 +585,27 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  		gp_addr = 0;
>  
>  	die_addr(desc, regs, error_code, gp_addr);
> +}
>  
> -exit:
> +DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> +{
> +	cond_local_irq_enable(regs);
> +
> +	if (static_cpu_has(X86_FEATURE_UMIP)) {
> +		if (user_mode(regs) && fixup_umip_exception(regs)) {
> +			cond_local_irq_disable(regs);
> +			return;
> +		}
> +	}
> +
> +	if (v8086_mode(regs)) {
> +		local_irq_enable();
> +		handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code);
> +		local_irq_disable();
> +		return;
> +	}
> +
> +	do_general_protection(regs, error_code);
>  	cond_local_irq_disable(regs);
>  }