Re: Early ACPI events prevent subsequent ACPI functionality on xen 4.3 + HVM domU

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Nov 06, 2013 at 04:14:20PM +0000, Ian Campbell wrote:
> On Wed, 2013-11-06 at 11:05 -0500, Konrad Rzeszutek Wilk wrote:
> > From f53c1f691f726a9d72ad11be56f84389c3f3d7b0 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Wed, 6 Nov 2013 10:57:56 -0500
> > Subject: [PATCH] xen/manage: Poweroff forcefully if user-space is not yet up.
> > 
> > The user can launch the guest in this sequence:
> > 
> > xl create -p /vm.cfg	[launch, but pause it]
> > xl shutdown latest	[sets control/shutdown=poweroff]
> > xl unpause latest
> > xl console latest	[and see that the guest has completlty
> 
> "completely"
> 
> > ignored the shutdown request]
> > 
> > In reality the guest hasn't ignored it. It registers a watch
> > and gets a notification that there is value. It then calls
> > the shutdown_handler which ends up calling orderly_shutdown.
> > 
> > Unfortunately that is so early in the bootup that there
> > are no user-space. Which means that the orderly_shutdown fails.
> > But since the force flag was set to false it continues on without
> > reporting.
> > 
> > We check if the system is still in the booting stage and if so
> > enable the force option (which will shutdown in early bootup
> > process). If in normal running case we don't force it.
> > 
> > Fixes-Bug: http://bugs.xenproject.org/xen/bug/6
> > Reported-by:  Alex Bligh <alex@alex.org.uk>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/xen/manage.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index 624e8dc..fe1c0a6 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -185,7 +185,7 @@ struct shutdown_handler {
> >  static void do_poweroff(void)
> >  {
> >  	shutting_down = SHUTDOWN_POWEROFF;
> > -	orderly_poweroff(false);
> > +	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
> 
> Does this DTRT for systems in SYSTEM_{HALTED,POWEROFF}? I suppose under
> those circumstances forcing is the desired action, insn't it

Yes. And there is also a guard (shutting_down) that gets set on
drivers/xen/manage.c so that the 'do_poweroff' will only get called
once. Which would guard against us powering off and then
receiving another 'xl shutdown' when the the system_state is in
HALTED or POWEROFF.

But I hadn't tested the case where the user does 'poweroff'
and at the same time the system admin does 'xl shutdown'.

Depending on the race, the state will be SYSTEM_RUNNING or
SYSTEM_POWER_OFF. If SYSTEM_RUNNING we just end up making
a duplicate call to 'poweroff' (while it is running).

That will fail or execute (And if executed then it will be
stuck in the reboot_mutex mutex). But nobody will care b/c the
machine is in poweroff sequence.

If the state is SYSTEM_POWER_OFF then we end up making
a duplicate call to kernel_power_off. There is no locking
there so we walk in the same steps as what 'poweroff'
has been doing.

This code in kernel/reboot.c doesn't look that safe when it
comes to a user-invoked 'poweroff' operation and a kernel
'orderly_poweroff' operation.

Perhaps what we should do is just:

	if (system_state == SYSTEM_BOOTING)
		orderly_poweroff(true);
	else if (system_state == SYSTEM_RUNNING)
		orderly_poweroff(false);
	else
		printk("Shutdown in progress. Ignoring xl shutdown");

But then 'system_state' is not guarded by a spinlock or such. Thought
it is guarded by the xenwatch mutex.

Perhaps to be extra safe we should add ourselves to the
register_reboot_notifier like so (not compile tested)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index fe1c0a6..fb43db6 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -36,7 +36,8 @@ enum shutdown_state {
 	 SHUTDOWN_HALT = 4,
 };
 
-/* Ignore multiple shutdown requests. */
+/* Ignore multiple shutdown requests. Our mutex for this is that
+ * shutdown handler is called with a mutex from xenwatch. */
 static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 
 struct suspend_info {
@@ -185,7 +186,12 @@ struct shutdown_handler {
 static void do_poweroff(void)
 {
 	shutting_down = SHUTDOWN_POWEROFF;
-	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
+	if (system_state == SYSTEM_RUNNING)
+		orderly_poweroff(false);
+	else if (system_state == SYSTEM_BOOTING)
+		orderly_poweroff(true);
+	else
+		printk(KERN_WARNING "Ignorning shutdown request as poweroff in progress.\n");
 }
 
 static void do_reboot(void)
@@ -250,7 +256,29 @@ static void shutdown_handler(struct xenbus_watch *watch,
 
 	kfree(str);
 }
+/*
+ * This function is called when the system is being rebooted.
+ */
+static int
+xxen_system_reboot(struct notifier_block *nb, unsigned long event, void *unused)
+{
+	switch (event) {
+	case SYS_RESTART:
+	case SYS_HALT:
+	case SYS_POWER_OFF:
+	default:
+		mutex_lock(&xenwatch_mutex);
+		shutting_down = SHUTDOWN_POWEROFF;
+		mutex_unlock(&xenwatch_mutex);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
 
+static struct notifier_block xen_shutdown_notifier = {
+        .notifier_call = xen_system_reboot,
+};
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handler(struct xenbus_watch *watch, const char **vec,
 			  unsigned int len)
@@ -308,7 +336,11 @@ static int setup_shutdown_watcher(void)
 		return err;
 	}
 #endif
-
+	err = register_reboot_notifier(&xen_shutdown_notifier);
+	if (err) {
+		pr_warn("Failed to register shutdown notifier\n");
+		return err;
+	}
 	return 0;
 }
 
diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index b6d5fff..ac25752 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -122,7 +122,7 @@ static DEFINE_SPINLOCK(watch_events_lock);
  * carrying out work.
  */
 static pid_t xenwatch_pid;
-static DEFINE_MUTEX(xenwatch_mutex);
+DEFINE_MUTEX(xenwatch_mutex);
 static DECLARE_WAIT_QUEUE_HEAD(watch_events_waitq);
 
 static int get_error(const char *errorstring)
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 40abaf6..57b3370 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -125,7 +125,7 @@ struct xenbus_transaction
 {
 	u32 id;
 };
-
+extern struct mutex xenwatch_mutex;
 /* Nil transaction ID. */
 #define XBT_NIL ((struct xenbus_transaction) { 0 })