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

[PATCHv2 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..83ae0886db89 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;
+
+#if defined(CONFIG_PM) && defined(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);
 
+#if defined(CONFIG_PM) && defined(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..c30502a5eeb8 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 */
 
+#if defined(CONFIG_PM) && defined(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_PM && 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_PM && 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

[PATCHv2 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   | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index fb8efb387aff..f8e6689f490b 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
 	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..533d3d010a21 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,38 @@ static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
 	return 0;
 }
 
+#if defined(CONFIG_PM) && defined(CONFIG_HAVE_KVM_PM_NOTIFIER)
+static int kvm_arch_suspend_notifier(struct kvm *kvm)
+{
+	struct kvm_vcpu *vcpu;
+	int i, ret;
+
+	mutex_lock(&kvm->lock);
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		ret = kvm_set_guest_paused(vcpu);
+		if (ret) {
+			pr_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_PM && 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: [PATCHv2 2/2] kvm: x86: implement KVM PM-notifier

[Marc Zyngier]

On Sat, 05 Jun 2021 03:30:42 +0100,
Sergey Senozhatsky <senozhatsky@chromium.org> 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   | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index fb8efb387aff..f8e6689f490b 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 you write this as:

	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..533d3d010a21 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,38 @@ static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PM) && defined(CONFIG_HAVE_KVM_PM_NOTIFIER)

... you can simplify this expression to be a simple

#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER

in both patches.

> +static int kvm_arch_suspend_notifier(struct kvm *kvm)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int i, ret;
> +
> +	mutex_lock(&kvm->lock);
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		ret = kvm_set_guest_paused(vcpu);
> +		if (ret) {
> +			pr_err("Failed to pause guest VCPU%d: %d\n",
> +			       vcpu->vcpu_id, ret);

Is it really a good idea to fail suspend when a guest doesn't have PV
time enabled? I also wonder how useful the pr_err() is, given that it
contains no information that would help identifying which guest failed
to pause.

> +			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_PM && 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
> 
> 

Thanks,

	M.

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

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

[Sergey Senozhatsky]

On (21/06/05 10:00), Marc Zyngier wrote:
[..]
> > +	select HAVE_KVM_PM_NOTIFIER
> 
> if you write this as:
> 
> 	select HAVE_KVM_PM_NOTIFIER if PM

[..]
> > +#if defined(CONFIG_PM) && defined(CONFIG_HAVE_KVM_PM_NOTIFIER)
> 
> ... you can simplify this expression to be a simple
> 
> #ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
> 
> in both patches.

Thanks.

> > +static int kvm_arch_suspend_notifier(struct kvm *kvm)
> > +{
> > +	struct kvm_vcpu *vcpu;
> > +	int i, ret;
> > +
> > +	mutex_lock(&kvm->lock);
> > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > +		ret = kvm_set_guest_paused(vcpu);
> > +		if (ret) {
> > +			pr_err("Failed to pause guest VCPU%d: %d\n",
> > +			       vcpu->vcpu_id, ret);
> 
> Is it really a good idea to fail suspend when a guest doesn't have PV
> time enabled? I also wonder how useful the pr_err() is, given that it
> contains no information that would help identifying which guest failed
> to pause.

No opinion. What shall we do when we fail to suspend the VM?
VM's watchdogs will trigger and maybe panic the system after
resume.

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

[Sergey Senozhatsky]

On (21/06/05 18:58), Sergey Senozhatsky wrote:
[..]
> > > +static int kvm_arch_suspend_notifier(struct kvm *kvm)
> > > +{
> > > +	struct kvm_vcpu *vcpu;
> > > +	int i, ret;
> > > +
> > > +	mutex_lock(&kvm->lock);
> > > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > > +		ret = kvm_set_guest_paused(vcpu);
> > > +		if (ret) {
> > > +			pr_err("Failed to pause guest VCPU%d: %d\n",
> > > +			       vcpu->vcpu_id, ret);
> > 
> > Is it really a good idea to fail suspend when a guest doesn't have PV
> > time enabled? I also wonder how useful the pr_err() is, given that it
> > contains no information that would help identifying which guest failed
> > to pause.
> 
> No opinion. What shall we do when we fail to suspend the VM?
> VM's watchdogs will trigger and maybe panic the system after
> resume.

For the time being kvm_set_guest_paused() errors out when
!vcpu->arch.pv_time_enabled, but this probably can change
in the future (who knows?). So shall I check vcpu->arch.pv_time_enabled
in  kvm_arch_suspend_notifier()?

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

[Marc Zyngier]

On Sat, 05 Jun 2021 11:03:40 +0100,
Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> 
> On (21/06/05 18:58), Sergey Senozhatsky wrote:
> [..]
> > > > +static int kvm_arch_suspend_notifier(struct kvm *kvm)
> > > > +{
> > > > +	struct kvm_vcpu *vcpu;
> > > > +	int i, ret;
> > > > +
> > > > +	mutex_lock(&kvm->lock);
> > > > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > > > +		ret = kvm_set_guest_paused(vcpu);
> > > > +		if (ret) {
> > > > +			pr_err("Failed to pause guest VCPU%d: %d\n",
> > > > +			       vcpu->vcpu_id, ret);
> > > 
> > > Is it really a good idea to fail suspend when a guest doesn't have PV
> > > time enabled? I also wonder how useful the pr_err() is, given that it
> > > contains no information that would help identifying which guest failed
> > > to pause.
> > 
> > No opinion. What shall we do when we fail to suspend the VM?
> > VM's watchdogs will trigger and maybe panic the system after
> > resume.

Panic the guest, maybe. Panic the host, surely not? You cannot decide
what the guest uses or doesn't use anyway, so that's out of your
hands. I don't think this should prevent the host from suspending, in
any case.

> For the time being kvm_set_guest_paused() errors out when
> !vcpu->arch.pv_time_enabled, but this probably can change in the
> future (who knows?).  So shall I check vcpu->arch.pv_time_enabled in
> kvm_arch_suspend_notifier()?

That, or check for the -EINVAL return value.

	M.

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

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

[Sergey Senozhatsky]

On (21/06/05 11:15), Marc Zyngier wrote:
> > For the time being kvm_set_guest_paused() errors out when
> > !vcpu->arch.pv_time_enabled, but this probably can change in the
> > future (who knows?).  So shall I check vcpu->arch.pv_time_enabled in
> > kvm_arch_suspend_notifier()?
> 
> That, or check for the -EINVAL return value.

I suppose this should do the trick then (hate to do `int ret = 0`,
but we can have no VCPUs with enabled pv_time)

---

+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) {
+                       pr_err("Failed to pause guest VCPU%d: %d\n",
+                              vcpu->vcpu_id, ret);
+                       break;
+               }
+       }
+       mutex_unlock(&kvm->lock);
+
+       return ret ? NOTIFY_BAD : NOTIFY_DONE;
+}

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

[Sergey Senozhatsky]

On (21/06/05 10:00), Marc Zyngier wrote:
> > +static int kvm_arch_suspend_notifier(struct kvm *kvm)
> > +{
> > +	struct kvm_vcpu *vcpu;
> > +	int i, ret;
> > +
> > +	mutex_lock(&kvm->lock);
> > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > +		ret = kvm_set_guest_paused(vcpu);
> > +		if (ret) {
> > +			pr_err("Failed to pause guest VCPU%d: %d\n",
> > +			       vcpu->vcpu_id, ret);
> 
> how useful the pr_err() is, given that it contains no information
> that would help identifying which guest failed to pause.

Do other printk-s contain such info? All I can see so far is
`#define pr_fmt(fmt) "kvm-guest: " fmt` which doesn't point
at any particular VM.

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

[Marc Zyngier]

On 2021-06-05 11:48, Sergey Senozhatsky wrote:
> On (21/06/05 10:00), Marc Zyngier wrote:
>> > +static int kvm_arch_suspend_notifier(struct kvm *kvm)
>> > +{
>> > +	struct kvm_vcpu *vcpu;
>> > +	int i, ret;
>> > +
>> > +	mutex_lock(&kvm->lock);
>> > +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> > +		ret = kvm_set_guest_paused(vcpu);
>> > +		if (ret) {
>> > +			pr_err("Failed to pause guest VCPU%d: %d\n",
>> > +			       vcpu->vcpu_id, ret);
>> 
>> how useful the pr_err() is, given that it contains no information
>> that would help identifying which guest failed to pause.
> 
> Do other printk-s contain such info? All I can see so far is
> `#define pr_fmt(fmt) "kvm-guest: " fmt` which doesn't point
> at any particular VM.

Look for kvm_{err,info,debug...} and vcpu_{err,debug...}, all of
which will at least give you a PID. Even x86 uses it.

         M.
-- 
Jazz is not dead. It just smells funny...

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

[Sergey Senozhatsky]

On (21/06/05 12:27), Marc Zyngier wrote:
> > Do other printk-s contain such info? All I can see so far is
> > `#define pr_fmt(fmt) "kvm-guest: " fmt` which doesn't point
> > at any particular VM.
> 
> Look for kvm_{err,info,debug...} and vcpu_{err,debug...}, all of
> which will at least give you a PID. Even x86 uses it.

Oh, I didn't notice it, arch/x86/kvm/x86.c has very few of them (2?).

Great, thanks.