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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> From: Munehisa Kamata <kamatam@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Munehisa Kamata <kamatam@xxxxxxxxxx>
> ---
>  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