kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host
@ 2011-10-31 20:07 Eric B Munson
  2011-10-31 20:07 ` [PATCH 1/6 V2] Add flag to indicate that a vm was stopped by the host Eric B Munson
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Eric B Munson @ 2011-10-31 20:07 UTC (permalink / raw)
  To: avi
  Cc: mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	aliguori, Eric B Munson

Changes from V1:
(Thanks Marcelo)
Host code has all been moved to arch/x86/kvm/x86.c
KVM_PAUSE_GUEST was renamed to KVM_GUEST_PAUSED

When a guest kernel is stopped by the host hypervisor it can look like a soft
lockup to the guest kernel.  This false warning can mask later soft lockup
warnings which may be real.  This patch series adds a method for a host
hypervisor to communicate to a guest kernel that it is being stopped.  The
final patch in the series has the watchdog check this flag when it goes to
issue a soft lockup warning and skip the warning if the guest knows it was
stopped.

It was attempted to solve this in Qemu, but the side effects of saving and
restoring the clock and tsc for each vcpu put the wall clock of the guest behind
by the amount of time of the pause.  This forces a guest to have ntp running
in order to keep the wall clock accurate.



Eric B Munson (6):
  Add flag to indicate that a vm was stopped by the host
  Add functions to check if the host has stopped the vm
  Add ioctl for KVM_GUEST_STOPPED
  Add generic stubs for kvm stop check functions
  Add check for suspended vm in softlockup detector
  Add age out of guest paused flag

 arch/x86/include/asm/pvclock-abi.h |    1 +
 arch/x86/include/asm/pvclock.h     |    7 +++++
 arch/x86/kernel/kvmclock.c         |   19 ++++++++++++++
 arch/x86/kvm/x86.c                 |   48 ++++++++++++++++++++++++++++++++++++
 include/asm-generic/pvclock.h      |   19 ++++++++++++++
 include/linux/kvm.h                |    4 +++
 include/linux/kvm_host.h           |    2 +
 kernel/watchdog.c                  |    9 ++++++
 8 files changed, 109 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-generic/pvclock.h

-- 
1.7.5.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/6 V2] Add flag to indicate that a vm was stopped by the host
  2011-10-31 20:07 [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host Eric B Munson
@ 2011-10-31 20:07 ` Eric B Munson
  2011-11-01 19:22   ` Anthony Liguori
  2011-10-31 20:07 ` [PATCH 2/6 V2] Add functions to check if the host has stopped the vm Eric B Munson
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Eric B Munson @ 2011-10-31 20:07 UTC (permalink / raw)
  To: avi
  Cc: mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	aliguori, Eric B Munson

This flag will be used to check if the vm was stopped by the host when a soft
lockup was detected.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
 arch/x86/include/asm/pvclock-abi.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/pvclock-abi.h b/arch/x86/include/asm/pvclock-abi.h
index 35f2d19..6167fd7 100644
--- a/arch/x86/include/asm/pvclock-abi.h
+++ b/arch/x86/include/asm/pvclock-abi.h
@@ -40,5 +40,6 @@ struct pvclock_wall_clock {
 } __attribute__((__packed__));
 
 #define PVCLOCK_TSC_STABLE_BIT	(1 << 0)
+#define PVCLOCK_GUEST_STOPPED	(1 << 1)
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_PVCLOCK_ABI_H */
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/6 V2] Add functions to check if the host has stopped the vm
  2011-10-31 20:07 [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host Eric B Munson
  2011-10-31 20:07 ` [PATCH 1/6 V2] Add flag to indicate that a vm was stopped by the host Eric B Munson
@ 2011-10-31 20:07 ` Eric B Munson
  2011-11-01 19:28   ` Anthony Liguori
  2011-10-31 20:07 ` [PATCH 3/6 V2] Add ioctl for KVM_GUEST_STOPPED Eric B Munson
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Eric B Munson @ 2011-10-31 20:07 UTC (permalink / raw)
  To: avi
  Cc: mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	aliguori, Eric B Munson

When a host stops or suspends a VM it will set a flag to show this.  The
watchdog will use these functions to determine if a softlockup is real, or the
result of a suspended VM.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
 arch/x86/include/asm/pvclock.h |    2 ++
 arch/x86/kernel/kvmclock.c     |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index c59cc97..7d3ba41 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -13,6 +13,8 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct timespec *ts);
 void pvclock_resume(void);
 
+bool kvm_check_and_clear_host_stopped(int cpu);
+
 /*
  * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
  * yielding a 64-bit result.
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index c1a0188..8ddcfaf 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -113,6 +113,25 @@ static void kvm_get_preset_lpj(void)
 	preset_lpj = lpj;
 }
 
+bool kvm_check_and_clear_host_stopped(int cpu)
+{
+	bool ret = false;
+	struct pvclock_vcpu_time_info *src;
+
+	/*
+	 * per_cpu() is safe here because this function is only called from
+	 * timer functions where preemption is already disabled.
+	 */
+	WARN_ON(!in_atomic());
+	src = &per_cpu(hv_clock, cpu);
+	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
+		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
+		ret = true;
+	}
+
+	return ret;
+}
+
 static struct clocksource kvm_clock = {
 	.name = "kvm-clock",
 	.read = kvm_clock_get_cycles,
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/6 V2] Add ioctl for KVM_GUEST_STOPPED
  2011-10-31 20:07 [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host Eric B Munson
  2011-10-31 20:07 ` [PATCH 1/6 V2] Add flag to indicate that a vm was stopped by the host Eric B Munson
  2011-10-31 20:07 ` [PATCH 2/6 V2] Add functions to check if the host has stopped the vm Eric B Munson
@ 2011-10-31 20:07 ` Eric B Munson
  2011-11-01 19:32   ` Anthony Liguori
  2011-10-31 20:07 ` [PATCH 4/6 V2] Add generic stubs for kvm stop check functions Eric B Munson
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Eric B Munson @ 2011-10-31 20:07 UTC (permalink / raw)
  To: avi
  Cc: mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	aliguori, Eric B Munson

Now that we have a flag that will tell the guest it was suspended, create an
interface for that communication using a KVM ioctl.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
Changes from V1:
 Add kvm_set_host_stopped to arch/x86/jvm/x86.c instead of kvmclock.c
 Rename KVM_PAUSE_GUEST to KVM_GUEST_PAUSED

 arch/x86/include/asm/pvclock.h |    3 +++
 arch/x86/kvm/x86.c             |   16 ++++++++++++++++
 include/linux/kvm.h            |    2 ++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 7d3ba41..9312814 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -3,6 +3,7 @@
 
 #include <linux/clocksource.h>
 #include <asm/pvclock-abi.h>
+#include <linux/kvm_host.h>
 
 /* some helper functions for xen and kvm pv clock sources */
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src);
@@ -13,6 +14,8 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct timespec *ts);
 void pvclock_resume(void);
 
+void kvm_set_host_stopped(struct kvm_vcpu *vcpu);
+
 bool kvm_check_and_clear_host_stopped(int cpu);
 
 /*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c38efd7..592ac3b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3295,6 +3295,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 		goto out;
 	}
+	case KVM_GUEST_PAUSED: {
+		r = 0;
+		kvm_set_host_stopped(vcpu);
+		break;
+	}
 	default:
 		r = -EINVAL;
 	}
@@ -6117,6 +6122,17 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
 }
 EXPORT_SYMBOL_GPL(kvm_task_switch);
 
+/*
+ * kvm_set_host_stopped() indicates to the guest kernel that it has been
+ * stopped by the hypervisor.  This function will be called from the host only.
+ */
+void kvm_set_host_stopped(struct kvm_vcpu *vcpu)
+{
+	struct pvclock_vcpu_time_info *src = &vcpu->arch.hv_clock;
+	src->flags |= PVCLOCK_GUEST_STOPPED;
+}
+EXPORT_SYMBOL_GPL(kvm_set_host_stopped);
+
 int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index f47fcd3..87cab0d 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -763,6 +763,8 @@ struct kvm_clock_data {
 #define KVM_CREATE_SPAPR_TCE	  _IOW(KVMIO,  0xa8, struct kvm_create_spapr_tce)
 /* Available with KVM_CAP_RMA */
 #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
+/* VM is being stopped by host */
+#define KVM_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/6 V2] Add generic stubs for kvm stop check functions
  2011-10-31 20:07 [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host Eric B Munson
                   ` (2 preceding siblings ...)
  2011-10-31 20:07 ` [PATCH 3/6 V2] Add ioctl for KVM_GUEST_STOPPED Eric B Munson
@ 2011-10-31 20:07 ` Eric B Munson
  2011-10-31 20:07 ` [PATCH 5/6 V2] Add check for suspended vm in softlockup detector Eric B Munson
  2011-10-31 20:07 ` [PATCH 6/6 V2] Add age out of guest paused flag Eric B Munson
  5 siblings, 0 replies; 16+ messages in thread
From: Eric B Munson @ 2011-10-31 20:07 UTC (permalink / raw)
  To: avi
  Cc: mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	aliguori, Eric B Munson

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
 include/asm-generic/pvclock.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-generic/pvclock.h

diff --git a/include/asm-generic/pvclock.h b/include/asm-generic/pvclock.h
new file mode 100644
index 0000000..b42a8eb
--- /dev/null
+++ b/include/asm-generic/pvclock.h
@@ -0,0 +1,19 @@
+#ifndef _ASM_GENERIC_PVCLOCK_H
+#define _ASM_GENERIC_PVCLOCK_H
+
+
+/*
+ * These functions are used by architectures that support kvm to avoid issuing
+ * false soft lockup messages.
+ */
+static inline bool kvm_check_and_clear_host_stopped(int cpu)
+{
+	return false;
+}
+
+static inline void kvm_clear_guest_paused(struct kvm_vcpu *vcpu)
+{
+	return;
+}
+
+#endif
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/6 V2] Add check for suspended vm in softlockup detector
  2011-10-31 20:07 [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host Eric B Munson
                   ` (3 preceding siblings ...)
  2011-10-31 20:07 ` [PATCH 4/6 V2] Add generic stubs for kvm stop check functions Eric B Munson
@ 2011-10-31 20:07 ` Eric B Munson
  2011-10-31 20:07 ` [PATCH 6/6 V2] Add age out of guest paused flag Eric B Munson
  5 siblings, 0 replies; 16+ messages in thread
From: Eric B Munson @ 2011-10-31 20:07 UTC (permalink / raw)
  To: avi
  Cc: mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	aliguori, Eric B Munson

A suspended VM can cause spurious soft lockup warnings.  To avoid these, the
watchdog now checks if the kernel knows it was stopped by the host and skips
the warning if so.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
 kernel/watchdog.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index d680381..ff86cae 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -25,6 +25,7 @@
 #include <linux/sysctl.h>
 
 #include <asm/irq_regs.h>
+#include <asm/pvclock.h>
 #include <linux/perf_event.h>
 
 int watchdog_enabled = 1;
@@ -292,6 +293,14 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	 */
 	duration = is_softlockup(touch_ts);
 	if (unlikely(duration)) {
+		/*
+		 * If a virtual machine is stopped by the host it can look to
+		 * the watchdog like a soft lockup, check to see if the host
+		 * stopped the vm before we issue the warning
+		 */
+		if (kvm_check_and_clear_host_stopped(smp_processor_id()))
+			return HRTIMER_RESTART;
+
 		/* only warn once */
 		if (__this_cpu_read(soft_watchdog_warn) == true)
 			return HRTIMER_RESTART;
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 6/6 V2] Add age out of guest paused flag
  2011-10-31 20:07 [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host Eric B Munson
                   ` (4 preceding siblings ...)
  2011-10-31 20:07 ` [PATCH 5/6 V2] Add check for suspended vm in softlockup detector Eric B Munson
@ 2011-10-31 20:07 ` Eric B Munson
  2011-11-01 19:34   ` Anthony Liguori
  5 siblings, 1 reply; 16+ messages in thread
From: Eric B Munson @ 2011-10-31 20:07 UTC (permalink / raw)
  To: avi
  Cc: mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	aliguori, Eric B Munson

The KVM_GUEST_PAUSED flag will prevent a guest from compaining about a soft
lockup but it can mask real soft lockups if the flag isn't cleared when it is
no longer relevant.  This patch adds a kvm ioctl that the hypervisor will use
when it resumes a guest to start a timer for aging out the flag.  The time out
will be specified by the hypervisor in the ioctl call.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
Cahnges from V1:
 Add host functions for flag management to arch/x86/kvm/x86.c instead of
kvmclock.c

 arch/x86/include/asm/pvclock.h |    2 ++
 arch/x86/kvm/x86.c             |   32 ++++++++++++++++++++++++++++++++
 include/linux/kvm.h            |    2 ++
 include/linux/kvm_host.h       |    2 ++
 4 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 9312814..e8460b9 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -18,6 +18,8 @@ void kvm_set_host_stopped(struct kvm_vcpu *vcpu);
 
 bool kvm_check_and_clear_host_stopped(int cpu);
 
+void kvm_clear_guest_paused(struct kvm_vcpu *vcpu, unsigned int length);
+
 /*
  * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
  * yielding a 64-bit result.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 592ac3b..fb0132a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -46,6 +46,7 @@
 #include <linux/hash.h>
 #include <linux/pci.h>
 #include <trace/events/kvm.h>
+#include <linux/timer.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
@@ -3300,6 +3301,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		kvm_set_host_stopped(vcpu);
 		break;
 	}
+	case KVM_CLEAR_GUEST_PAUSED: {
+		unsigned int length;
+		r = -EFAULT;
+		if (copy_from_user(&length, argp, sizeof length))
+			goto out;
+		r = 0;
+		kvm_clear_guest_paused(vcpu, length);
+		break;
+	}
 	default:
 		r = -EINVAL;
 	}
@@ -6133,6 +6143,28 @@ void kvm_set_host_stopped(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_set_host_stopped);
 
+static void kvm_timer_clear_guest_paused(unsigned long vcpu_addr)
+{
+	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)vcpu_addr;
+	struct pvclock_vcpu_time_info *src = &vcpu->arch.hv_clock;
+	src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
+}
+
+/*
+ * Host has resumed the guest, we need to clear the guest paused flag so we
+ * don't mask any real soft lockups.
+ */
+void kvm_clear_guest_paused(struct kvm_vcpu *vcpu, unsigned int length)
+{
+	if (!timer_pending(&vcpu->flag_timer))
+		setup_timer(&vcpu->flag_timer,
+			    kvm_timer_clear_guest_paused,
+			    (unsigned long)vcpu);
+	mod_timer(&vcpu->flag_timer,
+		  jiffies + (length * HZ));
+}
+EXPORT_SYMBOL_GPL(kvm_clear_guest_paused);
+
 int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 87cab0d..bd9724c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -765,6 +765,8 @@ struct kvm_clock_data {
 #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
 /* VM is being stopped by host */
 #define KVM_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
+/* Start the timer to clear the paused flag */
+#define KVM_CLEAR_GUEST_PAUSED	  _IO(KVMIO,   0xab)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index d526231..043af4d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/rcupdate.h>
 #include <linux/ratelimit.h>
+#include <linux/timer.h>
 #include <asm/signal.h>
 
 #include <linux/kvm.h>
@@ -154,6 +155,7 @@ struct kvm_vcpu {
 #endif
 
 	struct kvm_vcpu_arch arch;
+	struct timer_list flag_timer;
 };
 
 static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/6 V2] Add flag to indicate that a vm was stopped by the host
  2011-10-31 20:07 ` [PATCH 1/6 V2] Add flag to indicate that a vm was stopped by the host Eric B Munson
@ 2011-11-01 19:22   ` Anthony Liguori
  0 siblings, 0 replies; 16+ messages in thread
From: Anthony Liguori @ 2011-11-01 19:22 UTC (permalink / raw)
  To: Eric B Munson
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh,
	Jeremy Fitzhardinge

On 10/31/2011 03:07 PM, Eric B Munson wrote:
> This flag will be used to check if the vm was stopped by the host when a soft
> lockup was detected.
>
> Signed-off-by: Eric B Munson<emunson@mgebm.net>

Adding Jeremy since the pvclock ABI is shared across Xen and KVM.

Regards,

Anthony Liguori

> ---
>   arch/x86/include/asm/pvclock-abi.h |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/pvclock-abi.h b/arch/x86/include/asm/pvclock-abi.h
> index 35f2d19..6167fd7 100644
> --- a/arch/x86/include/asm/pvclock-abi.h
> +++ b/arch/x86/include/asm/pvclock-abi.h
> @@ -40,5 +40,6 @@ struct pvclock_wall_clock {
>   } __attribute__((__packed__));
>
>   #define PVCLOCK_TSC_STABLE_BIT	(1<<  0)
> +#define PVCLOCK_GUEST_STOPPED	(1<<  1)
>   #endif /* __ASSEMBLY__ */
>   #endif /* _ASM_X86_PVCLOCK_ABI_H */

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6 V2] Add functions to check if the host has stopped the vm
  2011-10-31 20:07 ` [PATCH 2/6 V2] Add functions to check if the host has stopped the vm Eric B Munson
@ 2011-11-01 19:28   ` Anthony Liguori
  2011-11-01 20:07     ` Eric B Munson
  0 siblings, 1 reply; 16+ messages in thread
From: Anthony Liguori @ 2011-11-01 19:28 UTC (permalink / raw)
  To: Eric B Munson
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

On 10/31/2011 03:07 PM, Eric B Munson wrote:
> When a host stops or suspends a VM it will set a flag to show this.  The
> watchdog will use these functions to determine if a softlockup is real, or the
> result of a suspended VM.
>
> Signed-off-by: Eric B Munson<emunson@mgebm.net>
> ---
>   arch/x86/include/asm/pvclock.h |    2 ++
>   arch/x86/kernel/kvmclock.c     |   19 +++++++++++++++++++
>   2 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index c59cc97..7d3ba41 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -13,6 +13,8 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
>   			    struct timespec *ts);
>   void pvclock_resume(void);
>
> +bool kvm_check_and_clear_host_stopped(int cpu);

A kvm-specific function shouldn't be declared in pvclock.h as this is shared 
code between KVM and Xen.

> +
>   /*
>    * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
>    * yielding a 64-bit result.
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index c1a0188..8ddcfaf 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -113,6 +113,25 @@ static void kvm_get_preset_lpj(void)
>   	preset_lpj = lpj;
>   }
>
> +bool kvm_check_and_clear_host_stopped(int cpu)
> +{
> +	bool ret = false;
> +	struct pvclock_vcpu_time_info *src;
> +
> +	/*
> +	 * per_cpu() is safe here because this function is only called from
> +	 * timer functions where preemption is already disabled.
> +	 */
> +	WARN_ON(!in_atomic());
> +	src =&per_cpu(hv_clock, cpu);
> +	if ((src->flags&  PVCLOCK_GUEST_STOPPED) != 0) {
> +		src->flags = src->flags&  (~PVCLOCK_GUEST_STOPPED);
> +		ret = true;
> +	}

It would be helpful to separate out the patches for guest enablement from host 
enablement.  This is a guest function, correct?

Don't you need an atomic getandset here?  Can the host clear this flag or only 
set it?

It's ashame we don't have a proper pvclock spec in Documentation/.  I would 
suggest at least adding a description of how this bit works in the first patch.

Regards,

Anthony Liguori

> +
> +	return ret;
> +}
> +
>   static struct clocksource kvm_clock = {
>   	.name = "kvm-clock",
>   	.read = kvm_clock_get_cycles,

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/6 V2] Add ioctl for KVM_GUEST_STOPPED
  2011-10-31 20:07 ` [PATCH 3/6 V2] Add ioctl for KVM_GUEST_STOPPED Eric B Munson
@ 2011-11-01 19:32   ` Anthony Liguori
  2011-11-01 20:11     ` Eric B Munson
  0 siblings, 1 reply; 16+ messages in thread
From: Anthony Liguori @ 2011-11-01 19:32 UTC (permalink / raw)
  To: Eric B Munson
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

On 10/31/2011 03:07 PM, Eric B Munson wrote:
> Now that we have a flag that will tell the guest it was suspended, create an
> interface for that communication using a KVM ioctl.
>
> Signed-off-by: Eric B Munson<emunson@mgebm.net>
> ---
> Changes from V1:
>   Add kvm_set_host_stopped to arch/x86/jvm/x86.c instead of kvmclock.c
>   Rename KVM_PAUSE_GUEST to KVM_GUEST_PAUSED
>
>   arch/x86/include/asm/pvclock.h |    3 +++
>   arch/x86/kvm/x86.c             |   16 ++++++++++++++++
>   include/linux/kvm.h            |    2 ++
>   3 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index 7d3ba41..9312814 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -3,6 +3,7 @@
>
>   #include<linux/clocksource.h>
>   #include<asm/pvclock-abi.h>
> +#include<linux/kvm_host.h>
>
>   /* some helper functions for xen and kvm pv clock sources */
>   cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src);
> @@ -13,6 +14,8 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
>   			    struct timespec *ts);
>   void pvclock_resume(void);
>
> +void kvm_set_host_stopped(struct kvm_vcpu *vcpu);

This can't go in pvclock.h.

> +
>   bool kvm_check_and_clear_host_stopped(int cpu);
>
>   /*
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c38efd7..592ac3b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3295,6 +3295,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>
>   		goto out;
>   	}
> +	case KVM_GUEST_PAUSED: {
> +		r = 0;
> +		kvm_set_host_stopped(vcpu);
> +		break;
> +	}
>   	default:
>   		r = -EINVAL;
>   	}
> @@ -6117,6 +6122,17 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
>   }
>   EXPORT_SYMBOL_GPL(kvm_task_switch);
>
> +/*
> + * kvm_set_host_stopped() indicates to the guest kernel that it has been
> + * stopped by the hypervisor.  This function will be called from the host only.
> + */
> +void kvm_set_host_stopped(struct kvm_vcpu *vcpu)
> +{
> +	struct pvclock_vcpu_time_info *src =&vcpu->arch.hv_clock;
> +	src->flags |= PVCLOCK_GUEST_STOPPED;

Shouldn't we throw some sort of error if you're trying to set a flag and 
kvmclock isn't enabled?

Regards,

Anthony Liguori

> +}
> +EXPORT_SYMBOL_GPL(kvm_set_host_stopped);
> +
>   int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>   				  struct kvm_sregs *sregs)
>   {
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index f47fcd3..87cab0d 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -763,6 +763,8 @@ struct kvm_clock_data {
>   #define KVM_CREATE_SPAPR_TCE	  _IOW(KVMIO,  0xa8, struct kvm_create_spapr_tce)
>   /* Available with KVM_CAP_RMA */
>   #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
> +/* VM is being stopped by host */
> +#define KVM_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
>
>   #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1<<  0)
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6 V2] Add age out of guest paused flag
  2011-10-31 20:07 ` [PATCH 6/6 V2] Add age out of guest paused flag Eric B Munson
@ 2011-11-01 19:34   ` Anthony Liguori
  2011-11-01 19:51     ` Eric B Munson
  0 siblings, 1 reply; 16+ messages in thread
From: Anthony Liguori @ 2011-11-01 19:34 UTC (permalink / raw)
  To: Eric B Munson
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

On 10/31/2011 03:07 PM, Eric B Munson wrote:
> The KVM_GUEST_PAUSED flag will prevent a guest from compaining about a soft
> lockup but it can mask real soft lockups if the flag isn't cleared when it is
> no longer relevant.  This patch adds a kvm ioctl that the hypervisor will use
> when it resumes a guest to start a timer for aging out the flag.  The time out
> will be specified by the hypervisor in the ioctl call.
>
> Signed-off-by: Eric B Munson<emunson@mgebm.net>

Why not have the guest clear the flag when it acknowledges it?

The hypervisor would unconditionally set the bit, and the guest would do a 
testandclear to check if the bit is set.  I think that avoids the whole aging 
business.

Regards,

Anthony Liguori

> ---
> Cahnges from V1:
>   Add host functions for flag management to arch/x86/kvm/x86.c instead of
> kvmclock.c
>
>   arch/x86/include/asm/pvclock.h |    2 ++
>   arch/x86/kvm/x86.c             |   32 ++++++++++++++++++++++++++++++++
>   include/linux/kvm.h            |    2 ++
>   include/linux/kvm_host.h       |    2 ++
>   4 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index 9312814..e8460b9 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -18,6 +18,8 @@ void kvm_set_host_stopped(struct kvm_vcpu *vcpu);
>
>   bool kvm_check_and_clear_host_stopped(int cpu);
>
> +void kvm_clear_guest_paused(struct kvm_vcpu *vcpu, unsigned int length);
> +
>   /*
>    * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
>    * yielding a 64-bit result.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 592ac3b..fb0132a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -46,6 +46,7 @@
>   #include<linux/hash.h>
>   #include<linux/pci.h>
>   #include<trace/events/kvm.h>
> +#include<linux/timer.h>
>
>   #define CREATE_TRACE_POINTS
>   #include "trace.h"
> @@ -3300,6 +3301,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>   		kvm_set_host_stopped(vcpu);
>   		break;
>   	}
> +	case KVM_CLEAR_GUEST_PAUSED: {
> +		unsigned int length;
> +		r = -EFAULT;
> +		if (copy_from_user(&length, argp, sizeof length))
> +			goto out;
> +		r = 0;
> +		kvm_clear_guest_paused(vcpu, length);
> +		break;
> +	}
>   	default:
>   		r = -EINVAL;
>   	}
> @@ -6133,6 +6143,28 @@ void kvm_set_host_stopped(struct kvm_vcpu *vcpu)
>   }
>   EXPORT_SYMBOL_GPL(kvm_set_host_stopped);
>
> +static void kvm_timer_clear_guest_paused(unsigned long vcpu_addr)
> +{
> +	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)vcpu_addr;
> +	struct pvclock_vcpu_time_info *src =&vcpu->arch.hv_clock;
> +	src->flags = src->flags&  (~PVCLOCK_GUEST_STOPPED);
> +}
> +
> +/*
> + * Host has resumed the guest, we need to clear the guest paused flag so we
> + * don't mask any real soft lockups.
> + */
> +void kvm_clear_guest_paused(struct kvm_vcpu *vcpu, unsigned int length)
> +{
> +	if (!timer_pending(&vcpu->flag_timer))
> +		setup_timer(&vcpu->flag_timer,
> +			    kvm_timer_clear_guest_paused,
> +			    (unsigned long)vcpu);
> +	mod_timer(&vcpu->flag_timer,
> +		  jiffies + (length * HZ));
> +}
> +EXPORT_SYMBOL_GPL(kvm_clear_guest_paused);
> +
>   int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>   				  struct kvm_sregs *sregs)
>   {
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 87cab0d..bd9724c 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -765,6 +765,8 @@ struct kvm_clock_data {
>   #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
>   /* VM is being stopped by host */
>   #define KVM_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
> +/* Start the timer to clear the paused flag */
> +#define KVM_CLEAR_GUEST_PAUSED	  _IO(KVMIO,   0xab)
>
>   #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1<<  0)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index d526231..043af4d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -19,6 +19,7 @@
>   #include<linux/slab.h>
>   #include<linux/rcupdate.h>
>   #include<linux/ratelimit.h>
> +#include<linux/timer.h>
>   #include<asm/signal.h>
>
>   #include<linux/kvm.h>
> @@ -154,6 +155,7 @@ struct kvm_vcpu {
>   #endif
>
>   	struct kvm_vcpu_arch arch;
> +	struct timer_list flag_timer;
>   };
>
>   static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6 V2] Add age out of guest paused flag
  2011-11-01 19:34   ` Anthony Liguori
@ 2011-11-01 19:51     ` Eric B Munson
  2011-11-01 19:58       ` Anthony Liguori
  0 siblings, 1 reply; 16+ messages in thread
From: Eric B Munson @ 2011-11-01 19:51 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

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

On Tue, 01 Nov 2011, Anthony Liguori wrote:

> On 10/31/2011 03:07 PM, Eric B Munson wrote:
> >The KVM_GUEST_PAUSED flag will prevent a guest from compaining about a soft
> >lockup but it can mask real soft lockups if the flag isn't cleared when it is
> >no longer relevant.  This patch adds a kvm ioctl that the hypervisor will use
> >when it resumes a guest to start a timer for aging out the flag.  The time out
> >will be specified by the hypervisor in the ioctl call.
> >
> >Signed-off-by: Eric B Munson<emunson@mgebm.net>
> 
> Why not have the guest clear the flag when it acknowledges it?
> 
> The hypervisor would unconditionally set the bit, and the guest
> would do a testandclear to check if the bit is set.  I think that
> avoids the whole aging business.
> 
> Regards,
> 
> Anthony Liguori

If you have a look at patch 5 of this series, the flag is cleared when the
guest checks the validity of a soft lockup.  However, the aging is meant to
cover the case where the guest never sees a soft lockup.  We don't want this
flag to be stored for ever and end up delaying real soft lockup messages.  With
that case in mind, I thought this was a good/simple compramise.

Eric

> 
> >---
> >Cahnges from V1:
> >  Add host functions for flag management to arch/x86/kvm/x86.c instead of
> >kvmclock.c
> >
> >  arch/x86/include/asm/pvclock.h |    2 ++
> >  arch/x86/kvm/x86.c             |   32 ++++++++++++++++++++++++++++++++
> >  include/linux/kvm.h            |    2 ++
> >  include/linux/kvm_host.h       |    2 ++
> >  4 files changed, 38 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> >index 9312814..e8460b9 100644
> >--- a/arch/x86/include/asm/pvclock.h
> >+++ b/arch/x86/include/asm/pvclock.h
> >@@ -18,6 +18,8 @@ void kvm_set_host_stopped(struct kvm_vcpu *vcpu);
> >
> >  bool kvm_check_and_clear_host_stopped(int cpu);
> >
> >+void kvm_clear_guest_paused(struct kvm_vcpu *vcpu, unsigned int length);
> >+
> >  /*
> >   * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
> >   * yielding a 64-bit result.
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 592ac3b..fb0132a 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -46,6 +46,7 @@
> >  #include<linux/hash.h>
> >  #include<linux/pci.h>
> >  #include<trace/events/kvm.h>
> >+#include<linux/timer.h>
> >
> >  #define CREATE_TRACE_POINTS
> >  #include "trace.h"
> >@@ -3300,6 +3301,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >  		kvm_set_host_stopped(vcpu);
> >  		break;
> >  	}
> >+	case KVM_CLEAR_GUEST_PAUSED: {
> >+		unsigned int length;
> >+		r = -EFAULT;
> >+		if (copy_from_user(&length, argp, sizeof length))
> >+			goto out;
> >+		r = 0;
> >+		kvm_clear_guest_paused(vcpu, length);
> >+		break;
> >+	}
> >  	default:
> >  		r = -EINVAL;
> >  	}
> >@@ -6133,6 +6143,28 @@ void kvm_set_host_stopped(struct kvm_vcpu *vcpu)
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_set_host_stopped);
> >
> >+static void kvm_timer_clear_guest_paused(unsigned long vcpu_addr)
> >+{
> >+	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)vcpu_addr;
> >+	struct pvclock_vcpu_time_info *src =&vcpu->arch.hv_clock;
> >+	src->flags = src->flags&  (~PVCLOCK_GUEST_STOPPED);
> >+}
> >+
> >+/*
> >+ * Host has resumed the guest, we need to clear the guest paused flag so we
> >+ * don't mask any real soft lockups.
> >+ */
> >+void kvm_clear_guest_paused(struct kvm_vcpu *vcpu, unsigned int length)
> >+{
> >+	if (!timer_pending(&vcpu->flag_timer))
> >+		setup_timer(&vcpu->flag_timer,
> >+			    kvm_timer_clear_guest_paused,
> >+			    (unsigned long)vcpu);
> >+	mod_timer(&vcpu->flag_timer,
> >+		  jiffies + (length * HZ));
> >+}
> >+EXPORT_SYMBOL_GPL(kvm_clear_guest_paused);
> >+
> >  int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> >  				  struct kvm_sregs *sregs)
> >  {
> >diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> >index 87cab0d..bd9724c 100644
> >--- a/include/linux/kvm.h
> >+++ b/include/linux/kvm.h
> >@@ -765,6 +765,8 @@ struct kvm_clock_data {
> >  #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
> >  /* VM is being stopped by host */
> >  #define KVM_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
> >+/* Start the timer to clear the paused flag */
> >+#define KVM_CLEAR_GUEST_PAUSED	  _IO(KVMIO,   0xab)
> >
> >  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1<<  0)
> >
> >diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >index d526231..043af4d 100644
> >--- a/include/linux/kvm_host.h
> >+++ b/include/linux/kvm_host.h
> >@@ -19,6 +19,7 @@
> >  #include<linux/slab.h>
> >  #include<linux/rcupdate.h>
> >  #include<linux/ratelimit.h>
> >+#include<linux/timer.h>
> >  #include<asm/signal.h>
> >
> >  #include<linux/kvm.h>
> >@@ -154,6 +155,7 @@ struct kvm_vcpu {
> >  #endif
> >
> >  	struct kvm_vcpu_arch arch;
> >+	struct timer_list flag_timer;
> >  };
> >
> >  static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6 V2] Add age out of guest paused flag
  2011-11-01 19:51     ` Eric B Munson
@ 2011-11-01 19:58       ` Anthony Liguori
  2011-11-01 20:10         ` Eric B Munson
  0 siblings, 1 reply; 16+ messages in thread
From: Anthony Liguori @ 2011-11-01 19:58 UTC (permalink / raw)
  To: Eric B Munson
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

On 11/01/2011 02:51 PM, Eric B Munson wrote:
> On Tue, 01 Nov 2011, Anthony Liguori wrote:
>
>> On 10/31/2011 03:07 PM, Eric B Munson wrote:
>>> The KVM_GUEST_PAUSED flag will prevent a guest from compaining about a soft
>>> lockup but it can mask real soft lockups if the flag isn't cleared when it is
>>> no longer relevant.  This patch adds a kvm ioctl that the hypervisor will use
>>> when it resumes a guest to start a timer for aging out the flag.  The time out
>>> will be specified by the hypervisor in the ioctl call.
>>>
>>> Signed-off-by: Eric B Munson<emunson@mgebm.net>
>>
>> Why not have the guest clear the flag when it acknowledges it?
>>
>> The hypervisor would unconditionally set the bit, and the guest
>> would do a testandclear to check if the bit is set.  I think that
>> avoids the whole aging business.
>>
>> Regards,
>>
>> Anthony Liguori
>
> If you have a look at patch 5 of this series, the flag is cleared when the
> guest checks the validity of a soft lockup.  However, the aging is meant to
> cover the case where the guest never sees a soft lockup.  We don't want this
> flag to be stored for ever and end up delaying real soft lockup messages.  With
> that case in mind, I thought this was a good/simple compramise.

If the guest clears the flag, then I don't think you have to worry about this.

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6 V2] Add functions to check if the host has stopped the vm
  2011-11-01 19:28   ` Anthony Liguori
@ 2011-11-01 20:07     ` Eric B Munson
  0 siblings, 0 replies; 16+ messages in thread
From: Eric B Munson @ 2011-11-01 20:07 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

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

On Tue, 01 Nov 2011, Anthony Liguori wrote:

> On 10/31/2011 03:07 PM, Eric B Munson wrote:
> >When a host stops or suspends a VM it will set a flag to show this.  The
> >watchdog will use these functions to determine if a softlockup is real, or the
> >result of a suspended VM.
> >
> >Signed-off-by: Eric B Munson<emunson@mgebm.net>
> >---
> >  arch/x86/include/asm/pvclock.h |    2 ++
> >  arch/x86/kernel/kvmclock.c     |   19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> >index c59cc97..7d3ba41 100644
> >--- a/arch/x86/include/asm/pvclock.h
> >+++ b/arch/x86/include/asm/pvclock.h
> >@@ -13,6 +13,8 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
> >  			    struct timespec *ts);
> >  void pvclock_resume(void);
> >
> >+bool kvm_check_and_clear_host_stopped(int cpu);
> 
> A kvm-specific function shouldn't be declared in pvclock.h as this
> is shared code between KVM and Xen.

I'll move it for V3.

> 
> >+
> >  /*
> >   * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
> >   * yielding a 64-bit result.
> >diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> >index c1a0188..8ddcfaf 100644
> >--- a/arch/x86/kernel/kvmclock.c
> >+++ b/arch/x86/kernel/kvmclock.c
> >@@ -113,6 +113,25 @@ static void kvm_get_preset_lpj(void)
> >  	preset_lpj = lpj;
> >  }
> >
> >+bool kvm_check_and_clear_host_stopped(int cpu)
> >+{
> >+	bool ret = false;
> >+	struct pvclock_vcpu_time_info *src;
> >+
> >+	/*
> >+	 * per_cpu() is safe here because this function is only called from
> >+	 * timer functions where preemption is already disabled.
> >+	 */
> >+	WARN_ON(!in_atomic());
> >+	src =&per_cpu(hv_clock, cpu);
> >+	if ((src->flags&  PVCLOCK_GUEST_STOPPED) != 0) {
> >+		src->flags = src->flags&  (~PVCLOCK_GUEST_STOPPED);
> >+		ret = true;
> >+	}
> 
> It would be helpful to separate out the patches for guest enablement
> from host enablement.  This is a guest function, correct?
> 

Yes, this is guest only.

> Don't you need an atomic getandset here?  Can the host clear this
> flag or only set it?

I didn't think they needed to be atomic because an over-write of this means that
a soft lockup message is delayed by the detector's timeout in the worst case.
If you disagree with that assessment, I can move to atomic get/set.

> 
> It's ashame we don't have a proper pvclock spec in Documentation/.
> I would suggest at least adding a description of how this bit works
> in the first patch.
> 



> Regards,
> 
> Anthony Liguori
> 
> >+
> >+	return ret;
> >+}
> >+
> >  static struct clocksource kvm_clock = {
> >  	.name = "kvm-clock",
> >  	.read = kvm_clock_get_cycles,
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6 V2] Add age out of guest paused flag
  2011-11-01 19:58       ` Anthony Liguori
@ 2011-11-01 20:10         ` Eric B Munson
  0 siblings, 0 replies; 16+ messages in thread
From: Eric B Munson @ 2011-11-01 20:10 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

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

On Tue, 01 Nov 2011, Anthony Liguori wrote:

> On 11/01/2011 02:51 PM, Eric B Munson wrote:
> >On Tue, 01 Nov 2011, Anthony Liguori wrote:
> >
> >>On 10/31/2011 03:07 PM, Eric B Munson wrote:
> >>>The KVM_GUEST_PAUSED flag will prevent a guest from compaining about a soft
> >>>lockup but it can mask real soft lockups if the flag isn't cleared when it is
> >>>no longer relevant.  This patch adds a kvm ioctl that the hypervisor will use
> >>>when it resumes a guest to start a timer for aging out the flag.  The time out
> >>>will be specified by the hypervisor in the ioctl call.
> >>>
> >>>Signed-off-by: Eric B Munson<emunson@mgebm.net>
> >>
> >>Why not have the guest clear the flag when it acknowledges it?
> >>
> >>The hypervisor would unconditionally set the bit, and the guest
> >>would do a testandclear to check if the bit is set.  I think that
> >>avoids the whole aging business.
> >>
> >>Regards,
> >>
> >>Anthony Liguori
> >
> >If you have a look at patch 5 of this series, the flag is cleared when the
> >guest checks the validity of a soft lockup.  However, the aging is meant to
> >cover the case where the guest never sees a soft lockup.  We don't want this
> >flag to be stored for ever and end up delaying real soft lockup messages.  With
> >that case in mind, I thought this was a good/simple compramise.
> 
> If the guest clears the flag, then I don't think you have to worry about this.
> 
> Regards,
> 
> Anthony Liguori

Except that the soft lockup doesn't trigger on every qemu "stop", only some of
them.  The flag isn't cleared until the detector is triggered.

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/6 V2] Add ioctl for KVM_GUEST_STOPPED
  2011-11-01 19:32   ` Anthony Liguori
@ 2011-11-01 20:11     ` Eric B Munson
  0 siblings, 0 replies; 16+ messages in thread
From: Eric B Munson @ 2011-11-01 20:11 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: avi, mingo, x86, hpa, arnd, linux-kernel, kvm, linux-arch, ryanh

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

On Tue, 01 Nov 2011, Anthony Liguori wrote:

> On 10/31/2011 03:07 PM, Eric B Munson wrote:
> >Now that we have a flag that will tell the guest it was suspended, create an
> >interface for that communication using a KVM ioctl.
> >
> >Signed-off-by: Eric B Munson<emunson@mgebm.net>
> >---
> >Changes from V1:
> >  Add kvm_set_host_stopped to arch/x86/jvm/x86.c instead of kvmclock.c
> >  Rename KVM_PAUSE_GUEST to KVM_GUEST_PAUSED
> >
> >  arch/x86/include/asm/pvclock.h |    3 +++
> >  arch/x86/kvm/x86.c             |   16 ++++++++++++++++
> >  include/linux/kvm.h            |    2 ++
> >  3 files changed, 21 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> >index 7d3ba41..9312814 100644
> >--- a/arch/x86/include/asm/pvclock.h
> >+++ b/arch/x86/include/asm/pvclock.h
> >@@ -3,6 +3,7 @@
> >
> >  #include<linux/clocksource.h>
> >  #include<asm/pvclock-abi.h>
> >+#include<linux/kvm_host.h>
> >
> >  /* some helper functions for xen and kvm pv clock sources */
> >  cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src);
> >@@ -13,6 +14,8 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
> >  			    struct timespec *ts);
> >  void pvclock_resume(void);
> >
> >+void kvm_set_host_stopped(struct kvm_vcpu *vcpu);
> 
> This can't go in pvclock.h.
> 

Will move for V3.

> >+
> >  bool kvm_check_and_clear_host_stopped(int cpu);
> >
> >  /*
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index c38efd7..592ac3b 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -3295,6 +3295,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >
> >  		goto out;
> >  	}
> >+	case KVM_GUEST_PAUSED: {
> >+		r = 0;
> >+		kvm_set_host_stopped(vcpu);
> >+		break;
> >+	}
> >  	default:
> >  		r = -EINVAL;
> >  	}
> >@@ -6117,6 +6122,17 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_task_switch);
> >
> >+/*
> >+ * kvm_set_host_stopped() indicates to the guest kernel that it has been
> >+ * stopped by the hypervisor.  This function will be called from the host only.
> >+ */
> >+void kvm_set_host_stopped(struct kvm_vcpu *vcpu)
> >+{
> >+	struct pvclock_vcpu_time_info *src =&vcpu->arch.hv_clock;
> >+	src->flags |= PVCLOCK_GUEST_STOPPED;
> 
> Shouldn't we throw some sort of error if you're trying to set a flag
> and kvmclock isn't enabled?
> 
> Regards,
> 
> Anthony Liguori
> 

Is kvmclock a compile time only option or something set at runtime?

> >+}
> >+EXPORT_SYMBOL_GPL(kvm_set_host_stopped);
> >+
> >  int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> >  				  struct kvm_sregs *sregs)
> >  {
> >diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> >index f47fcd3..87cab0d 100644
> >--- a/include/linux/kvm.h
> >+++ b/include/linux/kvm.h
> >@@ -763,6 +763,8 @@ struct kvm_clock_data {
> >  #define KVM_CREATE_SPAPR_TCE	  _IOW(KVMIO,  0xa8, struct kvm_create_spapr_tce)
> >  /* Available with KVM_CAP_RMA */
> >  #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
> >+/* VM is being stopped by host */
> >+#define KVM_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
> >
> >  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1<<  0)
> >
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2011-11-01 20:11 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-31 20:07 [PATCH 0/6 V2] Avoid soft lockup message when KVM is stopped by host Eric B Munson
2011-10-31 20:07 ` [PATCH 1/6 V2] Add flag to indicate that a vm was stopped by the host Eric B Munson
2011-11-01 19:22   ` Anthony Liguori
2011-10-31 20:07 ` [PATCH 2/6 V2] Add functions to check if the host has stopped the vm Eric B Munson
2011-11-01 19:28   ` Anthony Liguori
2011-11-01 20:07     ` Eric B Munson
2011-10-31 20:07 ` [PATCH 3/6 V2] Add ioctl for KVM_GUEST_STOPPED Eric B Munson
2011-11-01 19:32   ` Anthony Liguori
2011-11-01 20:11     ` Eric B Munson
2011-10-31 20:07 ` [PATCH 4/6 V2] Add generic stubs for kvm stop check functions Eric B Munson
2011-10-31 20:07 ` [PATCH 5/6 V2] Add check for suspended vm in softlockup detector Eric B Munson
2011-10-31 20:07 ` [PATCH 6/6 V2] Add age out of guest paused flag Eric B Munson
2011-11-01 19:34   ` Anthony Liguori
2011-11-01 19:51     ` Eric B Munson
2011-11-01 19:58       ` Anthony Liguori
2011-11-01 20:10         ` Eric B Munson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).