[PATCH 0/6] Avoid soft lockup message when KVM is stopped by host

[PATCH 0/6] Avoid soft lockup message when KVM is stopped by host

[Eric B Munson]

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         |   55 ++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.c                 |   14 +++++++++
 include/asm-generic/pvclock.h      |   19 ++++++++++++
 include/linux/kvm.h                |    4 ++
 include/linux/kvm_host.h           |    2 +
 kernel/watchdog.c                  |    9 ++++++
 8 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-generic/pvclock.h

-- 
1.7.5.4

[PATCH 1/6] Add flag to indicate that a vm was stopped by the host

[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

[PATCH 2/6] Add functions to check if the host has stopped the vm

[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

[PATCH 3/6] Add ioctl for KVM_GUEST_STOPPED

[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>
---
 arch/x86/include/asm/pvclock.h |    3 +++
 arch/x86/kernel/kvmclock.c     |   12 ++++++++++++
 arch/x86/kvm/x86.c             |    5 +++++
 include/linux/kvm.h            |    2 ++
 4 files changed, 22 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/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 8ddcfaf..f4fff3d 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -22,6 +22,7 @@
 #include <asm/msr.h>
 #include <asm/apic.h>
 #include <linux/percpu.h>
+#include <linux/kvm_host.h>
 
 #include <asm/x86_init.h>
 #include <asm/reboot.h>
@@ -113,6 +114,17 @@ static void kvm_get_preset_lpj(void)
 	preset_lpj = lpj;
 }
 
+/*
+ * 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);
+
 bool kvm_check_and_clear_host_stopped(int cpu)
 {
 	bool ret = false;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 84a28ea..16e029e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3264,6 +3264,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 		goto out;
 	}
+	case KVM_PAUSE_GUEST: {
+		r = 0;
+		kvm_set_host_stopped(vcpu);
+		break;
+	}
 	default:
 		r = -EINVAL;
 	}
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index aace6b8..a2697be 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -759,6 +759,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_PAUSE_GUEST		  _IO(KVMIO,   0xaa)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
-- 
1.7.5.4

[PATCH 4/6] Add generic stubs for kvm stop check functions

[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

[PATCH 5/6] Add check for suspended vm in softlockup detector

[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 36491cd..8107ba7 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

[PATCH 6/6] Add age out of guest paused flag

[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>
---
 arch/x86/include/asm/pvclock.h |    2 ++
 arch/x86/kernel/kvmclock.c     |   24 ++++++++++++++++++++++++
 arch/x86/kvm/x86.c             |    9 +++++++++
 include/linux/kvm.h            |    2 ++
 include/linux/kvm_host.h       |    2 ++
 5 files changed, 39 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/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index f4fff3d..f3f3935 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -23,6 +23,8 @@
 #include <asm/apic.h>
 #include <linux/percpu.h>
 #include <linux/kvm_host.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
 
 #include <asm/x86_init.h>
 #include <asm/reboot.h>
@@ -144,6 +146,28 @@ bool kvm_check_and_clear_host_stopped(int cpu)
 	return ret;
 }
 
+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);
+
 static struct clocksource kvm_clock = {
 	.name = "kvm-clock",
 	.read = kvm_clock_get_cycles,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 16e029e..b907b5a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3269,6 +3269,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;
 	}
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index a2697be..f333274 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -761,6 +761,8 @@ struct kvm_clock_data {
 #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
 /* VM is being stopped by host */
 #define KVM_PAUSE_GUEST		  _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 eabb21a..e21be4b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -18,6 +18,7 @@
 #include <linux/msi.h>
 #include <linux/slab.h>
 #include <linux/rcupdate.h>
+#include <linux/timer.h>
 #include <asm/signal.h>
 
 #include <linux/kvm.h>
@@ -152,6 +153,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

Re: [PATCH 3/6] Add ioctl for KVM_GUEST_STOPPED

[Marcelo Tosatti]

On Tue, Oct 25, 2011 at 03:26:16PM -0400, 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>
> ---
>  arch/x86/include/asm/pvclock.h |    3 +++
>  arch/x86/kernel/kvmclock.c     |   12 ++++++++++++
>  arch/x86/kvm/x86.c             |    5 +++++
>  include/linux/kvm.h            |    2 ++
>  4 files changed, 22 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/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 8ddcfaf..f4fff3d 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -22,6 +22,7 @@
>  #include <asm/msr.h>
>  #include <asm/apic.h>
>  #include <linux/percpu.h>
> +#include <linux/kvm_host.h>
>  
>  #include <asm/x86_init.h>
>  #include <asm/reboot.h>
> @@ -113,6 +114,17 @@ static void kvm_get_preset_lpj(void)
>  	preset_lpj = lpj;
>  }
>  
> +/*
> + * 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);
> +
>  bool kvm_check_and_clear_host_stopped(int cpu)
>  {
>  	bool ret = false;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 84a28ea..16e029e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3264,6 +3264,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  
>  		goto out;
>  	}
> +	case KVM_PAUSE_GUEST: {

Pause guest?? :)

> +		r = 0;
> +		kvm_set_host_stopped(vcpu);
> +		break;
> +	}
>  	default:
>  		r = -EINVAL;
>  	}

Re: [PATCH 6/6] Add age out of guest paused flag

[Marcelo Tosatti]

On Tue, Oct 25, 2011 at 03:26:19PM -0400, 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>
> ---
>  arch/x86/include/asm/pvclock.h |    2 ++
>  arch/x86/kernel/kvmclock.c     |   24 ++++++++++++++++++++++++
>  arch/x86/kvm/x86.c             |    9 +++++++++
>  include/linux/kvm.h            |    2 ++
>  include/linux/kvm_host.h       |    2 ++
>  5 files changed, 39 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/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index f4fff3d..f3f3935 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -23,6 +23,8 @@
>  #include <asm/apic.h>
>  #include <linux/percpu.h>
>  #include <linux/kvm_host.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
>  
>  #include <asm/x86_init.h>
>  #include <asm/reboot.h>
> @@ -144,6 +146,28 @@ bool kvm_check_and_clear_host_stopped(int cpu)
>  	return ret;
>  }
>  
> +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);

So this is why the scheme could become awkward. You can't determine a   
value for length that will guarantee that _no_ genuine softlockup is    
omitted.                                                                

Can you think of a way to achieve this?

Also note, all host code is located in arch/x86/kvm, guest 
code in arch/x86/kernel/kvmclock.c.

Re: [PATCH 3/6] Add ioctl for KVM_GUEST_STOPPED

[Eric B Munson]

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

On Thu, 27 Oct 2011, Marcelo Tosatti wrote:

> On Tue, Oct 25, 2011 at 03:26:16PM -0400, 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>
> > ---
> >  arch/x86/include/asm/pvclock.h |    3 +++
> >  arch/x86/kernel/kvmclock.c     |   12 ++++++++++++
> >  arch/x86/kvm/x86.c             |    5 +++++
> >  include/linux/kvm.h            |    2 ++
> >  4 files changed, 22 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/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index 8ddcfaf..f4fff3d 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> > @@ -22,6 +22,7 @@
> >  #include <asm/msr.h>
> >  #include <asm/apic.h>
> >  #include <linux/percpu.h>
> > +#include <linux/kvm_host.h>
> >  
> >  #include <asm/x86_init.h>
> >  #include <asm/reboot.h>
> > @@ -113,6 +114,17 @@ static void kvm_get_preset_lpj(void)
> >  	preset_lpj = lpj;
> >  }
> >  
> > +/*
> > + * 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);
> > +
> >  bool kvm_check_and_clear_host_stopped(int cpu)
> >  {
> >  	bool ret = false;
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 84a28ea..16e029e 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3264,6 +3264,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >  
> >  		goto out;
> >  	}
> > +	case KVM_PAUSE_GUEST: {
> 
> Pause guest?? :)

GUEST_PAUSED for the next version.

> 
> > +		r = 0;
> > +		kvm_set_host_stopped(vcpu);
> > +		break;
> > +	}
> >  	default:
> >  		r = -EINVAL;
> >  	}
> 

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

Re: [PATCH 6/6] Add age out of guest paused flag

[Eric B Munson]

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

Thanks for the review.

On Fri, 28 Oct 2011, Marcelo Tosatti wrote:

> On Tue, Oct 25, 2011 at 03:26:19PM -0400, 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>
> > ---
> >  arch/x86/include/asm/pvclock.h |    2 ++
> >  arch/x86/kernel/kvmclock.c     |   24 ++++++++++++++++++++++++
> >  arch/x86/kvm/x86.c             |    9 +++++++++
> >  include/linux/kvm.h            |    2 ++
> >  include/linux/kvm_host.h       |    2 ++
> >  5 files changed, 39 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/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index f4fff3d..f3f3935 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> > @@ -23,6 +23,8 @@
> >  #include <asm/apic.h>
> >  #include <linux/percpu.h>
> >  #include <linux/kvm_host.h>
> > +#include <linux/kobject.h>
> > +#include <linux/sysfs.h>
> >  
> >  #include <asm/x86_init.h>
> >  #include <asm/reboot.h>
> > @@ -144,6 +146,28 @@ bool kvm_check_and_clear_host_stopped(int cpu)
> >  	return ret;
> >  }
> >  
> > +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);
> 
> So this is why the scheme could become awkward. You can't determine a   
> value for length that will guarantee that _no_ genuine softlockup is    
> omitted.
> 
> Can you think of a way to achieve this?

Given the way this set works now, I don't see a way to completely avoid the
chance of covering the first notification of a soft lockup.  But, if I
understand the way this code works, the watchdog will come back and complain
if the CPU is still soft locked on the next pass.  Unless I am not reading the
watchdog code correctly, this will only delay the warning of a real soft lockup.

> 
> Also note, all host code is located in arch/x86/kvm, guest 
> code in arch/x86/kernel/kvmclock.c.

I will reshuffle for the next revision.
> 

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