Re: [PATCH 03/23] perf: Add ability to attach user level registers dump to sample

[Frederic Weisbecker <fweisbec@gmail.com>]

On Fri, Jun 29, 2012 at 09:50:43AM +0200, Jiri Olsa wrote:
> On Wed, Jun 27, 2012 at 05:30:05PM +0200, Frederic Weisbecker wrote:
> > On Wed, Jun 27, 2012 at 05:25:29PM +0200, Peter Zijlstra wrote:
> > > On Wed, 2012-06-27 at 17:24 +0200, Frederic Weisbecker wrote:
> > > > On Wed, Jun 27, 2012 at 05:13:44PM +0200, Stephane Eranian wrote:
> > > > > On Wed, Jun 27, 2012 at 5:11 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > > > > On Tue, Jun 19, 2012 at 05:47:54PM +0200, Jiri Olsa wrote:
> > > > > >> @@ -4061,6 +4105,19 @@ void perf_prepare_sample(struct perf_event_header *header,
> > > > > >>               }
> > > > > >>               header->size += size;
> > > > > >>       }
> > > > > >> +
> > > > > >> +     if (sample_type & PERF_SAMPLE_REGS_USER) {
> > > > > >> +             /* regs dump available bool */
> > > > > >> +             int size = sizeof(u64);
> > > > > >> +
> > > > > >> +             data->regs_user = perf_sample_regs_user(regs);
> > > > > >> +             if (data->regs_user) {
> > > > > >> +                     u64 mask = event->attr.sample_regs_user;
> > > > > >> +                     size += hweight64(mask) * sizeof(u64);
> > > > > >> +             }
> > > > > >> +
> > > > > >> +             header->size += size;
> > > > > >
> > > > > > We'll need to remove the 64 bits registers for compat tasks but other than
> > > > > > that, it looks ok.
> > > > > You cannot do this. You cannot remove register values from under the hood.
> > > > > The only way the user has to parse the sample is the sample_regs_users bitmask.
> > > > > You have to return 0 for those unexisting regs for compat tasks.
> > > > 
> > > > You mean fill unexisting reg values with 0? Yeah that works.
> > > 
> > > What does x32 look like? Is that still reported as a compat task? If so,
> > > we should record all registers and not 0 out anything.
> > 
> > To make it simple, we could always use the x86-64 registers mask interface for everyone:
> > native 32 bits kernel, compat 32 bits task, 64 bits everything. And we can't fill an x86-64
> > value because we are dealing with a native/compat 32 task we just fill out the requested value
> > with 0.
> > 
> > On post processing time, userspace can know if it's dealing with 32 bits task or not anyway, so
> > it knows what to skip and what is relevant.
> 
> well, thats basically what we have now..
> 
> when the kernel is compiled for 32 bits, the bitmask allows to store:
> 
> ax, bx, cx, dx, si, di, bp, sp, ip, flags, cs, ds, es, fs, gs, ss
> 
> and when  kernel is compiled for 64 bits, the bitmask adds 64bit stuff:
> 
> ax, bx, cx, dx, si, di, bp, sp, ip, flags, cs, ds, es, fs, gs, ss
> r8, r9, r10, r11, r12, r13, r14, r15
> 
> 
> - 32 bits kernel is straightforward
> 
> - for 64 bits kernel we store whatever bitmask instructs to,
>   regardless if we are in compat task or native 64,
>   user space will deal with that in post processing

I can think of the crazy scenario where perf itself is compat and there
are 64 bits apps that will be profiled by perf. It means perf must check
if the kernel is 32 or 64 and depending on this, request r8-r15 or not.

May be that's just too unlikely to matter.