[Qemu-devel] [PATCH v2 0/2] kvm: use kvm_vm_check_extension() with VM capabilities

[Qemu-devel] [PATCH v2 0/2] kvm: use kvm_vm_check_extension() with VM capabilities

[Greg Kurz]

Some VM capabilities are currently checked with kvm_check_extension(). This
doesn't have any impact for most host architectures because they don't depend
on the KVM type. However, this is a problem for server-class ppc hosts that
can support the PR and HV KVM types. Both implementations can co-exist in the
kernel at the same time and we decide which one will be used with the "type"
argument of the KVM_CREATE_VM ioctl.

Each KVM type has a different set of capabilities, and checking them with
kvm_check_extension() will always cause KVM to assume we're in HV mode,
even if they are VM specific and we have explicitely requested to run in
PR mode. This may produce unexpected results.

Similar issues were recently fixed in the ppc code:

https://github.com/qemu/qemu/commit/70a0c19e83aa4c71c879d51e426e89e4b3d4e014
https://github.com/dgibson/qemu/commit/b152d74ebaed61f89fa1ab2c9b1526d9e9dabae5

This series focuses on capabilities that are checked by the common code.

--
Greg

---

Greg Kurz (2):
      kvm: check KVM_CAP_SYNC_MMU with kvm_vm_check_extension()
      kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()


 accel/kvm/kvm-all.c    |   51 +++++++++++++++++++++++++-----------------------
 accel/stubs/kvm-stub.c |    4 ++--
 include/sysemu/kvm.h   |    2 +-
 3 files changed, 30 insertions(+), 27 deletions(-)

[Qemu-devel] [PATCH v2 1/2] kvm: check KVM_CAP_SYNC_MMU with kvm_vm_check_extension()

[Greg Kurz]

On a server-class ppc host, this capability depends on the KVM type,
ie, HV or PR. If both KVM are present in the kernel, we will always
get the HV specific value, even if we explicitely requested PR on
the command line.

This can have an impact if we're using hugepages or a balloon device.

Since we've already created the VM at the time any user calls
kvm_has_sync_mmu(), switching to kvm_vm_check_extension() is
enough to fix any potential issue.

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

While here, let's cache the state of this extension in a bool variable,
since it has several users in the code, as suggested by Thomas Huth.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 accel/kvm/kvm-all.c    |    8 +++++---
 accel/stubs/kvm-stub.c |    4 ++--
 include/sysemu/kvm.h   |    2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b0181d722083..4d96c1fee9fe 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -87,6 +87,7 @@ struct KVMState
 #endif
     int many_ioeventfds;
     int intx_set_mask;
+    bool sync_mmu;
     /* The man page (and posix) say ioctl numbers are signed int, but
      * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
      * unsigned, and treating them as signed here can break things */
@@ -1665,6 +1666,8 @@ static int kvm_init(MachineState *ms)
 
     s->many_ioeventfds = kvm_check_many_ioeventfds();
 
+    s->sync_mmu = !!kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
+
     return 0;
 
 err:
@@ -2131,10 +2134,9 @@ int kvm_device_access(int fd, int group, uint64_t attr,
     return err;
 }
 
-/* Return 1 on success, 0 on failure */
-int kvm_has_sync_mmu(void)
+bool kvm_has_sync_mmu(void)
 {
-    return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
+    return kvm_state->sync_mmu;
 }
 
 int kvm_has_vcpu_events(void)
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 3965c528d348..c964af3e1c97 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -64,9 +64,9 @@ int kvm_cpu_exec(CPUState *cpu)
     abort();
 }
 
-int kvm_has_sync_mmu(void)
+bool kvm_has_sync_mmu(void)
 {
-    return 0;
+    return false;
 }
 
 int kvm_has_many_ioeventfds(void)
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 3a458f50e9f4..bbf12a172339 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -207,7 +207,7 @@ extern KVMState *kvm_state;
 /* external API */
 
 bool kvm_has_free_slot(MachineState *ms);
-int kvm_has_sync_mmu(void);
+bool kvm_has_sync_mmu(void);
 int kvm_has_vcpu_events(void);
 int kvm_has_robust_singlestep(void);
 int kvm_has_debugregs(void);

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

[Greg Kurz]

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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 accel/kvm/kvm-all.c |   43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 4d96c1fee9fe..b81d373bc5f9 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1441,7 +1441,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;
 }
 
@@ -1531,26 +1531,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) {
-            warn_report("Number of %s cpus requested (%d) exceeds "
-                        "the recommended cpus supported by KVM (%d)",
-                        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);
@@ -1585,6 +1565,27 @@ 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) {
+            warn_report("Number of %s cpus requested (%d) exceeds "
+                        "the recommended cpus supported by KVM (%d)",
+                        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 v2 1/2] kvm: check KVM_CAP_SYNC_MMU with kvm_vm_check_extension()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3351 bytes --]

On Thu, Sep 21, 2017 at 06:00:53PM +0200, Greg Kurz wrote:
> On a server-class ppc host, this capability depends on the KVM type,
> ie, HV or PR. If both KVM are present in the kernel, we will always
> get the HV specific value, even if we explicitely requested PR on
> the command line.
> 
> This can have an impact if we're using hugepages or a balloon device.
> 
> Since we've already created the VM at the time any user calls
> kvm_has_sync_mmu(), switching to kvm_vm_check_extension() is
> enough to fix any potential issue.
> 
> It is okay for the other archs that also implement KVM_CAP_SYNC_MMU,
> ie, mips, s390, x86 and arm, because they don't depend on the VM being
> created or not.
> 
> While here, let's cache the state of this extension in a bool variable,
> since it has several users in the code, as suggested by Thomas Huth.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

> ---
>  accel/kvm/kvm-all.c    |    8 +++++---
>  accel/stubs/kvm-stub.c |    4 ++--
>  include/sysemu/kvm.h   |    2 +-
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b0181d722083..4d96c1fee9fe 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -87,6 +87,7 @@ struct KVMState
>  #endif
>      int many_ioeventfds;
>      int intx_set_mask;
> +    bool sync_mmu;
>      /* The man page (and posix) say ioctl numbers are signed int, but
>       * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
>       * unsigned, and treating them as signed here can break things */
> @@ -1665,6 +1666,8 @@ static int kvm_init(MachineState *ms)
>  
>      s->many_ioeventfds = kvm_check_many_ioeventfds();
>  
> +    s->sync_mmu = !!kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> +
>      return 0;
>  
>  err:
> @@ -2131,10 +2134,9 @@ int kvm_device_access(int fd, int group, uint64_t attr,
>      return err;
>  }
>  
> -/* Return 1 on success, 0 on failure */
> -int kvm_has_sync_mmu(void)
> +bool kvm_has_sync_mmu(void)
>  {
> -    return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> +    return kvm_state->sync_mmu;
>  }
>  
>  int kvm_has_vcpu_events(void)
> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
> index 3965c528d348..c964af3e1c97 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -64,9 +64,9 @@ int kvm_cpu_exec(CPUState *cpu)
>      abort();
>  }
>  
> -int kvm_has_sync_mmu(void)
> +bool kvm_has_sync_mmu(void)
>  {
> -    return 0;
> +    return false;
>  }
>  
>  int kvm_has_many_ioeventfds(void)
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 3a458f50e9f4..bbf12a172339 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -207,7 +207,7 @@ extern KVMState *kvm_state;
>  /* external API */
>  
>  bool kvm_has_free_slot(MachineState *ms);
> -int kvm_has_sync_mmu(void);
> +bool kvm_has_sync_mmu(void);
>  int kvm_has_vcpu_events(void);
>  int kvm_has_robust_singlestep(void);
>  int kvm_has_debugregs(void);
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] kvm: use kvm_vm_check_extension() with VM capabilities

[Greg Kurz]

Ping ?

On Thu, 21 Sep 2017 18:00:41 +0200
Greg Kurz <groug@kaod.org> wrote:

> Some VM capabilities are currently checked with kvm_check_extension(). This
> doesn't have any impact for most host architectures because they don't depend
> on the KVM type. However, this is a problem for server-class ppc hosts that
> can support the PR and HV KVM types. Both implementations can co-exist in the
> kernel at the same time and we decide which one will be used with the "type"
> argument of the KVM_CREATE_VM ioctl.
> 
> Each KVM type has a different set of capabilities, and checking them with
> kvm_check_extension() will always cause KVM to assume we're in HV mode,
> even if they are VM specific and we have explicitely requested to run in
> PR mode. This may produce unexpected results.
> 
> Similar issues were recently fixed in the ppc code:
> 
> https://github.com/qemu/qemu/commit/70a0c19e83aa4c71c879d51e426e89e4b3d4e014
> https://github.com/dgibson/qemu/commit/b152d74ebaed61f89fa1ab2c9b1526d9e9dabae5
> 
> This series focuses on capabilities that are checked by the common code.
> 
> --
> Greg
> 
> ---
> 
> Greg Kurz (2):
>       kvm: check KVM_CAP_SYNC_MMU with kvm_vm_check_extension()
>       kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()
> 
> 
>  accel/kvm/kvm-all.c    |   51 +++++++++++++++++++++++++-----------------------
>  accel/stubs/kvm-stub.c |    4 ++--
>  include/sysemu/kvm.h   |    2 +-
>  3 files changed, 30 insertions(+), 27 deletions(-)
> 
> 

Re: [Qemu-devel] [PATCH v2 0/2] kvm: use kvm_vm_check_extension() with VM capabilities

[Paolo Bonzini]

On 21/09/2017 18:00, Greg Kurz wrote:
> Some VM capabilities are currently checked with kvm_check_extension(). This
> doesn't have any impact for most host architectures because they don't depend
> on the KVM type. However, this is a problem for server-class ppc hosts that
> can support the PR and HV KVM types. Both implementations can co-exist in the
> kernel at the same time and we decide which one will be used with the "type"
> argument of the KVM_CREATE_VM ioctl.
> 
> Each KVM type has a different set of capabilities, and checking them with
> kvm_check_extension() will always cause KVM to assume we're in HV mode,
> even if they are VM specific and we have explicitely requested to run in
> PR mode. This may produce unexpected results.
> 
> Similar issues were recently fixed in the ppc code:
> 
> https://github.com/qemu/qemu/commit/70a0c19e83aa4c71c879d51e426e89e4b3d4e014
> https://github.com/dgibson/qemu/commit/b152d74ebaed61f89fa1ab2c9b1526d9e9dabae5
> 
> This series focuses on capabilities that are checked by the common code.

Queued, thanks.

Paolo

> --
> Greg
> 
> ---
> 
> Greg Kurz (2):
>       kvm: check KVM_CAP_SYNC_MMU with kvm_vm_check_extension()
>       kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()
> 
> 
>  accel/kvm/kvm-all.c    |   51 +++++++++++++++++++++++++-----------------------
>  accel/stubs/kvm-stub.c |    4 ++--
>  include/sysemu/kvm.h   |    2 +-
>  3 files changed, 30 insertions(+), 27 deletions(-)
>