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

From: Kuppuswamy, Sathyanarayanan
Date: Mon Mar 29 2021 - 17:56:24 EST




On 3/29/21 10:14 AM, Dave Hansen wrote:
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?
I will remove this and add individual one line comment for WBINVD and MONITOR
instructions. Some thing like "Privileged instruction, can only be executed
in ring 0. So raise a BUG.

+ 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.
Agree. I will call BUG().

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.
Agree. Since we are not supposed to come here, I will use BUG.

+ 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"?
I am clearing the MWAIT feature flag in early init code. We should also disable
this feature in firmware.
setup_clear_cpu_cap(X86_FEATURE_MWAIT);


--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer