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

[Anchal Agarwal <anchalag@amazon.com>]

On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> > From: Munehisa Kamata <kamatam@amazon.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@amazon.com>
> > Signed-off-by: Munehisa Kamata <kamatam@amazon.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).
> 
> 
This is for hibernation is for pvhvm/hvm/pv-on-hvm guests as you may call it.
The method is just there to check if "xen suspend" is in progress.
I do not see "xen_suspend" differentiating between pv or hvm
domain until later in the code hence, I abstracted it to xen_is_xen_suspend.
> > +{
> > +     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).
> 
> 
The mode was left there in case its needed for restore prepare cases. But you
are right the only thing I currently care about whether shutting_down ==
SHUTDOWN_SUSPEND. Infact, the notifier may not be needed in first place.
xen_is_xen_suspend could work right off the bat using 'shutting_down' variable
itself. *I think so* I will test it on my end and send an updated patch.
> > +             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)?
It would be great to support that however, its  out of
scope for this patch set.
I’ll be happy to discuss it separately.
> 
> 
> And PVH dom0.
That's another good use case to make it work with however, I still
think that should be tested/worked upon separately as the feature itself
(PVH Dom0) is very new.
> 
> 
Thanks,
Anchal
> -boris
> 
> 
>