| Message ID | 1466140958-1928-1-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 <ynorov@caviumnetworks.com> > Cc: Yury Norov <ynorov@caviumnetworks.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Kan Liang <kan.liang@intel.com> > Cc: Wang Nan <wangnan0@huawei.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Madhavan Srinivasan <maddy@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 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 thanks, jirka > + for_each_set_bit(rid, _mask, sizeof(mask) * 8) { > u64 val = regs[i++]; > > printf(".... %-5s 0x%" PRIx64 "\n", > -- > 1.9.1 >
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 <ynorov@caviumnetworks.com> >> Cc: Yury Norov <ynorov@caviumnetworks.com> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >> Cc: Jiri Olsa <jolsa@kernel.org> >> Cc: Adrian Hunter <adrian.hunter@intel.com> >> Cc: Kan Liang <kan.liang@intel.com> >> Cc: Wang Nan <wangnan0@huawei.com> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Madhavan Srinivasan <maddy@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 >>
On Fri, Jun 17, 2016 at 02:43:31PM +0530, Madhavan Srinivasan wrote: SNIP > > > 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? hum, sample data should be swap at this point already.. see perf_session__process_event jirka
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; + + for_each_set_bit(rid, _mask, sizeof(mask) * 8) { u64 val = regs[i++]; printf(".... %-5s 0x%" PRIx64 "\n",
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 <ynorov@caviumnetworks.com> Cc: Yury Norov <ynorov@caviumnetworks.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: Kan Liang <kan.liang@intel.com> Cc: Wang Nan <wangnan0@huawei.com> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- tools/perf/util/session.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)