| Message ID | 20200602052728.18227-1-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 50428fdc53ba48f6936b10dfdc0d644972403908 |
| Headers | show |
| Series | [1/4] powerpc: Add a ppc_inst_as_str() helper | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (00ec79b0b767994422c43792d73ff1327714a73f) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (30df74d67d48949da87e3a5b57c381763e8fd526) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (f359287765c04711ff54fbd11645271d8e5ff763) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (2f26ed1764b42a8c40d9c48441c73a70d805decf) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (e7b08814b16b80a0bf76eeca16317f8c2ed23b8c) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On Tue, 2 Jun 2020 at 05:31, Jordan Niethe <jniethe5@gmail.com> wrote: > > There are quite a few places where instructions are printed, this is > done using a '%x' format specifier. With the introduction of prefixed > instructions, this does not work well. Currently in these places, > ppc_inst_val() is used for the value for %x so only the first word of > prefixed instructions are printed. > > When the instructions are word instructions, only a single word should > be printed. For prefixed instructions both the prefix and suffix should > be printed. To accommodate both of these situations, instead of a '%x' > specifier use '%s' and introduce a helper, __ppc_inst_as_str() which > returns a char *. The char * __ppc_inst_as_str() returns is buffer that > is passed to it by the caller. > > It is cumbersome to require every caller of __ppc_inst_as_str() to now > declare a buffer. To make it more convenient to use __ppc_inst_as_str(), > wrap it in a macro that uses a compound statement to allocate a buffer > on the caller's stack before calling it. > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > arch/powerpc/include/asm/inst.h | 19 +++++++++++++++++++ > arch/powerpc/kernel/kprobes.c | 2 +- > arch/powerpc/kernel/trace/ftrace.c | 26 +++++++++++++------------- > arch/powerpc/lib/test_emulate_step.c | 4 ++-- > arch/powerpc/xmon/xmon.c | 2 +- > 5 files changed, 36 insertions(+), 17 deletions(-) > > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h > index 45f3ec868258..3df7806e6dc3 100644 > --- a/arch/powerpc/include/asm/inst.h > +++ b/arch/powerpc/include/asm/inst.h > @@ -122,6 +122,25 @@ static inline u64 ppc_inst_as_u64(struct ppc_inst x) > #endif > } > > +#define PPC_INST_STR_LEN sizeof("0x00000000 0x00000000") > + > +static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], struct ppc_inst x) > +{ > + if (ppc_inst_prefixed(x)) > + sprintf(str, "0x%08x 0x%08x", ppc_inst_val(x), ppc_inst_suffix(x)); > + else > + sprintf(str, "0x%08x", ppc_inst_val(x)); > + > + return str; > +} > + > +#define ppc_inst_as_str(x) \ > +({ \ > + char __str[PPC_INST_STR_LEN]; \ > + __ppc_inst_as_str(__str, x); \ > + __str; \ > +}) > + > int probe_user_read_inst(struct ppc_inst *inst, > struct ppc_inst __user *nip); > > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index 227510df8c55..d0797171dba3 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -244,7 +244,7 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs) > * So, we should never get here... but, its still > * good to catch them, just in case... > */ > - printk("Can't step on instruction %x\n", ppc_inst_val(insn)); > + printk("Can't step on instruction %s\n", ppc_inst_as_str(insn)); > BUG(); > } else { > /* > diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c > index 5e399628f51a..da11a26d8213 100644 > --- a/arch/powerpc/kernel/trace/ftrace.c > +++ b/arch/powerpc/kernel/trace/ftrace.c > @@ -73,8 +73,8 @@ ftrace_modify_code(unsigned long ip, struct ppc_inst old, struct ppc_inst new) > > /* Make sure it is what we expect it to be */ > if (!ppc_inst_equal(replaced, old)) { > - pr_err("%p: replaced (%#x) != old (%#x)", > - (void *)ip, ppc_inst_val(replaced), ppc_inst_val(old)); > + pr_err("%p: replaced (%s) != old (%s)", > + (void *)ip, ppc_inst_as_str(replaced), ppc_inst_as_str(old)); > return -EINVAL; > } > > @@ -137,7 +137,7 @@ __ftrace_make_nop(struct module *mod, > > /* Make sure that that this is still a 24bit jump */ > if (!is_bl_op(op)) { > - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); > + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); > return -EINVAL; > } > > @@ -172,8 +172,8 @@ __ftrace_make_nop(struct module *mod, > /* We expect either a mflr r0, or a std r0, LRSAVE(r1) */ > if (!ppc_inst_equal(op, ppc_inst(PPC_INST_MFLR)) && > !ppc_inst_equal(op, ppc_inst(PPC_INST_STD_LR))) { > - pr_err("Unexpected instruction %08x around bl _mcount\n", > - ppc_inst_val(op)); > + pr_err("Unexpected instruction %s around bl _mcount\n", > + ppc_inst_as_str(op)); > return -EINVAL; > } > #else > @@ -203,7 +203,7 @@ __ftrace_make_nop(struct module *mod, > } > > if (!ppc_inst_equal(op, ppc_inst(PPC_INST_LD_TOC))) { > - pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC, ppc_inst_val(op)); > + pr_err("Expected %08x found %s\n", PPC_INST_LD_TOC, ppc_inst_as_str(op)); > return -EINVAL; > } > #endif /* CONFIG_MPROFILE_KERNEL */ > @@ -231,7 +231,7 @@ __ftrace_make_nop(struct module *mod, > > /* Make sure that that this is still a 24bit jump */ > if (!is_bl_op(op)) { > - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); > + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); > return -EINVAL; > } > > @@ -406,7 +406,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr) > > /* Make sure that that this is still a 24bit jump */ > if (!is_bl_op(op)) { > - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); > + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); > return -EINVAL; > } > > @@ -533,8 +533,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) > return -EFAULT; > > if (!expected_nop_sequence(ip, op[0], op[1])) { > - pr_err("Unexpected call sequence at %p: %x %x\n", > - ip, ppc_inst_val(op[0]), ppc_inst_val(op[1])); > + pr_err("Unexpected call sequence at %p: %s %s\n", > + ip, ppc_inst_as_str(op[0]), ppc_inst_as_str(op[1])); > return -EINVAL; > } > > @@ -597,7 +597,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) > > /* It should be pointing to a nop */ > if (!ppc_inst_equal(op, ppc_inst(PPC_INST_NOP))) { > - pr_err("Expected NOP but have %x\n", ppc_inst_val(op)); > + pr_err("Expected NOP but have %s\n", ppc_inst_as_str(op)); > return -EINVAL; > } > > @@ -654,7 +654,7 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr) > } > > if (!ppc_inst_equal(op, ppc_inst(PPC_INST_NOP))) { > - pr_err("Unexpected call sequence at %p: %x\n", ip, ppc_inst_val(op)); > + pr_err("Unexpected call sequence at %p: %s\n", ip, ppc_inst_as_str(op)); > return -EINVAL; > } > > @@ -733,7 +733,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, > > /* Make sure that that this is still a 24bit jump */ > if (!is_bl_op(op)) { > - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); > + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); > return -EINVAL; > } > > diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c > index 46af80279ebc..5fb6f5267d70 100644 > --- a/arch/powerpc/lib/test_emulate_step.c > +++ b/arch/powerpc/lib/test_emulate_step.c > @@ -852,7 +852,7 @@ static int __init emulate_compute_instr(struct pt_regs *regs, > > if (analyse_instr(&op, regs, instr) != 1 || > GETTYPE(op.type) != COMPUTE) { > - pr_info("emulation failed, instruction = 0x%08x\n", ppc_inst_val(instr)); > + pr_info("execution failed, instruction = %s\n", ppc_inst_as_str(instr)); > return -EFAULT; > } > > @@ -872,7 +872,7 @@ static int __init execute_compute_instr(struct pt_regs *regs, > /* Patch the NOP with the actual instruction */ > patch_instruction_site(&patch__exec_instr, instr); > if (exec_instr(regs)) { > - pr_info("execution failed, instruction = 0x%08x\n", ppc_inst_val(instr)); > + pr_info("execution failed, instruction = %s\n", ppc_inst_as_str(instr)); > return -EFAULT; > } > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index 16ee6639a60c..1dd3bf02021b 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -2958,7 +2958,7 @@ generic_inst_dump(unsigned long adr, long count, int praddr, > dotted = 0; > last_inst = inst; > if (praddr) > - printf(REG" %.8x", adr, ppc_inst_val(inst)); > + printf(REG" %s", adr, ppc_inst_as_str(inst)); > printf("\t"); > dump_func(ppc_inst_val(inst), adr); > printf("\n"); > -- > 2.17.1 >
On Tue, Jun 02, 2020 at 03:27:25PM +1000, Jordan Niethe wrote: > There are quite a few places where instructions are printed, this is > done using a '%x' format specifier. With the introduction of prefixed > instructions, this does not work well. Currently in these places, > ppc_inst_val() is used for the value for %x so only the first word of > prefixed instructions are printed. > > When the instructions are word instructions, only a single word should > be printed. For prefixed instructions both the prefix and suffix should > be printed. To accommodate both of these situations, instead of a '%x' > specifier use '%s' and introduce a helper, __ppc_inst_as_str() which > returns a char *. The char * __ppc_inst_as_str() returns is buffer that > is passed to it by the caller. > > It is cumbersome to require every caller of __ppc_inst_as_str() to now > declare a buffer. To make it more convenient to use __ppc_inst_as_str(), > wrap it in a macro that uses a compound statement to allocate a buffer > on the caller's stack before calling it. > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> Acked-by: Segher Boessenkool <segher@kernel.crashing.org> > +#define PPC_INST_STR_LEN sizeof("0x00000000 0x00000000") sizeof is not a function or anything function-like; it's just an operator, like (unary) "+" or "-" or dereference "*". The parentheses are completely redundant here. And it kind of matters, as written a reader might think that a string object is actually constructed here. But, so very many people do this, I'm not going to fight this fight ;-) Segher
On Tue, 2 Jun 2020 15:27:25 +1000, Jordan Niethe wrote: > There are quite a few places where instructions are printed, this is > done using a '%x' format specifier. With the introduction of prefixed > instructions, this does not work well. Currently in these places, > ppc_inst_val() is used for the value for %x so only the first word of > prefixed instructions are printed. > > When the instructions are word instructions, only a single word should > be printed. For prefixed instructions both the prefix and suffix should > be printed. To accommodate both of these situations, instead of a '%x' > specifier use '%s' and introduce a helper, __ppc_inst_as_str() which > returns a char *. The char * __ppc_inst_as_str() returns is buffer that > is passed to it by the caller. > > [...] Patches 1-2 applied to powerpc/next. [1/4] powerpc: Add a ppc_inst_as_str() helper https://git.kernel.org/powerpc/c/50428fdc53ba48f6936b10dfdc0d644972403908 [2/4] powerpc/xmon: Improve dumping prefixed instructions https://git.kernel.org/powerpc/c/8b98afc117aaf825c66d7ddd59f1849e559b42cd cheers
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h index 45f3ec868258..3df7806e6dc3 100644 --- a/arch/powerpc/include/asm/inst.h +++ b/arch/powerpc/include/asm/inst.h @@ -122,6 +122,25 @@ static inline u64 ppc_inst_as_u64(struct ppc_inst x) #endif } +#define PPC_INST_STR_LEN sizeof("0x00000000 0x00000000") + +static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], struct ppc_inst x) +{ + if (ppc_inst_prefixed(x)) + sprintf(str, "0x%08x 0x%08x", ppc_inst_val(x), ppc_inst_suffix(x)); + else + sprintf(str, "0x%08x", ppc_inst_val(x)); + + return str; +} + +#define ppc_inst_as_str(x) \ +({ \ + char __str[PPC_INST_STR_LEN]; \ + __ppc_inst_as_str(__str, x); \ + __str; \ +}) + int probe_user_read_inst(struct ppc_inst *inst, struct ppc_inst __user *nip); diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 227510df8c55..d0797171dba3 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -244,7 +244,7 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs) * So, we should never get here... but, its still * good to catch them, just in case... */ - printk("Can't step on instruction %x\n", ppc_inst_val(insn)); + printk("Can't step on instruction %s\n", ppc_inst_as_str(insn)); BUG(); } else { /* diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c index 5e399628f51a..da11a26d8213 100644 --- a/arch/powerpc/kernel/trace/ftrace.c +++ b/arch/powerpc/kernel/trace/ftrace.c @@ -73,8 +73,8 @@ ftrace_modify_code(unsigned long ip, struct ppc_inst old, struct ppc_inst new) /* Make sure it is what we expect it to be */ if (!ppc_inst_equal(replaced, old)) { - pr_err("%p: replaced (%#x) != old (%#x)", - (void *)ip, ppc_inst_val(replaced), ppc_inst_val(old)); + pr_err("%p: replaced (%s) != old (%s)", + (void *)ip, ppc_inst_as_str(replaced), ppc_inst_as_str(old)); return -EINVAL; } @@ -137,7 +137,7 @@ __ftrace_make_nop(struct module *mod, /* Make sure that that this is still a 24bit jump */ if (!is_bl_op(op)) { - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); return -EINVAL; } @@ -172,8 +172,8 @@ __ftrace_make_nop(struct module *mod, /* We expect either a mflr r0, or a std r0, LRSAVE(r1) */ if (!ppc_inst_equal(op, ppc_inst(PPC_INST_MFLR)) && !ppc_inst_equal(op, ppc_inst(PPC_INST_STD_LR))) { - pr_err("Unexpected instruction %08x around bl _mcount\n", - ppc_inst_val(op)); + pr_err("Unexpected instruction %s around bl _mcount\n", + ppc_inst_as_str(op)); return -EINVAL; } #else @@ -203,7 +203,7 @@ __ftrace_make_nop(struct module *mod, } if (!ppc_inst_equal(op, ppc_inst(PPC_INST_LD_TOC))) { - pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC, ppc_inst_val(op)); + pr_err("Expected %08x found %s\n", PPC_INST_LD_TOC, ppc_inst_as_str(op)); return -EINVAL; } #endif /* CONFIG_MPROFILE_KERNEL */ @@ -231,7 +231,7 @@ __ftrace_make_nop(struct module *mod, /* Make sure that that this is still a 24bit jump */ if (!is_bl_op(op)) { - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); return -EINVAL; } @@ -406,7 +406,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr) /* Make sure that that this is still a 24bit jump */ if (!is_bl_op(op)) { - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); return -EINVAL; } @@ -533,8 +533,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) return -EFAULT; if (!expected_nop_sequence(ip, op[0], op[1])) { - pr_err("Unexpected call sequence at %p: %x %x\n", - ip, ppc_inst_val(op[0]), ppc_inst_val(op[1])); + pr_err("Unexpected call sequence at %p: %s %s\n", + ip, ppc_inst_as_str(op[0]), ppc_inst_as_str(op[1])); return -EINVAL; } @@ -597,7 +597,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) /* It should be pointing to a nop */ if (!ppc_inst_equal(op, ppc_inst(PPC_INST_NOP))) { - pr_err("Expected NOP but have %x\n", ppc_inst_val(op)); + pr_err("Expected NOP but have %s\n", ppc_inst_as_str(op)); return -EINVAL; } @@ -654,7 +654,7 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr) } if (!ppc_inst_equal(op, ppc_inst(PPC_INST_NOP))) { - pr_err("Unexpected call sequence at %p: %x\n", ip, ppc_inst_val(op)); + pr_err("Unexpected call sequence at %p: %s\n", ip, ppc_inst_as_str(op)); return -EINVAL; } @@ -733,7 +733,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, /* Make sure that that this is still a 24bit jump */ if (!is_bl_op(op)) { - pr_err("Not expected bl: opcode is %x\n", ppc_inst_val(op)); + pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op)); return -EINVAL; } diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c index 46af80279ebc..5fb6f5267d70 100644 --- a/arch/powerpc/lib/test_emulate_step.c +++ b/arch/powerpc/lib/test_emulate_step.c @@ -852,7 +852,7 @@ static int __init emulate_compute_instr(struct pt_regs *regs, if (analyse_instr(&op, regs, instr) != 1 || GETTYPE(op.type) != COMPUTE) { - pr_info("emulation failed, instruction = 0x%08x\n", ppc_inst_val(instr)); + pr_info("execution failed, instruction = %s\n", ppc_inst_as_str(instr)); return -EFAULT; } @@ -872,7 +872,7 @@ static int __init execute_compute_instr(struct pt_regs *regs, /* Patch the NOP with the actual instruction */ patch_instruction_site(&patch__exec_instr, instr); if (exec_instr(regs)) { - pr_info("execution failed, instruction = 0x%08x\n", ppc_inst_val(instr)); + pr_info("execution failed, instruction = %s\n", ppc_inst_as_str(instr)); return -EFAULT; } diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index 16ee6639a60c..1dd3bf02021b 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -2958,7 +2958,7 @@ generic_inst_dump(unsigned long adr, long count, int praddr, dotted = 0; last_inst = inst; if (praddr) - printf(REG" %.8x", adr, ppc_inst_val(inst)); + printf(REG" %s", adr, ppc_inst_as_str(inst)); printf("\t"); dump_func(ppc_inst_val(inst), adr); printf("\n");
There are quite a few places where instructions are printed, this is done using a '%x' format specifier. With the introduction of prefixed instructions, this does not work well. Currently in these places, ppc_inst_val() is used for the value for %x so only the first word of prefixed instructions are printed. When the instructions are word instructions, only a single word should be printed. For prefixed instructions both the prefix and suffix should be printed. To accommodate both of these situations, instead of a '%x' specifier use '%s' and introduce a helper, __ppc_inst_as_str() which returns a char *. The char * __ppc_inst_as_str() returns is buffer that is passed to it by the caller. It is cumbersome to require every caller of __ppc_inst_as_str() to now declare a buffer. To make it more convenient to use __ppc_inst_as_str(), wrap it in a macro that uses a compound statement to allocate a buffer on the caller's stack before calling it. Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- arch/powerpc/include/asm/inst.h | 19 +++++++++++++++++++ arch/powerpc/kernel/kprobes.c | 2 +- arch/powerpc/kernel/trace/ftrace.c | 26 +++++++++++++------------- arch/powerpc/lib/test_emulate_step.c | 4 ++-- arch/powerpc/xmon/xmon.c | 2 +- 5 files changed, 36 insertions(+), 17 deletions(-)