Messages in this thread Patch in this message | | | Date | Tue, 28 Dec 2021 20:08:49 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH 05/26] x86/tdx: Add HLT support for TDX guests (#VE approach) |
| |
On Tue, Dec 14, 2021 at 06:02:43PM +0300, Kirill A. Shutemov wrote: > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > index e9ee8b526319..273e4266b2c1 100644 > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -46,6 +46,7 @@ > #include <asm/proto.h> > #include <asm/frame.h> > #include <asm/unwind.h> > +#include <asm/tdx.h> > > #include "process.h" > > @@ -864,6 +865,12 @@ void select_idle_routine(const struct cpuinfo_x86 *c) > if (x86_idle || boot_option_idle_override == IDLE_POLL) > return; > > + if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { > + x86_idle = tdx_guest_idle; > + pr_info("using TDX aware idle routine\n"); > + return; > + }
Why isn't this part of the following if-else if-else if... noodle?
> + > if (boot_cpu_has_bug(X86_BUG_AMD_E400)) { > pr_info("using AMD E400 aware idle routine\n"); > x86_idle = amd_e400_idle;
This one starting here: ^^^^
> diff --git a/arch/x86/kernel/tdcall.S b/arch/x86/kernel/tdcall.S > index ee52dde01b24..e19187048be8 100644 > --- a/arch/x86/kernel/tdcall.S > +++ b/arch/x86/kernel/tdcall.S > @@ -3,6 +3,7 @@ > #include <asm/asm.h> > #include <asm/frame.h> > #include <asm/unwind_hints.h> > +#include <uapi/asm/vmx.h> > > #include <linux/linkage.h> > #include <linux/bits.h> > @@ -39,6 +40,13 @@ > */ > #define tdcall .byte 0x66,0x0f,0x01,0xcc > > +/* > + * Used in the __tdx_hypercall() function to test R15 register content > + * and optionally include the STI instruction before the TDCALL > + * instruction (for EXIT_REASON_HLT case).
"Used in __tdx_hypercall() to determine whether to enable interrupts before issuing TDCALL for the EXIT_REASON_HLT case."
Plain and simple.
> + */ > +#define do_sti 0x01
#define ENABLE_IRQS_BEFORE_HLT 0x01
and when you call it that, you don't even need the comment above it because the name is self-explanatory.
> + > /* > * __tdx_module_call() - Used by TDX guests to request services from > * the TDX module (does not include VMM services). > @@ -231,6 +239,30 @@ 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, add > + * support to conditionally call STI before TDCALL.
"add support"?
> + * > + * Since STI instruction is only required for the idle case > + * (a special case of EXIT_REASON_HLT), use the r15 register > + * value to identify it. Since the R15 register is not used > + * by the VMM as per EXIT_REASON_HLT ABI, re-use it in > + * software to identify the STI case. > + */ > + cmpl $EXIT_REASON_HLT, %r11d > + jne skip_sti > + cmpl $do_sti, %r15d > + jne skip_sti > + /* Set R15 register to 0, it is unused in EXIT_REASON_HLT case */ > + xor %r15, %r15 > + sti > +skip_sti:
.Lskip_sti:
> tdcall > > /* Restore output pointer to R9 */ > diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c > index b6d0e45e6589..6749ca3b2e3d 100644 > --- a/arch/x86/kernel/tdx.c > +++ b/arch/x86/kernel/tdx.c > @@ -6,6 +6,7 @@ > > #include <linux/cpufeature.h> > #include <asm/tdx.h> > +#include <asm/vmx.h> > > /* TDX Module Call Leaf IDs */ > #define TDX_GET_VEINFO 3
...
> +void __cpuidle tdx_guest_idle(void) > +{ > + tdx_safe_halt(); > +}
That wrapper looks useless...
-- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
| |