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@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).
>
>
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
>
>
>
