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

[Oleg Nesterov <oleg@redhat.com>]

On 08/16, Ananth N Mavinakayanahalli wrote:
>
> 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 <ananth@in.ibm.com>
> > > >
> > > > 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!

Great!

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

not really, this -EEXIST (already removed by recent changes) means that
bp was already installed.

But this doesn't matter,

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

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.

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

Oleg.