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

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

On Thu, Nov 07, 2013 at 11:24:45AM +0000, Ian Campbell wrote:
> > > @@ -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.
> 
> Right.
> 
> > 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.
> 
> Yes.
> 
> > 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");
> 
> (nb: switch() ;-)). I would also avoiding saying xl since it may not be
> true. "Ignoring Xen toolstack shutdown" or something
> 
> > But then 'system_state' is not guarded by a spinlock or such. Thought
> > it is guarded by the xenwatch mutex.
> 
> system_state is a core global though, so it must surely also be touched
> outside of xen code and therefore outside of xenwatch mutex.
> 
> maybe you meant s/system_state/shutting_down/?

I think I meant shutting_down. Which is a guard variable we use
to guard against calling the orderly_poweroff multiple times.

But then in my mind the system_state and shutting_down melted
in one.

I blame the sleep deprevation on that.

> 
> > Perhaps to be extra safe we should add ourselves to the
> > register_reboot_notifier like so (not compile tested)
> 
> I think this only makes sense if you did mean
> s/system_state/shutting_down/ above, so I'll assume that to be the case.
> 
> It's a shame this has to expose the watch mutex outside of the core xs
> code. Perhaps the core code could add the notifier itself and in turn
> call a manage.c notification function with the lock already held?

We could also make the 'shutting_down' be an atomic. That way
it will always have the correct value and we don't have to depend on
mutexes.

And then we won't go in the orderly_shutdown when a 'poweroff'
has been done from user-space. Problem solved :-)

We will still need the notifier naturally (in the manage.c code).
> 
> > 
> > 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 })
> >  
> > 
> 
>