Re: [Xen-devel] Early ACPI events prevent subsequent ACPI functionality on xen 4.3 + HVM domU

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

> > @@ -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/?

> 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?

> 
> 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 })
>  
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel