Re: [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
On Tue, Feb 11, 2020 at 4:57 PM Christophe Leroy
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
> > ---
> > 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 0x10c7
> > #define PPC_INST_VCMPEQUB 0x1006
> >
> > +/* 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
Re: [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
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
---
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 0x10c7
#define PPC_INST_VCMPEQUB 0x1006
+/* 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' ?
+#definePPC_NO_SUFFIX 0
+#definePPC_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_LDLEFT4 /* 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
[PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
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
---
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 0x10c7
#define PPC_INST_VCMPEQUB 0x1006
+/* macro to check if a word is a prefix */
+#define IS_PREFIX(x) (((x) >> 26) == 1)
+#definePPC_NO_SUFFIX 0
+#definePPC_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)
#define MKOP(t, f, s) ((t) | (f) | SIZE(s))
@@ -132,7 +136,7 @@ union vsx_reg {
* otherwise.
*/
extern int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
-unsigned int instr);
+unsigned int instr, unsigned int suffix);
/*
* Emulate an instruction that can be executed just by updating
@@ -149,7 +153,8 @@ void emulate_update_regs(struct pt_regs *reg, struct
instruction_op *op);
* 0 if it could not be emulated, or -1 for an instruction that
* should not be emulated (rfid, mtmsrd clearing MSR_RI, etc.).
*/
-extern int emulate_step(struct pt_regs *regs, unsigned int instr);
+extern int emulate_step(struct pt_regs *regs, unsigned int instr,
+ unsigned int suffix);
/*
* Emulate a load or store instruction by reading/writing the
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 92045ed64976..ba3bf5c3ab62 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -334,7 +334,7 @@ int fix_alignment(struct pt_regs *regs)
if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
return -EIO;
- r = analyse_instr(, regs, instr);
+ r = analyse_instr(, regs, instr, PPC_NO_SUFFIX);
if (r < 0)
return -EINVAL;
diff --git a/arch/powerpc/kernel/hw_breakpoint.c
b/arch/powerpc/kernel/hw_breakpoint.c
