Messages in this thread | | | Subject | Re: [PATCH][V3] arm64: perf: Get the wrong PC value in REGS_ABI_32 mode | From | Jiping Ma <> | Date | Thu, 28 May 2020 09:06:07 +0800 |
| |
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. >
| |