Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <de53b1ddea591e2a6fc8ac0510e3ab59f5f58512.camel@intel.com>
Subject: Re: [PATCH v3 03/21] x86/virt/tdx: Implement the SEAMCALL base
 function
From:   Kai Huang <kai.huang@intel.com>
To:     Dave Hansen <dave.hansen@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
Date:   Wed, 27 Apr 2022 11:29:39 +1200
In-Reply-To: <56f368c6-4a60-ea78-2cc7-cd2d57823e3a@intel.com>
References: <cover.1649219184.git.kai.huang@intel.com>
         <1c3f555934c73301a9cbf10232500f3d15efe3cc.1649219184.git.kai.huang@intel.com>
         <56f368c6-4a60-ea78-2cc7-cd2d57823e3a@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-04-26 at 13:37 -0700, Dave Hansen wrote:
> 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.

Thank you!

> 
> 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?

Thanks for the point. I'll revisit this series based on this in next version.

> 
> > 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.

Will change to (I saw TDX guest patch used similar way):

	Add a generic interface to do SEAMCALL leaf functions, using the
	assembly macro used by __tdx_module_call().

> 
> > 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.

Will get rid of the rest staff.

> 
> > 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.

Will remove.

> 
> > 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.

OK.


-- 
Thanks,
-Kai