On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote:
>From: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
>
>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@xxxxxxxxx>
>Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
>---
>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;
>+}
