Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Dexuan Cui <decui@microsoft.com>
Cc:     kys@microsoft.com, haiyangz@microsoft.com, sthemmin@microsoft.com,
        wei.liu@kernel.org, linux-hyperv@vger.kernel.org,
        linux-kernel@vger.kernel.org, mikelley@microsoft.com
Subject: Re: [PATCH] Drivers: hv: vmbus: Disallow the freeze PM operation
In-Reply-To: <1586296907-53744-1-git-send-email-decui@microsoft.com>
References: <1586296907-53744-1-git-send-email-decui@microsoft.com>
Date:   Wed, 08 Apr 2020 17:47:18 +0200
Message-ID: <877dyq2d4p.fsf@vitty.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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