Re: [PATCH V3 2/3] powerpc, ptrace: Enable support for transactional memory register sets

[Sam Bobroff]

On 24/05/14 01:15, Anshuman Khandual wrote:
> This patch enables get and set of transactional memory related register
> sets through PTRACE_GETREGSET/PTRACE_SETREGSET interface by implementing
> four new powerpc specific register sets i.e REGSET_TM_SPR, REGSET_TM_CGPR,
> REGSET_TM_CFPR, REGSET_CVMX support corresponding to these following new
> ELF core note types added previously in this regard.
> 
> 	(1) NT_PPC_TM_SPR
> 	(2) NT_PPC_TM_CGPR
> 	(3) NT_PPC_TM_CFPR
> 	(4) NT_PPC_TM_CVMX
> 
> Signed-off-by: Anshuman Khandual <khandual@xxxxxxxxxxxxxxxxxx>

Hi Anshuman,

I'm not Ben but I've reviewed your patch as well as I can and I have
some comments that might be useful to you.

First of all, I couldn't get this to compile without CONFIG_VSX and
CONFIG_PPC_TRANSACTIONAL_MEM defined: there are obvious typos ("esle"
instead of "else") and references to fields that aren't defined for
those cases. I haven't mentioned any of those issues below as the
compiler will do that but you should definitely test those configurations.

Also some of the code seems to assume that if CONFIG_VSX is defined then
CONFIG_PPC_TRANSACTIONAL_MEM must also be defined, but that isn't the
case (it's the other way round: CONFIG_PPC_TRANSACTIONAL_MEM implies
CONFIG_VSX).

> ---
>  arch/powerpc/include/asm/switch_to.h |   8 +
>  arch/powerpc/kernel/process.c        |  24 ++
>  arch/powerpc/kernel/ptrace.c         | 792 +++++++++++++++++++++++++++++++++--
>  3 files changed, 795 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
> index 0e83e7d..2737f46 100644
> --- a/arch/powerpc/include/asm/switch_to.h
> +++ b/arch/powerpc/include/asm/switch_to.h
> @@ -80,6 +80,14 @@ static inline void flush_spe_to_thread(struct task_struct *t)
>  }
>  #endif
>  
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +extern void flush_tmregs_to_thread(struct task_struct *);
> +#else
> +static inline void flush_tmregs_to_thread(struct task_struct *t)
> +{
> +}
> +#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> +
>  static inline void clear_task_ebb(struct task_struct *t)
>  {
>  #ifdef CONFIG_PPC_BOOK3S_64
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 31d0215..e247898 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -695,6 +695,30 @@ static inline void __switch_to_tm(struct task_struct *prev)
>  	}
>  }
>  
> +void flush_tmregs_to_thread(struct task_struct *tsk)
> +{
> +	/*
> +	 * If task is not current, it should have been flushed
> +	 * already to it's thread_struct during __switch_to().
> +	 */
> +	if (tsk != current)
> +		return;
> +
> +	preempt_disable();
> +	if (tsk->thread.regs) {
> +		/*
> +		 * If we are still current, the TM state need to
> +		 * be flushed to thread_struct as it will be still
> +		 * present in the current cpu.
> +		 */
> +		if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
> +			__switch_to_tm(tsk);
> +			tm_recheckpoint_new_task(tsk);

There is at least one other usage of this pair of calls in order to
"flush" the TM state (in arch_dup_task_struct()), so rather than copying
it you might want to create a new function and call it from both places.
(And include the nice comment from arch_dup_task_struct() that explains
how it works and why.)

> +		}
> +	}
> +	preempt_enable();
> +}
> +
>  /*
>   * This is called if we are on the way out to userspace and the
>   * TIF_RESTORE_TM flag is set.  It checks if we need to reload
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 2e3d2bf..17642ef 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -357,6 +357,17 @@ static int gpr_set(struct task_struct *target, const struct user_regset *regset,
>  	return ret;
>  }
>  
> +/*
> + * When any transaction is active, "thread_struct->transact_fp" holds
> + * the current running value of all FPR registers and "thread_struct->
> + * fp_state" holds the last checkpointed FPR registers state for the
> + * current transaction.
> + *
> + * struct data {
> + * 	u64	fpr[32];
> + * 	u64	fpscr;
> + * };

It would be nice to say why you've included "struct data" in the comment.

> + */
>  static int fpr_get(struct task_struct *target, const struct user_regset *regset,
>  		   unsigned int pos, unsigned int count,
>  		   void *kbuf, void __user *ubuf)
> @@ -365,21 +376,41 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
>  	u64 buf[33];
>  	int i;
>  #endif
> -	flush_fp_to_thread(target);
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
> +		flush_fp_to_thread(target);
> +		flush_altivec_to_thread(target);
> +		flush_tmregs_to_thread(target);
> +	} else {
> +		flush_fp_to_thread(target);
> +	}

I don't see why you need the else. Could this be:

	flush_fp_to_thread(target);
	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
		flush_altivec_to_thread(target);
		flush_tmregs_to_thread(target);
	}

>  
>  #ifdef CONFIG_VSX
>  	/* copy to local buffer then write that out */
> -	for (i = 0; i < 32 ; i++)
> -		buf[i] = target->thread.TS_FPR(i);
> -	buf[32] = target->thread.fp_state.fpscr;
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
> +		for (i = 0; i < 32 ; i++)
> +			buf[i] = target->thread.TS_TRANS_FPR(i);
> +		buf[32] = target->thread.transact_fp.fpscr;
> +	} else {
> +		for (i = 0; i < 32 ; i++)
> +			buf[i] = target->thread.TS_FPR(i);
> +		buf[32] = target->thread.fp_state.fpscr;
> +	}

I see several cases of similar code needing to access either fp_state or
transact_fp, or other similar pairs, so maybe you could use a macro.
Something like this (I'm not sure about the name!):

#define MABYE_TM(TSK,X,TM_X) \
((MSR_TM_ACTIVE((TSK)->thread.regs->msr) \
? &((TSK)->thread.(TM_X) \
: &((TSK)->thread.(X))

Then you could do this:

	struct thread_fp_state *fp;

	fp = MAYBE_TM(target,fp_state,transact_fp);
	for (i = 0; i < 32; i++)
		buf[i] = (*fp)[i][TS_FPROFFSET];

>  	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, buf, 0, -1);
>  
>  #else
> -	BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
> -		     offsetof(struct thread_fp_state, fpr[32][0]));
> +	if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
> +		BUILD_BUG_ON(offsetof(struct transact_fp, fpscr) !=
> +				offsetof(struct transact_fp, fpr[32][0]));
>  
> -	return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +		return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				   &target->thread.transact_fp, 0, -1);
> +	} esle {
> +		BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
> +			     offsetof(struct thread_fp_state, fpr[32][0]));
> +
> +		return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>  				   &target->thread.fp_state, 0, -1);
> +	}

The BUILD_BUG_ON() statements don't need to be in the "if", since
they're compile-time (and "struct transact_fp" is not a type so that one
isn't needed), and you could use the utility function (above) to shorten
this a lot, i.e.:
	BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
		     offsetof(struct thread_fp_state, fpr[32][0]));
	return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
				  MAYBE_TM(target,fp_state,transact_fp)
				   , 0, -1);

>  #endif
>  }
>  
> @@ -391,23 +422,44 @@ static int fpr_set(struct task_struct *target, const struct user_regset *regset,
>  	u64 buf[33];
>  	int i;
>  #endif
> -	flush_fp_to_thread(target);
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
> +		flush_fp_to_thread(target);
> +		flush_altivec_to_thread(target);
> +		flush_tmregs_to_thread(target);
> +	} else {
> +		flush_fp_to_thread(target);
> +	}

This could be simplified like the similar case above.

>  #ifdef CONFIG_VSX
>  	/* copy to local buffer then write that out */
>  	i = user_regset_copyin(&pos, &count, &kbuf, &ubuf, buf, 0, -1);
>  	if (i)
>  		return i;
> -	for (i = 0; i < 32 ; i++)
> -		target->thread.TS_FPR(i) = buf[i];
> -	target->thread.fp_state.fpscr = buf[32];
> +	for (i = 0; i < 32 ; i++) {
> +		if (MSR_TM_ACTIVE(target->thread.regs->msr))
> +			target->thread.TS_TRANS_FPR(i) = buf[i];
> +		else
> +			target->thread.TS_FPR(i) = buf[i];
> +	}
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr))
> +		target->thread.transact_fp.fpscr = buf[32];
> +	else
> +		target->thread.fp_state.fpscr = buf[32];
>  	return 0;

This could be shorted using the above macro.

>  #else
> -	BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
> -		     offsetof(struct thread_fp_state, fpr[32][0]));
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
> +		BUILD_BUG_ON(offsetof(struct transact_fp, fpscr) !=
> +			     offsetof(struct transact_fp, fpr[32][0]));
>  
> -	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> -				  &target->thread.fp_state, 0, -1);
> +		return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +					  &target->thread.transact_fp, 0, -1);
> +	} else {
> +		BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
> +			     offsetof(struct thread_fp_state, fpr[32][0]));
> +
> +		return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +				&target->thread.fp_state, 0, -1);
> +	}
>  #endif

... as could this.

>  }
>  
> @@ -432,20 +484,44 @@ static int vr_active(struct task_struct *target,
>  	return target->thread.used_vr ? regset->n : 0;
>  }
>  
> +/*
> + * When any transaction is active, "thread_struct->transact_vr" holds
> + * the current running value of all VMX registers and "thread_struct->
> + * vr_state" holds the last checkpointed value of VMX registers for the
> + * current transaction.
> + *
> + * struct data {
> + * 	vector128	vr[32];
> + * 	vector128	vscr;
> + * 	vector128	vrsave;
> + * };
> + */

Again a comment about "struct data" would be nice.

>  static int vr_get(struct task_struct *target, const struct user_regset *regset,
>  		  unsigned int pos, unsigned int count,
>  		  void *kbuf, void __user *ubuf)
>  {
>  	int ret;
> +	struct thread_vr_state *addr;
>  
> -	flush_altivec_to_thread(target);
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
> +		flush_fp_to_thread(target);
> +		flush_altivec_to_thread(target);
> +		flush_tmregs_to_thread(target);
> +	} else {
> +		flush_altivec_to_thread(target);
> +	}

As above.

>  
>  	BUILD_BUG_ON(offsetof(struct thread_vr_state, vscr) !=
>  		     offsetof(struct thread_vr_state, vr[32]));
>  
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr))
> +		addr = &target->thread.transact_vr;
> +	else
> +		addr = &target->thread.vr_state;
> +
>  	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> -				  &target->thread.vr_state, 0,
> -				  33 * sizeof(vector128));
> +				addr, 0, 33 * sizeof(vector128));
> +

As above.

>  	if (!ret) {
>  		/*
>  		 * Copy out only the low-order word of vrsave.
> @@ -455,11 +531,14 @@ static int vr_get(struct task_struct *target, const struct user_regset *regset,
>  			u32 word;
>  		} vrsave;
>  		memset(&vrsave, 0, sizeof(vrsave));
> -		vrsave.word = target->thread.vrsave;
> +		if (MSR_TM_ACTIVE(target->thread.regs->msr))
> +			vrsave.word = target->thread.transact_vrsave;
> +		else
> +			vrsave.word = target->thread.vrsave;
> +

As above.

>  		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &vrsave,
>  					  33 * sizeof(vector128), -1);
>  	}
> -
>  	return ret;
>  }
>  
> @@ -467,16 +546,27 @@ static int vr_set(struct task_struct *target, const struct user_regset *regset,
>  		  unsigned int pos, unsigned int count,
>  		  const void *kbuf, const void __user *ubuf)
>  {
> +	struct thread_vr_state *addr;
>  	int ret;
>  
> -	flush_altivec_to_thread(target);
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
> +		flush_fp_to_thread(target);
> +		flush_altivec_to_thread(target);
> +		flush_tmregs_to_thread(target);
> +	} else {
> +		flush_altivec_to_thread(target);
> +	}

Could flush_altivec_to_thread() be pulled out of the "if" or is it
important to call flush_fp_to_thread() first?

>  
>  	BUILD_BUG_ON(offsetof(struct thread_vr_state, vscr) !=
>  		     offsetof(struct thread_vr_state, vr[32]));
>  
> +	if (MSR_TM_ACTIVE(target->thread.regs->msr))
> +		addr = &target->thread.transact_vr;
> +	else
> +		addr = &target->thread.vr_state;
>  	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> -				 &target->thread.vr_state, 0,
> -				 33 * sizeof(vector128));
> +			addr, 0, 33 * sizeof(vector128));
> +

As above, and as a result you could remove "addr".

>  	if (!ret && count > 0) {
>  		/*
>  		 * We use only the first word of vrsave.
> @@ -486,13 +576,21 @@ static int vr_set(struct task_struct *target, const struct user_regset *regset,
>  			u32 word;
>  		} vrsave;
>  		memset(&vrsave, 0, sizeof(vrsave));
> -		vrsave.word = target->thread.vrsave;
> +
> +		if (MSR_TM_ACTIVE(target->thread.regs->msr))
> +			vrsave.word = target->thread.transact_vrsave;
> +		else
> +			vrsave.word = target->thread.vrsave;
> +

As above.

>  		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &vrsave,
>  					 33 * sizeof(vector128), -1);
> -		if (!ret)
> -			target->thread.vrsave = vrsave.word;
> +		if (!ret) {
> +			if (MSR_TM_ACTIVE(target->thread.regs->msr))
> +				target->thread.transact_vrsave = vrsave.word;
> +			else
> +				target->thread.vrsave = vrsave.word;
> +		}

As above.

>  	}
> -
>  	return ret;
>  }
>  #endif /* CONFIG_ALTIVEC */
> @@ -613,6 +711,442 @@ static int evr_set(struct task_struct *target, const struct user_regset *regset,
>  }
>  #endif /* CONFIG_SPE */
>  
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +
> +static int tm_spr_active(struct task_struct *target,
> +				const struct user_regset *regset)
> +{
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return 0;
> +
> +	return regset->n;
> +}
> +/*
> + *  Transactional memory SPR
> + *
> + * struct {
> + * 	u64		tm_tfhar;
> + *	u64		tm_texasr;
> + *	u64		tm_tfiar;
> + *	unsigned long	tm_orig_msr;
> + * 	unsigned long	tm_tar;
> + *	unsigned long	tm_ppr;
> + *	unsigned long	tm_dscr;
> + * };
> + */
> +static int tm_spr_get(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   void *kbuf, void __user *ubuf)
> +{
> +	int ret;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +	/* TFHAR register */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_tfhar, 0, sizeof(u64));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_tfhar) +
> +			sizeof(u64) != offsetof(struct thread_struct, tm_texasr));
> +
> +	/* TEXASR register */
> +	if (!ret)
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_texasr, sizeof(u64), 2 * sizeof(u64));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_texasr) +
> +			sizeof(u64) != offsetof(struct thread_struct, tm_tfiar));
> +
> +	/* TFIAR register */
> +	if (!ret)
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_tfiar, 2 * sizeof(u64), 3 * sizeof(u64));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_tfiar) +
> +			sizeof(u64) != offsetof(struct thread_struct, tm_orig_msr));
> +
> +	/* TM checkpointed original MSR */
> +	if (!ret)
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_orig_msr, 3 * sizeof(u64),
> +				3 * sizeof(u64) + sizeof(unsigned long));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_orig_msr) +
> +			sizeof(unsigned long) + sizeof(struct pt_regs)
> +				!= offsetof(struct thread_struct, tm_tar));
> +
> +	/* TM checkpointed TAR register */
> +	if (!ret)
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_tar, 3 * sizeof(u64) +
> +				sizeof(unsigned long) , 3 * sizeof(u64) +
> +					2 * sizeof(unsigned long));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_tar)
> +			+ sizeof(unsigned long) !=
> +				offsetof(struct thread_struct, tm_ppr));
> +
> +	/* TM checkpointed PPR register */
> +	if (!ret)
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				&target->thread.tm_ppr, 3 * sizeof(u64) +
> +					2 * sizeof(unsigned long), 3 * sizeof(u64) +
> +						3 * sizeof(unsigned long));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_ppr) +
> +			sizeof(unsigned long) !=
> +				offsetof(struct thread_struct, tm_dscr));
> +
> +	/* TM checkpointed DSCR register */
> +	if (!ret)
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_dscr, 3 * sizeof(u64)
> +				+ 3 * sizeof(unsigned long), 3 * sizeof(u64)
> +						+ 4 * sizeof(unsigned long));
> +	return ret;
> +}
> +
> +static int tm_spr_set(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   const void *kbuf, const void __user *ubuf)
> +{
> +	int ret;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +	/* TFHAR register */
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +				&target->thread.tm_tfhar, 0, sizeof(u64));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_tfhar)
> +		+ sizeof(u64) != offsetof(struct thread_struct, tm_texasr));
> +
> +	/* TEXASR register */
> +	if (!ret)
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_texasr, sizeof(u64), 2 * sizeof(u64));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_texasr)
> +		+ sizeof(u64) != offsetof(struct thread_struct, tm_tfiar));
> +
> +	/* TFIAR register */
> +	if (!ret)
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_tfiar, 2 * sizeof(u64), 3 * sizeof(u64));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_tfiar)
> +		+ sizeof(u64) != offsetof(struct thread_struct, tm_orig_msr));
> +
> +	/* TM checkpointed orig MSR */
> +	if (!ret)
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_orig_msr, 3 * sizeof(u64),
> +				3 * sizeof(u64) + sizeof(unsigned long));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_orig_msr)
> +		+ sizeof(unsigned long) + sizeof(struct pt_regs) !=
> +			offsetof(struct thread_struct, tm_tar));
> +
> +	/* TM checkpointed TAR register */
> +	if (!ret)
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.tm_tar, 3 * sizeof(u64) +
> +				sizeof(unsigned long), 3 * sizeof(u64) +
> +					2 * sizeof(unsigned long));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_tar)
> +			+ sizeof(unsigned long) != offsetof(struct thread_struct, tm_ppr));
> +
> +	/* TM checkpointed PPR register */
> +	if (!ret)
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +				&target->thread.tm_ppr, 3 * sizeof(u64)
> +					+ 2 * sizeof(unsigned long), 3 * sizeof(u64)
> +					+ 3 * sizeof(unsigned long));
> +
> +	BUILD_BUG_ON(offsetof(struct thread_struct, tm_ppr) +
> +			sizeof(unsigned long) !=
> +				offsetof(struct thread_struct, tm_dscr));
> +
> +	/* TM checkpointed DSCR register */
> +	if (!ret)
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +				&target->thread.tm_dscr,
> +					3 * sizeof(u64) + 3 * sizeof(unsigned long),
> +					3 * sizeof(u64) + 4 * sizeof(unsigned long));
> +
> +	return ret;
> +}

I don't understand why tm_spr_set() and tm_spr_get() are structured like
this. It looks like they're expecting user_regset_copyin() to fail part
of the way through and are being careful to set the registers until that
point, even if it's going to return a failure.

This seems strange because other functions are careful to construct an
intermediate buffer so that they can either succeed or fail entirely. If
there's a reason that this needs to be this way, it might be a good idea
to explain that in a comment.

If they dont need to act like that, wouldn't all the BUILD_BUG_ONs
guarantee that the thread_struct registers are contiguous and therefore
you could set them all with a single call to user_regset_copyin()? If
you don't need them to be contiguous then what are the BUILD_BUG_ONs for?

> +
> +static int tm_cgpr_active(struct task_struct *target,
> +				const struct user_regset *regset)
> +{
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return 0;
> +
> +	return regset->n;
> +}
> +
> +/*
> + * TM Checkpointed GPR
> + *
> + * struct data {
> + * 	struct pt_regs ckpt_regs;
> + * };
> + */
> +static int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   void *kbuf, void __user *ubuf)
> +{
> +	int ret;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);

I see this pattern of checking for TM and calling flush_fp_to_thread(),
flush_altivec_to_thread() and flush_tmregs_to_thread() in many places;
maybe it should be factored out to a function.

> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.ckpt_regs, 0,
> +				sizeof(struct pt_regs));
> +	return ret;
> +}
> +
> +static int tm_cgpr_set(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   const void *kbuf, const void __user *ubuf)
> +{
> +	int ret;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +					&target->thread.ckpt_regs, 0,
> +						sizeof(struct pt_regs));
> +	return ret;
> +}
> +
> +static int tm_cfpr_active(struct task_struct *target,
> +				const struct user_regset *regset)
> +{
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return 0;
> +
> +	return regset->n;
> +}
> +
> +/*
> + * TM Checkpointed FPR
> + *
> + * struct data {
> + * 	u64	fpr[32];
> + * 	u64	fpscr;
> + * };
> + */
> +static int tm_cfpr_get(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   void *kbuf, void __user *ubuf)
> +{
> +#ifdef CONFIG_VSX
> +	u64 buf[33];
> +	int i;
> +#endif
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +#ifdef CONFIG_VSX
> +	/* copy to local buffer then write that out */
> +	for (i = 0; i < 32 ; i++)
> +		buf[i] = target->thread.TS_FPR(i);
> +	buf[32] = target->thread.fp_state.fpscr;
> +	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, buf, 0, -1);
> +
> +#else
> +	BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
> +		offsetof(struct thread_fp_state, fpr[32][0]));
> +
> +	return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +			&target->thread.thread_fp_state, 0, -1);
> +#endif
> +}
> +
> +static int tm_cfpr_set(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   const void *kbuf, const void __user *ubuf)
> +{
> +#ifdef CONFIG_VSX
> +	u64 buf[33];
> +	int i;
> +#endif
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +#ifdef CONFIG_VSX
> +	/* copy to local buffer then write that out */
> +	i = user_regset_copyin(&pos, &count, &kbuf, &ubuf, buf, 0, -1);
> +	if (i)
> +		return i;
> +	for (i = 0; i < 32 ; i++)
> +		target->thread.TS_FPR(i) = buf[i];
> +	target->thread.fp_state.fpscr = buf[32];
> +	return 0;
> +#else
> +	BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
> +		      offsetof(struct thread_fp_state, fpr[32][0]));
> +
> +	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +				&target->thread.fp_state, 0, -1);
> +#endif
> +}
> +
> +static int tm_cvmx_active(struct task_struct *target,
> +				const struct user_regset *regset)
> +{
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return 0;
> +
> +	return regset->n;
> +}
> +
> +/*
> + * TM Checkpointed VMX
> + *
> + * struct data {
> + * 	vector128	vr[32];
> + * 	vector128	vscr;
> + * 	vector128	vrsave;
> + *};
> + */
> +static int tm_cvmx_get(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   void *kbuf, void __user *ubuf)
> +{
> +	int ret;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +	BUILD_BUG_ON(offsetof(struct thread_vr_state, vscr) !=
> +		     offsetof(struct thread_vr_state, vr[32]));
> +
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  &target->thread.vr_state, 0,
> +				  33 * sizeof(vector128));
> +	if (!ret) {
> +		/*
> +		 * Copy out only the low-order word of vrsave.
> +		 */
> +		union {
> +			elf_vrreg_t reg;
> +			u32 word;
> +		} vrsave;
> +		memset(&vrsave, 0, sizeof(vrsave));
> +		vrsave.word = target->thread.vrsave;
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &vrsave,
> +					  33 * sizeof(vector128), -1);

I see this pattern of returning only the low-order word several times.
Maybe it should be factored out, or there is already a function
somewhere to do this (it seems like a fairly generic operation).

> +	}
> +	return ret;
> +}
> +
> +static int tm_cvmx_set(struct task_struct *target, const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   const void *kbuf, const void __user *ubuf)
> +{
> +	int ret;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +	BUILD_BUG_ON(offsetof(struct thread_vr_state, vscr) !=
> +		offsetof(struct thread_vr_state, vr[32]));
> +
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +				 &target->thread.vr_state, 0,
> +				 33 * sizeof(vector128));
> +	if (!ret && count > 0) {
> +		/*
> +		 * We use only the first word of vrsave.
> +		 */
> +		union {
> +			elf_vrreg_t reg;
> +			u32 word;
> +		} vrsave;
> +		memset(&vrsave, 0, sizeof(vrsave));
> +		vrsave.word = target->thread.vrsave;
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &vrsave,
> +					 33 * sizeof(vector128), -1);
> +		if (!ret)
> +			target->thread.vrsave = vrsave.word;
> +	}
> +	return ret;
> +}
> +#endif	/* CONFIG_PPC_TRANSACTIONAL_MEM */
>  
>  /*
>   * These are our native regset flavors.
> @@ -629,6 +1163,12 @@ enum powerpc_regset {
>  #ifdef CONFIG_SPE
>  	REGSET_SPE,
>  #endif
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	REGSET_TM_SPR,		/* TM specific SPR */
> +	REGSET_TM_CGPR,		/* TM checkpointed GPR */
> +	REGSET_TM_CFPR,		/* TM checkpointed FPR */
> +	REGSET_TM_CVMX,		/* TM checkpointed VMX */
> +#endif
>  };
>  
>  static const struct user_regset native_regsets[] = {
> @@ -663,6 +1203,28 @@ static const struct user_regset native_regsets[] = {
>  		.active = evr_active, .get = evr_get, .set = evr_set
>  	},
>  #endif
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	[REGSET_TM_SPR] = {
> +		.core_note_type = NT_PPC_TM_SPR, .n = 7,
> +		.size = sizeof(u64), .align = sizeof(u64),
> +		.active = tm_spr_active, .get = tm_spr_get, .set = tm_spr_set
> +	},
> +	[REGSET_TM_CGPR] = {
> +		.core_note_type = NT_PPC_TM_CGPR, .n = ELF_NGREG,
> +		.size = sizeof(long), .align = sizeof(long),
> +		.active = tm_cgpr_active, .get = tm_cgpr_get, .set = tm_cgpr_set
> +	},
> +	[REGSET_TM_CFPR] = {
> +		.core_note_type = NT_PPC_TM_CFPR, .n = ELF_NFPREG,
> +		.size = sizeof(double), .align = sizeof(double),
> +		.active = tm_cfpr_active, .get = tm_cfpr_get, .set = tm_cfpr_set
> +	},
> +	[REGSET_TM_CVMX] = {
> +		.core_note_type = NT_PPC_TM_CVMX, .n = 34,
> +		.size = sizeof(vector128), .align = sizeof(vector128),
> +		.active = tm_cvmx_active, .get = tm_cvmx_get, .set = tm_cvmx_set
> +	},
> +#endif
>  };
>  
>  static const struct user_regset_view user_ppc_native_view = {
> @@ -690,7 +1252,7 @@ static int gpr32_get(struct task_struct *target,
>  	if (!FULL_REGS(target->thread.regs)) {
>  		/* We have a partial register set.  Fill 14-31 with bogus values */
>  		for (i = 14; i < 32; i++)
> -			target->thread.regs->gpr[i] = NV_REG_POISON; 
> +			target->thread.regs->gpr[i] = NV_REG_POISON;
>  	}
>  
>  	pos /= sizeof(reg);
> @@ -803,6 +1365,157 @@ static int gpr32_set(struct task_struct *target,
>  					 (PT_TRAP + 1) * sizeof(reg), -1);
>  }
>  
> +static int tm_cgpr32_get(struct task_struct *target,
> +		     const struct user_regset *regset,
> +		     unsigned int pos, unsigned int count,
> +		     void *kbuf, void __user *ubuf)
> +{
> +	const unsigned long *regs = &target->thread.ckpt_regs.gpr[0];
> +	compat_ulong_t *k = kbuf;
> +	compat_ulong_t __user *u = ubuf;
> +	compat_ulong_t reg;
> +	int i;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +	if (target->thread.regs == NULL)
> +		return -EIO;
> +
> +	if (!FULL_REGS(target->thread.regs)) {
> +		/* We have a partial register set.  Fill 14-31 with bogus values */
> +		for (i = 14; i < 32; i++)
> +			target->thread.regs->gpr[i] = NV_REG_POISON; 

The line above adds a whitespace error. (Did you run it through
checkpatch.pl?)

> +	}
> +
> +	pos /= sizeof(reg);
> +	count /= sizeof(reg);
> +
> +	if (kbuf)
> +		for (; count > 0 && pos < PT_MSR; --count)
> +			*k++ = regs[pos++];
> +	else
> +		for (; count > 0 && pos < PT_MSR; --count)
> +			if (__put_user((compat_ulong_t) regs[pos++], u++))
> +				return -EFAULT;
> +
> +	if (count > 0 && pos == PT_MSR) {
> +		reg = get_user_msr(target);
> +		if (kbuf)
> +			*k++ = reg;
> +		else if (__put_user(reg, u++))
> +			return -EFAULT;
> +		++pos;
> +		--count;
> +	}
> +
> +	if (kbuf)
> +		for (; count > 0 && pos < PT_REGS_COUNT; --count)
> +			*k++ = regs[pos++];
> +	else
> +		for (; count > 0 && pos < PT_REGS_COUNT; --count)
> +			if (__put_user((compat_ulong_t) regs[pos++], u++))
> +				return -EFAULT;
> +
> +	kbuf = k;
> +	ubuf = u;
> +	pos *= sizeof(reg);
> +	count *= sizeof(reg);
> +	return user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
> +					PT_REGS_COUNT * sizeof(reg), -1);
> +}
> +
> +static int tm_cgpr32_set(struct task_struct *target,
> +		     const struct user_regset *regset,
> +		     unsigned int pos, unsigned int count,
> +		     const void *kbuf, const void __user *ubuf)
> +{
> +	unsigned long *regs = &target->thread.ckpt_regs.gpr[0];
> +	const compat_ulong_t *k = kbuf;
> +	const compat_ulong_t __user *u = ubuf;
> +	compat_ulong_t reg;
> +
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if(!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +	flush_tmregs_to_thread(target);
> +
> +	if (target->thread.regs == NULL)
> +		return -EIO;
> +
> +	CHECK_FULL_REGS(target->thread.regs);
> +
> +	pos /= sizeof(reg);
> +	count /= sizeof(reg);
> +
> +	if (kbuf)
> +		for (; count > 0 && pos < PT_MSR; --count)
> +			regs[pos++] = *k++;
> +	else
> +		for (; count > 0 && pos < PT_MSR; --count) {
> +			if (__get_user(reg, u++))
> +				return -EFAULT;
> +			regs[pos++] = reg;
> +		}
> +
> +
> +	if (count > 0 && pos == PT_MSR) {
> +		if (kbuf)
> +			reg = *k++;
> +		else if (__get_user(reg, u++))
> +			return -EFAULT;
> +		set_user_msr(target, reg);
> +		++pos;
> +		--count;
> +	}
> +
> +	if (kbuf) {
> +		for (; count > 0 && pos <= PT_MAX_PUT_REG; --count)
> +			regs[pos++] = *k++;
> +		for (; count > 0 && pos < PT_TRAP; --count, ++pos)
> +			++k;
> +	} else {
> +		for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) {
> +			if (__get_user(reg, u++))
> +				return -EFAULT;
> +			regs[pos++] = reg;
> +		}
> +		for (; count > 0 && pos < PT_TRAP; --count, ++pos)
> +			if (__get_user(reg, u++))
> +				return -EFAULT;
> +	}
> +
> +	if (count > 0 && pos == PT_TRAP) {
> +		if (kbuf)
> +			reg = *k++;
> +		else if (__get_user(reg, u++))
> +			return -EFAULT;
> +		set_user_trap(target, reg);
> +		++pos;
> +		--count;
> +	}
> +
> +	kbuf = k;
> +	ubuf = u;
> +	pos *= sizeof(reg);
> +	count *= sizeof(reg);
> +	return user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
> +					 (PT_TRAP + 1) * sizeof(reg), -1);
> +}
> +
> +
>  /*
>   * These are the regset flavors matching the CONFIG_PPC32 native set.
>   */
> @@ -831,6 +1544,28 @@ static const struct user_regset compat_regsets[] = {
>  		.active = evr_active, .get = evr_get, .set = evr_set
>  	},
>  #endif
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	[REGSET_TM_SPR] = {
> +		.core_note_type = NT_PPC_TM_SPR, .n = 7,
> +		.size = sizeof(u64), .align = sizeof(u64),
> +		.active = tm_spr_active, .get = tm_spr_get, .set = tm_spr_set
> +	},
> +	[REGSET_TM_CGPR] = {
> +		.core_note_type = NT_PPC_TM_CGPR, .n = ELF_NGREG,
> +		.size = sizeof(long), .align = sizeof(long),
> +		.active = tm_cgpr_active, .get = tm_cgpr32_get, .set = tm_cgpr32_set
> +	},
> +	[REGSET_TM_CFPR] = {
> +		.core_note_type = NT_PPC_TM_CFPR, .n = ELF_NFPREG,
> +		.size = sizeof(double), .align = sizeof(double),
> +		.active = tm_cfpr_active, .get = tm_cfpr_get, .set = tm_cfpr_set
> +	},
> +	[REGSET_TM_CVMX] = {
> +		.core_note_type = NT_PPC_TM_CVMX, .n = 34,
> +		.size = sizeof(vector128), .align = sizeof(vector128),
> +		.active = tm_cvmx_active, .get = tm_cvmx_get, .set = tm_cvmx_set
> +	},
> +#endif
>  };
>  
>  static const struct user_regset_view user_ppc_compat_view = {
> @@ -1754,7 +2489,6 @@ long arch_ptrace(struct task_struct *child, long request,
>  					     REGSET_SPE, 0, 35 * sizeof(u32),
>  					     datavp);
>  #endif
> -
>  	default:
>  		ret = ptrace_request(child, request, addr, data);
>  		break;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/