Re: [PATCH v6 7/7] x86/vmware: Add TDX hypercall support
On 1/22/24 4:17 PM, H. Peter Anvin wrote:
On January 22, 2024 4:04:33 PM PST, Alexey Makhalov
wrote:
On 1/22/24 10:28 AM, H. Peter Anvin wrote:
On January 22, 2024 8:32:22 AM PST, Dave Hansen 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
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
Re: [PATCH v6 7/7] x86/vmware: Add TDX hypercall support
On January 22, 2024 4:04:33 PM PST, Alexey Makhalov
wrote:
>
>
>On 1/22/24 10:28 AM, H. Peter Anvin wrote:
>> On January 22, 2024 8:32:22 AM PST, Dave Hansen
>> 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
>>>
>>
>> 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...
Re: [PATCH v6 7/7] x86/vmware: Add TDX hypercall support
On 1/22/24 10:28 AM, H. Peter Anvin wrote:
On January 22, 2024 8:32:22 AM PST, Dave Hansen 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
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?
Re: [PATCH v6 7/7] x86/vmware: Add TDX hypercall support
On January 22, 2024 8:32:22 AM PST, Dave Hansen 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
>
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.
Re: [PATCH v6 7/7] x86/vmware: Add TDX hypercall support
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
