[PATCH RFC] KVM: x86: Drop arbitraty KVM_SOFT_MAX_VCPUS

[PATCH RFC] KVM: x86: Drop arbitraty KVM_SOFT_MAX_VCPUS

[Vitaly Kuznetsov]

KVM_CAP_NR_VCPUS is used to get the "recommended" maximum number of
VCPUs and arm64/mips/riscv report num_online_cpus(). Powerpc reports
either num_online_cpus() or num_present_cpus(), s390 has multiple
constants depending on hardware features. On x86, KVM reports an
arbitrary value of '710' which is supposed to be the maximum tested
value but it's possible to test all KVM_MAX_VCPUS even when there are
less physical CPUs available.

Drop the arbitrary '710' value and return num_online_cpus() on x86 as
well. The recommendation will match other architectures and will mean
'no CPU overcommit'.

For reference, QEMU only queries KVM_CAP_NR_VCPUS to print a warning
when the requested vCPU number exceeds it. The static limit of '710'
is quite weird as smaller systems with just a few physical CPUs should
certainly "recommend" less.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 1 -
 arch/x86/kvm/x86.c              | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 88fce6ab4bbd..0232a00598f2 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -38,7 +38,6 @@
 #define __KVM_HAVE_ARCH_VCPU_DEBUGFS
 
 #define KVM_MAX_VCPUS 1024
-#define KVM_SOFT_MAX_VCPUS 710
 
 /*
  * In x86, the VCPU ID corresponds to the APIC ID, and APIC IDs
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ac83d873d65b..91ef1b872b90 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4137,7 +4137,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		r = !static_call(kvm_x86_cpu_has_accelerated_tpr)();
 		break;
 	case KVM_CAP_NR_VCPUS:
-		r = KVM_SOFT_MAX_VCPUS;
+		r = num_online_cpus();
 		break;
 	case KVM_CAP_MAX_VCPUS:
 		r = KVM_MAX_VCPUS;
-- 
2.33.1

Re: [PATCH RFC] KVM: x86: Drop arbitraty KVM_SOFT_MAX_VCPUS

[Paolo Bonzini]

On 11/11/21 14:47, Vitaly Kuznetsov wrote:
> KVM_CAP_NR_VCPUS is used to get the "recommended" maximum number of
> VCPUs and arm64/mips/riscv report num_online_cpus(). Powerpc reports
> either num_online_cpus() or num_present_cpus(), s390 has multiple
> constants depending on hardware features. On x86, KVM reports an
> arbitrary value of '710' which is supposed to be the maximum tested
> value but it's possible to test all KVM_MAX_VCPUS even when there are
> less physical CPUs available.
> 
> Drop the arbitrary '710' value and return num_online_cpus() on x86 as
> well. The recommendation will match other architectures and will mean
> 'no CPU overcommit'.
> 
> For reference, QEMU only queries KVM_CAP_NR_VCPUS to print a warning
> when the requested vCPU number exceeds it. The static limit of '710'
> is quite weird as smaller systems with just a few physical CPUs should
> certainly "recommend" less.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Yes, this is a good idea.  We cannot move it entirely to common code due 
to POWER's handling of secondary threads in hypervisors; still, this is 
as close as we can get to a common idea of what KVM_CAP_NR_VCPUS means.

Queued, thanks.

Paolo

> ---
>   arch/x86/include/asm/kvm_host.h | 1 -
>   arch/x86/kvm/x86.c              | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 88fce6ab4bbd..0232a00598f2 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -38,7 +38,6 @@
>   #define __KVM_HAVE_ARCH_VCPU_DEBUGFS
>   
>   #define KVM_MAX_VCPUS 1024
> -#define KVM_SOFT_MAX_VCPUS 710
>   
>   /*
>    * In x86, the VCPU ID corresponds to the APIC ID, and APIC IDs
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ac83d873d65b..91ef1b872b90 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4137,7 +4137,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>   		r = !static_call(kvm_x86_cpu_has_accelerated_tpr)();
>   		break;
>   	case KVM_CAP_NR_VCPUS:
> -		r = KVM_SOFT_MAX_VCPUS;
> +		r = num_online_cpus();
>   		break;
>   	case KVM_CAP_MAX_VCPUS:
>   		r = KVM_MAX_VCPUS;
> 

Re: [PATCH RFC] KVM: x86: Drop arbitraty KVM_SOFT_MAX_VCPUS

[Sean Christopherson]

On Thu, Nov 11, 2021, Paolo Bonzini wrote:
> On 11/11/21 14:47, Vitaly Kuznetsov wrote:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index ac83d873d65b..91ef1b872b90 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4137,7 +4137,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >   		r = !static_call(kvm_x86_cpu_has_accelerated_tpr)();
> >   		break;
> >   	case KVM_CAP_NR_VCPUS:
> > -		r = KVM_SOFT_MAX_VCPUS;
> > +		r = num_online_cpus();

I doubt it matters much in practice, but this really should be

		r = min(num_online_cpus(), KVM_MAX_VCPUS);

> >   		break;
> >   	case KVM_CAP_MAX_VCPUS:
> >   		r = KVM_MAX_VCPUS;
> > 
> 

Re: [PATCH RFC] KVM: x86: Drop arbitraty KVM_SOFT_MAX_VCPUS

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Thu, Nov 11, 2021, Paolo Bonzini wrote:
>> On 11/11/21 14:47, Vitaly Kuznetsov wrote:
>> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> > index ac83d873d65b..91ef1b872b90 100644
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -4137,7 +4137,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>> >   		r = !static_call(kvm_x86_cpu_has_accelerated_tpr)();
>> >   		break;
>> >   	case KVM_CAP_NR_VCPUS:
>> > -		r = KVM_SOFT_MAX_VCPUS;
>> > +		r = num_online_cpus();
>
> I doubt it matters much in practice, but this really should be
>
> 		r = min(num_online_cpus(), KVM_MAX_VCPUS);
>

Nice catch, actually! It makes no sense to recommend > KVM_MAX_VCPUS. We
should fix this across all arches though, I'll take that as an action
item.

-- 
Vitaly

Re: [PATCH RFC] KVM: x86: Drop arbitraty KVM_SOFT_MAX_VCPUS

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/11/21 14:47, Vitaly Kuznetsov wrote:
>> KVM_CAP_NR_VCPUS is used to get the "recommended" maximum number of
>> VCPUs and arm64/mips/riscv report num_online_cpus(). Powerpc reports
>> either num_online_cpus() or num_present_cpus(), s390 has multiple
>> constants depending on hardware features. On x86, KVM reports an
>> arbitrary value of '710' which is supposed to be the maximum tested
>> value but it's possible to test all KVM_MAX_VCPUS even when there are
>> less physical CPUs available.
>> 
>> Drop the arbitrary '710' value and return num_online_cpus() on x86 as
>> well. The recommendation will match other architectures and will mean
>> 'no CPU overcommit'.
>> 
>> For reference, QEMU only queries KVM_CAP_NR_VCPUS to print a warning
>> when the requested vCPU number exceeds it. The static limit of '710'
>> is quite weird as smaller systems with just a few physical CPUs should
>> certainly "recommend" less.
>> 
>> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Yes, this is a good idea.  We cannot move it entirely to common code due 
> to POWER's handling of secondary threads in hypervisors; still, this is 
> as close as we can get to a common idea of what KVM_CAP_NR_VCPUS means.
>

S390's idea is also different and while I don't understand at all
all these hardware features, KVM_CAP_NR_VCPUS == KVM_CAP_MAX_VCPUS
(afaict). This was the first reason to keep KVM_CAP_NR_VCPUS handling in
arch specific code.

-- 
Vitaly