lkml.org 
[lkml]   [2021]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
From
Date


On 3/31/21 2:49 PM, Dave Hansen wrote:
> On 3/31/21 2:09 PM, Kuppuswamy Sathyanarayanan wrote:
>> As per Guest-Host Communication Interface (GHCI) Specification
>> for Intel TDX, sec 2.4.1, TDX architecture does not support
>> MWAIT, MONITOR and WBINVD instructions. So in non-root TDX mode,
>> if MWAIT/MONITOR instructions are executed with CPL != 0 it will
>> trigger #UD, and for CPL = 0 case, virtual exception (#VE) is
>> triggered. WBINVD instruction behavior is also similar to
>> MWAIT/MONITOR, but for CPL != 0 case, it will trigger #GP instead
>> of #UD.
>
> Could we give it a go to try this in plain English before jumping in and
> quoting the exact spec section? Also, the CPL language is nice and
> precise for talking inside Intel, but it's generally easier for me to
> read kernel descriptions when we just talk about the kernel.
>
> When running as a TDX guest, there are a number of existing,
> privileged instructions that do not work. If the guest kernel
> uses these instructions, the hardware generates a #VE.
I will fix it in next version.
>
> Which reminds me... The SDM says: MWAIT will "#UD ... If
> CPUID.01H:ECX.MONITOR[bit 3] = 0". So, is this an architectural change?
> The guest is *supposed* to see that CPUID bit as 0, so shouldn't it
> also get a #UD? Or is this all so that if SEAM *forgets* to clear the
> CPUID bit, the guest gets #VE?
AFAIK, we are only concerned about the case where the instruction support
is not disabled by SEAM. For disabled case, it should get #UD.
Sean, can you confirm it?
>
> What are we *actually* mitigating here?
we add support for #VE, when executing un-supported instruction in TD guest
kernel.
>
> Also, FWIW, MWAIT/MONITOR and WBINVD are pretty different beasts. I
> think this would all have been a lot more clear if this would have been
> two patches instead of shoehorning them into one.
Since all of them are unsupported instructions, I have grouped them
together. Even if we split it, there should be some duplication in commit
log (since handling is similar). But let me know if this is a desired
approach. I can split it in two patches.
>
>> To prevent TD guest from using these unsupported instructions,
>> following measures are adapted:
>>
>> 1. For MWAIT/MONITOR instructions, support for these instructions
>> are already disabled by TDX module (SEAM). So CPUID flags for
>> these instructions should be in disabled state. Also, just to be
>> sure that these instructions are disabled, forcefully unset
>> X86_FEATURE_MWAIT CPU cap in OS.
>>
>> 2. For WBINVD instruction, we use audit to find the code that uses
>> this instruction and disable them for TD.
>
> Really? Where are those patches?
For MWAIT/MONITOR, the change is included in the same patch.
For WBINVD, we have will have some patches included in next
series.
>
>> +static inline bool cpuid_has_mwait(void)
>> +{
>> + if (cpuid_ecx(1) & (1 << (X86_FEATURE_MWAIT % 32)))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> bool is_tdx_guest(void)
>> {
>> return static_cpu_has(X86_FEATURE_TDX_GUEST);
>> @@ -301,12 +309,25 @@ static int tdg_handle_mmio(struct pt_regs *regs, struct ve_info *ve)
>> return insn.length;
>> }
>>
>> +/* Initialize TDX specific CPU capabilities */
>> +static void __init tdx_cpu_cap_init(void)
>> +{
>> + setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
>> +
>> + if (cpuid_has_mwait()) {
>> + WARN(1, "TDX Module failed to disable MWAIT\n");
>
> WARN(1, "TDX guest enumerated support for MWAIT, disabling it").
will fix it in next version.
>
>> + /* MWAIT is not supported in TDX platform, so suppress it */
>> + setup_clear_cpu_cap(X86_FEATURE_MWAIT);
>> + }
>> +
>> +}
>
> Extra newline.
>
>> void __init tdx_early_init(void)
>> {
>> if (!cpuid_has_tdx_guest())
>> return;
>>
>> - setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
>> + tdx_cpu_cap_init();
>>
>> tdg_get_info();
>>
>> @@ -362,6 +383,27 @@ 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:
>> + /*
>> + * TDX architecture does not support WBINVD instruction.
>> + * Currently, usage of this instruction is prevented by
>> + * disabling the drivers which uses it. So if we still
>> + * reach here, it needs user attention.
>> + */
>
> This comment is awfully vague. "TDX architecture..." what? Any CPUs
> supporting the TDX architecture? TDX VMM's? TDX Guests?
>
> Let's also not waste byte on stating the obvious. If it didn't need
> attention we wouldn't be warning about it, eh?
>
> So, let's halve the size of the comment and say:
>
> /*
> * WBINVD is not supported inside TDX guests. All in-
> * kernel uses should have been disabled.
> */
ok. will fix it next version.
>
>> + pr_err("TD Guest used unsupported WBINVD instruction\n");
>> + BUG();
>> + break;
>> + case EXIT_REASON_MONITOR_INSTRUCTION:
>> + case EXIT_REASON_MWAIT_INSTRUCTION:
>> + /*
>> + * MWAIT/MONITOR features are disabled by TDX Module (SEAM)
>> + * and also re-suppressed in kernel by clearing
>> + * X86_FEATURE_MWAIT CPU feature flag in tdx_early_init(). So
>> + * if TD guest still executes MWAIT/MONITOR instruction with
>> + * above suppression, it needs user attention.
>> + */
>
> Again, let's trim this down:
>
> /*
> * Something in the kernel used MONITOR or MWAIT despite
> * X86_FEATURE_MWAIT being cleared for TDX guests.
> */
will fix it next version.
>
> Rather than naming the function, this makes it quite greppable to find
> where it could have *possibly* been cleared.
>
>> + WARN(1, "TD Guest used unsupported MWAIT/MONITOR instruction\n");
I think WARN_ONCE is good enough for this exception. Do you agree?
>> + break;
>> default:
>> pr_warn("Unexpected #VE: %d\n", ve->exit_reason);
>> return -EFAULT;
>>
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

\
 
 \ /
  Last update: 2021-04-01 00:31    [W:0.340 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site