linux-kernel - Re: [PATCH v18 023/121] KVM: TDX: Make KVM_CAP_MAX

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] [day] [month] [year] [list]

Message-ID: <ZfLT4AwkhGUQGPxD@chao-email>
Date: Thu, 14 Mar 2024 18:39:28 +0800
From: Chao Gao <chao.gao@...el.com>
To: <isaku.yamahata@...el.com>
CC: <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>
Subject: Re: [PATCH v18 023/121] KVM: TDX: Make KVM_CAP_MAX_VCPUS backend
 specific

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

I suppose kvm->max_vcpus should be always smaller than TDX_MAX_VCPUS, right?
if that's the case, the min() is pointless.

I don't get why kvm->max_vcpus is concerned. do you want to enable userspace to
read back the max_vcpus configured last time? this looks useless because userspace
can keep track of that value. how about:

	/*
	 * TDX module imposes additional restrictions on the maximum number of
	 * vCPUs of a TD guest.
	 */
	if (kvm && is_td(kvm))
		return min(TDX_MAX_VCPUS, KVM_MAX_VCPUS);
	else
		return KVM_MAX_VCPUS;

>+
>+	return kvm->max_vcpus;
>+}
>+
> static int vt_hardware_enable(void)
> {
> 	int ret;
>@@ -54,6 +66,14 @@ static void vt_hardware_unsetup(void)
> 	vmx_hardware_unsetup();
> }
> 
>+static int vt_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>+{
>+	if (is_td(kvm))
>+		return tdx_vm_enable_cap(kvm, cap);
>+
>+	return -EINVAL;
>+}
>+
> static int vt_vm_init(struct kvm *kvm)
> {
> 	if (is_td(kvm))
>@@ -91,7 +111,9 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
> 	.has_emulated_msr = vmx_has_emulated_msr,
> 
> 	.is_vm_type_supported = vt_is_vm_type_supported,
>+	.max_vcpus = vt_max_vcpus,
> 	.vm_size = sizeof(struct kvm_vmx),
>+	.vm_enable_cap = vt_vm_enable_cap,
> 	.vm_init = vt_vm_init,
> 	.vm_destroy = vmx_vm_destroy,
> 
>diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>index 8c463407f8a8..876ad7895b88 100644
>--- a/arch/x86/kvm/vmx/tdx.c
>+++ b/arch/x86/kvm/vmx/tdx.c
>@@ -100,6 +100,35 @@ struct tdx_info {
> /* Info about the TDX module. */
> static struct tdx_info *tdx_info;
> 
>+int tdx_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>+{
>+	int r;
>+
>+	switch (cap->cap) {
>+	case KVM_CAP_MAX_VCPUS: {
>+		if (cap->flags || cap->args[0] == 0)
>+			return -EINVAL;
>+		if (cap->args[0] > KVM_MAX_VCPUS ||
>+		    cap->args[0] > TDX_MAX_VCPUS)
>+			return -E2BIG;
>+
>+		mutex_lock(&kvm->lock);
>+		if (kvm->created_vcpus)
>+			r = -EBUSY;

Curly brackets are missing.

And -EBUSY looks improper because it isn't a temporary error.

>+		else {
>+			kvm->max_vcpus = cap->args[0];
>+			r = 0;
>+		}
>+		mutex_unlock(&kvm->lock);
>+		break;