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

[PATCH 1/4 V10] 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.  The host will set the flag when it stops the guest.  On
resume, the guest will check this flag if a soft lockup is detected and skip
issuing the warning.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 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/4 V10] 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>
asm-generic changes Acked-by: Arnd Bergmann <arnd@arndb.de>
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from V6:
 Use __this_cpu_and when clearing the PVCLOCK_GUEST_STOPPED flag

Changes from V5:
 Collapse generic stubs into this patch
 check_and_clear_guest_stopped() takes no args and uses __get_cpu_var()
 Include individual definitions in ia64, s390, and powerpc

 arch/ia64/include/asm/kvm_para.h    |    5 +++++
 arch/powerpc/include/asm/kvm_para.h |    5 +++++
 arch/s390/include/asm/kvm_para.h    |    5 +++++
 arch/x86/include/asm/kvm_para.h     |    8 ++++++++
 arch/x86/kernel/kvmclock.c          |   21 +++++++++++++++++++++
 include/asm-generic/kvm_para.h      |   14 ++++++++++++++
 6 files changed, 58 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-generic/kvm_para.h

diff --git a/arch/ia64/include/asm/kvm_para.h b/arch/ia64/include/asm/kvm_para.h
index 1588aee..2019cb9 100644
--- a/arch/ia64/include/asm/kvm_para.h
+++ b/arch/ia64/include/asm/kvm_para.h
@@ -26,6 +26,11 @@ static inline unsigned int kvm_arch_para_features(void)
 	return 0;
 }
 
+static inline bool kvm_check_and_clear_guest_paused(void)
+{
+	return false;
+}
+
 #endif
 
 #endif
diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
index 50533f9..1f80293 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h
@@ -169,6 +169,11 @@ static inline unsigned int kvm_arch_para_features(void)
 	return r;
 }
 
+static inline bool kvm_check_and_clear_guest_paused(void)
+{
+	return false;
+}
+
 #endif /* __KERNEL__ */
 
 #endif /* __POWERPC_KVM_PARA_H__ */
diff --git a/arch/s390/include/asm/kvm_para.h b/arch/s390/include/asm/kvm_para.h
index 6964db2..a988329 100644
--- a/arch/s390/include/asm/kvm_para.h
+++ b/arch/s390/include/asm/kvm_para.h
@@ -149,6 +149,11 @@ static inline unsigned int kvm_arch_para_features(void)
 	return 0;
 }
 
+static inline bool kvm_check_and_clear_guest_paused(void)
+{
+	return false;
+}
+
 #endif
 
 #endif /* __S390_KVM_PARA_H */
diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 734c376..99c4bbe 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -95,6 +95,14 @@ struct kvm_vcpu_pv_apf_data {
 extern void kvmclock_init(void);
 extern int kvm_register_clock(char *txt);
 
+#ifdef CONFIG_KVM_CLOCK
+bool kvm_check_and_clear_guest_paused(void);
+#else
+static inline bool kvm_check_and_clear_guest_paused(void)
+{
+	return false;
+}
+#endif /* CONFIG_KVMCLOCK */
 
 /* This instruction is vmcall.  On non-VT architectures, it will generate a
  * trap that we will then rewrite to the appropriate instruction.
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 44842d7..bdf6423 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/hardirq.h>
 
 #include <asm/x86_init.h>
 #include <asm/reboot.h>
@@ -114,6 +115,26 @@ static void kvm_get_preset_lpj(void)
 	preset_lpj = lpj;
 }
 
+bool kvm_check_and_clear_guest_paused(void)
+{
+	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 = &__get_cpu_var(hv_clock);
+	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
+		__this_cpu_and(hv_clock.flags, ~PVCLOCK_GUEST_STOPPED);
+		ret = true;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_check_and_clear_guest_paused);
+
 static struct clocksource kvm_clock = {
 	.name = "kvm-clock",
 	.read = kvm_clock_get_cycles,
diff --git a/include/asm-generic/kvm_para.h b/include/asm-generic/kvm_para.h
new file mode 100644
index 0000000..05ef7e7
--- /dev/null
+++ b/include/asm-generic/kvm_para.h
@@ -0,0 +1,14 @@
+#ifndef _ASM_GENERIC_KVM_PARA_H
+#define _ASM_GENERIC_KVM_PARA_H
+
+
+/*
+ * This function is used by architectures that support kvm to avoid issuing
+ * false soft lockup messages.
+ */
+static inline bool kvm_check_and_clear_guest_paused(void)
+{
+	return false;
+}
+
+#endif
-- 
1.7.5.4

[PATCH 3/4 V10] Add ioctl for KVMCLOCK_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>

Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from V9:
 Use kvm_for_each_vcpu to iterate online vcpu's

Changes from V8:
 Make KVM_GUEST_PAUSED a per vm ioctl instead of per vcpu

Changes from V7:
 Define KVM_CAP_GUEST_PAUSED and support check
 Call mark_page_dirty () after setting PVCLOCK_GUEST_STOPPED

Changes from V4:
 Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
 Add new ioctl description to api.txt

 Documentation/virtual/kvm/api.txt |   13 +++++++++++++
 arch/x86/kvm/x86.c                |   25 +++++++++++++++++++++++++
 include/linux/kvm.h               |    3 +++
 3 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index e1d94bf..1931e5c 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1491,6 +1491,19 @@ following algorithm:
 Some guests configure the LINT1 NMI input to cause a panic, aiding in
 debugging.
 
+4.65 KVMCLOCK_GUEST_PAUSED
+
+Capability: KVM_CAP_GUEST_PAUSED
+Architechtures: Any that implement pvclocks (currently x86 only)
+Type: vcpu ioctl
+Parameters: None
+Returns: 0 on success, -1 on error
+
+This signals to the host kernel that the specified guest is being paused by
+userspace.  The host will set a flag in the pvclock structure that is checked
+from the soft lockup watchdog.  This ioctl can be called during pause or
+unpause.
+
 5. The kvm_run structure
 
 Application code obtains a pointer to the kvm_run structure by
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 14d6cad..1341e3b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2056,6 +2056,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_X86_ROBUST_SINGLESTEP:
 	case KVM_CAP_XSAVE:
 	case KVM_CAP_ASYNC_PF:
+	case KVM_CAP_GUEST_PAUSED:
 	case KVM_CAP_GET_TSC_KHZ:
 		r = 1;
 		break;
@@ -3061,6 +3062,26 @@ out:
 	return r;
 }
 
+/*
+ * kvm_set_guest_paused() indicates to the guest kernel that it has been
+ * stopped by the hypervisor.  This function will be called from the host only.
+ */
+static int kvm_set_guest_paused(struct kvm *kvm)
+{
+	struct kvm_vcpu *vcpu;
+	struct pvclock_vcpu_time_info *src;
+	int i;
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		if (!vcpu->arch.time_page)
+			continue;
+		src = &vcpu->arch.hv_clock;
+		src->flags |= PVCLOCK_GUEST_STOPPED;
+		mark_page_dirty(vcpu->kvm, vcpu->arch.time >> PAGE_SHIFT);
+	}
+	return 0;
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
@@ -3351,6 +3372,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		r = 0;
 		break;
 	}
+	case KVMCLOCK_GUEST_PAUSED: {
+		r = kvm_set_guest_paused(kvm);
+		break;
+	}
 
 	default:
 		;
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 68e67e5..4ffe0df 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -558,6 +558,7 @@ struct kvm_ppc_pvinfo {
 #define KVM_CAP_PPC_PAPR 68
 #define KVM_CAP_S390_GMAP 71
 #define KVM_CAP_TSC_DEADLINE_TIMER 72
+#define KVM_CAP_GUEST_PAUSED 73
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -763,6 +764,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 KVMCLOCK_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
-- 
1.7.5.4

[PATCH 4/4 V10] 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.  When the watchdog is reset successfully, clear the guest
paused flag.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from V3:
 Clear the PAUSED flag when the watchdog is reset

 kernel/watchdog.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 1d7bca7..91485e5 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -25,6 +25,7 @@
 #include <linux/sysctl.h>
 
 #include <asm/irq_regs.h>
+#include <linux/kvm_para.h>
 #include <linux/perf_event.h>
 
 int watchdog_enabled = 1;
@@ -280,6 +281,9 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 			__this_cpu_write(softlockup_touch_sync, false);
 			sched_clock_tick();
 		}
+
+		/* Clear the guest paused flag on watchdog reset */
+		kvm_check_and_clear_guest_paused();
 		__touch_watchdog();
 		return HRTIMER_RESTART;
 	}
@@ -292,6 +296,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_guest_paused())
+			return HRTIMER_RESTART;
+
 		/* only warn once */
 		if (__this_cpu_read(soft_watchdog_warn) == true)
 			return HRTIMER_RESTART;
-- 
1.7.5.4

Re: [PATCH 0/4 V10] Avoid soft lockup message when KVM is stopped by host

[Eric B Munson]

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

On Tue, 17 Jan 2012, Eric B Munson wrote:

> Changes from V9:
> Use kvm_for_each_vcpu to iterate online vcpu's
> 
> Changes from V8:
> Make KVM_GUEST_PAUSED a per vm ioctl instead of per vcpu
> 
> Changes from V7:
> Define KVM_CAP_GUEST_PAUSED and support check
> Call mark_page_dirty () after setting PVCLOCK_GUEST_STOPPED
> 
> Changes from V6:
> Use __this_cpu_and when clearing the PVCLOCK_GUEST_STOPPED flag
> 
> Changes from V5:
> Collapse generic check_and_clear_guest_stopped into patch 2
> Include check_and_clear_guest_stopped defintion to ia64, s390, and powerpc
> Change check_and_clear_guest_stopped to use __get_cpu_var instead of taking the
>  cpuid arg.
> Protect check_and_clear_guest_stopped declaration with CONFIG_KVM_CLOCK check
> 
> Changes from V4:
> Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
> Add description of KVMCLOCK_GUEST_PAUSED ioctl to api.txt
> 
> Changes from V3:
> Include CC's on patch 3
> Drop clear flag ioctl and have the watchdog clear the flag when it is reset
> 
> Changes from V2:
> A new kvm functions defined in kvm_para.h, the only change to pvclock is the
> initial flag definition
> 
> 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
> 
> Eric B Munson (4):
>   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 KVMCLOCK_GUEST_STOPPED
>   Add check for suspended vm in softlockup detector
> 
>  Documentation/virtual/kvm/api.txt   |   13 +++++++++++++
>  arch/ia64/include/asm/kvm_para.h    |    5 +++++
>  arch/powerpc/include/asm/kvm_para.h |    5 +++++
>  arch/s390/include/asm/kvm_para.h    |    5 +++++
>  arch/x86/include/asm/kvm_para.h     |    8 ++++++++
>  arch/x86/include/asm/pvclock-abi.h  |    1 +
>  arch/x86/kernel/kvmclock.c          |   21 +++++++++++++++++++++
>  arch/x86/kvm/x86.c                  |   25 +++++++++++++++++++++++++
>  include/asm-generic/kvm_para.h      |   14 ++++++++++++++
>  include/linux/kvm.h                 |    3 +++
>  kernel/watchdog.c                   |   12 ++++++++++++
>  11 files changed, 112 insertions(+), 0 deletions(-)
>  create mode 100644 include/asm-generic/kvm_para.h
> 

Just poking to make sure this set hasn't fallen through the cracks.

Eric

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

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Avi Kivity]

On 01/17/2012 08:40 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.
>
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index e1d94bf..1931e5c 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1491,6 +1491,19 @@ following algorithm:
>  Some guests configure the LINT1 NMI input to cause a panic, aiding in
>  debugging.
>  
> +4.65 KVMCLOCK_GUEST_PAUSED
> +
> +Capability: KVM_CAP_GUEST_PAUSED
> +Architechtures: Any that implement pvclocks (currently x86 only)
> +Type: vcpu ioctl

vm ioctl.

> +Parameters: None
> +Returns: 0 on success, -1 on error
> +
> +This signals to the host kernel that the specified guest is being paused by
> +userspace.  The host will set a flag in the pvclock structure that is checked
> +from the soft lockup watchdog.  This ioctl can be called during pause or
> +unpause.
> +
>  5. The kvm_run structure
>  
>  
> +/*
> + * kvm_set_guest_paused() indicates to the guest kernel that it has been
> + * stopped by the hypervisor.  This function will be called from the host only.
> + */
> +static int kvm_set_guest_paused(struct kvm *kvm)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct pvclock_vcpu_time_info *src;
> +	int i;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (!vcpu->arch.time_page)
> +			continue;
> +		src = &vcpu->arch.hv_clock;
> +		src->flags |= PVCLOCK_GUEST_STOPPED;

This looks racy.  The vcpu can remove its kvmclock concurrently with
this access, and src will be NULL.

Can you point me to the discussion that moved this to be a vm ioctl?  In
general vm ioctls that do things for all vcpus are racy, like here. 
You're accessing variables that are protected by the vcpu mutex, and not
taking the mutex (nor can you, since it is held while the guest is
running, unlike most kernel mutexes).

> +		mark_page_dirty(vcpu->kvm, vcpu->arch.time >> PAGE_SHIFT);
> +	}
> +	return 0;
> +}
> +
>  long kvm_arch_vm_ioctl(struct file *filp,
>  		       unsigned int ioctl, unsigned long arg)
>  {
> @@ -3351,6 +3372,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		r = 0;
>  		break;
>  	}
> +	case KVMCLOCK_GUEST_PAUSED: {
> +		r = kvm_set_guest_paused(kvm);
> +		break;
> +	}
>  
>  	default:
>  		;
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 68e67e5..4ffe0df 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -558,6 +558,7 @@ struct kvm_ppc_pvinfo {
>  #define KVM_CAP_PPC_PAPR 68
>  #define KVM_CAP_S390_GMAP 71
>  #define KVM_CAP_TSC_DEADLINE_TIMER 72
> +#define KVM_CAP_GUEST_PAUSED 73
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -763,6 +764,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 KVMCLOCK_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
>  
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>  


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/4 V10] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 01/27/2012 10:45 PM, Eric B Munson wrote:
> Just poking to make sure this set hasn't fallen through the cracks.
>

Sorry, was on vacation and am still recovering.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Avi Kivity]

On 01/30/2012 05:07 PM, Avi Kivity wrote:
> > +Parameters: None
> > +Returns: 0 on success, -1 on error
> > +
> > +This signals to the host kernel that the specified guest is being paused by
> > +userspace.  The host will set a flag in the pvclock structure that is checked
> > +from the soft lockup watchdog.  This ioctl can be called during pause or
> > +unpause.
> > +
> >  5. The kvm_run structure
> >  
> >  
> > +/*
> > + * kvm_set_guest_paused() indicates to the guest kernel that it has been
> > + * stopped by the hypervisor.  This function will be called from the host only.
> > + */
> > +static int kvm_set_guest_paused(struct kvm *kvm)
> > +{
> > +	struct kvm_vcpu *vcpu;
> > +	struct pvclock_vcpu_time_info *src;
> > +	int i;
> > +
> > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > +		if (!vcpu->arch.time_page)
> > +			continue;
> > +		src = &vcpu->arch.hv_clock;
> > +		src->flags |= PVCLOCK_GUEST_STOPPED;
>
> This looks racy.  The vcpu can remove its kvmclock concurrently with
> this access, and src will be NULL.
>
> Can you point me to the discussion that moved this to be a vm ioctl?  In
> general vm ioctls that do things for all vcpus are racy, like here. 
> You're accessing variables that are protected by the vcpu mutex, and not
> taking the mutex (nor can you, since it is held while the guest is
> running, unlike most kernel mutexes).
>

Note, the standard way to fix this race is to
kvm_make_request(KVM_REQ_KVMCLOCK_STOP) and kvm_vcpu_kick(vcpu), and do
the update in vcpu_enter_guest().  But I think this is needless
complexity and want to understand what motivated the move to a vm ioctl.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Eric B Munson]

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

On Mon, 30 Jan 2012, Avi Kivity wrote:

> On 01/17/2012 08:40 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.
> >
> >
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index e1d94bf..1931e5c 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1491,6 +1491,19 @@ following algorithm:
> >  Some guests configure the LINT1 NMI input to cause a panic, aiding in
> >  debugging.
> >  
> > +4.65 KVMCLOCK_GUEST_PAUSED
> > +
> > +Capability: KVM_CAP_GUEST_PAUSED
> > +Architechtures: Any that implement pvclocks (currently x86 only)
> > +Type: vcpu ioctl
> 
> vm ioctl.
> 
> > +Parameters: None
> > +Returns: 0 on success, -1 on error
> > +
> > +This signals to the host kernel that the specified guest is being paused by
> > +userspace.  The host will set a flag in the pvclock structure that is checked
> > +from the soft lockup watchdog.  This ioctl can be called during pause or
> > +unpause.
> > +
> >  5. The kvm_run structure
> >  
> >  
> > +/*
> > + * kvm_set_guest_paused() indicates to the guest kernel that it has been
> > + * stopped by the hypervisor.  This function will be called from the host only.
> > + */
> > +static int kvm_set_guest_paused(struct kvm *kvm)
> > +{
> > +	struct kvm_vcpu *vcpu;
> > +	struct pvclock_vcpu_time_info *src;
> > +	int i;
> > +
> > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > +		if (!vcpu->arch.time_page)
> > +			continue;
> > +		src = &vcpu->arch.hv_clock;
> > +		src->flags |= PVCLOCK_GUEST_STOPPED;
> 
> This looks racy.  The vcpu can remove its kvmclock concurrently with
> this access, and src will be NULL.
> 
> Can you point me to the discussion that moved this to be a vm ioctl?  In
> general vm ioctls that do things for all vcpus are racy, like here. 
> You're accessing variables that are protected by the vcpu mutex, and not
> taking the mutex (nor can you, since it is held while the guest is
> running, unlike most kernel mutexes).
> 

Jan Kiszka suggested that becuase there isn't a use case for notifying
individual vcpus (can vcpu's be paused individually?) that it makes more sense
to have a vm ioctl.

http://thread.gmane.org/gmane.comp.emulators.qemu/131624

If the per vcpu ioctl is the right choice I can resend those patches.

Eric

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

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Eric B Munson]

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

On Mon, 30 Jan 2012, Avi Kivity wrote:

> On 01/30/2012 05:07 PM, Avi Kivity wrote:
> > > +Parameters: None
> > > +Returns: 0 on success, -1 on error
> > > +
> > > +This signals to the host kernel that the specified guest is being paused by
> > > +userspace.  The host will set a flag in the pvclock structure that is checked
> > > +from the soft lockup watchdog.  This ioctl can be called during pause or
> > > +unpause.
> > > +
> > >  5. The kvm_run structure
> > >  
> > >  
> > > +/*
> > > + * kvm_set_guest_paused() indicates to the guest kernel that it has been
> > > + * stopped by the hypervisor.  This function will be called from the host only.
> > > + */
> > > +static int kvm_set_guest_paused(struct kvm *kvm)
> > > +{
> > > +	struct kvm_vcpu *vcpu;
> > > +	struct pvclock_vcpu_time_info *src;
> > > +	int i;
> > > +
> > > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > > +		if (!vcpu->arch.time_page)
> > > +			continue;
> > > +		src = &vcpu->arch.hv_clock;
> > > +		src->flags |= PVCLOCK_GUEST_STOPPED;
> >
> > This looks racy.  The vcpu can remove its kvmclock concurrently with
> > this access, and src will be NULL.
> >
> > Can you point me to the discussion that moved this to be a vm ioctl?  In
> > general vm ioctls that do things for all vcpus are racy, like here. 
> > You're accessing variables that are protected by the vcpu mutex, and not
> > taking the mutex (nor can you, since it is held while the guest is
> > running, unlike most kernel mutexes).
> >
> 
> Note, the standard way to fix this race is to
> kvm_make_request(KVM_REQ_KVMCLOCK_STOP) and kvm_vcpu_kick(vcpu), and do
> the update in vcpu_enter_guest().  But I think this is needless
> complexity and want to understand what motivated the move to a vm ioctl.
> 

I will hold off on fixing this race until we settle the vm or vcpu ioctl
question.

Eric

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

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Avi Kivity]

On 01/30/2012 05:32 PM, Eric B Munson wrote:
> > 
> > Can you point me to the discussion that moved this to be a vm ioctl?  In
> > general vm ioctls that do things for all vcpus are racy, like here. 
> > You're accessing variables that are protected by the vcpu mutex, and not
> > taking the mutex (nor can you, since it is held while the guest is
> > running, unlike most kernel mutexes).
> > 
>
> Jan Kiszka suggested that becuase there isn't a use case for notifying
> individual vcpus (can vcpu's be paused individually?

They can, though the guest will grind to a halt very soon.

> ) that it makes more sense
> to have a vm ioctl.
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/131624
>
> If the per vcpu ioctl is the right choice I can resend those patches.

The races are solvable but I think it's easier in userspace.  It's also
more flexible, though I don't really see a use for this flexibility.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Eric B Munson]

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

On Mon, 30 Jan 2012, Avi Kivity wrote:

> On 01/30/2012 05:32 PM, Eric B Munson wrote:
> > > 
> > > Can you point me to the discussion that moved this to be a vm ioctl?  In
> > > general vm ioctls that do things for all vcpus are racy, like here. 
> > > You're accessing variables that are protected by the vcpu mutex, and not
> > > taking the mutex (nor can you, since it is held while the guest is
> > > running, unlike most kernel mutexes).
> > > 
> >
> > Jan Kiszka suggested that becuase there isn't a use case for notifying
> > individual vcpus (can vcpu's be paused individually?
> 
> They can, though the guest will grind to a halt very soon.
> 
> > ) that it makes more sense
> > to have a vm ioctl.
> >
> > http://thread.gmane.org/gmane.comp.emulators.qemu/131624
> >
> > If the per vcpu ioctl is the right choice I can resend those patches.
> 
> The races are solvable but I think it's easier in userspace.  It's also
> more flexible, though I don't really see a use for this flexibility.
> 

Okay, I will rebase the per vcpu patches and resend those.

Eric

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

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Jan Kiszka]

On 2012-01-30 16:07, Avi Kivity wrote:
> On 01/17/2012 08:40 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.
>>
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index e1d94bf..1931e5c 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1491,6 +1491,19 @@ following algorithm:
>>  Some guests configure the LINT1 NMI input to cause a panic, aiding in
>>  debugging.
>>  
>> +4.65 KVMCLOCK_GUEST_PAUSED
>> +
>> +Capability: KVM_CAP_GUEST_PAUSED
>> +Architechtures: Any that implement pvclocks (currently x86 only)
>> +Type: vcpu ioctl
> 
> vm ioctl.
> 
>> +Parameters: None
>> +Returns: 0 on success, -1 on error
>> +
>> +This signals to the host kernel that the specified guest is being paused by
>> +userspace.  The host will set a flag in the pvclock structure that is checked
>> +from the soft lockup watchdog.  This ioctl can be called during pause or
>> +unpause.
>> +
>>  5. The kvm_run structure
>>  
>>  
>> +/*
>> + * kvm_set_guest_paused() indicates to the guest kernel that it has been
>> + * stopped by the hypervisor.  This function will be called from the host only.
>> + */
>> +static int kvm_set_guest_paused(struct kvm *kvm)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	struct pvclock_vcpu_time_info *src;
>> +	int i;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		if (!vcpu->arch.time_page)
>> +			continue;
>> +		src = &vcpu->arch.hv_clock;
>> +		src->flags |= PVCLOCK_GUEST_STOPPED;
> 
> This looks racy.  The vcpu can remove its kvmclock concurrently with
> this access, and src will be NULL.

There is no race here (src is member of the vcpu), but arch.time might
have become invalid. KVM_REQ_CLOCK_UPDATE instead of mark_page_dirty
would indeed be the way to go. Trivial solution, I would say.

However, the concept of "guest stopped" has VM, not VCPU scope. That
makes the call more appropriate as a VM ioctl. If that thing should
really become per-vcpu, at least call it KVMCLOCK_VCPU_STOPPED.

Jan

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

Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Avi Kivity]

On 01/30/2012 06:18 PM, Jan Kiszka wrote:
> > 
> > This looks racy.  The vcpu can remove its kvmclock concurrently with
> > this access, and src will be NULL.
>
> There is no race here (src is member of the vcpu), but arch.time might
> have become invalid. KVM_REQ_CLOCK_UPDATE instead of mark_page_dirty
> would indeed be the way to go. Trivial solution, I would say.
>
> However, the concept of "guest stopped" has VM, not VCPU scope. 

We're not stopping the guest here, just setting a flag in kvmclock,
which certainly is a per-vcpu thing.

> That
> makes the call more appropriate as a VM ioctl. If that thing should
> really become per-vcpu, at least call it KVMCLOCK_VCPU_STOPPED.
>

All current ioctls start with KVM_. Maybe KVM_KVMCLOCK_CTRL?

-- 
error compiling committee.c: too many arguments to function