Re: [RFC/PATCH] Fix rtas_ibm_suspend_me bugs

[jschopp <jschopp@austin.ibm.com>]

 > -		for_each_possible_cpu(i)
 > -			plpar_hcall_norets(H_PROD,i);

...

 > +	for_each_online_cpu(i)
 > +		plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(i));

I assume this bit would be non-contriversial and could be sent up for 
immediate upstream inclusion.

Nathan Lynch wrote:
> (very rfc for now, no sign-off, needs more testing)
> 
> There are a couple of bugs in the rtas_ibm_suspend_me() and
> rtas_percpu_suspend_me() functions:
> 
> 1.  rtas_ibm_suspend_me() uses on_each_cpu() to invoke
> rtas_percpu_suspend_me() via IPI:
> 
> if (on_each_cpu(rtas_percpu_suspend_me, &data, 1, 0))
> ...
> 
> 'data' is on the stack, and rtas_ibm_suspend_me() takes no measures to
> ensure that all instances of rtas_percpu_suspend_me() are finished
> accessing 'data' before returning.  This can result in the IPI'd cpus
> accessing random stack data and getting stuck in H_JOIN.
> 
> Fix this by moving rtas_suspend_me_data off the stack, and protect it
> with a mutex.  Use a completion to ensure that all cpus are done
> accessing the data before unlocking.
> 
> 2.  rtas_percpu_suspend_me passes the Linux logical cpu id to the
> H_PROD hypervisor method, when it should be using the platform
> interrupt server value for that cpu (hard_smp_processor_id).  In
> practice, this probably causes problems only on partitions where
> processors have been removed and added in a particular order.
> 
> ---
>  arch/powerpc/kernel/rtas.c |   64 ++++++++++++++++++++++++++++++++-----------
>  1 files changed, 47 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 2147807..24faaea 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -19,6 +19,9 @@
>  #include <linux/init.h>
>  #include <linux/capability.h>
>  #include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/smp.h>
>  
>  #include <asm/prom.h>
>  #include <asm/rtas.h>
> @@ -34,6 +37,7 @@
>  #include <asm/lmb.h>
>  #include <asm/udbg.h>
>  #include <asm/syscalls.h>
> +#include <asm/atomic.h>
>  
>  struct rtas_t rtas = {
>  	.lock = SPIN_LOCK_UNLOCKED
> @@ -41,10 +45,21 @@ struct rtas_t rtas = {
>  EXPORT_SYMBOL(rtas);
>  
>  struct rtas_suspend_me_data {
> +	atomic_t working; /* number of cpus accessing rtas_suspend_me_data */
>  	long waiting;
>  	struct rtas_args *args;
> +	struct completion done; /* wait on this until working == 0 */
>  };
>  
> +static void rtas_suspend_me_data_init(struct rtas_suspend_me_data *rsmd,
> +				      struct rtas_args *args)
> +{
> +	atomic_set(&rsmd->working, 0);
> +	rsmd->waiting = 1;
> +	rsmd->args = args;
> +	init_completion(&rsmd->done);
> +}
> +
>  DEFINE_SPINLOCK(rtas_data_buf_lock);
>  EXPORT_SYMBOL(rtas_data_buf_lock);
>  
> @@ -671,36 +686,49 @@ static void rtas_percpu_suspend_me(void *info)
>  	 * we set it to <0.
>  	 */
>  	local_irq_save(flags);
> +	atomic_inc(&data->working);
>  	do {
>  		rc = plpar_hcall_norets(H_JOIN);
>  		smp_rmb();
>  	} while (rc == H_SUCCESS && data->waiting > 0);
>  	if (rc == H_SUCCESS)
> +		/* join is complete (or there was an error) and this
> +		 * cpu was prodded
> +		 */
>  		goto out;
>  
>  	if (rc == H_CONTINUE) {
> +		/* this cpu does the join */
>  		data->waiting = 0;
>  		data->args->args[data->args->nargs] =
>  			rtas_call(ibm_suspend_me_token, 0, 1, NULL);
> -		for_each_possible_cpu(i)
> -			plpar_hcall_norets(H_PROD,i);
>  	} else {
>  		data->waiting = -EBUSY;
>  		printk(KERN_ERR "Error on H_JOIN hypervisor call\n");
>  	}
>  
> +	/* This cpu did the join or got an error, so we need to prod
> +	 * everyone else.  Extra prods are harmless.
> +	 */
> +	for_each_online_cpu(i)
> +		plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(i));
> +
>  out:
> +	if (atomic_dec_return(&data->working) == 0)
> +		complete(&data->done);
>  	local_irq_restore(flags);
>  	return;
>  }
>  
> +static DEFINE_MUTEX(rsm_lock); /* protects rsm_data */
> +static struct rtas_suspend_me_data rsm_data;
> +
>  static int rtas_ibm_suspend_me(struct rtas_args *args)
>  {
> -	int i;
> +	int err;
>  	long state;
>  	long rc;
>  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
> -	struct rtas_suspend_me_data data;
>  
>  	/* Make sure the state is valid */
>  	rc = plpar_hcall(H_VASI_STATE, retbuf,
> @@ -721,25 +749,27 @@ static int rtas_ibm_suspend_me(struct rtas_args *args)
>  		return 0;
>  	}
>  
> -	data.waiting = 1;
> -	data.args = args;
> +	mutex_lock(&rsm_lock);
> +
> +	rtas_suspend_me_data_init(&rsm_data, args);
>  
> -	/* Call function on all CPUs.  One of us will make the
> -	 * rtas call
> +	/* Call function on all CPUs.  One of us (but not necessarily
> +	 * this one) will make the ibm,suspend-me call.
>  	 */
> -	if (on_each_cpu(rtas_percpu_suspend_me, &data, 1, 0))
> -		data.waiting = -EINVAL;
> +	if (on_each_cpu(rtas_percpu_suspend_me, &rsm_data, 1, 0))
> +		rsm_data.waiting = -EINVAL;
> +
> +	/* Must wait for all IPIs to complete before unlocking */
> +	wait_for_completion(&rsm_data.done);
>  
> -	if (data.waiting != 0)
> +	if (rsm_data.waiting != 0)
>  		printk(KERN_ERR "Error doing global join\n");
>  
> -	/* Prod each CPU.  This won't hurt, and will wake
> -	 * anyone we successfully put to sleep with H_JOIN.
> -	 */
> -	for_each_possible_cpu(i)
> -		plpar_hcall_norets(H_PROD, i);
> +	err = rsm_data.waiting;
> +
> +	mutex_unlock(&rsm_lock);
>  
> -	return data.waiting;
> +	return err;
>  }
>  #else /* CONFIG_PPC_PSERIES */
>  static int rtas_ibm_suspend_me(struct rtas_args *args)