Re: [PATCH v1 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On 3/30/21 8:10 AM, Dave Hansen wrote: On 3/30/21 8:00 AM, Andi Kleen wrote: + /* MWAIT is not supported in TDX platform, so suppress it */ + setup_clear_cpu_cap(X86_FEATURE_MWAIT); In fact, MWAIT bit returned by CPUID instruction is zero for TD guest. This is enforced by SEAM module. Good point. Do we still need to safeguard it by setup_clear_cpu_cap() here? I guess it doesn't hurt to do it explicitly. If this MWAIT behavior (clearing the CPUID bit) is part of the guest architecture, then it would also be appropriate to WARN() rather than silently clearing the X86_FEATURE bit. Makes sense. It will be useful to find the problem with TDX Module. -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
Re: [PATCH v1 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On 3/30/21 8:00 AM, Andi Kleen wrote: >>> + /* MWAIT is not supported in TDX platform, so suppress it */ >>> + setup_clear_cpu_cap(X86_FEATURE_MWAIT); >> In fact, MWAIT bit returned by CPUID instruction is zero for TD guest. This >> is enforced by SEAM module. > Good point. >> Do we still need to safeguard it by setup_clear_cpu_cap() here? > I guess it doesn't hurt to do it explicitly. If this MWAIT behavior (clearing the CPUID bit) is part of the guest architecture, then it would also be appropriate to WARN() rather than silently clearing the X86_FEATURE bit.
Re: [PATCH v1 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On Tue, Mar 30, 2021 at 12:56:41PM +0800, Xiaoyao Li wrote: > On 3/27/2021 8:18 AM, Kuppuswamy Sathyanarayanan wrote: > > In non-root TDX guest mode, MWAIT, MONITOR and WBINVD instructions > > are not supported. So handle #VE due to these instructions as no ops. > > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > Reviewed-by: Andi Kleen > > --- > > > > 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 | 23 +++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c > > index e936b2f88bf6..fb7d22b846fc 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); > > In fact, MWAIT bit returned by CPUID instruction is zero for TD guest. This > is enforced by SEAM module. Good point. > > Do we still need to safeguard it by setup_clear_cpu_cap() here? I guess it doesn't hurt to do it explicitly. -Andi
Re: [PATCH v1 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On 3/27/2021 8:18 AM, Kuppuswamy Sathyanarayanan wrote: In non-root TDX guest mode, MWAIT, MONITOR and WBINVD instructions are not supported. So handle #VE due to these instructions as no ops. Signed-off-by: Kuppuswamy Sathyanarayanan Reviewed-by: Andi Kleen --- 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 | 23 +++ 1 file changed, 23 insertions(+) diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c index e936b2f88bf6..fb7d22b846fc 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); In fact, MWAIT bit returned by CPUID instruction is zero for TD guest. This is enforced by SEAM module. Do we still need to safeguard it by setup_clear_cpu_cap() here? + tdg_get_info(); pv_ops.irq.safe_halt = tdg_safe_halt;
Re: [PATCH v1 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
> On Mar 26, 2021, at 8:40 PM, Kuppuswamy, Sathyanarayanan > wrote: > > > > On 3/26/21 7:40 PM, Andy Lutomirski wrote: On Mar 26, 2021, at 5:18 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 as no ops. >> These should at least be WARN. > I will change it to WARN. >> Does TDX send #UD if these instructions have the wrong CPL? > No, TDX does not trigger #UD for these instructions. > If the #VE came from user mode, we should send an appropriate signal instead. > It will be mapped into #GP(0) fault. This should be enough notification right? Yes. And I did mean #GP, not #UD.
Re: [PATCH v1 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
On 3/26/21 7:40 PM, Andy Lutomirski wrote:
On Mar 26, 2021, at 5:18 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 as no ops.
These should at least be WARN.
I will change it to WARN.
Does TDX send #UD if these instructions have the wrong CPL?
No, TDX does not trigger #UD for these instructions.
If the #VE came from user mode, we should send an appropriate signal instead.
It will be mapped into #GP(0) fault. This should be enough notification right?
Signed-off-by: Kuppuswamy Sathyanarayanan
Reviewed-by: Andi Kleen
---
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 | 23 +++
1 file changed, 23 insertions(+)
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index e936b2f88bf6..fb7d22b846fc 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,26 @@ int tdg_handle_virtualization_exception(struct pt_regs
*regs,
case EXIT_REASON_EPT_VIOLATION:
ve->instr_len = tdg_handle_mmio(regs, ve);
break;
+/*
+ * 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 ] 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.
+ */
+case EXIT_REASON_WBINVD:
+pr_warn_once("WBINVD #VE Exception\n");
+case EXIT_REASON_MONITOR_INSTRUCTION:
+/* Handle as nops. */
+break;
+case EXIT_REASON_MWAIT_INSTRUCTION:
+/* MWAIT is supressed, not supposed to reach here. */
+pr_warn("MWAIT unexpected #VE Exception\n");
+return -EFAULT;
default:
pr_warn("Unexpected #VE: %d\n", ve->exit_reason);
return -EFAULT;
--
2.25.1
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Re: [PATCH v1 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD
> On Mar 26, 2021, at 5:18 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 as no ops.
These should at least be WARN.
Does TDX send #UD if these instructions have the wrong CPL? If the #VE came
from user mode, we should send an appropriate signal instead.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> Reviewed-by: Andi Kleen
> ---
>
> 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 | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> index e936b2f88bf6..fb7d22b846fc 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,26 @@ int tdg_handle_virtualization_exception(struct pt_regs
> *regs,
>case EXIT_REASON_EPT_VIOLATION:
>ve->instr_len = tdg_handle_mmio(regs, ve);
>break;
> +/*
> + * 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 ] 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.
> + */
> +case EXIT_REASON_WBINVD:
> +pr_warn_once("WBINVD #VE Exception\n");
> +case EXIT_REASON_MONITOR_INSTRUCTION:
> +/* Handle as nops. */
> +break;
> +case EXIT_REASON_MWAIT_INSTRUCTION:
> +/* MWAIT is supressed, not supposed to reach here. */
> +pr_warn("MWAIT unexpected #VE Exception\n");
> +return -EFAULT;
>default:
>pr_warn("Unexpected #VE: %d\n", ve->exit_reason);
>return -EFAULT;
> --
> 2.25.1
>
[PATCH v1 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 as no ops.
Signed-off-by: Kuppuswamy Sathyanarayanan
Reviewed-by: Andi Kleen
---
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 | 23 +++
1 file changed, 23 insertions(+)
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index e936b2f88bf6..fb7d22b846fc 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,26 @@ int tdg_handle_virtualization_exception(struct pt_regs
*regs,
case EXIT_REASON_EPT_VIOLATION:
ve->instr_len = tdg_handle_mmio(regs, ve);
break;
+ /*
+* 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 ] 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.
+*/
+ case EXIT_REASON_WBINVD:
+ pr_warn_once("WBINVD #VE Exception\n");
+ case EXIT_REASON_MONITOR_INSTRUCTION:
+ /* Handle as nops. */
+ break;
+ case EXIT_REASON_MWAIT_INSTRUCTION:
+ /* MWAIT is supressed, not supposed to reach here. */
+ pr_warn("MWAIT unexpected #VE Exception\n");
+ return -EFAULT;
default:
pr_warn("Unexpected #VE: %d\n", ve->exit_reason);
return -EFAULT;
--
2.25.1
