| Message ID | 20240812224820.34826-11-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | TDX vCPU/VM creation | expand |
On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote: >From: Xiaoyao Li <xiaoyao.li@intel.com> > >While TDX module reports a set of capabilities/features that it >supports, what KVM currently supports might be a subset of them. >E.g., DEBUG and PERFMON are supported by TDX module but currently not >supported by KVM. > >Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. >supported_attrs and suppported_xfam are validated against fixed0/1 >values enumerated by TDX module. Configurable CPUID bits derive from TDX >module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), >i.e., mask off the bits that are configurable in the view of TDX module >but not supported by KVM yet. > >KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 >and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. If we convert KVM_TDX_CPUID_NO_SUBLEAF to 0 when reporting capabilities to QEMU, QEMU cannot distinguish a CPUID subleaf 0 from a CPUID w/o subleaf. Does it matter to QEMU? > >Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >--- >uAPI breakout v1: > - Change setup_kvm_tdx_caps() to use the exported 'struct tdx_sysinfo' > pointer. > - Change how to copy 'kvm_tdx_cpuid_config' since 'struct tdx_sysinfo' > doesn't have 'kvm_tdx_cpuid_config'. > - Updates for uAPI changes >--- > arch/x86/include/uapi/asm/kvm.h | 2 - > arch/x86/kvm/vmx/tdx.c | 81 +++++++++++++++++++++++++++++++++ > 2 files changed, 81 insertions(+), 2 deletions(-) > >diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h >index 47caf508cca7..c9eb2e2f5559 100644 >--- a/arch/x86/include/uapi/asm/kvm.h >+++ b/arch/x86/include/uapi/asm/kvm.h >@@ -952,8 +952,6 @@ struct kvm_tdx_cmd { > __u64 hw_error; > }; > >-#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) >- This definition can be dropped from the previous patch because it isn't used there. > struct kvm_tdx_cpuid_config { > __u32 leaf; > __u32 sub_leaf; >diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c >index 90b44ebaf864..d89973e554f6 100644 >--- a/arch/x86/kvm/vmx/tdx.c >+++ b/arch/x86/kvm/vmx/tdx.c >@@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid) > ida_free(&tdx_guest_keyid_pool, keyid); > } > >+#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) >+ >+struct kvm_tdx_caps { >+ u64 supported_attrs; >+ u64 supported_xfam; >+ >+ u16 num_cpuid_config; >+ /* This must the last member. */ >+ DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs); >+}; >+ >+static struct kvm_tdx_caps *kvm_tdx_caps; >+ > static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) > { > const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; >@@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) > return r; > } > >+#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE) >+ >+static int __init setup_kvm_tdx_caps(void) >+{ >+ const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; >+ u64 kvm_supported; >+ int i; >+ >+ kvm_tdx_caps = kzalloc(sizeof(*kvm_tdx_caps) + >+ sizeof(struct kvm_tdx_cpuid_config) * td_conf->num_cpuid_config, struct_size() >+ GFP_KERNEL); >+ if (!kvm_tdx_caps) >+ return -ENOMEM; >+ >+ kvm_supported = KVM_SUPPORTED_TD_ATTRS; >+ if ((kvm_supported & td_conf->attributes_fixed1) != td_conf->attributes_fixed1) >+ goto err; >+ >+ kvm_tdx_caps->supported_attrs = kvm_supported & td_conf->attributes_fixed0; >+ >+ kvm_supported = kvm_caps.supported_xcr0 | kvm_caps.supported_xss; >+ >+ /* >+ * PT and CET can be exposed to TD guest regardless of KVM's XSS, PT >+ * and, CET support. >+ */ >+ kvm_supported |= XFEATURE_MASK_PT | XFEATURE_MASK_CET_USER | >+ XFEATURE_MASK_CET_KERNEL; I prefer to add PT/CET bits in separate patches because PT/CET related MSRs may need save/restore. Putting them in separate patches can give us the chance to explain this in detail. >+ if ((kvm_supported & td_conf->xfam_fixed1) != td_conf->xfam_fixed1) >+ goto err; >+ >+ kvm_tdx_caps->supported_xfam = kvm_supported & td_conf->xfam_fixed0; >+ >+ kvm_tdx_caps->num_cpuid_config = td_conf->num_cpuid_config; >+ for (i = 0; i < td_conf->num_cpuid_config; i++) { >+ struct kvm_tdx_cpuid_config source = { >+ .leaf = (u32)td_conf->cpuid_config_leaves[i], >+ .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, >+ .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, >+ .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, >+ .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, >+ .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, >+ }; >+ struct kvm_tdx_cpuid_config *dest = >+ &kvm_tdx_caps->cpuid_configs[i]; >+ >+ memcpy(dest, &source, sizeof(struct kvm_tdx_cpuid_config)); this memcpy() looks superfluous. does this work? kvm_tdx_caps->cpuid_configs[i] = { .leaf = (u32)td_conf->cpuid_config_leaves[i], .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, }; >+ if (dest->sub_leaf == KVM_TDX_CPUID_NO_SUBLEAF) >+ dest->sub_leaf = 0; >+ } >+ >+ return 0; >+err: >+ kfree(kvm_tdx_caps); >+ return -EIO; >+}
On Tue, 2024-08-13 at 11:25 +0800, Chao Gao wrote: > > + for (i = 0; i < td_conf->num_cpuid_config; i++) { > > + struct kvm_tdx_cpuid_config source = { > > + .leaf = (u32)td_conf->cpuid_config_leaves[i], > > + .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, > > + .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, > > + .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, > > + .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, > > + .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, > > + }; > > + struct kvm_tdx_cpuid_config *dest = > > + &kvm_tdx_caps->cpuid_configs[i]; > > + > > + memcpy(dest, &source, sizeof(struct kvm_tdx_cpuid_config)); > > this memcpy() looks superfluous. does this work? > > kvm_tdx_caps->cpuid_configs[i] = { > .leaf = (u32)td_conf->cpuid_config_leaves[i], > .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, > .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, > .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, > .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, > .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, > }; This looks good to me. I didn't try to optimize because it's done in the module loading time.
On 8/13/2024 11:25 AM, Chao Gao wrote: > On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote: >> From: Xiaoyao Li <xiaoyao.li@intel.com> >> >> While TDX module reports a set of capabilities/features that it >> supports, what KVM currently supports might be a subset of them. >> E.g., DEBUG and PERFMON are supported by TDX module but currently not >> supported by KVM. >> >> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. >> supported_attrs and suppported_xfam are validated against fixed0/1 >> values enumerated by TDX module. Configurable CPUID bits derive from TDX >> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), >> i.e., mask off the bits that are configurable in the view of TDX module >> but not supported by KVM yet. >> >> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 >> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. > If we convert KVM_TDX_CPUID_NO_SUBLEAF to 0 when reporting capabilities to > QEMU, QEMU cannot distinguish a CPUID subleaf 0 from a CPUID w/o subleaf. > Does it matter to QEMU? According to "and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM". IIUC, KVM's ABI uses KVM_CPUID_FLAG_SIGNIFCANT_INDEX in flags of struct kvm_cpuid_entry2 to distinguish whether the index is significant.
On Tue, Aug 13, 2024 at 03:24:32PM +0800, Binbin Wu wrote: > > > >On 8/13/2024 11:25 AM, Chao Gao wrote: >> On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote: >> > From: Xiaoyao Li <xiaoyao.li@intel.com> >> > >> > While TDX module reports a set of capabilities/features that it >> > supports, what KVM currently supports might be a subset of them. >> > E.g., DEBUG and PERFMON are supported by TDX module but currently not >> > supported by KVM. >> > >> > Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. >> > supported_attrs and suppported_xfam are validated against fixed0/1 >> > values enumerated by TDX module. Configurable CPUID bits derive from TDX >> > module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), >> > i.e., mask off the bits that are configurable in the view of TDX module >> > but not supported by KVM yet. >> > >> > KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 >> > and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. >> If we convert KVM_TDX_CPUID_NO_SUBLEAF to 0 when reporting capabilities to >> QEMU, QEMU cannot distinguish a CPUID subleaf 0 from a CPUID w/o subleaf. >> Does it matter to QEMU? > >According to "and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the >concept of KVM". IIUC, KVM's ABI uses KVM_CPUID_FLAG_SIGNIFCANT_INDEX >in flags of struct kvm_cpuid_entry2 to distinguish whether the index >is significant. If KVM doesn't indicate which CPU leaf doesn't support subleafs when reporting TDX capabilities, how can QEMU know whether it should set the KVM_CPUID_FLAG_SIGNIFICANT_INDEX flag for a given CPUID leaf? Or is the expectation that QEMU can discover that on its own?
On 8/14/2024 8:26 AM, Chao Gao wrote: > On Tue, Aug 13, 2024 at 03:24:32PM +0800, Binbin Wu wrote: >> >> >> On 8/13/2024 11:25 AM, Chao Gao wrote: >>> On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote: >>>> From: Xiaoyao Li <xiaoyao.li@intel.com> >>>> >>>> While TDX module reports a set of capabilities/features that it >>>> supports, what KVM currently supports might be a subset of them. >>>> E.g., DEBUG and PERFMON are supported by TDX module but currently not >>>> supported by KVM. >>>> >>>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. >>>> supported_attrs and suppported_xfam are validated against fixed0/1 >>>> values enumerated by TDX module. Configurable CPUID bits derive from TDX >>>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), >>>> i.e., mask off the bits that are configurable in the view of TDX module >>>> but not supported by KVM yet. >>>> >>>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 >>>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. >>> If we convert KVM_TDX_CPUID_NO_SUBLEAF to 0 when reporting capabilities to >>> QEMU, QEMU cannot distinguish a CPUID subleaf 0 from a CPUID w/o subleaf. >>> Does it matter to QEMU? >> According to "and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the >> concept of KVM". IIUC, KVM's ABI uses KVM_CPUID_FLAG_SIGNIFCANT_INDEX >> in flags of struct kvm_cpuid_entry2 to distinguish whether the index >> is significant. > If KVM doesn't indicate which CPU leaf doesn't support subleafs when reporting > TDX capabilities, how can QEMU know whether it should set the > KVM_CPUID_FLAG_SIGNIFICANT_INDEX flag for a given CPUID leaf? Or is the > expectation that QEMU can discover that on its own? > When KVM report CPUIDs to userspace, for the entries that index is significant, it will set KVM_CPUID_FLAG_SIGNIFICANT_INDEX, including reporting CPUIDs for TDX. QEMU can check the flag to see if the subleaf is significant or not. On the other side, when QEMU build its own version, it also set KVM_CPUID_FLAG_SIGNIFICANT_INDEX for the entries that index is significant.
On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote: > From: Xiaoyao Li <xiaoyao.li@intel.com> > > While TDX module reports a set of capabilities/features that it > supports, what KVM currently supports might be a subset of them. > E.g., DEBUG and PERFMON are supported by TDX module but currently not > supported by KVM. > > Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. > supported_attrs and suppported_xfam are validated against fixed0/1 > values enumerated by TDX module. Configurable CPUID bits derive from TDX > module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), > i.e., mask off the bits that are configurable in the view of TDX module > but not supported by KVM yet. > But this mask is not implemented in this patch, which should be in patch24? > KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 > and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> [...]
On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote: > From: Xiaoyao Li <xiaoyao.li@intel.com> > > While TDX module reports a set of capabilities/features that it > supports, what KVM currently supports might be a subset of them. > E.g., DEBUG and PERFMON are supported by TDX module but currently not > supported by KVM. > > Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. > supported_attrs and suppported_xfam are validated against fixed0/1 > values enumerated by TDX module. Configurable CPUID bits derive from TDX > module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), > i.e., mask off the bits that are configurable in the view of TDX module > but not supported by KVM yet. > > KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 > and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > uAPI breakout v1: > - Change setup_kvm_tdx_caps() to use the exported 'struct tdx_sysinfo' > pointer. > - Change how to copy 'kvm_tdx_cpuid_config' since 'struct tdx_sysinfo' > doesn't have 'kvm_tdx_cpuid_config'. > - Updates for uAPI changes > --- <snip> > + > static int tdx_online_cpu(unsigned int cpu) > { > unsigned long flags; > @@ -217,11 +292,16 @@ static int __init __tdx_bringup(void) > goto get_sysinfo_err; > } > > + r = setup_kvm_tdx_caps(); nit: Since there are other similarly named functions that come later how about rename this to init_kvm_tdx_caps, so that it's clear that the functions that are executed ones are prefixed with "init_" and those that will be executed on every TDV boot up can be named prefixed with "setup_" > + if (r) > + goto get_sysinfo_err; > + > /* > * Leave hardware virtualization enabled after TDX is enabled > * successfully. TDX CPU hotplug depends on this. > */ > return 0; > + > get_sysinfo_err: > __do_tdx_cleanup(); > tdx_bringup_err: > @@ -232,6 +312,7 @@ static int __init __tdx_bringup(void) > void tdx_cleanup(void) > { > if (enable_tdx) { > + free_kvm_tdx_cap(); > __do_tdx_cleanup(); > kvm_disable_virtualization(); > }
On Mon, Aug 26, 2024 at 02:04:27PM +0300, Nikolay Borisov wrote: > On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote: > > From: Xiaoyao Li <xiaoyao.li@intel.com> > > static int tdx_online_cpu(unsigned int cpu) > > { > > unsigned long flags; > > @@ -217,11 +292,16 @@ static int __init __tdx_bringup(void) > > goto get_sysinfo_err; > > } > > + r = setup_kvm_tdx_caps(); > > nit: Since there are other similarly named functions that come later how > about rename this to init_kvm_tdx_caps, so that it's clear that the > functions that are executed ones are prefixed with "init_" and those that > will be executed on every TDV boot up can be named prefixed with "setup_" We can call setup_kvm_tdx_caps() from from tdx_get_kvm_supported_cpuid(), and drop the struct kvm_tdx_caps. So then the setup_kvm_tdx_caps() should be OK. Regards, Tony
On 8/19/2024 9:33 AM, Tao Su wrote: > On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote: >> From: Xiaoyao Li <xiaoyao.li@intel.com> >> >> While TDX module reports a set of capabilities/features that it >> supports, what KVM currently supports might be a subset of them. >> E.g., DEBUG and PERFMON are supported by TDX module but currently not >> supported by KVM. >> >> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. >> supported_attrs and suppported_xfam are validated against fixed0/1 >> values enumerated by TDX module. Configurable CPUID bits derive from TDX >> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), >> i.e., mask off the bits that are configurable in the view of TDX module >> but not supported by KVM yet. >> > > But this mask is not implemented in this patch, which should be in patch24? yes, the commit message needs to be updated. Even more the patches need to be re-organized. >> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 >> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > [...]
On Tue, Aug 13, 2024 at 11:25:37AM +0800, Chao Gao wrote: > On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote: > >From: Xiaoyao Li <xiaoyao.li@intel.com> > >+static int __init setup_kvm_tdx_caps(void) > >+{ > >+ const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; > >+ u64 kvm_supported; > >+ int i; > >+ > >+ kvm_tdx_caps = kzalloc(sizeof(*kvm_tdx_caps) + > >+ sizeof(struct kvm_tdx_cpuid_config) * td_conf->num_cpuid_config, > > struct_size() > > >+ GFP_KERNEL); > >+ if (!kvm_tdx_caps) > >+ return -ENOMEM; This will go away with the dropping of struct kvm_tdx_caps. Should be checked for other places though. Regards, Tony
On Tue, Aug 13, 2024 at 05:26:06AM +0000, Huang, Kai wrote: > On Tue, 2024-08-13 at 11:25 +0800, Chao Gao wrote: > > > + for (i = 0; i < td_conf->num_cpuid_config; i++) { > > > + struct kvm_tdx_cpuid_config source = { > > > + .leaf = (u32)td_conf->cpuid_config_leaves[i], > > > + .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, > > > + .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, > > > + .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, > > > + .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, > > > + .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, > > > + }; > > > + struct kvm_tdx_cpuid_config *dest = > > > + &kvm_tdx_caps->cpuid_configs[i]; > > > + > > > + memcpy(dest, &source, sizeof(struct kvm_tdx_cpuid_config)); > > > > this memcpy() looks superfluous. does this work? > > > > kvm_tdx_caps->cpuid_configs[i] = { > > .leaf = (u32)td_conf->cpuid_config_leaves[i], > > .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, > > .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, > > .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, > > .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, > > .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, > > }; > > This looks good to me. I didn't try to optimize because it's done in the > module loading time. I'll do a patch to initialize dest directly without a memcpy(). Tony
On Tue, 2024-08-13 at 11:25 +0800, Chao Gao wrote: > > + /* > > + * PT and CET can be exposed to TD guest regardless of KVM's XSS, PT > > + * and, CET support. > > + */ > > + kvm_supported |= XFEATURE_MASK_PT | XFEATURE_MASK_CET_USER | > > + XFEATURE_MASK_CET_KERNEL; > > I prefer to add PT/CET bits in separate patches because PT/CET related MSRs > may > need save/restore. Putting them in separate patches can give us the chance to > explain this in detail. I think we should just drop them from the base series to save required testing. We can leave them for the future.
On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote: > From: Xiaoyao Li <xiaoyao.li@intel.com> > > While TDX module reports a set of capabilities/features that it > supports, what KVM currently supports might be a subset of them. > E.g., DEBUG and PERFMON are supported by TDX module but currently not > supported by KVM. > > Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. > supported_attrs and suppported_xfam are validated against fixed0/1 > values enumerated by TDX module. Configurable CPUID bits derive from TDX > module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), > i.e., mask off the bits that are configurable in the view of TDX module > but not supported by KVM yet. > > KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 > and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > uAPI breakout v1: > - Change setup_kvm_tdx_caps() to use the exported 'struct tdx_sysinfo' > pointer. > - Change how to copy 'kvm_tdx_cpuid_config' since 'struct tdx_sysinfo' > doesn't have 'kvm_tdx_cpuid_config'. > - Updates for uAPI changes > --- > arch/x86/include/uapi/asm/kvm.h | 2 - > arch/x86/kvm/vmx/tdx.c | 81 +++++++++++++++++++++++++++++++++ > 2 files changed, 81 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h > index 47caf508cca7..c9eb2e2f5559 100644 > --- a/arch/x86/include/uapi/asm/kvm.h > +++ b/arch/x86/include/uapi/asm/kvm.h > @@ -952,8 +952,6 @@ struct kvm_tdx_cmd { > __u64 hw_error; > }; > > -#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) > - > struct kvm_tdx_cpuid_config { > __u32 leaf; > __u32 sub_leaf; > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > index 90b44ebaf864..d89973e554f6 100644 > --- a/arch/x86/kvm/vmx/tdx.c > +++ b/arch/x86/kvm/vmx/tdx.c > @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid) > ida_free(&tdx_guest_keyid_pool, keyid); > } > > +#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) > + > +struct kvm_tdx_caps { > + u64 supported_attrs; > + u64 supported_xfam; > + > + u16 num_cpuid_config; > + /* This must the last member. */ > + DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs); > +}; > + > +static struct kvm_tdx_caps *kvm_tdx_caps; > + > static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) > { > const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; > @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) > return r; > } > > +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE) Why isn't TDX_TD_ATTR_DEBUG added as well? <snip>
On 9/4/2024 7:58 PM, Nikolay Borisov wrote: > > > On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote: >> From: Xiaoyao Li <xiaoyao.li@intel.com> >> >> While TDX module reports a set of capabilities/features that it >> supports, what KVM currently supports might be a subset of them. >> E.g., DEBUG and PERFMON are supported by TDX module but currently not >> supported by KVM. >> >> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX. >> supported_attrs and suppported_xfam are validated against fixed0/1 >> values enumerated by TDX module. Configurable CPUID bits derive from TDX >> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID), >> i.e., mask off the bits that are configurable in the view of TDX module >> but not supported by KVM yet. >> >> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0 >> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >> --- >> uAPI breakout v1: >> - Change setup_kvm_tdx_caps() to use the exported 'struct tdx_sysinfo' >> pointer. >> - Change how to copy 'kvm_tdx_cpuid_config' since 'struct tdx_sysinfo' >> doesn't have 'kvm_tdx_cpuid_config'. >> - Updates for uAPI changes >> --- >> arch/x86/include/uapi/asm/kvm.h | 2 - >> arch/x86/kvm/vmx/tdx.c | 81 +++++++++++++++++++++++++++++++++ >> 2 files changed, 81 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/include/uapi/asm/kvm.h >> b/arch/x86/include/uapi/asm/kvm.h >> index 47caf508cca7..c9eb2e2f5559 100644 >> --- a/arch/x86/include/uapi/asm/kvm.h >> +++ b/arch/x86/include/uapi/asm/kvm.h >> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd { >> __u64 hw_error; >> }; >> -#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) >> - >> struct kvm_tdx_cpuid_config { >> __u32 leaf; >> __u32 sub_leaf; >> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c >> index 90b44ebaf864..d89973e554f6 100644 >> --- a/arch/x86/kvm/vmx/tdx.c >> +++ b/arch/x86/kvm/vmx/tdx.c >> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid) >> ida_free(&tdx_guest_keyid_pool, keyid); >> } >> +#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) >> + >> +struct kvm_tdx_caps { >> + u64 supported_attrs; >> + u64 supported_xfam; >> + >> + u16 num_cpuid_config; >> + /* This must the last member. */ >> + DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs); >> +}; >> + >> +static struct kvm_tdx_caps *kvm_tdx_caps; >> + >> static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) >> { >> const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; >> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) >> return r; >> } >> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE) > > Why isn't TDX_TD_ATTR_DEBUG added as well? Because so far KVM doesn't support all the features of a DEBUG TD for userspace. e.g., KVM doesn't provide interface for userspace to read/write private memory of DEBUG TD. > <snip>
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h index 47caf508cca7..c9eb2e2f5559 100644 --- a/arch/x86/include/uapi/asm/kvm.h +++ b/arch/x86/include/uapi/asm/kvm.h @@ -952,8 +952,6 @@ struct kvm_tdx_cmd { __u64 hw_error; }; -#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) - struct kvm_tdx_cpuid_config { __u32 leaf; __u32 sub_leaf; diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index 90b44ebaf864..d89973e554f6 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid) ida_free(&tdx_guest_keyid_pool, keyid); } +#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) + +struct kvm_tdx_caps { + u64 supported_attrs; + u64 supported_xfam; + + u16 num_cpuid_config; + /* This must the last member. */ + DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs); +}; + +static struct kvm_tdx_caps *kvm_tdx_caps; + static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) { const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) return r; } +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE) + +static int __init setup_kvm_tdx_caps(void) +{ + const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; + u64 kvm_supported; + int i; + + kvm_tdx_caps = kzalloc(sizeof(*kvm_tdx_caps) + + sizeof(struct kvm_tdx_cpuid_config) * td_conf->num_cpuid_config, + GFP_KERNEL); + if (!kvm_tdx_caps) + return -ENOMEM; + + kvm_supported = KVM_SUPPORTED_TD_ATTRS; + if ((kvm_supported & td_conf->attributes_fixed1) != td_conf->attributes_fixed1) + goto err; + + kvm_tdx_caps->supported_attrs = kvm_supported & td_conf->attributes_fixed0; + + kvm_supported = kvm_caps.supported_xcr0 | kvm_caps.supported_xss; + + /* + * PT and CET can be exposed to TD guest regardless of KVM's XSS, PT + * and, CET support. + */ + kvm_supported |= XFEATURE_MASK_PT | XFEATURE_MASK_CET_USER | + XFEATURE_MASK_CET_KERNEL; + if ((kvm_supported & td_conf->xfam_fixed1) != td_conf->xfam_fixed1) + goto err; + + kvm_tdx_caps->supported_xfam = kvm_supported & td_conf->xfam_fixed0; + + kvm_tdx_caps->num_cpuid_config = td_conf->num_cpuid_config; + for (i = 0; i < td_conf->num_cpuid_config; i++) { + struct kvm_tdx_cpuid_config source = { + .leaf = (u32)td_conf->cpuid_config_leaves[i], + .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, + .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, + .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, + .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, + .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, + }; + struct kvm_tdx_cpuid_config *dest = + &kvm_tdx_caps->cpuid_configs[i]; + + memcpy(dest, &source, sizeof(struct kvm_tdx_cpuid_config)); + if (dest->sub_leaf == KVM_TDX_CPUID_NO_SUBLEAF) + dest->sub_leaf = 0; + } + + return 0; +err: + kfree(kvm_tdx_caps); + return -EIO; +} + +static void free_kvm_tdx_cap(void) +{ + kfree(kvm_tdx_caps); +} + static int tdx_online_cpu(unsigned int cpu) { unsigned long flags; @@ -217,11 +292,16 @@ static int __init __tdx_bringup(void) goto get_sysinfo_err; } + r = setup_kvm_tdx_caps(); + if (r) + goto get_sysinfo_err; + /* * Leave hardware virtualization enabled after TDX is enabled * successfully. TDX CPU hotplug depends on this. */ return 0; + get_sysinfo_err: __do_tdx_cleanup(); tdx_bringup_err: @@ -232,6 +312,7 @@ static int __init __tdx_bringup(void) void tdx_cleanup(void) { if (enable_tdx) { + free_kvm_tdx_cap(); __do_tdx_cleanup(); kvm_disable_virtualization(); }