Re: [RFC][PATCH 10/11] perf, x86: use LBR for PEBS IP+1 fixup

Peter Zijlstra
2010-03-03T19:37:18Z

On Wed, 2010-03-03 at 13:05 -0500, Masami Hiramatsu wrote:
> Peter Zijlstra wrote:
> > PEBS always reports the IP+1, that is the instruction after the one
> > that got sampled, cure this by using the LBR to reliably rewind the
> > instruction stream.
>
> Hmm, does PEBS always report one byte after the end address of the
> sampled instruction? Or the instruction which will be executed next
> step?

The next instruction, its trap like.

> [...]
> > +#include <asm/insn.h>
> > +
> > +#define MAX_INSN_SIZE 16
>
> Hmm, we'd better integrate these kinds of definitions into
> asm/insn.h... (several features define it)

Agreed, I'll look at doing a patch to collect them all into asm/insn.h
if nobody beats me to it :-)

> > +
> > +static void intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
> > +{
> > +#if 0
> > + /*
> > + * Borken, makes the machine expode at times trying to
> > + * derefence funny userspace addresses.
> > + *
> > + * Should we always fwd decode from @to, instead of trying
> > + * to rewind as implemented?
> > + */
> > +
> > + struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> > + unsigned long from = cpuc->lbr_entries[0].from;
> > + unsigned long to = cpuc->lbr_entries[0].to;
>
> Ah, I see. For branch instruction case, we can use LBR to
> find previous IP...

Right, we use the LBR to find the basic block.

> > + unsigned long ip = regs->ip;
> > + u8 buf[2*MAX_INSN_SIZE];
> > + u8 *kaddr;
> > + int i;
> > +
> > + if (from && to) {
> > + /*
> > + * We sampled a branch insn, rewind using the LBR stack
> > + */
> > + if (ip == to) {
> > + regs->ip = from;
> > + return;
> > + }
> > + }
> > +
> > + if (user_mode(regs)) {
> > + int bytes = copy_from_user_nmi(buf,
> > + (void __user *)(ip - MAX_INSN_SIZE),
> > + 2*MAX_INSN_SIZE);
> > +
>
> maybe, you'd better check the source address range is within
> the user address range. e.g. ip < MAX_INSN_SIZE.

Not only that, I realized user_mode() checks regs->cs, which is not set
by the PEBS code, so I added some helpers.

> > +
> > + /*
> > + * Try to find the longest insn ending up at the given IP
> > + */
> > + for (i = MAX_INSN_SIZE; i > 0; i--) {
> > + struct insn insn;
> > +
> > + kernel_insn_init(&insn, kaddr + MAX_INSN_SIZE - i);
> > + insn_get_length(&insn);
> > + if (insn.length == i) {
> > + regs->ip -= i;
> > + return;
> > + }
> > + }
>
> Hmm, this will not work correctly on x86, since the decoder can
> miss-decode the tail bytes of previous instruction as prefix bytes. :(
>
> Thus, if you want to rewind instruction stream, you need to decode
> a function (or basic block) entirely.

Something like the below?

#ifdef CONFIG_X86_32
static bool kernel_ip(unsigned long ip)
{
return ip > TASK_SIZE;
}
#else
static bool kernel_ip(unsigned long ip)
{
return (long)ip < 0;
}
#endif

static int intel_pmu_pebs_fixup_ip(unsigned long *ipp)
{
struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
unsigned long from = cpuc->lbr_entries[0].from;
unsigned long old_to, to = cpuc->lbr_entries[0].to;
unsigned long ip = *ipp;
int i;

/*
* We don't need to fixup if the PEBS assist is fault like
*/
if (!x86_pmu.intel_perf_capabilities.pebs_trap)
return 0;

if (!cpuc->lbr_stack.nr || !from || !to)
return 0;

if (ip < to)
return 0;

/*
* We sampled a branch insn, rewind using the LBR stack
*/
if (ip == to) {
*ipp = from;
return 1;
}

do {
struct insn insn;
u8 buf[MAX_INSN_SIZE];
void *kaddr;

old_to = to;
if (!kernel_ip(ip)) {
int bytes = copy_from_user_nmi(buf, (void __user *)to,
MAX_INSN_SIZE);

if (bytes != MAX_INSN_SIZE)
return 0;

kaddr = buf;
} else kaddr = (void *)to;

kernel_insn_init(&insn, kaddr);
insn_get_length(&insn);
to += insn.length;
} while (to < ip);

if (to == ip) {
*ipp = old_to;
return 1;
}

return 0;
}

I thought about exposing the success of this fixup as a PERF_RECORD_MISC
bit.