Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD

[Dave Hansen <dave.hansen@intel.com>]

On 3/29/21 4:16 PM, Kuppuswamy Sathyanarayanan wrote:
> In non-root TDX guest mode, MWAIT, MONITOR and WBINVD instructions
> are not supported. So handle #VE due to these instructions
> appropriately.

This misses a key detail:

	"are not supported" ... and other patches have prevented a guest
	from using these instructions.

In other words, they're bad now, and we know it.  We tried to keep the
kernel from using them, but we failed.  Whoops.

> Since the impact of executing WBINVD instruction in non ring-0 mode
> can be heavy, use BUG() to report it. For others, raise a WARNING
> message.

"Heavy" makes it sound like there's a performance impact.



>  	pv_ops.irq.safe_halt = tdg_safe_halt;
> @@ -362,6 +365,32 @@ int tdg_handle_virtualization_exception(struct pt_regs *regs,
>  	case EXIT_REASON_EPT_VIOLATION:
>  		ve->instr_len = tdg_handle_mmio(regs, ve);
>  		break;
> +	case EXIT_REASON_WBINVD:
> +		/*
> +		 * WBINVD is a privileged instruction, can only be executed
> +		 * in ring 0. Since we reached here, the kernel is in buggy
> +		 * state.
> +		 */
> +		pr_err("WBINVD #VE Exception\n");

"#VE Exception" is basically wasted bytes.  It really tells us nothing.
 This, on the other hand:

	"TDX guest used unsupported WBINVD instruction"

gives us the clues that TDX is involved and that the kernel used the
instruction.  The fact that #VE itself is involved is kinda irrelevant.

I also hate the comment.  Don't waste the bytes saying we're in a buggy
state.  That's kinda obvious from the BUG().

Again, it might be nice to mention in the changelog *WHY* we're so sure
that WBINVD won't be called from a TDX guest.  What did we do to
guarantee that?  How could we be sure that someone looking at the splat
that this generates and then reading the comment can go fix the bug in
their kernel?

> +		BUG();
> +		break;
> +	case EXIT_REASON_MONITOR_INSTRUCTION:
> +		/*
> +		 * MONITOR is a privileged instruction, can only be executed
> +		 * in ring 0. So we are not supposed to reach here. Raise a
> +		 * warning message.
> +		 */
> +		WARN(1, "MONITOR unexpected #VE Exception\n");
> +		break;
> +	case EXIT_REASON_MWAIT_INSTRUCTION:
> +		/*
> +		 * MWAIT feature is suppressed in firmware and in
> +		 * tdx_early_init() by clearing X86_FEATURE_MWAIT CPU feature
> +		 * flag. Since we are not supposed to reach here, raise a
> +		 * warning message and return -EFAULT.
> +		 */
> +		WARN(1, "MWAIT unexpected #VE Exception\n");
> +		return -EFAULT;
>  	default:
>  		pr_warn("Unexpected #VE: %d\n", ve->exit_reason);
>  		return -EFAULT;

This is more of the same.  Don't waste comment bytes telling me we're
not suppose to reach a BUG() or WARN().