diff mbox series

[01/12] xen/manage: keep track of the on-going suspend mode

Message ID 20200519232451.GA18632@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com (mailing list archive)
State New, archived
Headers show
Series Fix PM hibernation in Xen guests | expand

Commit Message

Anchal Agarwal May 19, 2020, 11:24 p.m. UTC 
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 Changelog: Code refactoring]
Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
---
 drivers/xen/manage.c  | 73 +++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h |  3 ++
 2 files changed, 76 insertions(+)

Comments

Boris Ostrovsky May 30, 2020, 10:26 p.m. UTC | #1 
On 5/19/20 7:24 PM, Anchal Agarwal wrote:
>  
> +enum suspend_modes {
> +	NO_SUSPEND = 0,
> +	XEN_SUSPEND,
> +	PM_SUSPEND,
> +	PM_HIBERNATION,
> +};
> +
> +/* Protected by pm_mutex */
> +static enum suspend_modes suspend_mode = NO_SUSPEND;
> +
> +bool xen_suspend_mode_is_xen_suspend(void)
> +{
> +	return suspend_mode == XEN_SUSPEND;
> +}
> +
> +bool xen_suspend_mode_is_pm_suspend(void)
> +{
> +	return suspend_mode == PM_SUSPEND;
> +}
> +
> +bool xen_suspend_mode_is_pm_hibernation(void)
> +{
> +	return suspend_mode == PM_HIBERNATION;
> +}
> +


I don't see these last two used anywhere. Are you, in fact,
distinguishing between PM suspend and hibernation?


(I would also probably shorten the name a bit, perhaps
xen_is_pv/pm_suspend()?)


-boris
Anchal Agarwal June 1, 2020, 9 p.m. UTC | #2 
    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 5/19/20 7:24 PM, Anchal Agarwal wrote:
    >
    > +enum suspend_modes {
    > +     NO_SUSPEND = 0,
    > +     XEN_SUSPEND,
    > +     PM_SUSPEND,
    > +     PM_HIBERNATION,
    > +};
    > +
    > +/* Protected by pm_mutex */
    > +static enum suspend_modes suspend_mode = NO_SUSPEND;
    > +
    > +bool xen_suspend_mode_is_xen_suspend(void)
    > +{
    > +     return suspend_mode == XEN_SUSPEND;
    > +}
    > +
    > +bool xen_suspend_mode_is_pm_suspend(void)
    > +{
    > +     return suspend_mode == PM_SUSPEND;
    > +}
    > +
    > +bool xen_suspend_mode_is_pm_hibernation(void)
    > +{
    > +     return suspend_mode == PM_HIBERNATION;
    > +}
    > +


    I don't see these last two used anywhere. Are you, in fact,
    distinguishing between PM suspend and hibernation?

Yes, I am. Unless there is a better way to distinguish at runtime which I haven't figured out yet.
The initial design was to have separate states for separate modes. Currently, PM_HIBERNATION is handled 
by !xen_suspend . However, if any case arises where we need to set the suspend_mode, its available via 
this interface. This is basically to support PM* ops via ACPI path. Since, PM_SUSPEND is not handled by the series
the code piece can be removed and added later. Any comments?


    (I would also probably shorten the name a bit, perhaps
    xen_is_pv/pm_suspend()?)

Sure. Will fix in my next round of post.
    -boris

Thanks,
Anchal
Boris Ostrovsky June 1, 2020, 10:39 p.m. UTC | #3 
On 6/1/20 5:00 PM, Agarwal, Anchal wrote:
>    
>
>     I don't see these last two used anywhere. Are you, in fact,
>     distinguishing between PM suspend and hibernation?
>
> Yes, I am. Unless there is a better way to distinguish at runtime which I haven't figured out yet.
> The initial design was to have separate states for separate modes. Currently, PM_HIBERNATION is handled 
> by !xen_suspend . However, if any case arises where we need to set the suspend_mode, its available via 
> this interface. This is basically to support PM* ops via ACPI path. Since, PM_SUSPEND is not handled by the series
> the code piece can be removed and added later. Any comments?


Yes, if this is not being handled then I don't see any reason for this
code to be there.


-boris
diff mbox series

Patch

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index cd046684e0d1..0b30ab522b77 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,31 @@  enum shutdown_state {
 /* Ignore multiple shutdown requests. */
 static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 
+enum suspend_modes {
+	NO_SUSPEND = 0,
+	XEN_SUSPEND,
+	PM_SUSPEND,
+	PM_HIBERNATION,
+};
+
+/* Protected by pm_mutex */
+static enum suspend_modes suspend_mode = NO_SUSPEND;
+
+bool xen_suspend_mode_is_xen_suspend(void)
+{
+	return suspend_mode == XEN_SUSPEND;
+}
+
+bool xen_suspend_mode_is_pm_suspend(void)
+{
+	return suspend_mode == PM_SUSPEND;
+}
+
+bool xen_suspend_mode_is_pm_hibernation(void)
+{
+	return suspend_mode == PM_HIBERNATION;
+}
+
 struct suspend_info {
 	int cancelled;
 };
@@ -99,6 +125,10 @@  static void do_suspend(void)
 	int err;
 	struct suspend_info si;
 
+	lock_system_sleep();
+
+	suspend_mode = XEN_SUSPEND;
+
 	shutting_down = SHUTDOWN_SUSPEND;
 
 	err = freeze_processes();
@@ -162,6 +192,10 @@  static void do_suspend(void)
 	thaw_processes();
 out:
 	shutting_down = SHUTDOWN_INVALID;
+
+	suspend_mode = NO_SUSPEND;
+
+	unlock_system_sleep();
 }
 #endif	/* CONFIG_HIBERNATE_CALLBACKS */
 
@@ -387,3 +421,42 @@  int xen_setup_shutdown_event(void)
 EXPORT_SYMBOL_GPL(xen_setup_shutdown_event);
 
 subsys_initcall(xen_setup_shutdown_event);
+
+static int xen_pm_notifier(struct notifier_block *notifier,
+			   unsigned long pm_event, void *unused)
+{
+	switch (pm_event) {
+	case PM_SUSPEND_PREPARE:
+		suspend_mode = PM_SUSPEND;
+		break;
+	case PM_HIBERNATION_PREPARE:
+	case PM_RESTORE_PREPARE:
+		suspend_mode = PM_HIBERNATION;
+		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 struct notifier_block xen_pm_notifier_block = {
+	.notifier_call = xen_pm_notifier
+};
+
+static int xen_setup_pm_notifier(void)
+{
+	if (!xen_hvm_domain())
+		return -ENODEV;
+
+	return register_pm_notifier(&xen_pm_notifier_block);
+}
+
+subsys_initcall(xen_setup_pm_notifier);
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 095be1d66f31..4ffe031adfc7 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -40,6 +40,9 @@  u64 xen_steal_clock(int cpu);
 
 int xen_setup_shutdown_event(void);
 
+bool xen_suspend_mode_is_xen_suspend(void);
+bool xen_suspend_mode_is_pm_suspend(void);
+bool xen_suspend_mode_is_pm_hibernation(void);
 extern unsigned long *xen_contiguous_bitmap;
 
 #if defined(CONFIG_XEN_PV) || defined(CONFIG_ARM) || defined(CONFIG_ARM64)