[Qemu-devel] [PATCH v2] vl.c: Fix max_cpus

[Qemu-devel] [PATCH v2] vl.c: Fix max_cpus

[riegamaths]

From: Dunrong Huang <riegamaths@gmail.com>

The VCPU count limit in kernel now is 254, defined by KVM_MAX_VCPUS
in kernel's header files. But the count limit in QEMU is 255,
so QEMU will failed to start if user passes "-enable-kvm" and "-smp 255"
to it.

Exit QEMU with an error if KVM is enabled and number of SMP cpus requested
exceeds KVM_MAX_VCPUS.

Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
---
v1 -> v2:
Checking if the number of smp cpus requested exceeds KVM limit count
if and only if kvm is enabled.

 vl.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 8904db1..cdd1c96 100644
--- a/vl.c
+++ b/vl.c
@@ -169,6 +169,11 @@ int main(int argc, char **argv)
 
 #define MAX_VIRTIO_CONSOLES 1
 
+/* KVM_MAX_VCPUS defined in kernel's header files */
+#ifndef KVM_MAX_VCPUS
+#define KVM_MAX_VCPUS 254
+#endif
+
 static const char *data_dir;
 const char *bios_name = NULL;
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
@@ -3348,6 +3353,12 @@ int main(int argc, char **argv, char **envp)
 
     configure_accelerator();
 
+    if (kvm_enabled() && smp_cpus > KVM_MAX_VCPUS) {
+        fprintf(stderr, "Number of SMP cpus requested (%d) exceeds max cpus "
+                "supported by KVM (%d)\n", smp_cpus, KVM_MAX_VCPUS);
+        exit(1);
+    }
+
     qemu_init_cpu_loop();
     if (qemu_init_main_loop()) {
         fprintf(stderr, "qemu_init_main_loop failed\n");
-- 
1.7.8.6

Re: [Qemu-devel] [PATCH v2] vl.c: Fix max_cpus

[Peter Maydell]

On 25 July 2012 12:11,  <riegamaths@gmail.com> wrote:
> From: Dunrong Huang <riegamaths@gmail.com>
>
> The VCPU count limit in kernel now is 254, defined by KVM_MAX_VCPUS
> in kernel's header files. But the count limit in QEMU is 255,
> so QEMU will failed to start if user passes "-enable-kvm" and "-smp 255"
> to it.
>
> Exit QEMU with an error if KVM is enabled and number of SMP cpus requested
> exceeds KVM_MAX_VCPUS.

I think this is the wrong approach:
 * KVM_MAX_VCPUS isn't a constant that the kernel exposes to userspace
 * it is different on different host architectures (254 happens to
   be the x86 value at the moment)
 * on PPC it is set to NR_CPUS so the maximum value will depend on
   the host system
 * it might change in the future, perhaps

If we want to identify the maximum possible number of CPUs under
KVM in advance of trying to create them all, the kernel documentation
specifies how to do this properly:

# The maximum possible value for max_vcpus can be retrieved using the
# KVM_CAP_MAX_VCPUS of the KVM_CHECK_EXTENSION ioctl() at run-time.
# If the KVM_CAP_NR_VCPUS does not exist, you should assume that
# max_vcpus is 4 cpus max.
# If the KVM_CAP_MAX_VCPUS does not exist, you should assume that
# max_vcpus is the same as the value returned from KVM_CAP_NR_VCPUS.

You could just implement this check in kvm-all.c:kvm_init(), in fact,
since smp_cpus has been set up by that point. Then you can print a
useful message and we'll fail nicely.

-- PMM

Re: [Qemu-devel] [PATCH v2] vl.c: Fix max_cpus

[Stefan Hajnoczi]

On Wed, Jul 25, 2012 at 12:11 PM,  <riegamaths@gmail.com> wrote:
> From: Dunrong Huang <riegamaths@gmail.com>
>
> The VCPU count limit in kernel now is 254, defined by KVM_MAX_VCPUS
> in kernel's header files. But the count limit in QEMU is 255,
> so QEMU will failed to start if user passes "-enable-kvm" and "-smp 255"
> to it.
>
> Exit QEMU with an error if KVM is enabled and number of SMP cpus requested
> exceeds KVM_MAX_VCPUS.
>
> Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
> ---
> v1 -> v2:
> Checking if the number of smp cpus requested exceeds KVM limit count
> if and only if kvm is enabled.
>
>  vl.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 8904db1..cdd1c96 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -169,6 +169,11 @@ int main(int argc, char **argv)
>
>  #define MAX_VIRTIO_CONSOLES 1
>
> +/* KVM_MAX_VCPUS defined in kernel's header files */
> +#ifndef KVM_MAX_VCPUS
> +#define KVM_MAX_VCPUS 254
> +#endif
> +
>  static const char *data_dir;
>  const char *bios_name = NULL;
>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> @@ -3348,6 +3353,12 @@ int main(int argc, char **argv, char **envp)
>
>      configure_accelerator();
>
> +    if (kvm_enabled() && smp_cpus > KVM_MAX_VCPUS) {
> +        fprintf(stderr, "Number of SMP cpus requested (%d) exceeds max cpus "
> +                "supported by KVM (%d)\n", smp_cpus, KVM_MAX_VCPUS);
> +        exit(1);
> +    }

After a little discussion on IRC, two points emerged:
1. Use KVM_CAP_MAX_VCPUS to query the max number of vcpus at runtime.
2. We should fail gracefully when ioctl() fails.

In other words, using the KVM_MAX_VCPUS value from userspace isn't a
good idea.  Imagine what happens when the user upgrades their kernel
without recompiling QEMU.  If the KVM_MAX_VCPUS value increased in the
kernel QEMU would not know.

Please either drop this patch completely or at least using
KVM_CAP_MAX_VCPUS so we fetch the maximum value at runtime.

Stefan

Re: [Qemu-devel] [PATCH v2] vl.c: Fix max_cpus

[Dunrong Huang]

2012/7/31 Stefan Hajnoczi <stefanha@gmail.com>:
> On Wed, Jul 25, 2012 at 12:11 PM,  <riegamaths@gmail.com> wrote:
>> From: Dunrong Huang <riegamaths@gmail.com>
>>
>> The VCPU count limit in kernel now is 254, defined by KVM_MAX_VCPUS
>> in kernel's header files. But the count limit in QEMU is 255,
>> so QEMU will failed to start if user passes "-enable-kvm" and "-smp 255"
>> to it.
>>
>> Exit QEMU with an error if KVM is enabled and number of SMP cpus requested
>> exceeds KVM_MAX_VCPUS.
>>
>> Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
>> ---
>> v1 -> v2:
>> Checking if the number of smp cpus requested exceeds KVM limit count
>> if and only if kvm is enabled.
>>
>>  vl.c |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 8904db1..cdd1c96 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -169,6 +169,11 @@ int main(int argc, char **argv)
>>
>>  #define MAX_VIRTIO_CONSOLES 1
>>
>> +/* KVM_MAX_VCPUS defined in kernel's header files */
>> +#ifndef KVM_MAX_VCPUS
>> +#define KVM_MAX_VCPUS 254
>> +#endif
>> +
>>  static const char *data_dir;
>>  const char *bios_name = NULL;
>>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>> @@ -3348,6 +3353,12 @@ int main(int argc, char **argv, char **envp)
>>
>>      configure_accelerator();
>>
>> +    if (kvm_enabled() && smp_cpus > KVM_MAX_VCPUS) {
>> +        fprintf(stderr, "Number of SMP cpus requested (%d) exceeds max cpus "
>> +                "supported by KVM (%d)\n", smp_cpus, KVM_MAX_VCPUS);
>> +        exit(1);
>> +    }
>
> After a little discussion on IRC, two points emerged:
> 1. Use KVM_CAP_MAX_VCPUS to query the max number of vcpus at runtime.
> 2. We should fail gracefully when ioctl() fails.
>
> In other words, using the KVM_MAX_VCPUS value from userspace isn't a
> good idea.  Imagine what happens when the user upgrades their kernel
> without recompiling QEMU.  If the KVM_MAX_VCPUS value increased in the
> kernel QEMU would not know.
>
> Please either drop this patch completely or at least using
> KVM_CAP_MAX_VCPUS so we fetch the maximum value at runtime.
>
> Stefan

I see. Thanks for your comments

I will send a patch based the disscussion we talked on IRC

-- 
Best Regards,

Dunrong Huang