* [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper
@ 2020-06-02 5:27 Jordan Niethe
2020-06-02 5:27 ` [PATCH 2/4] powerpc/xmon: Improve dumping prefixed instructions Jordan Niethe
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Jordan Niethe @ 2020-06-02 5:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Jordan Niethe, alistair
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(-)
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] powerpc/xmon: Improve dumping prefixed instructions
2020-06-02 5:27 [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Jordan Niethe
@ 2020-06-02 5:27 ` Jordan Niethe
2020-06-02 5:27 ` [PATCH 3/4] powerpc: Handle prefixed instructions in show_user_instructions() Jordan Niethe
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Jordan Niethe @ 2020-06-02 5:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Jordan Niethe, alistair
Currently prefixed instructions are dumped as two separate word
instructions. Use mread_instr() so that prefixed instructions are read
as such and update the incrementor in the loop to take this into
account.
'dump_func' is print_insn_powerpc() which comes from ppc-dis.c which is
taken from binutils. When this is updated prefixed instructions will be
disassembled.
Currently dumping prefixed instructions looks like this:
0:mon> di c000000000094168
c000000000094168 0x06000000 .long 0x6000000
c00000000009416c 0x392a0003 addi r9,r10,3
c000000000094170 0x913f0028 stw r9,40(r31)
c000000000094174 0xe93f002a lwa r9,40(r31)
c000000000094178 0x7d234b78 mr r3,r9
c00000000009417c 0x383f0040 addi r1,r31,64
c000000000094180 0xebe1fff8 ld r31,-8(r1)
c000000000094184 0x4e800020 blr
c000000000094188 0x60000000 nop
...
c000000000094190 0x3c4c0121 addis r2,r12,289
c000000000094194 0x38429670 addi r2,r2,-27024
c000000000094198 0x7c0802a6 mflr r0
c00000000009419c 0x60000000 nop
c0000000000941a0 0xe9240100 ld r9,256(r4)
c0000000000941a4 0x39400001 li r10,1
After this it looks like:
0:mon> di c000000000094168
c000000000094168 0x06000000 0x392a0003 .long 0x392a000306000000
c000000000094170 0x913f0028 stw r9,40(r31)
c000000000094174 0xe93f002a lwa r9,40(r31)
c000000000094178 0x7d234b78 mr r3,r9
c00000000009417c 0x383f0040 addi r1,r31,64
c000000000094180 0xebe1fff8 ld r31,-8(r1)
c000000000094184 0x4e800020 blr
c000000000094188 0x60000000 nop
...
c000000000094190 0x3c4c0121 addis r2,r12,289
c000000000094194 0x38429570 addi r2,r2,-27280
c000000000094198 0x7c0802a6 mflr r0
c00000000009419c 0x60000000 nop
c0000000000941a0 0xe9240100 ld r9,256(r4)
c0000000000941a4 0x39400001 li r10,1
c0000000000941a8 0x3d02000b addis r8,r2,11
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/xmon/xmon.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 1dd3bf02021b..548571536bd1 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2935,11 +2935,10 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
int nr, dotted;
unsigned long first_adr;
struct ppc_inst inst, last_inst = ppc_inst(0);
- unsigned char val[4];
dotted = 0;
- for (first_adr = adr; count > 0; --count, adr += 4) {
- nr = mread(adr, val, 4);
+ for (first_adr = adr; count > 0; --count, adr += ppc_inst_len(inst)) {
+ nr = mread_instr(adr, &inst);
if (nr == 0) {
if (praddr) {
const char *x = fault_chars[fault_type];
@@ -2947,7 +2946,6 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
}
break;
}
- inst = ppc_inst(GETWORD(val));
if (adr > first_adr && ppc_inst_equal(inst, last_inst)) {
if (!dotted) {
printf(" ...\n");
@@ -2960,7 +2958,10 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
if (praddr)
printf(REG" %s", adr, ppc_inst_as_str(inst));
printf("\t");
- dump_func(ppc_inst_val(inst), adr);
+ if (!ppc_inst_prefixed(inst))
+ dump_func(ppc_inst_val(inst), adr);
+ else
+ dump_func(ppc_inst_as_u64(inst), adr);
printf("\n");
}
return adr - first_adr;
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] powerpc: Handle prefixed instructions in show_user_instructions()
2020-06-02 5:27 [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Jordan Niethe
2020-06-02 5:27 ` [PATCH 2/4] powerpc/xmon: Improve dumping prefixed instructions Jordan Niethe
@ 2020-06-02 5:27 ` Jordan Niethe
2022-02-22 14:34 ` Christophe Leroy
2020-06-02 5:27 ` [PATCH 4/4] powerpc: Handle prefixed instructions in show_instructions() Jordan Niethe
` (3 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Jordan Niethe @ 2020-06-02 5:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Jordan Niethe, alistair
Currently prefixed instructions are treated as two word instructions by
show_user_instructions(), treat them as a single instruction. '<' and
'>' are placed around the instruction at the NIP, and for prefixed
instructions this is placed around the prefix only. Make the '<' and '>'
wrap the prefix and suffix.
Currently showing a prefixed instruction looks like:
fbe1fff8 39200000 06000000 a3e30000 <04000000> f7e40000 ebe1fff8 4e800020
Make it look like:
0xfbe1fff8 0x39200000 0x06000000 0xa3e30000 <0x04000000 0xf7e40000> 0xebe1fff8 0x4e800020 0x00000000 0x00000000
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/kernel/process.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 048d64c4e115..b3f73e398d00 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1292,7 +1292,8 @@ void show_user_instructions(struct pt_regs *regs)
unsigned long pc;
int n = NR_INSN_TO_PRINT;
struct seq_buf s;
- char buf[96]; /* enough for 8 times 9 + 2 chars */
+ char buf[8 * sizeof("0x00000000 0x00000000") + 2];
+ struct ppc_inst instr;
pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
@@ -1303,14 +1304,15 @@ void show_user_instructions(struct pt_regs *regs)
seq_buf_clear(&s);
- for (i = 0; i < 8 && n; i++, n--, pc += sizeof(int)) {
- int instr;
+ for (i = 0; i < 8 && n; i++, n--, pc += ppc_inst_len(instr)) {
- if (probe_user_read(&instr, (void __user *)pc, sizeof(instr))) {
+ if (probe_user_read_inst(&instr, (void __user *)pc)) {
seq_buf_printf(&s, "XXXXXXXX ");
+ instr = ppc_inst(PPC_INST_NOP);
continue;
}
- seq_buf_printf(&s, regs->nip == pc ? "<%08x> " : "%08x ", instr);
+ seq_buf_printf(&s, regs->nip == pc ? "<%s> " : "%s ",
+ ppc_inst_as_str(instr));
}
if (!seq_buf_has_overflowed(&s))
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] powerpc: Handle prefixed instructions in show_instructions()
2020-06-02 5:27 [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Jordan Niethe
2020-06-02 5:27 ` [PATCH 2/4] powerpc/xmon: Improve dumping prefixed instructions Jordan Niethe
2020-06-02 5:27 ` [PATCH 3/4] powerpc: Handle prefixed instructions in show_user_instructions() Jordan Niethe
@ 2020-06-02 5:27 ` Jordan Niethe
2020-06-02 7:43 ` [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Joel Stanley
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Jordan Niethe @ 2020-06-02 5:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Jordan Niethe, alistair
Currently show_instructions() treats prefixed instructions as two
separate word instructions. '<' and '>' are placed around the
instruction at the NIP, but as a result they only wrap around the
prefix. Make '<' and '>' straddle the whole prefixed instruction.
Currently showing a prefixed instruction looks like:
Instruction dump:
60000000 60000000 60000000 60000000 60000000 60000000 60000000 60000000
60000000 60000000 60000000 60000000 <04000000> 00000000 60000000 60000000
Make it look like:
Instruction dump:
0x60000000 0x60000000 0x60000000 0x60000000 0x60000000 0x60000000 0x60000000 0x60000000
0x60000000 0x60000000 0x60000000 0x60000000 <0x04000000 0x00000000> 0x60000000 0x60000000 0x60000000
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/kernel/process.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b3f73e398d00..bcd7277a9395 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1258,7 +1258,7 @@ static void show_instructions(struct pt_regs *regs)
printk("Instruction dump:");
for (i = 0; i < NR_INSN_TO_PRINT; i++) {
- int instr;
+ struct ppc_inst instr;
if (!(i % 8))
pr_cont("\n");
@@ -1272,16 +1272,17 @@ static void show_instructions(struct pt_regs *regs)
#endif
if (!__kernel_text_address(pc) ||
- probe_kernel_address((const void *)pc, instr)) {
+ probe_kernel_read_inst(&instr, (struct ppc_inst *)pc)) {
+ instr = ppc_inst(PPC_INST_NOP);
pr_cont("XXXXXXXX ");
} else {
if (regs->nip == pc)
- pr_cont("<%08x> ", instr);
+ pr_cont("<%s> ", ppc_inst_as_str(instr));
else
- pr_cont("%08x ", instr);
+ pr_cont("%s ", ppc_inst_as_str(instr));
}
- pc += sizeof(int);
+ pc += ppc_inst_len(instr);
}
pr_cont("\n");
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper
2020-06-02 5:27 [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Jordan Niethe
` (2 preceding siblings ...)
2020-06-02 5:27 ` [PATCH 4/4] powerpc: Handle prefixed instructions in show_instructions() Jordan Niethe
@ 2020-06-02 7:43 ` Joel Stanley
2020-06-02 19:41 ` Segher Boessenkool
2020-07-24 13:24 ` Michael Ellerman
5 siblings, 0 replies; 9+ messages in thread
From: Joel Stanley @ 2020-06-02 7:43 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev, Alistair Popple
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper
2020-06-02 5:27 [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Jordan Niethe
` (3 preceding siblings ...)
2020-06-02 7:43 ` [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Joel Stanley
@ 2020-06-02 19:41 ` Segher Boessenkool
2020-07-24 13:24 ` Michael Ellerman
5 siblings, 0 replies; 9+ messages in thread
From: Segher Boessenkool @ 2020-06-02 19:41 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev, alistair
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper
2020-06-02 5:27 [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Jordan Niethe
` (4 preceding siblings ...)
2020-06-02 19:41 ` Segher Boessenkool
@ 2020-07-24 13:24 ` Michael Ellerman
5 siblings, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2020-07-24 13:24 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] powerpc: Handle prefixed instructions in show_user_instructions()
2020-06-02 5:27 ` [PATCH 3/4] powerpc: Handle prefixed instructions in show_user_instructions() Jordan Niethe
@ 2022-02-22 14:34 ` Christophe Leroy
2022-03-14 23:00 ` Jordan Niethe
0 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2022-02-22 14:34 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair
Le 02/06/2020 à 07:27, Jordan Niethe a écrit :
> Currently prefixed instructions are treated as two word instructions by
> show_user_instructions(), treat them as a single instruction. '<' and
> '>' are placed around the instruction at the NIP, and for prefixed
> instructions this is placed around the prefix only. Make the '<' and '>'
> wrap the prefix and suffix.
>
> Currently showing a prefixed instruction looks like:
> fbe1fff8 39200000 06000000 a3e30000 <04000000> f7e40000 ebe1fff8 4e800020
>
> Make it look like:
> 0xfbe1fff8 0x39200000 0x06000000 0xa3e30000 <0x04000000 0xf7e40000> 0xebe1fff8 0x4e800020 0x00000000 0x00000000
Is it really needed to have the leading 0x ?
And is there a reason for that two 0x00000000 at the end of the new line
that we don't have at the end of the old line ?
This is initially split into 8 instructions per line in order to fit in
a 80 columns screen/terminal.
Could you make it such that it still fits within 80 cols ?
Same for patch 4 on show_user_instructions()
Christophe
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] powerpc: Handle prefixed instructions in show_user_instructions()
2022-02-22 14:34 ` Christophe Leroy
@ 2022-03-14 23:00 ` Jordan Niethe
0 siblings, 0 replies; 9+ messages in thread
From: Jordan Niethe @ 2022-03-14 23:00 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Alistair Popple
On Wed, Feb 23, 2022 at 1:34 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 02/06/2020 à 07:27, Jordan Niethe a écrit :
> > Currently prefixed instructions are treated as two word instructions by
> > show_user_instructions(), treat them as a single instruction. '<' and
> > '>' are placed around the instruction at the NIP, and for prefixed
> > instructions this is placed around the prefix only. Make the '<' and '>'
> > wrap the prefix and suffix.
> >
> > Currently showing a prefixed instruction looks like:
> > fbe1fff8 39200000 06000000 a3e30000 <04000000> f7e40000 ebe1fff8 4e800020
> >
> > Make it look like:
> > 0xfbe1fff8 0x39200000 0x06000000 0xa3e30000 <0x04000000 0xf7e40000> 0xebe1fff8 0x4e800020 0x00000000 0x00000000
>
> Is it really needed to have the leading 0x ?
You are right, that is not consistent with how instructions are usually dumped.
That formatting comes from ppc_inst_as_str(), which when mpe merged he
removed the leading 0x.
>
> And is there a reason for that two 0x00000000 at the end of the new line
> that we don't have at the end of the old line ?
No, that is wrong.
>
> This is initially split into 8 instructions per line in order to fit in
> a 80 columns screen/terminal.
>
> Could you make it such that it still fits within 80 cols ?
Sure that makes sense.
>
> Same for patch 4 on show_user_instructions()
>
> Christophe
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-03-14 23:01 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-02 5:27 [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Jordan Niethe
2020-06-02 5:27 ` [PATCH 2/4] powerpc/xmon: Improve dumping prefixed instructions Jordan Niethe
2020-06-02 5:27 ` [PATCH 3/4] powerpc: Handle prefixed instructions in show_user_instructions() Jordan Niethe
2022-02-22 14:34 ` Christophe Leroy
2022-03-14 23:00 ` Jordan Niethe
2020-06-02 5:27 ` [PATCH 4/4] powerpc: Handle prefixed instructions in show_instructions() Jordan Niethe
2020-06-02 7:43 ` [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper Joel Stanley
2020-06-02 19:41 ` Segher Boessenkool
2020-07-24 13:24 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.