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:
> > On Wed, 2012-08-22 at 13:57 +0530, Ananth N Mavinakayanahalli wrote:
> > > From: Ananth N Mavinakayanahalli <ana...@in.ibm.com>
> > >
> > > 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@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 :)
> > 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 :)
> > > +#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.
> > > +#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?
> > > +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
> > > +/**
> > > + * 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.
Yeah, at least in the arch code I think we should just use u32 and
assign directly.
> > > +/* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
