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 reading the wrong word. In find_first_bit(),
one word at a time (based on BITS_PER_LONG) is loaded and
checked for any bit set. In 32bit BE userspace,
BITS_PER_LONG turns out to be 32, and for a mask value of
"0x00000000000000ff", find_first_bit will return 32, instead of 0.
Reason for this is that, value in the word0 is all zeros and value
in word1 is 0xff. Ideally, second word in the mask should be loaded
and searched. Patch swaps the word to look incase of 32bit BE.
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <redacted>
Cc: David Ahern <redacted>
Cc: George Spelvin <redacted>
Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Wang Nan <redacted>
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Madhavan Srinivasan <redacted>
---
tools/lib/find_bit.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
Than you for diagnosing this problem, but I don't think the fix
is correct.
1) It's not clear that all users of _find_next_bit and for_each_set_bit()
want this change.
2) Is your code even correct? I'd think you'd want addr[x ^ 1]. Are you
sure you shpuld be reversing the whole array, and not just the halves of
each 64-bit word?
3) You've now broken the case of 32-bit big-endian kernel.
I think the proper solution is uglier than this. :-(
Hi Madhavan,
On Wed, Jun 15, 2016 at 05:12:53PM +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 reading the wrong word. In find_first_bit(),
one word at a time (based on BITS_PER_LONG) is loaded and
checked for any bit set. In 32bit BE userspace,
BITS_PER_LONG turns out to be 32, and for a mask value of
"0x00000000000000ff", find_first_bit will return 32, instead of 0.
Reason for this is that, value in the word0 is all zeros and value
in word1 is 0xff. Ideally, second word in the mask should be loaded
and searched. Patch swaps the word to look incase of 32bit BE.
I think this is not a problem of find_bit() at all. You have wrong
typecast as the source of problem (tools/perf/util/session.c"):
940 static void regs_dump__printf(u64 mask, u64 *regs)
941 {
942 unsigned rid, i = 0;
943
944 for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
^^^^ Here ^^^^
945 u64 val = regs[i++];
946
947 printf(".... %-5s 0x%" PRIx64 "\n",
948 perf_reg_name(rid), val);
949 }
950 }
But for some reason you change correct find_bit()...
Though proper fix is like this for me:
static void regs_dump__printf(u64 mask, u64 *regs)
{
unsigned rid, i = 0;
unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
_mask[0] = mask & ULONG_MAX;
if (sizeof(mask) > sizeof(unsigned long))
_mask[1] = mask >> BITS_PER_LONG;
for_each_set_bit(rid, _mask, sizeof(mask) * BITS_PER_BYTE) {
u64 val = regs[i++];
printf(".... %-5s 0x%" PRIx64 "\n",
perf_reg_name(rid), val);
}
}
Maybe there already is some macro doing the conversion for you...
Yury.
quoted hunk
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <redacted>
Cc: David Ahern <redacted>
Cc: George Spelvin <redacted>
Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Wang Nan <redacted>
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Madhavan Srinivasan <redacted>
---
tools/lib/find_bit.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
On Wed, Jun 15, 2016 at 10:51:27PM +0300, Yury Norov wrote:
Hi Madhavan,
On Wed, Jun 15, 2016 at 05:12:53PM +0530, Madhavan Srinivasan wrote:
quoted
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 reading the wrong word. In find_first_bit(),
one word at a time (based on BITS_PER_LONG) is loaded and
checked for any bit set. In 32bit BE userspace,
BITS_PER_LONG turns out to be 32, and for a mask value of
"0x00000000000000ff", find_first_bit will return 32, instead of 0.
Reason for this is that, value in the word0 is all zeros and value
in word1 is 0xff. Ideally, second word in the mask should be loaded
and searched. Patch swaps the word to look incase of 32bit BE.
I think this is not a problem of find_bit() at all. You have wrong
typecast as the source of problem (tools/perf/util/session.c"):
940 static void regs_dump__printf(u64 mask, u64 *regs)
941 {
942 unsigned rid, i = 0;
943
944 for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
^^^^ Here ^^^^
945 u64 val = regs[i++];
946
947 printf(".... %-5s 0x%" PRIx64 "\n",
948 perf_reg_name(rid), val);
949 }
950 }
But for some reason you change correct find_bit()...
Though proper fix is like this for me:
static void regs_dump__printf(u64 mask, u64 *regs)
{
unsigned rid, i = 0;
unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
_mask[0] = mask & ULONG_MAX;
if (sizeof(mask) > sizeof(unsigned long))
_mask[1] = mask >> BITS_PER_LONG;
for_each_set_bit(rid, _mask, sizeof(mask) * BITS_PER_BYTE) {
u64 val = regs[i++];
printf(".... %-5s 0x%" PRIx64 "\n",
perf_reg_name(rid), val);
}
}
Maybe there already is some macro doing the conversion for you...
yes it is, cpu_to_le64() is what you want
Yury.
quoted
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <redacted>
Cc: David Ahern <redacted>
Cc: George Spelvin <redacted>
Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Wang Nan <redacted>
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Madhavan Srinivasan <redacted>
---
tools/lib/find_bit.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
From: Wang Nan <redacted>
The cpu_to_le* macros in kernel.h are defined without considering
endianese. This patch includes "byteoder/generic.h" instead to fix the
bug, and removes redundant le64_to_cpu definition in intel-bts.c.
Signed-off-by: Wang Nan <redacted>
Signed-off-by: He Kuang <redacted>
---
tools/include/linux/kernel.h | 5 ++---
tools/perf/util/intel-bts.c | 5 -----
2 files changed, 2 insertions(+), 8 deletions(-)
Em Thu, Jun 16, 2016 at 12:11:04AM +0300, Yury Norov escreveu:
quoted
On Wed, Jun 15, 2016 at 10:51:27PM +0300, Yury Norov wrote:
quoted
Maybe there already is some macro doing the conversion for you...
yes it is, cpu_to_le64() is what you want
Beware that the cpu_to_le64() in tools/perf is bogus, we need to grab a
copy from the kernel sources.
- Arnaldo
[PATCH 1/2] tools include: Sync byteorder/generic.h
[PATCH 2/2] tools include: Fix wrong macro definitions for cpu_to_le*
for big endian
Here're two patches related to this issue, sorry for wrongly sent two more
reduntant mails.
Thank you.
From: Wang Nan <redacted>
This patch copies "include/linux/byteorder/generic.h" to
"tools/include/linux/byteorder/generic.h" to enable other libraries to
use macros in it.
tools/perf/MANIFEST is also updated for 'make perf-*-src-pkg'.
Signed-off-by: Wang Nan <redacted>
Signed-off-by: He Kuang <redacted>
---
tools/include/linux/byteorder/generic.h | 48 +++++++++++++++++++++++++++++++++
tools/perf/MANIFEST | 1 +
2 files changed, 49 insertions(+)
create mode 100644 tools/include/linux/byteorder/generic.h
From: Wang Nan <redacted>
The cpu_to_le* macros in kernel.h are defined without considering
endianese. This patch includes "byteoder/generic.h" instead to fix the
bug, and removes redundant le64_to_cpu definition in intel-bts.c.
Signed-off-by: Wang Nan <redacted>
Signed-off-by: He Kuang <redacted>
---
tools/include/linux/kernel.h | 5 ++---
tools/perf/util/intel-bts.c | 5 -----
2 files changed, 2 insertions(+), 8 deletions(-)
From: Wang Nan <redacted>
The cpu_to_le* macros in kernel.h are defined without considering
endianese. This patch includes "byteoder/generic.h" instead to fix the
bug, and removes redundant le64_to_cpu definition in intel-bts.c.
Signed-off-by: Wang Nan <redacted>
Signed-off-by: He Kuang <redacted>
---
tools/include/linux/kernel.h | 5 ++---
tools/perf/util/intel-bts.c | 5 -----
2 files changed, 2 insertions(+), 8 deletions(-)
From: kbuild test robot <hidden> Date: 2016-06-16 06:23:12
Hi,
[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.7-rc3 next-20160615]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/He-Kuang/tools-include-Fix-wrong-macro-definitions-for-cpu_to_le-for-big-endian/20160616-092924
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from tools/include/linux/list.h:6:0,
from elf.h:23,
from special.h:22,
from special.c:26:
quoted
tools/include/linux/kernel.h:70:37: fatal error: linux/byteorder/generic.h: No such file or directory
compilation terminated.
In file included from tools/include/linux/list.h:6:0,
from elf.h:23,
from builtin-check.c:32:
quoted
tools/include/linux/kernel.h:70:37: fatal error: linux/byteorder/generic.h: No such file or directory
compilation terminated.
quoted
mv: cannot stat 'tools/objtool/.special.o.tmp': No such file or directory
make[4]: *** [tools/objtool/special.o] Error 1
quoted
mv: cannot stat 'tools/objtool/.builtin-check.o.tmp': No such file or directory
make[4]: *** [tools/objtool/builtin-check.o] Error 1
In file included from tools/include/linux/list.h:6:0,
from elf.h:23,
from elf.c:30:
quoted
tools/include/linux/kernel.h:70:37: fatal error: linux/byteorder/generic.h: No such file or directory
compilation terminated.
quoted
mv: cannot stat 'tools/objtool/.elf.o.tmp': No such file or directory
make[4]: *** [tools/objtool/elf.o] Error 1
In file included from tools/include/linux/list.h:6:0,
from arch/x86/../../elf.h:23,
from arch/x86/decode.c:26:
quoted
tools/include/linux/kernel.h:70:37: fatal error: linux/byteorder/generic.h: No such file or directory
compilation terminated.
quoted
mv: cannot stat 'tools/objtool/arch/x86/.decode.o.tmp': No such file or directory
make[5]: *** [tools/objtool/arch/x86/decode.o] Error 1
make[5]: Target '__build' not remade because of errors.
make[4]: *** [arch/x86] Error 2
make[4]: Target '__build' not remade because of errors.
make[3]: *** [tools/objtool/objtool-in.o] Error 2
make[3]: Target 'all' not remade because of errors.
make[2]: *** [objtool] Error 2
make[1]: *** [tools/objtool] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [sub-make] Error 2
vim +70 tools/include/linux/kernel.h
64 #endif
65
66 /*
67 * Need more care to handle endianness
68 * (Don't use bitmap_copy_le() for now)
69 */
> 70 #include <linux/byteorder/generic.h>
71
72 static inline int
73 vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: Jiri Olsa <hidden> Date: 2016-06-16 06:39:18
On Thu, Jun 16, 2016 at 01:32:09AM +0000, He Kuang wrote:
quoted hunk
From: Wang Nan <redacted>
The cpu_to_le* macros in kernel.h are defined without considering
endianese. This patch includes "byteoder/generic.h" instead to fix the
bug, and removes redundant le64_to_cpu definition in intel-bts.c.
Signed-off-by: Wang Nan <redacted>
Signed-off-by: He Kuang <redacted>
---
tools/include/linux/kernel.h | 5 ++---
tools/perf/util/intel-bts.c | 5 -----
2 files changed, 2 insertions(+), 8 deletions(-)
the purpose of this patchset is to unify these macros right?
there're more conversion defines in:
util/intel-pt-decoder/intel-pt-pkt-decoder.c,
please remove them as well
thanks,
jirka
From: Jiri Olsa <hidden> Date: 2016-06-16 06:39:44
On Thu, Jun 16, 2016 at 01:32:08AM +0000, He Kuang wrote:
From: Wang Nan <redacted>
This patch copies "include/linux/byteorder/generic.h" to
"tools/include/linux/byteorder/generic.h" to enable other libraries to
use macros in it.
it's not the file copied, as the changelog suggest,
just several macros, please fix the changelog
thanks,
jirka
quoted hunk
tools/perf/MANIFEST is also updated for 'make perf-*-src-pkg'.
Signed-off-by: Wang Nan <redacted>
Signed-off-by: He Kuang <redacted>
---
tools/include/linux/byteorder/generic.h | 48 +++++++++++++++++++++++++++++++++
tools/perf/MANIFEST | 1 +
2 files changed, 49 insertions(+)
create mode 100644 tools/include/linux/byteorder/generic.h
On Thursday 16 June 2016 01:21 AM, Yury Norov wrote:
Hi Madhavan,
On Wed, Jun 15, 2016 at 05:12:53PM +0530, Madhavan Srinivasan wrote:
quoted
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 reading the wrong word. In find_first_bit(),
one word at a time (based on BITS_PER_LONG) is loaded and
checked for any bit set. In 32bit BE userspace,
BITS_PER_LONG turns out to be 32, and for a mask value of
"0x00000000000000ff", find_first_bit will return 32, instead of 0.
Reason for this is that, value in the word0 is all zeros and value
in word1 is 0xff. Ideally, second word in the mask should be loaded
and searched. Patch swaps the word to look incase of 32bit BE.
I think this is not a problem of find_bit() at all. You have wrong
typecast as the source of problem (tools/perf/util/session.c"):
940 static void regs_dump__printf(u64 mask, u64 *regs)
941 {
942 unsigned rid, i = 0;
943
944 for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
^^^^ Here ^^^^
945 u64 val = regs[i++];
946
947 printf(".... %-5s 0x%" PRIx64 "\n",
948 perf_reg_name(rid), val);
949 }
950 }
But for some reason you change correct find_bit()...
Though proper fix is like this for me:
static void regs_dump__printf(u64 mask, u64 *regs)
{
unsigned rid, i = 0;
unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
_mask[0] = mask & ULONG_MAX;
if (sizeof(mask) > sizeof(unsigned long))
_mask[1] = mask >> BITS_PER_LONG;
for_each_set_bit(rid, _mask, sizeof(mask) * BITS_PER_BYTE) {
u64 val = regs[i++];
printf(".... %-5s 0x%" PRIx64 "\n",
perf_reg_name(rid), val);
}
}
Maybe there already is some macro doing the conversion for you...
Agreed, but reason for proposing fix in lib side is to avoid conversion
on each case (if any in future).
I will repost the fix as suggested.
Maddy
Yury.
quoted
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <redacted>
Cc: David Ahern <redacted>
Cc: George Spelvin <redacted>
Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Wang Nan <redacted>
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Madhavan Srinivasan <redacted>
---
tools/lib/find_bit.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
Than you for diagnosing this problem, but I don't think the fix
is correct.
1) It's not clear that all users of _find_next_bit and for_each_set_bit()
want this change.
2) Is your code even correct? I'd think you'd want addr[x ^ 1]. Are you
sure you shpuld be reversing the whole array, and not just the halves of
each 64-bit word?
3) You've now broken the case of 32-bit big-endian kernel.
Yes. But looks like we havent hit this case yet. Will post a fix.
Maddy
I think the proper solution is uglier than this. :-(
On Thursday 16 June 2016 02:41 AM, Yury Norov wrote:
On Wed, Jun 15, 2016 at 10:51:27PM +0300, Yury Norov wrote:
quoted
Hi Madhavan,
On Wed, Jun 15, 2016 at 05:12:53PM +0530, Madhavan Srinivasan wrote:
quoted
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 reading the wrong word. In find_first_bit(),
one word at a time (based on BITS_PER_LONG) is loaded and
checked for any bit set. In 32bit BE userspace,
BITS_PER_LONG turns out to be 32, and for a mask value of
"0x00000000000000ff", find_first_bit will return 32, instead of 0.
Reason for this is that, value in the word0 is all zeros and value
in word1 is 0xff. Ideally, second word in the mask should be loaded
and searched. Patch swaps the word to look incase of 32bit BE.
I think this is not a problem of find_bit() at all. You have wrong
typecast as the source of problem (tools/perf/util/session.c"):
940 static void regs_dump__printf(u64 mask, u64 *regs)
941 {
942 unsigned rid, i = 0;
943
944 for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
^^^^ Here ^^^^
945 u64 val = regs[i++];
946
947 printf(".... %-5s 0x%" PRIx64 "\n",
948 perf_reg_name(rid), val);
949 }
950 }
But for some reason you change correct find_bit()...
Though proper fix is like this for me:
static void regs_dump__printf(u64 mask, u64 *regs)
{
unsigned rid, i = 0;
unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
_mask[0] = mask & ULONG_MAX;
if (sizeof(mask) > sizeof(unsigned long))
_mask[1] = mask >> BITS_PER_LONG;
for_each_set_bit(rid, _mask, sizeof(mask) * BITS_PER_BYTE) {
u64 val = regs[i++];
printf(".... %-5s 0x%" PRIx64 "\n",
perf_reg_name(rid), val);
}
}
Maybe there already is some macro doing the conversion for you...
yes it is, cpu_to_le64() is what you want
no wait, on second look, cpu_to_le64() is not right.
Because we will end up swapping within 32bit.
But what you suggested looks to be fine. I can repost
this with one minor tweak, right shift with 32 instead
of BITS_PER_LONG (since I see compiler errors in 64bit).
Maddy
quoted
Yury.
quoted
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <redacted>
Cc: David Ahern <redacted>
Cc: George Spelvin <redacted>
Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Wang Nan <redacted>
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Madhavan Srinivasan <redacted>
---
tools/lib/find_bit.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)