Subject: Re: [PATCH v7 08/28] powerpc: Use a function for getting the instruction op code

From: Alistair Popple <[email-masked]>
Date: Mon, 04 May 2020 18:01:14 +1000

Looks good to me in that it doesn't look to change the behaviour of any 
existing code.

Reviewed-by: Alistair Popple <[email-masked]>

On Friday, 1 May 2020 1:42:00 PM AEST Jordan Niethe wrote:
> In preparation for using a data type for instructions that can not be
> directly used with the '>>' operator use a function for getting the op
> code of an instruction.
> 
> Signed-off-by: Jordan Niethe <[email-masked]>
> ---
> v4: New to series
> v6: - Rename ppc_inst_primary() to ppc_inst_primary_opcode()
>     - Use in vecemu.c, fault.c, sstep.c
>     - Move this patch after the ppc_inst_val() patch
> ---
>  arch/powerpc/include/asm/inst.h  | 5 +++++
>  arch/powerpc/kernel/align.c      | 2 +-
>  arch/powerpc/kernel/vecemu.c     | 3 ++-
>  arch/powerpc/lib/code-patching.c | 4 ++--
>  arch/powerpc/lib/sstep.c         | 2 +-
>  arch/powerpc/mm/fault.c          | 3 ++-
>  6 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/inst.h
> b/arch/powerpc/include/asm/inst.h index 8a9e73bfbd27..442a95f20de7 100644
> --- a/arch/powerpc/include/asm/inst.h
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -13,4 +13,9 @@ static inline u32 ppc_inst_val(u32 x)
>  	return x;
>  }
> 
> +static inline int ppc_inst_primary_opcode(u32 x)
> +{
> +	return ppc_inst_val(x) >> 26;
> +}
> +
>  #endif /* _ASM_INST_H */
> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> index 44921001f84a..47dbba81a227 100644
> --- a/arch/powerpc/kernel/align.c
> +++ b/arch/powerpc/kernel/align.c
> @@ -314,7 +314,7 @@ int fix_alignment(struct pt_regs *regs)
>  	}
> 
>  #ifdef CONFIG_SPE
> -	if ((ppc_inst_val(instr) >> 26) == 0x4) {
> +	if (ppc_inst_primary_opcode(instr) == 0x4) {
>  		int reg = (ppc_inst_val(instr) >> 21) & 0x1f;
>  		PPC_WARN_ALIGNMENT(spe, regs);
>  		return emulate_spe(regs, reg, instr);
> diff --git a/arch/powerpc/kernel/vecemu.c b/arch/powerpc/kernel/vecemu.c
> index 1f5e3b4c8ae4..a544590b90e5 100644
> --- a/arch/powerpc/kernel/vecemu.c
> +++ b/arch/powerpc/kernel/vecemu.c
> @@ -10,6 +10,7 @@
>  #include <asm/processor.h>
>  #include <asm/switch_to.h>
>  #include <linux/uaccess.h>
> +#include <asm/inst.h>
> 
>  /* Functions in vector.S */
>  extern void vaddfp(vector128 *dst, vector128 *a, vector128 *b);
> @@ -268,7 +269,7 @@ int emulate_altivec(struct pt_regs *regs)
>  		return -EFAULT;
> 
>  	word = ppc_inst_val(instr);
> -	if ((word >> 26) != 4)
> +	if (ppc_inst_primary_opcode(instr) != 4)
>  		return -EINVAL;		/* not an altivec instruction */
>  	vd = (word >> 21) & 0x1f;
>  	va = (word >> 16) & 0x1f;
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c index baa849b1a1f9..f5c6dcbac44b 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -231,7 +231,7 @@ bool is_offset_in_branch_range(long offset)
>   */
>  bool is_conditional_branch(unsigned int instr)
>  {
> -	unsigned int opcode = instr >> 26;
> +	unsigned int opcode = ppc_inst_primary_opcode(instr);
> 
>  	if (opcode == 16)       /* bc, bca, bcl, bcla */
>  		return true;
> @@ -289,7 +289,7 @@ int create_cond_branch(unsigned int *instr, const
> unsigned int *addr,
> 
>  static unsigned int branch_opcode(unsigned int instr)
>  {
> -	return (instr >> 26) & 0x3F;
> +	return ppc_inst_primary_opcode(instr) & 0x3F;
>  }
> 
>  static int instr_is_branch_iform(unsigned int instr)
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 14c93ee4ffc8..7f7be154da7e 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1175,7 +1175,7 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs, word = ppc_inst_val(instr);
>  	op->type = COMPUTE;
> 
> -	opcode = instr >> 26;
> +	opcode = ppc_inst_primary_opcode(instr);
>  	switch (opcode) {
>  	case 16:	/* bc */
>  		op->type = BRANCH;
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 9364921870df..0e7e145d5cad 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -41,6 +41,7 @@
>  #include <asm/siginfo.h>
>  #include <asm/debug.h>
>  #include <asm/kup.h>
> +#include <asm/inst.h>
> 
>  /*
>   * Check whether the instruction inst is a store using
> @@ -52,7 +53,7 @@ static bool store_updates_sp(unsigned int inst)
>  	if (((ppc_inst_val(inst) >> 16) & 0x1f) != 1)
>  		return false;
>  	/* check major opcode */
> -	switch (inst >> 26) {
> +	switch (ppc_inst_primary_opcode(inst)) {
>  	case OP_STWU:
>  	case OP_STBU:
>  	case OP_STHU: