[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Jean Pihet]

Add support for unwinding using the dwarf information in compat
mode. Using the correct user stack pointer allows perf to record
the frames correctly in the native and compat modes.

Note that although the dwarf frame unwinding works ok using
libunwind in native mode (on ARMv7 & ARMv8), some changes are
required to the libunwind code for the compat mode. Those changes
are posted separately on the libunwind mailing list.

Tested on ARMv8 platform with v8 and compat v7 binaries, the latter
are statically built.

Signed-off-by: Jean Pihet <jean.pihet@linaro.org>
---
 arch/arm64/include/asm/ptrace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index fbb0020..86d5b54 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -133,7 +133,7 @@ struct pt_regs {
 	(!((regs)->pstate & PSR_F_BIT))
 
 #define user_stack_pointer(regs) \
-	((regs)->sp)
+	(!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
 
 /*
  * Are the current registers suitable for user mode? (used to maintain
-- 
1.7.11.7

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Will Deacon]

Hi Jean,

On Thu, Jan 16, 2014 at 10:45:23AM +0000, Jean Pihet wrote:
> Add support for unwinding using the dwarf information in compat
> mode. Using the correct user stack pointer allows perf to record
> the frames correctly in the native and compat modes.
> 
> Note that although the dwarf frame unwinding works ok using
> libunwind in native mode (on ARMv7 & ARMv8), some changes are
> required to the libunwind code for the compat mode. Those changes
> are posted separately on the libunwind mailing list.
> 
> Tested on ARMv8 platform with v8 and compat v7 binaries, the latter
> are statically built.

I guess it makes sense to include this with your earlier series adding
support for compat backtracing?

> Signed-off-by: Jean Pihet <jean.pihet@linaro.org>
> ---
>  arch/arm64/include/asm/ptrace.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index fbb0020..86d5b54 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -133,7 +133,7 @@ struct pt_regs {
>  	(!((regs)->pstate & PSR_F_BIT))
>  
>  #define user_stack_pointer(regs) \
> -	((regs)->sp)
> +	(!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)

In your previous series, compat backtracing is actually split out into a
separate function (compat_user_backtrace), so it would be more consistent to
have a compat_user_stack_pointer macro, rather than add this check here.

Will

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Jean Pihet]

On 16 January 2014 12:56, Will Deacon <will.deacon@arm.com> wrote:
> Hi Jean,
>
> On Thu, Jan 16, 2014 at 10:45:23AM +0000, Jean Pihet wrote:
>> Add support for unwinding using the dwarf information in compat
>> mode. Using the correct user stack pointer allows perf to record
>> the frames correctly in the native and compat modes.
>>
>> Note that although the dwarf frame unwinding works ok using
>> libunwind in native mode (on ARMv7 & ARMv8), some changes are
>> required to the libunwind code for the compat mode. Those changes
>> are posted separately on the libunwind mailing list.
>>
>> Tested on ARMv8 platform with v8 and compat v7 binaries, the latter
>> are statically built.
>
> I guess it makes sense to include this with your earlier series adding
> support for compat backtracing?
>
>> Signed-off-by: Jean Pihet <jean.pihet@linaro.org>
>> ---
>>  arch/arm64/include/asm/ptrace.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index fbb0020..86d5b54 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -133,7 +133,7 @@ struct pt_regs {
>>       (!((regs)->pstate & PSR_F_BIT))
>>
>>  #define user_stack_pointer(regs) \
>> -     ((regs)->sp)
>> +     (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
>
> In your previous series, compat backtracing is actually split out into a
> separate function (compat_user_backtrace), so it would be more consistent to
> have a compat_user_stack_pointer macro, rather than add this check here.

Do you mean this change instead?

diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 569b2187..9b88d2e 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void)
        return true;
 }

-#define perf_user_stack_pointer(regs) user_stack_pointer(regs)
+#define perf_user_stack_pointer(regs) \
+       (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
 #else
 static inline bool arch_perf_have_user_stack_dump(void)
 {

If so let me prepare/test and re-submit this.

Thx!
Jean

>
> Will

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Will Deacon]

On Thu, Jan 16, 2014 at 12:26:53PM +0000, Jean Pihet wrote:
> On 16 January 2014 12:56, Will Deacon <will.deacon@arm.com> wrote:
> > In your previous series, compat backtracing is actually split out into a
> > separate function (compat_user_backtrace), so it would be more consistent to
> > have a compat_user_stack_pointer macro, rather than add this check here.
> 
> Do you mean this change instead?

I don't think so...

> diff --git a/kernel/events/internal.h b/kernel/events/internal.h
> index 569b2187..9b88d2e 100644
> --- a/kernel/events/internal.h
> +++ b/kernel/events/internal.h
> @@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void)
>         return true;
>  }
> 
> -#define perf_user_stack_pointer(regs) user_stack_pointer(regs)
> +#define perf_user_stack_pointer(regs) \
> +       (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)

This doesn't belong in core code; compat_user_mode and the fields of regs
are arm64-specific. So I suppose you need to rework your original patch to
call compat_user_stack_pointer (which we already define in compat.h for
arm64) if compat_user_mode(regs)).

The problem there is the inconsistency with respect to the regs argument:

  user_stack_pointer(regs)	// Returns user stack pointer for regs
  current_user_stack_pointer()	// Returns current user stack pointer
  compat_user_stack_pointer()	// Doesn't take a regs argument!

On top of that, x86 treats those last two functions differently when current
is a compat task.

So the simplest thing would be to make compat_user_stack_pointer expand to
user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your
original patch fixing user_stack_pointer.

Will

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Jean Pihet]

Will,

On 16 January 2014 13:57, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jan 16, 2014 at 12:26:53PM +0000, Jean Pihet wrote:
>> On 16 January 2014 12:56, Will Deacon <will.deacon@arm.com> wrote:
>> > In your previous series, compat backtracing is actually split out into a
>> > separate function (compat_user_backtrace), so it would be more consistent to
>> > have a compat_user_stack_pointer macro, rather than add this check here.
The compat_user_backtrace function is used to unwind using the frame
pointer, it is not used to unwind using the dwarf info (which uses the
user stack pointer).

>>
>> Do you mean this change instead?
>
> I don't think so...
>
>> diff --git a/kernel/events/internal.h b/kernel/events/internal.h
>> index 569b2187..9b88d2e 100644
>> --- a/kernel/events/internal.h
>> +++ b/kernel/events/internal.h
>> @@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void)
>>         return true;
>>  }
>>
>> -#define perf_user_stack_pointer(regs) user_stack_pointer(regs)
>> +#define perf_user_stack_pointer(regs) \
>> +       (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
>
> This doesn't belong in core code; compat_user_mode and the fields of regs
> are arm64-specific.
Right.

> So I suppose you need to rework your original patch to
> call compat_user_stack_pointer (which we already define in compat.h for
> arm64) if compat_user_mode(regs)).
The perf core code calls perf_user_stack_pointer(regs) to retrieve the
stack pointer, with perf_user_stack_pointer(regs) defined as
user_stack_pointer(regs).
The problem is that perf is not aware of the compat mode, so every
arch has to implement user_stack_pointer(regs) correctly.

For this reason I think the first patch proposal is the right one
unless the perf core code is redesigned to handle different ABIs. Do
you see a better implementation?

>
> The problem there is the inconsistency with respect to the regs argument:
>
>   user_stack_pointer(regs)      // Returns user stack pointer for regs
>   current_user_stack_pointer()  // Returns current user stack pointer
>   compat_user_stack_pointer()   // Doesn't take a regs argument!
>
> On top of that, x86 treats those last two functions differently when current
> is a compat task.
>
> So the simplest thing would be to make compat_user_stack_pointer expand to
> user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your
> original patch fixing user_stack_pointer.
>
> Will

Thx!
Jean

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Jean Pihet]

Hi Will,

Some more thoughts below

On 16 January 2014 14:47, Jean Pihet <jean.pihet@linaro.org> wrote:
> Will,
>
> On 16 January 2014 13:57, Will Deacon <will.deacon@arm.com> wrote:
>> On Thu, Jan 16, 2014 at 12:26:53PM +0000, Jean Pihet wrote:
>>> On 16 January 2014 12:56, Will Deacon <will.deacon@arm.com> wrote:
>>> > In your previous series, compat backtracing is actually split out into a
>>> > separate function (compat_user_backtrace), so it would be more consistent to
>>> > have a compat_user_stack_pointer macro, rather than add this check here.
> The compat_user_backtrace function is used to unwind using the frame
> pointer, it is not used to unwind using the dwarf info (which uses the
> user stack pointer).
>
>>>
>>> Do you mean this change instead?
>>
>> I don't think so...
>>
>>> diff --git a/kernel/events/internal.h b/kernel/events/internal.h
>>> index 569b2187..9b88d2e 100644
>>> --- a/kernel/events/internal.h
>>> +++ b/kernel/events/internal.h
>>> @@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void)
>>>         return true;
>>>  }
>>>
>>> -#define perf_user_stack_pointer(regs) user_stack_pointer(regs)
>>> +#define perf_user_stack_pointer(regs) \
>>> +       (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
>>
>> This doesn't belong in core code; compat_user_mode and the fields of regs
>> are arm64-specific.
> Right.
>
>> So I suppose you need to rework your original patch to
>> call compat_user_stack_pointer (which we already define in compat.h for
>> arm64) if compat_user_mode(regs)).
> The perf core code calls perf_user_stack_pointer(regs) to retrieve the
> stack pointer, with perf_user_stack_pointer(regs) defined as
> user_stack_pointer(regs).
> The problem is that perf is not aware of the compat mode, so every
> arch has to implement user_stack_pointer(regs) correctly.
>
> For this reason I think the first patch proposal is the right one
> unless the perf core code is redesigned to handle different ABIs. Do
> you see a better implementation?
>
>>
>> The problem there is the inconsistency with respect to the regs argument:
>>
>>   user_stack_pointer(regs)      // Returns user stack pointer for regs
>>   current_user_stack_pointer()  // Returns current user stack pointer
>>   compat_user_stack_pointer()   // Doesn't take a regs argument!
>>
>> On top of that, x86 treats those last two functions differently when current
>> is a compat task.
>>
>> So the simplest thing would be to make compat_user_stack_pointer expand to
>> user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your
>> original patch fixing user_stack_pointer.

I see 2 issues in your proposal:

1) user_stack_pointer(regs) calls compat_user_stack_pointer if
compat_user_mode(regs)) and compat_user_stack_pointer expands to
user_stack_pointer. I see a circular dependency in the macros.

2) current_pt_regs() returns the current task regs although perf
passes a regs struct that had been recorded previously.

Am I missing something?

Thx,
Jean

>>
>> Will
>
> Thx!
> Jean

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Will Deacon]

On Fri, Jan 17, 2014 at 09:00:09AM +0000, Jean Pihet wrote:
> On 16 January 2014 14:47, Jean Pihet <jean.pihet@linaro.org> wrote:
> >> So the simplest thing would be to make compat_user_stack_pointer expand to
> >> user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your
> >> original patch fixing user_stack_pointer.
> 
> I see 2 issues in your proposal:
> 
> 1) user_stack_pointer(regs) calls compat_user_stack_pointer if
> compat_user_mode(regs)) and compat_user_stack_pointer expands to
> user_stack_pointer. I see a circular dependency in the macros.

Not today it doesn't, so you just need to avoid writing the circular
dependency and instead make user_stack_pointer access (regs)->compat_sp
instead.

> 2) current_pt_regs() returns the current task regs although perf
> passes a regs struct that had been recorded previously.

Yes, but compat_user_stack_pointer doesn't take a regs paramater anyway, so
there's no change in behaviour here.

Will

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Jean Pihet]

Hi Will,

Here is an updated version of the change, which uses compat_sp at only
one place.
The drawback is that compat_user_mode is checked when calling
compat_user_stack_pointer, which seems unnecessary. Unfortunately the
check is not optimized out by the complier as I could check with
objdump -S.

What do you think?

diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
index fda2704..e71f81f 100644
--- a/arch/arm64/include/asm/compat.h
+++ b/arch/arm64/include/asm/compat.h
@@ -228,7 +228,7 @@ static inline compat_uptr_t ptr_to_compat(void __user *uptr)
        return (u32)(unsigned long)uptr;
 }

-#define compat_user_stack_pointer() (current_pt_regs()->compat_sp)
+#define compat_user_stack_pointer() (user_stack_pointer(current_pt_regs()))

 static inline void __user *arch_compat_alloc_user_space(long len)
 {
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index fbb0020..86d5b54 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -133,7 +133,7 @@ struct pt_regs {
        (!((regs)->pstate & PSR_F_BIT))

 #define user_stack_pointer(regs) \
-       ((regs)->sp)
+       (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)

 /*
  * Are the current registers suitable for user mode? (used to maintain



Regards,
Jean

On 17 January 2014 11:07, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jan 17, 2014 at 09:00:09AM +0000, Jean Pihet wrote:
>> On 16 January 2014 14:47, Jean Pihet <jean.pihet@linaro.org> wrote:
>> >> So the simplest thing would be to make compat_user_stack_pointer expand to
>> >> user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your
>> >> original patch fixing user_stack_pointer.
>>
>> I see 2 issues in your proposal:
>>
>> 1) user_stack_pointer(regs) calls compat_user_stack_pointer if
>> compat_user_mode(regs)) and compat_user_stack_pointer expands to
>> user_stack_pointer. I see a circular dependency in the macros.
>
> Not today it doesn't, so you just need to avoid writing the circular
> dependency and instead make user_stack_pointer access (regs)->compat_sp
> instead.
>
>> 2) current_pt_regs() returns the current task regs although perf
>> passes a regs struct that had been recorded previously.
>
> Yes, but compat_user_stack_pointer doesn't take a regs paramater anyway, so
> there's no change in behaviour here.
>
> Will

[PATCH] ARM64: perf: support dwarf unwinding in compat mode

[Will Deacon]

On Mon, Jan 20, 2014 at 05:05:14PM +0000, Jean Pihet wrote:
> Hi Will,
> 
> Here is an updated version of the change, which uses compat_sp at only
> one place.
> The drawback is that compat_user_mode is checked when calling
> compat_user_stack_pointer, which seems unnecessary. Unfortunately the
> check is not optimized out by the complier as I could check with
> objdump -S.
> 
> What do you think?

I think that's cleaner and really wouldn't worry about the couple of extra
instructions.

Cheers,

Will

> diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> index fda2704..e71f81f 100644
> --- a/arch/arm64/include/asm/compat.h
> +++ b/arch/arm64/include/asm/compat.h
> @@ -228,7 +228,7 @@ static inline compat_uptr_t ptr_to_compat(void __user *uptr)
>         return (u32)(unsigned long)uptr;
>  }
> 
> -#define compat_user_stack_pointer() (current_pt_regs()->compat_sp)
> +#define compat_user_stack_pointer() (user_stack_pointer(current_pt_regs()))
> 
>  static inline void __user *arch_compat_alloc_user_space(long len)
>  {
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index fbb0020..86d5b54 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -133,7 +133,7 @@ struct pt_regs {
>         (!((regs)->pstate & PSR_F_BIT))
> 
>  #define user_stack_pointer(regs) \
> -       ((regs)->sp)
> +       (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
> 
>  /*
>   * Are the current registers suitable for user mode? (used to maintain