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

[Kirill A. Shutemov]

On Wed, Feb 02, 2022 at 06:17:08PM +0100, Thomas Gleixner wrote:
> On Wed, Feb 02 2022 at 15:48, Kirill A. Shutemov wrote:
> 
> > On Tue, Feb 01, 2022 at 10:21:58PM +0100, Thomas Gleixner wrote:
> >> On Sun, Jan 30 2022 at 01:30, Kirill A. Shutemov wrote:
> >> This really can be simplified:
> >> 
> >>         cmpl	$EXIT_REASON_SAFE_HLT, %r11d
> >>         jne	.Lnohalt
> >>         movl	$EXIT_REASON_HLT, %r11d
> >>         sti
> >> .Lnohalt:
> >> 	tdcall
> >> 
> >> and the below becomes:
> >> 
> >> static bool tdx_halt(void)
> >> {
> >> 	return !!__tdx_hypercall(EXIT_REASON_HLT, !!irqs_disabled(), 0, 0, 0, NULL);
> >> }
> >> 
> >> void __cpuidle tdx_safe_halt(void)
> >> {
> >>         if (__tdx_hypercall(EXIT_REASON_SAFE_HLT, 0, 0, 0, 0, NULL)
> >>         	WARN_ONCE(1, "HLT instruction emulation failed\n");
> >> }
> >> 
> >> Hmm?
> >
> > EXIT_REASON_* are architectural, see SDM vol 3D, appendix C. There's no
> > EXIT_REASON_SAFE_HLT.
> >
> > Do you want to define a synthetic one? Like
> >
> > #define EXIT_REASON_SAFE_HLT	0x10000
> > ?
> 
> That was my idea, yes.
> 
> > 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 :)
> 
> 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.

I would rather keep assembly side simple: shuffle values from the struct
to registers and back. C side is resposible for making sense of the
registers.

With all this in mind the __tdx_hypercall() will boil down to

u64 __tdx_hypercall(struct tdx_hypercall_args *args, u64 flags);

with the only flag HCALL_ISSUE_STI. Is it what you want to see?

I personally don't see why flag is better than synthetic argument as we
have now.

-- 
 Kirill A. Shutemov