Messages in this thread | | | From | Dan Williams <> | Date | Mon, 10 May 2021 18:23:41 -0700 | Subject | Re: [RFC v2 16/32] x86/tdx: Handle MWAIT, MONITOR and WBINVD |
| |
On Mon, Apr 26, 2021 at 11:02 AM Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > 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. > > You can find the list of unsupported instructions in Intel > Trust Domain Extensions (Intel® TDX) Module specification, > sec 9.2.2 and in Guest-Host Communication Interface (GHCI) > Specification for Intel TDX, sec 2.4.1. >
Ah, better than the "handle port io" patch, these details at least give the reader a chance.
> To prevent TD guest from using 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.
Why does this not result in a #UD if the instruction is disabled by SEAM? How is it possible to execute a disabled instruction (one precluded by CPUID) to the point where it triggers #VE instead of #UD?
> After the above mentioned preventive measures, if TD guests still > execute these instructions, add appropriate warning messages in #VE > handler. For WBIND instruction, since it's related to memory writeback > and cache flushes, it's mainly used in context of IO devices. Since > TDX 1.0 does not support non-virtual I/O devices, skipping it should > not cause any fatal issues.
WBINVD is in a different class than MWAIT/MONITOR since it is not identified by CPUID, it can't possibly have the same #UD behaviour. It's not clear why WBINVD is included in the same patch as MWAIT/MONITOR?
I disagree with the assertion that WBINVD is mainly used in the context of I/O devices, it's also used for ACPI power management paths. WBINVD dependent functionality should be dynamically disabled rather than warned about.
Does a TDX guest support out-of-tree modules? The kernel is already tainted when out-of-tree modules are loaded. In other words in-tree modules preclude forbidden instructions because they can just be audited, and out-of-tree modules are ok to trigger abrupt failure if they attempt to use forbidden instructions.
> But to let users know about its usage, use > WARN() to report about it.. For MWAIT/MONITOR instruction, since its > unsupported use WARN() to report unsupported usage.
I'm not sure how useful warning is outside of a kernel developer's debug environment. The kernel should know what instructions are disabled and which are available. WBINVD in particular has potential data integrity implications. Code that might lead to a WBINVD usage should be disabled, not run all the way up to where WBINVD is attempted and then trigger an after-the-fact WARN_ONCE().
The WBINVD change deserves to be split off from MWAIT/MONITOR, and more thought needs to be put into where these spurious instruction usages are arising.
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Reviewed-by: Andi Kleen <ak@linux.intel.com> > --- > arch/x86/kernel/tdx.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c > index 3fe617978fc4..294dda5bf3f6 100644 > --- a/arch/x86/kernel/tdx.c > +++ b/arch/x86/kernel/tdx.c > @@ -371,6 +371,21 @@ 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 not supported inside TDX guests. All in- > + * kernel uses should have been disabled. > + */ > + WARN_ONCE(1, "TD Guest used unsupported WBINVD instruction\n"); > + break; > + case EXIT_REASON_MONITOR_INSTRUCTION: > + case EXIT_REASON_MWAIT_INSTRUCTION: > + /* > + * Something in the kernel used MONITOR or MWAIT despite > + * X86_FEATURE_MWAIT being cleared for TDX guests. > + */ > + WARN_ONCE(1, "TD Guest used unsupported MWAIT/MONITOR instruction\n"); > + break; > default: > pr_warn("Unexpected #VE: %lld\n", ve->exit_reason); > return -EFAULT; > -- > 2.25.1 >
| |