Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Chao Gao <chao.gao@intel.com>]

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;
>+}