From: Isaku Yamahata <isaku.yamahata@intel.com>

TDX attestation includes the maximum number of vcpu that the guest can
accommodate.  For that, the maximum number of vcpu needs to be specified
instead of constant, KVM_MAX_VCPUS.  Make KVM_ENABLE_CAP support
KVM_CAP_MAX_VCPUS.

Suggested-by: Sagi Shahar <sagis@google.com>
Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 virt/kvm/kvm_main.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a235b628b32f..1cfa7da92ad0 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4945,7 +4945,27 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
 		}
 
 		mutex_unlock(&kvm->slots_lock);
+		return r;
+	}
+	case KVM_CAP_MAX_VCPUS: {
+		int r;
 
+		if (cap->flags || cap->args[0] == 0)
+			return -EINVAL;
+		if (cap->args[0] >  kvm_vm_ioctl_check_extension(kvm, KVM_CAP_MAX_VCPUS))
+			return -E2BIG;
+
+		mutex_lock(&kvm->lock);
+		/* Only decreasing is allowed. */
+		if (cap->args[0] > kvm->max_vcpus)
+			r = -E2BIG;
+		else if (kvm->created_vcpus)
+			r = -EBUSY;
+		else {
+			kvm->max_vcpus = cap->args[0];
+			r = 0;
+		}
+		mutex_unlock(&kvm->lock);
 		return r;
 	}
 	default:
-- 
2.25.1

On Thu, 2023-01-12 at 08:31 -0800, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> TDX attestation includes the maximum number of vcpu that the guest can
> accommodate.  
> 

I don't understand why "attestation" is the reason here.  Let's say TDX is used
w/o attestation, I don't think this patch can be discarded?

IMHO the true reason is TDX has it's own control of maximum number of vcpus,
i.e. asking you to specify the value when creating the TD.  Therefore, the
constant KVM_MAX_VCPUS doesn't work for TDX guest anymore.

 
> For that, the maximum number of vcpu needs to be specified
> instead of constant, KVM_MAX_VCPUS.  Make KVM_ENABLE_CAP support
> KVM_CAP_MAX_VCPUS.
> 
> Suggested-by: Sagi Shahar <sagis@google.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  virt/kvm/kvm_main.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a235b628b32f..1cfa7da92ad0 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4945,7 +4945,27 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
>  		}
>  
>  		mutex_unlock(&kvm->slots_lock);
> +		return r;
> +	}
> +	case KVM_CAP_MAX_VCPUS: {
> +		int r;
>  
> +		if (cap->flags || cap->args[0] == 0)
> +			return -EINVAL;
> +		if (cap->args[0] >  kvm_vm_ioctl_check_extension(kvm, KVM_CAP_MAX_VCPUS))
> +			return -E2BIG;
> +
> +		mutex_lock(&kvm->lock);
> +		/* Only decreasing is allowed. */

Why?

> +		if (cap->args[0] > kvm->max_vcpus)
> +			r = -E2BIG;
> +		else if (kvm->created_vcpus)
> +			r = -EBUSY;
> +		else {
> +			kvm->max_vcpus = cap->args[0];
> +			r = 0;
> +		}
> +		mutex_unlock(&kvm->lock);
>  		return r;
>  	}
>  	default:

Also, IIUC this change is made to the generic kvm_main.c, which means other
archs are  affected too.  Is this OK to other archs?  Why such change cannot
TDX-specific (or, at least x86, or vmx specific)? 

On Mon, Jan 16, 2023 at 04:44:21AM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> On Thu, 2023-01-12 at 08:31 -0800, isaku.yamahata@intel.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > TDX attestation includes the maximum number of vcpu that the guest can
> > accommodate.  
> > 
> 
> I don't understand why "attestation" is the reason here.  Let's say TDX is used
> w/o attestation, I don't think this patch can be discarded?
>
> IMHO the true reason is TDX has it's own control of maximum number of vcpus,
> i.e. asking you to specify the value when creating the TD.  Therefore, the
> constant KVM_MAX_VCPUS doesn't work for TDX guest anymore.

Without TDX attestation, this can be discarded.  The TD is created with
max_vcpus=KVM_MAX_VCPUS by default.


> 
>  
> > For that, the maximum number of vcpu needs to be specified
> > instead of constant, KVM_MAX_VCPUS.  Make KVM_ENABLE_CAP support
> > KVM_CAP_MAX_VCPUS.
> > 
> > Suggested-by: Sagi Shahar <sagis@google.com>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  virt/kvm/kvm_main.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a235b628b32f..1cfa7da92ad0 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -4945,7 +4945,27 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
> >  		}
> >  
> >  		mutex_unlock(&kvm->slots_lock);
> > +		return r;
> > +	}
> > +	case KVM_CAP_MAX_VCPUS: {
> > +		int r;
> >  
> > +		if (cap->flags || cap->args[0] == 0)
> > +			return -EINVAL;
> > +		if (cap->args[0] >  kvm_vm_ioctl_check_extension(kvm, KVM_CAP_MAX_VCPUS))
> > +			return -E2BIG;
> > +
> > +		mutex_lock(&kvm->lock);
> > +		/* Only decreasing is allowed. */
> 
> Why?

I'll make it x86 specific and will drop this check.


> > +		if (cap->args[0] > kvm->max_vcpus)
> > +			r = -E2BIG;
> > +		else if (kvm->created_vcpus)
> > +			r = -EBUSY;
> > +		else {
> > +			kvm->max_vcpus = cap->args[0];
> > +			r = 0;
> > +		}
> > +		mutex_unlock(&kvm->lock);
> >  		return r;
> >  	}
> >  	default:
> 
> Also, IIUC this change is made to the generic kvm_main.c, which means other
> archs are  affected too.  Is this OK to other archs?  Why such change cannot
> TDX-specific (or, at least x86, or vmx specific)? 

Ok, I made it x86 specific. 
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

On Mon, 2023-02-27 at 13:26 -0800, Isaku Yamahata wrote:
> > > TDX attestation includes the maximum number of vcpu that the guest can
> > > accommodate.  
> > > 
> > 
> > I don't understand why "attestation" is the reason here.  Let's say TDX is
> > used
> > w/o attestation, I don't think this patch can be discarded?
> > 
> > IMHO the true reason is TDX has it's own control of maximum number of vcpus,
> > i.e. asking you to specify the value when creating the TD.  Therefore, the
> > constant KVM_MAX_VCPUS doesn't work for TDX guest anymore.
> 
> Without TDX attestation, this can be discarded.  The TD is created with
> max_vcpus=KVM_MAX_VCPUS by default.

This parses like: 

If we have attestation, the TD can be created with a user-specified non-default
value.  Otherwise, the TD is always created with default value.

It doesn't make sense, right?

Because architecturally whether TD can be created with a user specified value
doesn't depend on attestation at all.

On Tue, Feb 28, 2023 at 09:57:50PM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> On Mon, 2023-02-27 at 13:26 -0800, Isaku Yamahata wrote:
> > > > TDX attestation includes the maximum number of vcpu that the guest can
> > > > accommodate.  
> > > > 
> > > 
> > > I don't understand why "attestation" is the reason here.  Let's say TDX is
> > > used
> > > w/o attestation, I don't think this patch can be discarded?
> > > 
> > > IMHO the true reason is TDX has it's own control of maximum number of vcpus,
> > > i.e. asking you to specify the value when creating the TD.  Therefore, the
> > > constant KVM_MAX_VCPUS doesn't work for TDX guest anymore.
> > 
> > Without TDX attestation, this can be discarded.  The TD is created with
> > max_vcpus=KVM_MAX_VCPUS by default.
> 
> This parses like: 
> 
> If we have attestation, the TD can be created with a user-specified non-default
> value.  Otherwise, the TD is always created with default value.
> 
> It doesn't make sense, right?
> 
> Because architecturally whether TD can be created with a user specified value
> doesn't depend on attestation at all.

I'm not sure if I got your point.
Even without attestation, it's allowed to specify max vcpus.   Not "always".
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

On Tue, 2023-02-28 at 16:40 -0800, Isaku Yamahata wrote:
> On Tue, Feb 28, 2023 at 09:57:50PM +0000,
> "Huang, Kai" <kai.huang@intel.com> wrote:
> 
> > On Mon, 2023-02-27 at 13:26 -0800, Isaku Yamahata wrote:
> > > > > TDX attestation includes the maximum number of vcpu that the guest can
> > > > > accommodate.  
> > > > > 
> > > > 
> > > > I don't understand why "attestation" is the reason here.  Let's say TDX is
> > > > used
> > > > w/o attestation, I don't think this patch can be discarded?
> > > > 
> > > > IMHO the true reason is TDX has it's own control of maximum number of vcpus,
> > > > i.e. asking you to specify the value when creating the TD.  Therefore, the
> > > > constant KVM_MAX_VCPUS doesn't work for TDX guest anymore.
> > > 
> > > Without TDX attestation, this can be discarded.  The TD is created with
> > > max_vcpus=KVM_MAX_VCPUS by default.
> > 
> > This parses like: 
> > 
> > If we have attestation, the TD can be created with a user-specified non-default
> > value.  Otherwise, the TD is always created with default value.
> > 
> > It doesn't make sense, right?
> > 
> > Because architecturally whether TD can be created with a user specified value
> > doesn't depend on attestation at all.
> 
> I'm not sure if I got your point.
> Even without attestation, it's allowed to specify max vcpus.   Not "always".
> 

Exactly.

So "allow to specify max vcpus" isn't due to attestation, as you also said.  

Then why this patch is due to "TDX attestation includes the maximum number of
vcpu that the guest can accommodate"?  Shouldn't the reason be "TDX
architecturally has it's own control of maximum number of vcpus" as I mentioned
in my first reply?

On Thu, 12 Jan 2023 08:31:25 -0800
isaku.yamahata@intel.com wrote:

> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> TDX attestation includes the maximum number of vcpu that the guest can
> accommodate.  For that, the maximum number of vcpu needs to be specified
> instead of constant, KVM_MAX_VCPUS.  Make KVM_ENABLE_CAP support
> KVM_CAP_MAX_VCPUS.
> 
> Suggested-by: Sagi Shahar <sagis@google.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  virt/kvm/kvm_main.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a235b628b32f..1cfa7da92ad0 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4945,7 +4945,27 @@ static int kvm_vm_ioctl_enable_cap_generic(struct
> kvm *kvm, }
>  
>  		mutex_unlock(&kvm->slots_lock);
> +		return r;
> +	}
> +	case KVM_CAP_MAX_VCPUS: {

Better mention the KVM_CAP_MAX_VCPUS defined in XXX patch in the comments.

> +		int r;
>  
> +		if (cap->flags || cap->args[0] == 0)
> +			return -EINVAL;
> +		if (cap->args[0] >  kvm_vm_ioctl_check_extension(kvm,
> KVM_CAP_MAX_VCPUS))
> +			return -E2BIG;
> +
> +		mutex_lock(&kvm->lock);
> +		/* Only decreasing is allowed. */
> +		if (cap->args[0] > kvm->max_vcpus)
> +			r = -E2BIG;
> +		else if (kvm->created_vcpus)
> +			r = -EBUSY;
> +		else {
> +			kvm->max_vcpus = cap->args[0];
> +			r = 0;
> +		}
> +		mutex_unlock(&kvm->lock);
>  		return r;
>  	}
>  	default:

On Fri, Jan 13, 2023 at 02:55:07PM +0200,
Zhi Wang <zhi.wang.linux@gmail.com> wrote:

> On Thu, 12 Jan 2023 08:31:25 -0800
> isaku.yamahata@intel.com wrote:
> 
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > TDX attestation includes the maximum number of vcpu that the guest can
> > accommodate.  For that, the maximum number of vcpu needs to be specified
> > instead of constant, KVM_MAX_VCPUS.  Make KVM_ENABLE_CAP support
> > KVM_CAP_MAX_VCPUS.
> > 
> > Suggested-by: Sagi Shahar <sagis@google.com>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  virt/kvm/kvm_main.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a235b628b32f..1cfa7da92ad0 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -4945,7 +4945,27 @@ static int kvm_vm_ioctl_enable_cap_generic(struct
> > kvm *kvm, }
> >  
> >  		mutex_unlock(&kvm->slots_lock);
> > +		return r;
> > +	}
> > +	case KVM_CAP_MAX_VCPUS: {
> 
> Better mention the KVM_CAP_MAX_VCPUS defined in XXX patch in the comments.

This already exists as KVM api and documented in Documentation/virt/kvm/api.rst
to get the possible maximum number of vcpus.
This patch make it settable.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>