[PATCH] xen: suspend and resume system devices when running PVHVM

[PATCH] xen: suspend and resume system devices when running PVHVM

[Ian Campbell]

Otherwise we fail to properly suspend/resume all of the emulated devices.

Something between 2.6.38-rc2 and rc3 appears to have exposed this
issue, but it's always been wrong not to do this.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 drivers/xen/manage.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index db8c4c4..2417727 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -37,11 +37,19 @@ static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 #ifdef CONFIG_PM_SLEEP
 static int xen_hvm_suspend(void *data)
 {
+	int err;
 	struct sched_shutdown r = { .reason = SHUTDOWN_suspend };
 	int *cancelled = data;
 
 	BUG_ON(!irqs_disabled());
 
+	err = sysdev_suspend(PMSG_SUSPEND);
+	if (err) {
+		printk(KERN_ERR "xen_hvm_suspend: sysdev_suspend failed: %d\n",
+		       err);
+		return err;
+	}
+
 	*cancelled = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
 
 	xen_hvm_post_suspend(*cancelled);
@@ -53,6 +61,8 @@ static int xen_hvm_suspend(void *data)
 		xen_timer_resume();
 	}
 
+	sysdev_resume();
+
 	return 0;
 }
 
-- 
1.5.6.5

Re: [PATCH] xen: suspend and resume system devices when running PVHVM

[Stefano Stabellini]

On Mon, 14 Feb 2011, Ian Campbell wrote:
> Otherwise we fail to properly suspend/resume all of the emulated devices.
> 
> Something between 2.6.38-rc2 and rc3 appears to have exposed this
> issue, but it's always been wrong not to do this.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>

I can confirm that this patch fixes a sporadic save/restore issue with
upstream PVHVM kernels.
I am going to import it in the pvhvm branch I am preparing for the next
merge window.

Re: [PATCH] xen: suspend and resume system devices when running PVHVM

[Ian Campbell]

On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> On Mon, 14 Feb 2011, Ian Campbell wrote:
> > Otherwise we fail to properly suspend/resume all of the emulated devices.
> > 
> > Something between 2.6.38-rc2 and rc3 appears to have exposed this
> > issue, but it's always been wrong not to do this.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> 
> I can confirm that this patch fixes a sporadic save/restore issue with
> upstream PVHVM kernels.
> I am going to import it in the pvhvm branch I am preparing for the next
> merge window.

I was intending to send this one to Linus for the next rc, since it
appears to be currently broken.

Ian.

Re: [PATCH] xen: suspend and resume system devices when running PVHVM

[Stefano Stabellini]

On Mon, 14 Feb 2011, Ian Campbell wrote:
> On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> > On Mon, 14 Feb 2011, Ian Campbell wrote:
> > > Otherwise we fail to properly suspend/resume all of the emulated devices.
> > > 
> > > Something between 2.6.38-rc2 and rc3 appears to have exposed this
> > > issue, but it's always been wrong not to do this.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> > 
> > I can confirm that this patch fixes a sporadic save/restore issue with
> > upstream PVHVM kernels.
> > I am going to import it in the pvhvm branch I am preparing for the next
> > merge window.
> 
> I was intending to send this one to Linus for the next rc, since it
> appears to be currently broken.

great, even better

Re: [PATCH] xen: suspend and resume system devices when running PVHVM

[Jeremy Fitzhardinge]

On 02/14/2011 10:59 AM, Ian Campbell wrote:
> On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
>> On Mon, 14 Feb 2011, Ian Campbell wrote:
>>> Otherwise we fail to properly suspend/resume all of the emulated devices.
>>>
>>> Something between 2.6.38-rc2 and rc3 appears to have exposed this
>>> issue, but it's always been wrong not to do this.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>> I can confirm that this patch fixes a sporadic save/restore issue with
>> upstream PVHVM kernels.
>> I am going to import it in the pvhvm branch I am preparing for the next
>> merge window.
> I was intending to send this one to Linus for the next rc, since it
> appears to be currently broken.

Fine by me.

    J

Re: [PATCH] xen: suspend and resume system devices when running PVHVM

[Ian Campbell]

On Mon, 2011-02-14 at 23:30 +0000, Jeremy Fitzhardinge wrote:
> On 02/14/2011 10:59 AM, Ian Campbell wrote:
> > On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> >> On Mon, 14 Feb 2011, Ian Campbell wrote:
> >>> Otherwise we fail to properly suspend/resume all of the emulated devices.
> >>>
> >>> Something between 2.6.38-rc2 and rc3 appears to have exposed this
> >>> issue, but it's always been wrong not to do this.
> >>>
> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> >> I can confirm that this patch fixes a sporadic save/restore issue with
> >> upstream PVHVM kernels.
> >> I am going to import it in the pvhvm branch I am preparing for the next
> >> merge window.
> > I was intending to send this one to Linus for the next rc, since it
> > appears to be currently broken.
> 
> Fine by me.

Thanks, I'll take your and Stefano's responses as Acked-by's and send
upstream today.

Ian.

[PATCH 0/11] suspend cleanups, refactoring and fixes (Was: Re: [PATCH] xen: suspend and resume system devices when running PVHVM)

[Ian Campbell]

On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> I am going to import it in the pvhvm branch I am preparing for the next
> merge window.

Is there any conflict with the following series (which builds on this
patch)? It's not all pvhvm related but the bulk is the removal of the
separate xen_hvm_suspend function.

Do you want to carry these in your branch for the next merge window or
shall I create a separate branch?

Ian.

[PATCH 01/11] xen: do not respond to unknown xenstore control requests

[Ian Campbell]

The PV xenbus control/shutdown node is written by the toolstack as a
request to the guest to perform a particular action (shutdown, reboot,
suspend etc). The guest is expected to acknowledge that it will
complete a request by clearing the control node.

Previously it would acknowledge any request, even if it did not know
what to do with it. Specifically in the case where CONFIG_PM_SLEEP is
not enabled the kernel would acknowledge a suspend request even though
it was not actually going to do anything.

Instead make the kernel only acknowledge requests if it is actually
going to do something with it. This will improve the toolstack's
ability to diagnose and deal with failures.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |   49 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 2417727..6f866e2 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -172,12 +172,39 @@ out:
 }
 #endif	/* CONFIG_PM_SLEEP */
 
+struct shutdown_handler {
+	const char *command;
+	void (*cb)(void);
+};
+
+static void do_poweroff(void)
+{
+	shutting_down = SHUTDOWN_POWEROFF;
+	orderly_poweroff(false);
+}
+
+static void do_reboot(void)
+{
+	shutting_down = SHUTDOWN_POWEROFF; /* ? */
+	ctrl_alt_del();
+}
+
 static void shutdown_handler(struct xenbus_watch *watch,
 			     const char **vec, unsigned int len)
 {
 	char *str;
 	struct xenbus_transaction xbt;
 	int err;
+	static struct shutdown_handler handlers[] = {
+		{ "poweroff",	do_poweroff },
+		{ "halt",	do_poweroff },
+		{ "reboot",	do_reboot   },
+#ifdef CONFIG_PM_SLEEP
+		{ "suspend",	do_suspend  },
+#endif
+		{NULL, NULL},
+	};
+	static struct shutdown_handler *handler;
 
 	if (shutting_down != SHUTDOWN_INVALID)
 		return;
@@ -194,7 +221,14 @@ static void shutdown_handler(struct xenbus_watch *watch,
 		return;
 	}
 
-	xenbus_write(xbt, "control", "shutdown", "");
+	for (handler = &handlers[0]; handler->command; handler++) {
+		if (strcmp(str, handler->command) == 0)
+			break;
+	}
+
+	/* Only acknowledge commands which we are prepared to handle. */
+	if (handler->cb)
+		xenbus_write(xbt, "control", "shutdown", "");
 
 	err = xenbus_transaction_end(xbt, 0);
 	if (err == -EAGAIN) {
@@ -202,17 +236,8 @@ static void shutdown_handler(struct xenbus_watch *watch,
 		goto again;
 	}
 
-	if (strcmp(str, "poweroff") == 0 ||
-	    strcmp(str, "halt") == 0) {
-		shutting_down = SHUTDOWN_POWEROFF;
-		orderly_poweroff(false);
-	} else if (strcmp(str, "reboot") == 0) {
-		shutting_down = SHUTDOWN_POWEROFF; /* ? */
-		ctrl_alt_del();
-#ifdef CONFIG_PM_SLEEP
-	} else if (strcmp(str, "suspend") == 0) {
-		do_suspend();
-#endif
+	if (handler->cb) {
+		handler->cb();
 	} else {
 		printk(KERN_INFO "Ignoring shutdown request: %s\n", str);
 		shutting_down = SHUTDOWN_INVALID;
-- 
1.5.6.5

[PATCH 02/11] xen: use new schedop interface for suspend

[Ian Campbell]

Take the opportunity to comment on the semantics of the PV guest
suspend hypercall arguments.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/include/asm/xen/hypercall.h |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index a3c28ae..a7d5823 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -422,10 +422,17 @@ HYPERVISOR_set_segment_base(int reg, unsigned long value)
 #endif
 
 static inline int
-HYPERVISOR_suspend(unsigned long srec)
+HYPERVISOR_suspend(unsigned long start_info_mfn)
 {
-	return _hypercall3(int, sched_op, SCHEDOP_shutdown,
-			   SHUTDOWN_suspend, srec);
+	struct sched_shutdown r = { .reason = SHUTDOWN_suspend };
+
+	/*
+	 * For a PV guest the tools require that the start_info mfn be
+	 * present in rdx/edx when the hypercall is made. Per the
+	 * hypercall calling convention this is the third hypercall
+	 * argument, which is start_info_mfn here.
+	 */
+	return _hypercall3(int, sched_op_new, SCHEDOP_shutdown, &r, start_info_mfn);
 }
 
 static inline int
-- 
1.5.6.5

[PATCH 03/11] xen: switch to new schedop hypercall by default.

[Ian Campbell]

Rename old interface to sched_op_compat and rename sched_op_new to
simply sched_op.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/include/asm/xen/hypercall.h |    4 ++--
 include/xen/interface/xen.h          |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index a7d5823..8508bfe 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -287,7 +287,7 @@ HYPERVISOR_fpu_taskswitch(int set)
 static inline int
 HYPERVISOR_sched_op(int cmd, void *arg)
 {
-	return _hypercall2(int, sched_op_new, cmd, arg);
+	return _hypercall2(int, sched_op, cmd, arg);
 }
 
 static inline long
@@ -432,7 +432,7 @@ HYPERVISOR_suspend(unsigned long start_info_mfn)
 	 * hypercall calling convention this is the third hypercall
 	 * argument, which is start_info_mfn here.
 	 */
-	return _hypercall3(int, sched_op_new, SCHEDOP_shutdown, &r, start_info_mfn);
+	return _hypercall3(int, sched_op, SCHEDOP_shutdown, &r, start_info_mfn);
 }
 
 static inline int
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 2befa3e..b33257b 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -30,7 +30,7 @@
 #define __HYPERVISOR_stack_switch          3
 #define __HYPERVISOR_set_callbacks         4
 #define __HYPERVISOR_fpu_taskswitch        5
-#define __HYPERVISOR_sched_op              6
+#define __HYPERVISOR_sched_op_compat       6
 #define __HYPERVISOR_dom0_op               7
 #define __HYPERVISOR_set_debugreg          8
 #define __HYPERVISOR_get_debugreg          9
@@ -52,7 +52,7 @@
 #define __HYPERVISOR_mmuext_op            26
 #define __HYPERVISOR_acm_op               27
 #define __HYPERVISOR_nmi_op               28
-#define __HYPERVISOR_sched_op_new         29
+#define __HYPERVISOR_sched_op             29
 #define __HYPERVISOR_callback_op          30
 #define __HYPERVISOR_xenoprof_op          31
 #define __HYPERVISOR_event_channel_op     32
-- 
1.5.6.5

[PATCH 04/11] xen: suspend: use HYPERVISOR_suspend for PVHVM case instead of open coding

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 6f866e2..ba7caa3 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -38,7 +38,6 @@ static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 static int xen_hvm_suspend(void *data)
 {
 	int err;
-	struct sched_shutdown r = { .reason = SHUTDOWN_suspend };
 	int *cancelled = data;
 
 	BUG_ON(!irqs_disabled());
@@ -50,7 +49,12 @@ static int xen_hvm_suspend(void *data)
 		return err;
 	}
 
-	*cancelled = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
+	/*
+	 * This hypercall returns 1 if suspend was cancelled
+	 * or the domain was merely checkpointed, and 0 if it
+	 * is resuming in a new domain.
+	 */
+	*cancelled = HYPERVISOR_suspend(0UL);
 
 	xen_hvm_post_suspend(*cancelled);
 	gnttab_resume();
-- 
1.5.6.5

[PATCH 05/11] xen: suspend: refactor cancellation flag into a structure

[Ian Campbell]

Will add extra fields in subsequent patches.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index ba7caa3..22af876 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -34,11 +34,15 @@ enum shutdown_state {
 /* Ignore multiple shutdown requests. */
 static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 
+struct suspend_info {
+	int cancelled;
+};
+
 #ifdef CONFIG_PM_SLEEP
 static int xen_hvm_suspend(void *data)
 {
+	struct suspend_info *si = data;
 	int err;
-	int *cancelled = data;
 
 	BUG_ON(!irqs_disabled());
 
@@ -54,12 +58,12 @@ static int xen_hvm_suspend(void *data)
 	 * or the domain was merely checkpointed, and 0 if it
 	 * is resuming in a new domain.
 	 */
-	*cancelled = HYPERVISOR_suspend(0UL);
+	si->cancelled = HYPERVISOR_suspend(0UL);
 
-	xen_hvm_post_suspend(*cancelled);
+	xen_hvm_post_suspend(si->cancelled);
 	gnttab_resume();
 
-	if (!*cancelled) {
+	if (!si->cancelled) {
 		xen_irq_resume();
 		xen_console_resume();
 		xen_timer_resume();
@@ -72,8 +76,8 @@ static int xen_hvm_suspend(void *data)
 
 static int xen_suspend(void *data)
 {
+	struct suspend_info *si = data;
 	int err;
-	int *cancelled = data;
 
 	BUG_ON(!irqs_disabled());
 
@@ -93,13 +97,13 @@ static int xen_suspend(void *data)
 	 * or the domain was merely checkpointed, and 0 if it
 	 * is resuming in a new domain.
 	 */
-	*cancelled = HYPERVISOR_suspend(virt_to_mfn(xen_start_info));
+	si->cancelled = HYPERVISOR_suspend(virt_to_mfn(xen_start_info));
 
-	xen_post_suspend(*cancelled);
+	xen_post_suspend(si->cancelled);
 	gnttab_resume();
 	xen_mm_unpin_all();
 
-	if (!*cancelled) {
+	if (!si->cancelled) {
 		xen_irq_resume();
 		xen_console_resume();
 		xen_timer_resume();
@@ -113,7 +117,7 @@ static int xen_suspend(void *data)
 static void do_suspend(void)
 {
 	int err;
-	int cancelled = 1;
+	struct suspend_info si;
 
 	shutting_down = SHUTDOWN_SUSPEND;
 
@@ -143,20 +147,22 @@ static void do_suspend(void)
 		goto out_resume;
 	}
 
+	si.cancelled = 1;
+
 	if (xen_hvm_domain())
-		err = stop_machine(xen_hvm_suspend, &cancelled, cpumask_of(0));
+		err = stop_machine(xen_hvm_suspend, &si, cpumask_of(0));
 	else
-		err = stop_machine(xen_suspend, &cancelled, cpumask_of(0));
+		err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
 	dpm_resume_noirq(PMSG_RESUME);
 
 	if (err) {
 		printk(KERN_ERR "failed to start xen_suspend: %d\n", err);
-		cancelled = 1;
+		si.cancelled = 1;
 	}
 
 out_resume:
-	if (!cancelled) {
+	if (!si.cancelled) {
 		xen_arch_resume();
 		xs_resume();
 	} else
-- 
1.5.6.5

[PATCH 06/11] xen: suspend: pass extra hypercall argument via suspend_info struct

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 22af876..0647918 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -36,6 +36,7 @@ static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 
 struct suspend_info {
 	int cancelled;
+	unsigned long arg; /* extra hypercall argument */
 };
 
 #ifdef CONFIG_PM_SLEEP
@@ -58,7 +59,7 @@ static int xen_hvm_suspend(void *data)
 	 * or the domain was merely checkpointed, and 0 if it
 	 * is resuming in a new domain.
 	 */
-	si->cancelled = HYPERVISOR_suspend(0UL);
+	si->cancelled = HYPERVISOR_suspend(si->arg);
 
 	xen_hvm_post_suspend(si->cancelled);
 	gnttab_resume();
@@ -97,7 +98,7 @@ static int xen_suspend(void *data)
 	 * or the domain was merely checkpointed, and 0 if it
 	 * is resuming in a new domain.
 	 */
-	si->cancelled = HYPERVISOR_suspend(virt_to_mfn(xen_start_info));
+	si->cancelled = HYPERVISOR_suspend(si->arg);
 
 	xen_post_suspend(si->cancelled);
 	gnttab_resume();
@@ -150,6 +151,11 @@ static void do_suspend(void)
 	si.cancelled = 1;
 
 	if (xen_hvm_domain())
+		si.arg = 0UL;
+	else
+		si.arg = virt_to_mfn(xen_start_info);
+
+	if (xen_hvm_domain())
 		err = stop_machine(xen_hvm_suspend, &si, cpumask_of(0));
 	else
 		err = stop_machine(xen_suspend, &si, cpumask_of(0));
-- 
1.5.6.5

[PATCH 07/11] xen: suspend: add "arch" to pre/post suspend hooks

[Ian Campbell]

xen_pre_device_suspend is unused on ia64.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/ia64/xen/suspend.c |    9 ++-------
 arch/x86/xen/suspend.c  |    6 +++---
 drivers/xen/manage.c    |    6 +++---
 include/xen/xen-ops.h   |    6 +++---
 4 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/xen/suspend.c b/arch/ia64/xen/suspend.c
index fd66b04..419c862 100644
--- a/arch/ia64/xen/suspend.c
+++ b/arch/ia64/xen/suspend.c
@@ -37,19 +37,14 @@ xen_mm_unpin_all(void)
 	/* nothing */
 }
 
-void xen_pre_device_suspend(void)
-{
-	/* nothing */
-}
-
 void
-xen_pre_suspend()
+xen_arch_pre_suspend()
 {
 	/* nothing */
 }
 
 void
-xen_post_suspend(int suspend_cancelled)
+xen_arch_post_suspend(int suspend_cancelled)
 {
 	if (suspend_cancelled)
 		return;
diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
index 9bbd63a..96b1be2 100644
--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -12,7 +12,7 @@
 #include "xen-ops.h"
 #include "mmu.h"
 
-void xen_pre_suspend(void)
+void xen_arch_pre_suspend(void)
 {
 	xen_start_info->store_mfn = mfn_to_pfn(xen_start_info->store_mfn);
 	xen_start_info->console.domU.mfn =
@@ -26,7 +26,7 @@ void xen_pre_suspend(void)
 		BUG();
 }
 
-void xen_hvm_post_suspend(int suspend_cancelled)
+void xen_arch_hvm_post_suspend(int suspend_cancelled)
 {
 	int cpu;
 	xen_hvm_init_shared_info();
@@ -39,7 +39,7 @@ void xen_hvm_post_suspend(int suspend_cancelled)
 	}
 }
 
-void xen_post_suspend(int suspend_cancelled)
+void xen_arch_post_suspend(int suspend_cancelled)
 {
 	xen_build_mfn_list_list();
 
diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 0647918..7783722 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -61,7 +61,7 @@ static int xen_hvm_suspend(void *data)
 	 */
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
-	xen_hvm_post_suspend(si->cancelled);
+	xen_arch_hvm_post_suspend(si->cancelled);
 	gnttab_resume();
 
 	if (!si->cancelled) {
@@ -91,7 +91,7 @@ static int xen_suspend(void *data)
 
 	xen_mm_pin_all();
 	gnttab_suspend();
-	xen_pre_suspend();
+	xen_arch_pre_suspend();
 
 	/*
 	 * This hypercall returns 1 if suspend was cancelled
@@ -100,7 +100,7 @@ static int xen_suspend(void *data)
 	 */
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
-	xen_post_suspend(si->cancelled);
+	xen_arch_post_suspend(si->cancelled);
 	gnttab_resume();
 	xen_mm_unpin_all();
 
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 98b9215..03c85d7 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -5,9 +5,9 @@
 
 DECLARE_PER_CPU(struct vcpu_info *, xen_vcpu);
 
-void xen_pre_suspend(void);
-void xen_post_suspend(int suspend_cancelled);
-void xen_hvm_post_suspend(int suspend_cancelled);
+void xen_arch_pre_suspend(void);
+void xen_arch_post_suspend(int suspend_cancelled);
+void xen_arch_hvm_post_suspend(int suspend_cancelled);
 
 void xen_mm_pin_all(void);
 void xen_mm_unpin_all(void);
-- 
1.5.6.5

[PATCH 08/11] xen: suspend: refactor non-arch specific pre/post suspend hooks

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 7783722..f61c155 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -39,6 +39,23 @@ struct suspend_info {
 	unsigned long arg; /* extra hypercall argument */
 };
 
+static void xen_hvm_post_suspend(void)
+{
+	gnttab_resume();
+}
+
+static void xen_pre_suspend(void)
+{
+	xen_mm_pin_all();
+	gnttab_suspend();
+}
+
+static void xen_post_suspend(void)
+{
+	gnttab_resume();
+	xen_mm_unpin_all();
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int xen_hvm_suspend(void *data)
 {
@@ -62,7 +79,7 @@ static int xen_hvm_suspend(void *data)
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
 	xen_arch_hvm_post_suspend(si->cancelled);
-	gnttab_resume();
+	xen_hvm_post_suspend();
 
 	if (!si->cancelled) {
 		xen_irq_resume();
@@ -89,8 +106,7 @@ static int xen_suspend(void *data)
 		return err;
 	}
 
-	xen_mm_pin_all();
-	gnttab_suspend();
+	xen_pre_suspend();
 	xen_arch_pre_suspend();
 
 	/*
@@ -101,8 +117,7 @@ static int xen_suspend(void *data)
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
 	xen_arch_post_suspend(si->cancelled);
-	gnttab_resume();
-	xen_mm_unpin_all();
+	xen_post_suspend();
 
 	if (!si->cancelled) {
 		xen_irq_resume();
-- 
1.5.6.5

[PATCH 09/11] xen: suspend: move arch specific pre/post suspend hooks into generic hooks

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index f61c155..49a7458 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -39,8 +39,9 @@ struct suspend_info {
 	unsigned long arg; /* extra hypercall argument */
 };
 
-static void xen_hvm_post_suspend(void)
+static void xen_hvm_post_suspend(int cancelled)
 {
+	xen_arch_hvm_post_suspend(cancelled);
 	gnttab_resume();
 }
 
@@ -48,10 +49,12 @@ static void xen_pre_suspend(void)
 {
 	xen_mm_pin_all();
 	gnttab_suspend();
+	xen_arch_pre_suspend();
 }
 
-static void xen_post_suspend(void)
+static void xen_post_suspend(int cancelled)
 {
+	xen_arch_post_suspend(cancelled);
 	gnttab_resume();
 	xen_mm_unpin_all();
 }
@@ -78,8 +81,7 @@ static int xen_hvm_suspend(void *data)
 	 */
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
-	xen_arch_hvm_post_suspend(si->cancelled);
-	xen_hvm_post_suspend();
+	xen_hvm_post_suspend(si->cancelled);
 
 	if (!si->cancelled) {
 		xen_irq_resume();
@@ -107,7 +109,6 @@ static int xen_suspend(void *data)
 	}
 
 	xen_pre_suspend();
-	xen_arch_pre_suspend();
 
 	/*
 	 * This hypercall returns 1 if suspend was cancelled
@@ -116,8 +117,7 @@ static int xen_suspend(void *data)
 	 */
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
-	xen_arch_post_suspend(si->cancelled);
-	xen_post_suspend();
+	xen_post_suspend(si->cancelled);
 
 	if (!si->cancelled) {
 		xen_irq_resume();
-- 
1.5.6.5

[PATCH 10/11] xen: suspend: pull pre/post suspend hooks out into suspend_info

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 49a7458..5ccddab 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -37,6 +37,8 @@ static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 struct suspend_info {
 	int cancelled;
 	unsigned long arg; /* extra hypercall argument */
+	void (*pre)(void);
+	void (*post)(int cancelled);
 };
 
 static void xen_hvm_post_suspend(int cancelled)
@@ -74,6 +76,9 @@ static int xen_hvm_suspend(void *data)
 		return err;
 	}
 
+	if (si->pre)
+		si->pre();
+
 	/*
 	 * This hypercall returns 1 if suspend was cancelled
 	 * or the domain was merely checkpointed, and 0 if it
@@ -81,7 +86,8 @@ static int xen_hvm_suspend(void *data)
 	 */
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
-	xen_hvm_post_suspend(si->cancelled);
+	if (si->post)
+		si->post(si->cancelled);
 
 	if (!si->cancelled) {
 		xen_irq_resume();
@@ -108,7 +114,8 @@ static int xen_suspend(void *data)
 		return err;
 	}
 
-	xen_pre_suspend();
+	if (si->pre)
+		si->pre();
 
 	/*
 	 * This hypercall returns 1 if suspend was cancelled
@@ -117,7 +124,8 @@ static int xen_suspend(void *data)
 	 */
 	si->cancelled = HYPERVISOR_suspend(si->arg);
 
-	xen_post_suspend(si->cancelled);
+	if (si->post)
+		si->post(si->cancelled);
 
 	if (!si->cancelled) {
 		xen_irq_resume();
@@ -165,10 +173,15 @@ static void do_suspend(void)
 
 	si.cancelled = 1;
 
-	if (xen_hvm_domain())
+	if (xen_hvm_domain()) {
 		si.arg = 0UL;
-	else
+		si.pre = NULL;
+		si.post = &xen_hvm_post_suspend;
+	} else {
 		si.arg = virt_to_mfn(xen_start_info);
+		si.pre = &xen_pre_suspend;
+		si.post = &xen_post_suspend;
+	}
 
 	if (xen_hvm_domain())
 		err = stop_machine(xen_hvm_suspend, &si, cpumask_of(0));
-- 
1.5.6.5

[PATCH 11/11] xen: suspend: remove xen_hvm_suspend

[Ian Campbell]

It is now identical to xen_suspend, the differences are encapsulated
in the suspend_info struct.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/manage.c |   43 +------------------------------------------
 1 files changed, 1 insertions(+), 42 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 5ccddab..c255ed4 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -62,44 +62,6 @@ static void xen_post_suspend(int cancelled)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int xen_hvm_suspend(void *data)
-{
-	struct suspend_info *si = data;
-	int err;
-
-	BUG_ON(!irqs_disabled());
-
-	err = sysdev_suspend(PMSG_SUSPEND);
-	if (err) {
-		printk(KERN_ERR "xen_hvm_suspend: sysdev_suspend failed: %d\n",
-		       err);
-		return err;
-	}
-
-	if (si->pre)
-		si->pre();
-
-	/*
-	 * This hypercall returns 1 if suspend was cancelled
-	 * or the domain was merely checkpointed, and 0 if it
-	 * is resuming in a new domain.
-	 */
-	si->cancelled = HYPERVISOR_suspend(si->arg);
-
-	if (si->post)
-		si->post(si->cancelled);
-
-	if (!si->cancelled) {
-		xen_irq_resume();
-		xen_console_resume();
-		xen_timer_resume();
-	}
-
-	sysdev_resume();
-
-	return 0;
-}
-
 static int xen_suspend(void *data)
 {
 	struct suspend_info *si = data;
@@ -183,10 +145,7 @@ static void do_suspend(void)
 		si.post = &xen_post_suspend;
 	}
 
-	if (xen_hvm_domain())
-		err = stop_machine(xen_hvm_suspend, &si, cpumask_of(0));
-	else
-		err = stop_machine(xen_suspend, &si, cpumask_of(0));
+	err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
 	dpm_resume_noirq(PMSG_RESUME);
 
-- 
1.5.6.5

Re: [PATCH 0/11] suspend cleanups, refactoring and fixes (Was: Re: [PATCH] xen: suspend and resume system devices when running PVHVM)

[Stefano Stabellini]

On Tue, 15 Feb 2011, Ian Campbell wrote:
> On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> > I am going to import it in the pvhvm branch I am preparing for the next
> > merge window.
> 
> Is there any conflict with the following series (which builds on this
> patch)? It's not all pvhvm related but the bulk is the removal of the
> separate xen_hvm_suspend function.
> 
> Do you want to carry these in your branch for the next merge window or
> shall I create a separate branch?

They all seem very reasonable, I don't mind having them in my branch.

Re: [PATCH 0/11] suspend cleanups, refactoring and fixes (Was: Re: [PATCH] xen: suspend and resume system devices when running PVHVM)

[Ian Campbell]

On Tue, 2011-02-15 at 16:45 +0000, Stefano Stabellini wrote:
> On Tue, 15 Feb 2011, Ian Campbell wrote:
> > On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> > > I am going to import it in the pvhvm branch I am preparing for the next
> > > merge window.
> > 
> > Is there any conflict with the following series (which builds on this
> > patch)? It's not all pvhvm related but the bulk is the removal of the
> > separate xen_hvm_suspend function.
> > 
> > Do you want to carry these in your branch for the next merge window or
> > shall I create a separate branch?
> 
> They all seem very reasonable, I don't mind having them in my branch.

Thanks.

I resent the precursor patch to Linus as a git pull request. The
following includes that one commit followed by this series.

The following changes since commit 100b33c8bd8a3235fd0b7948338d6cbb3db3c63d:
  Linus Torvalds (1):
        Linux 2.6.38-rc4

are available in the git repository at:

  git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm

Ian Campbell (12):
      xen: suspend and resume system devices when running PVHVM
      xen: do not respond to unknown xenstore control requests
      xen: use new schedop interface for suspend
      xen: switch to new schedop hypercall by default.
      xen: suspend: use HYPERVISOR_suspend for PVHVM case instead of open coding
      xen: suspend: refactor cancellation flag into a structure
      xen: suspend: pass extra hypercall argument via suspend_info struct
      xen: suspend: add "arch" to pre/post suspend hooks
      xen: suspend: refactor non-arch specific pre/post suspend hooks
      xen: suspend: move arch specific pre/post suspend hooks into generic hooks
      xen: suspend: pull pre/post suspend hooks out into suspend_info
      xen: suspend: remove xen_hvm_suspend

 arch/ia64/xen/suspend.c              |    9 +--
 arch/x86/include/asm/xen/hypercall.h |   15 +++-
 arch/x86/xen/suspend.c               |    6 +-
 drivers/xen/manage.c                 |  126 ++++++++++++++++++++++------------
 include/xen/interface/xen.h          |    4 +-
 include/xen/xen-ops.h                |    6 +-
 6 files changed, 103 insertions(+), 63 deletions(-)

Re: [PATCH 0/11] suspend cleanups, refactoring and fixes (Was: Re: [PATCH] xen: suspend and resume system devices when running PVHVM)

[Stefano Stabellini]

On Thu, 17 Feb 2011, Ian Campbell wrote:
> On Tue, 2011-02-15 at 16:45 +0000, Stefano Stabellini wrote:
> > On Tue, 15 Feb 2011, Ian Campbell wrote:
> > > On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> > > > I am going to import it in the pvhvm branch I am preparing for the next
> > > > merge window.
> > > 
> > > Is there any conflict with the following series (which builds on this
> > > patch)? It's not all pvhvm related but the bulk is the removal of the
> > > separate xen_hvm_suspend function.
> > > 
> > > Do you want to carry these in your branch for the next merge window or
> > > shall I create a separate branch?
> > 
> > They all seem very reasonable, I don't mind having them in my branch.
> 
> Thanks.
> 
> I resent the precursor patch to Linus as a git pull request. The
> following includes that one commit followed by this series.
> 

I merged it into the branch 2.6.38-rc5-pvhvm

Re: [PATCH 0/11] suspend cleanups, refactoring and fixes (Was: Re: [PATCH] xen: suspend and resume system devices when running PVHVM)

[Konrad Rzeszutek Wilk]

On Tue, Feb 15, 2011 at 02:13:01PM +0000, Ian Campbell wrote:
> On Mon, 2011-02-14 at 18:41 +0000, Stefano Stabellini wrote:
> > I am going to import it in the pvhvm branch I am preparing for the next
> > merge window.
> 
> Is there any conflict with the following series (which builds on this
> patch)? It's not all pvhvm related but the bulk is the removal of the
> separate xen_hvm_suspend function.
> 
> Do you want to carry these in your branch for the next merge window or
> shall I create a separate branch?

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Re: [PATCH] xen: suspend and resume system devices when running PVHVM

[Shriram Rajagopalan]

On Mon, Feb 14, 2011 at 9:32 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> Otherwise we fail to properly suspend/resume all of the emulated devices.
>
> Something between 2.6.38-rc2 and rc3 appears to have exposed this
> issue, but it's always been wrong not to do this.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> ---
>  drivers/xen/manage.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index db8c4c4..2417727 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -37,11 +37,19 @@ static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
>  #ifdef CONFIG_PM_SLEEP
>  static int xen_hvm_suspend(void *data)
>  {
> +       int err;
>        struct sched_shutdown r = { .reason = SHUTDOWN_suspend };
>        int *cancelled = data;
>
>        BUG_ON(!irqs_disabled());
>
> +       err = sysdev_suspend(PMSG_SUSPEND);

Shouldnt this be PMSG_FREEZE ? based on the recent discussion with
linux-pm folks and the issue of xm save -c resulting in guest hangup.
> +       if (err) {
> +               printk(KERN_ERR "xen_hvm_suspend: sysdev_suspend failed: %d\n",
> +                      err);
> +               return err;
> +       }
> +
>        *cancelled = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
>
>        xen_hvm_post_suspend(*cancelled);
> @@ -53,6 +61,8 @@ static int xen_hvm_suspend(void *data)
>                xen_timer_resume();
>        }
>
> +       sysdev_resume();
> +
>        return 0;
>  }
>
> --
> 1.5.6.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Re: [PATCH] xen: suspend and resume system devices when running PVHVM

[Ian Campbell]

On Tue, 2011-02-15 at 05:49 +0000, Shriram Rajagopalan wrote:
> On Mon, Feb 14, 2011 at 9:32 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> > Otherwise we fail to properly suspend/resume all of the emulated devices.
> >
> > Something between 2.6.38-rc2 and rc3 appears to have exposed this
> > issue, but it's always been wrong not to do this.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> > ---
> >  drivers/xen/manage.c |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index db8c4c4..2417727 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -37,11 +37,19 @@ static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
> >  #ifdef CONFIG_PM_SLEEP
> >  static int xen_hvm_suspend(void *data)
> >  {
> > +       int err;
> >        struct sched_shutdown r = { .reason = SHUTDOWN_suspend };
> >        int *cancelled = data;
> >
> >        BUG_ON(!irqs_disabled());
> >
> > +       err = sysdev_suspend(PMSG_SUSPEND);
> 
> Shouldnt this be PMSG_FREEZE ? based on the recent discussion with
> linux-pm folks and the issue of xm save -c resulting in guest hangup.

After your patch goes upstream, yes I think it should be, however until
then PMSG_SUSPEND is consistent with the rest of the code.

Ian.

> > +       if (err) {
> > +               printk(KERN_ERR "xen_hvm_suspend: sysdev_suspend failed: %d\n",
> > +                      err);
> > +               return err;
> > +       }
> > +
> >        *cancelled = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
> >
> >        xen_hvm_post_suspend(*cancelled);
> > @@ -53,6 +61,8 @@ static int xen_hvm_suspend(void *data)
> >                xen_timer_resume();
> >        }
> >
> > +       sysdev_resume();
> > +
> >        return 0;
> >  }
> >
> > --
> > 1.5.6.5
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> >