Re: [PATCH v4 2/2] powerpc: Uprobes port to powerpc

On Thu, 2012-08-23 at 11:28 +0530, Ananth N Mavinakayanahalli wrote:

On Thu, Aug 23, 2012 at 02:28:20PM +1000, Michael Ellerman wrote:

quoted

On Wed, 2012-08-22 at 13:57 +0530, Ananth N Mavinakayanahalli wrote:

quoted

From: Ananth N Mavinakayanahalli <redacted>

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!

quoted

quoted

[root@xxxx ~]# ./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.

OK. Someone should put that on their TODO list, otherwise in a year or
two it'll be broken and we won't notice :)

quoted

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?

Add a new one :)

quoted

quoted

+#define MAX_UINSN_BYTES			4
+#define UPROBE_XOL_SLOT_BYTES		(MAX_UINSN_BYTES)
+
+#define UPROBE_SWBP_INSN		0x7fe00008

This is just "trap" ?

Yes. But since its referred to in arch agnostic code too, we'd have to alias
it thus.

Yep I was just checking, I think it's probably worth a comment.

quoted

quoted

+#define UPROBE_SWBP_INSN_SIZE		4 /* swbp insn size in bytes */
+
+#define IS_TW(instr)		(((instr) & 0xfc0007fe) == 0x7c000008)
+#define IS_TD(instr)		(((instr) & 0xfc0007fe) == 0x7c000088)
+#define IS_TDI(instr)		(((instr) & 0xfc000000) == 0x08000000)
+#define IS_TWI(instr)		(((instr) & 0xfc000000) == 0x0c000000)
+
+#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.

Hmm, OK. Any reason you can't include kprobes.h to get those?

quoted

quoted

+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...

OK, will argue with him :D

quoted

quoted

+/**
+ * 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.

+ */

:)

quoted

quoted

+ * @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.

Yeah, at least in the arch code I think we should just use u32 and
assign directly.

quoted

quoted

+/* callback routine for handling exceptions. */
+int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data)
+{
+	struct die_args *args = data;
+	struct pt_regs *regs = args->regs;
+
+	/* We are only interested in userspace traps */
+	if (regs && !user_mode(regs))
+		return NOTIFY_DONE;

Do we ever get here with a NULL regs?

I don't think so. Its just a paranoid check. Do you prefer it to be
removed?

Yeah. A NULL regs here is a kernel bug, so I think it's actually
preferable to crash than silently return.

cheers