[PATCH V5] Guest stop notification

[PATCH V5] Guest stop notification

[Eric B Munson]

Often when a guest is stopped from the qemu console, it will report spurious
soft lockup warnings on resume.  There are kernel patches being discussed that
will give the host the ability to tell the guest that it is being stopped and
should ignore the soft lockup warning that generates.  This patch uses the qemu
Notifier system to tell the guest it is about to be stopped.

Signed-off-by: Eric B Munson <emunson@mgebm.net>

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: kvm@vger.kernel.org
---
Changes from V4:
 Test if the guest paused capability is available before use

Changes from V3:
 Collapse new state change notification function into existsing function.
 Correct whitespace issues
 Change ioctl name to KVMCLOCK_GUEST_PAUSED
 Use for loop to iterate vpcu's

Changes from V2:
 Move ioctl into hw/kvmclock.c so as other arches can use it as it is
implemented

Changes from V1:
 Remove unnecessary encapsulating function

 hw/kvmclock.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/hw/kvmclock.c b/hw/kvmclock.c
index 5388bc4..d071d61 100644
--- a/hw/kvmclock.c
+++ b/hw/kvmclock.c
@@ -16,6 +16,7 @@
 #include "sysbus.h"
 #include "kvm.h"
 #include "kvmclock.h"
+#include "cpu-all.h"
 
 #include <linux/kvm.h>
 #include <linux/kvm_para.h>
@@ -62,10 +63,29 @@ static int kvmclock_post_load(void *opaque, int version_id)
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
+    int ret;
+    CPUState *penv = first_cpu;
     KVMClockState *s = opaque;
+    int cap_guest_paused = kvm_check_extension(kvm_state, KVM_CAP_GUEST_PAUSED);
 
     if (running) {
         s->clock_valid = false;
+
+        if (!cap_guest_paused) {
+            return;
+        }
+
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVMCLOCK_GUEST_PAUSED, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            "kvmclock_vm_state_change: %s\n",
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
     }
 }
 
-- 
1.7.5.4

[Qemu-devel] [PATCH V5] Guest stop notification

[Eric B Munson]

Often when a guest is stopped from the qemu console, it will report spurious
soft lockup warnings on resume.  There are kernel patches being discussed that
will give the host the ability to tell the guest that it is being stopped and
should ignore the soft lockup warning that generates.  This patch uses the qemu
Notifier system to tell the guest it is about to be stopped.

Signed-off-by: Eric B Munson <emunson@mgebm.net>

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: kvm@vger.kernel.org
---
Changes from V4:
 Test if the guest paused capability is available before use

Changes from V3:
 Collapse new state change notification function into existsing function.
 Correct whitespace issues
 Change ioctl name to KVMCLOCK_GUEST_PAUSED
 Use for loop to iterate vpcu's

Changes from V2:
 Move ioctl into hw/kvmclock.c so as other arches can use it as it is
implemented

Changes from V1:
 Remove unnecessary encapsulating function

 hw/kvmclock.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/hw/kvmclock.c b/hw/kvmclock.c
index 5388bc4..d071d61 100644
--- a/hw/kvmclock.c
+++ b/hw/kvmclock.c
@@ -16,6 +16,7 @@
 #include "sysbus.h"
 #include "kvm.h"
 #include "kvmclock.h"
+#include "cpu-all.h"
 
 #include <linux/kvm.h>
 #include <linux/kvm_para.h>
@@ -62,10 +63,29 @@ static int kvmclock_post_load(void *opaque, int version_id)
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
+    int ret;
+    CPUState *penv = first_cpu;
     KVMClockState *s = opaque;
+    int cap_guest_paused = kvm_check_extension(kvm_state, KVM_CAP_GUEST_PAUSED);
 
     if (running) {
         s->clock_valid = false;
+
+        if (!cap_guest_paused) {
+            return;
+        }
+
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVMCLOCK_GUEST_PAUSED, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            "kvmclock_vm_state_change: %s\n",
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
     }
 }
 
-- 
1.7.5.4

Re: [PATCH V5] Guest stop notification

[Jan Kiszka]

On 2012-01-11 19:17, Eric B Munson wrote:
> Often when a guest is stopped from the qemu console, it will report spurious
> soft lockup warnings on resume.  There are kernel patches being discussed that
> will give the host the ability to tell the guest that it is being stopped and
> should ignore the soft lockup warning that generates.  This patch uses the qemu
> Notifier system to tell the guest it is about to be stopped.
> 
> Signed-off-by: Eric B Munson <emunson@mgebm.net>
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: ryanh@linux.vnet.ibm.com
> Cc: aliguori@us.ibm.com
> Cc: kvm@vger.kernel.org
> ---
> Changes from V4:
>  Test if the guest paused capability is available before use
> 
> Changes from V3:
>  Collapse new state change notification function into existsing function.
>  Correct whitespace issues
>  Change ioctl name to KVMCLOCK_GUEST_PAUSED
>  Use for loop to iterate vpcu's
> 
> Changes from V2:
>  Move ioctl into hw/kvmclock.c so as other arches can use it as it is
> implemented
> 
> Changes from V1:
>  Remove unnecessary encapsulating function
> 
>  hw/kvmclock.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
> index 5388bc4..d071d61 100644
> --- a/hw/kvmclock.c
> +++ b/hw/kvmclock.c
> @@ -16,6 +16,7 @@
>  #include "sysbus.h"
>  #include "kvm.h"
>  #include "kvmclock.h"
> +#include "cpu-all.h"
>  
>  #include <linux/kvm.h>
>  #include <linux/kvm_para.h>
> @@ -62,10 +63,29 @@ static int kvmclock_post_load(void *opaque, int version_id)
>  static void kvmclock_vm_state_change(void *opaque, int running,
>                                       RunState state)
>  {
> +    int ret;
> +    CPUState *penv = first_cpu;
>      KVMClockState *s = opaque;
> +    int cap_guest_paused = kvm_check_extension(kvm_state, KVM_CAP_GUEST_PAUSED);
>  
>      if (running) {
>          s->clock_valid = false;
> +
> +        if (!cap_guest_paused) {
> +            return;
> +        }

Why? You already ignore -EINVAL.

> +
> +        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> +            ret = kvm_vcpu_ioctl(penv, KVMCLOCK_GUEST_PAUSED, 0);

This indicates that the interface could still be improved:
"GUEST_PAUSED" implies to me a VM state, but the IOCTL has to be applied
per VCPU. This is inconsistent.

Why not define a per-VM IOCTL? Would make user space's life a little bit
easier as well.

Or is there a valid use case of selectively paused VCPUs? Then call it
KVMCLOCK_VCPU_PAUSED.

> +            if (ret) {
> +                if (ret != -EINVAL) {

What is special about -EINVAL (as long as the cap is checked)?

> +                    fprintf(stderr,
> +                            "kvmclock_vm_state_change: %s\n",
> +                            strerror(-ret));
> +                }
> +                return;
> +            }
> +        }
>      }
>  }
>  

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH V5] Guest stop notification

[Jan Kiszka]

On 2012-01-11 19:17, Eric B Munson wrote:
> Often when a guest is stopped from the qemu console, it will report spurious
> soft lockup warnings on resume.  There are kernel patches being discussed that
> will give the host the ability to tell the guest that it is being stopped and
> should ignore the soft lockup warning that generates.  This patch uses the qemu
> Notifier system to tell the guest it is about to be stopped.
> 
> Signed-off-by: Eric B Munson <emunson@mgebm.net>
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: ryanh@linux.vnet.ibm.com
> Cc: aliguori@us.ibm.com
> Cc: kvm@vger.kernel.org
> ---
> Changes from V4:
>  Test if the guest paused capability is available before use
> 
> Changes from V3:
>  Collapse new state change notification function into existsing function.
>  Correct whitespace issues
>  Change ioctl name to KVMCLOCK_GUEST_PAUSED
>  Use for loop to iterate vpcu's
> 
> Changes from V2:
>  Move ioctl into hw/kvmclock.c so as other arches can use it as it is
> implemented
> 
> Changes from V1:
>  Remove unnecessary encapsulating function
> 
>  hw/kvmclock.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
> index 5388bc4..d071d61 100644
> --- a/hw/kvmclock.c
> +++ b/hw/kvmclock.c
> @@ -16,6 +16,7 @@
>  #include "sysbus.h"
>  #include "kvm.h"
>  #include "kvmclock.h"
> +#include "cpu-all.h"
>  
>  #include <linux/kvm.h>
>  #include <linux/kvm_para.h>
> @@ -62,10 +63,29 @@ static int kvmclock_post_load(void *opaque, int version_id)
>  static void kvmclock_vm_state_change(void *opaque, int running,
>                                       RunState state)
>  {
> +    int ret;
> +    CPUState *penv = first_cpu;
>      KVMClockState *s = opaque;
> +    int cap_guest_paused = kvm_check_extension(kvm_state, KVM_CAP_GUEST_PAUSED);
>  
>      if (running) {
>          s->clock_valid = false;
> +
> +        if (!cap_guest_paused) {
> +            return;
> +        }

Why? You already ignore -EINVAL.

> +
> +        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> +            ret = kvm_vcpu_ioctl(penv, KVMCLOCK_GUEST_PAUSED, 0);

This indicates that the interface could still be improved:
"GUEST_PAUSED" implies to me a VM state, but the IOCTL has to be applied
per VCPU. This is inconsistent.

Why not define a per-VM IOCTL? Would make user space's life a little bit
easier as well.

Or is there a valid use case of selectively paused VCPUs? Then call it
KVMCLOCK_VCPU_PAUSED.

> +            if (ret) {
> +                if (ret != -EINVAL) {

What is special about -EINVAL (as long as the cap is checked)?

> +                    fprintf(stderr,
> +                            "kvmclock_vm_state_change: %s\n",
> +                            strerror(-ret));
> +                }
> +                return;
> +            }
> +        }
>      }
>  }
>  

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux