<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 6 Apr 2020, 7:52 pm Alistair Popple, <<a href="mailto:alistair@popple.id.au">alistair@popple.id.au</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> diff --git a/arch/powerpc/include/asm/inst.h<br>
> b/arch/powerpc/include/asm/inst.h index 70b37a35a91a..7e23e7146c66 100644<br>
> --- a/arch/powerpc/include/asm/inst.h<br>
> +++ b/arch/powerpc/include/asm/inst.h<br>
> @@ -8,23 +8,67 @@<br>
> <br>
> struct ppc_inst {<br>
> u32 val;<br>
> +#ifdef __powerpc64__<br>
> + u32 suffix;<br>
> +#endif /* __powerpc64__ */<br>
> } __packed;<br>
> <br>
> -#define ppc_inst(x) ((struct ppc_inst){ .val = x })<br>
> +static inline int ppc_inst_opcode(struct ppc_inst x)<br>
> +{<br>
> + return x.val >> 26;<br>
> +}<br>
> <br>
> static inline u32 ppc_inst_val(struct ppc_inst x)<br>
> {<br>
> return x.val;<br>
> }<br>
> <br>
> -static inline bool ppc_inst_len(struct ppc_inst x)<br>
> +#ifdef __powerpc64__<br>
> +#define ppc_inst(x) ((struct ppc_inst){ .val = (x), .suffix = 0xff })<br>
> +<br>
> +#define ppc_inst_prefix(x, y) ((struct ppc_inst){ .val = (x), .suffix = (y)<br>
> }) +<br>
> +static inline u32 ppc_inst_suffix(struct ppc_inst x)<br>
> {<br>
> - return sizeof(struct ppc_inst);<br>
> + return x.suffix;<br>
> }<br>
> <br>
> -static inline int ppc_inst_opcode(struct ppc_inst x)<br>
> +static inline bool ppc_inst_prefixed(struct ppc_inst x) {<br>
> + return ((ppc_inst_val(x) >> 26) == 1) && ppc_inst_suffix(x) != 0xff;<br>
> +}<br>
> +<br>
> +static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)<br>
> {<br>
> - return x.val >> 26;<br>
> + return ppc_inst_prefix(swab32(ppc_inst_val(x)),<br>
> + swab32(ppc_inst_suffix(x)));<br>
> +}<br>
> +<br>
> +static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)<br>
> +{<br>
> + u32 val, suffix = 0xff;<br>
> + val = *(u32 *)ptr;<br>
> + if ((val >> 26) == 1)<br>
> + suffix = *((u32 *)ptr + 1);<br>
> + return ppc_inst_prefix(val, suffix);<br>
> +}<br>
> +<br>
> +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)<br>
> +{<br>
> + if (ppc_inst_prefixed(x)) {<br>
> + *(u32 *)ptr = x.val;<br>
> + *((u32 *)ptr + 1) = x.suffix;<br>
> + } else {<br>
> + *(u32 *)ptr = x.val;<br>
> + }<br>
> +}<br>
> +<br>
> +#else<br>
> +<br>
> +#define ppc_inst(x) ((struct ppc_inst){ .val = x })<br>
> +<br>
> +static inline bool ppc_inst_prefixed(ppc_inst x)<br>
> +{<br>
> + return 0;<br>
> }<br>
> <br>
> static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)<br>
> @@ -32,14 +76,31 @@ static inline struct ppc_inst ppc_inst_swab(struct<br>
> ppc_inst x) return ppc_inst(swab32(ppc_inst_val(x)));<br>
> }<br>
> <br>
> +static inline u32 ppc_inst_val(struct ppc_inst x)<br>
> +{<br>
> + return x.val;<br>
> +}<br>
> +<br>
> static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)<br>
> {<br>
> return *ptr;<br>
> }<br>
> <br>
> +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)<br>
> +{<br>
> + *ptr = x;<br>
> +}<br>
> +<br>
> +#endif /* __powerpc64__ */<br>
> +<br>
> static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)<br>
> {<br>
> return !memcmp(&x, &y, sizeof(struct ppc_inst));<br>
> }<br>
<br>
Apologies for not picking this up earlier, I was hoping to get to the bottom <br>
of the issue I was seeing before you sent out v5. However the above definition <br>
of instruction equality does not seem correct because it does not consider the <br>
case when an instruction is not prefixed - a non-prefixed instruction should be <br>
considered equal if the first 32-bit opcode/value is the same. Something like:<br>
<br>
if (ppc_inst_prefixed(x) != ppc_inst_prefixed(y))<br>
return false;<br>
else if (ppc_inst_prefixed(x))<br>
return !memcmp(&x, &y, sizeof(struct ppc_inst));<br>
else<br>
return x.val == y.val;<br>
<br>
This was causing failures in ftrace_modify_code() as it would falsely detect <br>
two non-prefixed instructions as being not equal due to differences in the suffix.<br></blockquote></div></div><div dir="auto">Hm I was intending that non prefixed instructions would always have the suffix set to the same value. If that's not happening, something must be wrong with where the instructions are created. </div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- Alistair<br>
<br>
> +static inline int ppc_inst_len(struct ppc_inst x)<br>
> +{<br>
> + return (ppc_inst_prefixed(x)) ? 8 : 4;<br>
> +}<br>
> +<br>
> #endif /* _ASM_INST_H */<br>
> diff --git a/arch/powerpc/include/asm/kprobes.h<br>
> b/arch/powerpc/include/asm/kprobes.h index 66b3f2983b22..4fc0e15e23a5<br>
> 100644<br>
> --- a/arch/powerpc/include/asm/kprobes.h<br>
> +++ b/arch/powerpc/include/asm/kprobes.h<br>
> @@ -43,7 +43,7 @@ extern kprobe_opcode_t optprobe_template_ret[];<br>
> extern kprobe_opcode_t optprobe_template_end[];<br>
> <br>
> /* Fixed instruction size for powerpc */<br>
> -#define MAX_INSN_SIZE 1<br>
> +#define MAX_INSN_SIZE 2<br>
> #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */<br>
> #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)<br>
> #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */<br>
> diff --git a/arch/powerpc/include/asm/uaccess.h<br>
> b/arch/powerpc/include/asm/uaccess.h index c0a35e4586a5..5a3f486ddf02<br>
> 100644<br>
> --- a/arch/powerpc/include/asm/uaccess.h<br>
> +++ b/arch/powerpc/include/asm/uaccess.h<br>
> @@ -105,11 +105,34 @@ static inline int __access_ok(unsigned long addr,<br>
> unsigned long size, #define __put_user_inatomic(x, ptr) \<br>
> __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))<br>
> <br>
> -#define __get_user_instr(x, ptr) \<br>
> - __get_user_nocheck((x).val, (u32 *)(ptr), sizeof(u32), true)<br>
> +#define __get_user_instr(x, ptr) \<br>
> +({ \<br>
> + long __gui_ret = 0; \<br>
> + unsigned int prefix, suffix; \<br>
> + __gui_ret = __get_user(prefix, (unsigned int __user *)ptr); \<br>
> + if (!__gui_ret && (prefix >> 26) == 1) { \<br>
> + __gui_ret = __get_user(suffix, (unsigned int __user *)ptr + 1); \<br>
> + (x) = ppc_inst_prefix(prefix, suffix); \<br>
> + } else { \<br>
> + (x) = ppc_inst(prefix); \<br>
> + } \<br>
> + __gui_ret; \<br>
> +})<br>
> +<br>
> +#define __get_user_instr_inatomic(x, ptr) \<br>
> +({ \<br>
> + long __gui_ret = 0; \<br>
> + unsigned int prefix, suffix; \<br>
> + __gui_ret = __get_user_inatomic(prefix, (unsigned int __user *)ptr); \<br>
> + if (!__gui_ret && (prefix >> 26) == 1) { \<br>
> + __gui_ret = __get_user_inatomic(suffix, (unsigned int __user *)ptr +<br>
> 1); \ + (x) = ppc_inst_prefix(prefix, suffix); \<br>
> + } else { \<br>
> + (x) = ppc_inst(prefix); \<br>
> + } \<br>
> + __gui_ret; \<br>
> +})<br>
> <br>
> -#define __get_user_instr_inatomic(x, ptr) \<br>
> - __get_user_nosleep((x).val, (u32 *)(ptr), sizeof(u32))<br>
> extern long __put_user_bad(void);<br>
> <br>
> /*<br>
> diff --git a/arch/powerpc/include/asm/uprobes.h<br>
> b/arch/powerpc/include/asm/uprobes.h index 7e3b329ba2d3..5bf65f5d44a9<br>
> 100644<br>
> --- a/arch/powerpc/include/asm/uprobes.h<br>
> +++ b/arch/powerpc/include/asm/uprobes.h<br>
> @@ -15,7 +15,7 @@<br>
> <br>
> typedef ppc_opcode_t uprobe_opcode_t;<br>
> <br>
> -#define MAX_UINSN_BYTES 4<br>
> +#define MAX_UINSN_BYTES 8<br>
> #define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)<br>
> <br>
> /* The following alias is needed for reference from arch-agnostic code */<br>
> diff --git a/arch/powerpc/kernel/optprobes.c<br>
> b/arch/powerpc/kernel/optprobes.c index 684640b8fa2e..689daf430161 100644<br>
> --- a/arch/powerpc/kernel/optprobes.c<br>
> +++ b/arch/powerpc/kernel/optprobes.c<br>
> @@ -159,38 +159,38 @@ void patch_imm32_load_insns(unsigned int val,<br>
> kprobe_opcode_t *addr)<br>
> <br>
> /*<br>
> * Generate instructions to load provided immediate 64-bit value<br>
> - * to register 'r3' and patch these instructions at 'addr'.<br>
> + * to register 'reg' and patch these instructions at 'addr'.<br>
> */<br>
> -void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)<br>
> +void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t<br>
> *addr) {<br>
> - /* lis r3,(op)@highest */<br>
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS |<br>
> ___PPC_RT(3) | + /* lis reg,(op)@highest */<br>
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS |<br>
> ___PPC_RT(reg) | ((val >> 48) & 0xffff)));<br>
> addr++;<br>
> <br>
> - /* ori r3,r3,(op)@higher */<br>
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |<br>
> ___PPC_RA(3) | - ___PPC_RS(3) | ((val >> 32) & 0xffff)));<br>
> + /* ori reg,reg,(op)@higher */<br>
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |<br>
> ___PPC_RA(reg) | + ___PPC_RS(reg) | ((val >> 32) & 0xffff)));<br>
> addr++;<br>
> <br>
> - /* rldicr r3,r3,32,31 */<br>
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR |<br>
> ___PPC_RA(3) | - ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31)));<br>
> + /* rldicr reg,reg,32,31 */<br>
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR |<br>
> ___PPC_RA(reg) | + ___PPC_RS(reg) | __PPC_SH64(32) | <br>
__PPC_ME64(31)));<br>
> addr++;<br>
> <br>
> - /* oris r3,r3,(op)@h */<br>
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |<br>
> ___PPC_RA(3) | - ___PPC_RS(3) | ((val >> 16) & 0xffff)));<br>
> + /* oris reg,reg,(op)@h */<br>
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |<br>
> ___PPC_RA(reg) | + ___PPC_RS(reg) | ((val >> 16) & 0xffff)));<br>
> addr++;<br>
> <br>
> - /* ori r3,r3,(op)@l */<br>
> - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |<br>
> ___PPC_RA(3) | - ___PPC_RS(3) | (val & 0xffff)));<br>
> + /* ori reg,reg,(op)@l */<br>
> + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |<br>
> ___PPC_RA(reg) | + ___PPC_RS(reg) | (val & 0xffff)));<br>
> }<br>
> <br>
> int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct<br>
> kprobe *p) {<br>
> - struct ppc_inst branch_op_callback, branch_emulate_step;<br>
> + struct ppc_inst branch_op_callback, branch_emulate_step, temp;<br>
> kprobe_opcode_t *op_callback_addr, *emulate_step_addr, *buff;<br>
> long b_offset;<br>
> unsigned long nip, size;<br>
> @@ -240,7 +240,7 @@ int arch_prepare_optimized_kprobe(struct<br>
> optimized_kprobe *op, struct kprobe *p) * Fixup the template with<br>
> instructions to:<br>
> * 1. load the address of the actual probepoint<br>
> */<br>
> - patch_imm64_load_insns((unsigned long)op, buff + TMPL_OP_IDX);<br>
> + patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);<br>
> <br>
> /*<br>
> * 2. branch to optimized_callback() and emulate_step()<br>
> @@ -271,7 +271,11 @@ int arch_prepare_optimized_kprobe(struct<br>
> optimized_kprobe *op, struct kprobe *p) /*<br>
> * 3. load instruction to be emulated into relevant register, and<br>
> */<br>
> - patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);<br>
> + temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);<br>
> + patch_imm64_load_insns(ppc_inst_val(temp) |<br>
> + ((u64)ppc_inst_suffix(temp) << 32),<br>
> + 4,<br>
> + buff + TMPL_INSN_IDX);<br>
> <br>
> /*<br>
> * 4. branch back from trampoline<br>
> diff --git a/arch/powerpc/kernel/optprobes_head.S<br>
> b/arch/powerpc/kernel/optprobes_head.S index cf383520843f..ff8ba4d3824d<br>
> 100644<br>
> --- a/arch/powerpc/kernel/optprobes_head.S<br>
> +++ b/arch/powerpc/kernel/optprobes_head.S<br>
> @@ -94,6 +94,9 @@ optprobe_template_insn:<br>
> /* 2, Pass instruction to be emulated in r4 */<br>
> nop<br>
> nop<br>
> + nop<br>
> + nop<br>
> + nop<br>
> <br>
> .global optprobe_template_call_emulate<br>
> optprobe_template_call_emulate:<br>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c<br>
> b/arch/powerpc/kernel/trace/ftrace.c index e78742613b36..16041a5c86d5<br>
> 100644<br>
> --- a/arch/powerpc/kernel/trace/ftrace.c<br>
> +++ b/arch/powerpc/kernel/trace/ftrace.c<br>
> @@ -41,11 +41,35 @@<br>
> #define NUM_FTRACE_TRAMPS 8<br>
> static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];<br>
> <br>
> +#ifdef __powerpc64__<br>
> static long<br>
> probe_kernel_read_inst(struct ppc_inst *inst, const void *src)<br>
> {<br>
> - return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);<br>
> + u32 val, suffix = 0;<br>
> + long err;<br>
> +<br>
> + err = probe_kernel_read((void *)&val,<br>
> + src, sizeof(val));<br>
> + if (err)<br>
> + return err;<br>
> +<br>
> + if ((val >> 26) == 1)<br>
> + err = probe_kernel_read((void *)&suffix,<br>
> + src + 4, MCOUNT_INSN_SIZE);<br>
> + if (err)<br>
> + return err;<br>
> +<br>
> + *inst = ppc_inst_prefix(val, suffix);<br>
> +<br>
> + return 0;<br>
> }<br>
> +#else<br>
> +static long<br>
> +probe_kernel_read_inst(struct ppc_inst *inst, const void *src)<br>
> +{<br>
> + return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE)<br>
> +}<br>
> +#endif<br>
> <br>
> static struct ppc_inst<br>
> ftrace_call_replace(unsigned long ip, unsigned long addr, int link)<br>
> diff --git a/arch/powerpc/lib/code-patching.c<br>
> b/arch/powerpc/lib/code-patching.c index c329ad657302..b4007e03d8fa 100644<br>
> --- a/arch/powerpc/lib/code-patching.c<br>
> +++ b/arch/powerpc/lib/code-patching.c<br>
> @@ -24,12 +24,19 @@ static int __patch_instruction(struct ppc_inst<br>
> *exec_addr, struct ppc_inst instr {<br>
> int err = 0;<br>
> <br>
> - __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");<br>
> - if (err)<br>
> - return err;<br>
> -<br>
> - asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),<br>
> - "r" (exec_addr));<br>
> + if (!ppc_inst_prefixed(instr)) {<br>
> + __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");<br>
> + if (err)<br>
> + return err;<br>
> + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),<br>
> + "r" (exec_addr));<br>
> + } else {<br>
> + __put_user_asm((u64)ppc_inst_suffix(instr) << 32 | ppc_inst_val(instr),<br>
> patch_addr, err, "std"); + if (err)<br>
> + return err;<br>
> + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),<br>
> + "r" (exec_addr));<br>
> + }<br>
> <br>
> return 0;<br>
> }<br>
> diff --git a/arch/powerpc/lib/feature-fixups.c<br>
> b/arch/powerpc/lib/feature-fixups.c index f00dd13b1c3c..5519cec83cc8 100644<br>
> --- a/arch/powerpc/lib/feature-fixups.c<br>
> +++ b/arch/powerpc/lib/feature-fixups.c<br>
> @@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value,<br>
> struct fixup_entry *fcur) src = alt_start;<br>
> dest = start;<br>
> <br>
> - for (; src < alt_end; src++, dest++) {<br>
> + for (; src < alt_end; src = (void *)src +<br>
> ppc_inst_len(ppc_inst_read(src)), + (dest = (void *)dest +<br>
> ppc_inst_len(ppc_inst_read(dest)))) { if (patch_alt_instruction(src, dest,<br>
> alt_start, alt_end))<br>
> return 1;<br>
> }<br>
> <br>
> - for (; dest < end; dest++)<br>
> + for (; dest < end; dest = (void *)dest +<br>
> ppc_inst_len(ppc_inst(PPC_INST_NOP))) raw_patch_instruction(dest,<br>
> ppc_inst(PPC_INST_NOP));<br>
> <br>
> return 0;<br>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c<br>
> index 52ddd3122dc8..8b285bf11218 100644<br>
> --- a/arch/powerpc/lib/sstep.c<br>
> +++ b/arch/powerpc/lib/sstep.c<br>
> @@ -1169,10 +1169,12 @@ int analyse_instr(struct instruction_op *op, const<br>
> struct pt_regs *regs, unsigned long int imm;<br>
> unsigned long int val, val2;<br>
> unsigned int mb, me, sh;<br>
> - unsigned int word;<br>
> + unsigned int word, suffix;<br>
> long ival;<br>
> <br>
> word = ppc_inst_val(instr);<br>
> + suffix = ppc_inst_suffix(instr);<br>
> +<br>
> op->type = COMPUTE;<br>
> <br>
> opcode = word >> 26;<br>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c<br>
> index 6f3bcdcfc9c7..b704aebb099a 100644<br>
> --- a/arch/powerpc/xmon/xmon.c<br>
> +++ b/arch/powerpc/xmon/xmon.c<br>
> @@ -761,8 +761,8 @@ static int xmon_bpt(struct pt_regs *regs)<br>
> <br>
> /* Are we at the trap at bp->instr[1] for some bp? */<br>
> bp = in_breakpoint_table(regs->nip, &offset);<br>
> - if (bp != NULL && offset == 4) {<br>
> - regs->nip = bp->address + 4;<br>
> + if (bp != NULL && (offset == 4 || offset == 8)) {<br>
> + regs->nip = bp->address + offset;<br>
> atomic_dec(&bp->ref_count);<br>
> return 1;<br>
> }<br>
> @@ -863,7 +863,7 @@ static struct bpt *in_breakpoint_table(unsigned long<br>
> nip, unsigned long *offp) if (off >= sizeof(bpt_table))<br>
> return NULL;<br>
> *offp = off % BPT_SIZE;<br>
> - if (*offp != 0 && *offp != 4)<br>
> + if (*offp != 0 && *offp != 4 && *offp != 8)<br>
> return NULL;<br>
> return bpts + (off / BPT_SIZE);<br>
> }<br>
> diff --git a/arch/powerpc/xmon/xmon_bpts.S b/arch/powerpc/xmon/xmon_bpts.S<br>
> index ebb2dbc70ca8..09058eb6abbd 100644<br>
> --- a/arch/powerpc/xmon/xmon_bpts.S<br>
> +++ b/arch/powerpc/xmon/xmon_bpts.S<br>
> @@ -3,6 +3,8 @@<br>
> #include <asm/asm-compat.h><br>
> #include "xmon_bpts.h"<br>
> <br>
> +/* Prefixed instructions can not cross 64 byte boundaries */<br>
> +.align 6<br>
> .global bpt_table<br>
> bpt_table:<br>
> - .space NBPTS * 8<br>
> + .space NBPTS * 16<br>
<br>
<br>
<br>
<br>
</blockquote></div></div></div>