linux-kernel - Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural definitions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240305082138.GD10568@ls.amr.corp.intel.com>
Date: Tue, 5 Mar 2024 00:21:38 -0800
From: Isaku Yamahata <isaku.yamahata@...ux.intel.com>
To: Yan Zhao <yan.y.zhao@...el.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
	Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
	Sean Christopherson <seanjc@...gle.com>,
	Sagi Shahar <sagis@...gle.com>, Kai Huang <kai.huang@...el.com>,
	chen.bo@...el.com, hang.yuan@...el.com, tina.zhang@...el.com,
	Sean Christopherson <sean.j.christopherson@...el.com>,
	Xiaoyao Li <xiaoyao.li@...el.com>, isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural
 definitions

On Fri, Mar 01, 2024 at 03:25:31PM +0800,
Yan Zhao <yan.y.zhao@...el.com> wrote:

> > + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is 1024B.
> > + */
> > +#define TDX_MAX_VCPUS	(~(u16)0)
> This value will be treated as -1 in tdx_vm_init(),
> 	"kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);"
> 
> This will lead to kvm->max_vcpus being -1 by default.
> Is this by design or just an error?
> If it's by design, why not set kvm->max_vcpus = -1 in tdx_vm_init() directly.
> If an unexpected error, may below is better?
> 
> #define TDX_MAX_VCPUS   (int)((u16)(~0UL))
> or
> #define TDX_MAX_VCPUS 65536

You're right. I'll use ((int)U16_MAX).
As TDX 1.5 introduced metadata MAX_VCPUS_PER_TD, I'll update to get the value
and trim it further. Something following.

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index f964d99f8701..31205f84d594 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -23,7 +23,6 @@ KVM_X86_OP(has_emulated_msr)
 /* TODO: Once all backend implemented this op, remove _OPTIONAL_RET0. */
 KVM_X86_OP_OPTIONAL_RET0(vcpu_check_cpuid)
 KVM_X86_OP(vcpu_after_set_cpuid)
-KVM_X86_OP_OPTIONAL(max_vcpus);
 KVM_X86_OP_OPTIONAL(vm_enable_cap)
 KVM_X86_OP(vm_init)
 KVM_X86_OP_OPTIONAL(flush_shadow_all_private)
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 6dd78230c9d4..deb59e94990f 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -13,17 +13,6 @@
 static bool enable_tdx __ro_after_init;
 module_param_named(tdx, enable_tdx, bool, 0444);
 
-static int vt_max_vcpus(struct kvm *kvm)
-{
-	if (!kvm)
-		return KVM_MAX_VCPUS;
-
-	if (is_td(kvm))
-		return min(kvm->max_vcpus, TDX_MAX_VCPUS);
-
-	return kvm->max_vcpus;
-}
-
 #if IS_ENABLED(CONFIG_HYPERV) || IS_ENABLED(CONFIG_INTEL_TDX_HOST)
 static int vt_flush_remote_tlbs(struct kvm *kvm);
 static int vt_flush_remote_tlbs_range(struct kvm *kvm, gfn_t gfn, gfn_t nr_pages);
@@ -1130,7 +1119,6 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
 	.hardware_disable = vt_hardware_disable,
 	.has_emulated_msr = vt_has_emulated_msr,
 
-	.max_vcpus = vt_max_vcpus,
 	.vm_size = sizeof(struct kvm_vmx),
 	.vm_enable_cap = vt_vm_enable_cap,
 	.vm_init = vt_vm_init,
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 7cca6a33ad97..a8cfb4f214a6 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -63,6 +63,8 @@ struct tdx_info {
 	u8 nr_tdcs_pages;
 	u8 nr_tdvpx_pages;
 
+	u16 max_vcpus_per_td;
+
 	/*
 	 * The number of WBINVD domains. 0 means that wbinvd domain is cpu
 	 * package.
@@ -100,7 +102,8 @@ int tdx_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
 		if (cap->flags || cap->args[0] == 0)
 			return -EINVAL;
 		if (cap->args[0] > KVM_MAX_VCPUS ||
-		    cap->args[0] > TDX_MAX_VCPUS)
+		    cap->args[0] > TDX_MAX_VCPUS ||
+		    cap->args[0] > tdx_info->max_vcpus_per_td)
 			return -E2BIG;
 
 		mutex_lock(&kvm->lock);
@@ -729,7 +732,8 @@ int tdx_vm_init(struct kvm *kvm)
 	 * TDX has its own limit of the number of vcpus in addition to
 	 * KVM_MAX_VCPUS.
 	 */
-	kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);
+	kvm->max_vcpus = min3(kvm->max_vcpus, tdx_info->max_vcpus_per_td,
+			     TDX_MAX_VCPUS);
 
 	mutex_init(&to_kvm_tdx(kvm)->source_lock);
 	return 0;
@@ -4667,6 +4671,7 @@ static int __init tdx_module_setup(void)
 		TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
 		TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
 		TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
+		TDX_INFO_MAP(MAX_VCPUS_PER_TD, max_vcpus_per_td),
 	};
 
 	ret = tdx_enable();
diff --git a/arch/x86/kvm/vmx/tdx_arch.h b/arch/x86/kvm/vmx/tdx_arch.h
index b10dad8f46bb..711855be6c83 100644
--- a/arch/x86/kvm/vmx/tdx_arch.h
+++ b/arch/x86/kvm/vmx/tdx_arch.h
@@ -144,7 +144,7 @@ struct tdx_cpuid_value {
 /*
  * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is 1024B.
  */
-#define TDX_MAX_VCPUS	(~(u16)0)
+#define TDX_MAX_VCPUS	((int)U16_MAX)
 
 struct td_params {
 	u64 attributes;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d097024fd974..6822a50e1d5d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4733,8 +4733,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		break;
 	case KVM_CAP_MAX_VCPUS:
 		r = KVM_MAX_VCPUS;
-		if (kvm_x86_ops.max_vcpus)
-			r = static_call(kvm_x86_max_vcpus)(kvm);
+		if (kvm)
+			r = kvm->max_vcpus;
 		break;
 	case KVM_CAP_MAX_VCPU_ID:
 		r = KVM_MAX_VCPU_IDS;

-- 
Isaku Yamahata <isaku.yamahata@...ux.intel.com>