Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
> Hmm, I forgot about that quirk. I would expect the TDX Module to inject a #GP > for that case. I can't find anything in the spec that confirms or denies > that, > but injecting #VE would be weird and pointless. > > Andi/Sathya, the TDX Module spec should be updated to state that XSETBV will > #GP at CPL!=0. If that's not already the behavior, the module should probably > be changed... I asked about this and the answer was that XSETBV behaves architecturally inside a TD (no #VE), thus there is nothing to document. -Andi
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On Tue, Mar 30, 2021, Andy Lutomirski wrote: > > > On Mar 30, 2021, at 8:14 AM, Sean Christopherson wrote: > > > > On Mon, Mar 29, 2021, Andy Lutomirski wrote: > >> > On Mar 29, 2021, at 7:04 PM, Andi Kleen wrote: > >>> > >>> > > > No, if these instructions take a #VE then they were executed at CPL=0. > > MONITOR > > and MWAIT will #UD without VM-Exit->#VE. Same for WBINVD, s/#UD/#GP. > > Dare I ask about XSETBV? > >>> > >>> XGETBV does not cause a #VE, it just works normally. The guest has full > >>> AVX capabilities. > >>> > >> > >> X *SET* BV > > > > Heh, XSETBV also works normally, relative to the features enumerated in > > CPUID. > > XSAVES/XRSTORS support is fixed to '1' in the virtual CPU model. A subset > > of > > the features managed by XSAVE can be hidden by the VMM, but attempting to > > enable > > unsupported features will #GP (either from hardware or injected by TDX > > Module), > > not #VE. > > Normally in non-root mode means that every XSETBV results in a VM exit and, > IIUC, there’s a buglet in that this happens even if CPL==3. Does something > special happen in TDX or does the exit get reflected back to the guest as a > #VE? Hmm, I forgot about that quirk. I would expect the TDX Module to inject a #GP for that case. I can't find anything in the spec that confirms or denies that, but injecting #VE would be weird and pointless. Andi/Sathya, the TDX Module spec should be updated to state that XSETBV will #GP at CPL!=0. If that's not already the behavior, the module should probably be changed...
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
> On Mar 30, 2021, at 8:14 AM, Sean Christopherson wrote: > > On Mon, Mar 29, 2021, Andy Lutomirski wrote: >> On Mar 29, 2021, at 7:04 PM, Andi Kleen wrote: >>> >>> > No, if these instructions take a #VE then they were executed at CPL=0. > MONITOR > and MWAIT will #UD without VM-Exit->#VE. Same for WBINVD, s/#UD/#GP. Dare I ask about XSETBV? >>> >>> XGETBV does not cause a #VE, it just works normally. The guest has full >>> AVX capabilities. >>> >> >> X *SET* BV > > Heh, XSETBV also works normally, relative to the features enumerated in CPUID. > XSAVES/XRSTORS support is fixed to '1' in the virtual CPU model. A subset of > the features managed by XSAVE can be hidden by the VMM, but attempting to > enable > unsupported features will #GP (either from hardware or injected by TDX > Module), > not #VE. Normally in non-root mode means that every XSETBV results in a VM exit and, IIUC, there’s a buglet in that this happens even if CPL==3. Does something special happen in TDX or does the exit get reflected back to the guest as a #VE?
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On Mon, Mar 29, 2021, Andy Lutomirski wrote: > > > On Mar 29, 2021, at 7:04 PM, Andi Kleen wrote: > > > > > >> > >>> No, if these instructions take a #VE then they were executed at CPL=0. > >>> MONITOR > >>> and MWAIT will #UD without VM-Exit->#VE. Same for WBINVD, s/#UD/#GP. > >> > >> Dare I ask about XSETBV? > > > > XGETBV does not cause a #VE, it just works normally. The guest has full > > AVX capabilities. > > > > X *SET* BV Heh, XSETBV also works normally, relative to the features enumerated in CPUID. XSAVES/XRSTORS support is fixed to '1' in the virtual CPU model. A subset of the features managed by XSAVE can be hidden by the VMM, but attempting to enable unsupported features will #GP (either from hardware or injected by TDX Module), not #VE.
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
> On Mar 29, 2021, at 7:04 PM, Andi Kleen wrote: > > >> >>> No, if these instructions take a #VE then they were executed at CPL=0. >>> MONITOR >>> and MWAIT will #UD without VM-Exit->#VE. Same for WBINVD, s/#UD/#GP. >> >> Dare I ask about XSETBV? > > XGETBV does not cause a #VE, it just works normally. The guest has full > AVX capabilities. > X *SET* BV
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
> > No, if these instructions take a #VE then they were executed at CPL=0. > > MONITOR > > and MWAIT will #UD without VM-Exit->#VE. Same for WBINVD, s/#UD/#GP. > > Dare I ask about XSETBV? XGETBV does not cause a #VE, it just works normally. The guest has full AVX capabilities. -Andi
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On Mon, Mar 29, 2021 at 4:42 PM Sean Christopherson wrote: > > On Mon, Mar 29, 2021, Kuppuswamy, Sathyanarayanan wrote: > > > > > > On 3/29/21 4:23 PM, Andy Lutomirski wrote: > > > > > > > On Mar 29, 2021, at 4:17 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. > > > > > > Is there something I missed elsewhere in the code that checks CPL? > > We don't check for CPL explicitly. But if we are reaching here, then we > > executing these instructions with wrong CPL. > > No, if these instructions take a #VE then they were executed at CPL=0. > MONITOR > and MWAIT will #UD without VM-Exit->#VE. Same for WBINVD, s/#UD/#GP. Dare I ask about XSETBV?
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On Mon, Mar 29, 2021, Kuppuswamy, Sathyanarayanan wrote: > > > On 3/29/21 4:23 PM, Andy Lutomirski wrote: > > > > > On Mar 29, 2021, at 4:17 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. > > > > Is there something I missed elsewhere in the code that checks CPL? > We don't check for CPL explicitly. But if we are reaching here, then we > executing these instructions with wrong CPL. No, if these instructions take a #VE then they were executed at CPL=0. MONITOR and MWAIT will #UD without VM-Exit->#VE. Same for WBINVD, s/#UD/#GP.
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On Mon, Mar 29, 2021, Andy Lutomirski wrote: > > > On Mar 29, 2021, at 4:17 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. > > Is there something I missed elsewhere in the code that checks CPL? #GP due to CPL!=0 has priority over VM-Exit, i.e. userspace will get a #GP directly; there will be no VM-Exit to the TDX Module and thus no #VE. SDM section "25.1.1 - Relative Priority of Faults and VM Exits".
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
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().
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On 3/29/21 4:23 PM, Andy Lutomirski wrote: On Mar 29, 2021, at 4:17 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. Is there something I missed elsewhere in the code that checks CPL? We don't check for CPL explicitly. But if we are reaching here, then we executing these instructions with wrong CPL. -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
Re: [PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
> On Mar 29, 2021, at 4:17 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. Is there something I missed elsewhere in the code that checks CPL?
[PATCH v3 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
In non-root TDX guest mode, MWAIT, MONITOR and WBINVD instructions
are not supported. So handle #VE due to these instructions
appropriately.
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.
Signed-off-by: Kuppuswamy Sathyanarayanan
Reviewed-by: Andi Kleen
---
Changes since v2:
* Added BUG() for WBINVD, WARN for MONITOR instructions.
* Fixed comments as per Dave's review.
Changes since v1:
* Added WARN() for MWAIT #VE exception.
Changes since previous series:
* Suppressed MWAIT feature as per Andi's comment.
* Added warning debug log for MWAIT #VE exception.
arch/x86/kernel/tdx.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index e936b2f88bf6..4c6336a055a3 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -308,6 +308,9 @@ void __init tdx_early_init(void)
setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
+ /* MWAIT is not supported in TDX platform, so suppress it */
+ setup_clear_cpu_cap(X86_FEATURE_MWAIT);
+
tdg_get_info();
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");
+ 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;
--
2.25.1
