[PATCH] Drivers: hv: vmbus: Disallow the freeze PM operation

[PATCH] Drivers: hv: vmbus: Disallow the freeze PM operation

[Dexuan Cui]

Before the hibernation patchset (e.g. f53335e3289f), a Linux VM on Hyper-V
can run "echo freeze > /sys/power/state" (or "systemctl suspend")
to freeze the system. The user can press the keyboard or move the mouse
to wake up the VM. Note: the two aforementioned commands are equivalent
here, because Hyper-V doesn't support the guest ACPI S3 state.

With the hibernation patchset, a Linux VM on Hyper-V can hibernate to disk
and resume back; however, the 'freeze' operation is broken for Hyper-V
Generation-2 VM (which doesn't have a legacy keyboard/mouse): when the
vmbus devices are suspended, the VM can not receive any interrupt from
the synthetic keyboard/mouse devices, so there is no way to wake up the
VM. This is not an issue for Generaton-1 VM, because it looks the legacy
keyboard/mouse devices can still be used to wake up the VM in my test.

IMO 'freeze' in a Linux VM on Hyper-V is not really useful in practice,
so let's disallow the operation for both Gen-1 and Gen-2 VMs, even if
it's not an issue for Gen-1 VMs.

Fixes: f53335e3289f ("Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation")
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 029378c..82a4327 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -28,6 +28,7 @@
 #include <linux/notifier.h>
 #include <linux/ptrace.h>
 #include <linux/screen_info.h>
+#include <linux/suspend.h>
 #include <linux/kdebug.h>
 #include <linux/efi.h>
 #include <linux/random.h>
@@ -2357,6 +2358,23 @@ static void hv_synic_resume(void)
 	.resume = hv_synic_resume,
 };
 
+/*
+ * Note: "freeze/suspend" here means "systemctl suspend".
+ * "systemctl hibernate" is still supported.
+ */
+static int hv_pm_notify(struct notifier_block *nb,
+			unsigned long val, void *ign)
+{
+	if (val == PM_SUSPEND_PREPARE) {
+		pr_info("freeze/suspend is not supported\n");
+		return NOTIFY_BAD;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block hv_pm_nb;
+
 static int __init hv_acpi_init(void)
 {
 	int ret, t;
@@ -2389,6 +2407,8 @@ static int __init hv_acpi_init(void)
 	hv_setup_crash_handler(hv_crash_handler);
 
 	register_syscore_ops(&hv_synic_syscore_ops);
+	hv_pm_nb.notifier_call = hv_pm_notify;
+	register_pm_notifier(&hv_pm_nb);
 
 	return 0;
 
@@ -2402,6 +2422,7 @@ static void __exit vmbus_exit(void)
 {
 	int cpu;
 
+	unregister_pm_notifier(&hv_pm_nb);
 	unregister_syscore_ops(&hv_synic_syscore_ops);
 
 	hv_remove_kexec_handler();
-- 
1.8.3.1

Re: [PATCH] Drivers: hv: vmbus: Disallow the freeze PM operation

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

> Before the hibernation patchset (e.g. f53335e3289f), a Linux VM on Hyper-V
> can run "echo freeze > /sys/power/state" (or "systemctl suspend")
> to freeze the system. The user can press the keyboard or move the mouse
> to wake up the VM. Note: the two aforementioned commands are equivalent
> here, because Hyper-V doesn't support the guest ACPI S3 state.
>
> With the hibernation patchset, a Linux VM on Hyper-V can hibernate to disk
> and resume back; however, the 'freeze' operation is broken for Hyper-V
> Generation-2 VM (which doesn't have a legacy keyboard/mouse): when the
> vmbus devices are suspended, the VM can not receive any interrupt from
> the synthetic keyboard/mouse devices, so there is no way to wake up the
> VM. This is not an issue for Generaton-1 VM, because it looks the legacy
> keyboard/mouse devices can still be used to wake up the VM in my test.
>
> IMO 'freeze' in a Linux VM on Hyper-V is not really useful in practice,
> so let's disallow the operation for both Gen-1 and Gen-2 VMs, even if
> it's not an issue for Gen-1 VMs.

Suspend-to-idle may not be very useful indeed, however, it worked before
and I think we can just fix it. In particular, why do we need to do
anything when we are not hibernating? 

>
> Fixes: f53335e3289f ("Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 029378c..82a4327 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -28,6 +28,7 @@
>  #include <linux/notifier.h>
>  #include <linux/ptrace.h>
>  #include <linux/screen_info.h>
> +#include <linux/suspend.h>
>  #include <linux/kdebug.h>
>  #include <linux/efi.h>
>  #include <linux/random.h>
> @@ -2357,6 +2358,23 @@ static void hv_synic_resume(void)
>  	.resume = hv_synic_resume,
>  };
>  
> +/*
> + * Note: "freeze/suspend" here means "systemctl suspend".
> + * "systemctl hibernate" is still supported.

Let's not use systemd terminology in kernel, let's use the ones from
admin-guide/pm/sleep-states.rst (Suspend-to-Idle/Standby/Suspend-to-RAM/
Hibernation).

> + */
> +static int hv_pm_notify(struct notifier_block *nb,
> +			unsigned long val, void *ign)
> +{
> +	if (val == PM_SUSPEND_PREPARE) {
> +		pr_info("freeze/suspend is not supported\n");
> +		return NOTIFY_BAD;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block hv_pm_nb;
> +
>  static int __init hv_acpi_init(void)
>  {
>  	int ret, t;
> @@ -2389,6 +2407,8 @@ static int __init hv_acpi_init(void)
>  	hv_setup_crash_handler(hv_crash_handler);
>  
>  	register_syscore_ops(&hv_synic_syscore_ops);
> +	hv_pm_nb.notifier_call = hv_pm_notify;
> +	register_pm_notifier(&hv_pm_nb);
>  
>  	return 0;
>  
> @@ -2402,6 +2422,7 @@ static void __exit vmbus_exit(void)
>  {
>  	int cpu;
>  
> +	unregister_pm_notifier(&hv_pm_nb);
>  	unregister_syscore_ops(&hv_synic_syscore_ops);
>  
>  	hv_remove_kexec_handler();

-- 
Vitaly

RE: [PATCH] Drivers: hv: vmbus: Disallow the freeze PM operation

[Dexuan Cui]

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Wednesday, April 8, 2020 8:47 AM
> > IMO 'freeze' in a Linux VM on Hyper-V is not really useful in practice,
> > so let's disallow the operation for both Gen-1 and Gen-2 VMs, even if
> > it's not an issue for Gen-1 VMs.
> 
> Suspend-to-idle may not be very useful indeed, however, it worked before
> and I think we can just fix it.

How can we fix Suspend-to-idle for a Gen-2 VM, in which no device can work
as wakeup devices? Note: in the case of Suspend-to-idle, now all the vmbus
devices including the synthetic keyboard/mouse are suspended completely.

Are you suggesting hv_vmbus should distinguish Suspend-to-idle from
hibernation, and for the former hv_vmbus should not suspend the synthetic
keyboard/mouse? This should be doable but IMO this is not a very trivial
effort, and I'm trying to avoid it since IMO Suspend-to-idle is not really 
useful in practice for a Linux VM on Hyper-V. :-)

> In particular, why do we need to do
> anything when we are not hibernating?

Are you suggesting hv_vmbus should not suspend the vmbus devices at all
in the case of Suspend-to-idle?

> > +/*
> > + * Note: "freeze/suspend" here means "systemctl suspend".
> > + * "systemctl hibernate" is still supported.
> 
> Let's not use systemd terminology in kernel, let's use the ones from
> admin-guide/pm/sleep-states.rst (Suspend-to-Idle/Standby/Suspend-to-RAM/
> Hibernation).
> --
> Vitaly

Thanks! I'll use the accurate terms.

Thanks,
-- Dexuan

RE: [PATCH] Drivers: hv: vmbus: Disallow the freeze PM operation

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

>> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Sent: Wednesday, April 8, 2020 8:47 AM
>> > IMO 'freeze' in a Linux VM on Hyper-V is not really useful in practice,
>> > so let's disallow the operation for both Gen-1 and Gen-2 VMs, even if
>> > it's not an issue for Gen-1 VMs.
>> 
>> Suspend-to-idle may not be very useful indeed, however, it worked before
>> and I think we can just fix it.
>
> How can we fix Suspend-to-idle for a Gen-2 VM, in which no device can work
> as wakeup devices? Note: in the case of Suspend-to-idle, now all the vmbus
> devices including the synthetic keyboard/mouse are suspended completely.
>
> Are you suggesting hv_vmbus should distinguish Suspend-to-idle from
> hibernation, and for the former hv_vmbus should not suspend the synthetic
> keyboard/mouse?

Yes, basically.

>  This should be doable but IMO this is not a very trivial
> effort, and I'm trying to avoid it since IMO Suspend-to-idle is not really 
> useful in practice for a Linux VM on Hyper-V. :-)

Well, to me it's equally (not) useful in all other cases :-) I think we
should Cc: linux-pm@vger.kernel.org and someone will describe a real
world usecase to educate us, we'll then see if there is any Hyper-V
specifics.

>
>> In particular, why do we need to do
>> anything when we are not hibernating?
>
> Are you suggesting hv_vmbus should not suspend the vmbus devices at all
> in the case of Suspend-to-idle?

That what we were doing prior to the hibernation series, right? AFAIU
suspend-to-idle is basically 'no processes are scheduled' mode but we
don't really need to do anything with devices.

>
>> > +/*
>> > + * Note: "freeze/suspend" here means "systemctl suspend".
>> > + * "systemctl hibernate" is still supported.
>> 
>> Let's not use systemd terminology in kernel, let's use the ones from
>> admin-guide/pm/sleep-states.rst (Suspend-to-Idle/Standby/Suspend-to-RAM/
>> Hibernation).
>> --
>> Vitaly
>
> Thanks! I'll use the accurate terms.
>
> Thanks,
> -- Dexuan
>

-- 
Vitaly

RE: [PATCH] Drivers: hv: vmbus: Disallow the freeze PM operation

[Dexuan Cui]

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Wednesday, April 8, 2020 12:24 PM
> To: Dexuan Cui <decui@microsoft.com>
> > ...
> > This should be doable but IMO this is not a very trivial
> > effort, and I'm trying to avoid it since IMO Suspend-to-idle is not really
> > useful in practice for a Linux VM on Hyper-V. :-)
> 
> Well, to me it's equally (not) useful in all other cases :-) I think we
> should Cc: linux-pm@vger.kernel.org and someone will describe a real
> world usecase to educate us, we'll then see if there is any Hyper-V
> specifics.
Maybe I should support Suspend-to-idle, anyway. :-)

> >> In particular, why do we need to do
> >> anything when we are not hibernating?
> >
> > Are you suggesting hv_vmbus should not suspend the vmbus devices at all
> > in the case of Suspend-to-idle?
> 
> That what we were doing prior to the hibernation series, right? AFAIU
Yes.

> suspend-to-idle is basically 'no processes are scheduled' mode but we
> don't really need to do anything with devices.
Got it. Let me try to make a patch to revert to the old behavior for 
Suspend-to-idle.

Thanks,
-- Dexuan