2020-04-07 22:03:39

by Dexuan Cui

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

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 <[email protected]>
---
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


2020-04-08 15:48:46

by Vitaly Kuznetsov

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

Dexuan Cui <[email protected]> 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 <[email protected]>
> ---
> 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

2020-04-08 19:54:13

by Dexuan Cui

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

> From: Vitaly Kuznetsov <[email protected]>
> 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

2020-04-08 20:31:52

by Vitaly Kuznetsov

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

Dexuan Cui <[email protected]> writes:

>> From: Vitaly Kuznetsov <[email protected]>
>> 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: [email protected] 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

2020-04-08 21:37:03

by Dexuan Cui

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

> From: Vitaly Kuznetsov <[email protected]>
> Sent: Wednesday, April 8, 2020 12:24 PM
> To: Dexuan Cui <[email protected]>
> > ...
> > 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: [email protected] 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