On Friday 17 June 2016 12:07 PM, Jiri Olsa wrote:
> On Fri, Jun 17, 2016 at 10:52:38AM +0530, Madhavan Srinivasan wrote:
>> When decoding the perf_regs mask in regs_dump__printf(),
>> we loop through the mask using find_first_bit and find_next_bit functions.
>> And mask is of type "u64". But "u64" is send as a "unsigned long *" to
>> lib functions along with sizeof().
>>
>> While the exisitng code works fine in most of the case, when using a
>> 32bit perf on a 64bit kernel (Big Endian), we end up reading the wrong word
>> in the u64 mask. Patch to fix the mask in regs_dump__printf().
>>
>> Suggested-by: Yury Norov <yno...@caviumnetworks.com>
>> Cc: Yury Norov <yno...@caviumnetworks.com>
>> Cc: Peter Zijlstra <pet...@infradead.org>
>> Cc: Ingo Molnar <mi...@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <a...@kernel.org>
>> Cc: Alexander Shishkin <alexander.shish...@linux.intel.com>
>> Cc: Jiri Olsa <jo...@kernel.org>
>> Cc: Adrian Hunter <adrian.hun...@intel.com>
>> Cc: Kan Liang <kan.li...@intel.com>
>> Cc: Wang Nan <wangn...@huawei.com>
>> Cc: Michael Ellerman <m...@ellerman.id.au>
>> Signed-off-by: Madhavan Srinivasan <ma...@linux.vnet.ibm.com>
>> ---
>> tools/perf/util/session.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index 5214974e841a..2eaa42a4832a 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -940,8 +940,13 @@ static void branch_stack__printf(struct perf_sample
>> *sample)
>> static void regs_dump__printf(u64 mask, u64 *regs)
>> {
>> unsigned rid, i = 0;
>> + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
>>
>> - for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
>> + _mask[0] = mask & ULONG_MAX;
>> + if (sizeof(mask) > sizeof(unsigned long))
>> + _mask[1] = mask >> 32;
>> +
> I think we should do this earlier when reading the mask,
> not at the moment we just print it, like we do for other
> types.. maybe we could do this as an extra bit for
Ok. I tried something like this. Currently
mask is read in perf_evsel__parse_sample(),
so added for both sample type (PERF_SAMPLE_REGS_USER
and PERF_SAMPLE_REGS_INTR)
if (data->user_regs.abi) {
- u64 mask = evsel->attr.sample_regs_user;
+ u.val64 = evsel->attr.sample_regs_user;
- sz = hweight_long(mask) * sizeof(u64);
+ if (sizeof(u64) > sizeof(unsigned long)) {
+ u64 mask = u.val64;
+ u.val32[1] = mask >> 32;
+ u.val32[0] = mask & ULONG_MAX;
+ }
+
+ sz = hweight_long(u.val64) * sizeof(u64);
OVERFLOW_CHECK(array, sz, max_size);
- data->user_regs.mask = mask;
+ data->user_regs.mask = u.val64;
data->user_regs.regs = (u64 *)array;
Issue I see is when printing the mask value in a 32bit perf
on a 64bit kernel (big endian).
442044948492 0xdc0 [0x188]: PERF_RECORD_SAMPLE(IP, 0x1): 7299/7299:
0xc000000000059200 period: 12599 addr: 0
... intr regs: mask 0xffffffff000007ff ABI 32-bit
^^^ shld have been 0x7ffffffffff
I agree it is better to fix this when reading, but
we need to swap again when printing?
> perf_event__swap_ops[PERF_RECORD_SAMPLE] function
>
> also there's print_sample_iregs in builtin-script.c that's
> most likely affected as well
My bad. Should have seen this too.
Maddy
>
> thanks,
> jirka
>
>> + for_each_set_bit(rid, _mask, sizeof(mask) * 8) {
>> u64 val = regs[i++];
>>
>> printf(".... %-5s 0x%" PRIx64 "\n",
>> --
>> 1.9.1
>>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
