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

[Dave Hansen]

On 3/27/21 3:54 PM, Kuppuswamy Sathyanarayanan wrote:
> +	/*
> +	 * Per Guest-Host-Communication Interface (GHCI) for Intel Trust
> +	 * Domain Extensions (Intel TDX) specification, sec 2.4,
> +	 * some instructions that unconditionally cause #VE (such as WBINVD,
> +	 * MONITOR, MWAIT) do not have corresponding TDCALL
> +	 * [TDG.VP.VMCALL <Instruction>] leaves, since the TD has been designed
> +	 * with no deterministic way to confirm the result of those operations
> +	 * performed by the host VMM.  In those cases, the goal is for the TD
> +	 * #VE handler to increment the RIP appropriately based on the VE
> +	 * information provided via TDCALL.
> +	 */

That's an awfully big comment.  Could you pare it down, please?  Maybe
focus on the fact that we should never get here and why, rather than
talking about some silly spec?

> +	case EXIT_REASON_WBINVD:
> +		pr_warn_once("WBINVD #VE Exception\n");

I actually think WBINVD in here should oops.  We use it for some really
important things.  If it can't be executed, and we're depending on it,
the kernel is in deep, deep trouble.

I think a noop here is dangerous.

> +	case EXIT_REASON_MONITOR_INSTRUCTION:
> +		/* Handle as nops. */
> +		break;

MONITOR is a privileged instruction, right?  So we can only end up in
here if the kernel screws up and isn't reading CPUID correctly, right?

That dosen't seem to me like something we want to suppress.  This needs
a warning, at least.  I assume that having a MONITOR instruction
immediately return doesn't do any harm.

> +	case EXIT_REASON_MWAIT_INSTRUCTION:
> +		/* MWAIT is supressed, not supposed to reach here. */
> +		WARN(1, "MWAIT unexpected #VE Exception\n");
> +		return -EFAULT;

How is MWAIT "supppressed"?