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

[Jordan Niethe <jniethe5@gmail.com>]

On Tue, Feb 11, 2020 at 4:57 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> 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@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' ?
Will do.
>
> > +#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 ?
Good point, will do.
>
> Christophe