Hi Peter, Kirill, all,

This series unifies the assembly code for TDCALL/SEAMCALL and TDVMCALL.
Now all of them use one singe TDX_MODULE_CALL asm macro.

With this series, I have verified the TDX guest can boot successfully
and the TDX module can also be initialized successfully.

The last two patches are SEAMCALL patches that are needed for TDX host
patchset.  They are not mandatory to be here though, i.e., can be in the
TDX host support series.  I put them here so we can have a complete view
how TDCALL/SEAMCALL and TDVMCALL are implemented.

Could you help to review?  Thanks in advance.

(Removed KVM list -- Sean/Paolo/Isaku are still CC'ed.)

------- Histroy --------

v1 -> v2:
 - Rebased to 6.5-rc2.
 - Fixed comments from Peter and others.
 - Split patch "x86/tdx: Unify TDX_HYPERCALL and TDX_MODULE_CALL assembly"
   into three smaller patches for better review.
 - A new patch to skip saving output registers when SEAMCALL fails due to
   VMFailInvalid.  
 - Removed patch "x86/tdx: Use cmovc to save a label in TDX_MODULE_CALL asm"
 - Merged patch "x86/tdx: Move FRAME_BEGIN/END to TDX_MODULE_CALL asm macro"
   to the new patch mentioned above.

v1: https://lore.kernel.org/lkml/b95c4169-88c8-219e-87b7-6c4e058c246a@suse.com/T/


Kai Huang (11):
  x86/tdx: Zero out the missing RSI in TDX_HYPERCALL macro
  x86/tdx: Skip saving output regs when SEAMCALL fails with
    VMFailInvalid
  x86/tdx: Make macros of TDCALLs consistent with the spec
  x86/tdx: Rename __tdx_module_call() to __tdcall()
  x86/tdx: Pass TDCALL/SEAMCALL input/output registers via a structure
  x86/tdx: Extend TDX_MODULE_CALL to support more TDCALL/SEAMCALL leafs
  x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
  x86/tdx: Reimplement __tdx_hypercall() using TDX_MODULE_CALL asm
  x86/tdx: Remove 'struct tdx_hypercall_args'
  x86/virt/tdx: Wire up basic SEAMCALL functions
  x86/virt/tdx: Allow SEAMCALL to handle #UD and #GP

 arch/x86/Kconfig                  |  12 ++
 arch/x86/Makefile                 |   2 +
 arch/x86/boot/compressed/tdx.c    |   6 +-
 arch/x86/coco/tdx/tdcall.S        | 231 ++++--------------------------
 arch/x86/coco/tdx/tdx-shared.c    |  28 +++-
 arch/x86/coco/tdx/tdx.c           |  73 +++++-----
 arch/x86/include/asm/shared/tdx.h |  83 ++++++-----
 arch/x86/include/asm/tdx.h        |  10 ++
 arch/x86/kernel/asm-offsets.c     |  33 ++---
 arch/x86/virt/Makefile            |   2 +
 arch/x86/virt/vmx/Makefile        |   2 +
 arch/x86/virt/vmx/tdx/Makefile    |   2 +
 arch/x86/virt/vmx/tdx/seamcall.S  |  42 ++++++
 arch/x86/virt/vmx/tdx/tdxcall.S   | 227 ++++++++++++++++++++++-------
 14 files changed, 406 insertions(+), 347 deletions(-)
 create mode 100644 arch/x86/virt/Makefile
 create mode 100644 arch/x86/virt/vmx/Makefile
 create mode 100644 arch/x86/virt/vmx/tdx/Makefile
 create mode 100644 arch/x86/virt/vmx/tdx/seamcall.S


base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
-- 
2.41.0

On Fri, Jul 21, 2023 at 12:28:03AM +1200, Kai Huang wrote:

> Kai Huang (11):
>   x86/tdx: Zero out the missing RSI in TDX_HYPERCALL macro
>   x86/tdx: Skip saving output regs when SEAMCALL fails with
>     VMFailInvalid
>   x86/tdx: Make macros of TDCALLs consistent with the spec
>   x86/tdx: Rename __tdx_module_call() to __tdcall()
>   x86/tdx: Pass TDCALL/SEAMCALL input/output registers via a structure
>   x86/tdx: Extend TDX_MODULE_CALL to support more TDCALL/SEAMCALL leafs
>   x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
>   x86/tdx: Reimplement __tdx_hypercall() using TDX_MODULE_CALL asm
>   x86/tdx: Remove 'struct tdx_hypercall_args'
>   x86/virt/tdx: Wire up basic SEAMCALL functions
>   x86/virt/tdx: Allow SEAMCALL to handle #UD and #GP

These look ok to me, thanks!

This does not yet re-order the args structure to conform to the hardware
index order as per kvm's requirement, right? That will be part of the
KVM series?

On Thu, 2023-07-20 at 15:16 +0200, Peter Zijlstra wrote:
> On Fri, Jul 21, 2023 at 12:28:03AM +1200, Kai Huang wrote:
> 
> > Kai Huang (11):
> >   x86/tdx: Zero out the missing RSI in TDX_HYPERCALL macro
> >   x86/tdx: Skip saving output regs when SEAMCALL fails with
> >     VMFailInvalid
> >   x86/tdx: Make macros of TDCALLs consistent with the spec
> >   x86/tdx: Rename __tdx_module_call() to __tdcall()
> >   x86/tdx: Pass TDCALL/SEAMCALL input/output registers via a structure
> >   x86/tdx: Extend TDX_MODULE_CALL to support more TDCALL/SEAMCALL leafs
> >   x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
> >   x86/tdx: Reimplement __tdx_hypercall() using TDX_MODULE_CALL asm
> >   x86/tdx: Remove 'struct tdx_hypercall_args'
> >   x86/virt/tdx: Wire up basic SEAMCALL functions
> >   x86/virt/tdx: Allow SEAMCALL to handle #UD and #GP
> 
> These look ok to me, thanks!

Thanks!

> 
> This does not yet re-order the args structure to conform to the hardware
> index order as per kvm's requirement, right? That will be part of the
> KVM series?

Unfortunately I don't think it's feasible.  Sean pointed out that
kvm_vcpu_arch::regs[] do follow the "register index" hardware layout in x86 (for
which I missed sorry), so we cannot re-order KVM part.  

And unfortunately RBP (5) is in middle of those registers:

	0 = RAX
	1 = RCX
	2 = RDX
	3 = RBX
	4 = RSP
	5 = RBP
	6 = RSI
	7 = RDI
	8–15 represent R8–R15, respectively...

Thus unless we add RBP to 'struct tdx_module_args', it's impossible to re-order
the structure to match KVM's layout.

On Fri, Jul 21, 2023 at 12:18:23AM +0000, Huang, Kai wrote:

> Unfortunately I don't think it's feasible.  Sean pointed out that
> kvm_vcpu_arch::regs[] do follow the "register index" hardware layout in x86 (for
> which I missed sorry), so we cannot re-order KVM part.  
> 
> And unfortunately RBP (5) is in middle of those registers:
> 
> 	0 = RAX
> 	1 = RCX
> 	2 = RDX
> 	3 = RBX
> 	4 = RSP
> 	5 = RBP
> 	6 = RSI
> 	7 = RDI
> 	8–15 represent R8–R15, respectively...
> 
> Thus unless we add RBP to 'struct tdx_module_args', it's impossible to re-order
> the structure to match KVM's layout.

Adding RBP to the struct shouldn't be a problem, we'll just not use it.
Same as RSP, nobody sane would expect that to be used. If you worry
about it you can give them funny names like:

struct tdx_module_args {
	unsigned long ax;
	unsigned long cx;
	unsigned long dx;
	unsigned long bx;
	unsigned long __sp_unused;
	unsigned long __bp_unused;
	unsigned long si;
	unsigned long di;
	...
};

I mean, at this point the whole thing is just 128 bytes of data anyway.

On Fri, 2023-07-21 at 10:06 +0200, Peter Zijlstra wrote:
> On Fri, Jul 21, 2023 at 12:18:23AM +0000, Huang, Kai wrote:
> 
> > Unfortunately I don't think it's feasible.  Sean pointed out that
> > kvm_vcpu_arch::regs[] do follow the "register index" hardware layout in x86 (for
> > which I missed sorry), so we cannot re-order KVM part.  
> > 
> > And unfortunately RBP (5) is in middle of those registers:
> > 
> > 	0 = RAX
> > 	1 = RCX
> > 	2 = RDX
> > 	3 = RBX
> > 	4 = RSP
> > 	5 = RBP
> > 	6 = RSI
> > 	7 = RDI
> > 	8–15 represent R8–R15, respectively...
> > 
> > Thus unless we add RBP to 'struct tdx_module_args', it's impossible to re-order
> > the structure to match KVM's layout.
> 
> Adding RBP to the struct shouldn't be a problem, we'll just not use it.
> Same as RSP, nobody sane would expect that to be used. If you worry
> about it you can give them funny names like:
> 
> struct tdx_module_args {
> 	unsigned long ax;
> 	unsigned long cx;
> 	unsigned long dx;
> 	unsigned long bx;
> 	unsigned long __sp_unused;
> 	unsigned long __bp_unused;
> 	unsigned long si;
> 	unsigned long di;
> 	...
> };
> 
> I mean, at this point the whole thing is just 128 bytes of data anyway.

Sure I can do.

How about adding an additional patch on top of this series?  

For instance, below patch (compile only):

From c63d01bf91fe23f1e2d2e085644326bdee114d49 Mon Sep 17 00:00:00 2001
From: Kai Huang <kai.huang@intel.com>
Date: Fri, 21 Jul 2023 22:06:31 +1200
Subject: [PATCH] x86/tdx: Add __seamcall_saved_ret() for TDH.VP.ENTER

For TDX guest, KVM uses TDH.VP.ENTER SEAMCALL leaf to enter the guest.
When TDH.VP.ENTER returns to KVM due to TDG.VP.VMCALL from the TDX
guest, all RCX/RDX/RBX/RDI/RSI/R8-R15 are valid output registers, and
the following TDH.VP.ENTER also uses all those registers as input.

Introduce __seamcall_saved_ret(), which uses all above registers as
both input/output, for KVM to support TDH.VP.ENTER.

Also, KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows
follows the "register index" hardware layout of x86 GPRs.  However the
__seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as
argument, thus there's a mismatch here.

One option is KVM to copy input registers from 'vcpu::regs[]' to a
'struct tdx_module_args' on the stack and use that to make the SEAMCALL,
but such memory copy isn't desired and should be avoided if possible.
It's not feasible to change KVM's 'vcpu::regs[]' layout due to various
reasons (e.g., emulation code uses decoded register index as array index
to access the register).  Therefore, adjust 'struct tdx_module_args' to
match KVM's 'vcpu::regs[]' so that KVM can simply do below:

        seamcall_saved_ret(TDH.VP.ENTER,
                        (struct tdx_module_args *)vcpu->arch->regs);

Note RAX/RSP/RBP are not used by the TDX_MODULE_CALL assembly, but they
are necessary in order match the layout of 'struct tdx_module_args' to
KVM's 'vcpu::regs[]'.  Thus add them to the structure, but name them as
*_unused.  Also don't include them to asm-offset.c so that any misuse of
them in the assembly would result in build error.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/include/asm/shared/tdx.h | 19 +++++++++++++------
 arch/x86/virt/vmx/tdx/seamcall.S  | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/shared/tdx.h
b/arch/x86/include/asm/shared/tdx.h
index 74fc466dfdcd..8d1427562c63 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -58,24 +58,31 @@
  * Used in __tdcall*() to gather the input/output registers' values of the
  * TDCALL instruction when requesting services from the TDX module. This is a
  * software only structure and not part of the TDX module/VMM ABI
+ *
+ * Note those *_unused are not used by the TDX_MODULE_CALL assembly.
+ * The layout of this structure also matches KVM's kvm_vcpu_arch::regs[]
+ * layout, which follows the "register index" order of x86 GPRs.  KVM
+ * then can simply type cast kvm_vcpu_arch::regs[] to this structure to
+ * avoid the extra memory copy between two structures when making
+ * TDH.VP.ENTER SEAMCALL.
  */
 struct tdx_module_args {
-       /* callee-clobbered */
+       u64 rax_unused;
        u64 rcx;
        u64 rdx;
+       u64 rbx;
+       u64 rsp_unused;
+       u64 rbp_unused;
+       u64 rsi;
+       u64 rdi;
        u64 r8;
        u64 r9;
-       /* extra callee-clobbered */
        u64 r10;
        u64 r11;
-       /* callee-saved + rdi/rsi */
        u64 r12;
        u64 r13;
        u64 r14;
        u64 r15;
-       u64 rbx;
-       u64 rdi;
-       u64 rsi;
 }; 

/* Used to communicate with the TDX module */
diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
index b32934837f16..f251ebadb014 100644
--- a/arch/x86/virt/vmx/tdx/seamcall.S
+++ b/arch/x86/virt/vmx/tdx/seamcall.S
@@ -40,3 +40,22 @@ SYM_FUNC_END(__seamcall)
 SYM_FUNC_START(__seamcall_ret)
        TDX_MODULE_CALL host=1 ret=1
 SYM_FUNC_END(__seamcall_ret)
+
+/*
+ * __seamcall_saved_ret() - Host-side interface functions to SEAM software
+ * (the P-SEAMLDR or the TDX module), with saving output registers to the
+ * 'struct tdx_module_args' used as input.
+ *
+ * __seamcall_saved_ret() function ABI:
+ *
+ * @fn   (RDI)  - SEAMCALL Leaf number, moved to RAX
+ * @args (RSI)  - struct tdx_module_args for input and output
+ *
+ * All registers in @args are used as input/output registers.
+ *
+ * Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
+ * fails, or the completion status of the SEAMCALL leaf function.
+ */
+SYM_FUNC_START(__seamcall_ret)
+       TDX_MODULE_CALL host=1 ret=1 saved=1
+SYM_FUNC_END(__seamcall_ret)
-- 
2.41.0

On Fri, 2023-07-21 at 11:02 +0000, Huang, Kai wrote:
> On Fri, 2023-07-21 at 10:06 +0200, Peter Zijlstra wrote:
> > On Fri, Jul 21, 2023 at 12:18:23AM +0000, Huang, Kai wrote:
> > 
> > > Unfortunately I don't think it's feasible.  Sean pointed out that
> > > kvm_vcpu_arch::regs[] do follow the "register index" hardware layout in x86 (for
> > > which I missed sorry), so we cannot re-order KVM part.  
> > > 
> > > And unfortunately RBP (5) is in middle of those registers:
> > > 
> > > 	0 = RAX
> > > 	1 = RCX
> > > 	2 = RDX
> > > 	3 = RBX
> > > 	4 = RSP
> > > 	5 = RBP
> > > 	6 = RSI
> > > 	7 = RDI
> > > 	8–15 represent R8–R15, respectively...
> > > 
> > > Thus unless we add RBP to 'struct tdx_module_args', it's impossible to re-order
> > > the structure to match KVM's layout.
> > 
> > Adding RBP to the struct shouldn't be a problem, we'll just not use it.
> > Same as RSP, nobody sane would expect that to be used. If you worry
> > about it you can give them funny names like:
> > 
> > struct tdx_module_args {
> > 	unsigned long ax;
> > 	unsigned long cx;
> > 	unsigned long dx;
> > 	unsigned long bx;
> > 	unsigned long __sp_unused;
> > 	unsigned long __bp_unused;
> > 	unsigned long si;
> > 	unsigned long di;
> > 	...
> > };
> > 
> > I mean, at this point the whole thing is just 128 bytes of data anyway.
> 
> Sure I can do.
> 
> How about adding an additional patch on top of this series?  
> 
> For instance, below patch (compile only):
> 
> From c63d01bf91fe23f1e2d2e085644326bdee114d49 Mon Sep 17 00:00:00 2001
> From: Kai Huang <kai.huang@intel.com>
> Date: Fri, 21 Jul 2023 22:06:31 +1200
> Subject: [PATCH] x86/tdx: Add __seamcall_saved_ret() for TDH.VP.ENTER
> 
> For TDX guest, KVM uses TDH.VP.ENTER SEAMCALL leaf to enter the guest.
> When TDH.VP.ENTER returns to KVM due to TDG.VP.VMCALL from the TDX
> guest, all RCX/RDX/RBX/RDI/RSI/R8-R15 are valid output registers, and
> the following TDH.VP.ENTER also uses all those registers as input.
> 
> Introduce __seamcall_saved_ret(), which uses all above registers as
> both input/output, for KVM to support TDH.VP.ENTER.
> 
> Also, KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows
> follows the "register index" hardware layout of x86 GPRs.  However the
> __seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as
> argument, thus there's a mismatch here.
> 
> One option is KVM to copy input registers from 'vcpu::regs[]' to a
> 'struct tdx_module_args' on the stack and use that to make the SEAMCALL,
> but such memory copy isn't desired and should be avoided if possible.
> It's not feasible to change KVM's 'vcpu::regs[]' layout due to various
> reasons (e.g., emulation code uses decoded register index as array index
> to access the register).  Therefore, adjust 'struct tdx_module_args' to
> match KVM's 'vcpu::regs[]' so that KVM can simply do below:
> 
>         seamcall_saved_ret(TDH.VP.ENTER,
>                         (struct tdx_module_args *)vcpu->arch->regs);
> 
> Note RAX/RSP/RBP are not used by the TDX_MODULE_CALL assembly, but they
> are necessary in order match the layout of 'struct tdx_module_args' to
> KVM's 'vcpu::regs[]'.  Thus add them to the structure, but name them as
> *_unused.  Also don't include them to asm-offset.c so that any misuse of
> them in the assembly would result in build error.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>  arch/x86/include/asm/shared/tdx.h | 19 +++++++++++++------
>  arch/x86/virt/vmx/tdx/seamcall.S  | 19 +++++++++++++++++++
>  2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/shared/tdx.h
> b/arch/x86/include/asm/shared/tdx.h
> index 74fc466dfdcd..8d1427562c63 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -58,24 +58,31 @@
>   * Used in __tdcall*() to gather the input/output registers' values of the
>   * TDCALL instruction when requesting services from the TDX module. This is a
>   * software only structure and not part of the TDX module/VMM ABI
> + *
> + * Note those *_unused are not used by the TDX_MODULE_CALL assembly.
> + * The layout of this structure also matches KVM's kvm_vcpu_arch::regs[]
> + * layout, which follows the "register index" order of x86 GPRs.  KVM
> + * then can simply type cast kvm_vcpu_arch::regs[] to this structure to
> + * avoid the extra memory copy between two structures when making
> + * TDH.VP.ENTER SEAMCALL.
>   */
>  struct tdx_module_args {
> -       /* callee-clobbered */
> +       u64 rax_unused;
>         u64 rcx;
>         u64 rdx;
> +       u64 rbx;
> +       u64 rsp_unused;
> +       u64 rbp_unused;
> +       u64 rsi;
> +       u64 rdi;
>         u64 r8;
>         u64 r9;
> -       /* extra callee-clobbered */
>         u64 r10;
>         u64 r11;
> -       /* callee-saved + rdi/rsi */
>         u64 r12;
>         u64 r13;
>         u64 r14;
>         u64 r15;
> -       u64 rbx;
> -       u64 rdi;
> -       u64 rsi;
>  }; 
> 
> /* Used to communicate with the TDX module */
> diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
> index b32934837f16..f251ebadb014 100644
> --- a/arch/x86/virt/vmx/tdx/seamcall.S
> +++ b/arch/x86/virt/vmx/tdx/seamcall.S
> @@ -40,3 +40,22 @@ SYM_FUNC_END(__seamcall)
>  SYM_FUNC_START(__seamcall_ret)
>         TDX_MODULE_CALL host=1 ret=1
>  SYM_FUNC_END(__seamcall_ret)
> +
> +/*
> + * __seamcall_saved_ret() - Host-side interface functions to SEAM software
> + * (the P-SEAMLDR or the TDX module), with saving output registers to the
> + * 'struct tdx_module_args' used as input.
> + *
> + * __seamcall_saved_ret() function ABI:
> + *
> + * @fn   (RDI)  - SEAMCALL Leaf number, moved to RAX
> + * @args (RSI)  - struct tdx_module_args for input and output
> + *
> + * All registers in @args are used as input/output registers.
> + *
> + * Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
> + * fails, or the completion status of the SEAMCALL leaf function.
> + */
> +SYM_FUNC_START(__seamcall_ret)
> +       TDX_MODULE_CALL host=1 ret=1 saved=1
> +SYM_FUNC_END(__seamcall_ret)

Sorry should be __seamcall_saved_ret.

> -- 
> 2.41.0
> 
> 

Sorry missing the declaration of __seamcall_saved_ret():

--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -80,6 +80,7 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned
long p1,
 #ifdef CONFIG_INTEL_TDX_HOST
 u64 __seamcall(u64 fn, struct tdx_module_args *args);
 u64 __seamcall_ret(u64 fn, struct tdx_module_args *args);
+u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);

And perhaps we should adjust the order of registers in asm-offset.c to match the
change to the 'struct tdx_module_args'?

--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -70,6 +70,9 @@ static void __used common(void)
        BLANK();
        OFFSET(TDX_MODULE_rcx, tdx_module_args, rcx);
        OFFSET(TDX_MODULE_rdx, tdx_module_args, rdx);
+       OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
+       OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
+       OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
        OFFSET(TDX_MODULE_r8,  tdx_module_args, r8);
        OFFSET(TDX_MODULE_r9,  tdx_module_args, r9);
        OFFSET(TDX_MODULE_r10, tdx_module_args, r10);
@@ -78,9 +81,6 @@ static void __used common(void)
        OFFSET(TDX_MODULE_r13, tdx_module_args, r13);
        OFFSET(TDX_MODULE_r14, tdx_module_args, r14);
        OFFSET(TDX_MODULE_r15, tdx_module_args, r15);
-       OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
-       OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
-       OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);

On Fri, 2023-07-21 at 11:44 +0000, Huang, Kai wrote:
> On Fri, 2023-07-21 at 11:02 +0000, Huang, Kai wrote:
> > On Fri, 2023-07-21 at 10:06 +0200, Peter Zijlstra wrote:
> > > On Fri, Jul 21, 2023 at 12:18:23AM +0000, Huang, Kai wrote:
> > > 
> > > > Unfortunately I don't think it's feasible.  Sean pointed out that
> > > > kvm_vcpu_arch::regs[] do follow the "register index" hardware layout in x86 (for
> > > > which I missed sorry), so we cannot re-order KVM part.  
> > > > 
> > > > And unfortunately RBP (5) is in middle of those registers:
> > > > 
> > > > 	0 = RAX
> > > > 	1 = RCX
> > > > 	2 = RDX
> > > > 	3 = RBX
> > > > 	4 = RSP
> > > > 	5 = RBP
> > > > 	6 = RSI
> > > > 	7 = RDI
> > > > 	8–15 represent R8–R15, respectively...
> > > > 
> > > > Thus unless we add RBP to 'struct tdx_module_args', it's impossible to re-order
> > > > the structure to match KVM's layout.
> > > 
> > > Adding RBP to the struct shouldn't be a problem, we'll just not use it.
> > > Same as RSP, nobody sane would expect that to be used. If you worry
> > > about it you can give them funny names like:
> > > 
> > > struct tdx_module_args {
> > > 	unsigned long ax;
> > > 	unsigned long cx;
> > > 	unsigned long dx;
> > > 	unsigned long bx;
> > > 	unsigned long __sp_unused;
> > > 	unsigned long __bp_unused;
> > > 	unsigned long si;
> > > 	unsigned long di;
> > > 	...
> > > };
> > > 
> > > I mean, at this point the whole thing is just 128 bytes of data anyway.
> > 
> > Sure I can do.
> > 
> > How about adding an additional patch on top of this series?  
> > 
> > For instance, below patch (compile only):
> > 
> > From c63d01bf91fe23f1e2d2e085644326bdee114d49 Mon Sep 17 00:00:00 2001
> > From: Kai Huang <kai.huang@intel.com>
> > Date: Fri, 21 Jul 2023 22:06:31 +1200
> > Subject: [PATCH] x86/tdx: Add __seamcall_saved_ret() for TDH.VP.ENTER
> > 
> > For TDX guest, KVM uses TDH.VP.ENTER SEAMCALL leaf to enter the guest.
> > When TDH.VP.ENTER returns to KVM due to TDG.VP.VMCALL from the TDX
> > guest, all RCX/RDX/RBX/RDI/RSI/R8-R15 are valid output registers, and
> > the following TDH.VP.ENTER also uses all those registers as input.
> > 
> > Introduce __seamcall_saved_ret(), which uses all above registers as
> > both input/output, for KVM to support TDH.VP.ENTER.
> > 
> > Also, KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows
> > follows the "register index" hardware layout of x86 GPRs.  However the
> > __seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as
> > argument, thus there's a mismatch here.
> > 
> > One option is KVM to copy input registers from 'vcpu::regs[]' to a
> > 'struct tdx_module_args' on the stack and use that to make the SEAMCALL,
> > but such memory copy isn't desired and should be avoided if possible.
> > It's not feasible to change KVM's 'vcpu::regs[]' layout due to various
> > reasons (e.g., emulation code uses decoded register index as array index
> > to access the register).  Therefore, adjust 'struct tdx_module_args' to
> > match KVM's 'vcpu::regs[]' so that KVM can simply do below:
> > 
> >         seamcall_saved_ret(TDH.VP.ENTER,
> >                         (struct tdx_module_args *)vcpu->arch->regs);
> > 
> > Note RAX/RSP/RBP are not used by the TDX_MODULE_CALL assembly, but they
> > are necessary in order match the layout of 'struct tdx_module_args' to
> > KVM's 'vcpu::regs[]'.  Thus add them to the structure, but name them as
> > *_unused.  Also don't include them to asm-offset.c so that any misuse of
> > them in the assembly would result in build error.
> > 
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> >  arch/x86/include/asm/shared/tdx.h | 19 +++++++++++++------
> >  arch/x86/virt/vmx/tdx/seamcall.S  | 19 +++++++++++++++++++
> >  2 files changed, 32 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/shared/tdx.h
> > b/arch/x86/include/asm/shared/tdx.h
> > index 74fc466dfdcd..8d1427562c63 100644
> > --- a/arch/x86/include/asm/shared/tdx.h
> > +++ b/arch/x86/include/asm/shared/tdx.h
> > @@ -58,24 +58,31 @@
> >   * Used in __tdcall*() to gather the input/output registers' values of the
> >   * TDCALL instruction when requesting services from the TDX module. This is a
> >   * software only structure and not part of the TDX module/VMM ABI
> > + *
> > + * Note those *_unused are not used by the TDX_MODULE_CALL assembly.
> > + * The layout of this structure also matches KVM's kvm_vcpu_arch::regs[]
> > + * layout, which follows the "register index" order of x86 GPRs.  KVM
> > + * then can simply type cast kvm_vcpu_arch::regs[] to this structure to
> > + * avoid the extra memory copy between two structures when making
> > + * TDH.VP.ENTER SEAMCALL.
> >   */
> >  struct tdx_module_args {
> > -       /* callee-clobbered */
> > +       u64 rax_unused;
> >         u64 rcx;
> >         u64 rdx;
> > +       u64 rbx;
> > +       u64 rsp_unused;
> > +       u64 rbp_unused;
> > +       u64 rsi;
> > +       u64 rdi;
> >         u64 r8;
> >         u64 r9;
> > -       /* extra callee-clobbered */
> >         u64 r10;
> >         u64 r11;
> > -       /* callee-saved + rdi/rsi */
> >         u64 r12;
> >         u64 r13;
> >         u64 r14;
> >         u64 r15;
> > -       u64 rbx;
> > -       u64 rdi;
> > -       u64 rsi;
> >  }; 
> > 
> > /* Used to communicate with the TDX module */
> > diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
> > index b32934837f16..f251ebadb014 100644
> > --- a/arch/x86/virt/vmx/tdx/seamcall.S
> > +++ b/arch/x86/virt/vmx/tdx/seamcall.S
> > @@ -40,3 +40,22 @@ SYM_FUNC_END(__seamcall)
> >  SYM_FUNC_START(__seamcall_ret)
> >         TDX_MODULE_CALL host=1 ret=1
> >  SYM_FUNC_END(__seamcall_ret)
> > +
> > +/*
> > + * __seamcall_saved_ret() - Host-side interface functions to SEAM software
> > + * (the P-SEAMLDR or the TDX module), with saving output registers to the
> > + * 'struct tdx_module_args' used as input.
> > + *
> > + * __seamcall_saved_ret() function ABI:
> > + *
> > + * @fn   (RDI)  - SEAMCALL Leaf number, moved to RAX
> > + * @args (RSI)  - struct tdx_module_args for input and output
> > + *
> > + * All registers in @args are used as input/output registers.
> > + *
> > + * Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
> > + * fails, or the completion status of the SEAMCALL leaf function.
> > + */
> > +SYM_FUNC_START(__seamcall_ret)
> > +       TDX_MODULE_CALL host=1 ret=1 saved=1
> > +SYM_FUNC_END(__seamcall_ret)
> 
> Sorry should be __seamcall_saved_ret.
> 
> > -- 
> > 2.41.0
> > 
> > 
> 
> Sorry missing the declaration of __seamcall_saved_ret():
> 
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -80,6 +80,7 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned
> long p1,
>  #ifdef CONFIG_INTEL_TDX_HOST
>  u64 __seamcall(u64 fn, struct tdx_module_args *args);
>  u64 __seamcall_ret(u64 fn, struct tdx_module_args *args);
> +u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);
> 
> And perhaps we should adjust the order of registers in asm-offset.c to match the
> change to the 'struct tdx_module_args'?
> 
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -70,6 +70,9 @@ static void __used common(void)
>         BLANK();
>         OFFSET(TDX_MODULE_rcx, tdx_module_args, rcx);
>         OFFSET(TDX_MODULE_rdx, tdx_module_args, rdx);
> +       OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
> +       OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
> +       OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
>         OFFSET(TDX_MODULE_r8,  tdx_module_args, r8);
>         OFFSET(TDX_MODULE_r9,  tdx_module_args, r9);
>         OFFSET(TDX_MODULE_r10, tdx_module_args, r10);
> @@ -78,9 +81,6 @@ static void __used common(void)
>         OFFSET(TDX_MODULE_r13, tdx_module_args, r13);
>         OFFSET(TDX_MODULE_r14, tdx_module_args, r14);
>         OFFSET(TDX_MODULE_r15, tdx_module_args, r15);
> -       OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
> -       OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
> -       OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
> 

Damn.. I think it's better to split this into two patches:

1) Introduce __seamcall_saved_ret() for VP.ENTER
2) Adjust 'struct tdx_module_args' layout

Because 1) is mandatory to support VP.ENTER, but 2) theoretically is an
optimization.

Then perhaps 1) can even be merged to patch "x86/virt/tdx: Wire up basic
SEAMCALL functions".

I'll make those patches and test the VP.ENTER actually works using the adjusted
'struct tdx_module_args' layout, and post v3 when done.

Thanks again Peter, and let me know if you have any comments here.