linux-kernel - Re: [RFC PATCH v5 014/104] KVM: TDX: Add a function for KVM to invoke SEAMCALL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 14 Mar 2022 11:42:24 +1300
From:   Kai Huang <kai.huang@...el.com>
To:     isaku.yamahata@...el.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>, erdemaktas@...gle.com,
        Connor Kuehl <ckuehl@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        kirill.shutemov@...ux.intel.com
Subject: Re: [RFC PATCH v5 014/104] KVM: TDX: Add a function for KVM to
 invoke SEAMCALL

On Fri, 2022-03-04 at 11:48 -0800, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> Add an assembly function for KVM to call the TDX module because __seamcall
> defined in arch/x86/virt/vmx/seamcall.S doesn't fit for the KVM use case.
> 
> TDX module API returns extended error information in registers, rcx, rdx,
> r8, r9, r10, and r11 in addition to success case.  KVM uses those extended
> error information in addition to the status code returned in RAX.  Update
> the assembly code to optionally return those outputs even in the error case
> and define the specific version for KVM to call the TDX module.

+Kirill.

Kirill's patches hasn't been merged yet.  Can Kirill just change his patch so
you don't have to change the assembly code again?  Btw this change is
reasonable for host kernel support series as well.  Sorry that I failed to
notice.

Btw, SEAMCALL C function is already implemented in host kernel support series:

https://lore.kernel.org/kvm/269a053607357eedd9a1e8ddf0e7240ae0c3985c.1647167475.git.kai.huang@intel.com/

I think maybe you can just export __seamcall() and use it?

> 
> SEAMCALL to the SEAM module (P-SEAMLDR or TDX module) can result in the
> error of VmFailInvalid indicated by CF=1 when VMX isn't enabled by VMXON
> instruction.  Because KVM guarantees that VMX is enabled, VmFailInvalid
> error won't happen.  Don't check the error for KVM.

Perhaps you can introduce kvm_seamcall() which calls __seamcall(), but WARN()
if it returns VMfailInvalid?

> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
>  arch/x86/kvm/Makefile       |  2 +-
>  arch/x86/kvm/vmx/seamcall.S | 55 +++++++++++++++++++++++++++++++++++++
>  arch/x86/virt/tdxcall.S     |  8 ++++--
>  3 files changed, 62 insertions(+), 3 deletions(-)
>  create mode 100644 arch/x86/kvm/vmx/seamcall.S
> 
> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index e2c05195cb95..e8f83a7d0dc3 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -24,7 +24,7 @@ kvm-$(CONFIG_KVM_XEN)	+= xen.o
>  kvm-intel-y		+= vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \
>  			   vmx/evmcs.o vmx/nested.o vmx/posted_intr.o vmx/main.o
>  kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
> -kvm-intel-$(CONFIG_INTEL_TDX_HOST)	+= vmx/tdx.o
> +kvm-intel-$(CONFIG_INTEL_TDX_HOST)	+= vmx/tdx.o vmx/seamcall.o
>  
>  kvm-amd-y		+= svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o svm/sev.o
>  
> diff --git a/arch/x86/kvm/vmx/seamcall.S b/arch/x86/kvm/vmx/seamcall.S
> new file mode 100644
> index 000000000000..4a15017fc7dd
> --- /dev/null
> +++ b/arch/x86/kvm/vmx/seamcall.S
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/linkage.h>
> +#include <asm/export.h>
> +#include <asm/frame.h>
> +
> +#include "../../virt/tdxcall.S"
> +
> +/*
> + * kvm_seamcall()  - Host-side interface functions to SEAM software (TDX module)
> + *
> + * Transform function call register arguments into the SEAMCALL register
> + * ABI.  Return the completion status of the SEAMCALL.  Additional output
> + * operands are saved in @out (if it is provided by the user).
> + * It doesn't check TDX_SEAMCALL_VMFAILINVALID unlike __semcall() because KVM
> + * guarantees that VMX is enabled so that TDX_SEAMCALL_VMFAILINVALID doesn't
> + * happen.  In the case of error completion status code, extended error code may
> + * be stored in leaf specific output registers.
> + *
> + *-------------------------------------------------------------------------
> + * 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.
> + *
> + *-------------------------------------------------------------------------
> + *
> + * kvm_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
> + *                       shared with the TDX module). It
> + *                       can be NULL.
> + *
> + * Return (via RAX) the completion status of the SEAMCALL
> + */
> +SYM_FUNC_START(kvm_seamcall)
> +        FRAME_BEGIN
> +        TDX_MODULE_CALL host=1 error_check=0
> +        FRAME_END
> +        ret
> +SYM_FUNC_END(kvm_seamcall)
> +EXPORT_SYMBOL_GPL(kvm_seamcall)

> diff --git a/arch/x86/virt/tdxcall.S b/arch/x86/virt/tdxcall.S
> index 90569faedacc..2e614b6b5f1e 100644
> --- a/arch/x86/virt/tdxcall.S
> +++ b/arch/x86/virt/tdxcall.S
> @@ -13,7 +13,7 @@
>  #define tdcall		.byte 0x66,0x0f,0x01,0xcc
>  #define seamcall	.byte 0x66,0x0f,0x01,0xcf
>  
> -.macro TDX_MODULE_CALL host:req
> +.macro TDX_MODULE_CALL host:req error_check=1
>  	/*
>  	 * R12 will be used as temporary storage for struct tdx_module_output
>  	 * pointer. Since R12-R15 registers are not used by TDCALL/SEAMCALL
> @@ -51,9 +51,11 @@
>  	 *
>  	 * Set %rax to TDX_SEAMCALL_VMFAILINVALID for VMfailInvalid.
>  	 * This value will never be used as actual SEAMCALL error code.
> -	 */
> +	*/
> +	.if \error_check
>  	jnc .Lno_vmfailinvalid
>  	mov $TDX_SEAMCALL_VMFAILINVALID, %rax
> +	.endif
>  .Lno_vmfailinvalid:
>  	.else
>  	tdcall
> @@ -66,8 +68,10 @@
>  	pop %r12
>  
>  	/* Check for success: 0 - Successful, otherwise failed */
> +	.if \error_check
>  	test %rax, %rax
>  	jnz .Lno_output_struct
> +	.endif
>  

Checking VMfailInvalid, and checking whether %rax is 0 are totally different
things.  I don't think it's good to use one single 'error_check' to handle.

As I mentioned above, I think checking whether %rax is 0 can be just removed
in Kirill's patch.  But I don't have strong opinion on whether you should add
another parameter to determine whether to check VMfailInvalid, or you should
just call __seamcall() which is implemented in host support series.