[PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

Modified the patch subject and the change description.

PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
that perf can not parser the backtrace of app with dwarf mode in the 
32bit system and 64bit kernel.

Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
---
 arch/arm64/kernel/perf_regs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
index 0bbac61..0ef2880 100644
--- a/arch/arm64/kernel/perf_regs.c
+++ b/arch/arm64/kernel/perf_regs.c
@@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 	if ((u32)idx == PERF_REG_ARM64_PC)
 		return regs->pc;
 
+	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
+		&& idx == 15)
+		return regs->pc;
+
 	return regs->regs[idx];
 }
 
-- 
1.9.1

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

Hi, Will

Please help to review the change.

Thanks,
Jiping

On 05/11/2020 10:52 AM, Jiping Ma wrote:
> Modified the patch subject and the change description.
>
> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> that perf can not parser the backtrace of app with dwarf mode in the
> 32bit system and 64bit kernel.
>
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> ---
>   arch/arm64/kernel/perf_regs.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> index 0bbac61..0ef2880 100644
> --- a/arch/arm64/kernel/perf_regs.c
> +++ b/arch/arm64/kernel/perf_regs.c
> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>   	if ((u32)idx == PERF_REG_ARM64_PC)
>   		return regs->pc;
>   
> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> +		&& idx == 15)
> +		return regs->pc;
> +
>   	return regs->regs[idx];
>   }
>   


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

Hi, Will

Please help to review the change.

Thanks,
Jiping

On 05/11/2020 10:52 AM, Jiping Ma wrote:
> Modified the patch subject and the change description.
>
> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> that perf can not parser the backtrace of app with dwarf mode in the
> 32bit system and 64bit kernel.
>
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> ---
>   arch/arm64/kernel/perf_regs.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> index 0bbac61..0ef2880 100644
> --- a/arch/arm64/kernel/perf_regs.c
> +++ b/arch/arm64/kernel/perf_regs.c
> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>   	if ((u32)idx == PERF_REG_ARM64_PC)
>   		return regs->pc;
>   
> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> +		&& idx == 15)
> +		return regs->pc;
> +
>   	return regs->regs[idx];
>   }
>   

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> Modified the patch subject and the change description.
> 
> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> that perf can not parser the backtrace of app with dwarf mode in the 
> 32bit system and 64bit kernel.
> 
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>

Thanks for this.


> ---
>  arch/arm64/kernel/perf_regs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> index 0bbac61..0ef2880 100644
> --- a/arch/arm64/kernel/perf_regs.c
> +++ b/arch/arm64/kernel/perf_regs.c
> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  	if ((u32)idx == PERF_REG_ARM64_PC)
>  		return regs->pc;
>  
> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> +		&& idx == 15)
> +		return regs->pc;

I think there are some more issues here, and we may need a more
substantial rework. For a compat thread, we always expose
PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
perf_reg_value() also munges the compat SP/LR into their ARM64
equivalents, which don't exist in the 32-bit sample ABI. We also don't
zero the regs that don't exist in 32-bit (including the aliasing PC).

I reckon what we should do is have seperate functions for the two ABIs,
to ensure we don't conflate them, e.g.

u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
{
	if ((u32)idx > PERF_REG_ARM32_PC)
		return 0;
	if (idx == PERF_REG_ARM32_PC)
		return regs->pc;
	
	/*
	 * Compat SP and LR already in-place
	 */
	return regs->regs[idx];
}

u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
{
	if ((u32)idx > PERF_REG_ARM64_MAX)
		return 0;
	if ((u32)idx == PERF_REG_ARM64_SP)
		return regs->sp;
	if ((u32)idx == PERF_REG_ARM64_PC)
		return regs->pc;
	
	reutrn regs->regs[idx];
}

u64 perf_reg_value(struct pt_regs *regs, int idx)
{
	if (compat_user_mode(regs))
		return perf_reg_value_abi32(regs, idx);
	else
		return perf_reg_value_abi64(regs, idx);
}

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> Modified the patch subject and the change description.
> 
> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> that perf can not parser the backtrace of app with dwarf mode in the 
> 32bit system and 64bit kernel.
> 
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>

Thanks for this.


> ---
>  arch/arm64/kernel/perf_regs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> index 0bbac61..0ef2880 100644
> --- a/arch/arm64/kernel/perf_regs.c
> +++ b/arch/arm64/kernel/perf_regs.c
> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  	if ((u32)idx == PERF_REG_ARM64_PC)
>  		return regs->pc;
>  
> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> +		&& idx == 15)
> +		return regs->pc;

I think there are some more issues here, and we may need a more
substantial rework. For a compat thread, we always expose
PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
perf_reg_value() also munges the compat SP/LR into their ARM64
equivalents, which don't exist in the 32-bit sample ABI. We also don't
zero the regs that don't exist in 32-bit (including the aliasing PC).

I reckon what we should do is have seperate functions for the two ABIs,
to ensure we don't conflate them, e.g.

u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
{
	if ((u32)idx > PERF_REG_ARM32_PC)
		return 0;
	if (idx == PERF_REG_ARM32_PC)
		return regs->pc;
	
	/*
	 * Compat SP and LR already in-place
	 */
	return regs->regs[idx];
}

u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
{
	if ((u32)idx > PERF_REG_ARM64_MAX)
		return 0;
	if ((u32)idx == PERF_REG_ARM64_SP)
		return regs->sp;
	if ((u32)idx == PERF_REG_ARM64_PC)
		return regs->pc;
	
	reutrn regs->regs[idx];
}

u64 perf_reg_value(struct pt_regs *regs, int idx)
{
	if (compat_user_mode(regs))
		return perf_reg_value_abi32(regs, idx);
	else
		return perf_reg_value_abi64(regs, idx);
}

Thanks,
Mark.

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Tue, May 26, 2020 at 11:26:11AM +0100, Mark Rutland wrote:
> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > Modified the patch subject and the change description.
> > 
> > PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> > is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> > that perf can not parser the backtrace of app with dwarf mode in the 
> > 32bit system and 64bit kernel.
> > 
> > Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> 
> Thanks for this.
> 
> 
> > ---
> >  arch/arm64/kernel/perf_regs.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> > index 0bbac61..0ef2880 100644
> > --- a/arch/arm64/kernel/perf_regs.c
> > +++ b/arch/arm64/kernel/perf_regs.c
> > @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> >  	if ((u32)idx == PERF_REG_ARM64_PC)
> >  		return regs->pc;
> >  
> > +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> > +		&& idx == 15)
> > +		return regs->pc;
> 
> I think there are some more issues here, and we may need a more
> substantial rework. For a compat thread, we always expose
> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> perf_reg_value() also munges the compat SP/LR into their ARM64
> equivalents, which don't exist in the 32-bit sample ABI. We also don't
> zero the regs that don't exist in 32-bit (including the aliasing PC).

I think this was for the case where you have a 64-bit perf profiling a
32-bit task, and it was passing the registers off to libunwind. Won't that
break if we follow your suggestion?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Tue, May 26, 2020 at 11:26:11AM +0100, Mark Rutland wrote:
> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > Modified the patch subject and the change description.
> > 
> > PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> > is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> > that perf can not parser the backtrace of app with dwarf mode in the 
> > 32bit system and 64bit kernel.
> > 
> > Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> 
> Thanks for this.
> 
> 
> > ---
> >  arch/arm64/kernel/perf_regs.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> > index 0bbac61..0ef2880 100644
> > --- a/arch/arm64/kernel/perf_regs.c
> > +++ b/arch/arm64/kernel/perf_regs.c
> > @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> >  	if ((u32)idx == PERF_REG_ARM64_PC)
> >  		return regs->pc;
> >  
> > +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> > +		&& idx == 15)
> > +		return regs->pc;
> 
> I think there are some more issues here, and we may need a more
> substantial rework. For a compat thread, we always expose
> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> perf_reg_value() also munges the compat SP/LR into their ARM64
> equivalents, which don't exist in the 32-bit sample ABI. We also don't
> zero the regs that don't exist in 32-bit (including the aliasing PC).

I think this was for the case where you have a 64-bit perf profiling a
32-bit task, and it was passing the registers off to libunwind. Won't that
break if we follow your suggestion?

Will

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/27/2020 03:54 AM, Will Deacon wrote:
> On Tue, May 26, 2020 at 11:26:11AM +0100, Mark Rutland wrote:
>> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>>> Modified the patch subject and the change description.
>>>
>>> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
>>> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
>>> that perf can not parser the backtrace of app with dwarf mode in the
>>> 32bit system and 64bit kernel.
>>>
>>> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
>> Thanks for this.
>>
>>
>>> ---
>>>   arch/arm64/kernel/perf_regs.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
>>> index 0bbac61..0ef2880 100644
>>> --- a/arch/arm64/kernel/perf_regs.c
>>> +++ b/arch/arm64/kernel/perf_regs.c
>>> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>>>   	if ((u32)idx == PERF_REG_ARM64_PC)
>>>   		return regs->pc;
>>>   
>>> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
>>> +		&& idx == 15)
>>> +		return regs->pc;
>> I think there are some more issues here, and we may need a more
>> substantial rework. For a compat thread, we always expose
>> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
>> perf_reg_value() also munges the compat SP/LR into their ARM64
>> equivalents, which don't exist in the 32-bit sample ABI. We also don't
>> zero the regs that don't exist in 32-bit (including the aliasing PC).
> I think this was for the case where you have a 64-bit perf profiling a
> 32-bit task, and it was passing the registers off to libunwind. Won't that
> break if we follow your suggestion?
Yes, it is for 64-bit perf profiling a 32-bit task, not for a compat thread.

>
> Will
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/27/2020 03:54 AM, Will Deacon wrote:
> On Tue, May 26, 2020 at 11:26:11AM +0100, Mark Rutland wrote:
>> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>>> Modified the patch subject and the change description.
>>>
>>> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
>>> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
>>> that perf can not parser the backtrace of app with dwarf mode in the
>>> 32bit system and 64bit kernel.
>>>
>>> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
>> Thanks for this.
>>
>>
>>> ---
>>>   arch/arm64/kernel/perf_regs.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
>>> index 0bbac61..0ef2880 100644
>>> --- a/arch/arm64/kernel/perf_regs.c
>>> +++ b/arch/arm64/kernel/perf_regs.c
>>> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>>>   	if ((u32)idx == PERF_REG_ARM64_PC)
>>>   		return regs->pc;
>>>   
>>> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
>>> +		&& idx == 15)
>>> +		return regs->pc;
>> I think there are some more issues here, and we may need a more
>> substantial rework. For a compat thread, we always expose
>> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
>> perf_reg_value() also munges the compat SP/LR into their ARM64
>> equivalents, which don't exist in the 32-bit sample ABI. We also don't
>> zero the regs that don't exist in 32-bit (including the aliasing PC).
> I think this was for the case where you have a 64-bit perf profiling a
> 32-bit task, and it was passing the registers off to libunwind. Won't that
> break if we follow your suggestion?
Yes, it is for 64-bit perf profiling a 32-bit task, not for a compat thread.

>
> Will
>

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Tue, May 26, 2020 at 08:54:19PM +0100, Will Deacon wrote:
> On Tue, May 26, 2020 at 11:26:11AM +0100, Mark Rutland wrote:
> > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > Modified the patch subject and the change description.
> > > 
> > > PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> > > is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> > > that perf can not parser the backtrace of app with dwarf mode in the 
> > > 32bit system and 64bit kernel.
> > > 
> > > Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> > 
> > Thanks for this.
> > 
> > 
> > > ---
> > >  arch/arm64/kernel/perf_regs.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> > > index 0bbac61..0ef2880 100644
> > > --- a/arch/arm64/kernel/perf_regs.c
> > > +++ b/arch/arm64/kernel/perf_regs.c
> > > @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> > >  	if ((u32)idx == PERF_REG_ARM64_PC)
> > >  		return regs->pc;
> > >  
> > > +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> > > +		&& idx == 15)
> > > +		return regs->pc;
> > 
> > I think there are some more issues here, and we may need a more
> > substantial rework. For a compat thread, we always expose
> > PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> > perf_reg_value() also munges the compat SP/LR into their ARM64
> > equivalents, which don't exist in the 32-bit sample ABI. We also don't
> > zero the regs that don't exist in 32-bit (including the aliasing PC).
> 
> I think this was for the case where you have a 64-bit perf profiling a
> 32-bit task, and it was passing the registers off to libunwind. Won't that
> break if we follow your suggestion?

Oh yuck; have we messed up the ABI here, or have I misunderstood?

Is arm64's PERF_SAMPLE_REGS_ABI_32 supposed to be the same as the 32-bit
arm's PERF_SAMPLE_REGS_ABI_32?

If yes, and the differences are being relied upon by 64-bit consumers,
that's a nasty ABI issue we've introduced for compat tasks, and I don't
think this patch alone is quite right.

If no, then I don't see that any change is necessary, as we already
expose the information, and it's a userspace bug to expect the PC in a
place where the kernel has never exposed it.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Tue, May 26, 2020 at 08:54:19PM +0100, Will Deacon wrote:
> On Tue, May 26, 2020 at 11:26:11AM +0100, Mark Rutland wrote:
> > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > Modified the patch subject and the change description.
> > > 
> > > PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> > > is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> > > that perf can not parser the backtrace of app with dwarf mode in the 
> > > 32bit system and 64bit kernel.
> > > 
> > > Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> > 
> > Thanks for this.
> > 
> > 
> > > ---
> > >  arch/arm64/kernel/perf_regs.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> > > index 0bbac61..0ef2880 100644
> > > --- a/arch/arm64/kernel/perf_regs.c
> > > +++ b/arch/arm64/kernel/perf_regs.c
> > > @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> > >  	if ((u32)idx == PERF_REG_ARM64_PC)
> > >  		return regs->pc;
> > >  
> > > +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> > > +		&& idx == 15)
> > > +		return regs->pc;
> > 
> > I think there are some more issues here, and we may need a more
> > substantial rework. For a compat thread, we always expose
> > PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> > perf_reg_value() also munges the compat SP/LR into their ARM64
> > equivalents, which don't exist in the 32-bit sample ABI. We also don't
> > zero the regs that don't exist in 32-bit (including the aliasing PC).
> 
> I think this was for the case where you have a 64-bit perf profiling a
> 32-bit task, and it was passing the registers off to libunwind. Won't that
> break if we follow your suggestion?

Oh yuck; have we messed up the ABI here, or have I misunderstood?

Is arm64's PERF_SAMPLE_REGS_ABI_32 supposed to be the same as the 32-bit
arm's PERF_SAMPLE_REGS_ABI_32?

If yes, and the differences are being relied upon by 64-bit consumers,
that's a nasty ABI issue we've introduced for compat tasks, and I don't
think this patch alone is quite right.

If no, then I don't see that any change is necessary, as we already
expose the information, and it's a userspace bug to expect the PC in a
place where the kernel has never exposed it.

Thanks,
Mark.

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/26/2020 06:26 PM, Mark Rutland wrote:
> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>> Modified the patch subject and the change description.
>>
>> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
>> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
>> that perf can not parser the backtrace of app with dwarf mode in the
>> 32bit system and 64bit kernel.
>>
>> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> Thanks for this.
>
>
>> ---
>>   arch/arm64/kernel/perf_regs.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
>> index 0bbac61..0ef2880 100644
>> --- a/arch/arm64/kernel/perf_regs.c
>> +++ b/arch/arm64/kernel/perf_regs.c
>> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>>   	if ((u32)idx == PERF_REG_ARM64_PC)
>>   		return regs->pc;
>>   
>> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
>> +		&& idx == 15)
>> +		return regs->pc;
> I think there are some more issues here, and we may need a more
> substantial rework. For a compat thread, we always expose
> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> perf_reg_value() also munges the compat SP/LR into their ARM64
> equivalents, which don't exist in the 32-bit sample ABI. We also don't
> zero the regs that don't exist in 32-bit (including the aliasing PC).
>
> I reckon what we should do is have seperate functions for the two ABIs,
> to ensure we don't conflate them, e.g.
>
> u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
> {
> 	if ((u32)idx > PERF_REG_ARM32_PC)
> 		return 0;
> 	if (idx == PERF_REG_ARM32_PC)
> 		return regs->pc;
> 	
> 	/*
> 	 * Compat SP and LR already in-place
> 	 */
> 	return regs->regs[idx];
> }
>
> u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
> {
> 	if ((u32)idx > PERF_REG_ARM64_MAX)
> 		return 0;
> 	if ((u32)idx == PERF_REG_ARM64_SP)
> 		return regs->sp;
> 	if ((u32)idx == PERF_REG_ARM64_PC)
> 		return regs->pc;
> 	
> 	reutrn regs->regs[idx];
> }
>
> u64 perf_reg_value(struct pt_regs *regs, int idx)
> {
> 	if (compat_user_mode(regs))
> 		return perf_reg_value_abi32(regs, idx);
> 	else
> 		return perf_reg_value_abi64(regs, idx);
> }
This modification can not fix our issue,  we need
perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 
32-bit task or not,
then return the correct PC value.
> Thanks,
> Mark.
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/26/2020 06:26 PM, Mark Rutland wrote:
> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>> Modified the patch subject and the change description.
>>
>> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
>> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
>> that perf can not parser the backtrace of app with dwarf mode in the
>> 32bit system and 64bit kernel.
>>
>> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> Thanks for this.
>
>
>> ---
>>   arch/arm64/kernel/perf_regs.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
>> index 0bbac61..0ef2880 100644
>> --- a/arch/arm64/kernel/perf_regs.c
>> +++ b/arch/arm64/kernel/perf_regs.c
>> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>>   	if ((u32)idx == PERF_REG_ARM64_PC)
>>   		return regs->pc;
>>   
>> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
>> +		&& idx == 15)
>> +		return regs->pc;
> I think there are some more issues here, and we may need a more
> substantial rework. For a compat thread, we always expose
> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> perf_reg_value() also munges the compat SP/LR into their ARM64
> equivalents, which don't exist in the 32-bit sample ABI. We also don't
> zero the regs that don't exist in 32-bit (including the aliasing PC).
>
> I reckon what we should do is have seperate functions for the two ABIs,
> to ensure we don't conflate them, e.g.
>
> u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
> {
> 	if ((u32)idx > PERF_REG_ARM32_PC)
> 		return 0;
> 	if (idx == PERF_REG_ARM32_PC)
> 		return regs->pc;
> 	
> 	/*
> 	 * Compat SP and LR already in-place
> 	 */
> 	return regs->regs[idx];
> }
>
> u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
> {
> 	if ((u32)idx > PERF_REG_ARM64_MAX)
> 		return 0;
> 	if ((u32)idx == PERF_REG_ARM64_SP)
> 		return regs->sp;
> 	if ((u32)idx == PERF_REG_ARM64_PC)
> 		return regs->pc;
> 	
> 	reutrn regs->regs[idx];
> }
>
> u64 perf_reg_value(struct pt_regs *regs, int idx)
> {
> 	if (compat_user_mode(regs))
> 		return perf_reg_value_abi32(regs, idx);
> 	else
> 		return perf_reg_value_abi64(regs, idx);
> }
This modification can not fix our issue,  we need
perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 
32-bit task or not,
then return the correct PC value.
> Thanks,
> Mark.
>

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> 
> 
> On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > Modified the patch subject and the change description.
> > > 
> > > PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> > > is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> > > that perf can not parser the backtrace of app with dwarf mode in the
> > > 32bit system and 64bit kernel.
> > > 
> > > Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> > Thanks for this.
> > 
> > 
> > > ---
> > >   arch/arm64/kernel/perf_regs.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> > > index 0bbac61..0ef2880 100644
> > > --- a/arch/arm64/kernel/perf_regs.c
> > > +++ b/arch/arm64/kernel/perf_regs.c
> > > @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> > >   	if ((u32)idx == PERF_REG_ARM64_PC)
> > >   		return regs->pc;
> > > +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> > > +		&& idx == 15)
> > > +		return regs->pc;
> > I think there are some more issues here, and we may need a more
> > substantial rework. For a compat thread, we always expose
> > PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> > perf_reg_value() also munges the compat SP/LR into their ARM64
> > equivalents, which don't exist in the 32-bit sample ABI. We also don't
> > zero the regs that don't exist in 32-bit (including the aliasing PC).
> > 
> > I reckon what we should do is have seperate functions for the two ABIs,
> > to ensure we don't conflate them, e.g.
> > 
> > u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
> > {
> > 	if ((u32)idx > PERF_REG_ARM32_PC)
> > 		return 0;
> > 	if (idx == PERF_REG_ARM32_PC)
> > 		return regs->pc;
> > 	
> > 	/*
> > 	 * Compat SP and LR already in-place
> > 	 */
> > 	return regs->regs[idx];
> > }
> > 
> > u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
> > {
> > 	if ((u32)idx > PERF_REG_ARM64_MAX)
> > 		return 0;
> > 	if ((u32)idx == PERF_REG_ARM64_SP)
> > 		return regs->sp;
> > 	if ((u32)idx == PERF_REG_ARM64_PC)
> > 		return regs->pc;
> > 	
> > 	reutrn regs->regs[idx];
> > }
> > 
> > u64 perf_reg_value(struct pt_regs *regs, int idx)
> > {
> > 	if (compat_user_mode(regs))
> > 		return perf_reg_value_abi32(regs, idx);
> > 	else
> > 		return perf_reg_value_abi64(regs, idx);
> > }
> This modification can not fix our issue,  we need
> perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> task or not,
> then return the correct PC value.

I must be missing something here.

The core code perf_reg_abi(task) is called with the task being sampled,
and the regs are from the task being sampled. For a userspace sample for
a compat task, compat_user_mode(regs) should be equivalent to the
is_compat_thread(task_thread_info(task)) check.

What am I missing?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> 
> 
> On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > Modified the patch subject and the change description.
> > > 
> > > PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
> > > is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
> > > that perf can not parser the backtrace of app with dwarf mode in the
> > > 32bit system and 64bit kernel.
> > > 
> > > Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> > Thanks for this.
> > 
> > 
> > > ---
> > >   arch/arm64/kernel/perf_regs.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> > > index 0bbac61..0ef2880 100644
> > > --- a/arch/arm64/kernel/perf_regs.c
> > > +++ b/arch/arm64/kernel/perf_regs.c
> > > @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> > >   	if ((u32)idx == PERF_REG_ARM64_PC)
> > >   		return regs->pc;
> > > +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
> > > +		&& idx == 15)
> > > +		return regs->pc;
> > I think there are some more issues here, and we may need a more
> > substantial rework. For a compat thread, we always expose
> > PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
> > perf_reg_value() also munges the compat SP/LR into their ARM64
> > equivalents, which don't exist in the 32-bit sample ABI. We also don't
> > zero the regs that don't exist in 32-bit (including the aliasing PC).
> > 
> > I reckon what we should do is have seperate functions for the two ABIs,
> > to ensure we don't conflate them, e.g.
> > 
> > u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
> > {
> > 	if ((u32)idx > PERF_REG_ARM32_PC)
> > 		return 0;
> > 	if (idx == PERF_REG_ARM32_PC)
> > 		return regs->pc;
> > 	
> > 	/*
> > 	 * Compat SP and LR already in-place
> > 	 */
> > 	return regs->regs[idx];
> > }
> > 
> > u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
> > {
> > 	if ((u32)idx > PERF_REG_ARM64_MAX)
> > 		return 0;
> > 	if ((u32)idx == PERF_REG_ARM64_SP)
> > 		return regs->sp;
> > 	if ((u32)idx == PERF_REG_ARM64_PC)
> > 		return regs->pc;
> > 	
> > 	reutrn regs->regs[idx];
> > }
> > 
> > u64 perf_reg_value(struct pt_regs *regs, int idx)
> > {
> > 	if (compat_user_mode(regs))
> > 		return perf_reg_value_abi32(regs, idx);
> > 	else
> > 		return perf_reg_value_abi64(regs, idx);
> > }
> This modification can not fix our issue,  we need
> perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> task or not,
> then return the correct PC value.

I must be missing something here.

The core code perf_reg_abi(task) is called with the task being sampled,
and the regs are from the task being sampled. For a userspace sample for
a compat task, compat_user_mode(regs) should be equivalent to the
is_compat_thread(task_thread_info(task)) check.

What am I missing?

Thanks,
Mark.

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/27/2020 11:19 PM, Mark Rutland wrote:
> On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
>>
>> On 05/26/2020 06:26 PM, Mark Rutland wrote:
>>> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>>>> Modified the patch subject and the change description.
>>>>
>>>> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
>>>> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
>>>> that perf can not parser the backtrace of app with dwarf mode in the
>>>> 32bit system and 64bit kernel.
>>>>
>>>> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
>>> Thanks for this.
>>>
>>>
>>>> ---
>>>>    arch/arm64/kernel/perf_regs.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
>>>> index 0bbac61..0ef2880 100644
>>>> --- a/arch/arm64/kernel/perf_regs.c
>>>> +++ b/arch/arm64/kernel/perf_regs.c
>>>> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>>>>    	if ((u32)idx == PERF_REG_ARM64_PC)
>>>>    		return regs->pc;
>>>> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
>>>> +		&& idx == 15)
>>>> +		return regs->pc;
>>> I think there are some more issues here, and we may need a more
>>> substantial rework. For a compat thread, we always expose
>>> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
>>> perf_reg_value() also munges the compat SP/LR into their ARM64
>>> equivalents, which don't exist in the 32-bit sample ABI. We also don't
>>> zero the regs that don't exist in 32-bit (including the aliasing PC).
>>>
>>> I reckon what we should do is have seperate functions for the two ABIs,
>>> to ensure we don't conflate them, e.g.
>>>
>>> u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
>>> {
>>> 	if ((u32)idx > PERF_REG_ARM32_PC)
>>> 		return 0;
>>> 	if (idx == PERF_REG_ARM32_PC)
>>> 		return regs->pc;
>>> 	
>>> 	/*
>>> 	 * Compat SP and LR already in-place
>>> 	 */
>>> 	return regs->regs[idx];
>>> }
>>>
>>> u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
>>> {
>>> 	if ((u32)idx > PERF_REG_ARM64_MAX)
>>> 		return 0;
>>> 	if ((u32)idx == PERF_REG_ARM64_SP)
>>> 		return regs->sp;
>>> 	if ((u32)idx == PERF_REG_ARM64_PC)
>>> 		return regs->pc;
>>> 	
>>> 	reutrn regs->regs[idx];
>>> }
>>>
>>> u64 perf_reg_value(struct pt_regs *regs, int idx)
>>> {
>>> 	if (compat_user_mode(regs))
>>> 		return perf_reg_value_abi32(regs, idx);
>>> 	else
>>> 		return perf_reg_value_abi64(regs, idx);
>>> }
>> This modification can not fix our issue,  we need
>> perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
>> task or not,
>> then return the correct PC value.
> I must be missing something here.
>
> The core code perf_reg_abi(task) is called with the task being sampled,
> and the regs are from the task being sampled. For a userspace sample for
> a compat task, compat_user_mode(regs) should be equivalent to the
> is_compat_thread(task_thread_info(task)) check.
>
> What am I missing?
This issue caused by PC value is not correct. regs are sampled in 
function perf_output_sample_regs, that call perf_reg_value(regs, bit) to 
get PC value.
PC value is regs[15] in perf_reg_value() function. it should be regs[32].

perf_output_sample_regs(struct perf_output_handle *handle,
                         struct pt_regs *regs, u64 mask)
{
         int bit;
         DECLARE_BITMAP(_mask, 64);

         bitmap_from_u64(_mask, mask);
         for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
                 u64 val;

                 val = perf_reg_value(regs, bit);
                 perf_output_put(handle, val);
         }
}

>
> Thanks,
> Mark.
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/27/2020 11:19 PM, Mark Rutland wrote:
> On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
>>
>> On 05/26/2020 06:26 PM, Mark Rutland wrote:
>>> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>>>> Modified the patch subject and the change description.
>>>>
>>>> PC value is get from regs[15] in REGS_ABI_32 mode, but correct PC
>>>> is regs->pc(regs[PERF_REG_ARM64_PC]) in arm64 kernel, which caused
>>>> that perf can not parser the backtrace of app with dwarf mode in the
>>>> 32bit system and 64bit kernel.
>>>>
>>>> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
>>> Thanks for this.
>>>
>>>
>>>> ---
>>>>    arch/arm64/kernel/perf_regs.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
>>>> index 0bbac61..0ef2880 100644
>>>> --- a/arch/arm64/kernel/perf_regs.c
>>>> +++ b/arch/arm64/kernel/perf_regs.c
>>>> @@ -32,6 +32,10 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>>>>    	if ((u32)idx == PERF_REG_ARM64_PC)
>>>>    		return regs->pc;
>>>> +	if (perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32
>>>> +		&& idx == 15)
>>>> +		return regs->pc;
>>> I think there are some more issues here, and we may need a more
>>> substantial rework. For a compat thread, we always expose
>>> PERF_SAMPLE_REGS_ABI_32 via per_reg_abi(), but for some reason
>>> perf_reg_value() also munges the compat SP/LR into their ARM64
>>> equivalents, which don't exist in the 32-bit sample ABI. We also don't
>>> zero the regs that don't exist in 32-bit (including the aliasing PC).
>>>
>>> I reckon what we should do is have seperate functions for the two ABIs,
>>> to ensure we don't conflate them, e.g.
>>>
>>> u64 perf_reg_value_abi32(struct pt_regs *regs, int idx)
>>> {
>>> 	if ((u32)idx > PERF_REG_ARM32_PC)
>>> 		return 0;
>>> 	if (idx == PERF_REG_ARM32_PC)
>>> 		return regs->pc;
>>> 	
>>> 	/*
>>> 	 * Compat SP and LR already in-place
>>> 	 */
>>> 	return regs->regs[idx];
>>> }
>>>
>>> u64 perf_reg_value_abi64(struct pt_regs *regs, int idx)
>>> {
>>> 	if ((u32)idx > PERF_REG_ARM64_MAX)
>>> 		return 0;
>>> 	if ((u32)idx == PERF_REG_ARM64_SP)
>>> 		return regs->sp;
>>> 	if ((u32)idx == PERF_REG_ARM64_PC)
>>> 		return regs->pc;
>>> 	
>>> 	reutrn regs->regs[idx];
>>> }
>>>
>>> u64 perf_reg_value(struct pt_regs *regs, int idx)
>>> {
>>> 	if (compat_user_mode(regs))
>>> 		return perf_reg_value_abi32(regs, idx);
>>> 	else
>>> 		return perf_reg_value_abi64(regs, idx);
>>> }
>> This modification can not fix our issue,  we need
>> perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
>> task or not,
>> then return the correct PC value.
> I must be missing something here.
>
> The core code perf_reg_abi(task) is called with the task being sampled,
> and the regs are from the task being sampled. For a userspace sample for
> a compat task, compat_user_mode(regs) should be equivalent to the
> is_compat_thread(task_thread_info(task)) check.
>
> What am I missing?
This issue caused by PC value is not correct. regs are sampled in 
function perf_output_sample_regs, that call perf_reg_value(regs, bit) to 
get PC value.
PC value is regs[15] in perf_reg_value() function. it should be regs[32].

perf_output_sample_regs(struct perf_output_handle *handle,
                         struct pt_regs *regs, u64 mask)
{
         int bit;
         DECLARE_BITMAP(_mask, 64);

         bitmap_from_u64(_mask, mask);
         for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
                 u64 val;

                 val = perf_reg_value(regs, bit);
                 perf_output_put(handle, val);
         }
}

>
> Thanks,
> Mark.
>

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
> On 05/27/2020 11:19 PM, Mark Rutland wrote:
> > On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> > > On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > > > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > This modification can not fix our issue,  we need
> > > perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> > > task or not,
> > > then return the correct PC value.
> > I must be missing something here.
> > 
> > The core code perf_reg_abi(task) is called with the task being sampled,
> > and the regs are from the task being sampled. For a userspace sample for
> > a compat task, compat_user_mode(regs) should be equivalent to the
> > is_compat_thread(task_thread_info(task)) check.
> > 
> > What am I missing?
> This issue caused by PC value is not correct. regs are sampled in function
> perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
> value.
> PC value is regs[15] in perf_reg_value() function. it should be regs[32].
> 
> perf_output_sample_regs(struct perf_output_handle *handle,
>                         struct pt_regs *regs, u64 mask)
> {
>         int bit;
>         DECLARE_BITMAP(_mask, 64);
> 
>         bitmap_from_u64(_mask, mask);
>         for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
>                 u64 val;
> 
>                 val = perf_reg_value(regs, bit);
>                 perf_output_put(handle, val);
>         }
> }

Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
Are you saying that's not the case? If so, please can you provide an example
of when they are different?

Leaving that aside for a second, I also think it's reasonable to question
whether this whole interface is busted or not. I looked at it last night but
struggled to work out what it's supposed to do. Consider these three
scenarios, all under an arm64 kernel:

  1. 64-bit perf + 64-bit application being profiled
  2. 64-bit perf + 32-bit application being profiled
  3. 32-bit perf + 32-bit application being profiled

It looks like the current code is a bodge to try to handle both (2) and
(3) at the same time:

  - In case (3), userspace only asks about registers 0-15
  - In case (2), we fudge the higher registers so that 64-bit SP and LR
    hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
    to unwind the stack

So the idea behind the patch looks fine because case (3) is expecting the PC
in register 15 and instead gets 0, but the temptation is to clean this up so
that cases (2) and (3) report the same data to userspace (along the lines of
Mark's patch), namely only the first 16 registers with the PC moved down. We
can only do that if the unwinder is happy, which it might be if it only ever
looks up dwarf register numbers based on the unwind tables in the binary.
Somebody would need to dig into that. Otherwise, if it generates unconditional
references to things like register 30 to grab the link register, then we're
stuck with the bodge and need to special-case the PC.

Thoughts?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
> On 05/27/2020 11:19 PM, Mark Rutland wrote:
> > On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> > > On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > > > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > This modification can not fix our issue,  we need
> > > perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> > > task or not,
> > > then return the correct PC value.
> > I must be missing something here.
> > 
> > The core code perf_reg_abi(task) is called with the task being sampled,
> > and the regs are from the task being sampled. For a userspace sample for
> > a compat task, compat_user_mode(regs) should be equivalent to the
> > is_compat_thread(task_thread_info(task)) check.
> > 
> > What am I missing?
> This issue caused by PC value is not correct. regs are sampled in function
> perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
> value.
> PC value is regs[15] in perf_reg_value() function. it should be regs[32].
> 
> perf_output_sample_regs(struct perf_output_handle *handle,
>                         struct pt_regs *regs, u64 mask)
> {
>         int bit;
>         DECLARE_BITMAP(_mask, 64);
> 
>         bitmap_from_u64(_mask, mask);
>         for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
>                 u64 val;
> 
>                 val = perf_reg_value(regs, bit);
>                 perf_output_put(handle, val);
>         }
> }

Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
Are you saying that's not the case? If so, please can you provide an example
of when they are different?

Leaving that aside for a second, I also think it's reasonable to question
whether this whole interface is busted or not. I looked at it last night but
struggled to work out what it's supposed to do. Consider these three
scenarios, all under an arm64 kernel:

  1. 64-bit perf + 64-bit application being profiled
  2. 64-bit perf + 32-bit application being profiled
  3. 32-bit perf + 32-bit application being profiled

It looks like the current code is a bodge to try to handle both (2) and
(3) at the same time:

  - In case (3), userspace only asks about registers 0-15
  - In case (2), we fudge the higher registers so that 64-bit SP and LR
    hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
    to unwind the stack

So the idea behind the patch looks fine because case (3) is expecting the PC
in register 15 and instead gets 0, but the temptation is to clean this up so
that cases (2) and (3) report the same data to userspace (along the lines of
Mark's patch), namely only the first 16 registers with the PC moved down. We
can only do that if the unwinder is happy, which it might be if it only ever
looks up dwarf register numbers based on the unwind tables in the binary.
Somebody would need to dig into that. Otherwise, if it generates unconditional
references to things like register 30 to grab the link register, then we're
stuck with the bodge and need to special-case the PC.

Thoughts?

Will

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/28/2020 03:54 PM, Will Deacon wrote:
> On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
>> On 05/27/2020 11:19 PM, Mark Rutland wrote:
>>> On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
>>>> On 05/26/2020 06:26 PM, Mark Rutland wrote:
>>>>> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>>>> This modification can not fix our issue,  we need
>>>> perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
>>>> task or not,
>>>> then return the correct PC value.
>>> I must be missing something here.
>>>
>>> The core code perf_reg_abi(task) is called with the task being sampled,
>>> and the regs are from the task being sampled. For a userspace sample for
>>> a compat task, compat_user_mode(regs) should be equivalent to the
>>> is_compat_thread(task_thread_info(task)) check.
>>>
>>> What am I missing?
>> This issue caused by PC value is not correct. regs are sampled in function
>> perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
>> value.
>> PC value is regs[15] in perf_reg_value() function. it should be regs[32].
>>
>> perf_output_sample_regs(struct perf_output_handle *handle,
>>                          struct pt_regs *regs, u64 mask)
>> {
>>          int bit;
>>          DECLARE_BITMAP(_mask, 64);
>>
>>          bitmap_from_u64(_mask, mask);
>>          for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
>>                  u64 val;
>>
>>                  val = perf_reg_value(regs, bit);
>>                  perf_output_put(handle, val);
>>          }
>> }
> Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
> exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
> Are you saying that's not the case? If so, please can you provide an example
> of when they are different?
Yes, compat_user_mode(regs) is same with 'perf_reg_abi(current) == 
PERF_SAMPLE_REGS_ABI_32'.
I tested it.

Jiping
>
> Leaving that aside for a second, I also think it's reasonable to question
> whether this whole interface is busted or not. I looked at it last night but
> struggled to work out what it's supposed to do. Consider these three
> scenarios, all under an arm64 kernel:
>
>    1. 64-bit perf + 64-bit application being profiled
>    2. 64-bit perf + 32-bit application being profiled
>    3. 32-bit perf + 32-bit application being profiled
>
> It looks like the current code is a bodge to try to handle both (2) and
> (3) at the same time:
>
>    - In case (3), userspace only asks about registers 0-15
>    - In case (2), we fudge the higher registers so that 64-bit SP and LR
>      hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
>      to unwind the stack
>
> So the idea behind the patch looks fine because case (3) is expecting the PC
> in register 15 and instead gets 0, but the temptation is to clean this up so
> that cases (2) and (3) report the same data to userspace (along the lines of
> Mark's patch), namely only the first 16 registers with the PC moved down. We
> can only do that if the unwinder is happy, which it might be if it only ever
> looks up dwarf register numbers based on the unwind tables in the binary.
> Somebody would need to dig into that. Otherwise, if it generates unconditional
> references to things like register 30 to grab the link register, then we're
> stuck with the bodge and need to special-case the PC.
>
> Thoughts?
>
> Will
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Jiping Ma]

On 05/28/2020 03:54 PM, Will Deacon wrote:
> On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
>> On 05/27/2020 11:19 PM, Mark Rutland wrote:
>>> On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
>>>> On 05/26/2020 06:26 PM, Mark Rutland wrote:
>>>>> On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
>>>> This modification can not fix our issue,  we need
>>>> perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
>>>> task or not,
>>>> then return the correct PC value.
>>> I must be missing something here.
>>>
>>> The core code perf_reg_abi(task) is called with the task being sampled,
>>> and the regs are from the task being sampled. For a userspace sample for
>>> a compat task, compat_user_mode(regs) should be equivalent to the
>>> is_compat_thread(task_thread_info(task)) check.
>>>
>>> What am I missing?
>> This issue caused by PC value is not correct. regs are sampled in function
>> perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
>> value.
>> PC value is regs[15] in perf_reg_value() function. it should be regs[32].
>>
>> perf_output_sample_regs(struct perf_output_handle *handle,
>>                          struct pt_regs *regs, u64 mask)
>> {
>>          int bit;
>>          DECLARE_BITMAP(_mask, 64);
>>
>>          bitmap_from_u64(_mask, mask);
>>          for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
>>                  u64 val;
>>
>>                  val = perf_reg_value(regs, bit);
>>                  perf_output_put(handle, val);
>>          }
>> }
> Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
> exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
> Are you saying that's not the case? If so, please can you provide an example
> of when they are different?
Yes, compat_user_mode(regs) is same with 'perf_reg_abi(current) == 
PERF_SAMPLE_REGS_ABI_32'.
I tested it.

Jiping
>
> Leaving that aside for a second, I also think it's reasonable to question
> whether this whole interface is busted or not. I looked at it last night but
> struggled to work out what it's supposed to do. Consider these three
> scenarios, all under an arm64 kernel:
>
>    1. 64-bit perf + 64-bit application being profiled
>    2. 64-bit perf + 32-bit application being profiled
>    3. 32-bit perf + 32-bit application being profiled
>
> It looks like the current code is a bodge to try to handle both (2) and
> (3) at the same time:
>
>    - In case (3), userspace only asks about registers 0-15
>    - In case (2), we fudge the higher registers so that 64-bit SP and LR
>      hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
>      to unwind the stack
>
> So the idea behind the patch looks fine because case (3) is expecting the PC
> in register 15 and instead gets 0, but the temptation is to clean this up so
> that cases (2) and (3) report the same data to userspace (along the lines of
> Mark's patch), namely only the first 16 registers with the PC moved down. We
> can only do that if the unwinder is happy, which it might be if it only ever
> looks up dwarf register numbers based on the unwind tables in the binary.
> Somebody would need to dig into that. Otherwise, if it generates unconditional
> references to things like register 30 to grab the link register, then we're
> stuck with the bodge and need to special-case the PC.
>
> Thoughts?
>
> Will
>

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Thu, May 28, 2020 at 08:54:19AM +0100, Will Deacon wrote:
> On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
> > On 05/27/2020 11:19 PM, Mark Rutland wrote:
> > > On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> > > > On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > > > > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > > This modification can not fix our issue,� we need
> > > > perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> > > > task or not,
> > > > then return the correct PC value.
> > > I must be missing something here.
> > > 
> > > The core code perf_reg_abi(task) is called with the task being sampled,
> > > and the regs are from the task being sampled. For a userspace sample for
> > > a compat task, compat_user_mode(regs) should be equivalent to the
> > > is_compat_thread(task_thread_info(task)) check.
> > > 
> > > What am I missing?
> > This issue caused by PC value is not correct. regs are sampled in function
> > perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
> > value.
> > PC value is regs[15] in perf_reg_value() function. it should be regs[32].
> > 
> > perf_output_sample_regs(struct perf_output_handle *handle,
> > ����������������������� struct pt_regs *regs, u64 mask)
> > {
> > ������� int bit;
> > ������� DECLARE_BITMAP(_mask, 64);
> > 
> > ������� bitmap_from_u64(_mask, mask);
> > ������� for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
> > ��������������� u64 val;
> > 
> > ��������������� val = perf_reg_value(regs, bit);
> > ��������������� perf_output_put(handle, val);
> > ������� }
> > }
> 
> Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
> exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
> Are you saying that's not the case? If so, please can you provide an example
> of when they are different?
> 
> Leaving that aside for a second, I also think it's reasonable to question
> whether this whole interface is busted or not. I looked at it last night but
> struggled to work out what it's supposed to do. Consider these three
> scenarios, all under an arm64 kernel:
> 
>   1. 64-bit perf + 64-bit application being profiled
>   2. 64-bit perf + 32-bit application being profiled
>   3. 32-bit perf + 32-bit application being profiled
> 
> It looks like the current code is a bodge to try to handle both (2) and
> (3) at the same time:
> 
>   - In case (3), userspace only asks about registers 0-15
>   - In case (2), we fudge the higher registers so that 64-bit SP and LR
>     hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
>     to unwind the stack

I think the fudging is nonsensical to begin with, as I would have
expected that PERF_REGS_ABI_32 should be the same layout regardless of
consumer (and therefore should be identical to the 32-bit arm native
format). I realise that doesn't change that we might be stuck with it...

> So the idea behind the patch looks fine because case (3) is expecting the PC
> in register 15 and instead gets 0, but the temptation is to clean this up so
> that cases (2) and (3) report the same data to userspace (along the lines of
> Mark's patch), namely only the first 16 registers with the PC moved down. We
> can only do that if the unwinder is happy, which it might be if it only ever
> looks up dwarf register numbers based on the unwind tables in the binary.
> Somebody would need to dig into that.

Agreed; I will try to figure out what the perf tool does in the three
cases above. I would be grateful if others could take a look too.

Another slightly scary thought: what happens for a 32-bit perf with a
64-bit application being profiled? I don't see how that'd be forbidden,
but I also don't see how it'd work.

> Otherwise, if it generates unconditional references to things like
> register 30 to grab the link register, then we're stuck with the bodge
> and need to special-case the PC.

I agree that in that case we'd have to keep the existing bodge, and we'd
have to special-case the PC, but I'd prefer to split the logic for case
1 into a separate function for cases 2 and 3 so that we can more easily
avoid getting this more confused.

Let's figure out what userspace does first...

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Thu, May 28, 2020 at 08:54:19AM +0100, Will Deacon wrote:
> On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
> > On 05/27/2020 11:19 PM, Mark Rutland wrote:
> > > On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> > > > On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > > > > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > > This modification can not fix our issue,� we need
> > > > perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> > > > task or not,
> > > > then return the correct PC value.
> > > I must be missing something here.
> > > 
> > > The core code perf_reg_abi(task) is called with the task being sampled,
> > > and the regs are from the task being sampled. For a userspace sample for
> > > a compat task, compat_user_mode(regs) should be equivalent to the
> > > is_compat_thread(task_thread_info(task)) check.
> > > 
> > > What am I missing?
> > This issue caused by PC value is not correct. regs are sampled in function
> > perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
> > value.
> > PC value is regs[15] in perf_reg_value() function. it should be regs[32].
> > 
> > perf_output_sample_regs(struct perf_output_handle *handle,
> > ����������������������� struct pt_regs *regs, u64 mask)
> > {
> > ������� int bit;
> > ������� DECLARE_BITMAP(_mask, 64);
> > 
> > ������� bitmap_from_u64(_mask, mask);
> > ������� for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
> > ��������������� u64 val;
> > 
> > ��������������� val = perf_reg_value(regs, bit);
> > ��������������� perf_output_put(handle, val);
> > ������� }
> > }
> 
> Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
> exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
> Are you saying that's not the case? If so, please can you provide an example
> of when they are different?
> 
> Leaving that aside for a second, I also think it's reasonable to question
> whether this whole interface is busted or not. I looked at it last night but
> struggled to work out what it's supposed to do. Consider these three
> scenarios, all under an arm64 kernel:
> 
>   1. 64-bit perf + 64-bit application being profiled
>   2. 64-bit perf + 32-bit application being profiled
>   3. 32-bit perf + 32-bit application being profiled
> 
> It looks like the current code is a bodge to try to handle both (2) and
> (3) at the same time:
> 
>   - In case (3), userspace only asks about registers 0-15
>   - In case (2), we fudge the higher registers so that 64-bit SP and LR
>     hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
>     to unwind the stack

I think the fudging is nonsensical to begin with, as I would have
expected that PERF_REGS_ABI_32 should be the same layout regardless of
consumer (and therefore should be identical to the 32-bit arm native
format). I realise that doesn't change that we might be stuck with it...

> So the idea behind the patch looks fine because case (3) is expecting the PC
> in register 15 and instead gets 0, but the temptation is to clean this up so
> that cases (2) and (3) report the same data to userspace (along the lines of
> Mark's patch), namely only the first 16 registers with the PC moved down. We
> can only do that if the unwinder is happy, which it might be if it only ever
> looks up dwarf register numbers based on the unwind tables in the binary.
> Somebody would need to dig into that.

Agreed; I will try to figure out what the perf tool does in the three
cases above. I would be grateful if others could take a look too.

Another slightly scary thought: what happens for a 32-bit perf with a
64-bit application being profiled? I don't see how that'd be forbidden,
but I also don't see how it'd work.

> Otherwise, if it generates unconditional references to things like
> register 30 to grab the link register, then we're stuck with the bodge
> and need to special-case the PC.

I agree that in that case we'd have to keep the existing bodge, and we'd
have to special-case the PC, but I'd prefer to split the logic for case
1 into a separate function for cases 2 and 3 so that we can more easily
avoid getting this more confused.

Let's figure out what userspace does first...

Thanks,
Mark.

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Thu, Jun 18, 2020 at 02:03:32PM +0100, Mark Rutland wrote:
> On Thu, May 28, 2020 at 08:54:19AM +0100, Will Deacon wrote:
> > On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
> > > On 05/27/2020 11:19 PM, Mark Rutland wrote:
> > > > On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> > > > > On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > > > > > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > > > This modification can not fix our issue,� we need
> > > > > perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> > > > > task or not,
> > > > > then return the correct PC value.
> > > > I must be missing something here.
> > > > 
> > > > The core code perf_reg_abi(task) is called with the task being sampled,
> > > > and the regs are from the task being sampled. For a userspace sample for
> > > > a compat task, compat_user_mode(regs) should be equivalent to the
> > > > is_compat_thread(task_thread_info(task)) check.
> > > > 
> > > > What am I missing?
> > > This issue caused by PC value is not correct. regs are sampled in function
> > > perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
> > > value.
> > > PC value is regs[15] in perf_reg_value() function. it should be regs[32].
> > > 
> > > perf_output_sample_regs(struct perf_output_handle *handle,
> > > ����������������������� struct pt_regs *regs, u64 mask)
> > > {
> > > ������� int bit;
> > > ������� DECLARE_BITMAP(_mask, 64);
> > > 
> > > ������� bitmap_from_u64(_mask, mask);
> > > ������� for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
> > > ��������������� u64 val;
> > > 
> > > ��������������� val = perf_reg_value(regs, bit);
> > > ��������������� perf_output_put(handle, val);
> > > ������� }

wtf happened here?!

> > Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
> > exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
> > Are you saying that's not the case? If so, please can you provide an example
> > of when they are different?
> > 
> > Leaving that aside for a second, I also think it's reasonable to question
> > whether this whole interface is busted or not. I looked at it last night but
> > struggled to work out what it's supposed to do. Consider these three
> > scenarios, all under an arm64 kernel:
> > 
> >   1. 64-bit perf + 64-bit application being profiled
> >   2. 64-bit perf + 32-bit application being profiled
> >   3. 32-bit perf + 32-bit application being profiled
> > 
> > It looks like the current code is a bodge to try to handle both (2) and
> > (3) at the same time:
> > 
> >   - In case (3), userspace only asks about registers 0-15
> >   - In case (2), we fudge the higher registers so that 64-bit SP and LR
> >     hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
> >     to unwind the stack
> 
> I think the fudging is nonsensical to begin with, as I would have
> expected that PERF_REGS_ABI_32 should be the same layout regardless of
> consumer (and therefore should be identical to the 32-bit arm native
> format). I realise that doesn't change that we might be stuck with it...

Thankfully, I don't think the fudging is affected by the patch here, so I'm
inclined to take it as a fix. In fact, with this change, doesn't the prefix
of the regs returned in (2) look the same as the one you would expect in
32-bit arm?

> > So the idea behind the patch looks fine because case (3) is expecting the PC
> > in register 15 and instead gets 0, but the temptation is to clean this up so
> > that cases (2) and (3) report the same data to userspace (along the lines of
> > Mark's patch), namely only the first 16 registers with the PC moved down. We
> > can only do that if the unwinder is happy, which it might be if it only ever
> > looks up dwarf register numbers based on the unwind tables in the binary.
> > Somebody would need to dig into that.
> 
> Agreed; I will try to figure out what the perf tool does in the three
> cases above. I would be grateful if others could take a look too.
> 
> Another slightly scary thought: what happens for a 32-bit perf with a
> 64-bit application being profiled? I don't see how that'd be forbidden,
> but I also don't see how it'd work.

Although you can obviously feed a 32-bit perf with a 64-bit perf.data,
a 32-bit task cannot spawn a 64-bit child so that rules out a lot of the
runtime weirdness, no?

> > Otherwise, if it generates unconditional references to things like
> > register 30 to grab the link register, then we're stuck with the bodge
> > and need to special-case the PC.
> 
> I agree that in that case we'd have to keep the existing bodge, and we'd
> have to special-case the PC, but I'd prefer to split the logic for case
> 1 into a separate function for cases 2 and 3 so that we can more easily
> avoid getting this more confused.
> 
> Let's figure out what userspace does first...

afaict, perf just slurps all the registers in one go and hands them off
to the unwinder, which will index into them according to the dwarf unwind
tables. I have doubts as to how well that works (for example, unwinder
address corrections due to Thumb would not be applied).

So, I think we should take this patch (which puts the PC where you'd expect
to find it for compat tasks) and then we could consider removing the current
lr/sp fudging as a separate patch, which we could revert if it causes a
problem. However, I'm not sure I want to open that up.

What do you think?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Thu, Jun 18, 2020 at 02:03:32PM +0100, Mark Rutland wrote:
> On Thu, May 28, 2020 at 08:54:19AM +0100, Will Deacon wrote:
> > On Thu, May 28, 2020 at 09:06:07AM +0800, Jiping Ma wrote:
> > > On 05/27/2020 11:19 PM, Mark Rutland wrote:
> > > > On Wed, May 27, 2020 at 09:33:00AM +0800, Jiping Ma wrote:
> > > > > On 05/26/2020 06:26 PM, Mark Rutland wrote:
> > > > > > On Mon, May 11, 2020 at 10:52:07AM +0800, Jiping Ma wrote:
> > > > > This modification can not fix our issue,� we need
> > > > > perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32 to judge if it is 32-bit
> > > > > task or not,
> > > > > then return the correct PC value.
> > > > I must be missing something here.
> > > > 
> > > > The core code perf_reg_abi(task) is called with the task being sampled,
> > > > and the regs are from the task being sampled. For a userspace sample for
> > > > a compat task, compat_user_mode(regs) should be equivalent to the
> > > > is_compat_thread(task_thread_info(task)) check.
> > > > 
> > > > What am I missing?
> > > This issue caused by PC value is not correct. regs are sampled in function
> > > perf_output_sample_regs, that call perf_reg_value(regs, bit) to get PC
> > > value.
> > > PC value is regs[15] in perf_reg_value() function. it should be regs[32].
> > > 
> > > perf_output_sample_regs(struct perf_output_handle *handle,
> > > ����������������������� struct pt_regs *regs, u64 mask)
> > > {
> > > ������� int bit;
> > > ������� DECLARE_BITMAP(_mask, 64);
> > > 
> > > ������� bitmap_from_u64(_mask, mask);
> > > ������� for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
> > > ��������������� u64 val;
> > > 
> > > ��������������� val = perf_reg_value(regs, bit);
> > > ��������������� perf_output_put(handle, val);
> > > ������� }

wtf happened here?!

> > Yes, but Mark's point is that checking 'compat_user_mode(regs)' should be
> > exactly the same as checking 'perf_reg_abi(current) == PERF_SAMPLE_REGS_ABI_32'.
> > Are you saying that's not the case? If so, please can you provide an example
> > of when they are different?
> > 
> > Leaving that aside for a second, I also think it's reasonable to question
> > whether this whole interface is busted or not. I looked at it last night but
> > struggled to work out what it's supposed to do. Consider these three
> > scenarios, all under an arm64 kernel:
> > 
> >   1. 64-bit perf + 64-bit application being profiled
> >   2. 64-bit perf + 32-bit application being profiled
> >   3. 32-bit perf + 32-bit application being profiled
> > 
> > It looks like the current code is a bodge to try to handle both (2) and
> > (3) at the same time:
> > 
> >   - In case (3), userspace only asks about registers 0-15
> >   - In case (2), we fudge the higher registers so that 64-bit SP and LR
> >     hold the 32-bit values as a bodge to allow a 64-bit dwarf unwinder
> >     to unwind the stack
> 
> I think the fudging is nonsensical to begin with, as I would have
> expected that PERF_REGS_ABI_32 should be the same layout regardless of
> consumer (and therefore should be identical to the 32-bit arm native
> format). I realise that doesn't change that we might be stuck with it...

Thankfully, I don't think the fudging is affected by the patch here, so I'm
inclined to take it as a fix. In fact, with this change, doesn't the prefix
of the regs returned in (2) look the same as the one you would expect in
32-bit arm?

> > So the idea behind the patch looks fine because case (3) is expecting the PC
> > in register 15 and instead gets 0, but the temptation is to clean this up so
> > that cases (2) and (3) report the same data to userspace (along the lines of
> > Mark's patch), namely only the first 16 registers with the PC moved down. We
> > can only do that if the unwinder is happy, which it might be if it only ever
> > looks up dwarf register numbers based on the unwind tables in the binary.
> > Somebody would need to dig into that.
> 
> Agreed; I will try to figure out what the perf tool does in the three
> cases above. I would be grateful if others could take a look too.
> 
> Another slightly scary thought: what happens for a 32-bit perf with a
> 64-bit application being profiled? I don't see how that'd be forbidden,
> but I also don't see how it'd work.

Although you can obviously feed a 32-bit perf with a 64-bit perf.data,
a 32-bit task cannot spawn a 64-bit child so that rules out a lot of the
runtime weirdness, no?

> > Otherwise, if it generates unconditional references to things like
> > register 30 to grab the link register, then we're stuck with the bodge
> > and need to special-case the PC.
> 
> I agree that in that case we'd have to keep the existing bodge, and we'd
> have to special-case the PC, but I'd prefer to split the logic for case
> 1 into a separate function for cases 2 and 3 so that we can more easily
> avoid getting this more confused.
> 
> Let's figure out what userspace does first...

afaict, perf just slurps all the registers in one go and hands them off
to the unwinder, which will index into them according to the dwarf unwind
tables. I have doubts as to how well that works (for example, unwinder
address corrections due to Thumb would not be applied).

So, I think we should take this patch (which puts the PC where you'd expect
to find it for compat tasks) and then we could consider removing the current
lr/sp fudging as a separate patch, which we could revert if it causes a
problem. However, I'm not sure I want to open that up.

What do you think?

Will

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Tue, Jun 23, 2020 at 06:19:10PM +0100, Will Deacon wrote:
> So, I think we should take this patch (which puts the PC where you'd expect
> to find it for compat tasks) and then we could consider removing the current
> lr/sp fudging as a separate patch, which we could revert if it causes a
> problem. However, I'm not sure I want to open that up.

Patch below...

Will

--->8

From 7452148b87ed8c82826474366dbe536fd960d3a7 Mon Sep 17 00:00:00 2001
From: Jiping Ma <jiping.ma2@windriver.com>
Date: Mon, 11 May 2020 10:52:07 +0800
Subject: [PATCH] arm64: perf: Report the PC value in REGS_ABI_32 mode

A 32-bit perf querying the registers of a compat task using REGS_ABI_32
will receive zeroes from w15, when it expects to find the PC.

Return the PC value for register dwarf register 15 when returning register
values for a compat task to perf.

Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
Link: https://lore.kernel.org/r/1589165527-188401-1-git-send-email-jiping.ma2@windriver.com
[will: Shuffled code and added a comment]
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/perf_regs.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
index 0bbac612146e..952b26a05d0f 100644
--- a/arch/arm64/kernel/perf_regs.c
+++ b/arch/arm64/kernel/perf_regs.c
@@ -15,15 +15,25 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 		return 0;
 
 	/*
-	 * Compat (i.e. 32 bit) mode:
-	 * - PC has been set in the pt_regs struct in kernel_entry,
-	 * - Handle SP and LR here.
+	 * Our handling of compat tasks (PERF_SAMPLE_REGS_ABI_32) is weird. For
+	 * a 32-bit perf inspecting a 32-bit task, then it will look at the
+	 * first 16 registers. These correspond directly to the registers saved
+	 * in our pt_regs structure, with the exception of the PC, so we copy
+	 * that down (x15 corresponds to SP_hyp in the architecture). So far, so
+	 * good. The oddity arises when a 64-bit perf looks at a 32-bit task and
+	 * asks for registers beyond PERF_REG_ARM_MAX. In this case, we return
+	 * SP_usr, LR_usr and PC in the positions where the AArch64 registers
+	 * would normally live. The initial idea was to allow a 64-bit unwinder
+	 * to unwinder a 32-bit task and, although it's not clear how well that
+	 * works in practice, we're kind of stuck with this interface now.
 	 */
 	if (compat_user_mode(regs)) {
 		if ((u32)idx == PERF_REG_ARM64_SP)
 			return regs->compat_sp;
 		if ((u32)idx == PERF_REG_ARM64_LR)
 			return regs->compat_lr;
+		if (idx == 15)
+			return regs->pc;
 	}
 
 	if ((u32)idx == PERF_REG_ARM64_SP)
-- 
2.27.0.111.gc72c7da667-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Will Deacon]

On Tue, Jun 23, 2020 at 06:19:10PM +0100, Will Deacon wrote:
> So, I think we should take this patch (which puts the PC where you'd expect
> to find it for compat tasks) and then we could consider removing the current
> lr/sp fudging as a separate patch, which we could revert if it causes a
> problem. However, I'm not sure I want to open that up.

Patch below...

Will

--->8

From 7452148b87ed8c82826474366dbe536fd960d3a7 Mon Sep 17 00:00:00 2001
From: Jiping Ma <jiping.ma2@windriver.com>
Date: Mon, 11 May 2020 10:52:07 +0800
Subject: [PATCH] arm64: perf: Report the PC value in REGS_ABI_32 mode

A 32-bit perf querying the registers of a compat task using REGS_ABI_32
will receive zeroes from w15, when it expects to find the PC.

Return the PC value for register dwarf register 15 when returning register
values for a compat task to perf.

Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
Link: https://lore.kernel.org/r/1589165527-188401-1-git-send-email-jiping.ma2@windriver.com
[will: Shuffled code and added a comment]
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/perf_regs.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
index 0bbac612146e..952b26a05d0f 100644
--- a/arch/arm64/kernel/perf_regs.c
+++ b/arch/arm64/kernel/perf_regs.c
@@ -15,15 +15,25 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 		return 0;
 
 	/*
-	 * Compat (i.e. 32 bit) mode:
-	 * - PC has been set in the pt_regs struct in kernel_entry,
-	 * - Handle SP and LR here.
+	 * Our handling of compat tasks (PERF_SAMPLE_REGS_ABI_32) is weird. For
+	 * a 32-bit perf inspecting a 32-bit task, then it will look at the
+	 * first 16 registers. These correspond directly to the registers saved
+	 * in our pt_regs structure, with the exception of the PC, so we copy
+	 * that down (x15 corresponds to SP_hyp in the architecture). So far, so
+	 * good. The oddity arises when a 64-bit perf looks at a 32-bit task and
+	 * asks for registers beyond PERF_REG_ARM_MAX. In this case, we return
+	 * SP_usr, LR_usr and PC in the positions where the AArch64 registers
+	 * would normally live. The initial idea was to allow a 64-bit unwinder
+	 * to unwinder a 32-bit task and, although it's not clear how well that
+	 * works in practice, we're kind of stuck with this interface now.
 	 */
 	if (compat_user_mode(regs)) {
 		if ((u32)idx == PERF_REG_ARM64_SP)
 			return regs->compat_sp;
 		if ((u32)idx == PERF_REG_ARM64_LR)
 			return regs->compat_lr;
+		if (idx == 15)
+			return regs->pc;
 	}
 
 	if ((u32)idx == PERF_REG_ARM64_SP)
-- 
2.27.0.111.gc72c7da667-goog

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Tue, Jun 23, 2020 at 06:44:56PM +0100, Will Deacon wrote:
> On Tue, Jun 23, 2020 at 06:19:10PM +0100, Will Deacon wrote:
> > So, I think we should take this patch (which puts the PC where you'd expect
> > to find it for compat tasks) and then we could consider removing the current
> > lr/sp fudging as a separate patch, which we could revert if it causes a
> > problem. However, I'm not sure I want to open that up.
> 
> Patch below...
> 
> Will
> 
> --->8
> 
> From 7452148b87ed8c82826474366dbe536fd960d3a7 Mon Sep 17 00:00:00 2001
> From: Jiping Ma <jiping.ma2@windriver.com>
> Date: Mon, 11 May 2020 10:52:07 +0800
> Subject: [PATCH] arm64: perf: Report the PC value in REGS_ABI_32 mode
> 
> A 32-bit perf querying the registers of a compat task using REGS_ABI_32
> will receive zeroes from w15, when it expects to find the PC.
> 
> Return the PC value for register dwarf register 15 when returning register
> values for a compat task to perf.
> 
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> Link: https://lore.kernel.org/r/1589165527-188401-1-git-send-email-jiping.ma2@windriver.com
> [will: Shuffled code and added a comment]
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/perf_regs.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> index 0bbac612146e..952b26a05d0f 100644
> --- a/arch/arm64/kernel/perf_regs.c
> +++ b/arch/arm64/kernel/perf_regs.c
> @@ -15,15 +15,25 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  		return 0;
>  
>  	/*
> -	 * Compat (i.e. 32 bit) mode:
> -	 * - PC has been set in the pt_regs struct in kernel_entry,
> -	 * - Handle SP and LR here.
> +	 * Our handling of compat tasks (PERF_SAMPLE_REGS_ABI_32) is weird. For
> +	 * a 32-bit perf inspecting a 32-bit task, then it will look at the
> +	 * first 16 registers. These correspond directly to the registers saved
> +	 * in our pt_regs structure, with the exception of the PC, so we copy
> +	 * that down (x15 corresponds to SP_hyp in the architecture). So far, so
> +	 * good. The oddity arises when a 64-bit perf looks at a 32-bit task and
> +	 * asks for registers beyond PERF_REG_ARM_MAX. In this case, we return
> +	 * SP_usr, LR_usr and PC in the positions where the AArch64 registers
> +	 * would normally live. The initial idea was to allow a 64-bit unwinder
> +	 * to unwinder a 32-bit task and, although it's not clear how well that
> +	 * works in practice, we're kind of stuck with this interface now.
>  	 */

Would you be happy with:

	/*
	 * For ABI reasons, PERF_SAMPLE_REGS_ABI_32 is messy.
	 *
	 * 32-bit consumers of the regs expect this to look like the
	 * native 32-bit layout with entries 0-12 being r0-r12, 13 being
	 * the SP, 14 being the LR, and 15 being the PC. The compat SP
	 * and LR are placed in x13 and x14 respectively upon an
	 * exception, but we need to copy the PC into the expected slot.
	 * Ideally the other slots would all be zeroed to match native
	 * 32-bit, but we can't do this because of existing 64-bit
	 * consumers.
	 *
	 * Existing 64-bit consumers assume that the PC, LR, and SP are
	 * in the same positions as the PERF_SAMPLE_REGS_ABI_64 layout,
	 * rather than interpreting PERF_SAMPLE_REGS_ABI_32 the same as
	 * the native 32-bit PERF_SAMPLE_REGS_ABI_32. To not break these
	 * we must copy the PC into their ABI_64 slots, and leave copies
	 * of the SP and LR in their ABI_64 slots.
	 *
	 * At the time we make a sample, we don't know what the consumer
	 * is, so we have to apply bodges both ways to avoid breaking
	 * some binaries.
	 */

Either way:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

>  	if (compat_user_mode(regs)) {
>  		if ((u32)idx == PERF_REG_ARM64_SP)
>  			return regs->compat_sp;
>  		if ((u32)idx == PERF_REG_ARM64_LR)
>  			return regs->compat_lr;
> +		if (idx == 15)
> +			return regs->pc;
>  	}
>  
>  	if ((u32)idx == PERF_REG_ARM64_SP)
> -- 
> 2.27.0.111.gc72c7da667-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode

[Mark Rutland]

On Tue, Jun 23, 2020 at 06:44:56PM +0100, Will Deacon wrote:
> On Tue, Jun 23, 2020 at 06:19:10PM +0100, Will Deacon wrote:
> > So, I think we should take this patch (which puts the PC where you'd expect
> > to find it for compat tasks) and then we could consider removing the current
> > lr/sp fudging as a separate patch, which we could revert if it causes a
> > problem. However, I'm not sure I want to open that up.
> 
> Patch below...
> 
> Will
> 
> --->8
> 
> From 7452148b87ed8c82826474366dbe536fd960d3a7 Mon Sep 17 00:00:00 2001
> From: Jiping Ma <jiping.ma2@windriver.com>
> Date: Mon, 11 May 2020 10:52:07 +0800
> Subject: [PATCH] arm64: perf: Report the PC value in REGS_ABI_32 mode
> 
> A 32-bit perf querying the registers of a compat task using REGS_ABI_32
> will receive zeroes from w15, when it expects to find the PC.
> 
> Return the PC value for register dwarf register 15 when returning register
> values for a compat task to perf.
> 
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> Link: https://lore.kernel.org/r/1589165527-188401-1-git-send-email-jiping.ma2@windriver.com
> [will: Shuffled code and added a comment]
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/perf_regs.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
> index 0bbac612146e..952b26a05d0f 100644
> --- a/arch/arm64/kernel/perf_regs.c
> +++ b/arch/arm64/kernel/perf_regs.c
> @@ -15,15 +15,25 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  		return 0;
>  
>  	/*
> -	 * Compat (i.e. 32 bit) mode:
> -	 * - PC has been set in the pt_regs struct in kernel_entry,
> -	 * - Handle SP and LR here.
> +	 * Our handling of compat tasks (PERF_SAMPLE_REGS_ABI_32) is weird. For
> +	 * a 32-bit perf inspecting a 32-bit task, then it will look at the
> +	 * first 16 registers. These correspond directly to the registers saved
> +	 * in our pt_regs structure, with the exception of the PC, so we copy
> +	 * that down (x15 corresponds to SP_hyp in the architecture). So far, so
> +	 * good. The oddity arises when a 64-bit perf looks at a 32-bit task and
> +	 * asks for registers beyond PERF_REG_ARM_MAX. In this case, we return
> +	 * SP_usr, LR_usr and PC in the positions where the AArch64 registers
> +	 * would normally live. The initial idea was to allow a 64-bit unwinder
> +	 * to unwinder a 32-bit task and, although it's not clear how well that
> +	 * works in practice, we're kind of stuck with this interface now.
>  	 */

Would you be happy with:

	/*
	 * For ABI reasons, PERF_SAMPLE_REGS_ABI_32 is messy.
	 *
	 * 32-bit consumers of the regs expect this to look like the
	 * native 32-bit layout with entries 0-12 being r0-r12, 13 being
	 * the SP, 14 being the LR, and 15 being the PC. The compat SP
	 * and LR are placed in x13 and x14 respectively upon an
	 * exception, but we need to copy the PC into the expected slot.
	 * Ideally the other slots would all be zeroed to match native
	 * 32-bit, but we can't do this because of existing 64-bit
	 * consumers.
	 *
	 * Existing 64-bit consumers assume that the PC, LR, and SP are
	 * in the same positions as the PERF_SAMPLE_REGS_ABI_64 layout,
	 * rather than interpreting PERF_SAMPLE_REGS_ABI_32 the same as
	 * the native 32-bit PERF_SAMPLE_REGS_ABI_32. To not break these
	 * we must copy the PC into their ABI_64 slots, and leave copies
	 * of the SP and LR in their ABI_64 slots.
	 *
	 * At the time we make a sample, we don't know what the consumer
	 * is, so we have to apply bodges both ways to avoid breaking
	 * some binaries.
	 */

Either way:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

>  	if (compat_user_mode(regs)) {
>  		if ((u32)idx == PERF_REG_ARM64_SP)
>  			return regs->compat_sp;
>  		if ((u32)idx == PERF_REG_ARM64_LR)
>  			return regs->compat_lr;
> +		if (idx == 15)
> +			return regs->pc;
>  	}
>  
>  	if ((u32)idx == PERF_REG_ARM64_SP)
> -- 
> 2.27.0.111.gc72c7da667-goog
>