[Qemu-devel] [PATCH 2/3] kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()

Greg Kurz posted 3 patches 8 years, 4 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 2/3] kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()
Posted by Greg Kurz 8 years, 4 months ago
On a modern server-class ppc host with the following CPU topology:

Architecture:          ppc64le
Byte Order:            Little Endian
CPU(s):                32
On-line CPU(s) list:   0,8,16,24
Off-line CPU(s) list:  1-7,9-15,17-23,25-31
Thread(s) per core:    1

If both KVM PR and KVM HV loaded and we pass:

        -machine pseries,accel=kvm,kvm-type=PR -smp 8

We expect QEMU to warn that this exceeds the number of online CPUs:

Warning: Number of SMP cpus requested (8) exceeds the recommended
 cpus supported by KVM (4)
Warning: Number of hotpluggable cpus requested (8) exceeds the
 recommended cpus supported by KVM (4)

but nothing is printed...

This happens because on ppc the KVM_CAP_NR_VCPUS capability is VM
specific  ndreally depends on the KVM type, but we currently use it
as a global capability. And KVM returns a fallback value based on
KVM HV being present. Maybe KVM on POWER shouldn't presume anything
as long as it doesn't have a VM, but in all cases, we should call
KVM_CREATE_VM first and use KVM_CAP_NR_VCPUS as a VM capability.

This patch hence changes kvm_recommended_vcpus() accordingly and
moves the sanity checking of smp_cpus after the VM creation.

It is okay for the other archs that also implement KVM_CAP_NR_VCPUS,
ie, mips, s390, x86 and arm, because they don't depend on the VM
being created or not.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 accel/kvm/kvm-all.c |   45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 323c567cfb68..d10534de2da1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1533,7 +1533,7 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
  */
 static int kvm_recommended_vcpus(KVMState *s)
 {
-    int ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
+    int ret = kvm_vm_check_extension(s, KVM_CAP_NR_VCPUS);
     return (ret) ? ret : 4;
 }
 
@@ -1623,27 +1623,6 @@ static int kvm_init(MachineState *ms)
         s->nr_slots = 32;
     }
 
-    /* check the vcpu limits */
-    soft_vcpus_limit = kvm_recommended_vcpus(s);
-    hard_vcpus_limit = kvm_max_vcpus(s);
-
-    while (nc->name) {
-        if (nc->num > soft_vcpus_limit) {
-            fprintf(stderr,
-                    "Warning: Number of %s cpus requested (%d) exceeds "
-                    "the recommended cpus supported by KVM (%d)\n",
-                    nc->name, nc->num, soft_vcpus_limit);
-
-            if (nc->num > hard_vcpus_limit) {
-                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
-                        "the maximum cpus supported by KVM (%d)\n",
-                        nc->name, nc->num, hard_vcpus_limit);
-                exit(1);
-            }
-        }
-        nc++;
-    }
-
     kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
     if (mc->kvm_type) {
         type = mc->kvm_type(kvm_type);
@@ -1678,6 +1657,28 @@ static int kvm_init(MachineState *ms)
     }
 
     s->vmfd = ret;
+
+    /* check the vcpu limits */
+    soft_vcpus_limit = kvm_recommended_vcpus(s);
+    hard_vcpus_limit = kvm_max_vcpus(s);
+
+    while (nc->name) {
+        if (nc->num > soft_vcpus_limit) {
+            fprintf(stderr,
+                    "Warning: Number of %s cpus requested (%d) exceeds "
+                    "the recommended cpus supported by KVM (%d)\n",
+                    nc->name, nc->num, soft_vcpus_limit);
+
+            if (nc->num > hard_vcpus_limit) {
+                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
+                        "the maximum cpus supported by KVM (%d)\n",
+                        nc->name, nc->num, hard_vcpus_limit);
+                exit(1);
+            }
+        }
+        nc++;
+    }
+
     missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
     if (!missing_cap) {
         missing_cap =


Re: [Qemu-devel] [PATCH 2/3] kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()
Posted by David Gibson 8 years, 4 months ago
On Thu, Sep 14, 2017 at 09:25:32PM +0200, Greg Kurz wrote:
1;4803;0c> On a modern server-class ppc host with the following CPU topology:
> 
> Architecture:          ppc64le
> Byte Order:            Little Endian
> CPU(s):                32
> On-line CPU(s) list:   0,8,16,24
> Off-line CPU(s) list:  1-7,9-15,17-23,25-31
> Thread(s) per core:    1
> 
> If both KVM PR and KVM HV loaded and we pass:
> 
>         -machine pseries,accel=kvm,kvm-type=PR -smp 8
> 
> We expect QEMU to warn that this exceeds the number of online CPUs:
> 
> Warning: Number of SMP cpus requested (8) exceeds the recommended
>  cpus supported by KVM (4)
> Warning: Number of hotpluggable cpus requested (8) exceeds the
>  recommended cpus supported by KVM (4)
> 
> but nothing is printed...
> 
> This happens because on ppc the KVM_CAP_NR_VCPUS capability is VM
> specific  ndreally depends on the KVM type, but we currently use it
> as a global capability. And KVM returns a fallback value based on
> KVM HV being present. Maybe KVM on POWER shouldn't presume anything
> as long as it doesn't have a VM, but in all cases, we should call
> KVM_CREATE_VM first and use KVM_CAP_NR_VCPUS as a VM capability.
> 
> This patch hence changes kvm_recommended_vcpus() accordingly and
> moves the sanity checking of smp_cpus after the VM creation.
> 
> It is okay for the other archs that also implement KVM_CAP_NR_VCPUS,
> ie, mips, s390, x86 and arm, because they don't depend on the VM
> being created or not.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  accel/kvm/kvm-all.c |   45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 323c567cfb68..d10534de2da1 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1533,7 +1533,7 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
>   */
>  static int kvm_recommended_vcpus(KVMState *s)
>  {
> -    int ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
> +    int ret = kvm_vm_check_extension(s, KVM_CAP_NR_VCPUS);
>      return (ret) ? ret : 4;
>  }
>  
> @@ -1623,27 +1623,6 @@ static int kvm_init(MachineState *ms)
>          s->nr_slots = 32;
>      }
>  
> -    /* check the vcpu limits */
> -    soft_vcpus_limit = kvm_recommended_vcpus(s);
> -    hard_vcpus_limit = kvm_max_vcpus(s);
> -
> -    while (nc->name) {
> -        if (nc->num > soft_vcpus_limit) {
> -            fprintf(stderr,
> -                    "Warning: Number of %s cpus requested (%d) exceeds "
> -                    "the recommended cpus supported by KVM (%d)\n",
> -                    nc->name, nc->num, soft_vcpus_limit);
> -
> -            if (nc->num > hard_vcpus_limit) {
> -                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
> -                        "the maximum cpus supported by KVM (%d)\n",
> -                        nc->name, nc->num, hard_vcpus_limit);
> -                exit(1);
> -            }
> -        }
> -        nc++;
> -    }
> -
>      kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
>      if (mc->kvm_type) {
>          type = mc->kvm_type(kvm_type);
> @@ -1678,6 +1657,28 @@ static int kvm_init(MachineState *ms)
>      }
>  
>      s->vmfd = ret;
> +
> +    /* check the vcpu limits */
> +    soft_vcpus_limit = kvm_recommended_vcpus(s);
> +    hard_vcpus_limit = kvm_max_vcpus(s);
> +
> +    while (nc->name) {
> +        if (nc->num > soft_vcpus_limit) {
> +            fprintf(stderr,
> +                    "Warning: Number of %s cpus requested (%d) exceeds "
> +                    "the recommended cpus supported by KVM (%d)\n",
> +                    nc->name, nc->num, soft_vcpus_limit);
> +
> +            if (nc->num > hard_vcpus_limit) {
> +                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
> +                        "the maximum cpus supported by KVM (%d)\n",
> +                        nc->name, nc->num, hard_vcpus_limit);
> +                exit(1);
> +            }
> +        }
> +        nc++;
> +    }
> +
>      missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
>      if (!missing_cap) {
>          missing_cap =
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH 2/3] kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()
Posted by Thomas Huth 8 years, 4 months ago
On 14.09.2017 21:25, Greg Kurz wrote:
> On a modern server-class ppc host with the following CPU topology:
> 
> Architecture:          ppc64le
> Byte Order:            Little Endian
> CPU(s):                32
> On-line CPU(s) list:   0,8,16,24
> Off-line CPU(s) list:  1-7,9-15,17-23,25-31
> Thread(s) per core:    1
> 
> If both KVM PR and KVM HV loaded and we pass:
> 
>         -machine pseries,accel=kvm,kvm-type=PR -smp 8
> 
> We expect QEMU to warn that this exceeds the number of online CPUs:
> 
> Warning: Number of SMP cpus requested (8) exceeds the recommended
>  cpus supported by KVM (4)
> Warning: Number of hotpluggable cpus requested (8) exceeds the
>  recommended cpus supported by KVM (4)
> 
> but nothing is printed...
> 
> This happens because on ppc the KVM_CAP_NR_VCPUS capability is VM
> specific  ndreally depends on the KVM type, but we currently use it
> as a global capability. And KVM returns a fallback value based on
> KVM HV being present. Maybe KVM on POWER shouldn't presume anything
> as long as it doesn't have a VM, but in all cases, we should call
> KVM_CREATE_VM first and use KVM_CAP_NR_VCPUS as a VM capability.
> 
> This patch hence changes kvm_recommended_vcpus() accordingly and
> moves the sanity checking of smp_cpus after the VM creation.
> 
> It is okay for the other archs that also implement KVM_CAP_NR_VCPUS,
> ie, mips, s390, x86 and arm, because they don't depend on the VM
> being created or not.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  accel/kvm/kvm-all.c |   45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 323c567cfb68..d10534de2da1 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1533,7 +1533,7 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
>   */
>  static int kvm_recommended_vcpus(KVMState *s)
>  {
> -    int ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
> +    int ret = kvm_vm_check_extension(s, KVM_CAP_NR_VCPUS);
>      return (ret) ? ret : 4;
>  }
>  
> @@ -1623,27 +1623,6 @@ static int kvm_init(MachineState *ms)
>          s->nr_slots = 32;
>      }
>  
> -    /* check the vcpu limits */
> -    soft_vcpus_limit = kvm_recommended_vcpus(s);
> -    hard_vcpus_limit = kvm_max_vcpus(s);
> -
> -    while (nc->name) {
> -        if (nc->num > soft_vcpus_limit) {
> -            fprintf(stderr,
> -                    "Warning: Number of %s cpus requested (%d) exceeds "
> -                    "the recommended cpus supported by KVM (%d)\n",
> -                    nc->name, nc->num, soft_vcpus_limit);
> -
> -            if (nc->num > hard_vcpus_limit) {
> -                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
> -                        "the maximum cpus supported by KVM (%d)\n",
> -                        nc->name, nc->num, hard_vcpus_limit);
> -                exit(1);
> -            }
> -        }
> -        nc++;
> -    }
> -
>      kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
>      if (mc->kvm_type) {
>          type = mc->kvm_type(kvm_type);
> @@ -1678,6 +1657,28 @@ static int kvm_init(MachineState *ms)
>      }
>  
>      s->vmfd = ret;
> +
> +    /* check the vcpu limits */
> +    soft_vcpus_limit = kvm_recommended_vcpus(s);
> +    hard_vcpus_limit = kvm_max_vcpus(s);
> +
> +    while (nc->name) {
> +        if (nc->num > soft_vcpus_limit) {
> +            fprintf(stderr,
> +                    "Warning: Number of %s cpus requested (%d) exceeds "
> +                    "the recommended cpus supported by KVM (%d)\n",
> +                    nc->name, nc->num, soft_vcpus_limit);
> +
> +            if (nc->num > hard_vcpus_limit) {
> +                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
> +                        "the maximum cpus supported by KVM (%d)\n",
> +                        nc->name, nc->num, hard_vcpus_limit);
> +                exit(1);
> +            }
> +        }
> +        nc++;
> +    }
> +
>      missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
>      if (!missing_cap) {
>          missing_cap =
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>