On Tue, 2 Jun 2020 at 05:31, Jordan Niethe <jniet...@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 <jniet...@gmail.com>
Reviewed-by: Joel Stanley <j...@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
>
