Re: [PATCHv2.1 05/29] x86/tdx: Add HLT support for TDX guests

Sean Christopherson
2022-02-07T22:52:31Z

On Fri, Feb 04, 2022, Kirill A. Shutemov wrote:
> > > Looks dubious to me, I donno. I worry about possible conflicts with the
> > > spec in the future.
> >
> > The spec should have a reserved space for such things :)

Heh, the problem is someone has to deal with munging the two things together.
E.g. if there's a EXIT_REASON_SAFE_HLT then the hypervisor would need a handler
that's identical to EXIT_REASON_HLT, except with guest.EFLAGS.IF forced to '1'.
The guest gets the short end of the stick because EXIT_REASON_HLT is already an
established VM-Exit reason.

> > But you might think about having a in/out struct similar to the module
> > call or just an array of u64.
> >
> > and the signature would become:
> >
> > __tdx_hypercall(u64 op, u64 flags, struct inout *args)
> > __tdx_hypercall(u64 op, u64 flags, u64 *args)
> >
> > and have flag bits:
> >
> > HCALL_ISSUE_STI
> > HCALL_HAS_OUTPUT
> >
> > Hmm?
>
> We have two distinct cases: standard hypercalls (defined in GHCI) and KVM
> hypercalls. In the first case R10 is 0 (indicating standard TDVMCALL) and
> R11 defines the operation. For KVM hypercalls R10 encodes the operation
> (KVM hypercalls indexed from 1) and R11 is the first argument. So we
> cannot get away with simple "RDI is op" interface.
>
> And we need to return two values: RAX indicates if TDCALL itself was
> successful and R10 is result of the hypercall. So we cannot easily get
> away without output struct. HCALL_HAS_OUTPUT is not needed.

But __tdx_hypercall() should never fail TDCALL. The TDX spec even says:

RAX TDCALL instruction return code. Always returns Intel TDX_SUCCESS (0).

IIRC, the original PoC went straight to a ud2 if tdcall failed. Why not do
something similar? That would get rid of the bajillion instances of:

if (__tdx_hypercall(...))
panic("Hypercall fn %llu failed (Buggy TDX module!)\n", fn);

E.g.

diff --git a/arch/x86/kernel/tdcall.S b/arch/x86/kernel/tdcall.S
index fde628791100..04284f0c198e 100644
--- a/arch/x86/kernel/tdcall.S
+++ b/arch/x86/kernel/tdcall.S
@@ -170,8 +170,10 @@ SYM_FUNC_START(__tdx_hypercall)
.Lskip_sti:
tdcall

+ test %rax, %rax,
+ jnz .Lerror
+
/* Copy hypercall result registers to arg struct: */
- movq %r10, TDX_HYPERCALL_r10(%rdi)
movq %r11, TDX_HYPERCALL_r11(%rdi)
movq %r12, TDX_HYPERCALL_r12(%rdi)
movq %r13, TDX_HYPERCALL_r13(%rdi)
@@ -194,7 +196,13 @@ SYM_FUNC_START(__tdx_hypercall)
pop %r14
pop %r15

- FRAME_END
+ FRAME_END
+
+ movq %r10, %rax
+ retq
+
+.Lerror:
+ <move stuff into correct registers if necessary>
+ call tdx_hypercall_error

- retq
SYM_FUNC_END(__tdx_hypercall)