Re: [PATCHv4 08/30] x86/tdx: Add HLT support for TDX guests

Dave Hansen
2022-02-24T18:43:07Z

On 2/24/22 07:56, Kirill A. Shutemov wrote:
> The HLT instruction is a privileged instruction, executing it stops
> instruction execution and places the processor in a HALT state. It
> is used in kernel for cases like reboot, idle loop and exception fixup
> handlers. For the idle case, interrupts will be enabled (using STI)
> before the HLT instruction (this is also called safe_halt()).
>
> To support the HLT instruction in TDX guests, it needs to be emulated
> using TDVMCALL (hypercall to VMM). More details about it can be found
> in Intel Trust Domain Extensions (Intel TDX) Guest-Host-Communication
> Interface (GHCI) specification, section TDVMCALL[Instruction.HLT].
>
> In TDX guests, executing HLT instruction will generate a #VE, which is
> used to emulate the HLT instruction. But #VE based emulation will not
> work for the safe_halt() flavor, because it requires STI instruction to
> be executed just before the TDCALL. Since idle loop is the only user of
> safe_halt() variant, handle it as a special case.
>
> To avoid *safe_halt() call in the idle function, define the
> tdx_guest_idle() and use it to override the "x86_idle" function pointer
> for a valid TDX guest.
>
> Alternative choices like PV ops have been considered for adding
> safe_halt() support. But it was rejected because HLT paravirt calls
> only exist under PARAVIRT_XXL, and enabling it in TDX guest just for
> safe_halt() use case is not worth the cost.

Thanks for all the history and background here.

> diff --git a/arch/x86/coco/tdcall.S b/arch/x86/coco/tdcall.S
> index c4dd9468e7d9..3c35a056974d 100644
> --- a/arch/x86/coco/tdcall.S
> +++ b/arch/x86/coco/tdcall.S
> @@ -138,6 +138,19 @@ SYM_FUNC_START(__tdx_hypercall)
>
> movl $TDVMCALL_EXPOSE_REGS_MASK, %ecx
>
> + /*
> + * For the idle loop STI needs to be called directly before the TDCALL
> + * that enters idle (EXIT_REASON_HLT case). STI instruction enables
> + * interrupts only one instruction later. If there is a window between
> + * STI and the instruction that emulates the HALT state, there is a
> + * chance for interrupts to happen in this window, which can delay the
> + * HLT operation indefinitely. Since this is the not the desired
> + * result, conditionally call STI before TDCALL.
> + */
> + testq $TDX_HCALL_ISSUE_STI, %rsi
> + jz .Lskip_sti
> + sti
> +.Lskip_sti:
> tdcall
>
> /*
> diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
> index 86a2f35e7308..0a2e6be0cdae 100644
> --- a/arch/x86/coco/tdx.c
> +++ b/arch/x86/coco/tdx.c
> @@ -7,6 +7,7 @@
> #include <linux/cpufeature.h>
> #include <asm/coco.h>
> #include <asm/tdx.h>
> +#include <asm/vmx.h>
>
> /* TDX module Call Leaf IDs */
> #define TDX_GET_INFO 1
> @@ -59,6 +60,62 @@ static void get_info(void)
> td_info.attributes = out.rdx;
> }
>
> +static u64 __cpuidle __halt(const bool irq_disabled, const bool do_sti)
> +{
> + struct tdx_hypercall_args args = {
> + .r10 = TDX_HYPERCALL_STANDARD,
> + .r11 = EXIT_REASON_HLT,
> + .r12 = irq_disabled,
> + };
> +
> + /*
> + * Emulate HLT operation via hypercall. More info about ABI
> + * can be found in TDX Guest-Host-Communication Interface
> + * (GHCI), section 3.8 TDG.VP.VMCALL<Instruction.HLT>.
> + *
> + * The VMM uses the "IRQ disabled" param to understand IRQ
> + * enabled status (RFLAGS.IF) of the TD guest and to determine
> + * whether or not it should schedule the halted vCPU if an
> + * IRQ becomes pending. E.g. if IRQs are disabled, the VMM
> + * can keep the vCPU in virtual HLT, even if an IRQ is
> + * pending, without hanging/breaking the guest.
> + */
> + return __tdx_hypercall(&args, do_sti ? TDX_HCALL_ISSUE_STI : 0);
> +}
> +
> +static bool handle_halt(void)
> +{
> + /*
> + * Since non safe halt is mainly used in CPU offlining
> + * and the guest will always stay in the halt state, don't
> + * call the STI instruction (set do_sti as false).
> + */
> + const bool irq_disabled = irqs_disabled();
> + const bool do_sti = false;
> +
> + if (__halt(irq_disabled, do_sti))
> + return false;
> +
> + return true;
> +}

One other note: I really do like the silly:

const bool do_sti = false;

variables as opposed to doing gunk like:

__halt(irq_disabled, false));

Thanks for doing that.

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>