[RFC,v5,026/104] KVM: TDX: x86: Add vm ioctl to get TDX systemwide parameters

Commit Message

Isaku Yamahata March 4, 2022, 7:48 p.m. UTC

From: Isaku Yamahata <[e-mail]>

Implement a VM-scoped subcomment to get system-wide parameters.  Although
this is system-wide parameters not per-VM, this subcomand is VM-scoped
because
- Device model needs TDX system-wide parameters after creating KVM VM.
- This subcommands requires to initialize TDX module.  For lazy
  initialization of the TDX module, vm-scope ioctl is better.

Signed-off-by: Sean Christopherson <[e-mail]>
Signed-off-by: Isaku Yamahata <[e-mail]>
---
 arch/x86/include/uapi/asm/kvm.h       | 22 ++++++++++++++
 arch/x86/kvm/vmx/tdx.c                | 41 +++++++++++++++++++++++++++
 tools/arch/x86/include/uapi/asm/kvm.h | 22 ++++++++++++++
 3 files changed, 85 insertions(+)

Comments

Paolo Bonzini April 5, 2022, 12:52 p.m. UTC | #1

On 3/4/22 20:48, [e-mail] wrote:
> Implement a VM-scoped subcomment to get system-wide parameters.  Although
> this is system-wide parameters not per-VM, this subcomand is VM-scoped
> because
> - Device model needs TDX system-wide parameters after creating KVM VM.
> - This subcommands requires to initialize TDX module.  For lazy
>    initialization of the TDX module, vm-scope ioctl is better.

Since there was agreement to install the TDX module on load, please 
place this ioctl on the /dev/kvm file descriptor.

At least for SEV, there were cases where the system-wide parameters are 
needed outside KVM, so it's better to avoid requiring a VM file descriptor.

Thanks,

Paolo

Xiaoyao Li April 6, 2022, 1:54 a.m. UTC | #2

On 4/5/2022 8:52 PM, Paolo Bonzini wrote:
> On 3/4/22 20:48, [e-mail] wrote:
>> Implement a VM-scoped subcomment to get system-wide parameters.  Although
>> this is system-wide parameters not per-VM, this subcomand is VM-scoped
>> because
>> - Device model needs TDX system-wide parameters after creating KVM VM.
>> - This subcommands requires to initialize TDX module.  For lazy
>>    initialization of the TDX module, vm-scope ioctl is better.
> 
> Since there was agreement to install the TDX module on load, please 
> place this ioctl on the /dev/kvm file descriptor.
> 
> At least for SEV, there were cases where the system-wide parameters are 
> needed outside KVM, so it's better to avoid requiring a VM file descriptor.

I don't have strong preference on KVM-scope ioctl or VM-scope.

Initially, we made it KVM-scope and change it to VM-scope in this 
version. Yes, it returns the info from TDX module, which doesn't vary 
per VM. However, what if we want to return different capabilities 
(software controlled capabilities) per VM? Part of the TDX capabilities 
serves like get_supported_cpuid, making it KVM wide lacks the 
flexibility to return differentiated capabilities for different TDs.


> Thanks,
> 
> Paolo
>

Huang, Kai April 7, 2022, 1:07 a.m. UTC | #3

On Wed, 2022-04-06 at 09:54 +0800, Xiaoyao Li wrote:
> On 4/5/2022 8:52 PM, Paolo Bonzini wrote:
> > On 3/4/22 20:48, [e-mail] wrote:
> > > Implement a VM-scoped subcomment to get system-wide parameters.  Although
> > > this is system-wide parameters not per-VM, this subcomand is VM-scoped
> > > because
> > > - Device model needs TDX system-wide parameters after creating KVM VM.
> > > - This subcommands requires to initialize TDX module.  For lazy
> > >    initialization of the TDX module, vm-scope ioctl is better.
> > 
> > Since there was agreement to install the TDX module on load, please 
> > place this ioctl on the /dev/kvm file descriptor.
> > 
> > At least for SEV, there were cases where the system-wide parameters are 
> > needed outside KVM, so it's better to avoid requiring a VM file descriptor.
> 
> I don't have strong preference on KVM-scope ioctl or VM-scope.
> 
> Initially, we made it KVM-scope and change it to VM-scope in this 
> version. Yes, it returns the info from TDX module, which doesn't vary 
> per VM. However, what if we want to return different capabilities 
> (software controlled capabilities) per VM? 
> 

In this case, you don't return different capabilities, instead, you return the
same capabilities but control the capabilities on per-VM basis.

> Part of the TDX capabilities 
> serves like get_supported_cpuid, making it KVM wide lacks the 
> flexibility to return differentiated capabilities for different TDs.
> 
> 
> > Thanks,
> > 
> > Paolo
> > 
>

Xiaoyao Li April 7, 2022, 1:17 a.m. UTC | #4

On 4/7/2022 9:07 AM, Kai Huang wrote:
> On Wed, 2022-04-06 at 09:54 +0800, Xiaoyao Li wrote:
>> On 4/5/2022 8:52 PM, Paolo Bonzini wrote:
>>> On 3/4/22 20:48, [e-mail] wrote:
>>>> Implement a VM-scoped subcomment to get system-wide parameters.  Although
>>>> this is system-wide parameters not per-VM, this subcomand is VM-scoped
>>>> because
>>>> - Device model needs TDX system-wide parameters after creating KVM VM.
>>>> - This subcommands requires to initialize TDX module.  For lazy
>>>>     initialization of the TDX module, vm-scope ioctl is better.
>>>
>>> Since there was agreement to install the TDX module on load, please
>>> place this ioctl on the /dev/kvm file descriptor.
>>>
>>> At least for SEV, there were cases where the system-wide parameters are
>>> needed outside KVM, so it's better to avoid requiring a VM file descriptor.
>>
>> I don't have strong preference on KVM-scope ioctl or VM-scope.
>>
>> Initially, we made it KVM-scope and change it to VM-scope in this
>> version. Yes, it returns the info from TDX module, which doesn't vary
>> per VM. However, what if we want to return different capabilities
>> (software controlled capabilities) per VM?
>>
> 
> In this case, you don't return different capabilities, instead, you return the
> same capabilities but control the capabilities on per-VM basis.

yes, so I'm not arguing it or insisting on per-VM.

I just speak out my concern since it's user ABI.

>> Part of the TDX capabilities
>> serves like get_supported_cpuid, making it KVM wide lacks the
>> flexibility to return differentiated capabilities for different TDs.
>>
>>
>>> Thanks,
>>>
>>> Paolo
>>>
>>
>

Isaku Yamahata April 8, 2022, 12:58 a.m. UTC | #5

On Thu, Apr 07, 2022 at 09:17:51AM +0800,
Xiaoyao Li <[e-mail]> wrote:

> On 4/7/2022 9:07 AM, Kai Huang wrote:
> > On Wed, 2022-04-06 at 09:54 +0800, Xiaoyao Li wrote:
> > > On 4/5/2022 8:52 PM, Paolo Bonzini wrote:
> > > > On 3/4/22 20:48, [e-mail] wrote:
> > > > > Implement a VM-scoped subcomment to get system-wide parameters.  Although
> > > > > this is system-wide parameters not per-VM, this subcomand is VM-scoped
> > > > > because
> > > > > - Device model needs TDX system-wide parameters after creating KVM VM.
> > > > > - This subcommands requires to initialize TDX module.  For lazy
> > > > >     initialization of the TDX module, vm-scope ioctl is better.
> > > > 
> > > > Since there was agreement to install the TDX module on load, please
> > > > place this ioctl on the /dev/kvm file descriptor.
> > > > 
> > > > At least for SEV, there were cases where the system-wide parameters are
> > > > needed outside KVM, so it's better to avoid requiring a VM file descriptor.
> > > 
> > > I don't have strong preference on KVM-scope ioctl or VM-scope.
> > > 
> > > Initially, we made it KVM-scope and change it to VM-scope in this
> > > version. Yes, it returns the info from TDX module, which doesn't vary
> > > per VM. However, what if we want to return different capabilities
> > > (software controlled capabilities) per VM?
> > > 
> > 
> > In this case, you don't return different capabilities, instead, you return the
> > same capabilities but control the capabilities on per-VM basis.
> 
> yes, so I'm not arguing it or insisting on per-VM.
> 
> I just speak out my concern since it's user ABI.

The reason why I made this API to VM-scope API is to reduce the number of patch
given qemu usage.  Now Paolo requested it, I'll change it KVM-scope API.

Patch

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 2ad61caf4e0b..70f9be4ea575 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -530,6 +530,8 @@  struct kvm_pmu_event_filter {
 
 /* Trust Domain eXtension sub-ioctl() commands. */
 enum kvm_tdx_cmd_id {
+	KVM_TDX_CAPABILITIES = 0,
+
 	KVM_TDX_CMD_NR_MAX,
 };
 
@@ -539,4 +541,24 @@  struct kvm_tdx_cmd {
 	__u64 data;
 };
 
+struct kvm_tdx_cpuid_config {
+	__u32 leaf;
+	__u32 sub_leaf;
+	__u32 eax;
+	__u32 ebx;
+	__u32 ecx;
+	__u32 edx;
+};
+
+struct kvm_tdx_capabilities {
+	__u64 attrs_fixed0;
+	__u64 attrs_fixed1;
+	__u64 xfam_fixed0;
+	__u64 xfam_fixed1;
+
+	__u32 nr_cpuid_configs;
+	__u32 padding;
+	struct kvm_tdx_cpuid_config cpuid_configs[0];
+};
+
 #endif /* _ASM_X86_KVM_H */
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 8c67444d052a..20b45bb0b032 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -349,6 +349,44 @@  int tdx_vm_init(struct kvm *kvm)
 	return ret;
 }
 
+static int tdx_capabilities(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
+{
+	struct kvm_tdx_capabilities __user *user_caps;
+	struct kvm_tdx_capabilities caps;
+
+	BUILD_BUG_ON(sizeof(struct kvm_tdx_cpuid_config) !=
+		     sizeof(struct tdx_cpuid_config));
+
+	WARN_ON(cmd->id != KVM_TDX_CAPABILITIES);
+	if (cmd->metadata)
+		return -EINVAL;
+
+	user_caps = (void __user *)cmd->data;
+	if (copy_from_user(&caps, user_caps, sizeof(caps)))
+		return -EFAULT;
+
+	if (caps.nr_cpuid_configs < tdx_caps.nr_cpuid_configs)
+		return -E2BIG;
+
+	caps = (struct kvm_tdx_capabilities) {
+		.attrs_fixed0 = tdx_caps.attrs_fixed0,
+		.attrs_fixed1 = tdx_caps.attrs_fixed1,
+		.xfam_fixed0 = tdx_caps.xfam_fixed0,
+		.xfam_fixed1 = tdx_caps.xfam_fixed1,
+		.nr_cpuid_configs = tdx_caps.nr_cpuid_configs,
+		.padding = 0,
+	};
+
+	if (copy_to_user(user_caps, &caps, sizeof(caps)))
+		return -EFAULT;
+	if (copy_to_user(user_caps->cpuid_configs, &tdx_caps.cpuid_configs,
+			 tdx_caps.nr_cpuid_configs *
+			 sizeof(struct tdx_cpuid_config)))
+		return -EFAULT;
+
+	return 0;
+}
+
 int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_tdx_cmd tdx_cmd;
@@ -360,6 +398,9 @@  int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
 	mutex_lock(&kvm->lock);
 
 	switch (tdx_cmd.id) {
+	case KVM_TDX_CAPABILITIES:
+		r = tdx_capabilities(kvm, &tdx_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
diff --git a/tools/arch/x86/include/uapi/asm/kvm.h b/tools/arch/x86/include/uapi/asm/kvm.h
index 2ad61caf4e0b..70f9be4ea575 100644
--- a/tools/arch/x86/include/uapi/asm/kvm.h
+++ b/tools/arch/x86/include/uapi/asm/kvm.h
@@ -530,6 +530,8 @@  struct kvm_pmu_event_filter {
 
 /* Trust Domain eXtension sub-ioctl() commands. */
 enum kvm_tdx_cmd_id {
+	KVM_TDX_CAPABILITIES = 0,
+
 	KVM_TDX_CMD_NR_MAX,
 };
 
@@ -539,4 +541,24 @@  struct kvm_tdx_cmd {
 	__u64 data;
 };
 
+struct kvm_tdx_cpuid_config {
+	__u32 leaf;
+	__u32 sub_leaf;
+	__u32 eax;
+	__u32 ebx;
+	__u32 ecx;
+	__u32 edx;
+};
+
+struct kvm_tdx_capabilities {
+	__u64 attrs_fixed0;
+	__u64 attrs_fixed1;
+	__u64 xfam_fixed0;
+	__u64 xfam_fixed1;
+
+	__u32 nr_cpuid_configs;
+	__u32 padding;
+	struct kvm_tdx_cpuid_config cpuid_configs[0];
+};
+
 #endif /* _ASM_X86_KVM_H */