Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <56f368c6-4a60-ea78-2cc7-cd2d57823e3a@intel.com>
Date:   Tue, 26 Apr 2022 13:37:39 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v3 03/21] x86/virt/tdx: Implement the SEAMCALL base
 function
Content-Language: en-US
To:     Kai Huang <kai.huang@intel.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     seanjc@google.com, pbonzini@redhat.com, len.brown@intel.com,
        tony.luck@intel.com, rafael.j.wysocki@intel.com,
        reinette.chatre@intel.com, dan.j.williams@intel.com,
        peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        isaku.yamahata@intel.com
References: <cover.1649219184.git.kai.huang@intel.com>
 <1c3f555934c73301a9cbf10232500f3d15efe3cc.1649219184.git.kai.huang@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <1c3f555934c73301a9cbf10232500f3d15efe3cc.1649219184.git.kai.huang@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/5/22 21:49, Kai Huang wrote:
> Secure Arbitration Mode (SEAM) is an extension of VMX architecture.  It
> defines a new VMX root operation (SEAM VMX root) and a new VMX non-root
> operation (SEAM VMX non-root) which are isolated from legacy VMX root
> and VMX non-root mode.

I feel like this is too much detail for an opening paragraph.

> A CPU-attested software module (called the 'TDX module') runs in SEAM
> VMX root to manage the crypto-protected VMs running in SEAM VMX non-root.
> SEAM VMX root is also used to host another CPU-attested software module
> (called the 'P-SEAMLDR') to load and update the TDX module.
>> Host kernel transits to either the P-SEAMLDR or the TDX module via the
> new SEAMCALL instruction.  SEAMCALL leaf functions are host-side
> interface functions defined by the P-SEAMLDR and the TDX module around
> the new SEAMCALL instruction.  They are similar to a hypercall, except
> they are made by host kernel to the SEAM software.

I think you can get rid of about half of this changelog so farand make
it more clear in the process with this:

	TDX introduces a new CPU mode: Secure Arbitration Mode (SEAM).
	This mode runs only the TDX module itself or other code needed
	to load the TDX module.

	The host kernel communicates with SEAM software via a new
	SEAMCALL instruction.  This is conceptually similar to
	a guest->host hypercall, except it is made from the host to SEAM
	software instead.

This is a technical document, but you're writing too technically for my
taste and focusing on the low-level details rather than the high-level
concepts.  What do I care that SEAM is two modes and what their names
are at this juncture?  Are those details necesarry to get me to
understand what a SEAMCALL is or what this patch implements?

> SEAMCALL leaf functions use an ABI different from the x86-64 system-v
> ABI.  Instead, they share the same ABI with the TDCALL leaf functions.
> %rax is used to carry both the SEAMCALL leaf function number (input) and
> the completion status code (output).  Additional GPRs (%rcx, %rdx,
> %r8->%r11) may be further used as both input and output operands in
> individual leaf functions.
> 
> Implement a C function __seamcall()

Your "C function" looks a bit like assembly to me.

> to do SEAMCALL leaf functions using
> the assembly macro used by __tdx_module_call() (the implementation of
> TDCALL leaf functions).  The only exception not covered here is TDENTER
> leaf function which takes all GPRs and XMM0-XMM15 as both input and
> output.  The caller of TDENTER should implement its own logic to call
> TDENTER directly instead of using this function.

I have no idea why this paragraph is here or what it is trying to tell me.

> SEAMCALL instruction is essentially a VMExit from VMX root to SEAM VMX
> root, and it can fail with VMfailInvalid, for instance, when the SEAM
> software module is not loaded.  The C function __seamcall() returns
> TDX_SEAMCALL_VMFAILINVALID, which doesn't conflict with any actual error
> code of SEAMCALLs, to uniquely represent this case.

Again, I'm lost.  Why is this detail here?  I don't even see
TDX_SEAMCALL_VMFAILINVALID in the patch.

> diff --git a/arch/x86/virt/vmx/tdx/Makefile b/arch/x86/virt/vmx/tdx/Makefile
> index 1bd688684716..fd577619620e 100644
> --- a/arch/x86/virt/vmx/tdx/Makefile
> +++ b/arch/x86/virt/vmx/tdx/Makefile
> @@ -1,2 +1,2 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -obj-$(CONFIG_INTEL_TDX_HOST)	+= tdx.o
> +obj-$(CONFIG_INTEL_TDX_HOST)	+= tdx.o seamcall.o
> diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
> new file mode 100644
> index 000000000000..327961b2dd5a
> --- /dev/null
> +++ b/arch/x86/virt/vmx/tdx/seamcall.S
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/linkage.h>
> +#include <asm/frame.h>
> +
> +#include "tdxcall.S"
> +
> +/*
> + * __seamcall()  - Host-side interface functions to SEAM software module
> + *		   (the P-SEAMLDR or the TDX module)
> + *
> + * Transform function call register arguments into the SEAMCALL register
> + * ABI.  Return TDX_SEAMCALL_VMFAILINVALID, or the completion status of
> + * the SEAMCALL.  Additional output operands are saved in @out (if it is
> + * provided by caller).

This needs to say:

	Returns TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself fails.

> + *-------------------------------------------------------------------------
> + * SEAMCALL ABI:
> + *-------------------------------------------------------------------------
> + * Input Registers:
> + *
> + * RAX                 - SEAMCALL Leaf number.
> + * RCX,RDX,R8-R9       - SEAMCALL Leaf specific input registers.
> + *
> + * Output Registers:
> + *
> + * RAX                 - SEAMCALL completion status code.
> + * RCX,RDX,R8-R11      - SEAMCALL Leaf specific output registers.
> + *
> + *-------------------------------------------------------------------------
> + *
> + * __seamcall() function ABI:
> + *
> + * @fn  (RDI)          - SEAMCALL Leaf number, moved to RAX
> + * @rcx (RSI)          - Input parameter 1, moved to RCX
> + * @rdx (RDX)          - Input parameter 2, moved to RDX
> + * @r8  (RCX)          - Input parameter 3, moved to R8
> + * @r9  (R8)           - Input parameter 4, moved to R9
> + *
> + * @out (R9)           - struct tdx_module_output pointer
> + *			 stored temporarily in R12 (not
> + *			 used by the P-SEAMLDR or the TDX
> + *			 module). It can be NULL.
> + *
> + * Return (via RAX) the completion status of the SEAMCALL, or
> + * TDX_SEAMCALL_VMFAILINVALID.
> + */
> +SYM_FUNC_START(__seamcall)
> +	FRAME_BEGIN
> +	TDX_MODULE_CALL host=1
> +	FRAME_END
> +	ret
> +SYM_FUNC_END(__seamcall)
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> new file mode 100644
> index 000000000000..9d5b6f554c20
> --- /dev/null
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _X86_VIRT_TDX_H
> +#define _X86_VIRT_TDX_H
> +
> +#include <linux/types.h>
> +
> +struct tdx_module_output;
> +u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> +	       struct tdx_module_output *out);
> +
> +#endif