Fix PM hibernation in Xen guests

From: Munehisa Kamata <[email protected]>

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 <[email protected]>

Signed-off-by Munehisa Kamata <[email protected]>

---
 drivers/xen/manage.c  | 60 +++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h |  1 +
 2 files changed, 61 insertions(+)

drivers/xen/manage.c +1 -0

index cd046684e0d1..69833fd6cfd1 100644
@@ -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;
 

drivers/xen/manage.c +4 -0

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

drivers/xen/manage.c +11 -0

+		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;
+	}

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.

Quoted 50 lines · Anchal Agarwal

From: Munehisa Kamata <[email protected]>

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 <[email protected]>

Signed-off-by Munehisa Kamata <[email protected]>

---
 drivers/xen/manage.c  | 60 +++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h |  1 +
 2 files changed, 61 insertions(+)

drivers/xen/manage.c +1 -0

index cd046684e0d1..69833fd6cfd1 100644
@@ -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;
 

drivers/xen/manage.c enum shutdown_state { · line 53

Quoted 10 lines

drivers/xen/manage.c +10 -0

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

Quoted 4 lines

drivers/xen/manage.c +4 -0

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

drivers/xen/manage.c int xen_setup_shutdown_event(void) · line 421

Quoted 9 lines

drivers/xen/manage.c +9 -0

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

Quoted 14 lines

drivers/xen/manage.c +14 -0

+             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;
+};

drivers/xen/manage.c int xen_setup_shutdown_event(void) · line 444

Quoted 4 lines

drivers/xen/manage.c +4 -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

Quoted 4 lines · Boris Ostrovsky

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.

Quoted 9 lines

Review

Quoted 2 lines · Anchal Agarwal

(quoted diff) +2 -0

+
+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.

Quoted 10 lines

Quoted 4 lines

drivers/xen/manage.c +4 -0

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

drivers/xen/manage.c +4 -0

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

Quoted 1 line

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.

Quoted 2 lines

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.

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.

Quoted 14 lines · Anchal Agarwal

Quoted 4 lines · Boris Ostrovsky

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.

Quoted 9 lines

Review

Quoted 2 lines · Anchal Agarwal

(quoted diff) +2 -0

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

Quoted 4 lines

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.

I meant "pv suspend" in the sense that this is paravirtual suspend, not suspend for paravirtual guests. Just like pv drivers are for both pv and hvm guests.

And then --- should it be pv suspend or pv hibernation?

Quoted 11 lines

Quoted 11 lines

Quoted 5 lines

drivers/xen/manage.c +4 -0

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

drivers/xen/manage.c +4 -0

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

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 6 lines

Quoted 3 lines

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.

Same question here --- will this break PVH dom0?

-boris

Quoted 23 lines · Boris Ostrovsky

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.

Quoted 13 lines · Anchal Agarwal

Quoted 4 lines · Boris Ostrovsky

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.

Quoted 8 lines

Review

Quoted 2 lines · Anchal Agarwal

(quoted diff) +2 -0

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

Quoted 4 lines

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.

Quoted 8 lines

I meant "pv suspend" in the sense that this is paravirtual suspend, not suspend for paravirtual guests. Just like pv drivers are for both pv and hvm guests.

And then --- should it be pv suspend or pv hibernation?

Ok so I think I am lot confused by this question. Here is what this function for, function xen_is_xen_suspend() just tells us whether the guest is in "SHUTDOWN_SUSPEND" state or not. This check is needed for correct invocation of syscore_ops callbacks registered for guest's hibernation and for xenbus to invoke respective callbacks[suspend/resume vs freeze/thaw/restore].
Since "shutting_down" state is defined static and is not directly available to other parts of the code, the function solves the purpose.

I am having hard time understanding why this should be called pv suspend/hibernation unless you are suggesting something else? Am I missing your point here?

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

drivers/xen/manage.c +4 -0

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

drivers/xen/manage.c +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Same question here --- will this break PVH dom0?

I haven't tested it as a part of this series. Is that a blocker here?

(Roger, question for you at the very end)

Quoted 19 lines · Anchal Agarwal

Quoted 18 lines · Boris Ostrovsky

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.

Quoted 10 lines · Anchal Agarwal

Quoted 6 lines · Boris Ostrovsky

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:
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.

Same question here --- will this break PVH dom0?

Quoted 1 line

I haven't tested it as a part of this series. Is that a blocker here?

I suspect dom0 will not do well now as far as hibernation goes, in which case you are not breaking anything.

Roger?

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.

Quoted 22 lines · Boris Ostrovsky

(Roger, question for you at the very end)

Quoted 19 lines · Anchal Agarwal

Quoted 18 lines · Boris Ostrovsky

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.

Quoted 10 lines · Anchal Agarwal

Quoted 6 lines · Boris Ostrovsky

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:
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.

Same question here --- will this break PVH dom0?

Quoted 8 lines

Quoted 1 line

I haven't tested it as a part of this series. Is that a blocker here?

I suspect dom0 will not do well now as far as hibernation goes, in which case you are not breaking anything.

Roger?

I sadly don't have any box ATM that supports hibernation where I could test it. We have hibernation support for PV dom0, so while I haven't done anything specific to support or test hibernation on PVH dom0 I would at least aim to not make this any worse, and hence the check should at least also fail for a PVH dom0?

if (!xen_hvm_domain() || xen_initial_domain())

    return -ENODEV;

Ie: none of this should be applied to a PVH dom0, as it doesn't have PV devices and hence should follow the bare metal device suspend.

Also I would contact the QubesOS guys, they rely heavily on the suspend feature for dom0, and that's something not currently tested by osstest so any breakages there go unnoticed.

Thanks, Roger.

Quoted 42 lines · Roger Pau Monné

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.

Quoted 22 lines · Boris Ostrovsky

(Roger, question for you at the very end)

Quoted 19 lines · Anchal Agarwal

Quoted 18 lines · Boris Ostrovsky

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.

Quoted 10 lines · Anchal Agarwal

Quoted 6 lines · Boris Ostrovsky

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:
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.

Same question here --- will this break PVH dom0?

Quoted 8 lines

Quoted 1 line

I haven't tested it as a part of this series. Is that a blocker here?

I suspect dom0 will not do well now as far as hibernation goes, in which case you are not breaking anything.

Roger?

I sadly don't have any box ATM that supports hibernation where I could test it. We have hibernation support for PV dom0, so while I haven't done anything specific to support or test hibernation on PVH dom0 I would at least aim to not make this any worse, and hence the check should at least also fail for a PVH dom0?

Quoted 6 lines

if (!xen_hvm_domain() || xen_initial_domain())

    return -ENODEV;

Ie: none of this should be applied to a PVH dom0, as it doesn't have PV devices and hence should follow the bare metal device suspend.

So from what I understand you meant for any guest running on pvh dom0 should not hibernate if hibernation is triggered from within the guest or should they?

Quoted 4 lines

Also I would contact the QubesOS guys, they rely heavily on the suspend feature for dom0, and that's something not currently tested by osstest so any breakages there go unnoticed.

Was this for me or Boris? If its the former then I have no idea how to?

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.

Marek: I'm adding you in case you could be able to give this a try and make sure it doesn't break suspend for dom0.

Quoted 43 lines · Anchal Agarwal

Quoted 42 lines · Roger Pau Monné

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.

Quoted 22 lines · Boris Ostrovsky

(Roger, question for you at the very end)

Quoted 19 lines · Anchal Agarwal

Quoted 18 lines · Boris Ostrovsky

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.

Quoted 10 lines · Anchal Agarwal

Quoted 6 lines · Boris Ostrovsky

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:
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.

Same question here --- will this break PVH dom0?

Quoted 8 lines

Quoted 1 line

I haven't tested it as a part of this series. Is that a blocker here?

I suspect dom0 will not do well now as far as hibernation goes, in which case you are not breaking anything.

Roger?

I sadly don't have any box ATM that supports hibernation where I could test it. We have hibernation support for PV dom0, so while I haven't done anything specific to support or test hibernation on PVH dom0 I would at least aim to not make this any worse, and hence the check should at least also fail for a PVH dom0?

Quoted 8 lines

Quoted 6 lines

if (!xen_hvm_domain() || xen_initial_domain())

    return -ENODEV;

Ie: none of this should be applied to a PVH dom0, as it doesn't have PV devices and hence should follow the bare metal device suspend.

So from what I understand you meant for any guest running on pvh dom0 should not hibernate if hibernation is triggered from within the guest or should they?

Er no to both I think. What I meant is that a PVH dom0 should be able to properly suspend, and we should make sure this work doesn't make this any harder (or breaks it if it's currently working).

Or at least that's how I understood the question raised by Boris.

You are adding code to the generic suspend path that's also used by dom0 in order to perform bare metal suspension. This is fine now for a PV dom0 because the code is gated on xen_hvm_domain, but you should also take into account that a PVH dom0 is considered a HVM domain, and hence will get the notifier registered.

Quoted 5 lines

Quoted 4 lines

Also I would contact the QubesOS guys, they rely heavily on the suspend feature for dom0, and that's something not currently tested by osstest so any breakages there go unnoticed.

Was this for me or Boris? If its the former then I have no idea how to?

I've now added Marek.

Roger.

Quoted 64 lines · Roger Pau Monné

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.

Marek: I'm adding you in case you could be able to give this a try and make sure it doesn't break suspend for dom0.

Quoted 43 lines · Anchal Agarwal

Quoted 42 lines · Roger Pau Monné

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.

Quoted 22 lines · Boris Ostrovsky

(Roger, question for you at the very end)

Quoted 19 lines · Anchal Agarwal

Quoted 18 lines · Boris Ostrovsky

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.

Quoted 10 lines · Anchal Agarwal

Quoted 6 lines · Boris Ostrovsky

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:
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.

Same question here --- will this break PVH dom0?

Quoted 8 lines

Quoted 1 line

I haven't tested it as a part of this series. Is that a blocker here?

I suspect dom0 will not do well now as far as hibernation goes, in which case you are not breaking anything.

Roger?

I sadly don't have any box ATM that supports hibernation where I could test it. We have hibernation support for PV dom0, so while I haven't done anything specific to support or test hibernation on PVH dom0 I would at least aim to not make this any worse, and hence the check should at least also fail for a PVH dom0?

Quoted 8 lines

Quoted 6 lines

if (!xen_hvm_domain() || xen_initial_domain())

    return -ENODEV;

Ie: none of this should be applied to a PVH dom0, as it doesn't have PV devices and hence should follow the bare metal device suspend.

So from what I understand you meant for any guest running on pvh dom0 should not hibernate if hibernation is triggered from within the guest or should they?

Er no to both I think. What I meant is that a PVH dom0 should be able to properly suspend, and we should make sure this work doesn't make this any harder (or breaks it if it's currently working).

Quoted 8 lines

Or at least that's how I understood the question raised by Boris.

You are adding code to the generic suspend path that's also used by dom0 in order to perform bare metal suspension. This is fine now for a PV dom0 because the code is gated on xen_hvm_domain, but you should also take into account that a PVH dom0 is considered a HVM domain, and hence will get the notifier registered.

Ok that makes sense now. This is good to be safe, but my patch series is only to support domU hibernation, so I am not sure if this will affect pvh dom0. However, since I do not have a good way of testing sure I will add the check.

Moreover, in Patch-0004, I do register suspend/resume syscore_ops specifically for domU hibernation only if its xen_hvm_domain.

I don't see any reason that should not
be registered for domU's running on pvh dom0.

Those suspend/resume callbacks will
only be invoked in case hibernation and will be skipped if generic suspend path is in progress. Do you see any issue with that?

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.

(Roger, question for you at the very end)

Quoted 41 lines · Anchal Agarwal

Quoted 23 lines · Boris Ostrovsky

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.

Quoted 13 lines · Anchal Agarwal

Quoted 4 lines · Boris Ostrovsky

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.

Quoted 8 lines

Review

Quoted 2 lines · Anchal Agarwal

(quoted diff) +2 -0

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

Quoted 4 lines

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.

Quoted 8 lines

I meant "pv suspend" in the sense that this is paravirtual suspend, not suspend for paravirtual guests. Just like pv drivers are for both pv and hvm guests.

And then --- should it be pv suspend or pv hibernation?

Ok so I think I am lot confused by this question. Here is what this function for, function xen_is_xen_suspend() just tells us whether the guest is in "SHUTDOWN_SUSPEND" state or not. This check is needed for correct invocation of syscore_ops callbacks registered for guest's hibernation and for xenbus to invoke respective callbacks[suspend/resume vs freeze/thaw/restore].
Since "shutting_down" state is defined static and is not directly available to other parts of the code, the function solves the purpose.

Quoted 3 lines

I am having hard time understanding why this should be called pv suspend/hibernation unless you are suggesting something else? Am I missing your point here?

I think I understand now what you are trying to say --- it's whether we are going to use xen_suspend() routine, right? If that's the case then sure, you can use "xen_suspend" term. (I'd probably still change xen_is_xen_suspend() to is_xen_suspend())

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

drivers/xen/manage.c +4 -0

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

drivers/xen/manage.c +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 7 lines

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

I don't know what would happen if ARM guest tries to handle hibernation callbacks. The only ones that you are introducing are in block and net fronts and that's arch-independent.

You do add a bunch of x86-specific code though (syscore ops), would something similar be needed for ARM?

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Same question here --- will this break PVH dom0?

I haven't tested it as a part of this series. Is that a blocker here?

I suspect dom0 will not do well now as far as hibernation goes, in which case you are not breaking anything.

Roger?

-boris

Quoted 24 lines

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 7 lines

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

Quoted 9 lines

I don't know what would happen if ARM guest tries to handle hibernation callbacks. The only ones that you are introducing are in block and net fronts and that's arch-independent.

You do add a bunch of x86-specific code though (syscore ops), would something similar be needed for ARM?

I don't expect this to work out of the box on ARM. To start with something similar will be needed for ARM too.
We may still want to keep the driver code as-is.

I understand the concern here wrt ARM, however, currently the support is only proposed for x86 guests here and similar work could be carried out for ARM. Also, if regular hibernation works correctly on arm, then all is needed is to fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as this series is concerned.

Quoted 36 lines

Quoted 23 lines

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 7 lines

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

Quoted 9 lines

I don't know what would happen if ARM guest tries to handle hibernation callbacks. The only ones that you are introducing are in block and net fronts and that's arch-independent.

You do add a bunch of x86-specific code though (syscore ops), would something similar be needed for ARM?

I don't expect this to work out of the box on ARM. To start with something similar will be needed for ARM too.
We may still want to keep the driver code as-is.

Quoted 7 lines

I understand the concern here wrt ARM, however, currently the support is only proposed for x86 guests here and similar work could be carried out for ARM. Also, if regular hibernation works correctly on arm, then all is needed is to fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as this series is concerned.

If you are not sure what the effects are (or sure that it won't work) on ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.

if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())

	return -ENODEV;

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.

Quoted 36 lines · Boris Ostrovsky

Quoted 36 lines

Quoted 23 lines

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 7 lines

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

Quoted 9 lines

I don't know what would happen if ARM guest tries to handle hibernation callbacks. The only ones that you are introducing are in block and net fronts and that's arch-independent.

You do add a bunch of x86-specific code though (syscore ops), would something similar be needed for ARM?

I don't expect this to work out of the box on ARM. To start with something similar will be needed for ARM too.
We may still want to keep the driver code as-is.

Quoted 7 lines

Quoted 7 lines

I understand the concern here wrt ARM, however, currently the support is only proposed for x86 guests here and similar work could be carried out for ARM. Also, if regular hibernation works correctly on arm, then all is needed is to fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as this series is concerned.

Just to clarify: new features don't need to work on ARM or cause any addition efforts to you to make them work on ARM. The patch series only needs not to break existing code paths (on ARM and any other platforms). It should also not make it overly difficult to implement the ARM side of things (if there is one) at some point in the future.

FYI drivers/xen/manage.c is compiled and working on ARM today, however Xen suspend/resume is not supported. I don't know for sure if guest-initiated hibernation works because I have not tested it.

Quoted 4 lines

If you are not sure what the effects are (or sure that it won't work) on ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.

Quoted 2 lines

if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())

      return -ENODEV;

That is a good principle to have and thanks for suggesting it. However, in this specific case there is nothing in this patch that doesn't work on ARM. From an ARM perspective I think we should enable it and &xen_pm_notifier_block should be registered.

Given that all guests are HVM guests on ARM, it should work fine as is.

I gave a quick look at the rest of the series and everything looks fine to me from an ARM perspective. I cannot imaging that the new freeze, thaw, and restore callbacks for net and block are going to cause any trouble on ARM. The two main x86-specific functions are xen_syscore_suspend/resume and they look trivial to implement on ARM (in the sense that they are likely going to look exactly the same.)

One question for Anchal: what's going to happen if you trigger a hibernation, you have the new callbacks, but you are missing xen_syscore_suspend/resume?

Is it any worse than not having the new freeze, thaw and restore callbacks at all and try to do a hibernation?

Quoted 6 lines · Stefano Stabellini

Quoted 6 lines

If you are not sure what the effects are (or sure that it won't work) on ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.

Quoted 7 lines

Quoted 2 lines

if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())

      return -ENODEV;

That is a good principle to have and thanks for suggesting it. However, in this specific case there is nothing in this patch that doesn't work on ARM. From an ARM perspective I think we should enable it and &xen_pm_notifier_block should be registered.

This question is for Boris, I think you we decided to get rid of the notifier in V3 as all we need to check is SHUTDOWN_SUSPEND state which sounds plausible to me. So this check may go away. It may still be needed for sycore_ops callbacks registration.

Quoted 10 lines

Given that all guests are HVM guests on ARM, it should work fine as is.

I gave a quick look at the rest of the series and everything looks fine to me from an ARM perspective. I cannot imaging that the new freeze, thaw, and restore callbacks for net and block are going to cause any trouble on ARM. The two main x86-specific functions are xen_syscore_suspend/resume and they look trivial to implement on ARM (in the sense that they are likely going to look exactly the same.)

Yes but for now since things are not tested I will put this !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe and not break anything.

Quoted 6 lines

One question for Anchal: what's going to happen if you trigger a hibernation, you have the new callbacks, but you are missing xen_syscore_suspend/resume?

Is it any worse than not having the new freeze, thaw and restore callbacks at all and try to do a hibernation?

If callbacks are not there, I don't expect hibernation to work correctly. These callbacks takes care of xen primitives like shared_info_page, grant table, sched clock, runstate time which are important to save the correct state of the guest and bring it back up. Other patches in the series, adds all the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver level.

Thanks,
Anchal

Quoted 61 lines · Stefano Stabellini

Quoted 36 lines · Boris Ostrovsky

Quoted 36 lines

Quoted 23 lines

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 7 lines

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

Quoted 9 lines

I don't know what would happen if ARM guest tries to handle hibernation callbacks. The only ones that you are introducing are in block and net fronts and that's arch-independent.

You do add a bunch of x86-specific code though (syscore ops), would something similar be needed for ARM?

I don't expect this to work out of the box on ARM. To start with something similar will be needed for ARM too.
We may still want to keep the driver code as-is.

Quoted 7 lines

Quoted 7 lines

I understand the concern here wrt ARM, however, currently the support is only proposed for x86 guests here and similar work could be carried out for ARM. Also, if regular hibernation works correctly on arm, then all is needed is to fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as this series is concerned.

Just to clarify: new features don't need to work on ARM or cause any addition efforts to you to make them work on ARM. The patch series only needs not to break existing code paths (on ARM and any other platforms). It should also not make it overly difficult to implement the ARM side of things (if there is one) at some point in the future.

FYI drivers/xen/manage.c is compiled and working on ARM today, however Xen suspend/resume is not supported. I don't know for sure if guest-initiated hibernation works because I have not tested it.

Quoted 4 lines

If you are not sure what the effects are (or sure that it won't work) on ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.

Quoted 8 lines

Quoted 2 lines

if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())

      return -ENODEV;

That is a good principle to have and thanks for suggesting it. However, in this specific case there is nothing in this patch that doesn't work on ARM. From an ARM perspective I think we should enable it and &xen_pm_notifier_block should be registered.

This question is for Boris, I think you we decided to get rid of the notifier in V3 as all we need to check is SHUTDOWN_SUSPEND state which sounds plausible to me. So this check may go away. It may still be needed for sycore_ops callbacks registration.

Quoted 10 lines

Given that all guests are HVM guests on ARM, it should work fine as is.

I gave a quick look at the rest of the series and everything looks fine to me from an ARM perspective. I cannot imaging that the new freeze, thaw, and restore callbacks for net and block are going to cause any trouble on ARM. The two main x86-specific functions are xen_syscore_suspend/resume and they look trivial to implement on ARM (in the sense that they are likely going to look exactly the same.)

Yes but for now since things are not tested I will put this !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe and not break anything.

Quoted 5 lines

One question for Anchal: what's going to happen if you trigger a hibernation, you have the new callbacks, but you are missing xen_syscore_suspend/resume?

Quoted 2 lines

Is it any worse than not having the new freeze, thaw and restore callbacks at all and try to do a hibernation?

If callbacks are not there, I don't expect hibernation to work correctly. These callbacks takes care of xen primitives like shared_info_page, grant table, sched clock, runstate time which are important to save the correct state of the guest and bring it back up. Other patches in the series, adds all the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver level.

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.

Quoted 92 lines · Anchal Agarwal

Quoted 61 lines · Stefano Stabellini

Quoted 36 lines · Boris Ostrovsky

Quoted 36 lines

Quoted 23 lines

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 7 lines

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

Quoted 9 lines

I don't know what would happen if ARM guest tries to handle hibernation callbacks. The only ones that you are introducing are in block and net fronts and that's arch-independent.

You do add a bunch of x86-specific code though (syscore ops), would something similar be needed for ARM?

I don't expect this to work out of the box on ARM. To start with something similar will be needed for ARM too.
We may still want to keep the driver code as-is.

Quoted 7 lines

Quoted 7 lines

I understand the concern here wrt ARM, however, currently the support is only proposed for x86 guests here and similar work could be carried out for ARM. Also, if regular hibernation works correctly on arm, then all is needed is to fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as this series is concerned.

Just to clarify: new features don't need to work on ARM or cause any addition efforts to you to make them work on ARM. The patch series only needs not to break existing code paths (on ARM and any other platforms). It should also not make it overly difficult to implement the ARM side of things (if there is one) at some point in the future.

FYI drivers/xen/manage.c is compiled and working on ARM today, however Xen suspend/resume is not supported. I don't know for sure if guest-initiated hibernation works because I have not tested it.

Quoted 4 lines

If you are not sure what the effects are (or sure that it won't work) on ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.

Quoted 8 lines

Quoted 2 lines

if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())

      return -ENODEV;

That is a good principle to have and thanks for suggesting it. However, in this specific case there is nothing in this patch that doesn't work on ARM. From an ARM perspective I think we should enable it and &xen_pm_notifier_block should be registered.

This question is for Boris, I think you we decided to get rid of the notifier in V3 as all we need to check is SHUTDOWN_SUSPEND state which sounds plausible to me. So this check may go away. It may still be needed for sycore_ops callbacks registration.

Quoted 10 lines

Given that all guests are HVM guests on ARM, it should work fine as is.

I gave a quick look at the rest of the series and everything looks fine to me from an ARM perspective. I cannot imaging that the new freeze, thaw, and restore callbacks for net and block are going to cause any trouble on ARM. The two main x86-specific functions are xen_syscore_suspend/resume and they look trivial to implement on ARM (in the sense that they are likely going to look exactly the same.)

Yes but for now since things are not tested I will put this !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe and not break anything.

Quoted 5 lines

One question for Anchal: what's going to happen if you trigger a hibernation, you have the new callbacks, but you are missing xen_syscore_suspend/resume?

Quoted 8 lines

Quoted 2 lines

Is it any worse than not having the new freeze, thaw and restore callbacks at all and try to do a hibernation?

If callbacks are not there, I don't expect hibernation to work correctly. These callbacks takes care of xen primitives like shared_info_page, grant table, sched clock, runstate time which are important to save the correct state of the guest and bring it back up. Other patches in the series, adds all the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver level.

I meant the other way around :-) Let me rephrase the question.

Do you think that implementing freeze/thaw/restore at the driver level without having xen_syscore_suspend/resume can potentially make things worse compared to not having freeze/thaw/restore at the driver level at all?

Quoted 106 lines · Stefano Stabellini

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.

Quoted 92 lines · Anchal Agarwal

Quoted 61 lines · Stefano Stabellini

Quoted 36 lines · Boris Ostrovsky

Quoted 36 lines

Quoted 23 lines

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +4 -0

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

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

I wasn't implying that this *should* work on ARM but rather whether this will break ARM somehow (because xen_hvm_domain() is true there).

Quoted 7 lines

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM events PM SUSPEND and PM hibernation. Now since we only care about PM HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state. However, I may have to fix other patches in the series where this check may appear and cater it only for x86 right?

Quoted 9 lines

I don't know what would happen if ARM guest tries to handle hibernation callbacks. The only ones that you are introducing are in block and net fronts and that's arch-independent.

You do add a bunch of x86-specific code though (syscore ops), would something similar be needed for ARM?

I don't expect this to work out of the box on ARM. To start with something similar will be needed for ARM too.
We may still want to keep the driver code as-is.

Quoted 7 lines

Quoted 7 lines

I understand the concern here wrt ARM, however, currently the support is only proposed for x86 guests here and similar work could be carried out for ARM. Also, if regular hibernation works correctly on arm, then all is needed is to fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as this series is concerned.

Just to clarify: new features don't need to work on ARM or cause any addition efforts to you to make them work on ARM. The patch series only needs not to break existing code paths (on ARM and any other platforms). It should also not make it overly difficult to implement the ARM side of things (if there is one) at some point in the future.

FYI drivers/xen/manage.c is compiled and working on ARM today, however Xen suspend/resume is not supported. I don't know for sure if guest-initiated hibernation works because I have not tested it.

Quoted 4 lines

If you are not sure what the effects are (or sure that it won't work) on ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.

Quoted 8 lines

Quoted 2 lines

if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())

      return -ENODEV;

That is a good principle to have and thanks for suggesting it. However, in this specific case there is nothing in this patch that doesn't work on ARM. From an ARM perspective I think we should enable it and &xen_pm_notifier_block should be registered.

This question is for Boris, I think you we decided to get rid of the notifier in V3 as all we need to check is SHUTDOWN_SUSPEND state which sounds plausible to me. So this check may go away. It may still be needed for sycore_ops callbacks registration.

Quoted 10 lines

Given that all guests are HVM guests on ARM, it should work fine as is.

I gave a quick look at the rest of the series and everything looks fine to me from an ARM perspective. I cannot imaging that the new freeze, thaw, and restore callbacks for net and block are going to cause any trouble on ARM. The two main x86-specific functions are xen_syscore_suspend/resume and they look trivial to implement on ARM (in the sense that they are likely going to look exactly the same.)

Yes but for now since things are not tested I will put this !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe and not break anything.

Quoted 5 lines

One question for Anchal: what's going to happen if you trigger a hibernation, you have the new callbacks, but you are missing xen_syscore_suspend/resume?

Quoted 8 lines

Quoted 2 lines

Is it any worse than not having the new freeze, thaw and restore callbacks at all and try to do a hibernation?

If callbacks are not there, I don't expect hibernation to work correctly. These callbacks takes care of xen primitives like shared_info_page, grant table, sched clock, runstate time which are important to save the correct state of the guest and bring it back up. Other patches in the series, adds all the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver level.

Quoted 6 lines

I meant the other way around :-) Let me rephrase the question.

Do you think that implementing freeze/thaw/restore at the driver level without having xen_syscore_suspend/resume can potentially make things worse compared to not having freeze/thaw/restore at the driver level at all?

I think in both the cases I don't expect it to work. System may end up in different state if you register vs not. Hibernation does not work properly at least for domU instances without these changes on x86 and I am assuming the same for ARM.

If you do not register freeze/thaw/restore callbacks for arm, then on invocation of xenbus_dev_suspend, default suspend/resume callbacks
will be called for each driver and since you do not have any code to save domU's xen primitives state (syscore_ops), hibernation will either fail or will demand a reboot. I do no have setup to test the current state of ARM's hibernation

If you only register freeze/thaw/restore and no syscore_ops, it will again fail. Since, I do not have an ARM setup running, I quickly ran a similar test on x86, may not be an apple to apple comparison but instance failed to resume or I should say stuck showing huge jump in time and required a reboot.

Now if this doesn't happen currently when you trigger hibernation on arm domU instances or if system is still alive when you trigger hibernation in xen guest then not registering the callbacks may be a better idea. In that case may be I need to put arch specific check when registering freeze/thaw/restore handlers.

Hope that answers your question.

Yes, it does, thank you. I'd rather not introduce unknown regressions so I would recommend to add an arch-specific check on registering freeze/thaw/restore handlers. Maybe something like the following:

#ifdef CONFIG_X86
.freeze = blkfront_freeze,
.thaw = blkfront_restore,
.restore = blkfront_restore
#endif

maybe Boris has a better suggestion on how to do it

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.

Quoted 11 lines · Stefano Stabellini

Yes, it does, thank you. I'd rather not introduce unknown regressions so I would recommend to add an arch-specific check on registering freeze/thaw/restore handlers. Maybe something like the following:

#ifdef CONFIG_X86
.freeze = blkfront_freeze,
.thaw = blkfront_restore,
.restore = blkfront_restore
#endif

Quoted 1 line

maybe Boris has a better suggestion on how to do it

An alternative might be to still install pm notifier in drivers/xen/manage.c (I think as result of latest discussions we decided we won't need it) and return -ENOTSUPP for ARM for PM_HIBERNATION_PREPARE and friends. Would that work?

-boris

Quoted 16 lines · Boris Ostrovsky

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.

Quoted 11 lines · Stefano Stabellini

Yes, it does, thank you. I'd rather not introduce unknown regressions so I would recommend to add an arch-specific check on registering freeze/thaw/restore handlers. Maybe something like the following:

#ifdef CONFIG_X86
.freeze = blkfront_freeze,
.thaw = blkfront_restore,
.restore = blkfront_restore
#endif

Quoted 7 lines

Quoted 1 line

maybe Boris has a better suggestion on how to do it

An alternative might be to still install pm notifier in drivers/xen/manage.c (I think as result of latest discussions we decided we won't need it) and return -ENOTSUPP for ARM for PM_HIBERNATION_PREPARE and friends. Would that work?

I think the question here is for registering driver specific freeze/thaw/restore callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending testing. So I think just registering driver specific callbacks for x86 only is a good option. What do you think?

Quoted 24 lines · Anchal Agarwal

Quoted 16 lines · Boris Ostrovsky

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.

Quoted 11 lines · Stefano Stabellini

Yes, it does, thank you. I'd rather not introduce unknown regressions so I would recommend to add an arch-specific check on registering freeze/thaw/restore handlers. Maybe something like the following:

#ifdef CONFIG_X86
.freeze = blkfront_freeze,
.thaw = blkfront_restore,
.restore = blkfront_restore
#endif

Quoted 7 lines

Quoted 1 line

maybe Boris has a better suggestion on how to do it

An alternative might be to still install pm notifier in drivers/xen/manage.c (I think as result of latest discussions we decided we won't need it) and return -ENOTSUPP for ARM for PM_HIBERNATION_PREPARE and friends. Would that work?

Quoted 4 lines

I think the question here is for registering driver specific freeze/thaw/restore callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending testing. So I think just registering driver specific callbacks for x86 only is a good option. What do you think?

I suggested using the notifier under assumption that if it returns an error then that will prevent callbacks to be called because hibernation will be effectively disabled.

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.

Quoted 24 lines · Anchal Agarwal

Quoted 16 lines · Boris Ostrovsky

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.

Quoted 11 lines · Stefano Stabellini

Yes, it does, thank you. I'd rather not introduce unknown regressions so I would recommend to add an arch-specific check on registering freeze/thaw/restore handlers. Maybe something like the following:

#ifdef CONFIG_X86
.freeze = blkfront_freeze,
.thaw = blkfront_restore,
.restore = blkfront_restore
#endif

Quoted 7 lines

Quoted 1 line

maybe Boris has a better suggestion on how to do it

An alternative might be to still install pm notifier in drivers/xen/manage.c (I think as result of latest discussions we decided we won't need it) and return -ENOTSUPP for ARM for PM_HIBERNATION_PREPARE and friends. Would that work?

Quoted 4 lines

I think the question here is for registering driver specific freeze/thaw/restore callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending testing. So I think just registering driver specific callbacks for x86 only is a good option. What do you think?

I suggested using the notifier under assumption that if it returns an error then that will prevent callbacks to be called because hibernation will be effectively disabled. But I haven't looked at PM code so I don't know whether this is actually the case.

The advantage of doing it in the notifier is that instead of adding ifdefs to each driver you will be able to prevent callbacks from a single place. Plus you can use this do disable hibernation for PVH dom0 as well.

-boris

I think this could be done. PM_HIBERNATION_PREPARE could return -ENOTSUPP for arm and pvh dom0 when the notifier call chain is invoked for this case in hibernate(). This will then be an empty notifier just for checking two usecases.
Also, for pvh dom0, the earlier code didn't register any notifier, with this approach you are suggesting setup the notifier for hvm/pvh dom0 and arm but fail during notifier call chain during PM_HIBERNATION_PREPARE ?

I think still getting rid of suspend mode that was earlier a part of this notifier is a good idea as it seems redundant as you pointed out earlier.

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.

Quoted 8 lines · Anchal Agarwal

I think this could be done. PM_HIBERNATION_PREPARE could return -ENOTSUPP for arm and pvh dom0 when the notifier call chain is invoked for this case in hibernate(). This will then be an empty notifier just for checking two usecases.
Also, for pvh dom0, the earlier code didn't register any notifier, with this approach you are suggesting setup the notifier for hvm/pvh dom0 and arm but fail during notifier call chain during PM_HIBERNATION_PREPARE ?

Right.

(Although the earlier code did register the notifier:
xen_setup_pm_notifier() would return an error for !xen_hvm_domain() and PVH *is* an HVM domain, so registration would actually happen)

Quoted 4 lines

I think still getting rid of suspend mode that was earlier a part of this notifier is a good idea as it seems redundant as you pointed out earlier.

Yes.

-boris

Gentle ping on this series.

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.

Quoted 1 line · Anchal

Gentle ping on this series.

Have you tested save/restore?

-bois

Quoted 4 lines · Boris Ostrovsky

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.

Quoted 5 lines · Anchal

Quoted 1 line

Gentle ping on this series.

Have you tested save/restore?

No, not with the last few series. But a good point, I will test that and get back to you. Do you see anything specific in the series that suggests otherwise?

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.

Quoted 5 lines · Anchal Agarwal

Quoted 4 lines · Boris Ostrovsky

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.

Quoted 7 lines

Quoted 5 lines · Anchal

Quoted 1 line

Gentle ping on this series.

Have you tested save/restore?

No, not with the last few series. But a good point, I will test that and get back to you. Do you see anything specific in the series that suggests otherwise?

root@ovs104> xl save pvh saved
Saving to saved new xl format (info 0x3/0x0/1699)
xc: info: Saving domain 3, type x86 HVM
xc: Frames: 1044480/1044480 100%
xc: End of stream: 0/0 0%
root@ovs104> xl restore saved
Loading new save file saved (new xl fmt info 0x3/0x0/1699)
Savefile contains xl domain config in JSON format
Parsing config from <saved>
xc: info: Found x86 HVM domain from Xen 4.13
xc: info: Restoring domain
xc: info: Restore successful
xc: info: XenStore: mfn 0xfeffc, dom 0, evt 1
xc: info: Console: mfn 0xfefff, dom 0, evt 2
root@ovs104> xl console pvh
[  139.943872] ------------[ cut here ]------------
[  139.943872] kernel BUG at arch/x86/xen/enlighten.c:205!
[  139.943872] invalid opcode: 0000 [#1] SMP PTI
[  139.943872] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.8.0-rc5 #26 [  139.943872] RIP: 0010:xen_vcpu_setup+0x16d/0x180
[  139.943872] Code: 4a 8b 14 f5 40 c9 1b 82 48 89 d8 48 89 2c 02 8b 05 a4 d4 40 01 85 c0 0f 85 15 ff ff ff 4a 8b 04 f5 40 c9 1b 82 e9 f4 fe ff ff <0f> 0b b8 ed ff ff ff e9 14 ff ff ff e8 12 4f 86 00 66 90 66 66 66 [  139.943872] RSP: 0018:ffffc9000006bdb0 EFLAGS: 00010046
[  139.943872] RAX: 0000000000000000 RBX: ffffc9000014fe00 RCX:
0000000000000000
[  139.943872] RDX: ffff88803fc00000 RSI: 0000000000016128 RDI:
0000000000000000
[  139.943872] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000000
[  139.943872] R10: ffffffff826174a0 R11: ffffc9000006bcb4 R12:
0000000000016120
[  139.943872] R13: 0000000000016120 R14: 0000000000016128 R15:
0000000000000000
[  139.943872] FS: 0000000000000000(0000) GS:ffff88803fc00000(0000) knlGS:0000000000000000
[  139.943872] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [  139.943872] CR2: 00007f704be8b000 CR3: 000000003901a004 CR4:
00000000000606f0
[  139.943872] Call Trace:
[  139.943872]  ? __kmalloc+0x167/0x260
[  139.943872]  ? xen_manage_runstate_time+0x14a/0x170
[  139.943872]  xen_vcpu_restore+0x134/0x170
[  139.943872]  xen_hvm_post_suspend+0x1d/0x30
[  139.943872]  xen_arch_post_suspend+0x13/0x30
[  139.943872]  xen_suspend+0x87/0x190
[  139.943872]  multi_cpu_stop+0x6d/0x110
[  139.943872]  ? stop_machine_yield+0x10/0x10
[  139.943872]  cpu_stopper_thread+0x47/0x100
[  139.943872]  smpboot_thread_fn+0xc5/0x160
[  139.943872]  ? sort_range+0x20/0x20
[  139.943872]  kthread+0xfe/0x140
[  139.943872]  ? kthread_park+0x90/0x90
[  139.943872]  ret_from_fork+0x22/0x30
[  139.943872] Modules linked in:
[  139.943872] ---[ end trace 74716859a6b4f0a8 ]---
[  139.943872] RIP: 0010:xen_vcpu_setup+0x16d/0x180
[  139.943872] Code: 4a 8b 14 f5 40 c9 1b 82 48 89 d8 48 89 2c 02 8b 05 a4 d4 40 01 85 c0 0f 85 15 ff ff ff 4a 8b 04 f5 40 c9 1b 82 e9 f4 fe ff ff <0f> 0b b8 ed ff ff ff e9 14 ff ff ff e8 12 4f 86 00 66 90 66 66 66 [  139.943872] RSP: 0018:ffffc9000006bdb0 EFLAGS: 00010046
[  139.943872] RAX: 0000000000000000 RBX: ffffc9000014fe00 RCX:
0000000000000000
[  139.943872] RDX: ffff88803fc00000 RSI: 0000000000016128 RDI:
0000000000000000
[  139.943872] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000000
[  139.943872] R10: ffffffff826174a0 R11: ffffc9000006bcb4 R12:
0000000000016120
[  139.943872] R13: 0000000000016120 R14: 0000000000016128 R15:
0000000000000000
[  139.943872] FS: 0000000000000000(0000) GS:ffff88803fc00000(0000)

knlGS:0000000000000000
[  139.943872] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [  139.943872] CR2: 00007f704be8b000 CR3: 000000003901a004 CR4:
00000000000606f0
[  139.943872] Kernel panic - not syncing: Fatal exception [  139.943872] Shutting down cpus with NMI
[  143.927559] Kernel Offset: disabled
root@ovs104>