Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
On Mon, Jan 13, 2020 at 5:18 PM Balamuruhan S wrote:
>
> On Tue, Nov 26, 2019 at 04:21:28PM +1100, Jordan Niethe wrote:
> > 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 take a suffix as a
>
> typo - s/take a take a/take a
Thanks for catching this.
>
> > 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
> > ---
> > arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> > arch/powerpc/include/asm/sstep.h | 8 +--
> > arch/powerpc/include/asm/uaccess.h| 30 +++
> > 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 | 2 +-
> > 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 | 4 ++--
> > 13 files changed, 71 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> > b/arch/powerpc/include/asm/ppc-opcode.h
> > index c1df75edde44..a1dfa4bdd22f 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -377,6 +377,9 @@
> > #define PPC_INST_VCMPEQUD0x10c7
> > #define PPC_INST_VCMPEQUB0x1006
> >
> > +/* macro to check if a word is a prefix */
> > +#define IS_PREFIX(x) (((x) >> 26) == 1)
> > +
> > /* 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..6d4cb602e231 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -89,6 +89,9 @@ enum instruction_type {
> > #define VSX_LDLEFT 4 /* load VSX register from left */
> > #define VSX_CHECK_VEC8 /* 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)
> > @@ -132,7 +135,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 sufx);
> >
> > /*
> > * Emulate an instruction that can be executed just by updating
> > @@ -149,7 +152,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 sufx);
> >
> > /*
> > * Emulate a load or store instruction by reading/writing the
> > diff --git a/arch/powerpc/include/asm/uaccess.h
> > b/arch/powerpc/include/asm/uaccess.h
> > index 15002b51ff18..bc585399e0c7 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -423,4 +423,34 @@ extern long __copy_from_user_flushcache(void *dst,
> > const void __user *src,
> > extern void memcpy_page_flushcache(char *to, struct page *page, size_t
> > offset,
> > size_t len);
> >
> > +/*
> > + * When reading an
Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
On Tue, Nov 26, 2019 at 04:21:28PM +1100, Jordan Niethe wrote:
> 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 take a suffix as a
typo - s/take a take a/take 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
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> arch/powerpc/include/asm/sstep.h | 8 +--
> arch/powerpc/include/asm/uaccess.h| 30 +++
> 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 | 2 +-
> 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 | 4 ++--
> 13 files changed, 71 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> b/arch/powerpc/include/asm/ppc-opcode.h
> index c1df75edde44..a1dfa4bdd22f 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -377,6 +377,9 @@
> #define PPC_INST_VCMPEQUD0x10c7
> #define PPC_INST_VCMPEQUB0x1006
>
> +/* macro to check if a word is a prefix */
> +#define IS_PREFIX(x) (((x) >> 26) == 1)
> +
> /* 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..6d4cb602e231 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -89,6 +89,9 @@ enum instruction_type {
> #define VSX_LDLEFT 4 /* load VSX register from left */
> #define VSX_CHECK_VEC8 /* 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)
> @@ -132,7 +135,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 sufx);
>
> /*
> * Emulate an instruction that can be executed just by updating
> @@ -149,7 +152,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 sufx);
>
> /*
> * Emulate a load or store instruction by reading/writing the
> diff --git a/arch/powerpc/include/asm/uaccess.h
> b/arch/powerpc/include/asm/uaccess.h
> index 15002b51ff18..bc585399e0c7 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -423,4 +423,34 @@ extern long __copy_from_user_flushcache(void *dst, const
> void __user *src,
> extern void memcpy_page_flushcache(char *to, struct page *page, size_t
> offset,
> size_t len);
>
> +/*
> + * When reading an instruction iff it is a prefix, the suffix needs to be
> also
> + * loaded.
> + */
> +#define __get_user_instr(x, y, ptr) \
It will be better to have `__get_user_instr()` and
`__get_user_instr_inatomic()` in separate commit or squashed with patch
[PATCH 10/18]
Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
On Fri, Dec 20, 2019 at 4:17 PM Jordan Niethe wrote:
>
> On Thu, Dec 19, 2019 at 1:15 AM Daniel Axtens wrote:
> >
> > Jordan Niethe writes:
> >
> > > 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 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
> > > ---
> > > arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> > > arch/powerpc/include/asm/sstep.h | 8 +--
> > > arch/powerpc/include/asm/uaccess.h| 30 +++
> > > 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 | 2 +-
> > > 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 | 4 ++--
> > > 13 files changed, 71 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> > > b/arch/powerpc/include/asm/ppc-opcode.h
> > > index c1df75edde44..a1dfa4bdd22f 100644
> > > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > > @@ -377,6 +377,9 @@
> > > #define PPC_INST_VCMPEQUD0x10c7
> > > #define PPC_INST_VCMPEQUB0x1006
> > >
> > > +/* macro to check if a word is a prefix */
> > > +#define IS_PREFIX(x) (((x) >> 26) == 1)
> > > +
> > > /* 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..6d4cb602e231 100644
> > > --- a/arch/powerpc/include/asm/sstep.h
> > > +++ b/arch/powerpc/include/asm/sstep.h
> > > @@ -89,6 +89,9 @@ enum instruction_type {
> > > #define VSX_LDLEFT 4 /* load VSX register from left */
> > > #define VSX_CHECK_VEC8 /* 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)
> > > @@ -132,7 +135,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 sufx);
> > >
> >
> > I'm not saying this is necessarily better, but did you consider:
> >
> > - making instr 64 bits and using masking and shifting macros to get the
> >prefix and suffix?
> >
> > - defining an instruction type/struct/union/whatever that contains both
> >halves in one object?
> >
> > I'm happy to be told that it ends up being way, way uglier/worse/etc,
> > but I just thought I'd ask.
> >
> > Regards,
> > Daniel
>
> It is a good question and something I thought and am not completely confident
> that this approach is the best. Basically what I ended up thinking was that
> the prefixed instructions were a bit of a special case, and by doing
> it like this
> the normal word instructions would just carry on the same as before.
>
> I can see this is a pretty flimsy reason, so I am happy for suggestions as
> to what would end up being clearer.
>
>
Sorry I was pretty vague here. Some more thoughts:
The current representation of an instruction is
Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
Le 20/12/2019 à 06:11, Jordan Niethe a écrit :
On Wed, Dec 18, 2019 at 7:35 PM Daniel Axtens wrote:
Jordan Niethe writes:
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 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
---
arch/powerpc/include/asm/ppc-opcode.h | 3 +++
arch/powerpc/include/asm/sstep.h | 8 +--
arch/powerpc/include/asm/uaccess.h| 30 +++
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 | 2 +-
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 | 4 ++--
13 files changed, 71 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h
b/arch/powerpc/include/asm/ppc-opcode.h
index c1df75edde44..a1dfa4bdd22f 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -377,6 +377,9 @@
#define PPC_INST_VCMPEQUD0x10c7
#define PPC_INST_VCMPEQUB0x1006
+/* macro to check if a word is a prefix */
+#define IS_PREFIX(x) (((x) >> 26) == 1)
+
/* 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..6d4cb602e231 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -89,6 +89,9 @@ enum instruction_type {
#define VSX_LDLEFT 4 /* load VSX register from left */
#define VSX_CHECK_VEC8 /* 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)
@@ -132,7 +135,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 sufx);
/*
* Emulate an instruction that can be executed just by updating
@@ -149,7 +152,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 sufx);
/*
* Emulate a load or store instruction by reading/writing the
diff --git a/arch/powerpc/include/asm/uaccess.h
b/arch/powerpc/include/asm/uaccess.h
index 15002b51ff18..bc585399e0c7 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -423,4 +423,34 @@ extern long __copy_from_user_flushcache(void *dst, const
void __user *src,
extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
size_t len);
+/*
+ * When reading an instruction iff it is a prefix, the suffix needs to be also
+ * loaded.
+ */
+#define __get_user_instr(x, y, ptr) \
+({ \
+ long __gui_ret = 0; \
+ y = 0; \
+ __gui_ret = __get_user(x, ptr); \
+ if (!__gui_ret) { \
+ if
Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
On Thu, Dec 19, 2019 at 1:15 AM Daniel Axtens wrote:
>
> Jordan Niethe writes:
>
> > 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 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
> > ---
> > arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> > arch/powerpc/include/asm/sstep.h | 8 +--
> > arch/powerpc/include/asm/uaccess.h| 30 +++
> > 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 | 2 +-
> > 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 | 4 ++--
> > 13 files changed, 71 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> > b/arch/powerpc/include/asm/ppc-opcode.h
> > index c1df75edde44..a1dfa4bdd22f 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -377,6 +377,9 @@
> > #define PPC_INST_VCMPEQUD0x10c7
> > #define PPC_INST_VCMPEQUB0x1006
> >
> > +/* macro to check if a word is a prefix */
> > +#define IS_PREFIX(x) (((x) >> 26) == 1)
> > +
> > /* 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..6d4cb602e231 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -89,6 +89,9 @@ enum instruction_type {
> > #define VSX_LDLEFT 4 /* load VSX register from left */
> > #define VSX_CHECK_VEC8 /* 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)
> > @@ -132,7 +135,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 sufx);
> >
>
> I'm not saying this is necessarily better, but did you consider:
>
> - making instr 64 bits and using masking and shifting macros to get the
>prefix and suffix?
>
> - defining an instruction type/struct/union/whatever that contains both
>halves in one object?
>
> I'm happy to be told that it ends up being way, way uglier/worse/etc,
> but I just thought I'd ask.
>
> Regards,
> Daniel
It is a good question and something I thought and am not completely confident
that this approach is the best. Basically what I ended up thinking was that
the prefixed instructions were a bit of a special case, and by doing
it like this
the normal word instructions would just carry on the same as before.
I can see this is a pretty flimsy reason, so I am happy for suggestions as
to what would end up being clearer.
>
> > /*
> > * Emulate an instruction that can be executed just by updating
> > @@ -149,7 +152,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
Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
On Wed, Dec 18, 2019 at 7:35 PM Daniel Axtens wrote:
>
> Jordan Niethe writes:
>
> > 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 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
> > ---
> > arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> > arch/powerpc/include/asm/sstep.h | 8 +--
> > arch/powerpc/include/asm/uaccess.h| 30 +++
> > 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 | 2 +-
> > 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 | 4 ++--
> > 13 files changed, 71 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> > b/arch/powerpc/include/asm/ppc-opcode.h
> > index c1df75edde44..a1dfa4bdd22f 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -377,6 +377,9 @@
> > #define PPC_INST_VCMPEQUD0x10c7
> > #define PPC_INST_VCMPEQUB0x1006
> >
> > +/* macro to check if a word is a prefix */
> > +#define IS_PREFIX(x) (((x) >> 26) == 1)
> > +
> > /* 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..6d4cb602e231 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -89,6 +89,9 @@ enum instruction_type {
> > #define VSX_LDLEFT 4 /* load VSX register from left */
> > #define VSX_CHECK_VEC8 /* 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)
> > @@ -132,7 +135,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 sufx);
> >
> > /*
> > * Emulate an instruction that can be executed just by updating
> > @@ -149,7 +152,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 sufx);
> >
> > /*
> > * Emulate a load or store instruction by reading/writing the
> > diff --git a/arch/powerpc/include/asm/uaccess.h
> > b/arch/powerpc/include/asm/uaccess.h
> > index 15002b51ff18..bc585399e0c7 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -423,4 +423,34 @@ extern long __copy_from_user_flushcache(void *dst,
> > const void __user *src,
> > extern void memcpy_page_flushcache(char *to, struct page *page, size_t
> > offset,
> > size_t len);
> >
> > +/*
> > + * When reading an instruction iff it is a prefix, the suffix needs to be
> > also
> > + * loaded.
> > + */
> >
Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
Jordan Niethe writes:
> 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 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
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> arch/powerpc/include/asm/sstep.h | 8 +--
> arch/powerpc/include/asm/uaccess.h| 30 +++
> 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 | 2 +-
> 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 | 4 ++--
> 13 files changed, 71 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> b/arch/powerpc/include/asm/ppc-opcode.h
> index c1df75edde44..a1dfa4bdd22f 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -377,6 +377,9 @@
> #define PPC_INST_VCMPEQUD0x10c7
> #define PPC_INST_VCMPEQUB0x1006
>
> +/* macro to check if a word is a prefix */
> +#define IS_PREFIX(x) (((x) >> 26) == 1)
> +
> /* 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..6d4cb602e231 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -89,6 +89,9 @@ enum instruction_type {
> #define VSX_LDLEFT 4 /* load VSX register from left */
> #define VSX_CHECK_VEC8 /* 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)
> @@ -132,7 +135,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 sufx);
>
I'm not saying this is necessarily better, but did you consider:
- making instr 64 bits and using masking and shifting macros to get the
prefix and suffix?
- defining an instruction type/struct/union/whatever that contains both
halves in one object?
I'm happy to be told that it ends up being way, way uglier/worse/etc,
but I just thought I'd ask.
Regards,
Daniel
> /*
> * Emulate an instruction that can be executed just by updating
> @@ -149,7 +152,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 sufx);
>
> /*
> * Emulate a load or store instruction by reading/writing the
> diff --git a/arch/powerpc/include/asm/uaccess.h
> b/arch/powerpc/include/asm/uaccess.h
> index 15002b51ff18..bc585399e0c7 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -423,4 +423,34 @@ extern long __copy_from_user_flushcache(void *dst, const
> void __user *src,
> extern void memcpy_page_flushcache(char *to, struct page *page, size_t
> offset,
> size_t len);
>
> +/*
> + * When
Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
Jordan Niethe writes:
> 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 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
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> arch/powerpc/include/asm/sstep.h | 8 +--
> arch/powerpc/include/asm/uaccess.h| 30 +++
> 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 | 2 +-
> 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 | 4 ++--
> 13 files changed, 71 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> b/arch/powerpc/include/asm/ppc-opcode.h
> index c1df75edde44..a1dfa4bdd22f 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -377,6 +377,9 @@
> #define PPC_INST_VCMPEQUD0x10c7
> #define PPC_INST_VCMPEQUB0x1006
>
> +/* macro to check if a word is a prefix */
> +#define IS_PREFIX(x) (((x) >> 26) == 1)
> +
> /* 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..6d4cb602e231 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -89,6 +89,9 @@ enum instruction_type {
> #define VSX_LDLEFT 4 /* load VSX register from left */
> #define VSX_CHECK_VEC8 /* 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)
> @@ -132,7 +135,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 sufx);
>
> /*
> * Emulate an instruction that can be executed just by updating
> @@ -149,7 +152,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 sufx);
>
> /*
> * Emulate a load or store instruction by reading/writing the
> diff --git a/arch/powerpc/include/asm/uaccess.h
> b/arch/powerpc/include/asm/uaccess.h
> index 15002b51ff18..bc585399e0c7 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -423,4 +423,34 @@ extern long __copy_from_user_flushcache(void *dst, const
> void __user *src,
> extern void memcpy_page_flushcache(char *to, struct page *page, size_t
> offset,
> size_t len);
>
> +/*
> + * When reading an instruction iff it is a prefix, the suffix needs to be
> also
> + * loaded.
> + */
> +#define __get_user_instr(x, y, ptr) \
> +({ \
> + long __gui_ret = 0; \
> + y = 0; \
> + __gui_ret = __get_user(x, ptr);
