[PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
Christophe Leroy
christophe.leroy at c-s.fr
Fri Dec 20 16:40:16 AEDT 2019
Le 20/12/2019 à 06:11, Jordan Niethe a écrit :
> On Wed, Dec 18, 2019 at 7:35 PM Daniel Axtens <dja at axtens.net> wrote:
>>
>> Jordan Niethe <jniethe5 at gmail.com> 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 <jniethe5 at gmail.com>
>>> ---
>>> 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_VCMPEQUD 0x100000c7
>>> #define PPC_INST_VCMPEQUB 0x10000006
>>>
>>> +/* 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_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)
>>> @@ -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 (IS_PREFIX(x)) \
>>> + __gui_ret = __get_user(y, ptr + 1); \
>>> + } \
>>> + \
>>> + __gui_ret; \
>>> +})
>>> +
>>> +#define __get_user_instr_inatomic(x, y, ptr) \
>>> +({ \
>>> + long __gui_ret = 0; \
>>> + y = 0; \
>>> + __gui_ret = __get_user_inatomic(x, ptr); \
>>> + if (!__gui_ret) { \
>>> + if (IS_PREFIX(x)) \
>>> + __gui_ret = __get_user_inatomic(y, ptr + 1); \
>>> + } \
>>> + \
>>> + __gui_ret; \
>>> +})
>>> +
>>> #endif /* _ARCH_POWERPC_UACCESS_H */
>>> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
>>> index 92045ed64976..245e79792a01 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(&op, regs, instr);
>>> + r = analyse_instr(&op, regs, instr, 0);
>>> if (r < 0)
>>> return -EINVAL;
>>>
>>> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
>>> index 58ce3d37c2a3..f4530961998c 100644
>>> --- a/arch/powerpc/kernel/hw_breakpoint.c
>>> +++ b/arch/powerpc/kernel/hw_breakpoint.c
>>> @@ -248,7 +248,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
>>> if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
>>> goto fail;
>>>
>>> - ret = analyse_instr(&op, regs, instr);
>>> + ret = analyse_instr(&op, regs, instr, 0);
>>> type = GETTYPE(op.type);
>>> size = GETSIZE(op.type);
>>>
>>> @@ -272,7 +272,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
>>> return false;
>>> }
>>>
>>> - if (!emulate_step(regs, instr))
>>> + if (!emulate_step(regs, instr, 0))
>>> goto fail;
>>>
>>> return true;
>>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>>> index 2d27ec4feee4..7303fe3856cc 100644
>>> --- a/arch/powerpc/kernel/kprobes.c
>>> +++ b/arch/powerpc/kernel/kprobes.c
>>> @@ -219,7 +219,7 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
>>> unsigned int insn = *p->ainsn.insn;
>>>
>>> /* regs->nip is also adjusted if emulate_step returns 1 */
>>> - ret = emulate_step(regs, insn);
>>> + ret = emulate_step(regs, insn, 0);
>>> if (ret > 0) {
>>> /*
>>> * Once this instruction has been boosted
>>> diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
>>> index 1cbf7f1a4e3d..d862bb549158 100644
>>> --- a/arch/powerpc/kernel/mce_power.c
>>> +++ b/arch/powerpc/kernel/mce_power.c
>>> @@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
>>> if (pfn != ULONG_MAX) {
>>> instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
>>> instr = *(unsigned int *)(instr_addr);
>>> - if (!analyse_instr(&op, &tmp, instr)) {
>>> + if (!analyse_instr(&op, &tmp, instr, 0)) {
>>> pfn = addr_to_pfn(regs, op.ea);
>>> *addr = op.ea;
>>> *phys_addr = (pfn << PAGE_SHIFT);
>>> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
>>> index 024f7aad1952..82dc8a589c87 100644
>>> --- a/arch/powerpc/kernel/optprobes.c
>>> +++ b/arch/powerpc/kernel/optprobes.c
>>> @@ -100,7 +100,7 @@ static unsigned long can_optimize(struct kprobe *p)
>>> * and that can be emulated.
>>> */
>>> if (!is_conditional_branch(*p->ainsn.insn) &&
>>> - analyse_instr(&op, ®s, *p->ainsn.insn) == 1) {
>>> + analyse_instr(&op, ®s, *p->ainsn.insn, 0) == 1) {
>>> emulate_update_regs(®s, &op);
>>> nip = regs.nip;
>>> }
>>> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
>>> index 1cfef0e5fec5..ab1077dc6148 100644
>>> --- a/arch/powerpc/kernel/uprobes.c
>>> +++ b/arch/powerpc/kernel/uprobes.c
>>> @@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>>> * emulate_step() returns 1 if the insn was successfully emulated.
>>> * For all other cases, we need to single-step in hardware.
>>> */
>>> - ret = emulate_step(regs, auprobe->insn);
>>> + ret = emulate_step(regs, auprobe->insn, 0);
>>> if (ret > 0)
>>> return true;
>>>
>>> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
>>> index 2e496eb86e94..fcab1f31b48d 100644
>>> --- a/arch/powerpc/kvm/emulate_loadstore.c
>>> +++ b/arch/powerpc/kvm/emulate_loadstore.c
>>> @@ -100,7 +100,7 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
>>>
>>> emulated = EMULATE_FAIL;
>>> vcpu->arch.regs.msr = vcpu->arch.shared->msr;
>>> - if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
>>> + if (analyse_instr(&op, &vcpu->arch.regs, inst, 0) == 0) {
>>> int type = op.type & INSTR_TYPE_MASK;
>>> int size = GETSIZE(op.type);
>>>
>>> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
>>> index c077acb983a1..ade3f5eba2e5 100644
>>> --- a/arch/powerpc/lib/sstep.c
>>> +++ b/arch/powerpc/lib/sstep.c
>>> @@ -1163,7 +1163,7 @@ static nokprobe_inline int trap_compare(long v1, long v2)
>>> * otherwise.
>>> */
>>> int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>>> - unsigned int instr)
>>> + unsigned int instr, unsigned int sufx)
>>
>> I know we really like shortenings in arch/powerpc, but I think we can
>> afford the two extra characters to spell 'suffix' in full :)
>>
> 'suffix' was what I used initially but somewhere along the line
> I found it looked unbalanced to see the abbreviation inst{r,} in the same
> context as the unabbreviated 'suffix'. Happy to change it if it is clearer.
>
I guess 'instruction' is pretty long, while 'suffix' is half the length.
In addition, 'instr' is rather common while 'suffix' is quite new.
So I agree it would be clearer to keep 'suffix'.
Christophe
More information about the Linuxppc-dev
mailing list