netdev - Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50298859-0d0e-6eb0-029b-30df2a4ecd63@oracle.com>
Date:   Mon, 13 Jul 2020 11:52:01 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Anchal Agarwal <anchalag@...zon.com>, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, hpa@...or.com, x86@...nel.org,
        jgross@...e.com, linux-pm@...r.kernel.org, linux-mm@...ck.org,
        kamatam@...zon.com, sstabellini@...nel.org, konrad.wilk@...cle.com,
        roger.pau@...rix.com, axboe@...nel.dk, davem@...emloft.net,
        rjw@...ysocki.net, len.brown@...el.com, pavel@....cz,
        peterz@...radead.org, eduval@...zon.com, sblbir@...zon.com,
        xen-devel@...ts.xenproject.org, vkuznets@...hat.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        dwmw@...zon.co.uk, benh@...nel.crashing.org
Subject: Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend
 mode

On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> From: Munehisa Kamata <kamatam@...zon.com>
>
> Guest hibernation is different from xen suspend/resume/live migration.
> Xen save/restore does not use pm_ops as is needed by guest hibernation.
> Hibernation in guest follows ACPI path and is guest inititated , the
> hibernation image is saved within guest as compared to later modes
> which are xen toolstack assisted and image creation/storage is in
> control of hypervisor/host machine.
> To differentiate between Xen suspend and PM hibernation, keep track
> of the on-going suspend mode by mainly using a new PM notifier.
> Introduce simple functions which help to know the on-going suspend mode
> so that other Xen-related code can behave differently according to the
> current suspend mode.
> Since Xen suspend doesn't have corresponding PM event, its main logic
> is modfied to acquire pm_mutex and set the current mode.
>
> Though, acquirng pm_mutex is still right thing to do, we may
> see deadlock if PM hibernation is interrupted by Xen suspend.
> PM hibernation depends on xenwatch thread to process xenbus state
> transactions, but the thread will sleep to wait pm_mutex which is
> already held by PM hibernation context in the scenario. Xen shutdown
> code may need some changes to avoid the issue.
>
> [Anchal Agarwal: Changelog]:
>  RFC v1->v2: Code refactoring
>  v1->v2:     Remove unused functions for PM SUSPEND/PM hibernation
>
> Signed-off-by: Anchal Agarwal <anchalag@...zon.com>
> Signed-off-by: Munehisa Kamata <kamatam@...zon.com>
> ---
>  drivers/xen/manage.c  | 60 +++++++++++++++++++++++++++++++++++++++++++
>  include/xen/xen-ops.h |  1 +
>  2 files changed, 61 insertions(+)
>
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index cd046684e0d1..69833fd6cfd1 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -14,6 +14,7 @@
>  #include <linux/freezer.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/export.h>
> +#include <linux/suspend.h>
>  
>  #include <xen/xen.h>
>  #include <xen/xenbus.h>
> @@ -40,6 +41,20 @@ enum shutdown_state {
>  /* Ignore multiple shutdown requests. */
>  static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
>  
> +enum suspend_modes {
> +	NO_SUSPEND = 0,
> +	XEN_SUSPEND,
> +	PM_HIBERNATION,
> +};
> +
> +/* Protected by pm_mutex */
> +static enum suspend_modes suspend_mode = NO_SUSPEND;
> +
> +bool xen_is_xen_suspend(void)


Weren't you going to call this pv suspend? (And also --- is this suspend
or hibernation? Your commit messages and cover letter talk about fixing
hibernation).


> +{
> +	return suspend_mode == XEN_SUSPEND;
> +}
> +



> +
> +static int xen_pm_notifier(struct notifier_block *notifier,
> +			unsigned long pm_event, void *unused)
> +{
> +	switch (pm_event) {
> +	case PM_SUSPEND_PREPARE:
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_RESTORE_PREPARE:
> +		suspend_mode = PM_HIBERNATION;


Do you ever use this mode? It seems to me all you care about is whether
or not we are doing XEN_SUSPEND. And so perhaps suspend_mode could
become a boolean. And then maybe even drop it altogether because it you
should be able to key off (shutting_down == SHUTDOWN_SUSPEND).


> +		break;
> +	case PM_POST_SUSPEND:
> +	case PM_POST_RESTORE:
> +	case PM_POST_HIBERNATION:
> +		/* Set back to the default */
> +		suspend_mode = NO_SUSPEND;
> +		break;
> +	default:
> +		pr_warn("Receive unknown PM event 0x%lx\n", pm_event);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +};



> +static int xen_setup_pm_notifier(void)
> +{
> +	if (!xen_hvm_domain())
> +		return -ENODEV;


I forgot --- what did we decide about non-x86 (i.e. ARM)?


And PVH dom0.


-boris