[PATCHv3 1/2] kvm: add PM-notifier

[PATCHv3 1/2] kvm: add PM-notifier

[Sergey Senozhatsky]

Add KVM PM-notifier so that architectures can have arch-specific
VM suspend/resume routines. Such architectures need to select
CONFIG_HAVE_KVM_PM_NOTIFIER and implement kvm_arch_pm_notifier().

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 include/linux/kvm_host.h |  9 +++++++++
 virt/kvm/Kconfig         |  3 +++
 virt/kvm/kvm_main.c      | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 76102efbf079..3db4c22e93cd 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -28,6 +28,7 @@
 #include <linux/rcuwait.h>
 #include <linux/refcount.h>
 #include <linux/nospec.h>
+#include <linux/notifier.h>
 #include <asm/signal.h>
 
 #include <linux/kvm.h>
@@ -585,6 +586,10 @@ struct kvm {
 	pid_t userspace_pid;
 	unsigned int max_halt_poll_ns;
 	u32 dirty_ring_size;
+
+#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
+	struct notifier_block pm_notifier;
+#endif
 };
 
 #define kvm_err(fmt, ...) \
@@ -998,6 +1003,10 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu);
 
+#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
+int kvm_arch_pm_notifier(struct kvm *kvm, unsigned long state);
+#endif
+
 #ifdef __KVM_HAVE_ARCH_VCPU_DEBUGFS
 void kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu, struct dentry *debugfs_dentry);
 #endif
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 1c37ccd5d402..62b39149b8c8 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -63,3 +63,6 @@ config HAVE_KVM_NO_POLL
 
 config KVM_XFER_TO_GUEST_WORK
        bool
+
+config HAVE_KVM_PM_NOTIFIER
+       bool
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index eb440eb1225a..1cceead1af46 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -51,6 +51,7 @@
 #include <linux/io.h>
 #include <linux/lockdep.h>
 #include <linux/kthread.h>
+#include <linux/suspend.h>
 
 #include <asm/processor.h>
 #include <asm/ioctl.h>
@@ -780,6 +781,38 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
 
 #endif /* CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER */
 
+#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
+static int kvm_pm_notifier_call(struct notifier_block *bl,
+				unsigned long state,
+				void *unused)
+{
+	struct kvm *kvm = container_of(bl, struct kvm, pm_notifier);
+
+	return kvm_arch_pm_notifier(kvm, state);
+}
+
+static void kvm_init_pm_notifier(struct kvm *kvm)
+{
+	kvm->pm_notifier.notifier_call = kvm_pm_notifier_call;
+	/* Suspend KVM before we suspend ftrace, RCU, etc. */
+	kvm->pm_notifier.priority = INT_MAX;
+	register_pm_notifier(&kvm->pm_notifier);
+}
+
+static void kvm_destroy_pm_notifier(struct kvm *kvm)
+{
+	unregister_pm_notifier(&kvm->pm_notifier);
+}
+#else /* !CONFIG_HAVE_KVM_PM_NOTIFIER */
+static void kvm_init_pm_notifier(struct kvm *kvm)
+{
+}
+
+static void kvm_destroy_pm_notifier(struct kvm *kvm)
+{
+}
+#endif /* CONFIG_HAVE_KVM_PM_NOTIFIER */
+
 static struct kvm_memslots *kvm_alloc_memslots(void)
 {
 	int i;
@@ -963,6 +996,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	mutex_unlock(&kvm_lock);
 
 	preempt_notifier_inc();
+	kvm_init_pm_notifier(kvm);
 
 	return kvm;
 
@@ -1010,6 +1044,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	int i;
 	struct mm_struct *mm = kvm->mm;
 
+	kvm_destroy_pm_notifier(kvm);
 	kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
 	kvm_destroy_vm_debugfs(kvm);
 	kvm_arch_sync_events(kvm);
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCHv3 2/2] kvm: x86: implement KVM PM-notifier

[Sergey Senozhatsky]

Implement PM hibernation/suspend prepare notifiers so that KVM
can reliably set PVCLOCK_GUEST_STOPPED on VCPUs and properly
suspend VMs.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 arch/x86/kvm/Kconfig |  1 +
 arch/x86/kvm/x86.c   | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index fb8efb387aff..ac69894eab88 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -43,6 +43,7 @@ config KVM
 	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
 	select KVM_VFIO
 	select SRCU
+	select HAVE_KVM_PM_NOTIFIER if PM
 	help
 	  Support hosting fully virtualized guest machines using hardware
 	  virtualization extensions.  You will need a fairly recent
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b594275d49b5..af1ab527a0cb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -58,6 +58,7 @@
 #include <linux/sched/isolation.h>
 #include <linux/mem_encrypt.h>
 #include <linux/entry-kvm.h>
+#include <linux/suspend.h>
 
 #include <trace/events/kvm.h>
 
@@ -5615,6 +5616,41 @@ static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
 	return 0;
 }
 
+#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
+static int kvm_arch_suspend_notifier(struct kvm *kvm)
+{
+	struct kvm_vcpu *vcpu;
+	int i, ret = 0;
+
+	mutex_lock(&kvm->lock);
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		if (!vcpu->arch.pv_time_enabled)
+			continue;
+
+		ret = kvm_set_guest_paused(vcpu);
+		if (ret) {
+			kvm_err("Failed to pause guest VCPU%d: %d\n",
+				vcpu->vcpu_id, ret);
+			break;
+		}
+	}
+	mutex_unlock(&kvm->lock);
+
+	return ret ? NOTIFY_BAD : NOTIFY_DONE;
+}
+
+int kvm_arch_pm_notifier(struct kvm *kvm, unsigned long state)
+{
+	switch (state) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_SUSPEND_PREPARE:
+		return kvm_arch_suspend_notifier(kvm);
+	}
+
+	return NOTIFY_DONE;
+}
+#endif /* CONFIG_HAVE_KVM_PM_NOTIFIER */
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
-- 
2.32.0.rc1.229.g3e70b5a671-goog

Re: [PATCHv3 2/2] kvm: x86: implement KVM PM-notifier

[Maxim Levitsky]

On Sun, 2021-06-06 at 11:10 +0900, Sergey Senozhatsky wrote:
> Implement PM hibernation/suspend prepare notifiers so that KVM
> can reliably set PVCLOCK_GUEST_STOPPED on VCPUs and properly
> suspend VMs.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  arch/x86/kvm/Kconfig |  1 +
>  arch/x86/kvm/x86.c   | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index fb8efb387aff..ac69894eab88 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -43,6 +43,7 @@ config KVM
>  	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
>  	select KVM_VFIO
>  	select SRCU
> +	select HAVE_KVM_PM_NOTIFIER if PM
>  	help
>  	  Support hosting fully virtualized guest machines using hardware
>  	  virtualization extensions.  You will need a fairly recent
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b594275d49b5..af1ab527a0cb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -58,6 +58,7 @@
>  #include <linux/sched/isolation.h>
>  #include <linux/mem_encrypt.h>
>  #include <linux/entry-kvm.h>
> +#include <linux/suspend.h>
>  
>  #include <trace/events/kvm.h>
>  
> @@ -5615,6 +5616,41 @@ static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
> +static int kvm_arch_suspend_notifier(struct kvm *kvm)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int i, ret = 0;
> +
> +	mutex_lock(&kvm->lock);
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (!vcpu->arch.pv_time_enabled)
> +			continue;
> +
> +		ret = kvm_set_guest_paused(vcpu);
> +		if (ret) {
> +			kvm_err("Failed to pause guest VCPU%d: %d\n",
> +				vcpu->vcpu_id, ret);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&kvm->lock);
> +
> +	return ret ? NOTIFY_BAD : NOTIFY_DONE;
> +}
> +
> +int kvm_arch_pm_notifier(struct kvm *kvm, unsigned long state)
> +{
> +	switch (state) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +		return kvm_arch_suspend_notifier(kvm);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +#endif /* CONFIG_HAVE_KVM_PM_NOTIFIER */
> +
>  long kvm_arch_vm_ioctl(struct file *filp,
>  		       unsigned int ioctl, unsigned long arg)
>  {

Overall this looks OK to me.
 
Do you have a test case in which this patch helps to make the guest behave better after
the host suspend though? I tested this and I don't see any significant change.
(guest works after host suspend before and after, and I still have clocksource
watchdogs firing in the guest)
 
 
Also I would like to add my .02 cents on my observations on what happens when I suspend my system
with guests running, which I do once in a while.
I haven't dug deep into it yet as host suspend with VM running wasn't high on my priority list.
 
First of all after a host suspend/resume cycle (and that is true on all 3 machines I own),
the host TSC is reset to 0 on all CPUs, thus while it is still synchronized, it jumps backward.
 
Host kernel has no issues coping with this.
 
Guests however complain about clocksource watchdog and mark the tsc clocksource as unstable,
at least when invtsc is used (regardless of this patch, I wasn't able to notice a difference
with and without it yet).
 
 
[  287.515864] clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
[  287.516926] clocksource:                       'kvm-clock' wd_now: 4437767926 wd_last: 429c3c42f5 mask: ffffffffffffffff
[  287.527100] clocksource:                       'tsc' cs_now: c33f6ce157 cs_last: c1be2ad19f mask: ffffffffffffffff
[  287.528493] tsc: Marking TSC unstable due to clocksource watchdog
[  287.556640] clocksource: Switched to clocksource kvm-clock
 
 
This is from Intel system with stable TSC, but I have seen this on my AMD systems as well,
but these have other issues which might affect this (see below).
 
AFAIK, we have code in kvm_arch_hardware_enable for this exact case but it might not work
correctly or be not enough to deal with this.
 
Also I notice that this code sets kvm->arch.backwards_tsc_observed = true which 
in turn disables the master clock which is not good as well.
 
I haven't yet allocated time to investigate this.
 
 
Another bit of information which I didn't start a discussion (but I think I should), 
which is relevant to AMD systems, is in 'unsynchronized_tsc' function.

On AMD guest it will mark the TSC as unstable in the guest as long as invtsc is not used.
I patched that code out for myself, that is why I am mentioning it.

Best regards,
	Maxim Levitsky

Re: [PATCHv3 2/2] kvm: x86: implement KVM PM-notifier

[Sergey Senozhatsky]

On (21/06/07 15:54), Maxim Levitsky wrote:
> 
> Overall this looks OK to me.
>  
> Do you have a test case in which this patch helps to make the guest behave better after
> the host suspend though? I tested this and I don't see any significant change.
> (guest works after host suspend before and after, and I still have clocksource
> watchdogs firing in the guest)

Yes, we have automated suspend-resume tests. What happens for us is that,
when not properly paused, guests, upon host resume, discover jiffies spikes
and trigger all sort of watchdogs.

Re: [PATCHv3 2/2] kvm: x86: implement KVM PM-notifier

[Sergey Senozhatsky]

On (21/06/07 15:54), Maxim Levitsky wrote:
[..]
> Also I would like to add my .02 cents on my observations on what happens when I suspend my system
> with guests running, which I do once in a while.
> I haven't dug deep into it yet as host suspend with VM running wasn't high on my priority list.
>  
> First of all after a host suspend/resume cycle (and that is true on all 3 machines I own),
> the host TSC is reset to 0 on all CPUs, thus while it is still synchronized, it jumps backward.
>  
> Host kernel has no issues coping with this.
>  
> Guests however complain about clocksource watchdog and mark the tsc clocksource as unstable,
> at least when invtsc is used (regardless of this patch, I wasn't able to notice a difference
> with and without it yet).
>  
>  
> [  287.515864] clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
> [  287.516926] clocksource:                       'kvm-clock' wd_now: 4437767926 wd_last: 429c3c42f5 mask: ffffffffffffffff
> [  287.527100] clocksource:                       'tsc' cs_now: c33f6ce157 cs_last: c1be2ad19f mask: ffffffffffffffff
> [  287.528493] tsc: Marking TSC unstable due to clocksource watchdog
> [  287.556640] clocksource: Switched to clocksource kvm-clock
>  
>  
> This is from Intel system with stable TSC, but I have seen this on my AMD systems as well,
> but these have other issues which might affect this (see below).
>  
> AFAIK, we have code in kvm_arch_hardware_enable for this exact case but it might not work
> correctly or be not enough to deal with this.
>  
> Also I notice that this code sets kvm->arch.backwards_tsc_observed = true which 
> in turn disables the master clock which is not good as well.
>  
> I haven't yet allocated time to investigate this.
>  
>  
> Another bit of information which I didn't start a discussion (but I think I should), 
> which is relevant to AMD systems, is in 'unsynchronized_tsc' function.
> 
> On AMD guest it will mark the TSC as unstable in the guest as long as invtsc is not used.
> I patched that code out for myself, that is why I am mentioning it.

If you are going to fork this discussion the could you please Cc Suleiman
and me? I believe we are having a bunch of problems with the guest clocks
here on our side.

Re: [PATCHv3 2/2] kvm: x86: implement KVM PM-notifier

[Paolo Bonzini]

On 06/06/21 04:10, Sergey Senozhatsky wrote:
> Implement PM hibernation/suspend prepare notifiers so that KVM
> can reliably set PVCLOCK_GUEST_STOPPED on VCPUs and properly
> suspend VMs.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>   arch/x86/kvm/Kconfig |  1 +
>   arch/x86/kvm/x86.c   | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index fb8efb387aff..ac69894eab88 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -43,6 +43,7 @@ config KVM
>   	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
>   	select KVM_VFIO
>   	select SRCU
> +	select HAVE_KVM_PM_NOTIFIER if PM
>   	help
>   	  Support hosting fully virtualized guest machines using hardware
>   	  virtualization extensions.  You will need a fairly recent
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b594275d49b5..af1ab527a0cb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -58,6 +58,7 @@
>   #include <linux/sched/isolation.h>
>   #include <linux/mem_encrypt.h>
>   #include <linux/entry-kvm.h>
> +#include <linux/suspend.h>
>   
>   #include <trace/events/kvm.h>
>   
> @@ -5615,6 +5616,41 @@ static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
>   	return 0;
>   }
>   
> +#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
> +static int kvm_arch_suspend_notifier(struct kvm *kvm)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int i, ret = 0;
> +
> +	mutex_lock(&kvm->lock);
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (!vcpu->arch.pv_time_enabled)
> +			continue;
> +
> +		ret = kvm_set_guest_paused(vcpu);
> +		if (ret) {
> +			kvm_err("Failed to pause guest VCPU%d: %d\n",
> +				vcpu->vcpu_id, ret);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&kvm->lock);
> +
> +	return ret ? NOTIFY_BAD : NOTIFY_DONE;
> +}
> +
> +int kvm_arch_pm_notifier(struct kvm *kvm, unsigned long state)
> +{
> +	switch (state) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +		return kvm_arch_suspend_notifier(kvm);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +#endif /* CONFIG_HAVE_KVM_PM_NOTIFIER */
> +
>   long kvm_arch_vm_ioctl(struct file *filp,
>   		       unsigned int ioctl, unsigned long arg)
>   {
> 

Queued, thanks.

Paolo

Re: [PATCHv3 1/2] kvm: add PM-notifier

[Marc Zyngier]

On Sun, 06 Jun 2021 03:10:44 +0100,
Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> 
> Add KVM PM-notifier so that architectures can have arch-specific
> VM suspend/resume routines. Such architectures need to select
> CONFIG_HAVE_KVM_PM_NOTIFIER and implement kvm_arch_pm_notifier().
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.