Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Feb 2022 17:44:03 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
To:     Kai Huang <kai.huang@intel.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>, mingo@redhat.com,
        bp@alien8.de, dave.hansen@intel.com, luto@kernel.org,
        peterz@infradead.org, sathyanarayanan.kuppuswamy@linux.intel.com,
        aarcange@redhat.com, ak@linux.intel.com, dan.j.williams@intel.com,
        david@redhat.com, hpa@zytor.com, jgross@suse.com,
        jmattson@google.com, joro@8bytes.org, jpoimboe@redhat.com,
        knsathya@kernel.org, pbonzini@redhat.com, sdeep@vmware.com,
        seanjc@google.com, tony.luck@intel.com, vkuznets@redhat.com,
        wanpengli@tencent.com, x86@kernel.org,
        linux-kernel@vger.kernel.org, Kai Huang <kai.huang@linux.intel.com>
Subject: Re: [PATCHv2 03/29] x86/tdx: Add __tdx_module_call() and
 __tdx_hypercall() helper functions
Message-ID: <20220203144403.ytyw5vcv4gshxyhn@black.fi.intel.com>
References: <20220124150215.36893-1-kirill.shutemov@linux.intel.com>
 <20220124150215.36893-4-kirill.shutemov@linux.intel.com>
 <87a6faz7cs.ffs@tglx>
 <20220202025519.csymm7r23b3ltpia@black.fi.intel.com>
 <20220202235910.c512857813ea5b14aa73267f@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220202235910.c512857813ea5b14aa73267f@intel.com>
Precedence: bulk

On Wed, Feb 02, 2022 at 11:59:10PM +1300, Kai Huang wrote:
> 
> >  /*
> > diff --git a/arch/x86/kernel/tdxcall.S b/arch/x86/kernel/tdxcall.S
> > new file mode 100644
> > index 000000000000..ba5e8e35de36
> > --- /dev/null
> > +++ b/arch/x86/kernel/tdxcall.S
> > @@ -0,0 +1,76 @@
> > +#include <asm/asm-offsets.h>
> > +
> > +/*
> > + * TDX guests use the TDCALL instruction to make requests to the
> > + * TDX module and hypercalls to the VMM.
> > + *
> > + * TDX host user SEAMCALL instruction to make requests to TDX module.
> > + *
> > + * They are supported in Binutils >= 2.36.
> > + */
> > +#define tdcall		.byte 0x66,0x0f,0x01,0xcc
> > +#define seamcall	.byte 0x66,0x0f,0x01,0xcf
> > +
> > +.macro TDX_MODULE_CALL host:req
> > +	/*
> > +	 * R12 will be used as temporary storage for struct tdx_module_output
> > +	 * pointer. Since R12-R15 registers are not used by TDCALL/SEAMCALL
> > +	 * services supported by this function, it can be reused.
> > +	 */
> > +
> > +	/* Callee saved, so preserve it */
> > +	push %r12
> > +
> > +	/*
> > +	 * Push output pointer to stack.
> > +	 * After the operation, it will be fetched into R12 register.
> > +	 */
> > +	push %r9
> > +
> > +	/* Mangle function call ABI into TDCALL/SEAMCALL ABI: */
> > +	/* Move Leaf ID to RAX */
> > +	mov %rdi, %rax
> > +	/* Move input 4 to R9 */
> > +	mov %r8,  %r9
> > +	/* Move input 3 to R8 */
> > +	mov %rcx, %r8
> > +	/* Move input 1 to RCX */
> > +	mov %rsi, %rcx
> > +	/* Leave input param 2 in RDX */
> 
> Currently host seamcall also uses a data structure which has all possible input
> registers as input argument, rather than having one argument for each register:
> 
> 	struct seamcall_regs_in {
> 		u64 rcx;
> 		u64 rdx;
> 		u64 r8;
> 		u64 r9;
> 	};
> 
> Which way is better (above struct name can be changed of course)?
> 
> Or should we rename tdx_module_output to tdx_module_regs, and use it as both
> input and output (similar to __tdx_hypercall())?

Unlike hypercall case, here we have more managable number of arguments.
I would rather keep input arguments outside of any structs. It is easier
for callers, IMO.

Any objections?

> > +
> > +	.if \host
> > +	seamcall
> > +	.else
> > +	tdcall
> > +	.endif
> > +
> > +	/*
> > +	 * Fetch output pointer from stack to R12 (It is used
> > +	 * as temporary storage)
> > +	 */
> > +	pop %r12
> 
> For host SEAMCALL, one additional check against VMfailInvalid needs to be
> done after here.  This is because at host side, both P-SEAMLDR and TDX module
> are expected to be loaded by UEFI (i.e. UEFI shell tool) before booting to the
> kernel, therefore host kernel needs to detect whether them have been loaded, by
> issuing SEAMCALL.
> 
> When SEAM software (P-SEAMLDR or TDX module) is not loaded, the SEAMCALL fails
> with VMfailInvalid.  VMfailInvalid is indicated via a combination of RFLAGS
> rather than using %rax.  In practice, RFLAGS.CF=1 can be used to check whether
> VMfailInvalid happened, and we need to have something like below:
> 
> 	/*
> 	 * SEAMCALL instruction is essentially a VMExit from VMX root
> 	 * mode to SEAM VMX root mode.  VMfailInvalid (CF=1) indicates
> 	 * that the targeted SEAM firmware is not loaded or disabled,
> 	 * or P-SEAMLDR is busy with another SEAMCALL.  %rax is not
> 	 * changed in this case.
> 	 *
> 	 * Set %rax to VMFAILINVALID for VMfailInvalid.  This value
> 	 * will never be used as actual SEAMCALL error code.
> 	 */
> 	jnb     .Lno_vmfailinvalid
> 	mov     $(VMFAILINVALID), %rax
> 	jmp     .Lno_output_struct
> 
> .Lno_vmfailinvalid:

Okay, I will add it under .if \host.

But maybe use JNC instead of JNB? Since we check for CF flag,
Jump-if-Not-Carry sounds more reasonable than Jump-if-Not-Below.
Not-Below of what?

-- 
 Kirill A. Shutemov