[PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions

[Christophe Leroy]

Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> Currently all instructions are a single word long. A future ISA version
> will include prefixed instructions which have a double word length. The
> functions used for analysing and emulating instructions need to be
> modified so that they can handle these new instruction types.
> 
> A prefixed instruction is a word prefix followed by a word suffix. All
> prefixes uniquely have the primary op-code 1. Suffixes may be valid word
> instructions or instructions that only exist as suffixes.
> 
> In handling prefixed instructions it will be convenient to treat the
> suffix and prefix as separate words. To facilitate this modify
> analyse_instr() and emulate_step() to take a suffix as a
> parameter. For word instructions it does not matter what is passed in
> here - it will be ignored.
> 
> We also define a new flag, PREFIXED, to be used in instruction_op:type.
> This flag will indicate when emulating an analysed instruction if the
> NIP should be advanced by word length or double word length.
> 
> The callers of analyse_instr() and emulate_step() will need their own
> changes to be able to support prefixed instructions. For now modify them
> to pass in 0 as a suffix.
> 
> Note that at this point no prefixed instructions are emulated or
> analysed - this is just making it possible to do so.
> 
> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> ---
> v2: - Move definition of __get_user_instr() and
> __get_user_instr_inatomic() to "powerpc: Support prefixed instructions
> in alignment handler."
>      - Use a macro for returning the length of an op
>      - Rename sufx -> suffix
>      - Define and use PPC_NO_SUFFIX instead of 0
> ---
>   arch/powerpc/include/asm/ppc-opcode.h |  5 +++++
>   arch/powerpc/include/asm/sstep.h      |  9 ++++++--
>   arch/powerpc/kernel/align.c           |  2 +-
>   arch/powerpc/kernel/hw_breakpoint.c   |  4 ++--
>   arch/powerpc/kernel/kprobes.c         |  2 +-
>   arch/powerpc/kernel/mce_power.c       |  2 +-
>   arch/powerpc/kernel/optprobes.c       |  3 ++-
>   arch/powerpc/kernel/uprobes.c         |  2 +-
>   arch/powerpc/kvm/emulate_loadstore.c  |  2 +-
>   arch/powerpc/lib/sstep.c              | 12 ++++++-----
>   arch/powerpc/lib/test_emulate_step.c  | 30 +++++++++++++--------------
>   arch/powerpc/xmon/xmon.c              |  5 +++--
>   12 files changed, 46 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index c1df75edde44..72783bc92e50 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -377,6 +377,11 @@
>   #define PPC_INST_VCMPEQUD		0x100000c7
>   #define PPC_INST_VCMPEQUB		0x10000006
>   
> +/* macro to check if a word is a prefix */
> +#define IS_PREFIX(x)	(((x) >> 26) == 1)

Can you add an OP_PREFIX in the OP list and use it instead of '1' ?

> +#define	PPC_NO_SUFFIX	0
> +#define	PPC_INST_LENGTH(x)	(IS_PREFIX(x) ? 8 : 4)
> +
>   /* macros to insert fields into opcodes */
>   #define ___PPC_RA(a)	(((a) & 0x1f) << 16)
>   #define ___PPC_RB(b)	(((b) & 0x1f) << 11)
> diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> index 769f055509c9..9ea8904a1549 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -89,11 +89,15 @@ enum instruction_type {
>   #define VSX_LDLEFT	4	/* load VSX register from left */
>   #define VSX_CHECK_VEC	8	/* check MSR_VEC not MSR_VSX for reg >= 32 */
>   
> +/* Prefixed flag, ORed in with type */
> +#define PREFIXED	0x800
> +
>   /* Size field in type word */
>   #define SIZE(n)		((n) << 12)
>   #define GETSIZE(w)	((w) >> 12)
>   
>   #define GETTYPE(t)	((t) & INSTR_TYPE_MASK)
> +#define OP_LENGTH(t)	(((t) & PREFIXED) ? 8 : 4)

Is it worth naming it OP_LENGTH ? Can't it be mistaken as one of the 
OP_xxx from the list in asm/opcode.h ?

What about GETLENGTH() instead to be consistant with the above lines ?

Christophe