Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
On 2/4/21 4:19 PM, Ravi Bangoria wrote: On 2/4/21 4:17 PM, Ravi Bangoria wrote: Don't allow Uprobe on 2nd word of a prefixed instruction. As per ISA 3.1, prefixed instruction should not cross 64-byte boundary. So don't allow Uprobe on such prefixed instruction as well. There are two ways probed instruction is changed in mapped pages. First, when Uprobe is activated, it searches for all the relevant pages and replace instruction in them. In this case, if we notice that probe is on the 2nd word of prefixed instruction, error out directly. Second, when Uprobe is already active and user maps a relevant page via mmap(), instruction is replaced via mmap() code path. But because Uprobe is invalid, entire mmap() operation can not be stopped. In this case just print an error and continue. @mpe, arch_uprobe_analyze_insn() can return early if cpu_has_feature(CPU_FTR_ARCH_31) is not set. But that will miss out a rare scenario of user running binary with prefixed instruction on p10 predecessors. Please let me know if I should add cpu_has_feature(CPU_FTR_ARCH_31) or not. Wouldn't that binary get a SIGILL in any case? I concur with Naveen... it makes sense to add the check. -- Ananth
[PATCH v2 2/2] powerpc/sstep: Fix incorrect return from analyze_instr()
We currently just percolate the return value from analyze_instr()
to the caller of emulate_step(), especially if it is a -1.
For one particular case (opcode = 4) for instructions that aren't
currently emulated, we are returning 'should not be single-stepped'
while we should have returned 0 which says 'did not emulate, may
have to single-step'.
Fixes: 930d6288a26787 ("powerpc: sstep: Add support for maddhd, maddhdu, maddld
instructions")
Signed-off-by: Ananth N Mavinakayanahalli
Suggested-by: Michael Ellerman
Tested-by: Naveen N. Rao
Reviewed-by: Sandipan Das
---
arch/powerpc/lib/sstep.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index f859cbbb6375..e96cff845ef7 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1445,6 +1445,11 @@ int analyse_instr(struct instruction_op *op, const
struct pt_regs *regs,
#ifdef __powerpc64__
case 4:
+ /*
+* There are very many instructions with this primary opcode
+* introduced in the ISA as early as v2.03. However, the ones
+* we currently emulate were all introduced with ISA 3.0
+*/
if (!cpu_has_feature(CPU_FTR_ARCH_300))
goto unknown_opcode;
@@ -1472,7 +1477,7 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
* There are other instructions from ISA 3.0 with the same
* primary opcode which do not have emulation support yet.
*/
- return -1;
+ goto unknown_opcode;
#endif
case 7: /* mulli */
[PATCH v4 1/2] [PATCH] powerpc/sstep: Check instruction validity against ISA version before emulation
We currently unconditionally try to emulate newer instructions on older
Power versions that could cause issues. Gate it.
Fixes: 350779a29f11 ("powerpc: Handle most loads and stores in instruction
emulation code")
Signed-off-by: Ananth N Mavinakayanahalli
---
[v4] Based on feedback from Paul Mackerras, Naveen Rao and Michael Ellerman,
changed return code to 0, after setting opcode type to UNKNOWN
[v3] Addressed Naveen's comments on scv and addpcis
[v2] Fixed description
---
arch/powerpc/lib/sstep.c | 78 +-
1 file changed, 62 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index bf7a7d62ae8b..f859cbbb6375 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1304,9 +1304,11 @@ int analyse_instr(struct instruction_op *op, const
struct pt_regs *regs,
if ((word & 0xfe2) == 2)
op->type = SYSCALL;
else if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) &&
- (word & 0xfe3) == 1)
+ (word & 0xfe3) == 1) { /* scv */
op->type = SYSCALL_VECTORED_0;
- else
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ goto unknown_opcode;
+ } else
op->type = UNKNOWN;
return 0;
#endif
@@ -1410,7 +1412,7 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
#ifdef __powerpc64__
case 1:
if (!cpu_has_feature(CPU_FTR_ARCH_31))
- return -1;
+ goto unknown_opcode;
prefix_r = GET_PREFIX_R(word);
ra = GET_PREFIX_RA(suffix);
@@ -1444,7 +1446,7 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
#ifdef __powerpc64__
case 4:
if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
+ goto unknown_opcode;
switch (word & 0x3f) {
case 48:/* maddhd */
@@ -1530,6 +1532,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 19:
if (((word >> 1) & 0x1f) == 2) {
/* addpcis */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ goto unknown_opcode;
imm = (short) (word & 0xffc1); /* d0 + d2 fields */
imm |= (word >> 15) & 0x3e; /* d1 field */
op->val = regs->nip + (imm << 16) + 4;
@@ -1842,7 +1846,7 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
#ifdef __powerpc64__
case 265: /* modud */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
+ goto unknown_opcode;
op->val = regs->gpr[ra] % regs->gpr[rb];
goto compute_done;
#endif
@@ -1852,7 +1856,7 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 267: /* moduw */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
+ goto unknown_opcode;
op->val = (unsigned int) regs->gpr[ra] %
(unsigned int) regs->gpr[rb];
goto compute_done;
@@ -1889,7 +1893,7 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
#endif
case 755: /* darn */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
+ goto unknown_opcode;
switch (ra & 0x3) {
case 0:
/* 32-bit conditioned */
@@ -1911,14 +1915,14 @@ int analyse_instr(struct instruction_op *op, const
struct pt_regs *regs,
#ifdef __powerpc64__
case 777: /* modsd */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
+ goto unknown_opcode;
op->val = (long int) regs->gpr[ra] %
(long int) regs->gpr[rb];
goto compute_done;
#endif
case 779: /* modsw */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
+ goto unknown_opcode;
op->val = (int) regs->gpr[ra] %
(int) regs->gpr[rb];
Re: [PATCH] lib/sstep: Fix incorrect return from analyze_instr()
On 1/23/21 6:03 AM, Michael Ellerman wrote:
Ananth N Mavinakayanahalli writes:
We currently just percolate the return value from analyze_instr()
to the caller of emulate_step(), especially if it is a -1.
For one particular case (opcode = 4) for instructions that
aren't currently emulated, we are returning 'should not be
single-stepped' while we should have returned 0 which says
'did not emulate, may have to single-step'.
Signed-off-by: Ananth N Mavinakayanahalli
Tested-by: Naveen N. Rao
---
arch/powerpc/lib/sstep.c | 49 +-
1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 5a425a4a1d88..a3a0373843cd 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1445,34 +1445,39 @@ int analyse_instr(struct instruction_op *op, const
struct pt_regs *regs,
#ifdef __powerpc64__
case 4:
- if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
-
- switch (word & 0x3f) {
- case 48:/* maddhd */
- asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
-"=r" (op->val) : "r" (regs->gpr[ra]),
-"r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
- goto compute_done;
+ /*
+* There are very many instructions with this primary opcode
+* introduced in the ISA as early as v2.03. However, the ones
+* we currently emulate were all introduced with ISA 3.0
+*/
+ if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+ switch (word & 0x3f) {
+ case 48:/* maddhd */
+ asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
+"=r" (op->val) : "r"
(regs->gpr[ra]),
+"r" (regs->gpr[rb]), "r"
(regs->gpr[rc]));
+ goto compute_done;
Indenting everything makes this patch harder to read, and I think makes
the resulting code harder to read too. We already have two levels of
switch here, and we're inside a ~1700 line function, so keeping things
simple is important I think.
Doesn't this achieve the same result?
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index bf7a7d62ae8b..d631baaf1da2 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1443,8 +1443,10 @@ int analyse_instr(struct instruction_op *op, const
struct pt_regs *regs,
#ifdef __powerpc64__
case 4:
- if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
+ op->type = UNKNOWN;
+ return 0;
+ }
switch (word & 0x3f) {
case 48:/* maddhd */
@@ -1470,7 +1472,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
* There are other instructions from ISA 3.0 with the same
* primary opcode which do not have emulation support yet.
*/
- return -1;
+ op->type = UNKNOWN;
+ return 0;
#endif
case 7: /* mulli */
Looks good to me.
Acked-by: Ananth N Mavinakayanahalli
--
Ananth
[PATCH] lib/sstep: Fix incorrect return from analyze_instr()
We currently just percolate the return value from analyze_instr()
to the caller of emulate_step(), especially if it is a -1.
For one particular case (opcode = 4) for instructions that
aren't currently emulated, we are returning 'should not be
single-stepped' while we should have returned 0 which says
'did not emulate, may have to single-step'.
Signed-off-by: Ananth N Mavinakayanahalli
Tested-by: Naveen N. Rao
---
arch/powerpc/lib/sstep.c | 49 +-
1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 5a425a4a1d88..a3a0373843cd 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1445,34 +1445,39 @@ int analyse_instr(struct instruction_op *op, const
struct pt_regs *regs,
#ifdef __powerpc64__
case 4:
- if (!cpu_has_feature(CPU_FTR_ARCH_300))
- return -1;
-
- switch (word & 0x3f) {
- case 48:/* maddhd */
- asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
-"=r" (op->val) : "r" (regs->gpr[ra]),
-"r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
- goto compute_done;
+ /*
+* There are very many instructions with this primary opcode
+* introduced in the ISA as early as v2.03. However, the ones
+* we currently emulate were all introduced with ISA 3.0
+*/
+ if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+ switch (word & 0x3f) {
+ case 48:/* maddhd */
+ asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
+"=r" (op->val) : "r"
(regs->gpr[ra]),
+"r" (regs->gpr[rb]), "r"
(regs->gpr[rc]));
+ goto compute_done;
- case 49:/* maddhdu */
- asm volatile(PPC_MADDHDU(%0, %1, %2, %3) :
-"=r" (op->val) : "r" (regs->gpr[ra]),
-"r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
- goto compute_done;
+ case 49:/* maddhdu */
+ asm volatile(PPC_MADDHDU(%0, %1, %2, %3) :
+"=r" (op->val) : "r"
(regs->gpr[ra]),
+"r" (regs->gpr[rb]), "r"
(regs->gpr[rc]));
+ goto compute_done;
- case 51:/* maddld */
- asm volatile(PPC_MADDLD(%0, %1, %2, %3) :
-"=r" (op->val) : "r" (regs->gpr[ra]),
-"r" (regs->gpr[rb]), "r" (regs->gpr[rc]));
- goto compute_done;
+ case 51:/* maddld */
+ asm volatile(PPC_MADDLD(%0, %1, %2, %3) :
+"=r" (op->val) : "r"
(regs->gpr[ra]),
+"r" (regs->gpr[rb]), "r"
(regs->gpr[rc]));
+ goto compute_done;
+ }
}
/*
-* There are other instructions from ISA 3.0 with the same
-* primary opcode which do not have emulation support yet.
+* Rest of the instructions with this primary opcode do not
+* have emulation support yet.
*/
- return -1;
+ op->type = UNKNOWN;
+ return 0;
#endif
case 7: /* mulli */
[PATCH v3] [PATCH] powerpc/sstep: Check ISA 3.0 instruction validity before emulation
We currently unconditionally try to emulate newer instructions on older
Power versions that could cause issues. Gate it.
Signed-off-by: Ananth N Mavinakayanahalli
---
[v3] Addressed Naveen's comments on scv and addpcis
[v2] Fixed description
arch/powerpc/lib/sstep.c | 46 --
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index bf7a7d62ae8b..5a425a4a1d88 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1304,9 +1304,11 @@ int analyse_instr(struct instruction_op *op, const
struct pt_regs *regs,
if ((word & 0xfe2) == 2)
op->type = SYSCALL;
else if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) &&
- (word & 0xfe3) == 1)
+ (word & 0xfe3) == 1) { /* scv */
op->type = SYSCALL_VECTORED_0;
- else
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
+ } else
op->type = UNKNOWN;
return 0;
#endif
@@ -1530,6 +1532,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 19:
if (((word >> 1) & 0x1f) == 2) {
/* addpcis */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
imm = (short) (word & 0xffc1); /* d0 + d2 fields */
imm |= (word >> 15) & 0x3e; /* d1 field */
op->val = regs->nip + (imm << 16) + 4;
@@ -2439,6 +2443,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 268: /* lxvx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 16);
op->element_size = 16;
@@ -2448,6 +2454,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 269: /* lxvl */
case 301: { /* lxvll */
int nb;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->ea = ra ? regs->gpr[ra] : 0;
nb = regs->gpr[rb] & 0xff;
@@ -2475,6 +2483,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 364: /* lxvwsx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 4);
op->element_size = 4;
@@ -2482,6 +2492,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 396: /* stxvx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(STORE_VSX, 0, 16);
op->element_size = 16;
@@ -2491,6 +2503,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 397: /* stxvl */
case 429: { /* stxvll */
int nb;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->ea = ra ? regs->gpr[ra] : 0;
nb = regs->gpr[rb] & 0xff;
@@ -2542,6 +2556,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 781: /* lxsibzx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 1);
op->element_size = 8;
@@ -2549,6 +2565,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 812: /* lxvh8x */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1)
Re: [PATCH] [PATCH] powerpc/sstep: Check ISA 3.0 instruction validity before emulation
On 1/20/21 3:44 PM, Naveen N. Rao wrote:
On 2021/01/20 03:16PM, Ananth N Mavinakayanahalli wrote:
...
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index bf7a7d62ae8b..ed119858e5e9 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1528,6 +1528,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
goto compute_done;
case 19:
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
if (((word >> 1) & 0x1f) == 2) {
/* addpcis */
imm = (short) (word & 0xffc1); /* d0 + d2 fields */
The cpu feature check should be within the if condition above since
there are other instructions under opcode 19. This is not an issue right
now as we don't emulate any of the others after this point, but it would
be good to restrict the change to specific instructions.
Rest of the changes below look good to me. The only other v3.0
instruction we need to gate is the 'scv' instruction. It would be good
to handle that too.
I missed this in v2.. will send a v3. There is a bigger change needed to
accommodate scv since we currently don't check for it in analyze_insn().
--
Ananth
[PATCH] [PATCH V2] powerpc/sstep: Check ISA 3.0 instruction validity before emulation
We currently unconditionally try to emulate newer instructions on older
Power versions that could cause issues. Gate it.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/lib/sstep.c | 40
1 file changed, 40 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index bf7a7d62ae8b..ed119858e5e9 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1528,6 +1528,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
goto compute_done;
case 19:
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
if (((word >> 1) & 0x1f) == 2) {
/* addpcis */
imm = (short) (word & 0xffc1); /* d0 + d2 fields */
@@ -2439,6 +2441,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 268: /* lxvx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 16);
op->element_size = 16;
@@ -2448,6 +2452,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 269: /* lxvl */
case 301: { /* lxvll */
int nb;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->ea = ra ? regs->gpr[ra] : 0;
nb = regs->gpr[rb] & 0xff;
@@ -2475,6 +2481,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 364: /* lxvwsx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 4);
op->element_size = 4;
@@ -2482,6 +2490,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 396: /* stxvx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(STORE_VSX, 0, 16);
op->element_size = 16;
@@ -2491,6 +2501,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 397: /* stxvl */
case 429: { /* stxvll */
int nb;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->ea = ra ? regs->gpr[ra] : 0;
nb = regs->gpr[rb] & 0xff;
@@ -2542,6 +2554,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 781: /* lxsibzx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 1);
op->element_size = 8;
@@ -2549,6 +2563,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 812: /* lxvh8x */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 16);
op->element_size = 2;
@@ -2556,6 +2572,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 813: /* lxsihzx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 2);
op->element_size = 8;
@@ -2569,6 +2587,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 876: /* lxvb16x */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((wor
[PATCH] [PATCH] powerpc/sstep: Check ISA 3.0 instruction validity before emulation
We currently unconditionally try to newer emulate instructions on older
Power versions that could cause issues. Gate it.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/lib/sstep.c | 40
1 file changed, 40 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index bf7a7d62ae8b..ed119858e5e9 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1528,6 +1528,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
goto compute_done;
case 19:
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
if (((word >> 1) & 0x1f) == 2) {
/* addpcis */
imm = (short) (word & 0xffc1); /* d0 + d2 fields */
@@ -2439,6 +2441,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 268: /* lxvx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 16);
op->element_size = 16;
@@ -2448,6 +2452,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 269: /* lxvl */
case 301: { /* lxvll */
int nb;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->ea = ra ? regs->gpr[ra] : 0;
nb = regs->gpr[rb] & 0xff;
@@ -2475,6 +2481,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 364: /* lxvwsx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 4);
op->element_size = 4;
@@ -2482,6 +2490,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 396: /* stxvx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(STORE_VSX, 0, 16);
op->element_size = 16;
@@ -2491,6 +2501,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
case 397: /* stxvl */
case 429: { /* stxvll */
int nb;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->ea = ra ? regs->gpr[ra] : 0;
nb = regs->gpr[rb] & 0xff;
@@ -2542,6 +2554,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 781: /* lxsibzx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 1);
op->element_size = 8;
@@ -2549,6 +2563,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 812: /* lxvh8x */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 16);
op->element_size = 2;
@@ -2556,6 +2572,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 813: /* lxsihzx */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((word & 1) << 5);
op->type = MKOP(LOAD_VSX, 0, 2);
op->element_size = 8;
@@ -2569,6 +2587,8 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
break;
case 876: /* lxvb16x */
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ return -1;
op->reg = rd | ((wor
Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.
On 10/5/20 9:42 AM, Mahesh Salgaonkar wrote: Every error log reported by OPAL is exported to userspace through a sysfs interface and notified using kobject_uevent(). The userspace daemon (opal_errd) then reads the error log and acknowledges it error log is saved safely to disk. Once acknowledged the kernel removes the respective sysfs file entry causing respective resources getting released including kobject. However there are chances where user daemon may already be scanning elog entries while new sysfs elog entry is being created by kernel. User daemon may read this new entry and ack it even before kernel can notify userspace about it through kobject_uevent() call. If that happens then we have a potential race between elog_ack_store->kobject_put() and kobject_uevent which can lead to use-after-free issue of a kernfs object resulting into a kernel crash. This patch fixes this race by protecting a sysfs file creation/notification by holding an additional reference count on kobject until we safely send kobject_uevent(). Reported-by: Oliver O'Halloran Signed-off-by: Mahesh Salgaonkar Signed-off-by: Aneesh Kumar K.V cc stable? -- Ananth
Re: [PATCH v7 8/9] powerpc/mce: Add sysctl control for recovery action on MCE.
On Thu, Aug 09, 2018 at 06:02:53PM +1000, Nicholas Piggin wrote: > On Thu, 09 Aug 2018 16:34:07 +1000 > Michael Ellerman wrote: > > > "Aneesh Kumar K.V" writes: > > > On 08/08/2018 08:26 PM, Michael Ellerman wrote: > > >> Mahesh J Salgaonkar writes: > > >>> From: Mahesh Salgaonkar > > >>> > > >>> Introduce recovery action for recovered memory errors (MCEs). There are > > >>> soft memory errors like SLB Multihit, which can be a result of a bad > > >>> hardware OR software BUG. Kernel can easily recover from these soft > > >>> errors > > >>> by flushing SLB contents. After the recovery kernel can still continue > > >>> to > > >>> function without any issue. But in some scenario's we may keep getting > > >>> these soft errors until the root cause is fixed. To be able to analyze > > >>> and > > >>> find the root cause, best way is to gather enough data and system state > > >>> at > > >>> the time of MCE. Hence this patch introduces a sysctl knob where user > > >>> can > > >>> decide either to continue after recovery or panic the kernel to capture > > >>> the > > >>> dump. > > >> > > >> I'm not convinced we want this. > > >> > > >> As we've discovered it's often not possible to reconstruct what happened > > >> based on a dump anyway. > > >> > > >> The key thing you need is the content of the SLB and that's not included > > >> in a dump. > > >> > > >> So I think we should dump the SLB content when we get the MCE (which > > >> this series does) and any other useful info, and then if we can recover > > >> we should. > > > > > > The reasoning there is what if we got multi-hit due to some corruption > > > in slb_cache_ptr. ie. some part of kernel is wrongly updating the paca > > > data structure due to wrong pointer. Now that is far fetched, but then > > > possible right?. Hence the idea that, if we don't have much insight into > > > why a slb multi-hit occur from the dmesg which include slb content, > > > slb_cache contents etc, there should be an easy way to force a dump that > > > might assist in further debug. > > > > If you're debugging something complex that you can't determine from the > > SLB dump then you should be running a debug kernel anyway. And if > > anything you want to drop into xmon and sit there, preserving the most > > state, rather than taking a dump. > > I'm not saying for a dump specifically, just some form of crash. And we > really should have an option to xmon on panic, but that's another story. That's fine during development or in a lab, not something we could enforce in a customer environment, could we? > I think HA/failover kind of environments use options like this too. If > anything starts going bad they don't want to try limping along but stop > ASAP. Right. And in this particular case, can we guarantee no corruption (leading to or post the multihit recovery) when running a customer workload, is the question... Ananth
Re: [PATCH] powerpc/kprobes: Fix call trace due to incorrect preempt count
On Wed, Jan 17, 2018 at 05:52:24PM +0530, Naveen N. Rao wrote: > Michael Ellerman reported the following call trace when running > ftracetest: > > BUG: using __this_cpu_write() in preemptible [] code: ftracetest/6178 > caller is opt_pre_handler+0xc4/0x110 > CPU: 1 PID: 6178 Comm: ftracetest Not tainted 4.15.0-rc7-gcc6x-gb2cd1df #1 > Call Trace: > [c000f9ec39c0] [c0ac4304] dump_stack+0xb4/0x100 (unreliable) > [c000f9ec3a00] [c061159c] check_preemption_disabled+0x15c/0x170 > [c000f9ec3a90] [c0217e84] opt_pre_handler+0xc4/0x110 > [c000f9ec3af0] [c004cf68] optimized_callback+0x148/0x170 > [c000f9ec3b40] [c004d954] optinsn_slot+0xec/0x1 > [c000f9ec3e30] [c004bae0] kretprobe_trampoline+0x0/0x10 > > This is showing up since OPTPROBES is now enabled with CONFIG_PREEMPT. > > trampoline_probe_handler() considers itself to be a special kprobe > handler for kretprobes. In doing so, it expects to be called from > kprobe_handler() on a trap, and re-enables preemption before returning a > non-zero return value so as to suppress any subsequent processing of the > trap by the kprobe_handler(). > > However, with optprobes, we don't deal with special handlers (we ignore > the return code) and just try to re-enable preemption causing the above > trace. > > To address this, modify trampoline_probe_handler() to not be special. > The only additional processing done in kprobe_handler() is to emulate > the instruction (in this case, a 'nop'). We adjust the value of > regs->nip for the purpose and delegate the job of re-enabling > preemption and resetting current kprobe to the probe handlers > (kprobe_handler() or optimized_callback()). > > Reported-by: Michael Ellerman > Signed-off-by: Naveen N. Rao Acked-by: Ananth N Mavinakayanahalli
Re: [PATCH v2] ppc64/kprobe: Fix oops when kprobed on 'stdu' instruction
On Tue, Apr 11, 2017 at 10:38:13AM +0530, Ravi Bangoria wrote:
> If we set a kprobe on a 'stdu' instruction on powerpc64, we see a kernel
> OOPS:
>
> [ 1275.165932] Bad kernel stack pointer cd93c840 at c0009868
> [ 1275.166378] Oops: Bad kernel stack pointer, sig: 6 [#1]
> ...
> GPR00: c01fcd93cb30 cd93c840 c15c5e00 cd93c840
> ...
> [ 1275.178305] NIP [c0009868] resume_kernel+0x2c/0x58
> [ 1275.178594] LR [c0006208] program_check_common+0x108/0x180
>
> Basically, on 64 bit system, when user probes on 'stdu' instruction,
> kernel does not emulate actual store in emulate_step itself because it
> may corrupt exception frame. So kernel does actual store operation in
> exception return code i.e. resume_kernel().
>
> resume_kernel() loads the saved stack pointer from memory using lwz,
> effectively loading a corrupt (32bit) address, causing the kernel crash.
>
> Fix this by loading the 64bit value instead.
>
> Fixes: be96f63375a1 ("powerpc: Split out instruction analysis part of
> emulate_step()")
> Signed-off-by: Ravi Bangoria
> Reviewed-by: Naveen N. Rao
Reviewed-by: Ananth N Mavinakayanahalli
Re: [PATCH v2 2/5] powerpc: kretprobes: override default function entry offset
On Wed, Feb 22, 2017 at 07:23:38PM +0530, Naveen N. Rao wrote: > With ABIv2, we offset 8 bytes into a function to get at the local entry > point. > Looks good. > Signed-off-by: Naveen N. Rao Acked-by: Ananth N Mavinakayanahalli
Re: [PATCH 1/3] powerpc: kprobes: add support for KPROBES_ON_FTRACE
On Wed, Feb 15, 2017 at 12:28:34AM +0530, Naveen N. Rao wrote:
> Allow kprobes to be placed on ftrace _mcount() call sites. This
> optimization avoids the use of a trap, by riding on ftrace
> infrastructure.
>
> This depends on HAVE_DYNAMIC_FTRACE_WITH_REGS which depends on
> MPROFILE_KERNEL, which is only currently enabled on powerpc64le with
> newer toolchains.
>
> Based on the x86 code by Masami.
>
> Signed-off-by: Naveen N. Rao
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/kprobes.h | 10
> arch/powerpc/kernel/Makefile | 3 ++
> arch/powerpc/kernel/kprobes-ftrace.c | 100
> +++
> arch/powerpc/kernel/kprobes.c| 4 +-
> arch/powerpc/kernel/optprobes.c | 3 ++
> 6 files changed, 120 insertions(+), 1 deletion(-)
> create mode 100644 arch/powerpc/kernel/kprobes-ftrace.c
You'll also need to update
Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> +/* Ftrace callback handler for kprobes */
> +void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
> +struct ftrace_ops *ops, struct pt_regs *regs)
> +{
> + struct kprobe *p;
> + struct kprobe_ctlblk *kcb;
> + unsigned long flags;
> +
> + /* Disable irq for emulating a breakpoint and avoiding preempt */
> + local_irq_save(flags);
> + hard_irq_disable();
> +
> + p = get_kprobe((kprobe_opcode_t *)nip);
> + if (unlikely(!p) || kprobe_disabled(p))
> + goto end;
> +
> + kcb = get_kprobe_ctlblk();
> + if (kprobe_running()) {
> + kprobes_inc_nmissed_count(p);
> + } else {
> + unsigned long orig_nip = regs->nip;
> + /* Kprobe handler expects regs->nip = nip + 1 as breakpoint hit
> */
Can you clarify this? On powerpc, the regs->nip at the time of
breakpoint hit points to the probed instruction, not the one after.
Ananth
Re: [PATCH 3/3] powerpc: kprobes: emulate instructions on kprobe handler re-entry
On Tue, Feb 14, 2017 at 02:08:03PM +0530, Naveen N. Rao wrote: > On kprobe handler re-entry, try to emulate the instruction rather than > single stepping always. > > As a related change, remove the duplicate saving of msr as that is > already done in set_current_kprobe() > > Signed-off-by: Naveen N. Rao Acked-by: Ananth N Mavinakayanahalli
Re: [PATCH 2/3] powerpc: kprobes: factor out code to emulate instruction into a helper
On Tue, Feb 14, 2017 at 02:08:02PM +0530, Naveen N. Rao wrote: > This helper will be used in a subsequent patch to emulate instructions > on re-entering the kprobe handler. No functional change. > > Signed-off-by: Naveen N. Rao Acked-by: Ananth N Mavinakayanahalli
Re: [PATCH 1/3] powerpc: kprobes: fix handling of function offsets on ABIv2
On Tue, Feb 14, 2017 at 02:08:01PM +0530, Naveen N. Rao wrote:
> commit 239aeba76409 ("perf powerpc: Fix kprobe and kretprobe handling
> with kallsyms on ppc64le") changed how we use the offset field in struct
> kprobe on ABIv2. perf now offsets from the GEP (Global entry point) if an
> offset is specified and otherwise chooses the LEP (Local entry point).
>
> Fix the same in kernel for kprobe API users. We do this by extending
> kprobe_lookup_name() to accept an additional parameter to indicate the
> offset specified with the kprobe registration. If offset is 0, we return
> the local function entry and return the global entry point otherwise.
>
> With:
> # cd /sys/kernel/debug/tracing/
> # echo "p _do_fork" >> kprobe_events
> # echo "p _do_fork+0x10" >> kprobe_events
>
> before this patch:
> # cat ../kprobes/list
> c00d0748 k _do_fork+0x8[DISABLED]
> c00d0758 k _do_fork+0x18[DISABLED]
> c00412b0 k kretprobe_trampoline+0x0[OPTIMIZED]
>
> and after:
> # cat ../kprobes/list
> c00d04c8 k _do_fork+0x8[DISABLED]
> c00d04d0 k _do_fork+0x10[DISABLED]
> c00412b0 k kretprobe_trampoline+0x0[OPTIMIZED]
>
> Signed-off-by: Naveen N. Rao
Acked-by: Ananth N Mavinakayanahalli
Re: [PATCH] powerpc: kprobes: invoke handlers directly
On Fri, Nov 18, 2016 at 05:09:26PM +0530, Naveen N. Rao wrote:
> ... rather than through notify_die(), to reduce path taken for handling
> kprobes. Similar to commit 6f6343f53d13 ("kprobes/x86: Call exception
> handlers directly from do_int3/do_debug").
>
> While at it, rename post_kprobe_handler() to kprobe_post_handler() for
> more uniform naming.
>
> Reported-by: Masami Hiramatsu
> Signed-off-by: Naveen N. Rao
Acked-by: Ananth N Mavinakayanahalli
Re: [PATCH 1/2] perf/powerpc: Fix kprobe and kretprobe handling with kallsyms
On Wed, Apr 06, 2016 at 06:02:57PM +0530, Naveen N. Rao wrote: > + if (!pev->uprobes && map->dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS) > tev->point.offset += PPC64LE_LEP_OFFSET; uprobes check against kallsysms? Am I missing something here? Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] tools/perf: Fix kallsyms perf test on ppc64le
On Wed, Apr 06, 2016 at 06:02:58PM +0530, Naveen N. Rao wrote: > ppc64le functions have a Global Entry Point (GEP) and a Local Entry > Point (LEP). While placing a probe, we always prefer the LEP since it > catches function calls through both the GEP and the LEP. In order to do > this, we fixup the function entry points during elf symbol table lookup > to point to the LEPs. This works, but breaks 'perf test kallsyms' since > the symbols loaded from the symbol table (pointing to the LEP) do not > match the symbols in kallsyms. > > To fix this, we do not adjust all the symbols during symbol table load, > but only adjust the probe trace point. > > Cc: Mark Wielaard > Cc: Thiago Jung Bauermann > Cc: Ananth N Mavinakayanahalli > Cc: Arnaldo Carvalho de Melo > Cc: Masami Hiramatsu > Reported-by: Michael Ellerman > Signed-off-by: Naveen N. Rao Acked-by: Ananth N Mavinakayanahalli ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: powerpc: Add user-return-notifier support
On Tue, Sep 08, 2015 at 07:24:39PM +1000, Michael Ellerman wrote: > On Wed, 2015-09-02 at 10:39 +0530, Ananth N Mavinakayanahalli wrote: > > On Tue, Sep 01, 2015 at 10:29:12PM -0500, Scott Wood wrote: > > > > > Why is this selected by KVM on PPC if KVM on PPC doesn't use it? What is > > > the > > > user you're trying to enable? > > > > I copied Paul and Gautham to get their thoughts on whether it is > > something they could use on Power. At this time, apart from enabling the > > feature, I do not have a specific usecase for it. We can park this patch > > pending a real user on powerpc. > > OK, thanks for hashing that out guys. I think parking it is the best solution > for now, we could merge it and disable it somehow, but that would be ugly and > prone to breaking. > > Do we want to do something like this? Yes. This looks fine -- folks who want it down the line know where to look. Thanks, Ananth > > cheers > > diff --git a/Documentation/features/arch-support.txt > b/Documentation/features/arch-support.txt > index d22a1095e661..3e2cf6161cf4 100644 > --- a/Documentation/features/arch-support.txt > +++ b/Documentation/features/arch-support.txt > @@ -8,4 +8,5 @@ The meaning of entries in the tables is: > | ok | # feature supported by the architecture > |TODO| # feature not yet supported by the architecture > | .. | # feature cannot be supported by the hardware > +|n/a | # feature is not needed/wanted/meaningful for the architecture > > diff --git a/Documentation/features/debug/user-ret-profiler/arch-support.txt > b/Documentation/features/debug/user-ret-profiler/arch-support.txt > index 44cc1ff3f603..669813180dc5 100644 > --- a/Documentation/features/debug/user-ret-profiler/arch-support.txt > +++ b/Documentation/features/debug/user-ret-profiler/arch-support.txt > @@ -27,7 +27,7 @@ > | nios2: | TODO | > |openrisc: | TODO | > | parisc: | TODO | > -| powerpc: | TODO | > +| powerpc: | n/a |# > http://lkml.kernel.org/r/20150825054110.gd3...@in.ibm.com > |s390: | TODO | > | score: | TODO | > | sh: | TODO | > > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: powerpc: Add user-return-notifier support
On Tue, Sep 01, 2015 at 10:29:12PM -0500, Scott Wood wrote: > On Wed, 2015-09-02 at 08:07 +0530, Ananth N Mavinakayanahalli wrote: > > On Tue, Sep 01, 2015 at 07:03:12PM -0500, Scott Wood wrote: > > > On Tue, 2015-09-01 at 12:11 +0530, Ananth N Mavinakayanahalli wrote: > > > > On Mon, Aug 31, 2015 at 08:35:17PM +1000, Michael Ellerman wrote: > > > > > On Tue, 2015-25-08 at 05:41:10 UTC, Ananth N Mavinakayanahalli wrote: ... > > > > We could certainly make this a generic config option.. but I am yet to > > > > see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for > > > > something very similar, though that needs to fire much before the > > > > outstanding signals get handled. The user-return notifier is the last > > > > thing executed before return to userspace -- the two cannot be merged. > > > > > > > > Ananth > > > > > > > > PS: I am not sure having an 'ok' against the Documentation/features/ for > > > > powerpc is a valid enough argument for this :-) > > > > > > So how did you test this? What platforms did you test it on? What > > > hardware > > > support does it need? > > > > Simple kernel module below... Its fairly evident from the patch that the > > feature is a software only construct and no specific hardware support is > > needed. > > It's evident now that I'm aware of what the purpose is... It wasn't 100% > > clear before, at least from a quick glance, given that the explanation was > > "look at this hardware-specific KVM patch", whether there was an assumption > of something else going on -- and even things that are implemented in > software often work differently on book3s versus book3e, ppc32 versus ppc64, > etc. I meant the infrastructure is agnostic of the underlying Power platform. Sorry if I wasn't clear. > Why is this selected by KVM on PPC if KVM on PPC doesn't use it? What is the > user you're trying to enable? I copied Paul and Gautham to get their thoughts on whether it is something they could use on Power. At this time, apart from enabling the feature, I do not have a specific usecase for it. We can park this patch pending a real user on powerpc. > Where is the "profiler" that Documentation/features/debug/user-ret- > profiler/arch-support.txt hints at? I guess profiler is a misnomer. Not sure why it was named that and not a notifier. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: powerpc: Add user-return-notifier support
On Tue, Sep 01, 2015 at 07:03:12PM -0500, Scott Wood wrote:
> On Tue, 2015-09-01 at 12:11 +0530, Ananth N Mavinakayanahalli wrote:
> > On Mon, Aug 31, 2015 at 08:35:17PM +1000, Michael Ellerman wrote:
> > > On Tue, 2015-25-08 at 05:41:10 UTC, Ananth N Mavinakayanahalli wrote:
> > > > Add user return notifier support for powerpc. Similar to x86, this
> > > > feature
> > > > keys off of the KVM Kconfig.
> > >
> > > Please flesh this out.
> > >
> > > What is it, why do we want it, why is your implementation correct.
> >
> > The only current in-kernel user of the infrastructure is KVM on x86. It
> > is useful for optimizations when MSR values can continue to be used
> > between the host and guest. Commit log for 18863bdd60f8 upstream has a
> > more complete explanation.
> >
> > I do not know the inner details of the KVM implementation on Power,
> > perhaps Paul/Gautham can comment on if a similar optimization will
> > benefit Power systems too?
>
> "MSR" is x86-specific terminology and is pretty vague. What specifically is
> the functionality being optimized, in terms of things that actually exist on
> PPC?
>
> In any case, that commit log doesn't explain what user-return-notifier is or
> how it works, just that it's being used.
User return notifier is a simple mechanism to fire a one-shot custom handler in
the context of the thread its registered against, just before it returns to
userspace. It works by setting a TIF flag to indicate that a handler needs to
run, and on the userspace return path (do_notify_resume()), this flag is checked
to call any registered handlers.
> > We could certainly make this a generic config option.. but I am yet to
> > see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for
> > something very similar, though that needs to fire much before the
> > outstanding signals get handled. The user-return notifier is the last
> > thing executed before return to userspace -- the two cannot be merged.
> >
> > Ananth
> >
> > PS: I am not sure having an 'ok' against the Documentation/features/ for
> > powerpc is a valid enough argument for this :-)
>
> So how did you test this? What platforms did you test it on? What hardware
> support does it need?
Simple kernel module below... Its fairly evident from the patch that the
feature is a software only construct and no specific hardware support is
needed. I've tested this on a P7.
Ananth
---
/* uret test module */
#include
#include
#include
#include
struct user_return_notifier urn;
static int count;
static void test_on_user_return(struct user_return_notifier *urn)
{
count++;
}
static int init_urn(void)
{
urn.on_user_return = test_on_user_return;
user_return_notifier_register(&urn);
printk("urn registered\n");
return 0;
}
static void cleanup_urn(void)
{
user_return_notifier_unregister(&urn);
printk("urn unregistered; count = %d\n", count);
}
module_init(init_urn);
module_exit(cleanup_urn);
MODULE_LICENSE("GPL");
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: powerpc: Add user-return-notifier support
On Mon, Aug 31, 2015 at 08:35:17PM +1000, Michael Ellerman wrote: > On Tue, 2015-25-08 at 05:41:10 UTC, Ananth N Mavinakayanahalli wrote: > > Add user return notifier support for powerpc. Similar to x86, this feature > > keys off of the KVM Kconfig. > > Please flesh this out. > > What is it, why do we want it, why is your implementation correct. The only current in-kernel user of the infrastructure is KVM on x86. It is useful for optimizations when MSR values can continue to be used between the host and guest. Commit log for 18863bdd60f8 upstream has a more complete explanation. I do not know the inner details of the KVM implementation on Power, perhaps Paul/Gautham can comment on if a similar optimization will benefit Power systems too? We could certainly make this a generic config option.. but I am yet to see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for something very similar, though that needs to fire much before the outstanding signals get handled. The user-return notifier is the last thing executed before return to userspace -- the two cannot be merged. Ananth PS: I am not sure having an 'ok' against the Documentation/features/ for powerpc is a valid enough argument for this :-) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Add user-return-notifier support
Add user return notifier support for powerpc. Similar to x86, this feature keys off of the KVM Kconfig. Signed-off-by: Ananth N Mavinakayanahalli --- Documentation/features/debug/user-ret-profiler/arch-support.txt |2 +- arch/powerpc/Kconfig|1 + arch/powerpc/include/asm/thread_info.h |2 ++ arch/powerpc/kernel/process.c |4 arch/powerpc/kernel/signal.c|4 arch/powerpc/kvm/Kconfig|1 + 6 files changed, 13 insertions(+), 1 deletion(-) Index: linux-4.2-rc6/Documentation/features/debug/user-ret-profiler/arch-support.txt === --- linux-4.2-rc6.orig/Documentation/features/debug/user-ret-profiler/arch-support.txt +++ linux-4.2-rc6/Documentation/features/debug/user-ret-profiler/arch-support.txt @@ -27,7 +27,7 @@ | nios2: | TODO | |openrisc: | TODO | | parisc: | TODO | -| powerpc: | TODO | +| powerpc: | ok | |s390: | TODO | | score: | TODO | | sh: | TODO | Index: linux-4.2-rc6/arch/powerpc/Kconfig === --- linux-4.2-rc6.orig/arch/powerpc/Kconfig +++ linux-4.2-rc6/arch/powerpc/Kconfig @@ -106,6 +106,7 @@ config PPC select HAVE_ARCH_KGDB select HAVE_KRETPROBES select HAVE_ARCH_TRACEHOOK + select HAVE_USER_RETURN_NOTIFIER select HAVE_MEMBLOCK select HAVE_MEMBLOCK_NODE_MAP select HAVE_DMA_ATTRS Index: linux-4.2-rc6/arch/powerpc/include/asm/thread_info.h === --- linux-4.2-rc6.orig/arch/powerpc/include/asm/thread_info.h +++ linux-4.2-rc6/arch/powerpc/include/asm/thread_info.h @@ -86,6 +86,7 @@ static inline struct thread_info *curren TIF_NEED_RESCHED */ #define TIF_32BIT 4 /* 32 bit binary */ #define TIF_RESTORE_TM 5 /* need to restore TM FP/VEC/VSX */ +#define TIF_USER_RETURN_NOTIFY 6 /* notify kernel of userspace return */ #define TIF_SYSCALL_AUDIT 7 /* syscall auditing active */ #define TIF_SINGLESTEP 8 /* singlestepping active */ #define TIF_NOHZ 9 /* in adaptive nohz mode */ @@ -109,6 +110,7 @@ static inline struct thread_info *curren #define _TIF_POLLING_NRFLAG(1< #include #include +#include #include #include @@ -894,6 +895,9 @@ struct task_struct *__switch_to(struct t } #endif /* CONFIG_PPC_BOOK3S_64 */ + if (unlikely(task_thread_info(prev)->flags & _TIF_USER_RETURN_NOTIFY)) + propagate_user_return_notify(prev, new); + return last; } Index: linux-4.2-rc6/arch/powerpc/kernel/signal.c === --- linux-4.2-rc6.orig/arch/powerpc/kernel/signal.c +++ linux-4.2-rc6/arch/powerpc/kernel/signal.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -159,6 +160,9 @@ void do_notify_resume(struct pt_regs *re tracehook_notify_resume(regs); } + if (thread_info_flags & _TIF_USER_RETURN_NOTIFY) + fire_user_return_notifiers(); + user_enter(); } Index: linux-4.2-rc6/arch/powerpc/kvm/Kconfig === --- linux-4.2-rc6.orig/arch/powerpc/kvm/Kconfig +++ linux-4.2-rc6/arch/powerpc/kvm/Kconfig @@ -22,6 +22,7 @@ config KVM select ANON_INODES select HAVE_KVM_EVENTFD select SRCU + select USER_RETURN_NOTIFIER config KVM_BOOK3S_HANDLER bool ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] kprobes: Mark OPTPROBES n/a for powerpc
On Tue, Jul 21, 2015 at 12:53:07PM +1000, Michael Ellerman wrote: > On Sun, 2015-07-19 at 11:21 +0900, Masami Hiramatsu wrote: > > On 2015/07/16 19:56, Ananth N Mavinakayanahalli wrote: > > > Kprobes uses a breakpoint instruction to trap into execution flow > > > and the probed instruction is single-stepped from an alternate location. > > > > > > On some architectures like x86, under certain conditions, the OPTPROBES > > > feature enables replacing the probed instruction with a jump instead, > > > resulting in a significant perfomance boost (one single-step exception > > > is bypassed for each kprobe). > > > > The OPTPROBE is not only for bypassing the single-step exception, but also > > the breakpoint exception. > > Please see commit 0dc016dbd820260b (ARM: kprobes: enable OPTPROBES for ARM > > 32) too, > > which shows how it is done on RISC processor. > > > > > Powerpc has an in-kernel instruction emulator. Kprobes on powerpc uses > > > this emulator already and bypasses the single-step exception, with a > > > lot less complexity. > > > > So, this might miss the point. Since it is impossible to do on some RISC > > processor, I agree with this change, but it should be committed with > > correct comments. > > I don't think it's impossible on powerpc. > > So we should leave it as a TODO for now. OK. I put it on my TODO list. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V3 2/2] kprobes: Mark OPTPROBES na for powerpc
Kprobes uses a breakpoint instruction to trap into execution flow and the probed instruction is single-stepped from an alternate location. On some architectures like x86, under certain conditions, the OPTPROBES feature enables replacing the probed instruction with a jump instead, resulting in a significant perfomance boost (both the breakpoint and single-step exception is bypassed for each kprobe). Powerpc has an in-kernel instruction emulator. Kprobes on powerpc uses this emulator already and bypasses the single-step exception, with a lot less complexity. There is a potential gain to be had with a direct jump instead of a breakpoint, but the caveats need to be traded off with the complexity it brings in. For now, mark OPTPROBES na for powerpc. Signed-off-by: Ananth N Mavinakayanahalli --- .../features/debug/optprobes/arch-support.txt |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/features/debug/optprobes/arch-support.txt b/Documentation/features/debug/optprobes/arch-support.txt index b8999d8..73662f9 100644 --- a/Documentation/features/debug/optprobes/arch-support.txt +++ b/Documentation/features/debug/optprobes/arch-support.txt @@ -27,7 +27,7 @@ | nios2: | TODO | |openrisc: | TODO | | parisc: | TODO | -| powerpc: | TODO | +| powerpc: | na | |s390: | TODO | | score: | TODO | | sh: | TODO | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V3 1/2] Documentation/features: Add na key to arch-support.txt
To be used for features we will not support on a particular architecture. The git log that adds this needs to provide the justification 'why?' Signed-off-by: Ananth N Mavinakayanahalli --- Documentation/features/arch-support.txt |1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/features/arch-support.txt b/Documentation/features/arch-support.txt index d22a109..c0bc92b 100644 --- a/Documentation/features/arch-support.txt +++ b/Documentation/features/arch-support.txt @@ -8,4 +8,5 @@ The meaning of entries in the tables is: | ok | # feature supported by the architecture |TODO| # feature not yet supported by the architecture | .. | # feature cannot be supported by the hardware +| na | # feature is not needed by the architecture ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] kprobes: Mark OPTPROBES n/a for powerpc
On Sun, Jul 19, 2015 at 11:21:50AM +0900, Masami Hiramatsu wrote: > On 2015/07/16 19:56, Ananth N Mavinakayanahalli wrote: > > Kprobes uses a breakpoint instruction to trap into execution flow > > and the probed instruction is single-stepped from an alternate location. > > > > On some architectures like x86, under certain conditions, the OPTPROBES > > feature enables replacing the probed instruction with a jump instead, > > resulting in a significant perfomance boost (one single-step exception > > is bypassed for each kprobe). > > The OPTPROBE is not only for bypassing the single-step exception, but also > the breakpoint exception. > Please see commit 0dc016dbd820260b (ARM: kprobes: enable OPTPROBES for ARM > 32) too, > which shows how it is done on RISC processor. Yes, will fix and send. > > Powerpc has an in-kernel instruction emulator. Kprobes on powerpc uses > > this emulator already and bypasses the single-step exception, with a > > lot less complexity. > > So, this might miss the point. Since it is impossible to do on some RISC > processor, I agree with this change, but it should be committed with > correct comments. Sure, thanks! Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V2 2/2] kprobes: Mark OPTPROBES na for powerpc
Kprobes uses a breakpoint instruction to trap into execution flow and the probed instruction is single-stepped from an alternate location. On some architectures like x86, under certain conditions, the OPTPROBES feature enables replacing the probed instruction with a jump instead, resulting in a significant perfomance boost (one single-step exception is bypassed for each kprobe). Powerpc has an in-kernel instruction emulator. Kprobes on powerpc uses this emulator already and bypasses the single-step exception, with a lot less complexity. Hence, mark OPTPROBES na for powerpc. Signed-off-by: Ananth N Mavinakayanahalli --- .../features/debug/optprobes/arch-support.txt |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/features/debug/optprobes/arch-support.txt b/Documentation/features/debug/optprobes/arch-support.txt index b8999d8..73662f9 100644 --- a/Documentation/features/debug/optprobes/arch-support.txt +++ b/Documentation/features/debug/optprobes/arch-support.txt @@ -27,7 +27,7 @@ | nios2: | TODO | |openrisc: | TODO | | parisc: | TODO | -| powerpc: | TODO | +| powerpc: | na | |s390: | TODO | | score: | TODO | | sh: | TODO | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V2 1/2] Documentation/features: Add na key to arch-support.txt
To be used for features we will not support on a particular architecture. The git log that adds this needs to provide the justification 'why?' Signed-off-by: Ananth N Mavinakayanahalli --- Documentation/features/arch-support.txt |1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/features/arch-support.txt b/Documentation/features/arch-support.txt index d22a109..f2b272e 100644 --- a/Documentation/features/arch-support.txt +++ b/Documentation/features/arch-support.txt @@ -8,4 +8,5 @@ The meaning of entries in the tables is: | ok | # feature supported by the architecture |TODO| # feature not yet supported by the architecture | .. | # feature cannot be supported by the hardware +| na | # feature will not be supported by the architecture ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] kprobes: Mark OPTPROBES n/a for powerpc
Kprobes uses a breakpoint instruction to trap into execution flow and the probed instruction is single-stepped from an alternate location. On some architectures like x86, under certain conditions, the OPTPROBES feature enables replacing the probed instruction with a jump instead, resulting in a significant perfomance boost (one single-step exception is bypassed for each kprobe). Powerpc has an in-kernel instruction emulator. Kprobes on powerpc uses this emulator already and bypasses the single-step exception, with a lot less complexity. Hence, mark OPTPROBES n/a for powerpc. Signed-off-by: Ananth N Mavinakayanahalli --- .../features/debug/optprobes/arch-support.txt |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/features/debug/optprobes/arch-support.txt b/Documentation/features/debug/optprobes/arch-support.txt index b8999d8..0a3ca33 100644 --- a/Documentation/features/debug/optprobes/arch-support.txt +++ b/Documentation/features/debug/optprobes/arch-support.txt @@ -27,7 +27,7 @@ | nios2: | TODO | |openrisc: | TODO | | parisc: | TODO | -| powerpc: | TODO | +| powerpc: | n/a | |s390: | TODO | | score: | TODO | | sh: | TODO | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
On Thu, Mar 12, 2015 at 05:24:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> > Currently, perf probe considers patterns including a '.' to be a file.
> > However, this causes problems on powerpc ABIv1 where all functions have
> > a leading '.':
> >
> > $ perf probe -F | grep schedule_timeout_interruptible
> > .schedule_timeout_interruptible
> > $ perf probe .schedule_timeout_interruptible
> > Semantic error :File always requires line number or lazy pattern.
> > Error: Command Parse Error.
> >
> > Fix this by checking the probe pattern in more detail.
>
> Masami, can I have your Acked-by or Reviewed-by?
Arnaldo,
FWIW, I have reviewed this code...
Reviewed-by: Ananth N Mavinakayanahalli
>
>
> - Arnaldo
>
> > Signed-off-by: Naveen N. Rao
> > ---
> > tools/perf/util/probe-event.c | 23 ---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 28eb141..9943ff3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct
> > perf_probe_event *pev)
> > arg = tmp;
> > }
> >
> > + /*
> > +* Check arg is function or file name and copy it.
> > +*
> > +* We consider arg to be a file spec if and only if it satisfies
> > +* all of the below criteria::
> > +* - it does not include any of "+@%",
> > +* - it includes one of ":;", and
> > +* - it has a period '.' in the name.
> > +*
> > +* Otherwise, we consider arg to be a function specification.
> > +*/
> > + c = 0;
> > + if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> > + /* This is a file spec if it includes a '.' before ; or : */
> > + if (memchr(arg, '.', ptr-arg))
> > + c = 1;
> > + }
> > +
> > ptr = strpbrk(arg, ";:+@%");
> > if (ptr) {
> > nc = *ptr;
> > @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct
> > perf_probe_event *pev)
> > if (tmp == NULL)
> > return -ENOMEM;
> >
> > - /* Check arg is function or file and copy it */
> > - if (strchr(tmp, '.')) /* File */
> > + if (c == 1)
> > pp->file = tmp;
> > - else/* Function */
> > + else
> > pp->function = tmp;
> >
> > /* Parse other options */
> > --
> > 2.1.3
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH 0/8] Fix perf probe issues on powerpc
On Tue, Dec 09, 2014 at 11:03:58PM +0530, Naveen N. Rao wrote: > This patchset fixes various issues with perf probe on powerpc > across ABIv1 and ABIv2: > - in the presence of DWARF debug-info, > - in the absence of DWARF, but with the symbol table, and > - in the absence of debug-info, but with kallsyms. > > Applies cleanly on v3.18 and on -tip with minor changes to patch 6. > Tested on ppc64 BE and LE. > > - Naveen > > Naveen N. Rao (8): > kprobes: Fix kallsyms lookup across powerpc ABIv1 and ABIv2 > perf probe powerpc: Fix symbol fixup issues due to ELF type > perf probe: Improve detection of file/function name in the probe > pattern > perf probe powerpc: Handle powerpc dot symbols > perf probe powerpc: Allow matching against dot symbols > perf tools powerpc: Fix PPC64 ELF ABIv2 symbol decoding > perf probe powerpc: Use DWARF info only if necessary > perf probe powerpc: Fixup function entry if using kallsyms lookup > > arch/powerpc/include/asm/code-patching.h | 26 > arch/powerpc/include/asm/kprobes.h| 58 > ++- > tools/perf/arch/powerpc/Makefile | 1 + > tools/perf/arch/powerpc/util/elf-sym-decode.c | 27 + > tools/perf/config/Makefile| 1 + > tools/perf/util/elf_sym.h | 13 ++ > tools/perf/util/probe-event.c | 57 -- > tools/perf/util/symbol-elf.c | 11 - > tools/perf/util/symbol.c | 6 +++ > 9 files changed, 170 insertions(+), 30 deletions(-) > create mode 100644 tools/perf/arch/powerpc/util/elf-sym-decode.c > create mode 100644 tools/perf/util/elf_sym.h For the full patchset... Reviewed-by: Ananth N Mavinakayanahalli ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH 8/8] perf probe powerpc: Fixup function entry if using kallsyms lookup
On Tue, Dec 09, 2014 at 06:14:00PM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Dec 09, 2014 at 11:04:06PM +0530, Naveen N. Rao escreveu: > > On powerpc ABIv2, if no debug-info is found and we use kallsyms, > > we need to fixup the function entry to point to the local entry point. > > Use offset of 8 since current toolchains always generate 2 > > instructions (8 bytes). > > Hi Michael and Ananth, may I have your Acked-by or Reviewed-by for these > patches? > > The ones, like this, that are affects only ppc I'm can assume was tested > and applying it won't break other arch users, but having a/rev-by from > ppc developers should speed up this process. Hi Arnaldo, Yes, I have reviewed the patches. So, for all patches... Reviewed-by: Ananth N Mavinakayanahalli ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFT PATCH -next ] [BUGFIX] kprobes: Fix "Failed to find blacklist" error on ia64 and ppc64
On Thu, May 08, 2014 at 02:40:00PM +0900, Masami Hiramatsu wrote: > (2014/05/08 13:47), Ananth N Mavinakayanahalli wrote: > > On Wed, May 07, 2014 at 08:55:51PM +0900, Masami Hiramatsu wrote: > > > > ... > > > >> +#if defined(CONFIG_PPC64) && (!defined(_CALL_ELF) || _CALL_ELF == 1) > >> +/* > >> + * On PPC64 ABIv1 the function pointer actually points to the > >> + * function's descriptor. The first entry in the descriptor is the > >> + * address of the function text. > >> + */ > >> +#define constant_function_entry(fn) (((func_descr_t *)(fn))->entry) > >> +#else > >> +#define constant_function_entry(fn) ((unsigned long)(fn)) > >> +#endif > >> + > >> #endif /* __ASSEMBLY__ */ > > > > Hi Masami, > > > > You could just use ppc_function_entry() instead. > > No, I think ppc_function_entry() has two problems (on the latest -next kernel) > > At first, that is an inlined functions which is not applied in build time. > Since the NOKPROBE_SYMBOL() is used outside of any functions as like as > EXPORT_SYMBOL(), we can only use preprocessed macros. > Next, on PPC64 ABI*v2*, ppc_function_entry() returns local function entry, > which seems global function entry + 2 insns. I'm not sure about implementation > of the kallsyms on PPC64 ABIv2, but I guess we need global function entry > for kallsyms. ABIv2 does away with function descriptors and Anton fixed up that routine to handle the change (the +2 is an artefact of that). > BTW, could you test this patch on the latest -next tree on PPC64 if possible? I'll test it, but it may take a bit. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFT PATCH -next ] [BUGFIX] kprobes: Fix "Failed to find blacklist" error on ia64 and ppc64
On Wed, May 07, 2014 at 08:55:51PM +0900, Masami Hiramatsu wrote: ... > +#if defined(CONFIG_PPC64) && (!defined(_CALL_ELF) || _CALL_ELF == 1) > +/* > + * On PPC64 ABIv1 the function pointer actually points to the > + * function's descriptor. The first entry in the descriptor is the > + * address of the function text. > + */ > +#define constant_function_entry(fn) (((func_descr_t *)(fn))->entry) > +#else > +#define constant_function_entry(fn) ((unsigned long)(fn)) > +#endif > + > #endif /* __ASSEMBLY__ */ Hi Masami, You could just use ppc_function_entry() instead. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc: ftrace: bugfix for test_24bit_addr
On Wed, Feb 26, 2014 at 10:23:01AM +0800, Liu Ping Fan wrote:
> The branch target should be the func addr, not the addr of func_descr_t.
> So using ppc_function_entry() to generate the right target addr.
>
> Signed-off-by: Liu Ping Fan
> ---
> This bug will make ftrace fail to work. It can be triggered when the kernel
> size grows up.
> ---
> arch/powerpc/kernel/ftrace.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index 9b27b29..b0ded97 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -74,6 +74,7 @@ ftrace_modify_code(unsigned long ip, unsigned int old,
> unsigned int new)
> */
> static int test_24bit_addr(unsigned long ip, unsigned long addr)
> {
> + addr = ppc_function_entry((void *)addr);
Won't this break on LE?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc: OE=1 Form Instructions Not Decoded Correctly
On Mon, Sep 09, 2013 at 03:20:58PM -0500, Tom Musta wrote: > > > Isn't that code occasionally used with uprobes too nowadays ? > > > > Yes. I believe so. > > I'm going to back-pedal a little. I reread code and can connect > single step code to kprobes but not necessarily to uprobes. So > I am not sure that this code is used with uprobes. Yes, emulate_step() is used for uprobes too. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/rtas_flash: New return code to indicate FW entitlement expiry
On Tue, Apr 23, 2013 at 03:32:30PM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2013-04-23 at 10:35 +0530, Ananth N Mavinakayanahalli wrote: > > On Tue, Apr 23, 2013 at 10:40:10AM +1000, Benjamin Herrenschmidt wrote: > > > On Fri, 2013-04-19 at 17:14 +0530, Vasant Hegde wrote: > > > > Add new return code to rtas_flash to indicate firmware entitlement > > > > expiry. This will be used by the update_flash script to return > > > > appropriate message to the user. > > > > > > What's the point of that patch ? It adds a definition to a private .c > > > file not exposed to user space and doesn't do anything with it ... > > > > Ben, > > > > The userspace update_flash script invokes the rtas_flash module. With > > upcoming System p servers, the firmware will have the entitlement dates > > encoded in it and RTAS will return an error if the entitlement has > > expired. All we need from this module is for it to return that new error > > which will then be communicated to the user by the update_flash. > > That doesn't answer my question :-) > > What is the point of adding a #define to a piece of code without any user > of that definition and in a file that isn't exposed to user space ? > > IE. What is the point of the patch ? Strictly, we don't need this (kernel) update... But to keep the code in sync with PAPR, this was added. Agree that the other return codes also don't say much about what they are for. Will redo the patch with that info for better code readability. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/rtas_flash: New return code to indicate FW entitlement expiry
On Tue, Apr 23, 2013 at 10:40:10AM +1000, Benjamin Herrenschmidt wrote: > On Fri, 2013-04-19 at 17:14 +0530, Vasant Hegde wrote: > > Add new return code to rtas_flash to indicate firmware entitlement > > expiry. This will be used by the update_flash script to return > > appropriate message to the user. > > What's the point of that patch ? It adds a definition to a private .c > file not exposed to user space and doesn't do anything with it ... Ben, The userspace update_flash script invokes the rtas_flash module. With upcoming System p servers, the firmware will have the entitlement dates encoded in it and RTAS will return an error if the entitlement has expired. All we need from this module is for it to return that new error which will then be communicated to the user by the update_flash. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 4/4] uprobes/powerpc: remove additional trap instruction check
From: Ananth N Mavinakayanahalli prepare_uprobe() already checks if the underlying unstruction (on file) is a trap variant. We don't need to check this again. Signed-off-by: Ananth N Mavinakayanahalli --- arch/powerpc/kernel/uprobes.c |6 -- 1 file changed, 6 deletions(-) Index: linux-3.9-rc3/arch/powerpc/kernel/uprobes.c === --- linux-3.9-rc3.orig/arch/powerpc/kernel/uprobes.c +++ linux-3.9-rc3/arch/powerpc/kernel/uprobes.c @@ -53,12 +53,6 @@ int arch_uprobe_analyze_insn(struct arch if (addr & 0x03) return -EINVAL; - /* -* We currently don't support a uprobe on an already -* existing breakpoint instruction underneath -*/ - if (is_trap(auprobe->ainsn)) - return -ENOTSUPP; return 0; } ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 3/4] uprobes/powerpc: teach uprobes to ignore gdb breakpoints
From: Ananth N Mavinakayanahalli
Powerpc has many trap variants that could be used by entities like gdb.
Currently, running gdb on a program being traced by uprobes causes an
endless loop since uprobes doesn't understand that the trap was inserted
by some other entity and a SIGTRAP needs to be delivered.
Teach uprobes to ignore breakpoints that do not belong to it.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/kernel/uprobes.c | 10 ++
1 file changed, 10 insertions(+)
Index: linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
===
--- linux-3.9-rc3.orig/arch/powerpc/kernel/uprobes.c
+++ linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
@@ -31,6 +31,16 @@
#define UPROBE_TRAP_NR UINT_MAX
/**
+ * is_trap_insn - check if the instruction is a trap variant
+ * @insn: instruction to be checked.
+ * Returns true if @insn is a trap variant.
+ */
+bool is_trap_insn(uprobe_opcode_t *insn)
+{
+ return (is_trap(*insn));
+}
+
+/**
* arch_uprobe_analyze_insn
* @mm: the probed address space.
* @arch_uprobe: the probepoint information.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 2/4] uprobes: refuse uprobe on trap variants
From: Ananth N Mavinakayanahalli Refuse to place a uprobe if a trap variant already exists in the file copy at the address. Signed-off-by: Ananth N Mavinakayanahalli --- kernel/events/uprobes.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux-3.9-rc3/kernel/events/uprobes.c === --- linux-3.9-rc3.orig/kernel/events/uprobes.c +++ linux-3.9-rc3/kernel/events/uprobes.c @@ -573,7 +573,7 @@ static int prepare_uprobe(struct uprobe goto out; ret = -ENOTSUPP; - if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn)) + if (is_trap_insn((uprobe_opcode_t *)uprobe->arch.insn)) goto out; ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr); ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 1/4] uprobes: add trap variant helper
From: Ananth N Mavinakayanahalli
Some architectures like powerpc have multiple variants of the trap
instruction. Introduce an additional helper is_trap_insn() for run-time
handling of non-uprobe traps on such architectures.
While there, change is_swbp_at_addr() to is_trap_at_addr() for reading
clarity.
With this change, the uprobe registration path will supercede any trap
instruction inserted at the requested location, while taking care of
delivering the SIGTRAP for cases where the trap notification came in
for an address without a uprobe. See [1] for a more detailed explanation.
[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-March/104771.html
This change was suggested by Oleg Nesterov.
Signed-off-by: Ananth N Mavinakayanahalli
---
include/linux/uprobes.h |1 +
kernel/events/uprobes.c | 32
2 files changed, 29 insertions(+), 4 deletions(-)
Index: linux-3.9-rc3/include/linux/uprobes.h
===
--- linux-3.9-rc3.orig/include/linux/uprobes.h
+++ linux-3.9-rc3/include/linux/uprobes.h
@@ -100,6 +100,7 @@ struct uprobes_state {
extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm,
unsigned long vaddr);
extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm,
unsigned long vaddr);
extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
+extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
extern int uprobe_register(struct inode *inode, loff_t offset, struct
uprobe_consumer *uc);
extern int uprobe_apply(struct inode *inode, loff_t offset, struct
uprobe_consumer *uc, bool);
extern void uprobe_unregister(struct inode *inode, loff_t offset, struct
uprobe_consumer *uc);
Index: linux-3.9-rc3/kernel/events/uprobes.c
===
--- linux-3.9-rc3.orig/kernel/events/uprobes.c
+++ linux-3.9-rc3/kernel/events/uprobes.c
@@ -173,6 +173,20 @@ bool __weak is_swbp_insn(uprobe_opcode_t
return *insn == UPROBE_SWBP_INSN;
}
+/**
+ * is_trap_insn - check if instruction is breakpoint instruction.
+ * @insn: instruction to be checked.
+ * Default implementation of is_trap_insn
+ * Returns true if @insn is a breakpoint instruction.
+ *
+ * This function is needed for the case where an architecture has multiple
+ * trap instructions (like powerpc).
+ */
+bool __weak is_trap_insn(uprobe_opcode_t *insn)
+{
+ return is_swbp_insn(insn);
+}
+
static void copy_opcode(struct page *page, unsigned long vaddr,
uprobe_opcode_t *opcode)
{
void *kaddr = kmap_atomic(page);
@@ -185,6 +199,15 @@ static int verify_opcode(struct page *pa
uprobe_opcode_t old_opcode;
bool is_swbp;
+ /*
+* Note: We only check if the old_opcode is UPROBE_SWBP_INSN here.
+* We do not check if it is any other 'trap variant' which could
+* be conditional trap instruction such as the one powerpc supports.
+*
+* The logic is that we do not care if the underlying instruction
+* is a trap variant; uprobes always wins over any other (gdb)
+* breakpoint.
+*/
copy_opcode(page, vaddr, &old_opcode);
is_swbp = is_swbp_insn(&old_opcode);
@@ -204,7 +227,7 @@ static int verify_opcode(struct page *pa
* Expect the breakpoint instruction to be the smallest size instruction for
* the architecture. If an arch has variable length instruction and the
* breakpoint instruction is not of the smallest length instruction
- * supported by that architecture then we need to modify is_swbp_at_addr and
+ * supported by that architecture then we need to modify is_trap_at_addr and
* write_opcode accordingly. This would never be a problem for archs that
* have fixed length instructions.
*/
@@ -1431,7 +1454,7 @@ static void mmf_recalc_uprobes(struct mm
clear_bit(MMF_HAS_UPROBES, &mm->flags);
}
-static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)
+static int is_trap_at_addr(struct mm_struct *mm, unsigned long vaddr)
{
struct page *page;
uprobe_opcode_t opcode;
@@ -1452,7 +1475,8 @@ static int is_swbp_at_addr(struct mm_str
copy_opcode(page, vaddr, &opcode);
put_page(page);
out:
- return is_swbp_insn(&opcode);
+ /* This needs to return true for any variant of the trap insn */
+ return is_trap_insn(&opcode);
}
static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
@@ -1472,7 +1496,7 @@ static struct uprobe *find_active_uprobe
}
if (!uprobe)
- *is_swbp = is_swbp_at_addr(mm, bp_vaddr);
+ *is_swbp = is_trap_at_addr(mm, bp_vaddr);
} else {
*is_swbp = -EFAULT;
}
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3] uprobes: add trap variant helper
On Fri, Mar 22, 2013 at 03:54:06PM +0100, Oleg Nesterov wrote:
> On 03/22, Ananth N Mavinakayanahalli wrote:
> >
> > +/**
> > + * is_trap_insn - check if instruction is breakpoint instruction.
> > + * @insn: instruction to be checked.
> > + * Default implementation of is_trap_insn
> > + * Returns true if @insn is a breakpoint instruction.
> > + *
> > + * This function is needed for the case where an architecture has multiple
> > + * trap instructions (like powerpc).
> > + */
> > +bool __weak is_trap_insn(uprobe_opcode_t *insn)
> > +{
> > + return is_swbp_insn(insn);
> > +}
>
> OK, thanks, the whole series looks fine, just one note...
OK.
> My patch also changed prepare_uprobe() to use is_trap_insn(), and I think
> this is right. Exactly because of 3/3 which removes is_trap() from
> arch_uprobe_analyze_insn().
>
> If the original insn is_trap(), we do not want to singlestep it and get
> another trap after we hit handle_swbp().
OK, I'll send a v2 with that change.
Ananth
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 3/3] uprobes/powerpc: ignore trap variants during register
From: Ananth N Mavinakayanahalli The current implementation of uprobes assumes that uprobes always wins even when a register request is at a location with a conditional breakpoint by some other entity. Refer to [1] for more details. Remove the breakpoint instruction check during registration on powerpc, so that uprobes behavior on powerpc matches that of x86. [1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-March/104771.html Signed-off-by: Ananth N Mavinakayanahalli --- arch/powerpc/kernel/uprobes.c |6 -- 1 file changed, 6 deletions(-) Index: linux-3.9-rc3/arch/powerpc/kernel/uprobes.c === --- linux-3.9-rc3.orig/arch/powerpc/kernel/uprobes.c +++ linux-3.9-rc3/arch/powerpc/kernel/uprobes.c @@ -53,12 +53,6 @@ int arch_uprobe_analyze_insn(struct arch if (addr & 0x03) return -EINVAL; - /* -* We currently don't support a uprobe on an already -* existing breakpoint instruction underneath -*/ - if (is_trap(auprobe->ainsn)) - return -ENOTSUPP; return 0; } ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/3] uprobes/powerpc: teach uprobes to ignore gdb breakpoints
From: Ananth N Mavinakayanahalli
Powerpc has many trap variants that could be used by entities like gdb.
Currently, running gdb on a program being traced by uprobes causes an
endless loop since uprobes doesn't understand that the trap was inserted
by some other entity and a SIGTRAP needs to be delivered.
Teach uprobes to ignore breakpoints that do not belong to it.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/kernel/uprobes.c | 10 ++
1 file changed, 10 insertions(+)
Index: linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
===
--- linux-3.9-rc3.orig/arch/powerpc/kernel/uprobes.c
+++ linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
@@ -31,6 +31,16 @@
#define UPROBE_TRAP_NR UINT_MAX
/**
+ * is_trap_insn - check if the instruction is a trap variant
+ * @insn: instruction to be checked.
+ * Returns true if @insn is a trap variant.
+ */
+bool is_trap_insn(uprobe_opcode_t *insn)
+{
+ return (is_trap(*insn));
+}
+
+/**
* arch_uprobe_analyze_insn
* @mm: the probed address space.
* @arch_uprobe: the probepoint information.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 1/3] uprobes: add trap variant helper
From: Ananth N Mavinakayanahalli
Some architectures like powerpc have multiple variants of the trap
instruction. Introduce an additional helper is_trap_insn() for run-time
handling of non-uprobe traps on such architectures.
While there, change is_swbp_at_addr() to is_trap_at_addr() for reading
clarity.
With this change, the uprobe registration path will supercede any trap
instruction inserted at the requested location, while taking care of
delivering the SIGTRAP for cases where the trap notification came in
for an address without a uprobe. See [1] for a more detailed explanation.
[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-March/104771.html
This change was suggested by Oleg Nesterov.
Signed-off-by: Ananth N Mavinakayanahalli
---
include/linux/uprobes.h |1 +
kernel/events/uprobes.c | 32
2 files changed, 29 insertions(+), 4 deletions(-)
Index: linux-3.9-rc3/include/linux/uprobes.h
===
--- linux-3.9-rc3.orig/include/linux/uprobes.h
+++ linux-3.9-rc3/include/linux/uprobes.h
@@ -100,6 +100,7 @@ struct uprobes_state {
extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm,
unsigned long vaddr);
extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm,
unsigned long vaddr);
extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
+extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
extern int uprobe_register(struct inode *inode, loff_t offset, struct
uprobe_consumer *uc);
extern int uprobe_apply(struct inode *inode, loff_t offset, struct
uprobe_consumer *uc, bool);
extern void uprobe_unregister(struct inode *inode, loff_t offset, struct
uprobe_consumer *uc);
Index: linux-3.9-rc3/kernel/events/uprobes.c
===
--- linux-3.9-rc3.orig/kernel/events/uprobes.c
+++ linux-3.9-rc3/kernel/events/uprobes.c
@@ -173,6 +173,20 @@ bool __weak is_swbp_insn(uprobe_opcode_t
return *insn == UPROBE_SWBP_INSN;
}
+/**
+ * is_trap_insn - check if instruction is breakpoint instruction.
+ * @insn: instruction to be checked.
+ * Default implementation of is_trap_insn
+ * Returns true if @insn is a breakpoint instruction.
+ *
+ * This function is needed for the case where an architecture has multiple
+ * trap instructions (like powerpc).
+ */
+bool __weak is_trap_insn(uprobe_opcode_t *insn)
+{
+ return is_swbp_insn(insn);
+}
+
static void copy_opcode(struct page *page, unsigned long vaddr,
uprobe_opcode_t *opcode)
{
void *kaddr = kmap_atomic(page);
@@ -185,6 +199,15 @@ static int verify_opcode(struct page *pa
uprobe_opcode_t old_opcode;
bool is_swbp;
+ /*
+* Note: We only check if the old_opcode is UPROBE_SWBP_INSN here.
+* We do not check if it is any other 'trap variant' which could
+* be conditional trap instruction such as the one powerpc supports.
+*
+* The logic is that we do not care if the underlying instruction
+* is a trap variant; uprobes always wins over any other (gdb)
+* breakpoint.
+*/
copy_opcode(page, vaddr, &old_opcode);
is_swbp = is_swbp_insn(&old_opcode);
@@ -204,7 +227,7 @@ static int verify_opcode(struct page *pa
* Expect the breakpoint instruction to be the smallest size instruction for
* the architecture. If an arch has variable length instruction and the
* breakpoint instruction is not of the smallest length instruction
- * supported by that architecture then we need to modify is_swbp_at_addr and
+ * supported by that architecture then we need to modify is_trap_at_addr and
* write_opcode accordingly. This would never be a problem for archs that
* have fixed length instructions.
*/
@@ -1431,7 +1454,7 @@ static void mmf_recalc_uprobes(struct mm
clear_bit(MMF_HAS_UPROBES, &mm->flags);
}
-static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)
+static int is_trap_at_addr(struct mm_struct *mm, unsigned long vaddr)
{
struct page *page;
uprobe_opcode_t opcode;
@@ -1452,7 +1475,8 @@ static int is_swbp_at_addr(struct mm_str
copy_opcode(page, vaddr, &opcode);
put_page(page);
out:
- return is_swbp_insn(&opcode);
+ /* This needs to return true for any variant of the trap insn */
+ return is_trap_insn(&opcode);
}
static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
@@ -1472,7 +1496,7 @@ static struct uprobe *find_active_uprobe
}
if (!uprobe)
- *is_swbp = is_swbp_at_addr(mm, bp_vaddr);
+ *is_swbp = is_trap_at_addr(mm, bp_vaddr);
} else {
*is_swbp = -EFAULT;
}
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints
On Thu, Mar 21, 2013 at 04:58:09PM +0100, Oleg Nesterov wrote: > On 03/21, Ananth N Mavinakayanahalli wrote: > > > > On Wed, Mar 20, 2013 at 05:06:44PM +0100, Oleg Nesterov wrote: > > > > > > > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just > > > > > to > > > > > verify. If not, we need 2 definitions. is_uprobe_insn() should still > > > > > check > > > > > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap(). > > > > Its fine from gdb's perspective with my patch. > > Yes, but this doesn't look right from uprobe's perspective. > > > > > So, install_breakpoint()->prepare_uprobe()->is_swbp_insn() will return > > > > ENOTSUPP. In fact, arch_uprobe_analyze_insn() will also do the same. > > > > > > Yes and the check in arch_uprobe_analyze_insn() should go away. > > > > > > But you missed my point. Please forget about prepare_uprobe(), it is > > > wrong anyway. And, prepare_uprobe() inspects the _original_ insn from > > > the file, this has nothing install_breakpoint/etc. > > > > > > I meant verify_opcode() called by install_breakpoint/etc. > > > > For the case where X already exists, verify_opcode() currently returns 0. > > IMO, it should return -EEXIST, > > Oh, this is debatable. Currently we assume that uprobe should "win". And there is that one case where gdb also uses an unconditional trap... but that's besides the point if we don't care about gdb. > See below... > > > unless you are proposing that uprobes > > should ride on the existing trap (even if its a variant). > > Yes. And this is what the current implementation does. > > > If you are proposing that uprobes ride on X if it already exists, that's > > not always possible and is a big can of worms... see below... > > Sure. Whatever we do uprobe and gdb can confuse each other. Unless we > rework the vm code completely (not sure this is realistic). Agreed. > > > OK. So, if I understand correctly, gdb can use some conditional > > > breakpoint, and it is possible that this insn won't generate the > > > trap? > > > > Yes it is possible if the condition is not met. If the condition is > > met, the instruction will generate a trap, and uprobes will do a > > send_sig(SIGTRAP) from handle_swbp(). > > Unless there is uprobe at the same address. Once again, uprobe wins. In which case, we will need to replace the conditional trap with the unconditional one when the uprobe register happens. That is doable. > Your patch only fixes the case when the task hits a non-UPROBE_SWBP_INSN > breakpoint and there is no uprobe at the same address. Correct. > > > Then this patch is not right, or at least we need another change > > > on top? > > > > > > Once again. Suppose that gdb installs the TRAP_IF_R1_GT_R2. > > > > > > After that uprobe_register() is called, but it won't change this > > > insn because verify_opcode() returns 0. > > > > > > Then the probed task hits this breakoint with "r1 < r2" and we do > > > not report this event. > > > > At this time, the condition for the trap is not satisfied, so no > > exception occurs. > > Yes, and thus uprobe->handler() is not called, this was my point. > > > If the expectation is that the trap always trigger, > > then all such trap variants need to be replaced with the unconditional > > trap > > Yes. that is why I suggested the patch which doesn't affect verify_opcode(). > uprobe_register() should replace the conditional trap with the unconditional > UPROBE_SWBP_INSN. uprobes should win. OK, I see your point. ... > > But, unlike x86, we cannot > > expect a uprobe with an underlying trap variant (X) to always trigger. > > And that is why I think write_opcode() should rewrite the conditional > trap. OK > > IMHO, its not a good idea to do that for x86 either, > > This change has no effect fo x86. > > > IMHO, I really think we should not allow uprobe_register() to succeed if > > the underlying instruction is a breakpoint (or a variant thereof). > > I disagree. > > Just for example. Suppose we change install_breakpoint() so that it fails > if the underlying instruction is int3. (once again, "underlying" does not > mean the original insn from vma->vm_file). > > First of all, this is very much nontrivial. I simply do not see how we > can do this. If nothing else, uprobe_register() can race with uprobe_mmap() > and
Re: [PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints
On Thu, Mar 21, 2013 at 05:00:02PM +0100, Oleg Nesterov wrote: > On 03/21, Ananth N Mavinakayanahalli wrote: > ? > > On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote: > > > On 03/20, Ananth N Mavinakayanahalli wrote: > > > > > > > > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote: > > > > > On 03/20, Oleg Nesterov wrote: > > > > > > > > > > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment. > > > > > Suppose we apply the patch below. Will uprobes on powerpc work? > > > > > > > > > > If yes, then your patch should be fine. If not, we probably need more > > > > > changes. > > > > > > > > Yes, it will work fine. > > > > > > Even if this new insn is conditional? > > > > Yes. > > But this can't be true. If it is conditional, it won't always generate a > trap, this means uprobe won't actually work. I meant to say, uprobes if we use a conditional trap instruction as long as the condition is met. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints
On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote: > On 03/20, Ananth N Mavinakayanahalli wrote: > > > > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote: > > > On 03/20, Oleg Nesterov wrote: > > > > > > > > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to > > > > verify. If not, we need 2 definitions. is_uprobe_insn() should still > > > > check > > > > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap(). > > > > > > > > And I am just curious, could you explain how X and UPROBE_SWBP_INSN > > > > differ? > > > > > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment. > > > Suppose we apply the patch below. Will uprobes on powerpc work? > > > > > > If yes, then your patch should be fine. If not, we probably need more > > > changes. > > > > Yes, it will work fine. > > Even if this new insn is conditional? Yes. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints
On Wed, Mar 20, 2013 at 05:06:44PM +0100, Oleg Nesterov wrote: > On 03/20, Ananth N Mavinakayanahalli wrote: > > > > On Wed, Mar 20, 2013 at 01:26:39PM +0100, Oleg Nesterov wrote: > > > > > But, at the same time, is the new definition fine for verify_opcode()? > > > > > > IOW, powerpc has another is_trap() insn(s) used by gdb, lets denote it X. > > > X != UPROBE_SWBP_INSN. > > > > > > Suppose that gdb installs the trap X at some addr, and then > > > uprobe_register() > > > tries to install uprobe at the same address. Then set_swbp() will do > > > nothing, > > > assuming the uprobe was already installed. I think that is not right... see below... > > > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to > > > verify. If not, we need 2 definitions. is_uprobe_insn() should still check > > > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap(). Its fine from gdb's perspective with my patch. > > is_trap() checks for all trap variants on powerpc, including the one > > uprobe uses. It returns true if the instruction is *any* trap variant. > > I understand, > > > So, install_breakpoint()->prepare_uprobe()->is_swbp_insn() will return > > ENOTSUPP. In fact, arch_uprobe_analyze_insn() will also do the same. > > Yes and the check in arch_uprobe_analyze_insn() should go away. > > But you missed my point. Please forget about prepare_uprobe(), it is > wrong anyway. And, prepare_uprobe() inspects the _original_ insn from > the file, this has nothing install_breakpoint/etc. > > I meant verify_opcode() called by install_breakpoint/etc. For the case where X already exists, verify_opcode() currently returns 0. IMO, it should return -EEXIST, unless you are proposing that uprobes should ride on the existing trap (even if its a variant). If you are proposing that uprobes ride on X if it already exists, that's not always possible and is a big can of worms... see below... > > This itself should take care of all the cases. > > > > > And I am just curious, could you explain how X and UPROBE_SWBP_INSN > > > differ? > > > > Powerpc has numerous variants of the trap instruction based on > > comparison of two registers or a regsiter and immediate value and a > > condition > > (less than, greater than, [signed forms thereof], or equal to). > > > > Uprobes uses 0x7fe0008 which is 'tw 31,0,0' which essentially is an > > unconditional trap. > > > > Gdb uses many traps, one of which is 0x7d821008 which is twge r2,r2, > > which is basically trap if r2 greater than or equal to r2. > > OK. So, if I understand correctly, gdb can use some conditional > breakpoint, and it is possible that this insn won't generate the > trap? Yes it is possible if the condition is not met. If the condition is met, the instruction will generate a trap, and uprobes will do a send_sig(SIGTRAP) from handle_swbp(). > Then this patch is not right, or at least we need another change > on top? > > Once again. Suppose that gdb installs the TRAP_IF_R1_GT_R2. > > After that uprobe_register() is called, but it won't change this > insn because verify_opcode() returns 0. > > Then the probed task hits this breakoint with "r1 < r2" and we do > not report this event. At this time, the condition for the trap is not satisfied, so no exception occurs. If the expectation is that the trap always trigger, then all such trap variants need to be replaced with the unconditional trap and we should either add logic to re-execute the condional trap after uprobe handling and send_sig() via handle_swbp() or emulate the condition in software and do a send_sig() if needed. > So. I still think that we actually need something like below, and > powerpc should reimplement is_trap_insn() to use is_trap(insn). > > No? I don't see how this will help, especially since the gdb<->uprobes is fraught with races. With your proposed patch, we refuse to insert a uprobe if the underlying instruction is a UPROBE_SWBP_INSTRUCTION; changing is_swbp_at_addr() will need changes in handle_swbp() too. But, unlike x86, we cannot expect a uprobe with an underlying trap variant (X) to always trigger. IMHO, its not a good idea to do that for x86 either, since you'll run into many other complications (what if the entity that put the original breakpoint, removed it, etc). IMHO, I really think we should not allow uprobe_register() to succeed if the underlying instruction is a breakpoint (or a variant thereof). Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints
On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote: > On 03/20, Oleg Nesterov wrote: > > > > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to > > verify. If not, we need 2 definitions. is_uprobe_insn() should still check > > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap(). > > > > And I am just curious, could you explain how X and UPROBE_SWBP_INSN > > differ? > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment. > Suppose we apply the patch below. Will uprobes on powerpc work? > > If yes, then your patch should be fine. If not, we probably need more > changes. Yes, it will work fine. > And, forgot to mention. If you change is_swbp_insn(), you can remove > is_trap() from arch_uprobe_analyze_insn(). Yeah, that check is no longer needed. I'll send a separate cleanup after this patch gets applied. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints
On Wed, Mar 20, 2013 at 01:26:39PM +0100, Oleg Nesterov wrote:
> Hi Ananth,
>
> First of all, let me remind that I know nothing about powerpc ;)
>
> But iirc we already discussed this a bit, I forgot the details but
> still I have some concerns...
>
> On 03/20, Ananth N Mavinakayanahalli wrote:
> >
> > GDB uses a variant of the trap instruction that is different from the
> > one used by uprobes. Currently, running gdb on a program being traced
> > by uprobes causes an endless loop since uprobes doesn't understand
> > that the trap is inserted by some other entity and hence a SIGTRAP needs
> > to be delivered.
>
> Yes, and thus is_swbp_at_addr()->is_swbp_insn() called by handle_swbp()
> should be updated,
>
> > +bool is_swbp_insn(uprobe_opcode_t *insn)
> > +{
> > + return (is_trap(*insn));
> > +}
>
> And this patch should fix the problem. (and probably this is fine
> for prepare_uprobe()).
Correct
> But, at the same time, is the new definition fine for verify_opcode()?
>
> IOW, powerpc has another is_trap() insn(s) used by gdb, lets denote it X.
> X != UPROBE_SWBP_INSN.
>
> Suppose that gdb installs the trap X at some addr, and then uprobe_register()
> tries to install uprobe at the same address. Then set_swbp() will do nothing,
> assuming the uprobe was already installed.
>
> But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to
> verify. If not, we need 2 definitions. is_uprobe_insn() should still check
> insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap().
is_trap() checks for all trap variants on powerpc, including the one
uprobe uses. It returns true if the instruction is *any* trap variant.
So, install_breakpoint()->prepare_uprobe()->is_swbp_insn() will return
ENOTSUPP. In fact, arch_uprobe_analyze_insn() will also do the same.
This itself should take care of all the cases.
> And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> differ?
Powerpc has numerous variants of the trap instruction based on
comparison of two registers or a regsiter and immediate value and a condition
(less than, greater than, [signed forms thereof], or equal to).
Uprobes uses 0x7fe0008 which is 'tw 31,0,0' which essentially is an
unconditional trap.
Gdb uses many traps, one of which is 0x7d821008 which is twge r2,r2,
which is basically trap if r2 greater than or equal to r2.
Ananth
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints
From: Ananth N Mavinakayanahalli
GDB uses a variant of the trap instruction that is different from the
one used by uprobes. Currently, running gdb on a program being traced
by uprobes causes an endless loop since uprobes doesn't understand
that the trap is inserted by some other entity and hence a SIGTRAP needs
to be delivered.
Teach uprobes to ignore breakpoints that doesn't belong to it.
Signed-off-by: Ananth N Mavinakayanahalli
Cc: sta...@vger.kernel.org
---
arch/powerpc/kernel/uprobes.c | 10 ++
1 file changed, 10 insertions(+)
Index: linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
===
--- linux-3.9-rc3.orig/arch/powerpc/kernel/uprobes.c
+++ linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
@@ -80,6 +80,16 @@ unsigned long uprobe_get_swbp_addr(struc
return instruction_pointer(regs);
}
+/**
+ * is_swbp_insn - check if the instruction is a breakpoint instruction.
+ * @insn: instruction to be checked.
+ * Returns true if @insn is a breakpoint instruction.
+ */
+bool is_swbp_insn(uprobe_opcode_t *insn)
+{
+ return (is_trap(*insn));
+}
+
/*
* If xol insn itself traps and generates a signal (SIGILL/SIGSEGV/etc),
* then detect the case where a singlestepped instruction jumps back to its
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/4] powerpc: Move the single step enable code to a generic path
On Mon, Dec 03, 2012 at 08:38:37PM +0530, Suzuki K. Poulose wrote: > From: Suzuki K. Poulose > > This patch moves the single step enable code used by kprobe to a generic > routine header so that, it can be re-used by other code, in this case, > uprobes. No functional changes. > > Signed-off-by: Suzuki K. Poulose > Cc: Ananth N Mavinakaynahalli > Cc: Kumar Gala > Cc: linuxppc-...@ozlabs.org Acked-by: Ananth N Mavinakayanahalli ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: build failure after merge of the final tree (powerpc tree related)
On Thu, Sep 06, 2012 at 05:11:53PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the final tree, today's linux-next build (powerpc allyesconfig)
> failed like this:
>
> In file included from drivers/atm/fore200e.c:70:0:
> drivers/atm/fore200e.h:263:3: error: redefinition of typedef 'opcode_t' with
> different type
> arch/powerpc/include/asm/probes.h:25:13: note: previous declaration of
> 'opcode_t' was here
>
> Caused by commit 7118e7e648e0 ("powerpc: Consolidate {k,u}probe
> definitions") from the powerpc tree.
>
> I have reverted that commit (and the two following:
> 41ab5266c362 "powerpc: Add trap_nr to thread_struct"
> 8b7b80b9ebb4 "powerpc: Uprobes port to powerpc")
> for today.
Hi Stephen,
I have just posted a patch [1] to fix the issue.
Ananth
[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-September/100813.html
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] Rename opcode_t in probes.h to ppc_opcode_t
On Thu, Sep 06, 2012 at 12:56:12PM +1000, Benjamin Herrenschmidt wrote: > On Thu, 2012-09-06 at 10:19 +0800, Fengguang Wu wrote: > > Hi Ananth, > > > > FYI, kernel build failed on > > > > tree: git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git next > > head: 8b64a9dfb091f1eca8b7e58da82f1e7d1d5fe0ad > > commit: 8b7b80b9ebb46dd88fbb94e918297295cf312b59 [24/29] powerpc: Uprobes > > port to powerpc > > config: powerpc-allmodconfig (attached as .config) > > > > All related error/warning messages: > > > > In file included from drivers/atm/fore200e.c:70:0: > > drivers/atm/fore200e.h:263:3: error: redefinition of typedef 'opcode_t' > > with different type > > arch/powerpc/include/asm/probes.h:25:13: note: previous declaration of > > 'opcode_t' was here > > This is a bit more annoying. Ananth, do we need that to be called > opcode_t for generic reasons or can we make it ppc_opcode_t ? If it has > to remain, I suppose we can try to change that ATM driver to use a > different type name... We can make it ppc_opcode_t. Attached is the patch that fixes this. Regards, Ananth --- From: Ananth N Mavinakayanahalli tree: git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git next head: 8b64a9dfb091f1eca8b7e58da82f1e7d1d5fe0ad commit: 8b7b80b9ebb46dd88fbb94e918297295cf312b59 [24/29] powerpc: Uprobes port to powerpc config: powerpc-allmodconfig (attached as .config) All related error/warning messages: In file included from drivers/atm/fore200e.c:70:0: drivers/atm/fore200e.h:263:3: error: redefinition of typedef 'opcode_t' with different type arch/powerpc/include/asm/probes.h:25:13: note: previous declaration of 'opcode_t' was here Fix the namespace clash by making opcode_t in probes.h to ppc_opcode_t. Signed-off-by: Ananth N Mavinakayanahalli --- Index: linux-tip-22aug/arch/powerpc/include/asm/probes.h === --- linux-tip-22aug.orig/arch/powerpc/include/asm/probes.h 2012-09-04 20:00:19.747069793 +0530 +++ linux-tip-22aug/arch/powerpc/include/asm/probes.h 2012-09-04 20:42:08.147286718 +0530 @@ -22,7 +22,7 @@ */ #include -typedef u32 opcode_t; +typedef u32 ppc_opcode_t; #define BREAKPOINT_INSTRUCTION 0x7fe8 /* trap */ /* Trap definitions per ISA */ Index: linux-tip-22aug/arch/powerpc/include/asm/uprobes.h === --- linux-tip-22aug.orig/arch/powerpc/include/asm/uprobes.h 2012-09-04 20:01:30.617071747 +0530 +++ linux-tip-22aug/arch/powerpc/include/asm/uprobes.h 2012-09-04 20:42:26.657287349 +0530 @@ -25,7 +25,7 @@ #include #include -typedef opcode_t uprobe_opcode_t; +typedef ppc_opcode_t uprobe_opcode_t; #define MAX_UINSN_BYTES4 #define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES) Index: linux-tip-22aug/arch/powerpc/include/asm/kprobes.h === --- linux-tip-22aug.orig/arch/powerpc/include/asm/kprobes.h 2012-09-04 20:00:19.747069793 +0530 +++ linux-tip-22aug/arch/powerpc/include/asm/kprobes.h 2012-09-04 20:42:15.557286955 +0530 @@ -36,7 +36,7 @@ struct pt_regs; struct kprobe; -typedef opcode_t kprobe_opcode_t; +typedef ppc_opcode_t kprobe_opcode_t; #define MAX_INSN_SIZE 1 #ifdef CONFIG_PPC64 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 3/3] powerpc: Uprobes port to powerpc
On Wed, Sep 05, 2012 at 03:26:59PM +1000, Benjamin Herrenschmidt wrote: > On Fri, 2012-08-24 at 13:01 +0530, Ananth N Mavinakayanahalli wrote: > > From: Ananth N Mavinakayanahalli > > > > This is the port of uprobes to powerpc. Usage is similar to x86. > > Guys, can you do a minimum of build testing ? > > This one breaks due to uprobe_get_swbp_addr() being defined in both > asm and include/linux when CONFIG_UPROBE isn't set. You don't need > to define it at all in fact, the generic code takes care of both the > declaration for CONFIG_UPROBE and the empty inline for !CONFIG_UPROBE. > > I'm fixing that one up myself but please, please, get yourself a test > build script or something to make sure you don't at least break the > build when the stuff you're adding isn't enabled (among others). Sorry Ben. That was an oversight. Won't happen again. Regards, Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 2/2] powerpc: Uprobes port to powerpc
On Fri, Aug 24, 2012 at 05:07:31PM +1000, Benjamin Herrenschmidt wrote: > On Fri, 2012-08-24 at 11:13 +1000, Michael Ellerman wrote: > > > > Yeah. A NULL regs here is a kernel bug, so I think it's actually > > preferable to crash than silently return. > > Or best, if you think there's a remote chance that the bug might hit: > > if (WARN(!regs)) > return Incorporated this and other suggestions from Michael in v5 I just posted. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v5 3/3] powerpc: Uprobes port to powerpc
From: Ananth N Mavinakayanahalli
This is the port of uprobes to powerpc. Usage is similar to x86.
[root@ ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
Added new event:
probe_libc:malloc(on 0xb4860)
You can now use it in all perf tools, such as:
perf record -e probe_libc:malloc -aR sleep 1
[root@ ~]# ./bin/perf record -e probe_libc:malloc -aR sleep 20
[ perf record: Woken up 22 times to write data ]
[ perf record: Captured and wrote 5.843 MB perf.data (~255302 samples) ]
[root@ ~]# ./bin/perf report --stdio
...
# Samples: 83K of event 'probe_libc:malloc'
# Event count (approx.): 83484
#
# Overhead Command Shared Object Symbol
# . ..
#
69.05% tar libc-2.12.so [.] malloc
28.57%rm libc-2.12.so [.] malloc
1.32% avahi-daemon libc-2.12.so [.] malloc
0.58% bash libc-2.12.so [.] malloc
0.28% sshd libc-2.12.so [.] malloc
0.08%irqbalance libc-2.12.so [.] malloc
0.05% bzip2 libc-2.12.so [.] malloc
0.04% sleep libc-2.12.so [.] malloc
0.03%multipathd libc-2.12.so [.] malloc
0.01% sendmail libc-2.12.so [.] malloc
0.01% automount libc-2.12.so [.] malloc
The trap_nr addition patch is a prereq.
Signed-off-by: Ananth N Mavinakayanahalli
---
Tested on POWER6; I don't see anything here that should stop it from
working on a ppc32; since I don't have access to a ppc32 machine, it
would be good if somoene could verify that part.
TODO:
Testsuite for uprobes (Not powerpc specific).
V5:
a. Incorporated Oleg's suggestion to make arch_uprobe a union for
easier deref of u32 on powerpc.
b. Incorporated various review comments by Michael Ellerman and
Benjamin Herrenschmidt.
V4:
Enhance arch_analyze_uprobe_insn() to reject uprobes on trap variants.
V3:
Added abort_xol() logic for powerpc, using thread_struct.trap_nr to
determine if the stepped instruction caused an exception.
V2:
a. arch_uprobe_analyze_insn() now gets unsigned long addr.
b. Verified that mtmsr[d] and rfi[d] are handled correctly by
emulate_step() (no changes to this patch).
arch/powerpc/Kconfig |3
arch/powerpc/include/asm/thread_info.h |4
arch/powerpc/include/asm/uprobes.h | 55 +
arch/powerpc/kernel/Makefile |1
arch/powerpc/kernel/signal.c |6 +
arch/powerpc/kernel/uprobes.c | 184 +
6 files changed, 252 insertions(+), 1 deletion(-)
Index: linux-tip-23aug/arch/powerpc/include/asm/thread_info.h
===
--- linux-tip-23aug.orig/arch/powerpc/include/asm/thread_info.h
+++ linux-tip-23aug/arch/powerpc/include/asm/thread_info.h
@@ -102,6 +102,7 @@ static inline struct thread_info *curren
#define TIF_RESTOREALL 11 /* Restore all regs (implies NOERROR) */
#define TIF_NOERROR12 /* Force successful syscall return */
#define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
+#define TIF_UPROBE 14 /* breakpointed or single-stepping */
#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
/* as above, but as bit values */
@@ -118,12 +119,13 @@ static inline struct thread_info *curren
#define _TIF_RESTOREALL(1<
+ */
+
+#include
+#include
+
+typedef opcode_t uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES4
+#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+
+/* The following alias is needed for reference from arch-agnostic code */
+#define UPROBE_SWBP_INSN BREAKPOINT_INSTRUCTION
+#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
+
+struct arch_uprobe {
+ union {
+ u8 insn[MAX_UINSN_BYTES];
+ u32 ainsn;
+ };
+};
+
+struct arch_uprobe_task {
+ unsigned long saved_trap_nr;
+};
+
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm, unsigned long addr);
+extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
+extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned
long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+#endif /* _ASM_UPROBES_H */
Index: linux-tip-23aug/arch/powerpc/kernel/Makefile
===
--- linux-tip-23aug.orig/arch/powerpc/kernel/Makefile
+++ linux-tip-23aug/arch/powerpc/kernel/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_MODULES) += ppc_ksyms.o
obj-$(CONFIG_BOOTX_TEXT)
[PATCH 2/3] powerpc: Add trap_nr to thread_struct
From: Ananth N Mavinakayanahalli
Add thread_struct.trap_nr and use it to store the last exception
the thread experienced. In this patch, we populate the field at
various places where we force_sig_info() to the process.
This is also used in uprobes to determine if the probed instruction
caused an exception.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/include/asm/processor.h |1 +
arch/powerpc/kernel/process.c|2 ++
arch/powerpc/kernel/traps.c |1 +
arch/powerpc/mm/fault.c |1 +
4 files changed, 5 insertions(+)
Index: linux-26jul/arch/powerpc/include/asm/processor.h
===
--- linux-26jul.orig/arch/powerpc/include/asm/processor.h
+++ linux-26jul/arch/powerpc/include/asm/processor.h
@@ -219,6 +219,7 @@ struct thread_struct {
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#endif
unsigned long dabr; /* Data address breakpoint register */
+ unsigned long trap_nr;/* last trap # on this thread */
#ifdef CONFIG_ALTIVEC
/* Complete AltiVec register set */
vector128 vr[32] __attribute__((aligned(16)));
Index: linux-26jul/arch/powerpc/kernel/process.c
===
--- linux-26jul.orig/arch/powerpc/kernel/process.c
+++ linux-26jul/arch/powerpc/kernel/process.c
@@ -258,6 +258,7 @@ void do_send_trap(struct pt_regs *regs,
{
siginfo_t info;
+ current->thread.trap_nr = signal_code;
if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
11, SIGSEGV) == NOTIFY_STOP)
return;
@@ -275,6 +276,7 @@ void do_dabr(struct pt_regs *regs, unsig
{
siginfo_t info;
+ current->thread.trap_nr = TRAP_HWBKPT;
if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
11, SIGSEGV) == NOTIFY_STOP)
return;
Index: linux-26jul/arch/powerpc/kernel/traps.c
===
--- linux-26jul.orig/arch/powerpc/kernel/traps.c
+++ linux-26jul/arch/powerpc/kernel/traps.c
@@ -251,6 +251,7 @@ void _exception(int signr, struct pt_reg
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
+ current->thread.trap_nr = code;
memset(&info, 0, sizeof(info));
info.si_signo = signr;
info.si_code = code;
Index: linux-26jul/arch/powerpc/mm/fault.c
===
--- linux-26jul.orig/arch/powerpc/mm/fault.c
+++ linux-26jul/arch/powerpc/mm/fault.c
@@ -133,6 +133,7 @@ static int do_sigbus(struct pt_regs *reg
up_read(¤t->mm->mmap_sem);
if (user_mode(regs)) {
+ current->thread.trap_nr = BUS_ADRERR;
info.si_signo = SIGBUS;
info.si_errno = 0;
info.si_code = BUS_ADRERR;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 1/3] powerpc: Consolidate *probe definitions
From: Ananth N Mavinakayanahalli
Move is_trap() and relatives to a common file to be shared between *probes.
Code movement only; no change in functionality.
Suggested by Michael Ellerman.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/include/asm/kprobes.h | 15 +
arch/powerpc/include/asm/probes.h | 42 +
2 files changed, 44 insertions(+), 13 deletions(-)
Index: linux-tip-23aug/arch/powerpc/include/asm/kprobes.h
===
--- linux-tip-23aug.orig/arch/powerpc/include/asm/kprobes.h
+++ linux-tip-23aug/arch/powerpc/include/asm/kprobes.h
@@ -29,21 +29,16 @@
#include
#include
#include
+#include
#define __ARCH_WANT_KPROBES_INSN_SLOT
struct pt_regs;
struct kprobe;
-typedef unsigned int kprobe_opcode_t;
-#define BREAKPOINT_INSTRUCTION 0x7fe8 /* trap */
+typedef opcode_t kprobe_opcode_t;
#define MAX_INSN_SIZE 1
-#define IS_TW(instr) (((instr) & 0xfc0007fe) == 0x7c08)
-#define IS_TD(instr) (((instr) & 0xfc0007fe) == 0x7c88)
-#define IS_TDI(instr) (((instr) & 0xfc00) == 0x0800)
-#define IS_TWI(instr) (((instr) & 0xfc00) == 0x0c00)
-
#ifdef CONFIG_PPC64
/*
* 64bit powerpc uses function descriptors.
@@ -72,12 +67,6 @@ typedef unsigned int kprobe_opcode_t;
addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name); \
} \
}
-
-#define is_trap(instr) (IS_TW(instr) || IS_TD(instr) || \
- IS_TWI(instr) || IS_TDI(instr))
-#else
-/* Use stock kprobe_lookup_name since ppc32 doesn't use function descriptors */
-#define is_trap(instr) (IS_TW(instr) || IS_TWI(instr))
#endif
#define flush_insn_slot(p) do { } while (0)
Index: linux-tip-23aug/arch/powerpc/include/asm/probes.h
===
--- /dev/null
+++ linux-tip-23aug/arch/powerpc/include/asm/probes.h
@@ -0,0 +1,42 @@
+#ifndef _ASM_POWERPC_PROBES_H
+#define _ASM_POWERPC_PROBES_H
+#ifdef __KERNEL__
+/*
+ * Definitions common to probes files
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright IBM Corporation, 2012
+ */
+#include
+
+typedef u32 opcode_t;
+#define BREAKPOINT_INSTRUCTION 0x7fe8 /* trap */
+
+/* Trap definitions per ISA */
+#define IS_TW(instr) (((instr) & 0xfc0007fe) == 0x7c08)
+#define IS_TD(instr) (((instr) & 0xfc0007fe) == 0x7c88)
+#define IS_TDI(instr) (((instr) & 0xfc00) == 0x0800)
+#define IS_TWI(instr) (((instr) & 0xfc00) == 0x0c00)
+
+#ifdef CONFIG_PPC64
+#define is_trap(instr) (IS_TW(instr) || IS_TD(instr) || \
+ IS_TWI(instr) || IS_TDI(instr))
+#else
+#define is_trap(instr) (IS_TW(instr) || IS_TWI(instr))
+#endif /* CONFIG_PPC64 */
+
+#endif /* __KERNEL__ */
+#endif /* _ASM_POWERPC_PROBES_H */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 2/2] powerpc: Uprobes port to powerpc
On Thu, Aug 23, 2012 at 02:28:20PM +1000, Michael Ellerman wrote:
> On Wed, 2012-08-22 at 13:57 +0530, Ananth N Mavinakayanahalli wrote:
> > From: Ananth N Mavinakayanahalli
> >
> > This is the port of uprobes to powerpc. Usage is similar to x86.
>
> Hi Ananth,
>
> Excuse my ignorance of uprobes, some comments inline ...
Thanks for the review Michael!
> > [root@ ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
> > Added new event:
> > probe_libc:malloc(on 0xb4860)
> >
> > You can now use it in all perf tools, such as:
> >
> > perf record -e probe_libc:malloc -aR sleep 1
>
> Is there a test suite for any of this?
We don't have a formal testsuite yet, but the usual way of testing it is
to run kernbench while registering/unregistering a bunch of probes
periodically.
...
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
> > USA.
> > + *
> > + * Copyright (C) IBM Corporation, 2007-2012
>
> The lawyers say we shouldn't use (C).
Will remove that.
> Is it really copyright IBM 2007-2012? Or is that because you copied
> another header?
The later. This is adapted from the x86 version.
> > +typedef unsigned int uprobe_opcode_t;
>
> I'd prefer u32.
OK. Will change.
> It would be nice if someone could consolidate this with kprobe_opcode_t.
Thats on the TODO after the uprobes code stabilizes further.
I am wondering which file would be appropriate? We could either
consolidate a bunch of these into asm/kdebug.h or asm/ptrace.h. Any
preference/suggestion?
> > +#define MAX_UINSN_BYTES4
> > +#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
> > +
> > +#define UPROBE_SWBP_INSN 0x7fe8
>
> This is just "trap" ?
Yes. But since its referred to in arch agnostic code too, we'd have to alias
it thus.
> > +#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
> > +
> > +#define IS_TW(instr) (((instr) & 0xfc0007fe) == 0x7c08)
> > +#define IS_TD(instr) (((instr) & 0xfc0007fe) == 0x7c88)
> > +#define IS_TDI(instr) (((instr) & 0xfc00) == 0x0800)
> > +#define IS_TWI(instr) (((instr) & 0xfc00) == 0x0c00)
> > +
> > +#define is_trap(instr) (IS_TW(instr) || IS_TD(instr) || \
> > + IS_TWI(instr) || IS_TDI(instr))
>
> These seem to be duplicated in kprobes.h, can we consolidate them.
Yes, similar to the opcode_t types above.
> > +struct arch_uprobe {
> > + u8 insn[MAX_UINSN_BYTES];
> > +};
>
> Why not uprobe_opcode_t insn ?
I had a similar discussion with Srikar while doing the port, but he has
reasons for this...
...
> > void do_notify_resume(struct pt_regs *regs, unsigned long
> > thread_info_flags)
> > {
> > + if (thread_info_flags & _TIF_UPROBE) {
> > + clear_thread_flag(TIF_UPROBE);
> > + uprobe_notify_resume(regs);
> > + }
>
> Presumably this ordering is crucial, ie. uprobes before signals.
Correct!
...
> > +#define UPROBE_TRAP_NR UINT_MAX
>
> In the comments below you talk about -1 a few times, but you actually
> mean UINT_MAX.
Correct. I will fix those references.
> > +/**
> > + * arch_uprobe_analyze_insn
>
> Analyze what about the instruction?
Depends on the architecture. On x86, we need to verify if the address is
at an instruction boundary, and if the instruction can be probed at all.
On powerpc, we have an easier time. We just validate the address is
aligned at instruction boundary and flag if the instruction at the
address is a trap variant.
> > + * @mm: the probed address space.
> > + * @arch_uprobe: the probepoint information.
> > + * @addr: vaddr to probe.
> > + * Return 0 on success or a -ve number on error.
> > + */
> > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct
> > *mm, unsigned long addr)
> > +{
> > + unsigned int insn;
> > +
> > + if (addr & 0x03)
> > + return -EINVAL;
> > +
> > + memcpy(&insn, auprobe->insn, MAX_UINSN_BYTES);
>
> We shouldn't need to use memcpy, we know it's a u32.
OK. Right now, its u8 insn[4], so I did this to be 'correct'. But I
agree we can just do an assignment.
> > + if (is_trap(insn))
> > + return -ENOTSUPP;
>
> A comment saying why we can't handle this would be nice.
Will add.
>
Re: [PATCH v3 2/2] powerpc: Uprobes port to powerpc
On Tue, Aug 21, 2012 at 03:09:30PM +0200, Oleg Nesterov wrote: ... > > This is true for Intel like architectures that have *one* swbp > > instruction. On Powerpc, gdb for instance, can insert a trap variant at > > the address. Therefore, is_swbp_insn() by definition should return true > > for all trap variants. > > Not in this case, I think. > > OK, I was going to do this later, but this discussion makes me think > I should try to send the patch sooner. > > set_swbp()->is_swbp_at_addr() is simply unneeded and in fact should > be considered as unnecessary pessimization. > > set_orig_insn()->is_swbp_at_addr() makes more sense, but it can't fix > all races with userpace. Still it should die. > > > OK. I will separate out the is_swbp_insn() change into a separate patch. > > Great thanks. And if we remove is_swbp_insn() from set_swbp() and > set_orig_insn() then the semantics of is_swbp_insn() will much more > clear, and in this case I powerpc probably really needs to change it. Oleg, I have posted a new version for review [1] without the is_swbp_insn() change. I will await your changes around is_swbp_at_addr() and make changes to the powerpc code if necessary. Regards, Ananth [1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-August/100524.html ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v4 2/2] powerpc: Uprobes port to powerpc
From: Ananth N Mavinakayanahalli
This is the port of uprobes to powerpc. Usage is similar to x86.
[root@ ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
Added new event:
probe_libc:malloc(on 0xb4860)
You can now use it in all perf tools, such as:
perf record -e probe_libc:malloc -aR sleep 1
[root@ ~]# ./bin/perf record -e probe_libc:malloc -aR sleep 20
[ perf record: Woken up 22 times to write data ]
[ perf record: Captured and wrote 5.843 MB perf.data (~255302 samples) ]
[root@ ~]# ./bin/perf report --stdio
...
# Samples: 83K of event 'probe_libc:malloc'
# Event count (approx.): 83484
#
# Overhead Command Shared Object Symbol
# . ..
#
69.05% tar libc-2.12.so [.] malloc
28.57%rm libc-2.12.so [.] malloc
1.32% avahi-daemon libc-2.12.so [.] malloc
0.58% bash libc-2.12.so [.] malloc
0.28% sshd libc-2.12.so [.] malloc
0.08%irqbalance libc-2.12.so [.] malloc
0.05% bzip2 libc-2.12.so [.] malloc
0.04% sleep libc-2.12.so [.] malloc
0.03%multipathd libc-2.12.so [.] malloc
0.01% sendmail libc-2.12.so [.] malloc
0.01% automount libc-2.12.so [.] malloc
The trap_nr addition patch is a prereq.
A few minor changes can be expected down the line based on the
discussions currently happening on lkml around the single-stepping
infrastructure code (http://marc.info/?l=linux-kernel&m=134545967710242&w=2).
Signed-off-by: Ananth N Mavinakayanahalli
---
Tested on POWER6; I don't see anything here that should stop it from
working on a ppc32; since I don't have access to a ppc32 machine, it
would be good if somoene could verify that part.
V4:
Enhance arch_analyze_uprobe_insn() to reject uprobes on trap variants.
V3:
Added abort_xol() logic for powerpc, using thread_struct.trap_nr to
determine if the stepped instruction caused an exception.
V2:
a. arch_uprobe_analyze_insn() now gets unsigned long addr.
b. Verified that mtmsr[d] and rfi[d] are handled correctly by
emulate_step() (no changes to this patch).
arch/powerpc/Kconfig |3
arch/powerpc/include/asm/thread_info.h |4
arch/powerpc/include/asm/uprobes.h | 58 ++
arch/powerpc/kernel/Makefile |1
arch/powerpc/kernel/signal.c |6 +
arch/powerpc/kernel/uprobes.c | 180 +
6 files changed, 251 insertions(+), 1 deletion(-)
Index: linux-tip-16aug/arch/powerpc/include/asm/thread_info.h
===
--- linux-tip-16aug.orig/arch/powerpc/include/asm/thread_info.h
+++ linux-tip-16aug/arch/powerpc/include/asm/thread_info.h
@@ -102,6 +102,7 @@ static inline struct thread_info *curren
#define TIF_RESTOREALL 11 /* Restore all regs (implies NOERROR) */
#define TIF_NOERROR12 /* Force successful syscall return */
#define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
+#define TIF_UPROBE 14 /* breakpointed or single-stepping */
#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
/* as above, but as bit values */
@@ -118,12 +119,13 @@ static inline struct thread_info *curren
#define _TIF_RESTOREALL(1<
+ */
+
+#include
+
+typedef unsigned int uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES4
+#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+
+#define UPROBE_SWBP_INSN 0x7fe8
+#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
+
+#define IS_TW(instr) (((instr) & 0xfc0007fe) == 0x7c08)
+#define IS_TD(instr) (((instr) & 0xfc0007fe) == 0x7c88)
+#define IS_TDI(instr) (((instr) & 0xfc00) == 0x0800)
+#define IS_TWI(instr) (((instr) & 0xfc00) == 0x0c00)
+
+#define is_trap(instr) (IS_TW(instr) || IS_TD(instr) || \
+ IS_TWI(instr) || IS_TDI(instr))
+
+struct arch_uprobe {
+ u8 insn[MAX_UINSN_BYTES];
+};
+
+struct arch_uprobe_task {
+ unsigned long saved_trap_nr;
+};
+
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm, unsigned long addr);
+extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
+extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned
long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+#endif /* _ASM_UPROBES_H */
Inde
[PATCH 1/2] powerpc: Add trap_nr to thread_struct
From: Ananth N Mavinakayanahalli
Add thread_struct.trap_nr and use it to store the last exception
the thread experienced. In this patch, we populate the field at
various places where we force_sig_info() to the process.
This is also used in uprobes to determine if the probed instruction
caused an exception.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/include/asm/processor.h |1 +
arch/powerpc/kernel/process.c|2 ++
arch/powerpc/kernel/traps.c |1 +
arch/powerpc/mm/fault.c |1 +
4 files changed, 5 insertions(+)
Index: linux-26jul/arch/powerpc/include/asm/processor.h
===
--- linux-26jul.orig/arch/powerpc/include/asm/processor.h
+++ linux-26jul/arch/powerpc/include/asm/processor.h
@@ -219,6 +219,7 @@ struct thread_struct {
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#endif
unsigned long dabr; /* Data address breakpoint register */
+ unsigned long trap_nr;/* last trap # on this thread */
#ifdef CONFIG_ALTIVEC
/* Complete AltiVec register set */
vector128 vr[32] __attribute__((aligned(16)));
Index: linux-26jul/arch/powerpc/kernel/process.c
===
--- linux-26jul.orig/arch/powerpc/kernel/process.c
+++ linux-26jul/arch/powerpc/kernel/process.c
@@ -258,6 +258,7 @@ void do_send_trap(struct pt_regs *regs,
{
siginfo_t info;
+ current->thread.trap_nr = signal_code;
if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
11, SIGSEGV) == NOTIFY_STOP)
return;
@@ -275,6 +276,7 @@ void do_dabr(struct pt_regs *regs, unsig
{
siginfo_t info;
+ current->thread.trap_nr = TRAP_HWBKPT;
if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
11, SIGSEGV) == NOTIFY_STOP)
return;
Index: linux-26jul/arch/powerpc/kernel/traps.c
===
--- linux-26jul.orig/arch/powerpc/kernel/traps.c
+++ linux-26jul/arch/powerpc/kernel/traps.c
@@ -251,6 +251,7 @@ void _exception(int signr, struct pt_reg
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
+ current->thread.trap_nr = code;
memset(&info, 0, sizeof(info));
info.si_signo = signr;
info.si_code = code;
Index: linux-26jul/arch/powerpc/mm/fault.c
===
--- linux-26jul.orig/arch/powerpc/mm/fault.c
+++ linux-26jul/arch/powerpc/mm/fault.c
@@ -133,6 +133,7 @@ static int do_sigbus(struct pt_regs *reg
up_read(¤t->mm->mmap_sem);
if (user_mode(regs)) {
+ current->thread.trap_nr = BUS_ADRERR;
info.si_signo = SIGBUS;
info.si_errno = 0;
info.si_code = BUS_ADRERR;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/2] powerpc: Uprobes port to powerpc
On Fri, Aug 17, 2012 at 05:00:31PM +0200, Oleg Nesterov wrote: > On 08/17, Ananth N Mavinakayanahalli wrote: > > > > On Thu, Aug 16, 2012 at 05:21:12PM +0200, Oleg Nesterov wrote: > > > > > Hmm, I am not sure. is_swbp_insn(insn), as it is used in the arch agnostic > > > code, should only return true if insn == UPROBE_SWBP_INSN (just in case, > > > this logic needs more fixes but this is offtopic). > > > > I think it does... > > > > > If powerpc has another insn(s) which can trigger powerpc's do_int3() > > > counterpart, they should be rejected by arch_uprobe_analyze_insn(). > > > I think. > > > > The insn that gets passed to arch_uprobe_analyze_insn() is copy_insn()'s > > version, which is the file copy of the instruction. > > Yes, exactly. And we are going to single-step this saved uprobe->arch.insn, > even if gdb/whatever replaces the original insn later or already replaced. > > So arch_uprobe_analyze_insn() should reject the "unsafe" instructions which > we can't step over safely. Agreed. > > We should also take > > care of the in-memory copy, in case gdb had inserted a breakpoint at the > > same location, right? > > gdb (or even the application itself) and uprobes can obviously confuse > each other, in many ways, and we can do nothing at least currently. > Just we should ensure that the kernel can't crash/hang/etc. Absolutely. The proper fix for this at least from a breakpoint insertion perspective is to educate gdb (possibly ptrace itself) to fail on a breakpoint insertion request on an already existing one. > > Updating is_swbp_insn() per-arch where needed will > > take care of both the cases, 'cos it gets called before > > arch_analyze_uprobe_insn() too. > > For example. set_swbp()->is_swbp_insn() == T means that (for example) > uprobe_register() and uprobe_mmap() raced with each other and there is > no need for set_swbp(). This is true for Intel like architectures that have *one* swbp instruction. On Powerpc, gdb for instance, can insert a trap variant at the address. Therefore, is_swbp_insn() by definition should return true for all trap variants. > However, find_active_uprobe()->is_swbp_at_addr()->is_swbp_insn() is > different, "true" confirms that this insn has triggered do_int3() and > thus we need send_sig(SIGTRAP) (just in case, this is not strictly > correct too but offtopic again). > > We definitely need more changes/fixes/improvements in this area. And > perhaps powerpc requires more changes in the arch-neutral code, I dunno. For powerpc, just having is_swbp_insn() (already a weak function) handle the trap variants, should suffice. > In particular, I think is_swbp_insn() should have a single caller, > is_swbp_at_addr(), and this caller should always play with current->mm. > And many, many other changes in the long term. > > So far I think that, if powerpc really needs to change is_swbp_insn(), > it would be better to make another patch and discuss this change. > But of course I can't judge. OK. I will separate out the is_swbp_insn() change into a separate patch. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/2] powerpc: Uprobes port to powerpc
On Thu, Aug 16, 2012 at 05:21:12PM +0200, Oleg Nesterov wrote: ... > > So, the arch agnostic code itself > > takes care of this case... > > Yes. I forgot about install_breakpoint()->is_swbp_insn() check which > returns -ENOTSUPP, somehow I thought arch_uprobe_analyze_insn() does > this. > > > or am I missing something? > > No, it is me. > > > However, I see that we need a powerpc specific is_swbp_insn() > > implementation since we will have to take care of all the trap variants. > > Hmm, I am not sure. is_swbp_insn(insn), as it is used in the arch agnostic > code, should only return true if insn == UPROBE_SWBP_INSN (just in case, > this logic needs more fixes but this is offtopic). I think it does... > If powerpc has another insn(s) which can trigger powerpc's do_int3() > counterpart, they should be rejected by arch_uprobe_analyze_insn(). > I think. The insn that gets passed to arch_uprobe_analyze_insn() is copy_insn()'s version, which is the file copy of the instruction. We should also take care of the in-memory copy, in case gdb had inserted a breakpoint at the same location, right? Updating is_swbp_insn() per-arch where needed will take care of both the cases, 'cos it gets called before arch_analyze_uprobe_insn() too. > > I will need to update the patches based on changes being made by Oleg > > and Sebastien for the single-step issues. > > Perhaps you can do this in a separate change? > > We need some (simple) changes in the arch agnostic code first, they > should not break poweppc. These changes are still under discussion. > Once we have "__weak arch_uprobe_step*" you can reimplement these > hooks and fix the problems with single-stepping. OK. Agreed. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/2] powerpc: Uprobes port to powerpc
On Thu, Aug 16, 2012 at 07:41:53AM +1000, Benjamin Herrenschmidt wrote: > On Wed, 2012-08-15 at 18:59 +0200, Oleg Nesterov wrote: > > On 07/26, Ananth N Mavinakayanahalli wrote: > > > > > > From: Ananth N Mavinakayanahalli > > > > > > This is the port of uprobes to powerpc. Usage is similar to x86. > > > > I am just curious why this series was ignored by powerpc maintainers... > > Because it arrived too late for the previous merge window considering my > limited bandwidth for reviewing things and that nobody else seems to > have reviewed it :-) > > It's still on track for the next one, and I'm hoping to dedicate most of > next week going through patches & doing a powerpc -next. Thanks Ben! > > Of course I can not review this code, I know nothing about powerpc, > > but the patches look simple/straightforward. > > > > Paul, Benjamin? > > > > Just one question... Shouldn't arch_uprobe_pre_xol() forbid to probe > > UPROBE_SWBP_INSN (at least) ? > > > > (I assume that emulate_step() can't handle this case but of course I > > do not understand arch/powerpc/lib/sstep.c) > > > > Note that uprobe_pre_sstep_notifier() sets utask->state = UTASK_BP_HIT > > without any checks. This doesn't look right if it was UTASK_SSTEP... > > > > But again, I do not know what powepc will actually do if we try to > > single-step over UPROBE_SWBP_INSN. > > Ananth ? set_swbp() will return -EEXIST to install_breakpoint if we are trying to put a breakpoint on UPROBE_SWBP_INSN. So, the arch agnostic code itself takes care of this case... or am I missing something? However, I see that we need a powerpc specific is_swbp_insn() implementation since we will have to take care of all the trap variants. I will need to update the patches based on changes being made by Oleg and Sebastien for the single-step issues. Will incorporate the powerpc specific is_swbp_insn() change along with the changes required for the single-step part and send out the next version. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3 2/2] powerpc: Uprobes port to powerpc
From: Ananth N Mavinakayanahalli
This is the port of uprobes to powerpc. Usage is similar to x86.
[root@ ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
Added new event:
probe_libc:malloc(on 0xb4860)
You can now use it in all perf tools, such as:
perf record -e probe_libc:malloc -aR sleep 1
[root@ ~]# ./bin/perf record -e probe_libc:malloc -aR sleep 20
[ perf record: Woken up 22 times to write data ]
[ perf record: Captured and wrote 5.843 MB perf.data (~255302 samples) ]
[root@ ~]# ./bin/perf report --stdio
...
# Samples: 83K of event 'probe_libc:malloc'
# Event count (approx.): 83484
#
# Overhead Command Shared Object Symbol
# . ..
#
69.05% tar libc-2.12.so [.] malloc
28.57%rm libc-2.12.so [.] malloc
1.32% avahi-daemon libc-2.12.so [.] malloc
0.58% bash libc-2.12.so [.] malloc
0.28% sshd libc-2.12.so [.] malloc
0.08%irqbalance libc-2.12.so [.] malloc
0.05% bzip2 libc-2.12.so [.] malloc
0.04% sleep libc-2.12.so [.] malloc
0.03%multipathd libc-2.12.so [.] malloc
0.01% sendmail libc-2.12.so [.] malloc
0.01% automount libc-2.12.so [.] malloc
Patch applies on the current master branch of Linus' tree (bdc0077af).
The trap_nr addition patch is a prereq.
Signed-off-by: Ananth N Mavinakayanahalli
---
Tested on POWER6; I don't see anything here that should stop it from
working on a ppc32; since I don't have access to a ppc32 machine, it
would be good if somoene could verify that part.
V3:
Added abort_xol() logic for powerpc, using thread_struct.trap_nr to
determine if the stepped instruction caused an exception.
V2:
a. arch_uprobe_analyze_insn() now gets unsigned long addr.
b. Verified that mtmsr[d] and rfi[d] are handled correctly by
emulate_step() (no changes to this patch).
arch/powerpc/Kconfig |3
arch/powerpc/include/asm/thread_info.h |4
arch/powerpc/include/asm/uprobes.h | 50 +
arch/powerpc/kernel/Makefile |1
arch/powerpc/kernel/signal.c |6 +
arch/powerpc/kernel/uprobes.c | 174 +
6 files changed, 237 insertions(+), 1 deletion(-)
Index: linux-26jul/arch/powerpc/include/asm/thread_info.h
===
--- linux-26jul.orig/arch/powerpc/include/asm/thread_info.h
+++ linux-26jul/arch/powerpc/include/asm/thread_info.h
@@ -102,6 +102,7 @@ static inline struct thread_info *curren
#define TIF_RESTOREALL 11 /* Restore all regs (implies NOERROR) */
#define TIF_NOERROR12 /* Force successful syscall return */
#define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
+#define TIF_UPROBE 14 /* breakpointed or single-stepping */
#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
/* as above, but as bit values */
@@ -118,12 +119,13 @@ static inline struct thread_info *curren
#define _TIF_RESTOREALL(1<
+ */
+
+#include
+
+typedef unsigned int uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES4
+#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+
+#define UPROBE_SWBP_INSN 0x7fe8
+#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
+
+struct arch_uprobe {
+ u8 insn[MAX_UINSN_BYTES];
+};
+
+struct arch_uprobe_task {
+ unsigned long saved_trap_nr;
+};
+
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm, unsigned long addr);
+extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
+extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned
long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+#endif /* _ASM_UPROBES_H */
Index: linux-26jul/arch/powerpc/kernel/Makefile
===
--- linux-26jul.orig/arch/powerpc/kernel/Makefile
+++ linux-26jul/arch/powerpc/kernel/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_MODULES) += ppc_ksyms.o
obj-$(CONFIG_BOOTX_TEXT) += btext.o
obj-$(CONFIG_SMP) += smp.o
obj-$(CONFIG_KPROBES) += kprobes.o
+obj-$(CONFIG_UPROBES) += uprobes.o
obj-$(CONFIG_PPC_UDBG_16550) += legacy_serial.o udbg_16550.o
obj-$(CONFIG_STACKTRACE) += stacktrace.o
obj-$(CONFIG_SWIOTLB) += dma-swiotlb.o
Index: linux-26jul/arch/powerpc/kernel/signal.c
=
[PATCH 1/2] powerpc: Add trap_nr to thread_struct
From: Ananth N Mavinakayanahalli
Add thread_struct.trap_nr and use it to store the last exception
the thread experienced. In this patch, we populate the field at
various places where we force_sig_info() to the process.
This is also used in uprobes to determine if the probed instruction
caused an exception.
Patch applies on the current master branch of Linus' tree (bdc0077af)
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/include/asm/processor.h |1 +
arch/powerpc/kernel/process.c|2 ++
arch/powerpc/kernel/traps.c |1 +
arch/powerpc/mm/fault.c |1 +
4 files changed, 5 insertions(+)
Index: linux-26jul/arch/powerpc/include/asm/processor.h
===
--- linux-26jul.orig/arch/powerpc/include/asm/processor.h
+++ linux-26jul/arch/powerpc/include/asm/processor.h
@@ -219,6 +219,7 @@ struct thread_struct {
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#endif
unsigned long dabr; /* Data address breakpoint register */
+ unsigned long trap_nr;/* last trap # on this thread */
#ifdef CONFIG_ALTIVEC
/* Complete AltiVec register set */
vector128 vr[32] __attribute__((aligned(16)));
Index: linux-26jul/arch/powerpc/kernel/process.c
===
--- linux-26jul.orig/arch/powerpc/kernel/process.c
+++ linux-26jul/arch/powerpc/kernel/process.c
@@ -258,6 +258,7 @@ void do_send_trap(struct pt_regs *regs,
{
siginfo_t info;
+ current->thread.trap_nr = signal_code;
if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
11, SIGSEGV) == NOTIFY_STOP)
return;
@@ -275,6 +276,7 @@ void do_dabr(struct pt_regs *regs, unsig
{
siginfo_t info;
+ current->thread.trap_nr = TRAP_HWBKPT;
if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
11, SIGSEGV) == NOTIFY_STOP)
return;
Index: linux-26jul/arch/powerpc/kernel/traps.c
===
--- linux-26jul.orig/arch/powerpc/kernel/traps.c
+++ linux-26jul/arch/powerpc/kernel/traps.c
@@ -251,6 +251,7 @@ void _exception(int signr, struct pt_reg
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
+ current->thread.trap_nr = code;
memset(&info, 0, sizeof(info));
info.si_signo = signr;
info.si_code = code;
Index: linux-26jul/arch/powerpc/mm/fault.c
===
--- linux-26jul.orig/arch/powerpc/mm/fault.c
+++ linux-26jul/arch/powerpc/mm/fault.c
@@ -133,6 +133,7 @@ static int do_sigbus(struct pt_regs *reg
up_read(¤t->mm->mmap_sem);
if (user_mode(regs)) {
+ current->thread.trap_nr = BUS_ADRERR;
info.si_signo = SIGBUS;
info.si_errno = 0;
info.si_code = BUS_ADRERR;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
On Tue, Jun 12, 2012 at 02:01:46PM +1000, Michael Ellerman wrote: > On Fri, 2012-06-08 at 14:51 +0530, Ananth N Mavinakayanahalli wrote: > > On Fri, Jun 08, 2012 at 04:38:17PM +1000, Michael Ellerman wrote: > > > On Fri, 2012-06-08 at 11:49 +0530, Ananth N Mavinakayanahalli wrote: > > > > On Fri, Jun 08, 2012 at 04:17:44PM +1000, Michael Ellerman wrote: > > > > > On Fri, 2012-06-08 at 11:31 +0530, Ananth N Mavinakayanahalli wrote: > > > > > > On Fri, Jun 08, 2012 at 03:51:54PM +1000, Michael Ellerman wrote: > > > > > > > On Fri, 2012-06-08 at 10:06 +0530, Ananth N Mavinakayanahalli > > > > > > > wrote: > > > > > > > > > > > > But MSR_PR=1 and hence emulate_step() will return -1 and hence we > > > > > > will > > > > > > end up single-stepping using user_enable_single_step(). Same with > > > > > > rfid. > > > > > > > > > > Right. But that was exactly Jim's point, you may be asked to emulate > > > > > those instructions even though you wouldn't expect to see them in > > > > > userspace code, so you need to handle it. > > > > > > > > > > Luckily it looks like emulate_step() will do the right thing for you. > > > > > It'd be good to test it to make 100% sure. > > > > > > > > Sure. Will add that check and send v2. > > > > > > Sorry I didn't mean add a test in the code, I meant construct a test > > > case to confirm that it works as expected. > > > > Michael, > > > > I just hand-coded the instr to emulate_step() and here are the results: > > > > MSR_PR is set > > insn = 7c600124, ret = 0 /* mtmsr */ > > insn = 7c600164, ret = 0 /* mtmsrd */ > > insn = 4c24, ret = -1 /* rfid */ > > insn = 4c64, ret = 0 /* rfi */ > > > > Also verified that standalone programs with those instructions in inline > > asm will die with a SIGILL. > > > > So, for mtmsr, mtmsrd and rfi, we have to single-step them which will > > result in a SIGILL in turn. > > What happens in the rfid case? You don't handle -1 from emulate_step() > any differently AFAICS, so don't we try to single step that too? -1 is just emulate_step() flagging cases where instructions must not be single-stepped (rfi[d], mtmsr that clears MSR_RI). But as with the other OEA instructions in user space, we fail with a SIGILL. As the application is hozed in any case if we encounter an OEA instruction, I'd think there is no point in handling a -1 from emulate_step() any differently. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 2/2] [POWERPC] uprobes: powerpc port
From: Ananth N Mavinakayanahalli
This is the port of uprobes to powerpc. Usage is similar to x86.
One TODO in this port compared to x86 is the uprobe abort_xol() logic.
x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine
if a signal was caused when the uprobed instruction was single-stepped/
emulated, in which case, we reset the instruction pointer to the probed
address and retry the probe again.
Tested on POWER6; I don't see anything here that should stop it from
working on a ppc32; since I don't have access to a ppc32 machine, it
would be good if somoene could verify that part.
[root@ ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
Added new event:
probe_libc:malloc(on 0xb4860)
You can now use it in all perf tools, such as:
perf record -e probe_libc:malloc -aR sleep 1
[root@ ~]# ./bin/perf record -e probe_libc:malloc -aR sleep 20
[ perf record: Woken up 22 times to write data ]
[ perf record: Captured and wrote 5.843 MB perf.data (~255302 samples) ]
[root@ ~]# ./bin/perf report --stdio
#
# captured on: Mon Jun 4 05:26:31 2012
# hostname : .ibm.com
# os release : 3.4.0-uprobe
# perf version : 3.4.0
# arch : ppc64
# nrcpus online : 4
# nrcpus avail : 4
# cpudesc : POWER6 (raw), altivec supported
# cpuid : 62,769
# total memory : 7310528 kB
# cmdline : /root/bin/perf record -e probe_libc:malloc -aR sleep 20
# event : name = probe_libc:malloc, type = 2, config = 0x124, config1 = 0x0, con
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
#
#
# Samples: 83K of event 'probe_libc:malloc'
# Event count (approx.): 83484
#
# Overhead Command Shared Object Symbol
# . ..
#
69.05% tar libc-2.12.so [.] malloc
28.57%rm libc-2.12.so [.] malloc
1.32% avahi-daemon libc-2.12.so [.] malloc
0.58% bash libc-2.12.so [.] malloc
0.28% sshd libc-2.12.so [.] malloc
0.08%irqbalance libc-2.12.so [.] malloc
0.05% bzip2 libc-2.12.so [.] malloc
0.04% sleep libc-2.12.so [.] malloc
0.03%multipathd libc-2.12.so [.] malloc
0.01% sendmail libc-2.12.so [.] malloc
0.01% automount libc-2.12.so [.] malloc
V2:
a. arch_uprobe_analyze_insn() now gets unsigned long addr.
b. Verified that mtmsr[d] and rfi[d] are handled correctly by
emulate_step() (no changes to this patch).
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/Kconfig |3
arch/powerpc/include/asm/thread_info.h |4
arch/powerpc/include/asm/uprobes.h | 49 +
arch/powerpc/kernel/Makefile |1
arch/powerpc/kernel/signal.c |6 +
arch/powerpc/kernel/uprobes.c | 164 +
6 files changed, 226 insertions(+), 1 deletion(-)
Index: linux-3.5-rc1/arch/powerpc/include/asm/thread_info.h
===
--- linux-3.5-rc1.orig/arch/powerpc/include/asm/thread_info.h
+++ linux-3.5-rc1/arch/powerpc/include/asm/thread_info.h
@@ -96,6 +96,7 @@ static inline struct thread_info *curren
#define TIF_RESTOREALL 11 /* Restore all regs (implies NOERROR) */
#define TIF_NOERROR12 /* Force successful syscall return */
#define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
+#define TIF_UPROBE 14 /* breakpointed or single-stepping */
#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
/* as above, but as bit values */
@@ -112,12 +113,13 @@ static inline struct thread_info *curren
#define _TIF_RESTOREALL(1<
+ */
+
+#include
+
+typedef unsigned int uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES4
+#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+
+#define UPROBE_SWBP_INSN 0x7fe8
+#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
+
+struct arch_uprobe {
+ u8 insn[MAX_UINSN_BYTES];
+};
+
+struct arch_uprobe_task {
+};
+
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm, unsigned long addr);
+extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
+extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned
long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+#endif /* _ASM_UPROBES_H */
Index: linux-3.5-rc1/arch/powerpc/kernel/Makefile
===
--- linux-3.5-rc1.orig/arch/
[PATCH v2 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Ananth N Mavinakayanahalli
On RISC architectures like powerpc, instructions are fixed size.
Instruction analysis on such platforms is just a matter of (insn % 4).
Pass the vaddr at which the uprobe is to be inserted so that
arch_uprobe_analyze_insn() can flag misaligned registration requests.
Changes in V2:
Pass (unsigned long)addr instead of (loff_t)vaddr to
arch_uprobe_analyze_insn(). We need the loff_t vaddr to take care of
the offset of inode:offset pair for large file sizes on 32bit.
Signed-off-by: Ananth N Mavinakaynahalli
---
arch/x86/include/asm/uprobes.h |2 +-
arch/x86/kernel/uprobes.c |3 ++-
kernel/events/uprobes.c|2 +-
3 files changed, 4 insertions(+), 3 deletions(-)
Index: linux-3.5-rc1/arch/x86/include/asm/uprobes.h
===
--- linux-3.5-rc1.orig/arch/x86/include/asm/uprobes.h
+++ linux-3.5-rc1/arch/x86/include/asm/uprobes.h
@@ -48,7 +48,7 @@ struct arch_uprobe_task {
#endif
};
-extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm);
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm, unsigned long addr);
extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
Index: linux-3.5-rc1/arch/x86/kernel/uprobes.c
===
--- linux-3.5-rc1.orig/arch/x86/kernel/uprobes.c
+++ linux-3.5-rc1/arch/x86/kernel/uprobes.c
@@ -409,9 +409,10 @@ static int validate_insn_bits(struct arc
* arch_uprobe_analyze_insn - instruction analysis including validity and
fixups.
* @mm: the probed address space.
* @arch_uprobe: the probepoint information.
+ * @addr: virtual address at which to install the probepoint
* Return 0 on success or a -ve number on error.
*/
-int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm)
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct
*mm, unsigned long addr)
{
int ret;
struct insn insn;
Index: linux-3.5-rc1/kernel/events/uprobes.c
===
--- linux-3.5-rc1.orig/kernel/events/uprobes.c
+++ linux-3.5-rc1/kernel/events/uprobes.c
@@ -697,7 +697,7 @@ install_breakpoint(struct uprobe *uprobe
if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
return -EEXIST;
- ret = arch_uprobe_analyze_insn(&uprobe->arch, mm);
+ ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, addr);
if (ret)
return ret;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
On Fri, Jun 08, 2012 at 04:38:17PM +1000, Michael Ellerman wrote: > On Fri, 2012-06-08 at 11:49 +0530, Ananth N Mavinakayanahalli wrote: > > On Fri, Jun 08, 2012 at 04:17:44PM +1000, Michael Ellerman wrote: > > > On Fri, 2012-06-08 at 11:31 +0530, Ananth N Mavinakayanahalli wrote: > > > > On Fri, Jun 08, 2012 at 03:51:54PM +1000, Michael Ellerman wrote: > > > > > On Fri, 2012-06-08 at 10:06 +0530, Ananth N Mavinakayanahalli wrote: > > > > > > > > But MSR_PR=1 and hence emulate_step() will return -1 and hence we will > > > > end up single-stepping using user_enable_single_step(). Same with rfid. > > > > > > Right. But that was exactly Jim's point, you may be asked to emulate > > > those instructions even though you wouldn't expect to see them in > > > userspace code, so you need to handle it. > > > > > > Luckily it looks like emulate_step() will do the right thing for you. > > > It'd be good to test it to make 100% sure. > > > > Sure. Will add that check and send v2. > > Sorry I didn't mean add a test in the code, I meant construct a test > case to confirm that it works as expected. Michael, I just hand-coded the instr to emulate_step() and here are the results: MSR_PR is set insn = 7c600124, ret = 0 /* mtmsr */ insn = 7c600164, ret = 0 /* mtmsrd */ insn = 4c24, ret = -1 /* rfid */ insn = 4c64, ret = 0 /* rfi */ Also verified that standalone programs with those instructions in inline asm will die with a SIGILL. So, for mtmsr, mtmsrd and rfi, we have to single-step them which will result in a SIGILL in turn. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
On Fri, Jun 08, 2012 at 04:17:44PM +1000, Michael Ellerman wrote: > On Fri, 2012-06-08 at 11:31 +0530, Ananth N Mavinakayanahalli wrote: > > On Fri, Jun 08, 2012 at 03:51:54PM +1000, Michael Ellerman wrote: > > > On Fri, 2012-06-08 at 10:06 +0530, Ananth N Mavinakayanahalli wrote: > > > > On Wed, Jun 06, 2012 at 11:08:04AM -0700, Jim Keniston wrote: > > > > > On Wed, 2012-06-06 at 15:05 +0530, Ananth N Mavinakayanahalli wrote: > > > > > > On Wed, Jun 06, 2012 at 11:27:02AM +0200, Peter Zijlstra wrote: > > > > > > > On Wed, 2012-06-06 at 14:51 +0530, Ananth N Mavinakayanahalli > > > > > > > wrote: > > > > > > > > ... > > > > > > > > > > For the kernel, the only ones that are off limits are rfi (return > > > > > > from > > > > > > interrupt), mtmsr (move to msr). All other instructions can be > > > > > > probed. > > > > > > > > > > > > Both those instructions are supervisor level, so we won't see them > > > > > > in > > > > > > userspace at all; so we should be able to probe all user level > > > > > > instructions. > > > > > > > > > > Presumably rfi or mtmsr could show up in the instruction stream via an > > > > > erroneous or mischievous asm statement. It'd be good to verify that > > > > > you > > > > > handle that gracefully. > > > > > > > > That'd be flagged elsewhere, by the architecture itself -- you'd get a > > > > privileged instruciton exception if you try execute any instruction not > > > > part of the UISA. I therefore don't think its a necessary check in the > > > > uprobes code. > > > > > > But you're not executing the instruction, you're passing it to > > > emulate_step(). Or am I missing something? > > > > But MSR_PR=1 and hence emulate_step() will return -1 and hence we will > > end up single-stepping using user_enable_single_step(). Same with rfid. > > Right. But that was exactly Jim's point, you may be asked to emulate > those instructions even though you wouldn't expect to see them in > userspace code, so you need to handle it. > > Luckily it looks like emulate_step() will do the right thing for you. > It'd be good to test it to make 100% sure. Sure. Will add that check and send v2. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
On Fri, Jun 08, 2012 at 03:51:54PM +1000, Michael Ellerman wrote: > On Fri, 2012-06-08 at 10:06 +0530, Ananth N Mavinakayanahalli wrote: > > On Wed, Jun 06, 2012 at 11:08:04AM -0700, Jim Keniston wrote: > > > On Wed, 2012-06-06 at 15:05 +0530, Ananth N Mavinakayanahalli wrote: > > > > On Wed, Jun 06, 2012 at 11:27:02AM +0200, Peter Zijlstra wrote: > > > > > On Wed, 2012-06-06 at 14:51 +0530, Ananth N Mavinakayanahalli wrote: > > > > ... > > > > > > For the kernel, the only ones that are off limits are rfi (return from > > > > interrupt), mtmsr (move to msr). All other instructions can be probed. > > > > > > > > Both those instructions are supervisor level, so we won't see them in > > > > userspace at all; so we should be able to probe all user level > > > > instructions. > > > > > > Presumably rfi or mtmsr could show up in the instruction stream via an > > > erroneous or mischievous asm statement. It'd be good to verify that you > > > handle that gracefully. > > > > That'd be flagged elsewhere, by the architecture itself -- you'd get a > > privileged instruciton exception if you try execute any instruction not > > part of the UISA. I therefore don't think its a necessary check in the > > uprobes code. > > But you're not executing the instruction, you're passing it to > emulate_step(). Or am I missing something? But MSR_PR=1 and hence emulate_step() will return -1 and hence we will end up single-stepping using user_enable_single_step(). Same with rfid. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
On Wed, Jun 06, 2012 at 11:08:04AM -0700, Jim Keniston wrote: > On Wed, 2012-06-06 at 15:05 +0530, Ananth N Mavinakayanahalli wrote: > > On Wed, Jun 06, 2012 at 11:27:02AM +0200, Peter Zijlstra wrote: > > > On Wed, 2012-06-06 at 14:51 +0530, Ananth N Mavinakayanahalli wrote: ... > > For the kernel, the only ones that are off limits are rfi (return from > > interrupt), mtmsr (move to msr). All other instructions can be probed. > > > > Both those instructions are supervisor level, so we won't see them in > > userspace at all; so we should be able to probe all user level > > instructions. > > Presumably rfi or mtmsr could show up in the instruction stream via an > erroneous or mischievous asm statement. It'd be good to verify that you > handle that gracefully. That'd be flagged elsewhere, by the architecture itself -- you'd get a privileged instruciton exception if you try execute any instruction not part of the UISA. I therefore don't think its a necessary check in the uprobes code. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
On Wed, Jun 06, 2012 at 05:14:23PM +0530, Srikar Dronamraju wrote: > * Ingo Molnar [2012-06-06 11:40:15]: > > > > > * Ananth N Mavinakayanahalli wrote: > > > > > On Wed, Jun 06, 2012 at 11:23:52AM +0200, Peter Zijlstra wrote: > > > > On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote: > > > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct > > > > > mm_struct *mm, loff_t vaddr) > > > > > > > > Don't we traditionally use unsigned long to pass vaddrs? > > > > > > Right. But the vaddr we pass here is vma_info->vaddr which is loff_t. > > > I guess I should've made that clear in the patch description. > > > > Why not fix struct vma_info's vaddr type? > > > > Calculating and comparing vaddr results either uses variables of type loff_t. > To avoid typecasting and avoid overflow at each of these places, we used > loff_t. > > Ananth, install_breakpoint() already has a variable of type addr of type > unsigned long. Why dont you use addr instead of vaddr. Ok, makes sense. I'll change it in v2. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
On Wed, Jun 06, 2012 at 11:40:15AM +0200, Ingo Molnar wrote: > > * Ananth N Mavinakayanahalli wrote: > > > On Wed, Jun 06, 2012 at 11:23:52AM +0200, Peter Zijlstra wrote: > > > On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote: > > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct > > > > mm_struct *mm, loff_t vaddr) > > > > > > Don't we traditionally use unsigned long to pass vaddrs? > > > > Right. But the vaddr we pass here is vma_info->vaddr which is loff_t. > > I guess I should've made that clear in the patch description. > > Why not fix struct vma_info's vaddr type? Agreed. Will fix and send v2. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
On Wed, Jun 06, 2012 at 11:23:52AM +0200, Peter Zijlstra wrote: > On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote: > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct > > *mm, loff_t vaddr) > > Don't we traditionally use unsigned long to pass vaddrs? Right. But the vaddr we pass here is vma_info->vaddr which is loff_t. I guess I should've made that clear in the patch description. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
On Wed, Jun 06, 2012 at 11:27:02AM +0200, Peter Zijlstra wrote: > On Wed, 2012-06-06 at 14:51 +0530, Ananth N Mavinakayanahalli wrote: > > One TODO in this port compared to x86 is the uprobe abort_xol() logic. > > x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine > > if a signal was caused when the uprobed instruction was single-stepped/ > > emulated, in which case, we reset the instruction pointer to the probed > > address and retry the probe again. > > Another curious difference is that x86 uses an instruction decoder and > contains massive tables to validate we can probe a particular > instruction. > > Can we probe all possible PPC instructions? For the kernel, the only ones that are off limits are rfi (return from interrupt), mtmsr (move to msr). All other instructions can be probed. Both those instructions are supervisor level, so we won't see them in userspace at all; so we should be able to probe all user level instructions. I am not aware of specific caveats for vector/altivec instructions; maybe Paul or Ben are more suitable to comment on that. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/2] [POWERPC] uprobes: powerpc port
From: Ananth N Mavinakayanahalli
This is the port of uprobes to powerpc. Usage is similar to x86.
One TODO in this port compared to x86 is the uprobe abort_xol() logic.
x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine
if a signal was caused when the uprobed instruction was single-stepped/
emulated, in which case, we reset the instruction pointer to the probed
address and retry the probe again.
[root@ ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
Added new event:
probe_libc:malloc(on 0xb4860)
You can now use it in all perf tools, such as:
perf record -e probe_libc:malloc -aR sleep 1
[root@ ~]# ./bin/perf record -e probe_libc:malloc -aR sleep 20
[ perf record: Woken up 22 times to write data ]
[ perf record: Captured and wrote 5.843 MB perf.data (~255302 samples) ]
[root@ ~]# ./bin/perf report --stdio
#
# captured on: Mon Jun 4 05:26:31 2012
# hostname : .ibm.com
# os release : 3.4.0-uprobe
# perf version : 3.4.0
# arch : ppc64
# nrcpus online : 4
# nrcpus avail : 4
# cpudesc : POWER6 (raw), altivec supported
# cpuid : 62,769
# total memory : 7310528 kB
# cmdline : /root/bin/perf record -e probe_libc:malloc -aR sleep 20
# event : name = probe_libc:malloc, type = 2, config = 0x124, config1 = 0x0, con
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
#
#
# Samples: 83K of event 'probe_libc:malloc'
# Event count (approx.): 83484
#
# Overhead Command Shared Object Symbol
# . ..
#
69.05% tar libc-2.12.so [.] malloc
28.57%rm libc-2.12.so [.] malloc
1.32% avahi-daemon libc-2.12.so [.] malloc
0.58% bash libc-2.12.so [.] malloc
0.28% sshd libc-2.12.so [.] malloc
0.08%irqbalance libc-2.12.so [.] malloc
0.05% bzip2 libc-2.12.so [.] malloc
0.04% sleep libc-2.12.so [.] malloc
0.03%multipathd libc-2.12.so [.] malloc
0.01% sendmail libc-2.12.so [.] malloc
0.01% automount libc-2.12.so [.] malloc
Signed-off-by: Ananth N Mavinakayanahalli
Index: linux-3.5-rc1/arch/powerpc/include/asm/thread_info.h
===
--- linux-3.5-rc1.orig/arch/powerpc/include/asm/thread_info.h 2012-06-03
06:59:26.0 +0530
+++ linux-3.5-rc1/arch/powerpc/include/asm/thread_info.h2012-06-03
21:05:48.226233001 +0530
@@ -96,6 +96,7 @@
#define TIF_RESTOREALL 11 /* Restore all regs (implies NOERROR) */
#define TIF_NOERROR12 /* Force successful syscall return */
#define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
+#define TIF_UPROBE 14 /* breakpointed or single-stepping */
#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
/* as above, but as bit values */
@@ -112,12 +113,13 @@
#define _TIF_RESTOREALL(1<
+ */
+
+#include
+
+typedef unsigned int uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES4
+#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+
+#define UPROBE_SWBP_INSN 0x7fe8
+#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
+
+struct arch_uprobe {
+ u8 insn[MAX_UINSN_BYTES];
+};
+
+struct arch_uprobe_task {
+};
+
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm, loff_t vaddr);
+extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
+extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned
long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
+#endif /* _ASM_UPROBES_H */
Index: linux-3.5-rc1/arch/powerpc/kernel/Makefile
===
--- linux-3.5-rc1.orig/arch/powerpc/kernel/Makefile 2012-06-03
06:59:26.0 +0530
+++ linux-3.5-rc1/arch/powerpc/kernel/Makefile 2012-06-03 21:05:48.226233001
+0530
@@ -96,6 +96,7 @@
obj-$(CONFIG_BOOTX_TEXT) += btext.o
obj-$(CONFIG_SMP) += smp.o
obj-$(CONFIG_KPROBES) += kprobes.o
+obj-$(CONFIG_UPROBES) += uprobes.o
obj-$(CONFIG_PPC_UDBG_16550) += legacy_serial.o udbg_16550.o
obj-$(CONFIG_STACKTRACE) += stacktrace.o
obj-$(CONFIG_SWIOTLB) += dma-swiotlb.o
Index: linux-3.5-rc1/arch/powerpc/kernel/signal.c
===
--- linux-3.5-rc1.orig/arch/powerpc/kernel/signal.c 2012-06-03
06:59:26.0 +0530
+++ linux-3.5-rc1/arch/powerpc/kernel/signal.c
[PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Ananth N Mavinakayanahalli
On RISC architectures like powerpc, instructions are fixed size.
Instruction analysis on such platforms is just a matter of (insn % 4).
Pass the vaddr at which the uprobe is to be inserted so that
arch_uprobe_analyze_insn() can flag misaligned registration requests.
Signed-off-by: Ananth N Mavinakaynahalli
---
arch/x86/include/asm/uprobes.h |2 +-
arch/x86/kernel/uprobes.c |3 ++-
kernel/events/uprobes.c|2 +-
3 files changed, 4 insertions(+), 3 deletions(-)
Index: uprobes-24may/arch/x86/include/asm/uprobes.h
===
--- uprobes-24may.orig/arch/x86/include/asm/uprobes.h
+++ uprobes-24may/arch/x86/include/asm/uprobes.h
@@ -48,7 +48,7 @@ struct arch_uprobe_task {
#endif
};
-extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm);
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct
*mm, loff_t vaddr);
extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs
*regs);
extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
Index: uprobes-24may/arch/x86/kernel/uprobes.c
===
--- uprobes-24may.orig/arch/x86/kernel/uprobes.c
+++ uprobes-24may/arch/x86/kernel/uprobes.c
@@ -409,9 +409,10 @@ static int validate_insn_bits(struct arc
* arch_uprobe_analyze_insn - instruction analysis including validity and
fixups.
* @mm: the probed address space.
* @arch_uprobe: the probepoint information.
+ * @vaddr: virtual address at which to install the probepoint
* Return 0 on success or a -ve number on error.
*/
-int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm)
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct
*mm, loff_t vaddr)
{
int ret;
struct insn insn;
Index: uprobes-24may/kernel/events/uprobes.c
===
--- uprobes-24may.orig/kernel/events/uprobes.c
+++ uprobes-24may/kernel/events/uprobes.c
@@ -697,7 +697,7 @@ install_breakpoint(struct uprobe *uprobe
if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
return -EEXIST;
- ret = arch_uprobe_analyze_insn(&uprobe->arch, mm);
+ ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
if (ret)
return ret;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V2] powerpc: Export PIR data through sysfs
On Fri, Nov 11, 2011 at 10:17:55AM +0530, Ananth N Mavinakayanahalli wrote: > > > > At this rate we're going to end up with no bits left for CPU features > > way too quickly... Especially for something we only care about once at > > boot time. > > > > Wouldn't CPU_FTR_PPCAS_ARCH_V2 be a good enough test ? > > /me checks Cell manuals... yes, that test would be good enough. I will > cook up a patch to use this. Here it is... --- From: Ananth N Mavinakayanahalli The Processor Identification Register (PIR) on some powerpc platforms provides information to decode the processor identification tag that can be used for node/core/thread affinity tests and for debugging. Decoding this information is platform specific. Export PIR contents through sysfs. [V2] Use CPU_FTR_PPCAS_ARCH_V2 as a test for PIR's presence per BenH's suggestion. Signed-off-by: Ananth N Mavinakayanahalli --- arch/powerpc/kernel/sysfs.c |8 1 file changed, 8 insertions(+) Index: linux-3.2-rc1/arch/powerpc/kernel/sysfs.c === --- linux-3.2-rc1.orig/arch/powerpc/kernel/sysfs.c +++ linux-3.2-rc1/arch/powerpc/kernel/sysfs.c @@ -177,11 +177,13 @@ SYSFS_PMCSETUP(mmcra, SPRN_MMCRA); SYSFS_PMCSETUP(purr, SPRN_PURR); SYSFS_PMCSETUP(spurr, SPRN_SPURR); SYSFS_PMCSETUP(dscr, SPRN_DSCR); +SYSFS_PMCSETUP(pir, SPRN_PIR); static SYSDEV_ATTR(mmcra, 0600, show_mmcra, store_mmcra); static SYSDEV_ATTR(spurr, 0600, show_spurr, NULL); static SYSDEV_ATTR(dscr, 0600, show_dscr, store_dscr); static SYSDEV_ATTR(purr, 0600, show_purr, store_purr); +static SYSDEV_ATTR(pir, 0400, show_pir, NULL); unsigned long dscr_default = 0; EXPORT_SYMBOL(dscr_default); @@ -392,6 +394,9 @@ static void __cpuinit register_cpu_onlin if (cpu_has_feature(CPU_FTR_DSCR)) sysdev_create_file(s, &attr_dscr); + + if (cpu_has_feature(CPU_FTR_PPCAS_ARCH_V2)) + sysdev_create_file(s, &attr_pir); #endif /* CONFIG_PPC64 */ cacheinfo_cpu_online(cpu); @@ -462,6 +467,9 @@ static void unregister_cpu_online(unsign if (cpu_has_feature(CPU_FTR_DSCR)) sysdev_remove_file(s, &attr_dscr); + + if (cpu_has_feature(CPU_FTR_PPCAS_ARCH_V2)) + sysdev_remove_file(s, &attr_pir); #endif /* CONFIG_PPC64 */ cacheinfo_cpu_offline(cpu); ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc: Export PIR data through sysfs
On Fri, Nov 11, 2011 at 03:18:14PM +1100, Benjamin Herrenschmidt wrote: > On Thu, 2011-11-10 at 14:18 +0530, Ananth N Mavinakayanahalli wrote: > > > > > From: Ananth N Mavinakayanahalli > > > > The Processor Identification Register (PIR) on some powerpc platforms > > provides information to decode the processor identification tag. > > Decoding this information is platform specific. > > > > We currently need this information for POWERx processors and hence > > follows a similar model as adopted for the other POWERx specific > > features. > > At this rate we're going to end up with no bits left for CPU features > way too quickly... Especially for something we only care about once at > boot time. > > Wouldn't CPU_FTR_PPCAS_ARCH_V2 be a good enough test ? /me checks Cell manuals... yes, that test would be good enough. I will cook up a patch to use this. > Can you tell us a bit more about the real use for that feature ? I still > don't see what's the point of getting the underlying HW ID. This is a requirement from the hardware system test folks for use with their core, node and thread tests. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc: Export PIR data through sysfs
On Wed, Nov 09, 2011 at 09:48:25AM -0600, Scott Wood wrote: > On Wed, Nov 09, 2011 at 10:11:24AM +0530, Ananth N Mavinakayanahalli wrote: > > On Tue, Nov 08, 2011 at 10:59:46AM -0600, Scott Wood wrote: > > > On 11/08/2011 12:58 AM, Ananth N Mavinakayanahalli wrote: > > > > On Mon, Nov 07, 2011 at 11:18:32AM -0600, Scott Wood wrote: > > > >> What use does userspace have for this? If you want to return the > > > >> currently executing CPU (which unless you're pinned could change as > > > >> soon > > > >> as the value is read...), why not just return smp_processor_id() or > > > >> hard_smp_processor_id()? > > > > > > > > Its not just the current cpu. Decoding PIR can tell you the core id, > > > > thread id in case of SMT, and this information can be used by userspace > > > > apps to set affinities, etc. > > > > > > Wouldn't it make more sense to expose the thread to core mappings in a > > > general way, not tied to hardware or what thread we're currently running > > > on? > > > > AFAIK, the information encoding in PIR is platform dependent. There is > > no general way to expose this information unless you want have a > > per-platform ifdef. Even then, I am not sure if that information will > > generally be available or provided. > > > > > What's the use case for knowing this information only about the current > > > thread (or rather the state the current thread was in a few moments ago)? > > > > Its not information about the thread but about the cpu. Unless you have > > a shared LPAR environment, the data will be consistent and can be used > > by applications with knowledge of the platform. > > I'm not sure what a "shared LPAR environment" is, but unless you're > pinned there's no guarantee the CPU you're running on once the read() > syscall returns is the same as the one that PIR was read on. Do you mean > you're expecting this to be run from inside a partition that runs only on > one CPU, and thus whichever thread you'll be migrated to will have the > other data remain the same? This will be used from a partition with a dedicated set of cpus and so the data will remain consistent. > > I think this calls for a CPU_FTR_PIR. What do you suggest? > > Unless someone wants to test what actually happens when you read PIR on > all these CPUs... > > What platform is this meant to be useful for? Perhaps it could just be a > platform-specific sysfs entry? Currently I only care about pseries and have tested the following patch on them. I've therefore made the code similar to what currently exists for other features unique to POWER. Ananth --- From: Ananth N Mavinakayanahalli The Processor Identification Register (PIR) on some powerpc platforms provides information to decode the processor identification tag. Decoding this information is platform specific. We currently need this information for POWERx processors and hence follows a similar model as adopted for the other POWERx specific features. Signed-off-by: Ananth N Mavinakayanahalli --- arch/powerpc/include/asm/cputable.h |9 + arch/powerpc/kernel/sysfs.c |8 2 files changed, 13 insertions(+), 4 deletions(-) Index: linux-3.2-rc1/arch/powerpc/include/asm/cputable.h === --- linux-3.2-rc1.orig/arch/powerpc/include/asm/cputable.h +++ linux-3.2-rc1/arch/powerpc/include/asm/cputable.h @@ -201,6 +201,7 @@ extern const char *powerpc_base_platform #define CPU_FTR_POPCNTB LONG_ASM_CONST(0x0400) #define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0800) #define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000) +#define CPU_FTR_PIRLONG_ASM_CONST(0x2000) #ifndef __ASSEMBLY__ @@ -400,7 +401,7 @@ extern const char *powerpc_base_platform #define CPU_FTRS_POWER4(CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \ CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \ CPU_FTR_MMCRA | CPU_FTR_CP_USE_DCBTZ | \ - CPU_FTR_STCX_CHECKS_ADDRESS) + CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_PIR) #define CPU_FTRS_PPC970(CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \ CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | CPU_FTR_ARCH_201 | \ CPU_FTR_ALTIVEC_COMP | CPU_FTR_CAN_NAP | CPU_FTR_MMCRA | \ @@ -408,19 +409,19 @@ extern const char *powerpc_base_platform CPU_FTR_HVMODE) #define CPU_FTRS_POWER5(CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \ CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \ - CPU_FTR_MMCRA | CPU_FTR_SMT | \ +
Re: [PATCH] powerpc: Export PIR data through sysfs
On Tue, Nov 08, 2011 at 10:59:46AM -0600, Scott Wood wrote: > On 11/08/2011 12:58 AM, Ananth N Mavinakayanahalli wrote: > > On Mon, Nov 07, 2011 at 11:18:32AM -0600, Scott Wood wrote: > >> What use does userspace have for this? If you want to return the > >> currently executing CPU (which unless you're pinned could change as soon > >> as the value is read...), why not just return smp_processor_id() or > >> hard_smp_processor_id()? > > > > Its not just the current cpu. Decoding PIR can tell you the core id, > > thread id in case of SMT, and this information can be used by userspace > > apps to set affinities, etc. > > Wouldn't it make more sense to expose the thread to core mappings in a > general way, not tied to hardware or what thread we're currently running on? AFAIK, the information encoding in PIR is platform dependent. There is no general way to expose this information unless you want have a per-platform ifdef. Even then, I am not sure if that information will generally be available or provided. > What's the use case for knowing this information only about the current > thread (or rather the state the current thread was in a few moments ago)? Its not information about the thread but about the cpu. Unless you have a shared LPAR environment, the data will be consistent and can be used by applications with knowledge of the platform. > > +#if defined(CONFIG_SMP) && defined(SPRN_PIR) > > +SYSFS_PMCSETUP(pir, SPRN_PIR); > > +static SYSDEV_ATTR(pir, 0400, show_pir, NULL); > > +#endif > > This only helps on architectures such as 8xx where you can't build as > SMP -- and I don't think #ifdef SPRN_PIR excludes any builds. > > It doesn't help on chips like 750 or e300 where you can run a normal 6xx > SMP build, you just won't have multiple CPUs, and thus won't run things > like the secondary entry code. Ugh! Booke builds seem to be fun :-) I think this calls for a CPU_FTR_PIR. What do you suggest? Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc: Export PIR data through sysfs
On Mon, Nov 07, 2011 at 11:18:32AM -0600, Scott Wood wrote:
> On 11/06/2011 10:47 PM, Ananth N Mavinakayanahalli wrote:
> > The Processor Identification Register (PIR) on powerpc provides
> > information to decode the processor identification tag. Decoding
> > this information platform specfic.
> >
> > Export PIR data via sysfs.
> >
> > (Powerpc manuals state this register is 'optional'. I am not sure
> > though if there are any Linux supported powerpc platforms that
> > don't have it. Code in the kernel referencing PIR isn't under
> > a platform ifdef).
>
> Those references are in platform-specific files, under #ifdef
> CONFIG_SMP, often in areas that would only be executed in the presence
> of multiple CPUs (e.g. secondary release). The reference in misc_32.S
> is inside #ifdef CONFIG_KEXEC and is fairly recent -- it may not have
> been tested on these systems.
>
> I don't see PIR (other than in the acronym definition section) in
> manuals for UP-only cores such as e300, 8xx, and 750.
I saw that SPRN_PIR is defined for booke in reg_booke.h but wasn't sure
if it is applicable to all platforms. Thanks for the clarification.
> What use does userspace have for this? If you want to return the
> currently executing CPU (which unless you're pinned could change as soon
> as the value is read...), why not just return smp_processor_id() or
> hard_smp_processor_id()?
Its not just the current cpu. Decoding PIR can tell you the core id,
thread id in case of SMT, and this information can be used by userspace
apps to set affinities, etc.
How does the following look?
Ananth
---
From: Ananth N Mavinakayanahalli
The Processor Identification Register (PIR) on powerpc provides
information to decode the processor identification tag. Decoding
this information platform specfic.
Export PIR data via sysfs.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/kernel/sysfs.c | 13 +
1 file changed, 13 insertions(+)
Index: linux-3.1/arch/powerpc/kernel/sysfs.c
===
--- linux-3.1.orig/arch/powerpc/kernel/sysfs.c
+++ linux-3.1/arch/powerpc/kernel/sysfs.c
@@ -330,6 +330,11 @@ static struct sysdev_attribute pa6t_attr
#endif /* HAS_PPC_PMC_PA6T */
#endif /* HAS_PPC_PMC_CLASSIC */
+#if defined(CONFIG_SMP) && defined(SPRN_PIR)
+SYSFS_PMCSETUP(pir, SPRN_PIR);
+static SYSDEV_ATTR(pir, 0400, show_pir, NULL);
+#endif
+
static void __cpuinit register_cpu_online(unsigned int cpu)
{
struct cpu *c = &per_cpu(cpu_devices, cpu);
@@ -394,6 +399,10 @@ static void __cpuinit register_cpu_onlin
sysdev_create_file(s, &attr_dscr);
#endif /* CONFIG_PPC64 */
+#if defined(CONFIG_SMP) && defined(SPRN_PIR)
+ sysdev_create_file(s, &attr_pir);
+#endif
+
cacheinfo_cpu_online(cpu);
}
@@ -464,6 +473,10 @@ static void unregister_cpu_online(unsign
sysdev_remove_file(s, &attr_dscr);
#endif /* CONFIG_PPC64 */
+#if defined(CONFIG_SMP) && defined(SPRN_PIR)
+ sysdev_remove_file(s, &attr_pir);
+#endif
+
cacheinfo_cpu_offline(cpu);
}
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Export PIR data through sysfs
The Processor Identification Register (PIR) on powerpc provides information to decode the processor identification tag. Decoding this information platform specfic. Export PIR data via sysfs. (Powerpc manuals state this register is 'optional'. I am not sure though if there are any Linux supported powerpc platforms that don't have it. Code in the kernel referencing PIR isn't under a platform ifdef). Signed-off-by: Ananth N Mavinakayanahalli --- arch/powerpc/kernel/sysfs.c |6 ++ 1 file changed, 6 insertions(+) Index: linux-3.1/arch/powerpc/kernel/sysfs.c === --- linux-3.1.orig/arch/powerpc/kernel/sysfs.c +++ linux-3.1/arch/powerpc/kernel/sysfs.c @@ -177,11 +177,13 @@ SYSFS_PMCSETUP(mmcra, SPRN_MMCRA); SYSFS_PMCSETUP(purr, SPRN_PURR); SYSFS_PMCSETUP(spurr, SPRN_SPURR); SYSFS_PMCSETUP(dscr, SPRN_DSCR); +SYSFS_PMCSETUP(pir, SPRN_PIR); static SYSDEV_ATTR(mmcra, 0600, show_mmcra, store_mmcra); static SYSDEV_ATTR(spurr, 0600, show_spurr, NULL); static SYSDEV_ATTR(dscr, 0600, show_dscr, store_dscr); static SYSDEV_ATTR(purr, 0600, show_purr, store_purr); +static SYSDEV_ATTR(pir, 0400, show_pir, NULL); unsigned long dscr_default = 0; EXPORT_SYMBOL(dscr_default); @@ -394,6 +396,8 @@ static void __cpuinit register_cpu_onlin sysdev_create_file(s, &attr_dscr); #endif /* CONFIG_PPC64 */ + sysdev_create_file(s, &attr_pir); + cacheinfo_cpu_online(cpu); } @@ -464,6 +468,8 @@ static void unregister_cpu_online(unsign sysdev_remove_file(s, &attr_dscr); #endif /* CONFIG_PPC64 */ + sysdev_remove_file(s, &attr_pir); + cacheinfo_cpu_offline(cpu); } ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v2 PATCH 2/2] booke/kprobe: remove unnecessary preempt_enable_no_resched
On Mon, Jul 11, 2011 at 10:39:35AM +0800, Tiejun Chen wrote: > When enable CONFIG_PREEMPT we will trigger the following call trace: > > BUG: scheduling while atomic: swapper/1/0x1000 > ... > > krpobe always goes through the following path: > > program_check_exception() > | > + notify_die(DIE_BPT, "breakpoint",...) > | > + kprobe_handler() > | > + preempt_disable(); > + break_handler() <- preempt_enable_no_resched() > + emulate_step() > + preempt_enable_no_resched() > ... > exit > > We should remove unnecessary preempt_enable_no_resched() inside of > break_handler() > since looks longjmp_break_handler() always go the above path. The current code is correct. Reasoning follows... setjmp_pre_handler() and longjmp_break_handler() are used only for jprobes. In the case of a jprobe, the code flow would be: bp hit -> kprobe_handler() -> preempt_disable() -> setjmp_pre_handler() (not that since this routine returns 1, we skip sstep here) -> jp->entry() -> jprobe_return() -> bp hit -> kprobe_handler() -> preempt_disable() again -> longjmp_break_handler() -> preempt_enable() -> sstep -> preempt_enable() (for the second kprobe_handler() entry). You could verify this with a preempt_count() printk with a CONFIG_PREEMPT=y kernel. > Signed-off-by: Tiejun Chen Nack, sorry :-) Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [BUG?]3.0-rc4+ftrace+kprobe: set kprobe at instruction 'stwu' lead to system crash/freeze
On Wed, Jun 29, 2011 at 02:23:28PM +0800, Yong Zhang wrote: > On Mon, Jun 27, 2011 at 6:01 PM, Ananth N Mavinakayanahalli > wrote: > > On Sun, Jun 26, 2011 at 11:47:13PM +0900, Masami Hiramatsu wrote: > >> (2011/06/24 19:29), Steven Rostedt wrote: > >> > On Fri, 2011-06-24 at 17:21 +0800, Yong Zhang wrote: > >> >> Hi, > >> >> > >> >> When I use kprobe to do something, I found some wired thing. > >> >> > >> >> When CONFIG_FUNCTION_TRACER is disabled: > >> >> (gdb) disassemble do_fork > >> >> Dump of assembler code for function do_fork: > >> >> 0xc0037390 <+0>: mflr r0 > >> >> 0xc0037394 <+4>: stwu r1,-64(r1) > >> >> 0xc0037398 <+8>: mfcr r12 > >> >> 0xc003739c <+12>: stmw r27,44(r1) > >> >> > >> >> Then I: > >> >> modprobe kprobe_example func=do_fork offset=4 > >> >> ls > >> >> Things works well. > >> >> > >> >> But when CONFIG_FUNCTION_TRACER is enabled: > >> >> (gdb) disassemble do_fork > >> >> Dump of assembler code for function do_fork: > >> >> 0xc0040334 <+0>: mflr r0 > >> >> 0xc0040338 <+4>: stw r0,4(r1) > >> >> 0xc004033c <+8>: bl 0xc00109d4 > >> >> 0xc0040340 <+12>: stwu r1,-80(r1) > >> >> 0xc0040344 <+16>: mflr r0 > >> >> 0xc0040348 <+20>: stw r0,84(r1) > >> >> 0xc004034c <+24>: mfcr r12 > >> >> Then I: > >> >> modprobe kprobe_example func=do_fork offset=12 > >> >> ls > >> >> 'ls' will never retrun. system freeze. > >> > > >> > I'm not sure if x86 had a similar issue. > >> > > >> > Masami, have any ideas to why this happened? > >> > >> No, I don't familiar with ppc implementation. I guess > >> that single-step resume code failed to emulate the > >> instruction, but it strongly depends on ppc arch. > >> Maybe IBM people may know what happened. > >> > >> Ananth, Jim, would you have any ideas? > > > > On powerpc, we emulate sstep whenever possible. Only recently support to > > emulate loads and stores got added. I don't have access to a powerpc box > > today... but will try to recreate the problem ASAP and see what could be > > happening in the presence of mcount. > > After taking more testing on it, it looks like the issue doesn't > depend on mcount > (AKA. CONFIG_FUNCTION_TRACER) > > As I said in the first email, with eldk-5.0 CONFIG_FUNCTION_TRACER=n > will work well. > > But when I'm using eldk-4.2[1], both will fail. But the funny thing is when I > set kprobe at several functions some works fine but some will fail. For > example, > at this time do_fork() works well, but show_interrupt() will crash. Certain functions are off limits for probing -- look for __kprobe annotations in the kernel. Some such functions are arch specific, but show_interrupts() would definitely not be one of them. It works fine on my (64bit) test box. At this time, I think your best bet is to work with the eldk folks to narrow down the problem. Given the current set of data, I am inclined to think it could be an eldk bug, not a kernel one. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [BUG?]3.0-rc4+ftrace+kprobe: set kprobe at instruction 'stwu' lead to system crash/freeze
On Mon, Jun 27, 2011 at 03:31:05PM +0530, Ananth N Mavinakayanahalli wrote: > On Sun, Jun 26, 2011 at 11:47:13PM +0900, Masami Hiramatsu wrote: > > (2011/06/24 19:29), Steven Rostedt wrote: > > > On Fri, 2011-06-24 at 17:21 +0800, Yong Zhang wrote: > > >> Hi, > > >> > > >> When I use kprobe to do something, I found some wired thing. > > >> > > >> When CONFIG_FUNCTION_TRACER is disabled: > > >> (gdb) disassemble do_fork > > >> Dump of assembler code for function do_fork: > > >>0xc0037390 <+0>: mflrr0 > > >>0xc0037394 <+4>: stwur1,-64(r1) > > >>0xc0037398 <+8>: mfcrr12 > > >>0xc003739c <+12>: stmwr27,44(r1) > > >> > > >> Then I: > > >> modprobe kprobe_example func=do_fork offset=4 > > >> ls > > >> Things works well. > > >> > > >> But when CONFIG_FUNCTION_TRACER is enabled: > > >> (gdb) disassemble do_fork > > >> Dump of assembler code for function do_fork: > > >>0xc0040334 <+0>: mflrr0 > > >>0xc0040338 <+4>: stw r0,4(r1) > > >>0xc004033c <+8>: bl 0xc00109d4 > > >>0xc0040340 <+12>: stwur1,-80(r1) > > >>0xc0040344 <+16>: mflrr0 > > >>0xc0040348 <+20>: stw r0,84(r1) > > >>0xc004034c <+24>: mfcrr12 > > >> Then I: > > >> modprobe kprobe_example func=do_fork offset=12 > > >> ls > > >> 'ls' will never retrun. system freeze. My access to a 32bit powerpc box is very limited. Also, embedded powerpc has had issues with gcc-4.6 while gcc-4.5 worked fine. > > > I'm not sure if x86 had a similar issue. > > > > > > Masami, have any ideas to why this happened? > > > > No, I don't familiar with ppc implementation. I guess > > that single-step resume code failed to emulate the > > instruction, but it strongly depends on ppc arch. > > Maybe IBM people may know what happened. > > > > Ananth, Jim, would you have any ideas? > > On powerpc, we emulate sstep whenever possible. Only recently support to > emulate loads and stores got added. I don't have access to a powerpc box > today... but will try to recreate the problem ASAP and see what could be > happening in the presence of mcount. I tried to recreate this problem on a 64-bit pSeries box without success. Every one of the instructions in the stream at .do_fork are emulated and work fine there -- no hangs/crashes with or without function tracer. Yong, I am copying Kumar to see if he knows of any issues with 32-bit kprobes (he wrote it) or with the function tracer, or with the toolchain itself. You may want to check if, in the failure case, the instruction in question is single-stepped or emulated (print out the value of kprobe->ainsn.boostable in the post_handler) and see if you can find a pattern to the failure. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [BUG?]3.0-rc4+ftrace+kprobe: set kprobe at instruction 'stwu' lead to system crash/freeze
On Sun, Jun 26, 2011 at 11:47:13PM +0900, Masami Hiramatsu wrote: > (2011/06/24 19:29), Steven Rostedt wrote: > > On Fri, 2011-06-24 at 17:21 +0800, Yong Zhang wrote: > >> Hi, > >> > >> When I use kprobe to do something, I found some wired thing. > >> > >> When CONFIG_FUNCTION_TRACER is disabled: > >> (gdb) disassemble do_fork > >> Dump of assembler code for function do_fork: > >>0xc0037390 <+0>:mflrr0 > >>0xc0037394 <+4>:stwur1,-64(r1) > >>0xc0037398 <+8>:mfcrr12 > >>0xc003739c <+12>: stmwr27,44(r1) > >> > >> Then I: > >> modprobe kprobe_example func=do_fork offset=4 > >> ls > >> Things works well. > >> > >> But when CONFIG_FUNCTION_TRACER is enabled: > >> (gdb) disassemble do_fork > >> Dump of assembler code for function do_fork: > >>0xc0040334 <+0>:mflrr0 > >>0xc0040338 <+4>:stw r0,4(r1) > >>0xc004033c <+8>:bl 0xc00109d4 > >>0xc0040340 <+12>: stwur1,-80(r1) > >>0xc0040344 <+16>: mflrr0 > >>0xc0040348 <+20>: stw r0,84(r1) > >>0xc004034c <+24>: mfcrr12 > >> Then I: > >> modprobe kprobe_example func=do_fork offset=12 > >> ls > >> 'ls' will never retrun. system freeze. > > > > I'm not sure if x86 had a similar issue. > > > > Masami, have any ideas to why this happened? > > No, I don't familiar with ppc implementation. I guess > that single-step resume code failed to emulate the > instruction, but it strongly depends on ppc arch. > Maybe IBM people may know what happened. > > Ananth, Jim, would you have any ideas? On powerpc, we emulate sstep whenever possible. Only recently support to emulate loads and stores got added. I don't have access to a powerpc box today... but will try to recreate the problem ASAP and see what could be happening in the presence of mcount. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] powerpc: Emulate nop too
On Fri, May 28, 2010 at 02:23:30PM +1000, Michael Neuling wrote: > > > In message <20100528041645.gb25...@in.ibm.com> you wrote: > > On Fri, May 28, 2010 at 12:28:43PM +1000, Michael Neuling wrote: > > > > > > > > > In message <20100527141203.ga20...@in.ibm.com> you wrote: > > > > Hi Paul, > > > > > > > > While we are at it, can we also add nop to the list of emulated > > > > instructions? > > > > > > > > Ananth > > > > --- > > > > From: Ananth N Mavinakayanahalli > > > > > > > > Emulate ori 0,0,0 (nop). > > > > > > > > The long winded way is to do: > > > > > > > > case 24: > > > > rd = (instr >> 21) & 0x1f; > > > > if (rd != 0) > > > > break; > > > > rb = (instr >> 11) & 0x1f; > > > > if (rb != 0) > > > > break; > > > > > > Don't we just need rb == rd? > > > > Sure. But for this case, just checking against the opcode seems simple > > enough. > > Simple, sure. You could not emulate anything and remove the code > altogether. That would be truly simple. :-P Ah.. I misunderstood your initial point about rb == rd with imm = 0. > Why not eliminate as much as possible? > > Anyway, sounds like paulus as a better solution. Right. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
powerpc: remove resume_execution() in kprobes
On Fri, May 28, 2010 at 12:05:56PM +1000, Paul Mackerras wrote:
> On Thu, May 27, 2010 at 07:42:03PM +0530, Ananth N Mavinakayanahalli wrote:
>
> > While we are at it, can we also add nop to the list of emulated
> > instructions?
>
> I have a patch in development that emulates most of the arithmetic,
> logical and shift/rotate instructions, including ori.
OK.
> While you're here (in a virtual sense at least :), could you explain
> what's going on with the emulate_step() call in resume_execution() in
> arch/powerpc/kernel/kprobes.c? It looks like, having decided that
> emulate_step() can't handle the instruction, you single-step the
> instruction out of line and then call emulate_step again on the same
> instruction, in resume_execution(). Why on earth is it trying to
> emulate the instruction when it has already been executed at this
> point? Is there any reason why we can't just remove the emulate_step
> call from resume_execution()?
You are right. We needed emulate_step() in resume_execution() before we
had the code to handle the emulation in kprobe_handler() at the time of
the breakpoint it. At the time we needed it mainly to ensure branch
targets are reflected correctly in regs->nip if the stepped instruction
was a branch.
However, we now don't get to post_kprobe_handler() at all if
emulate_step() returned 1 at the time of the breakpoint hit. It suffices
if we just fixup the nip here. Patch below. Tested for various
instructions that can and can't be emulated...
---
From: Ananth N Mavinakayanahalli
emulate_step() in kprobe_handler() would've already determined if the
probed instruction can be emulated. We single-step in hardware only if
the instruction couldn't be emulated. resume_execution() therefore is
superfluous -- all we need is to fix up the instruction pointer after
single-stepping.
Thanks to Paul Mackerras for catching this.
Signed-off-by: Ananth N Mavinakayanahalli
---
arch/powerpc/kernel/kprobes.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
Index: linux-2.6.34/arch/powerpc/kernel/kprobes.c
===
--- linux-2.6.34.orig/arch/powerpc/kernel/kprobes.c
+++ linux-2.6.34/arch/powerpc/kernel/kprobes.c
@@ -375,17 +375,6 @@ static int __kprobes trampoline_probe_ha
* single-stepped a copy of the instruction. The address of this
* copy is p->ainsn.insn.
*/
-static void __kprobes resume_execution(struct kprobe *p, struct pt_regs *regs)
-{
- int ret;
- unsigned int insn = *p->ainsn.insn;
-
- regs->nip = (unsigned long)p->addr;
- ret = emulate_step(regs, insn);
- if (ret == 0)
- regs->nip = (unsigned long)p->addr + 4;
-}
-
static int __kprobes post_kprobe_handler(struct pt_regs *regs)
{
struct kprobe *cur = kprobe_running();
@@ -403,7 +392,8 @@ static int __kprobes post_kprobe_handler
cur->post_handler(cur, regs, 0);
}
- resume_execution(cur, regs);
+ /* Adjust nip to after the single-stepped instruction */
+ regs->nip = (unsigned long)cur->addr + 4;
regs->msr |= kcb->kprobe_saved_msr;
/*Restore back the original saved kprobes variables and continue. */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] powerpc: Emulate nop too
On Fri, May 28, 2010 at 12:28:43PM +1000, Michael Neuling wrote: > > > In message <20100527141203.ga20...@in.ibm.com> you wrote: > > Hi Paul, > > > > While we are at it, can we also add nop to the list of emulated > > instructions? > > > > Ananth > > --- > > From: Ananth N Mavinakayanahalli > > > > Emulate ori 0,0,0 (nop). > > > > The long winded way is to do: > > > > case 24: > > rd = (instr >> 21) & 0x1f; > > if (rd != 0) > > break; > > rb = (instr >> 11) & 0x1f; > > if (rb != 0) > > break; > > Don't we just need rb == rd? Sure. But for this case, just checking against the opcode seems simple enough. Ananth ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
