On Thu, Jan 30, 2025 at 1:28 AM Kirill A. Shutemov [off-list ref] wrote:

On Wed, Jan 29, 2025 at 11:25:25PM +0000, Vishal Annapurve wrote:

quoted

Direct HLT instruction execution causes #VEs for TDX VMs which is routed
to hypervisor via tdvmcall. This process renders HLT instruction
execution inatomic, so any preceding instructions like STI/MOV SS will
end up enabling interrupts before the HLT instruction is routed to the
hypervisor. This creates scenarios where interrupts could land during
HLT instruction emulation without aborting halt operation leading to
idefinite halt wait times.

Commit bfe6ed0c6727 ("x86/tdx: Add HLT support for TDX guests") already
upgraded x86_idle() to invoke tdvmcall to avoid such scenarios, but
it didn't cover pv_native_safe_halt() which can be invoked using
raw_safe_halt() from call sites like acpi_safe_halt().

raw_safe_halt() also returns with interrupts enabled so upgrade
tdx_safe_halt() to enable interrupts by default and ensure that paravirt
safe_halt() executions invoke tdx_safe_halt(). Earlier x86_idle() is now
handled via tdx_idle() which simply invokes tdvmcall while preserving
irq state.

To avoid future call sites which cause HLT instruction emulation with
irqs enabled, add a warn and fail the HLT instruction emulation.

Cc: stable@vger.kernel.org
Fixes: bfe6ed0c6727 ("x86/tdx: Add HLT support for TDX guests")
Signed-off-by: Vishal Annapurve <redacted>
---
Changes since V1:
1) Addressed comments from Dave H
   - Comment regarding adding a check for TDX VMs in halt path is not
     resolved in v2, would like feedback around better place to do so,
     maybe in pv_native_safe_halt (?).
2) Added a new version of tdx_safe_halt() that will enable interrupts.
3) Previous tdx_safe_halt() implementation is moved to newly introduced
tdx_idle().

V1: https://lore.kernel.org/lkml/Z5l6L3Hen9_Y3SGC@google.com/T/ (local)

 arch/x86/coco/tdx/tdx.c    | 23 ++++++++++++++++++++++-
 arch/x86/include/asm/tdx.h |  2 +-
 arch/x86/kernel/process.c  |  2 +-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 0d9b090b4880..cc2a637dca15 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c

@@ -14,6 +14,7 @@
 #include <asm/ia32.h>
 #include <asm/insn.h>
 #include <asm/insn-eval.h>
+#include <asm/paravirt_types.h>
 #include <asm/pgtable.h>
 #include <asm/set_memory.h>
 #include <asm/traps.h>

@@ -380,13 +381,18 @@ static int handle_halt(struct ve_info *ve)
 {
      const bool irq_disabled = irqs_disabled();

+     if (!irq_disabled) {
+             WARN_ONCE(1, "HLT instruction emulation unsafe with irqs enabled\n");
+             return -EIO;
+     }
+

I think it is worth to putting this into a separate patch and not
backport. The rest of the patch is bugfix and this doesn't belong.

Otherwise, looks good to me:

Reviewed-by: Kirill A. Shutemov <redacted>@linux.intel.com>

--
  Kiryl Shutsemau / Kirill A. Shutemov

Thanks Kirill for the review.

Thinking more about this fix, now I am wondering why the efforts [1]
to move halt/safe_halt under CONFIG_PARAVIRT were abandoned. Currently
proposed fix is incomplete as it would not handle scenarios where
CONFIG_PARAVIRT_XXL is disabled. I am tilting towards reviving [1] and
requiring CONFIG_PARAVIRT for TDX VMs. WDYT?

[1] https://lore.kernel.org/lkml/20210517235008.257241-1-sathyanarayanan.kuppuswamy@linux.intel.com/ (local)