Future TDX modules may provide support for future HW features, but run with
KVM versions that lack support for them. In this case, userspace may try to
use features that KVM does not have support, and develop assumptions around
KVM's behavior. Then KVM would have to deal with not breaking such
userspace.

Simplify KVM's job by preventing userspace from configuring any unsupported
CPUID feature bits.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
uAPI breakout v1:
 - New patch
---
 arch/x86/kvm/vmx/tdx.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index c6bfeb0b3cc9..d45b4f7b69ba 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1086,8 +1086,9 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
 	return ret;
 }
 
-static int __maybe_unused tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
+static int tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
 {
+
 	int r;
 	static const u32 funcs[] = {
 		0, 0x80000000, KVM_CPUID_SIGNATURE,
@@ -1235,8 +1236,10 @@ int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
 static int __init setup_kvm_tdx_caps(void)
 {
 	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
+	struct kvm_cpuid_entry2 *cpuid_e;
+	struct kvm_cpuid2 *supported_cpuid;
 	u64 kvm_supported;
-	int i;
+	int i, r = -EIO;
 
 	kvm_tdx_caps = kzalloc(sizeof(*kvm_tdx_caps) +
 			       sizeof(struct kvm_tdx_cpuid_config) * td_conf->num_cpuid_config,
@@ -1263,6 +1266,10 @@ static int __init setup_kvm_tdx_caps(void)
 
 	kvm_tdx_caps->supported_xfam = kvm_supported & td_conf->xfam_fixed0;
 
+	r = tdx_get_kvm_supported_cpuid(&supported_cpuid);
+	if (r)
+		goto err;
+
 	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 = {
@@ -1283,12 +1290,24 @@ static int __init setup_kvm_tdx_caps(void)
 		/* Work around missing support on old TDX modules */
 		if (dest->leaf == 0x80000008)
 			dest->eax |= 0x00ff0000;
+
+		cpuid_e = kvm_find_cpuid_entry2(supported_cpuid->entries, supported_cpuid->nent,
+						dest->leaf, dest->sub_leaf);
+		if (!cpuid_e) {
+			dest->eax = dest->ebx = dest->ecx = dest->edx = 0;
+		} else {
+			dest->eax &= cpuid_e->eax;
+			dest->ebx &= cpuid_e->ebx;
+			dest->ecx &= cpuid_e->ecx;
+			dest->edx &= cpuid_e->edx;
+		}
 	}
 
+	kfree(supported_cpuid);
 	return 0;
 err:
 	kfree(kvm_tdx_caps);
-	return -EIO;
+	return r;
 }
 
 static void free_kvm_tdx_cap(void)
-- 
2.34.1

On 8/13/24 00:48, Rick Edgecombe wrote:
> +
> +		cpuid_e = kvm_find_cpuid_entry2(supported_cpuid->entries, supported_cpuid->nent,
> +						dest->leaf, dest->sub_leaf);
> +		if (!cpuid_e) {
> +			dest->eax = dest->ebx = dest->ecx = dest->edx = 0;
> +		} else {
> +			dest->eax &= cpuid_e->eax;
> +			dest->ebx &= cpuid_e->ebx;
> +			dest->ecx &= cpuid_e->ecx;
> +			dest->edx &= cpuid_e->edx;
> +		}

This can only work with CPUID entries that consists of 4*32 features, so 
it has to be done specifically for each leaf, unfortunately.  I suggest 
defining a kvm_merge_cpuid_entries in cpuid.c that takes two struct 
cpuid_entry2* that refer to the same leaf and subleaf.

Paolo

On Mon, Aug 12, 2024 at 03:48:19PM -0700, Rick Edgecombe wrote:
> Future TDX modules may provide support for future HW features, but run with
> KVM versions that lack support for them. In this case, userspace may try to
> use features that KVM does not have support, and develop assumptions around
> KVM's behavior. Then KVM would have to deal with not breaking such
> userspace.
> 
> Simplify KVM's job by preventing userspace from configuring any unsupported
> CPUID feature bits.
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v1:
>  - New patch
> ---
>  arch/x86/kvm/vmx/tdx.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index c6bfeb0b3cc9..d45b4f7b69ba 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1086,8 +1086,9 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
>  	return ret;
>  }
>  
> -static int __maybe_unused tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
> +static int tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)

This func is already used in patch #21, put the change in that patch.

>  {
> +

remove the blank line.

>  	int r;

Thanks,
Yilun

On Mon, Aug 19, 2024 at 01:02:41PM +0800, Xu Yilun wrote:
> On Mon, Aug 12, 2024 at 03:48:19PM -0700, Rick Edgecombe wrote:
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1086,8 +1086,9 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
> >  	return ret;
> >  }
> >  
> > -static int __maybe_unused tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
> > +static int tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
> 
> This func is already used in patch #21, put the change in that patch.
> 
> >  {
> > +
> 
> remove the blank line.
> 
> >  	int r;

Yes looks like that got removed in patch 25/25.

Tony