Re: [PATCH v2 05/15] powerpc/uaccess: Move get_user_instr helpers in asm/inst.h

[Daniel Axtens]

Hi Christophe,

> Those helpers use get_user helpers but they don't participate
> in their implementation, so they do not belong to asm/uaccess.h
>
> Move them in asm/inst.h

Hmm, is asm/inst.h the right place for this?

asm/inst.h seems to be entirely concerned with the ppc_inst type:
converting things to and from ppc_inst, print ppc_inst as a string,
dealing with prefixed instructs, etc., etc. The only things currently
that look at memory are the probe_user_read_inst and
probe_kernel_read_inst prototypes...

Having said that, I'm not sure quite where else to put it, and none of
the other places in arch/powerpc/include that currently reference
ppc_inst seem any better...

If we do use asm/inst.h, I think maybe it makes sense to put the
code towards the end rather than at the start, as uses structs and calls
macros that are defined later on in the function.

Kind regards,
Daniel

>
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> ---
>  arch/powerpc/include/asm/inst.h    | 34 ++++++++++++++++++++++++++++++
>  arch/powerpc/include/asm/uaccess.h | 34 ------------------------------
>  2 files changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> index cc73c1267572..19e18af2fac9 100644
> --- a/arch/powerpc/include/asm/inst.h
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -4,6 +4,40 @@
>  
>  #include <asm/ppc-opcode.h>
>  
> +#ifdef CONFIG_PPC64
> +
> +#define ___get_user_instr(gu_op, dest, ptr)				\
> +({									\
> +	long __gui_ret = 0;						\
> +	unsigned long __gui_ptr = (unsigned long)ptr;			\
> +	struct ppc_inst __gui_inst;					\
> +	unsigned int __prefix, __suffix;				\
> +	__gui_ret = gu_op(__prefix, (unsigned int __user *)__gui_ptr);	\
> +	if (__gui_ret == 0) {						\
> +		if ((__prefix >> 26) == OP_PREFIX) {			\
> +			__gui_ret = gu_op(__suffix,			\
> +				(unsigned int __user *)__gui_ptr + 1);	\
> +			__gui_inst = ppc_inst_prefix(__prefix,		\
> +						     __suffix);		\
> +		} else {						\
> +			__gui_inst = ppc_inst(__prefix);		\
> +		}							\
> +		if (__gui_ret == 0)					\
> +			(dest) = __gui_inst;				\
> +	}								\
> +	__gui_ret;							\
> +})
> +#else /* !CONFIG_PPC64 */
> +#define ___get_user_instr(gu_op, dest, ptr)				\
> +	gu_op((dest).val, (u32 __user *)(ptr))
> +#endif /* CONFIG_PPC64 */
> +
> +#define get_user_instr(x, ptr) \
> +	___get_user_instr(get_user, x, ptr)
> +
> +#define __get_user_instr(x, ptr) \
> +	___get_user_instr(__get_user, x, ptr)
> +
>  /*
>   * Instruction data type for POWER
>   */
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 01aea0df4dd0..eaa828a6a419 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -53,40 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>  #define __put_user(x, ptr) \
>  	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>  
> -#ifdef CONFIG_PPC64
> -
> -#define ___get_user_instr(gu_op, dest, ptr)				\
> -({									\
> -	long __gui_ret = 0;						\
> -	unsigned long __gui_ptr = (unsigned long)ptr;			\
> -	struct ppc_inst __gui_inst;					\
> -	unsigned int __prefix, __suffix;				\
> -	__gui_ret = gu_op(__prefix, (unsigned int __user *)__gui_ptr);	\
> -	if (__gui_ret == 0) {						\
> -		if ((__prefix >> 26) == OP_PREFIX) {			\
> -			__gui_ret = gu_op(__suffix,			\
> -				(unsigned int __user *)__gui_ptr + 1);	\
> -			__gui_inst = ppc_inst_prefix(__prefix,		\
> -						     __suffix);		\
> -		} else {						\
> -			__gui_inst = ppc_inst(__prefix);		\
> -		}							\
> -		if (__gui_ret == 0)					\
> -			(dest) = __gui_inst;				\
> -	}								\
> -	__gui_ret;							\
> -})
> -#else /* !CONFIG_PPC64 */
> -#define ___get_user_instr(gu_op, dest, ptr)				\
> -	gu_op((dest).val, (u32 __user *)(ptr))
> -#endif /* CONFIG_PPC64 */
> -
> -#define get_user_instr(x, ptr) \
> -	___get_user_instr(get_user, x, ptr)
> -
> -#define __get_user_instr(x, ptr) \
> -	___get_user_instr(__get_user, x, ptr)
> -
>  extern long __put_user_bad(void);
>  
>  #define __put_user_size(x, ptr, size, retval)			\
> -- 
> 2.25.0