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

[Kuppuswamy, Sathyanarayanan]

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