Skip to contentSkip to conversation

Guest stop notification

PATCH Superseded Raghavendra K T 2 participants 6 messages Apr 6, 2012 – Apr 7, 2012 KVM

v8 of 9 Superseded Latest is v9 →

lore
Share & tools
lore

Raghavendra K T

author
#0 lore raw

PATCH v8 1/1 Guest stop notification

From: Eric B Munson <[email protected]>

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 <[email protected]>
Signed-off-by Raghavendra K T <[email protected]>
Cc Eric B Munson <[email protected]>
Cc Avi Kivity <[email protected]>
Cc Marcelo Tosatti <[email protected]>
Cc Anthony Liguori <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc "Andreas Färber" <[email protected]>

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL KVM_GUEST_PAUSED is pervcpu again
CPUState renamed to CPUArchState KVMCLOCK_GUEST_PAUSED changed to KVM_KVMCLOCK_CTRL

Changes from V6:
Remove unnecessary include

Changes from V5:
KVM_GUEST_PAUSED is now a per vm ioctl instead of per vcpu

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/kvm/clock.c +18 -0
index 446bd62..c8a34a5 100644
@@ -64,10 +64,28 @@ static int kvmclock_post_load(void *opaque, int version_id)
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
+    int ret;
     KVMClockState *s = opaque;
+    CPUArchState *penv = first_cpu;
+    int cap_clock_ctrl = kvm_check_extension(kvm_state, KVM_CAP_KVMCLOCK_CTRL);
 
     if (running) {
         s->clock_valid = false;
+
+        if (!cap_clock_ctrl) {
+            return;
+        }
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            "kvmclock_vm_state_change: %s\n",
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
     }
 }
#1 lore raw

Am 06.04.2012 09:21, schrieb Raghavendra K T:

Quoted 17 lines

From: Eric B Munson <[email protected]>

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 <[email protected]>
Signed-off-by Raghavendra K T <[email protected]>
Cc Eric B Munson <[email protected]>
Cc Avi Kivity <[email protected]>
Cc Marcelo Tosatti <[email protected]>
Cc Anthony Liguori <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc "Andreas FÀrber" <[email protected]>
Quoted 5 lines

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL
KVM_GUEST_PAUSED is pervcpu again
CPUState renamed to CPUArchState

Thanks, change looks right to me.

Long-term I should probably consider supplying some cpu_foreach() macro to iterate over them, but that would still need manual declaration of a properly typed variable for the CPUArchState -> CPUState switch.

Quoted 30 lines

KVMCLOCK_GUEST_PAUSED changed to KVM_KVMCLOCK_CTRL

Changes from V6:
Remove unnecessary include

Changes from V5:
KVM_GUEST_PAUSED is now a per vm ioctl instead of per vcpu

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/kvm/clock.c +0 -0
index 446bd62..c8a34a5 100644

hw/kvm/clock.c static int kvmclock_post_load(void *opaque, int version_id) · line 67

Quoted 4 lines
hw/kvm/clock.c +1 -0
@@ -64,10 +64,28 @@ static int kvmclock_post_load(void *opaque, int version_id)
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
+    int ret;

Minor nitpick: We usually assign opaque values first thing in the function, so maybe order ret last if you resend?

Quoted 7 lines
hw/kvm/clock.c +3 -0
     KVMClockState *s = opaque;
+    CPUArchState *penv = first_cpu;
+    int cap_clock_ctrl = kvm_check_extension(kvm_state, KVM_CAP_KVMCLOCK_CTRL);
 
     if (running) {
         s->clock_valid = false;
+

hw/kvm/clock.c static int kvmclock_post_load(void *opaque, int version_id) · line 84

Quoted 10 lines
hw/kvm/clock.c +10 -0
+        if (!cap_clock_ctrl) {
+            return;
+        }
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            "kvmclock_vm_state_change: %s\n",
+                            strerror(-ret));

I always recommend to use __func__. Otherwise looks okay to me.

Andreas

Quoted 7 lines
hw/kvm/clock.c +4 -0
+                }
+                return;
+            }
+        }
     }
 }
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
author
#2 lore raw
Quoted 86 lines · Andreas Färber

Am 06.04.2012 09:21, schrieb Raghavendra K T:

Quoted 17 lines

From: Eric B Munson<[email protected]>

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<[email protected]>
Signed-off-by Raghavendra K T<[email protected]>
Cc Eric B Munson<[email protected]>
Cc Avi Kivity<[email protected]>
Cc Marcelo Tosatti<[email protected]>
Cc Anthony Liguori<[email protected]>
Cc Jan Kiszka<[email protected]>
Cc "Andreas FÀrber"<[email protected]>
Quoted 5 lines

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL
KVM_GUEST_PAUSED is pervcpu again
CPUState renamed to CPUArchState

Thanks, change looks right to me.

Long-term I should probably consider supplying some cpu_foreach() macro to iterate over them, but that would still need manual declaration of a properly typed variable for the CPUArchState -> CPUState switch.

Quoted 30 lines

KVMCLOCK_GUEST_PAUSED changed to KVM_KVMCLOCK_CTRL

Changes from V6:
Remove unnecessary include

Changes from V5:
KVM_GUEST_PAUSED is now a per vm ioctl instead of per vcpu

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/kvm/clock.c +0 -0
index 446bd62..c8a34a5 100644

hw/kvm/clock.c static int kvmclock_post_load(void *opaque, int version_id) · line 67

Quoted 4 lines
hw/kvm/clock.c +1 -0
@@ -64,10 +64,28 @@ static int kvmclock_post_load(void *opaque, int version_id)
  static void kvmclock_vm_state_change(void *opaque, int running,
                                       RunState state)
  {
+    int ret;

Minor nitpick: We usually assign opaque values first thing in the function, so maybe order ret last if you resend?

Quoted 7 lines
hw/kvm/clock.c +3 -0
      KVMClockState *s = opaque;
+    CPUArchState *penv = first_cpu;
+    int cap_clock_ctrl = kvm_check_extension(kvm_state, KVM_CAP_KVMCLOCK_CTRL);
 
      if (running) {
          s->clock_valid = false;
+

hw/kvm/clock.c static int kvmclock_post_load(void *opaque, int version_id) · line 84

Quoted 10 lines
hw/kvm/clock.c +10 -0
+        if (!cap_clock_ctrl) {
+            return;
+        }
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            "kvmclock_vm_state_change: %s\n",
+                            strerror(-ret));
Quoted 12 lines

I always recommend to use __func__. Otherwise looks okay to me.

Andreas

Quoted 7 lines
hw/kvm/clock.c +4 -0
+                }
+                return;
+            }
+        }
      }
  }

Thanks for Review. Sending with comments incorporated.
---

hw/kvm/clock.c +18 -0
index 446bd62..a6aa6e4 100644
@@ -65,9 +65,27 @@ static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
     KVMClockState *s = opaque;
+    CPUArchState *penv = first_cpu;
+    int cap_clock_ctrl = kvm_check_extension(kvm_state, KVM_CAP_KVMCLOCK_CTRL);
+    int ret;
 
     if (running) {
         s->clock_valid = false;
+
+        if (!cap_clock_ctrl) {
+            return;
+        }
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            " %s: %s\n", __FUNCTION__,
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
     }
 }
author Acked-by Andreas Färber
#3 lore raw

Re: [Qemu-devel] [PATCH V8 1/1] Guest stop notificationorry for rduplicate mail

Quoted 29 lines · Raghavendra K T
Quoted 28 lines · Andreas Färber

Am 06.04.2012 09:21, schrieb Raghavendra K T:

Quoted 26 lines

From: Eric B Munson<[email protected]>

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<[email protected]>
Signed-off-by Raghavendra K T<[email protected]>
Cc Eric B Munson<[email protected]>
Cc Avi Kivity<[email protected]>
Cc Marcelo Tosatti<[email protected]>
Cc Anthony Liguori<[email protected]>
Cc Jan Kiszka<[email protected]>
Cc "Andreas FÀrber"<[email protected]>

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL KVM_GUEST_PAUSED is pervcpu again CPUState renamed to CPUArchState

Quoted 1 line
Quoted 1 line

Thanks, change looks right to me.

I think I should have added Acked-by and resent full patch. So here is it. sorry for duplicate mail.
---
From: Eric B Munson <[email protected]>

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.

Acked-by "Andreas Färber" <[email protected]>
Signed-off-by Eric B Munson <[email protected]>
Signed-off-by Raghavendra K T <[email protected]>
Cc Eric B Munson <[email protected]>
Cc Avi Kivity <[email protected]>
Cc Marcelo Tosatti <[email protected]>
Cc Anthony Liguori <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc "Andreas Färber" <[email protected]>

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL KVM_GUEST_PAUSED is pervcpu again
CPUState renamed to CPUArchState KVMCLOCK_GUEST_PAUSED changed to KVM_KVMCLOCK_CTRL incorporated Andrea's comments (__FUNCTION__) etc

Changes from V6:
Remove unnecessary include

Changes from V5:
KVM_GUEST_PAUSED is now a per vm ioctl instead of per vcpu

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/kvm/clock.c +18 -0
index 446bd62..a6aa6e4 100644
@@ -65,9 +65,27 @@ static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
     KVMClockState *s = opaque;
+    CPUArchState *penv = first_cpu;
+    int cap_clock_ctrl = kvm_check_extension(kvm_state, KVM_CAP_KVMCLOCK_CTRL);
+    int ret;
 
     if (running) {
         s->clock_valid = false;
+
+        if (!cap_clock_ctrl) {
+            return;
+        }
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            " %s: %s\n", __FUNCTION__,
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
     }
 }
Reviewed-by
#4 lore raw

Re: [Qemu-devel] [PATCH V8 1/1] Guest stop notificationorry for rduplicate mail

Am 06.04.2012 15:01, schrieb Raghavendra K T:

Quoted 30 lines · Raghavendra K T
Quoted 29 lines · Andreas Färber
Quoted 28 lines

Am 06.04.2012 09:21, schrieb Raghavendra K T:

Quoted 26 lines

From: Eric B Munson<[email protected]>

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<[email protected]>
Signed-off-by Raghavendra K T<[email protected]>
Cc Eric B Munson<[email protected]>
Cc Avi Kivity<[email protected]>
Cc Marcelo Tosatti<[email protected]>
Cc Anthony Liguori<[email protected]>
Cc Jan Kiszka<[email protected]>
Cc "Andreas FÀrber"<[email protected]>

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL KVM_GUEST_PAUSED is pervcpu again CPUState renamed to CPUArchState

Quoted 4 lines
Quoted 1 line
Quoted 1 line

Thanks, change looks right to me.

I think I should have added Acked-by and resent full patch. So here is it. sorry for duplicate mail.

No, it was not intended as such since I can't ack the ioctl. Resends are best done with git-send-email, i.e. a v9 with change log (whether as reply or not, opinions are divided) to make sure the right version gets applied in the end.

Quoted 13 lines

---
From: Eric B Munson <[email protected]>

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.

Quoted 3 lines
Acked-by "Andreas Färber" <[email protected]>
Signed-off-by Eric B Munson <[email protected]>
Signed-off-by Raghavendra K T <[email protected]>

*-bys should be added in chronological order, i.e. at the bottom.

Quoted 54 lines
Cc Eric B Munson <[email protected]>
Cc Avi Kivity <[email protected]>
Cc Marcelo Tosatti <[email protected]>
Cc Anthony Liguori <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc "Andreas Färber" <[email protected]>

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL KVM_GUEST_PAUSED is pervcpu again
CPUState renamed to CPUArchState KVMCLOCK_GUEST_PAUSED changed to KVM_KVMCLOCK_CTRL incorporated Andrea's comments (__FUNCTION__) etc

Changes from V6:
Remove unnecessary include

Changes from V5:
KVM_GUEST_PAUSED is now a per vm ioctl instead of per vcpu

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/kvm/clock.c +3 -0
index 446bd62..a6aa6e4 100644
@@ -65,9 +65,27 @@ static void kvmclock_vm_state_change(void *opaque,
 int running,
                                      RunState state)
 {
     KVMClockState *s = opaque;
+    CPUArchState *penv = first_cpu;
+    int cap_clock_ctrl = kvm_check_extension(kvm_state,
 KVM_CAP_KVMCLOCK_CTRL);
+    int ret;
 
     if (running) {
         s->clock_valid = false;

hw/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, · line 85

Quoted 10 lines
hw/kvm/clock.c +10 -0
+
+        if (!cap_clock_ctrl) {
+            return;
+        }
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            " %s: %s\n", __FUNCTION__,

Is the whitespace before %s intentional? Wasn't there in v8.

The GCC manual recommends __func__, like I suggested, saying it's C99.
http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Function-Names.html#Function-Names
__FUNCTION__ usage is currently 432 vs. __func__ 579, so not wrong.

If you want to leave it that way you can add my

Reviewed-by Andreas Färber <[email protected]>

Andreas

Quoted 8 lines
hw/kvm/clock.c +5 -0
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
     }

}

Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
author
#5 lore raw

Re: [Qemu-devel] [PATCH V8 1/1] Guest stop notificationorry for rduplicate mail ndreas

Quoted 33 lines · Andreas Färber

Am 06.04.2012 15:01, schrieb Raghavendra K T:

Quoted 32 lines · Raghavendra K T
Quoted 30 lines · Andreas Färber
Quoted 29 lines

Am 06.04.2012 09:21, schrieb Raghavendra K T:

Quoted 21 lines

From: Eric B Munson<[email protected]>

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<[email protected]>
Signed-off-by Raghavendra K T<[email protected]>
Cc Eric B Munson<[email protected]>
Cc Avi Kivity<[email protected]>
Cc Marcelo Tosatti<[email protected]>
Cc Anthony Liguori<[email protected]>
Cc Jan Kiszka<[email protected]>
Cc "Andreas FÀrber"<[email protected]>
Quoted 5 lines

---
Changes from V7:
capabilty changed to KVM_CAP_KVMCLOCK_CTRL KVM_GUEST_PAUSED is pervcpu again CPUState renamed to CPUArchState

Thanks, change looks right to me.

Quoted 7 lines
Quoted 2 lines

I think I should have added Acked-by and resent full patch. So here is it. sorry for duplicate mail.

No, it was not intended as such since I can't ack the ioctl. Resends are best done with git-send-email, i.e. a v9 with change log (whether as reply or not, opinions are divided) to make sure the right version gets applied in the end.

Ok. Thanks Andreas. sending V9 shortly

Quoted 1 line

[...]

Quoted 11 lines

hw/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 83

Quoted 4 lines
hw/kvm/clock.c +4 -0
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            " %s: %s\n", __FUNCTION__,

Is the whitespace before %s intentional? Wasn't there in v8.

The GCC manual recommends __func__, like I suggested, saying it's C99.
http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Function-Names.html#Function-Names
__FUNCTION__ usage is currently 432 vs. __func__ 579, so not wrong.

will correct them.

Quoted 15 lines

If you want to leave it that way you can add my

Reviewed-by Andreas Färber<[email protected]>

Andreas

Quoted 8 lines
hw/kvm/clock.c +5 -0
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
      }
  }