diff mbox series

[v6,7/7] x86/vmware: Add TDX hypercall support

Message ID 20240109084052.58661-8-amakhalov@vmware.com (mailing list archive)
State Not Applicable
Headers show
Series VMware hypercalls enhancements | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1083 this patch: 1083
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1109 this patch: 1109
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1111 this patch: 1111
netdev/checkpatch warning CHECK: Prefer kernel type 'u32' over 'uint32_t' CHECK: extern prototypes should be avoided in .h files WARNING: From:/Signed-off-by: email address mismatch: 'From: Alexey Makhalov <alexey.makhalov@broadcom.com>' != 'Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Alexey Makhalov Jan. 9, 2024, 8:40 a.m. UTC 
VMware hypercalls use I/O port, VMCALL or VMMCALL instructions.
Add __tdx_hypercall path to support TDX guests.

No change in high bandwidth hypercalls, as only low bandwidth
ones are supported for TDX guests.

Co-developed-by: Tim Merrifield <timothym@vmware.com>
Signed-off-by: Tim Merrifield <timothym@vmware.com>
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/vmware.h | 79 +++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/vmware.c  | 25 +++++++++++
 2 files changed, 104 insertions(+)

Comments

Dave Hansen Jan. 22, 2024, 4:32 p.m. UTC | #1 
On 1/9/24 00:40, Alexey Makhalov wrote:
> +#ifdef CONFIG_INTEL_TDX_GUEST
> +unsigned long vmware_tdx_hypercall(unsigned long cmd,
> +				   struct tdx_module_args *args)
> +{
> +	if (!hypervisor_is_type(X86_HYPER_VMWARE))
> +		return ULONG_MAX;
> +
> +	if (cmd & ~VMWARE_CMD_MASK) {
> +		pr_warn_once("Out of range command %lx\n", cmd);
> +		return ULONG_MAX;
> +	}
> +
> +	args->r10 = VMWARE_TDX_VENDOR_LEAF;
> +	args->r11 = VMWARE_TDX_HCALL_FUNC;
> +	args->r12 = VMWARE_HYPERVISOR_MAGIC;
> +	args->r13 = cmd;
> +	args->r15 = 0; /* CPL */
> +
> +	__tdx_hypercall(args);
> +
> +	return args->r12;
> +}
> +EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
> +#endif

This is the kind of wrapper that I was hoping for.  Thanks.

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
H. Peter Anvin Jan. 22, 2024, 6:28 p.m. UTC | #2 
On January 22, 2024 8:32:22 AM PST, Dave Hansen <dave.hansen@intel.com> wrote:
>On 1/9/24 00:40, Alexey Makhalov wrote:
>> +#ifdef CONFIG_INTEL_TDX_GUEST
>> +unsigned long vmware_tdx_hypercall(unsigned long cmd,
>> +				   struct tdx_module_args *args)
>> +{
>> +	if (!hypervisor_is_type(X86_HYPER_VMWARE))
>> +		return ULONG_MAX;
>> +
>> +	if (cmd & ~VMWARE_CMD_MASK) {
>> +		pr_warn_once("Out of range command %lx\n", cmd);
>> +		return ULONG_MAX;
>> +	}
>> +
>> +	args->r10 = VMWARE_TDX_VENDOR_LEAF;
>> +	args->r11 = VMWARE_TDX_HCALL_FUNC;
>> +	args->r12 = VMWARE_HYPERVISOR_MAGIC;
>> +	args->r13 = cmd;
>> +	args->r15 = 0; /* CPL */
>> +
>> +	__tdx_hypercall(args);
>> +
>> +	return args->r12;
>> +}
>> +EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
>> +#endif
>
>This is the kind of wrapper that I was hoping for.  Thanks.
>
>Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>

I'm slightly confused by this TBH.

Why are the arguments passed in as a structure, which is modified by the wrapper to boot? This is analogous to a system call interface.

Furthermore, this is an out-of-line function; it should never be called with !X86_HYPER_VMWARE or you are introducing overhead for other hypervisors; I believe a pr_warn_once() is in order at least, just as you have for the out-of-range test.
Alexey Makhalov Jan. 23, 2024, 12:04 a.m. UTC | #3 
On 1/22/24 10:28 AM, H. Peter Anvin wrote:
> On January 22, 2024 8:32:22 AM PST, Dave Hansen <dave.hansen@intel.com> wrote:
>> On 1/9/24 00:40, Alexey Makhalov wrote:
>>> +#ifdef CONFIG_INTEL_TDX_GUEST
>>> +unsigned long vmware_tdx_hypercall(unsigned long cmd,
>>> +				   struct tdx_module_args *args)
>>> +{
>>> +	if (!hypervisor_is_type(X86_HYPER_VMWARE))
>>> +		return ULONG_MAX;
>>> +
>>> +	if (cmd & ~VMWARE_CMD_MASK) {
>>> +		pr_warn_once("Out of range command %lx\n", cmd);
>>> +		return ULONG_MAX;
>>> +	}
>>> +
>>> +	args->r10 = VMWARE_TDX_VENDOR_LEAF;
>>> +	args->r11 = VMWARE_TDX_HCALL_FUNC;
>>> +	args->r12 = VMWARE_HYPERVISOR_MAGIC;
>>> +	args->r13 = cmd;
>>> +	args->r15 = 0; /* CPL */
>>> +
>>> +	__tdx_hypercall(args);
>>> +
>>> +	return args->r12;
>>> +}
>>> +EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
>>> +#endif
>>
>> This is the kind of wrapper that I was hoping for.  Thanks.
>>
>> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>>
> 
> I'm slightly confused by this TBH.
> 
> Why are the arguments passed in as a structure, which is modified by the wrapper to boot? This is analogous to a system call interface.
> 
> Furthermore, this is an out-of-line function; it should never be called with !X86_HYPER_VMWARE or you are introducing overhead for other hypervisors; I believe a pr_warn_once() is in order at least, just as you have for the out-of-range test.
> 

This patch series introduces vmware_hypercall family of functions 
similar to kvm_hypercall. Similarity: both vmware and kvm 
implementations are static inline functions and both of them use 
__tdx_hypercall (global not exported symbol). Difference: kvm_hypercall 
functions are used _only_ within the kernel, but vmware_hypercall are 
also used by modules.
Exporting __tdx_hypercall function is an original Dave's concern.
So we ended up with exporting wrapper, not generic, but VMware specific 
with added checks against arbitrary use.
vmware_tdx_hypercall is not designed for !X86_HYPER_VMWARE callers. But 
such a calls are not forbidden.
Arguments in a structure is an API for __tdx_hypercall(). Input and 
output argument handling are done by vmware_hypercall callers, while 
VMware specific dress up is inside the wrapper.

Peter, do you think code comments are required to make it clear for the 
reader?
H. Peter Anvin Jan. 23, 2024, 12:17 a.m. UTC | #4 
On January 22, 2024 4:04:33 PM PST, Alexey Makhalov <alexey.makhalov@broadcom.com> wrote:
>
>
>On 1/22/24 10:28 AM, H. Peter Anvin wrote:
>> On January 22, 2024 8:32:22 AM PST, Dave Hansen <dave.hansen@intel.com> wrote:
>>> On 1/9/24 00:40, Alexey Makhalov wrote:
>>>> +#ifdef CONFIG_INTEL_TDX_GUEST
>>>> +unsigned long vmware_tdx_hypercall(unsigned long cmd,
>>>> +				   struct tdx_module_args *args)
>>>> +{
>>>> +	if (!hypervisor_is_type(X86_HYPER_VMWARE))
>>>> +		return ULONG_MAX;
>>>> +
>>>> +	if (cmd & ~VMWARE_CMD_MASK) {
>>>> +		pr_warn_once("Out of range command %lx\n", cmd);
>>>> +		return ULONG_MAX;
>>>> +	}
>>>> +
>>>> +	args->r10 = VMWARE_TDX_VENDOR_LEAF;
>>>> +	args->r11 = VMWARE_TDX_HCALL_FUNC;
>>>> +	args->r12 = VMWARE_HYPERVISOR_MAGIC;
>>>> +	args->r13 = cmd;
>>>> +	args->r15 = 0; /* CPL */
>>>> +
>>>> +	__tdx_hypercall(args);
>>>> +
>>>> +	return args->r12;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
>>>> +#endif
>>> 
>>> This is the kind of wrapper that I was hoping for.  Thanks.
>>> 
>>> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>>> 
>> 
>> I'm slightly confused by this TBH.
>> 
>> Why are the arguments passed in as a structure, which is modified by the wrapper to boot? This is analogous to a system call interface.
>> 
>> Furthermore, this is an out-of-line function; it should never be called with !X86_HYPER_VMWARE or you are introducing overhead for other hypervisors; I believe a pr_warn_once() is in order at least, just as you have for the out-of-range test.
>> 
>
>This patch series introduces vmware_hypercall family of functions similar to kvm_hypercall. Similarity: both vmware and kvm implementations are static inline functions and both of them use __tdx_hypercall (global not exported symbol). Difference: kvm_hypercall functions are used _only_ within the kernel, but vmware_hypercall are also used by modules.
>Exporting __tdx_hypercall function is an original Dave's concern.
>So we ended up with exporting wrapper, not generic, but VMware specific with added checks against arbitrary use.
>vmware_tdx_hypercall is not designed for !X86_HYPER_VMWARE callers. But such a calls are not forbidden.
>Arguments in a structure is an API for __tdx_hypercall(). Input and output argument handling are done by vmware_hypercall callers, while VMware specific dress up is inside the wrapper.
>
>Peter, do you think code comments are required to make it clear for the reader?
>
>

TBH that explanation didn't make much sense to me...
Alexey Makhalov Jan. 26, 2024, 12:55 a.m. UTC | #5 
On 1/22/24 4:17 PM, H. Peter Anvin wrote:
> On January 22, 2024 4:04:33 PM PST, Alexey Makhalov <alexey.makhalov@broadcom.com> wrote:
>>
>>
>> On 1/22/24 10:28 AM, H. Peter Anvin wrote:
>>> On January 22, 2024 8:32:22 AM PST, Dave Hansen <dave.hansen@intel.com> wrote:
>>>> On 1/9/24 00:40, Alexey Makhalov wrote:
>>>>> +#ifdef CONFIG_INTEL_TDX_GUEST
>>>>> +unsigned long vmware_tdx_hypercall(unsigned long cmd,
>>>>> +				   struct tdx_module_args *args)
>>>>> +{
>>>>> +	if (!hypervisor_is_type(X86_HYPER_VMWARE))
>>>>> +		return ULONG_MAX;
>>>>> +
>>>>> +	if (cmd & ~VMWARE_CMD_MASK) {
>>>>> +		pr_warn_once("Out of range command %lx\n", cmd);
>>>>> +		return ULONG_MAX;
>>>>> +	}
>>>>> +
>>>>> +	args->r10 = VMWARE_TDX_VENDOR_LEAF;
>>>>> +	args->r11 = VMWARE_TDX_HCALL_FUNC;
>>>>> +	args->r12 = VMWARE_HYPERVISOR_MAGIC;
>>>>> +	args->r13 = cmd;
>>>>> +	args->r15 = 0; /* CPL */
>>>>> +
>>>>> +	__tdx_hypercall(args);
>>>>> +
>>>>> +	return args->r12;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
>>>>> +#endif
>>>>
>>>> This is the kind of wrapper that I was hoping for.  Thanks.
>>>>
>>>> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>>>>
>>>
>>> I'm slightly confused by this TBH.
>>>
>>> Why are the arguments passed in as a structure, which is modified by the wrapper to boot? This is analogous to a system call interface.
>>>
>>> Furthermore, this is an out-of-line function; it should never be called with !X86_HYPER_VMWARE or you are introducing overhead for other hypervisors; I believe a pr_warn_once() is in order at least, just as you have for the out-of-range test.
>>>
>>
>> This patch series introduces vmware_hypercall family of functions similar to kvm_hypercall. Similarity: both vmware and kvm implementations are static inline functions and both of them use __tdx_hypercall (global not exported symbol). Difference: kvm_hypercall functions are used _only_ within the kernel, but vmware_hypercall are also used by modules.
>> Exporting __tdx_hypercall function is an original Dave's concern.
>> So we ended up with exporting wrapper, not generic, but VMware specific with added checks against arbitrary use.
>> vmware_tdx_hypercall is not designed for !X86_HYPER_VMWARE callers. But such a calls are not forbidden.
>> Arguments in a structure is an API for __tdx_hypercall(). Input and output argument handling are done by vmware_hypercall callers, while VMware specific dress up is inside the wrapper.
>>
>> Peter, do you think code comments are required to make it clear for the reader?
>>
>>
> 
> TBH that explanation didn't make much sense to me...

Peter,

I would like to understand your concerns.

1. Are you suggesting to move structure (tdx parameters) initialization 
in one please, instead of one part there another part here? Do you 
prefer to pass all arguments as is to vmware_tdx_hypercall() and only 
define tdx_module_args there?

2. And second suggestion is to add pr_warn_once under "if 
(!hypervisor_is_type(X86_HYPER_VMWARE))" ?

--Alexey
diff mbox series

Patch

diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h
index 84a31f579a30..3bd593c6591d 100644
--- a/arch/x86/include/asm/vmware.h
+++ b/arch/x86/include/asm/vmware.h
@@ -18,6 +18,12 @@ 
  * arg2 - Hypercall command
  * arg3 bits [15:0] - Port number, LB and direction flags
  *
+ * - Low bandwidth TDX hypercalls (x86_64 only) are similar to LB
+ * hypercalls. They also have up to 6 input and 6 output on registers
+ * arguments, with different argument to register mapping:
+ * %r12 (arg0), %rbx (arg1), %r13 (arg2), %rdx (arg3),
+ * %rsi (arg4), %rdi (arg5).
+ *
  * - High bandwidth (HB) hypercalls are I/O port based only. They have
  * up to 7 input and 7 output arguments passed and returned using
  * registers: %eax (arg0), %ebx (arg1), %ecx (arg2), %edx (arg3),
@@ -54,12 +60,61 @@ 
 #define VMWARE_CMD_GETHZ		45
 #define VMWARE_CMD_GETVCPU_INFO		68
 #define VMWARE_CMD_STEALCLOCK		91
+/*
+ * Hypercall command mask:
+ *   bits [6:0] command, range [0, 127]
+ *   bits [19:16] sub-command, range [0, 15]
+ */
+#define VMWARE_CMD_MASK			0xf007fU
 
 #define CPUID_VMWARE_FEATURES_ECX_VMMCALL	BIT(0)
 #define CPUID_VMWARE_FEATURES_ECX_VMCALL	BIT(1)
 
 extern u8 vmware_hypercall_mode;
 
+#define VMWARE_TDX_VENDOR_LEAF 0x1af7e4909ULL
+#define VMWARE_TDX_HCALL_FUNC  1
+
+extern unsigned long vmware_tdx_hypercall(unsigned long cmd,
+					  struct tdx_module_args *args);
+
+/*
+ * TDCALL[TDG.VP.VMCALL] uses %rax (arg0) and %rcx (arg2). Therefore,
+ * we remap those registers to %r12 and %r13, respectively.
+ */
+static inline
+unsigned long vmware_tdx_hypercall_args(unsigned long cmd, unsigned long in1,
+					unsigned long in3, unsigned long in4,
+					unsigned long in5,
+					uint32_t *out1, uint32_t *out2,
+					uint32_t *out3, uint32_t *out4,
+					uint32_t *out5)
+{
+	unsigned long ret;
+
+	struct tdx_module_args args = {
+		.rbx = in1,
+		.rdx = in3,
+		.rsi = in4,
+		.rdi = in5,
+	};
+
+	ret = vmware_tdx_hypercall(cmd, &args);
+
+	if (out1)
+		*out1 = args.rbx;
+	if (out2)
+		*out2 = args.r13;
+	if (out3)
+		*out3 = args.rdx;
+	if (out4)
+		*out4 = args.rsi;
+	if (out5)
+		*out5 = args.rdi;
+
+	return ret;
+}
+
 /*
  * The low bandwidth call. The low word of %edx is presumed to have OUT bit
  * set. The high word of %edx may contain input data from the caller.
@@ -87,6 +142,10 @@  unsigned long vmware_hypercall1(unsigned long cmd, unsigned long in1)
 {
 	unsigned long out0;
 
+	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0,
+						 NULL, NULL, NULL, NULL, NULL);
+
 	asm_inline volatile (VMWARE_HYPERCALL
 		: "=a" (out0)
 		: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -105,6 +164,10 @@  unsigned long vmware_hypercall3(unsigned long cmd, unsigned long in1,
 {
 	unsigned long out0;
 
+	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0,
+						 out1, out2, NULL, NULL, NULL);
+
 	asm_inline volatile (VMWARE_HYPERCALL
 		: "=a" (out0), "=b" (*out1), "=c" (*out2)
 		: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -124,6 +187,10 @@  unsigned long vmware_hypercall4(unsigned long cmd, unsigned long in1,
 {
 	unsigned long out0;
 
+	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0,
+						 out1, out2, out3, NULL, NULL);
+
 	asm_inline volatile (VMWARE_HYPERCALL
 		: "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
 		: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -143,6 +210,10 @@  unsigned long vmware_hypercall5(unsigned long cmd, unsigned long in1,
 {
 	unsigned long out0;
 
+	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return vmware_tdx_hypercall_args(cmd, in1, in3, in4, in5,
+						 NULL, out2, NULL, NULL, NULL);
+
 	asm_inline volatile (VMWARE_HYPERCALL
 		: "=a" (out0), "=c" (*out2)
 		: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -165,6 +236,10 @@  unsigned long vmware_hypercall6(unsigned long cmd, unsigned long in1,
 {
 	unsigned long out0;
 
+	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return vmware_tdx_hypercall_args(cmd, in1, in3, 0, 0,
+						 NULL, out2, out3, out4, out5);
+
 	asm_inline volatile (VMWARE_HYPERCALL
 		: "=a" (out0), "=c" (*out2), "=d" (*out3), "=S" (*out4),
 		  "=D" (*out5)
@@ -186,6 +261,10 @@  unsigned long vmware_hypercall7(unsigned long cmd, unsigned long in1,
 {
 	unsigned long out0;
 
+	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return vmware_tdx_hypercall_args(cmd, in1, in3, in4, in5,
+						 out1, out2, out3, NULL, NULL);
+
 	asm_inline volatile (VMWARE_HYPERCALL
 		: "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
 		: [port] "i" (VMWARE_HYPERVISOR_PORT),
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 3aa1adaed18f..84caa67d4820 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -428,6 +428,31 @@  static bool __init vmware_legacy_x2apic_available(void)
 		(eax & BIT(VCPU_LEGACY_X2APIC));
 }
 
+#ifdef CONFIG_INTEL_TDX_GUEST
+unsigned long vmware_tdx_hypercall(unsigned long cmd,
+				   struct tdx_module_args *args)
+{
+	if (!hypervisor_is_type(X86_HYPER_VMWARE))
+		return ULONG_MAX;
+
+	if (cmd & ~VMWARE_CMD_MASK) {
+		pr_warn_once("Out of range command %lx\n", cmd);
+		return ULONG_MAX;
+	}
+
+	args->r10 = VMWARE_TDX_VENDOR_LEAF;
+	args->r11 = VMWARE_TDX_HCALL_FUNC;
+	args->r12 = VMWARE_HYPERVISOR_MAGIC;
+	args->r13 = cmd;
+	args->r15 = 0; /* CPL */
+
+	__tdx_hypercall(args);
+
+	return args->r12;
+}
+EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
+#endif
+
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 static void vmware_sev_es_hcall_prepare(struct ghcb *ghcb,
 					struct pt_regs *regs)