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

From: Sean Christopherson <seanjc@google.com>
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	bp@alien8.de, aarcange@redhat.com, ak@linux.intel.com,
	dan.j.williams@intel.com, dave.hansen@intel.com,
	david@redhat.com, hpa@zytor.com, jgross@suse.com,
	jmattson@google.com, joro@8bytes.org, jpoimboe@redhat.com,
	knsathya@kernel.org, linux-kernel@vger.kernel.org,
	luto@kernel.org, mingo@redhat.com, pbonzini@redhat.com,
	peterz@infradead.org, sathyanarayanan.kuppuswamy@linux.intel.com,
	sdeep@vmware.com, tony.luck@intel.com, vkuznets@redhat.com,
	wanpengli@tencent.com, x86@kernel.org
Subject: Re: [PATCHv2.1 05/29] x86/tdx: Add HLT support for TDX guests
Date: Mon, 7 Feb 2022 22:52:19 +0000	[thread overview]
Message-ID: <YgGioxo4hnJBJUgT@google.com> (raw)
In-Reply-To: <20220204165539.oqw7bj3ri4hzjiy6@black.fi.intel.com>

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)
