Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1216203 > unrolled thread

[PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Adrian Hunter <adrian.hunter@intel.com> - 2015-08-31 16:10 +0200
    [PATCH 4/4] x86/insn: perf tools: Add new memory instructions Adrian Hunter <adrian.hunter@intel.com> - 2015-08-31 16:10 +0200
    Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-08-31 16:50 +0200
    Re: [PATCH 1/4] perf tools: Add a test for decoding of new x86 instructions Adrian Hunter <adrian.hunter@intel.com> - 2015-09-01 10:30 +0200
      RE: [PATCH 1/4] perf tools: Add a test for decoding of new x86  instructions 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-09-01 13:10 +0200
    Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Ingo Molnar <mingo@kernel.org> - 2015-09-01 11:00 +0200
      RE: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-09-01 13:40 +0200
        Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Adrian Hunter <adrian.hunter@intel.com> - 2015-09-01 14:20 +0200
          Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Ingo Molnar <mingo@kernel.org> - 2015-09-01 15:00 +0200
          RE: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-09-01 17:20 +0200
      Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Adrian Hunter <adrian.hunter@intel.com> - 2015-09-01 14:20 +0200
        Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Jiri Olsa <jolsa@redhat.com> - 2015-09-01 16:00 +0200
          Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-01 17:00 +0200
        Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-01 16:00 +0200
        Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-01 22:00 +0200
          Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Jiri Olsa <jolsa@redhat.com> - 2015-09-02 08:00 +0200
            RE: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-09-02 08:50 +0200
              Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Ingo Molnar <mingo@kernel.org> - 2015-09-02 09:40 +0200
                RE: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-09-02 12:30 +0200

#1216203 — [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions

Hi

perf tools has a copy of the x86 instruction decoder for decoding
Intel PT.  This patch set adds a perf tools test to use it to
test new instructions.  Subsequent patches add a few new x86
instructions, or very slightly modify them in the case of MPX.
Those changes affect both perf tools and x86/insn.

I suggest Arnaldo takes all these patches as they mainly affect
perf tools, at least in terms of lines-of-code.


Adrian Hunter (4):
      perf tools: Add a test for decoding of new x86 instructions
      x86/insn: perf tools: Pedantically tweak opcode map for MPX instructions
      x86/insn: perf tools: Add new SHA instructions
      x86/insn: perf tools: Add new memory instructions

 arch/x86/lib/x86-opcode-map.txt                    |  19 +-
 tools/perf/tests/Build                             |   3 +
 tools/perf/tests/builtin-test.c                    |   8 +
 tools/perf/tests/gen-insn-x86-dat.awk              |  75 ++
 tools/perf/tests/gen-insn-x86-dat.sh               |  43 ++
 tools/perf/tests/insn-x86-dat-32.c                 | 640 ++++++++++++++++
 tools/perf/tests/insn-x86-dat-64.c                 | 738 ++++++++++++++++++
 tools/perf/tests/insn-x86-dat-src.c                | 835 +++++++++++++++++++++
 tools/perf/tests/insn-x86.c                        | 180 +++++
 tools/perf/tests/tests.h                           |   1 +
 .../perf/util/intel-pt-decoder/x86-opcode-map.txt  |  19 +-
 11 files changed, 2553 insertions(+), 8 deletions(-)
 create mode 100644 tools/perf/tests/gen-insn-x86-dat.awk
 create mode 100755 tools/perf/tests/gen-insn-x86-dat.sh
 create mode 100644 tools/perf/tests/insn-x86-dat-32.c
 create mode 100644 tools/perf/tests/insn-x86-dat-64.c
 create mode 100644 tools/perf/tests/insn-x86-dat-src.c
 create mode 100644 tools/perf/tests/insn-x86.c


Regards
Adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1216206 — [PATCH 4/4] x86/insn: perf tools: Add new memory instructions

Intel Architecture Instruction Set Extensions Programing
Reference (Oct 2014) describes 3 new memory instructions,
namely clflushopt, clwb and pcommit.  Add them to the op
code map and the perf tools new instructions test. e.g.

    $ tools/perf/perf test list 2>&1 | grep "x86 ins"
    39: Test x86 instruction decoder - new instructions
    $ tools/perf/perf test 39
    39: Test x86 instruction decoder - new instructions          : Ok

Or to see the details:

    $ tools/perf/perf test -v 39

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 arch/x86/lib/x86-opcode-map.txt                    |  4 +-
 tools/perf/tests/insn-x86-dat-32.c                 | 22 +++++++++++
 tools/perf/tests/insn-x86-dat-64.c                 | 34 ++++++++++++++++
 tools/perf/tests/insn-x86-dat-src.c                | 46 ++++++++++++++++++++++
 .../perf/util/intel-pt-decoder/x86-opcode-map.txt  |  4 +-
 5 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/arch/x86/lib/x86-opcode-map.txt b/arch/x86/lib/x86-opcode-map.txt
index 25dad388b371..f4f0451a301e 100644
--- a/arch/x86/lib/x86-opcode-map.txt
+++ b/arch/x86/lib/x86-opcode-map.txt
@@ -943,8 +943,8 @@ GrpTable: Grp15
 3: vstmxcsr Md (v1) | WRGSBASE Ry (F3),(11B)
 4: XSAVE
 5: XRSTOR | lfence (11B)
-6: XSAVEOPT | mfence (11B)
-7: clflush | sfence (11B)
+6: XSAVEOPT | clwb (66) | mfence (11B)
+7: clflush | clflushopt (66) | sfence (11B) | pcommit (66),(11B)
 EndTable
 
 GrpTable: Grp16
diff --git a/tools/perf/tests/insn-x86-dat-32.c b/tools/perf/tests/insn-x86-dat-32.c
index 83f5078e74e1..4b09b7e130a0 100644
--- a/tools/perf/tests/insn-x86-dat-32.c
+++ b/tools/perf/tests/insn-x86-dat-32.c
@@ -616,3 +616,25 @@
 "0f 38 cd 84 08 78 56 34 12 \tsha256msg2 0x12345678(%eax,%ecx,1),%xmm0",},
 {{0x0f, 0x38, 0xcd, 0x84, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 9, 0, "", "",
 "0f 38 cd 84 c8 78 56 34 12 \tsha256msg2 0x12345678(%eax,%ecx,8),%xmm0",},
+{{0x66, 0x0f, 0xae, 0x38, }, 4, 0, "", "",
+"66 0f ae 38          \tclflushopt (%eax)",},
+{{0x66, 0x0f, 0xae, 0x3d, 0x78, 0x56, 0x34, 0x12, }, 8, 0, "", "",
+"66 0f ae 3d 78 56 34 12 \tclflushopt 0x12345678",},
+{{0x66, 0x0f, 0xae, 0xbc, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 9, 0, "", "",
+"66 0f ae bc c8 78 56 34 12 \tclflushopt 0x12345678(%eax,%ecx,8)",},
+{{0x0f, 0xae, 0x38, }, 3, 0, "", "",
+"0f ae 38             \tclflush (%eax)",},
+{{0x0f, 0xae, 0xf8, }, 3, 0, "", "",
+"0f ae f8             \tsfence ",},
+{{0x66, 0x0f, 0xae, 0x30, }, 4, 0, "", "",
+"66 0f ae 30          \tclwb   (%eax)",},
+{{0x66, 0x0f, 0xae, 0x35, 0x78, 0x56, 0x34, 0x12, }, 8, 0, "", "",
+"66 0f ae 35 78 56 34 12 \tclwb   0x12345678",},
+{{0x66, 0x0f, 0xae, 0xb4, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 9, 0, "", "",
+"66 0f ae b4 c8 78 56 34 12 \tclwb   0x12345678(%eax,%ecx,8)",},
+{{0x0f, 0xae, 0x30, }, 3, 0, "", "",
+"0f ae 30             \txsaveopt (%eax)",},
+{{0x0f, 0xae, 0xf0, }, 3, 0, "", "",
+"0f ae f0             \tmfence ",},
+{{0x66, 0x0f, 0xae, 0xf8, }, 4, 0, "", "",
+"66 0f ae f8          \tpcommit ",},
diff --git a/tools/perf/tests/insn-x86-dat-64.c b/tools/perf/tests/insn-x86-dat-64.c
index 13f008588590..5da235a4414f 100644
--- a/tools/perf/tests/insn-x86-dat-64.c
+++ b/tools/perf/tests/insn-x86-dat-64.c
@@ -702,3 +702,37 @@
 "0f 38 cd 84 c8 78 56 34 12 \tsha256msg2 0x12345678(%rax,%rcx,8),%xmm0",},
 {{0x44, 0x0f, 0x38, 0xcd, 0xbc, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 10, 0, "", "",
 "44 0f 38 cd bc c8 78 56 34 12 \tsha256msg2 0x12345678(%rax,%rcx,8),%xmm15",},
+{{0x66, 0x0f, 0xae, 0x38, }, 4, 0, "", "",
+"66 0f ae 38          \tclflushopt (%rax)",},
+{{0x66, 0x41, 0x0f, 0xae, 0x38, }, 5, 0, "", "",
+"66 41 0f ae 38       \tclflushopt (%r8)",},
+{{0x66, 0x0f, 0xae, 0x3c, 0x25, 0x78, 0x56, 0x34, 0x12, }, 9, 0, "", "",
+"66 0f ae 3c 25 78 56 34 12 \tclflushopt 0x12345678",},
+{{0x66, 0x0f, 0xae, 0xbc, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 9, 0, "", "",
+"66 0f ae bc c8 78 56 34 12 \tclflushopt 0x12345678(%rax,%rcx,8)",},
+{{0x66, 0x41, 0x0f, 0xae, 0xbc, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 10, 0, "", "",
+"66 41 0f ae bc c8 78 56 34 12 \tclflushopt 0x12345678(%r8,%rcx,8)",},
+{{0x0f, 0xae, 0x38, }, 3, 0, "", "",
+"0f ae 38             \tclflush (%rax)",},
+{{0x41, 0x0f, 0xae, 0x38, }, 4, 0, "", "",
+"41 0f ae 38          \tclflush (%r8)",},
+{{0x0f, 0xae, 0xf8, }, 3, 0, "", "",
+"0f ae f8             \tsfence ",},
+{{0x66, 0x0f, 0xae, 0x30, }, 4, 0, "", "",
+"66 0f ae 30          \tclwb   (%rax)",},
+{{0x66, 0x41, 0x0f, 0xae, 0x30, }, 5, 0, "", "",
+"66 41 0f ae 30       \tclwb   (%r8)",},
+{{0x66, 0x0f, 0xae, 0x34, 0x25, 0x78, 0x56, 0x34, 0x12, }, 9, 0, "", "",
+"66 0f ae 34 25 78 56 34 12 \tclwb   0x12345678",},
+{{0x66, 0x0f, 0xae, 0xb4, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 9, 0, "", "",
+"66 0f ae b4 c8 78 56 34 12 \tclwb   0x12345678(%rax,%rcx,8)",},
+{{0x66, 0x41, 0x0f, 0xae, 0xb4, 0xc8, 0x78, 0x56, 0x34, 0x12, }, 10, 0, "", "",
+"66 41 0f ae b4 c8 78 56 34 12 \tclwb   0x12345678(%r8,%rcx,8)",},
+{{0x0f, 0xae, 0x30, }, 3, 0, "", "",
+"0f ae 30             \txsaveopt (%rax)",},
+{{0x41, 0x0f, 0xae, 0x30, }, 4, 0, "", "",
+"41 0f ae 30          \txsaveopt (%r8)",},
+{{0x0f, 0xae, 0xf0, }, 3, 0, "", "",
+"0f ae f0             \tmfence ",},
+{{0x66, 0x0f, 0xae, 0xf8, }, 4, 0, "", "",
+"66 0f ae f8          \tpcommit ",},
diff --git a/tools/perf/tests/insn-x86-dat-src.c b/tools/perf/tests/insn-x86-dat-src.c
index 7d06c9b22070..482637f44245 100644
--- a/tools/perf/tests/insn-x86-dat-src.c
+++ b/tools/perf/tests/insn-x86-dat-src.c
@@ -421,6 +421,30 @@ int main(void)
 	asm volatile("sha256msg2 0x12345678(%rax,%rcx,8), %xmm0");
 	asm volatile("sha256msg2 0x12345678(%rax,%rcx,8), %xmm15");
 
+	/* clflushopt m8 */
+
+	asm volatile("clflushopt (%rax)");
+	asm volatile("clflushopt (%r8)");
+	asm volatile("clflushopt (0x12345678)");
+	asm volatile("clflushopt 0x12345678(%rax,%rcx,8)");
+	asm volatile("clflushopt 0x12345678(%r8,%rcx,8)");
+	/* Also check instructions in the same group encoding as clflushopt */
+	asm volatile("clflush (%rax)");
+	asm volatile("clflush (%r8)");
+	asm volatile("sfence");
+
+	/* clwb m8 */
+
+	asm volatile("clwb (%rax)");
+	asm volatile("clwb (%r8)");
+	asm volatile("clwb (0x12345678)");
+	asm volatile("clwb 0x12345678(%rax,%rcx,8)");
+	asm volatile("clwb 0x12345678(%r8,%rcx,8)");
+	/* Also check instructions in the same group encoding as clwb */
+	asm volatile("xsaveopt (%rax)");
+	asm volatile("xsaveopt (%r8)");
+	asm volatile("mfence");
+
 #else  /* #ifdef __x86_64__ */
 
 	/* bndmk m32, bnd */
@@ -780,8 +804,30 @@ int main(void)
 	asm volatile("sha256msg2 0x12345678(%eax,%ecx,1), %xmm0");
 	asm volatile("sha256msg2 0x12345678(%eax,%ecx,8), %xmm0");
 
+	/* clflushopt m8 */
+
+	asm volatile("clflushopt (%eax)");
+	asm volatile("clflushopt (0x12345678)");
+	asm volatile("clflushopt 0x12345678(%eax,%ecx,8)");
+	/* Also check instructions in the same group encoding as clflushopt */
+	asm volatile("clflush (%eax)");
+	asm volatile("sfence");
+
+	/* clwb m8 */
+
+	asm volatile("clwb (%eax)");
+	asm volatile("clwb (0x12345678)");
+	asm volatile("clwb 0x12345678(%eax,%ecx,8)");
+	/* Also check instructions in the same group encoding as clwb */
+	asm volatile("xsaveopt (%eax)");
+	asm volatile("mfence");
+
 #endif /* #ifndef __x86_64__ */
 
+	/* pcommit */
+
+	asm volatile("pcommit");
+
 	/* Following line is a marker for the awk script - do not change */
 	asm volatile("rdtsc"); /* Stop here */
 
diff --git a/tools/perf/util/intel-pt-decoder/x86-opcode-map.txt b/tools/perf/util/intel-pt-decoder/x86-opcode-map.txt
index 25dad388b371..f4f0451a301e 100644
--- a/tools/perf/util/intel-pt-decoder/x86-opcode-map.txt
+++ b/tools/perf/util/intel-pt-decoder/x86-opcode-map.txt
@@ -943,8 +943,8 @@ GrpTable: Grp15
 3: vstmxcsr Md (v1) | WRGSBASE Ry (F3),(11B)
 4: XSAVE
 5: XRSTOR | lfence (11B)
-6: XSAVEOPT | mfence (11B)
-7: clflush | sfence (11B)
+6: XSAVEOPT | clwb (66) | mfence (11B)
+7: clflush | clflushopt (66) | sfence (11B) | pcommit (66),(11B)
 EndTable
 
 GrpTable: Grp16
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216233

Em Mon, Aug 31, 2015 at 04:58:38PM +0300, Adrian Hunter escreveu:
> Hi
> 
> perf tools has a copy of the x86 instruction decoder for decoding
> Intel PT.  This patch set adds a perf tools test to use it to
> test new instructions.  Subsequent patches add a few new x86
> instructions, or very slightly modify them in the case of MPX.
> Those changes affect both perf tools and x86/insn.
> 
> I suggest Arnaldo takes all these patches as they mainly affect
> perf tools, at least in terms of lines-of-code.

I'll process them, anyone thinking this shouldn't be the case, holler.

- Arnaldo
 
> 
> Adrian Hunter (4):
>       perf tools: Add a test for decoding of new x86 instructions
>       x86/insn: perf tools: Pedantically tweak opcode map for MPX instructions
>       x86/insn: perf tools: Add new SHA instructions
>       x86/insn: perf tools: Add new memory instructions
> 
>  arch/x86/lib/x86-opcode-map.txt                    |  19 +-
>  tools/perf/tests/Build                             |   3 +
>  tools/perf/tests/builtin-test.c                    |   8 +
>  tools/perf/tests/gen-insn-x86-dat.awk              |  75 ++
>  tools/perf/tests/gen-insn-x86-dat.sh               |  43 ++
>  tools/perf/tests/insn-x86-dat-32.c                 | 640 ++++++++++++++++
>  tools/perf/tests/insn-x86-dat-64.c                 | 738 ++++++++++++++++++
>  tools/perf/tests/insn-x86-dat-src.c                | 835 +++++++++++++++++++++
>  tools/perf/tests/insn-x86.c                        | 180 +++++
>  tools/perf/tests/tests.h                           |   1 +
>  .../perf/util/intel-pt-decoder/x86-opcode-map.txt  |  19 +-
>  11 files changed, 2553 insertions(+), 8 deletions(-)
>  create mode 100644 tools/perf/tests/gen-insn-x86-dat.awk
>  create mode 100755 tools/perf/tests/gen-insn-x86-dat.sh
>  create mode 100644 tools/perf/tests/insn-x86-dat-32.c
>  create mode 100644 tools/perf/tests/insn-x86-dat-64.c
>  create mode 100644 tools/perf/tests/insn-x86-dat-src.c
>  create mode 100644 tools/perf/tests/insn-x86.c
> 
> 
> Regards
> Adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216674 — Re: [PATCH 1/4] perf tools: Add a test for decoding of new x86 instructions

On 01/09/15 03:18, 平松雅巳 / HIRAMATU，MASAMI wrote:
>> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
>>
>> Add a new test titled:
>>
>> 	Test x86 instruction decoder - new instructions
>>
>> The purpose of this test is to check the instruction decoder
>> after new instructions have been added.  Initially, MPX
>> instructions are tested which are already supported, but the
>> definitions in x86-opcode-map.txt will be tweaked in a
>> subsequent patch, after which this test can be run to verify
>> those changes.
> 
> Hmm, btw, why should this test in perf? It seems that we need
> this test in kselftest or build-time selftest.
> I prefer to put this in arch/x86/tools/ or lib/. What would you
> think ?

There are 2 reasons perf tools needs a test:
	1. perf tools is source code independent from the kernel i.e. it has its
own copy of the instruction decoder.
	2. perf tools test also tests the Intel PT decoder's categorization of
instructions.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216775 — RE: [PATCH 1/4] perf tools: Add a test for decoding of new x86 instructions

PiBGcm9tOiBBZHJpYW4gSHVudGVyIFttYWlsdG86YWRyaWFuLmh1bnRlckBpbnRlbC5jb21dDQo+
IE9uIDAxLzA5LzE1IDAzOjE4LCDlubPmnb7pm4Xlt7MgLyBISVJBTUFUVe+8jE1BU0FNSSB3cm90
ZToNCj4gPj4gRnJvbTogQWRyaWFuIEh1bnRlciBbbWFpbHRvOmFkcmlhbi5odW50ZXJAaW50ZWwu
Y29tXQ0KPiA+Pg0KPiA+PiBBZGQgYSBuZXcgdGVzdCB0aXRsZWQ6DQo+ID4+DQo+ID4+IAlUZXN0
IHg4NiBpbnN0cnVjdGlvbiBkZWNvZGVyIC0gbmV3IGluc3RydWN0aW9ucw0KPiA+Pg0KPiA+PiBU
aGUgcHVycG9zZSBvZiB0aGlzIHRlc3QgaXMgdG8gY2hlY2sgdGhlIGluc3RydWN0aW9uIGRlY29k
ZXINCj4gPj4gYWZ0ZXIgbmV3IGluc3RydWN0aW9ucyBoYXZlIGJlZW4gYWRkZWQuICBJbml0aWFs
bHksIE1QWA0KPiA+PiBpbnN0cnVjdGlvbnMgYXJlIHRlc3RlZCB3aGljaCBhcmUgYWxyZWFkeSBz
dXBwb3J0ZWQsIGJ1dCB0aGUNCj4gPj4gZGVmaW5pdGlvbnMgaW4geDg2LW9wY29kZS1tYXAudHh0
IHdpbGwgYmUgdHdlYWtlZCBpbiBhDQo+ID4+IHN1YnNlcXVlbnQgcGF0Y2gsIGFmdGVyIHdoaWNo
IHRoaXMgdGVzdCBjYW4gYmUgcnVuIHRvIHZlcmlmeQ0KPiA+PiB0aG9zZSBjaGFuZ2VzLg0KPiA+
DQo+ID4gSG1tLCBidHcsIHdoeSBzaG91bGQgdGhpcyB0ZXN0IGluIHBlcmY/IEl0IHNlZW1zIHRo
YXQgd2UgbmVlZA0KPiA+IHRoaXMgdGVzdCBpbiBrc2VsZnRlc3Qgb3IgYnVpbGQtdGltZSBzZWxm
dGVzdC4NCj4gPiBJIHByZWZlciB0byBwdXQgdGhpcyBpbiBhcmNoL3g4Ni90b29scy8gb3IgbGli
Ly4gV2hhdCB3b3VsZCB5b3UNCj4gPiB0aGluayA/DQo+IA0KPiBUaGVyZSBhcmUgMiByZWFzb25z
IHBlcmYgdG9vbHMgbmVlZHMgYSB0ZXN0Og0KPiAJMS4gcGVyZiB0b29scyBpcyBzb3VyY2UgY29k
ZSBpbmRlcGVuZGVudCBmcm9tIHRoZSBrZXJuZWwgaS5lLiBpdCBoYXMgaXRzDQo+IG93biBjb3B5
IG9mIHRoZSBpbnN0cnVjdGlvbiBkZWNvZGVyLg0KPiAJMi4gcGVyZiB0b29scyB0ZXN0IGFsc28g
dGVzdHMgdGhlIEludGVsIFBUIGRlY29kZXIncyBjYXRlZ29yaXphdGlvbiBvZg0KPiBpbnN0cnVj
dGlvbnMuDQoNCk9LLCB0aGVuLCBjYW4gSSBwb3J0IHRoaXMgaW5zbiB0ZXN0cyBpbnRvIHRoZSBr
YnVpbGQ/IEknZCBsaWtlIHRvIHVzZSB0aGlzLA0KYnV0IGJlY2F1c2Ugb2YgZmluZGluZyBidWdz
IGluIGVhcmx5IHN0YWdlLCBJIHRoaW5rIHNhbWUgdGVzdCBzaG91bGQgYmUNCmRvbmUgaW4gdGhl
IGtlcm5lbCBidWlsZCBwcm9jZXNzIChhcyBhIGtidWlsZCBvcHRpb24pLg0KDQpUaGFuayB5b3Us
DQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216701

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> Hi
> 
> perf tools has a copy of the x86 instruction decoder for decoding
> Intel PT. [...]

So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses 
for kprobes et al - and the two versions already forked slightly:

-#include "inat.h"
-#include "insn.h"
+#include <asm/inat.h>
+#include <asm/insn.h>

it would be nice to add a diff check to the perf build, and (non-fatally) warn 
during the build if the two versions depart from each other?

This will make sure the two versions are fully in sync in the long run as well.

( Alternatively we could perhaps also librarize it into tools/lib/, and teach the 
  kernel build to pick that one up? )

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216786

> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo Molnar
> 
> 
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> > Hi
> >
> > perf tools has a copy of the x86 instruction decoder for decoding
> > Intel PT. [...]
> 
> So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses
> for kprobes et al - and the two versions already forked slightly:
> 
> -#include "inat.h"
> -#include "insn.h"
> +#include <asm/inat.h>
> +#include <asm/insn.h>
> 
> it would be nice to add a diff check to the perf build, and (non-fatally) warn
> during the build if the two versions depart from each other?
> 
> This will make sure the two versions are fully in sync in the long run as well.
> 
> ( Alternatively we could perhaps also librarize it into tools/lib/, and teach the
>   kernel build to pick that one up? )

Agreed, what I concern is that someone finds a bug and fixes one of them and
another is not fixed.

I'll see the forked version and check if it can be merged into the kernel.

Thank you,

> 
> Thanks,
> 
> 	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216809

On 01/09/15 14:38, 平松雅巳 / HIRAMATU，MASAMI wrote:
>> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo Molnar
>>
>>
>> * Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>>> Hi
>>>
>>> perf tools has a copy of the x86 instruction decoder for decoding
>>> Intel PT. [...]
>>
>> So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses
>> for kprobes et al - and the two versions already forked slightly:
>>
>> -#include "inat.h"
>> -#include "insn.h"
>> +#include <asm/inat.h>
>> +#include <asm/insn.h>
>>
>> it would be nice to add a diff check to the perf build, and (non-fatally) warn
>> during the build if the two versions depart from each other?
>>
>> This will make sure the two versions are fully in sync in the long run as well.
>>
>> ( Alternatively we could perhaps also librarize it into tools/lib/, and teach the
>>   kernel build to pick that one up? )
> 
> Agreed, what I concern is that someone finds a bug and fixes one of them and
> another is not fixed.
> 
> I'll see the forked version and check if it can be merged into the kernel.

Ever since Linus complained about perf tools including kernel headers, I
have assumed we should have separate source code.  That email thread was not
cc'ed to a mailing list but here is a quote:

Em Sat, Jul 04, 2015 at 08:53:46AM -0700, Linus Torvalds escreveu:
> So this is more fundamental, and looks like it's just due to perf
> abusing the kernel headers, and now that rbtree has rcu support
> ("rbtree: Make lockless searches non-fatal"), it gets tons of headers
> included that really don't work from user space.
>
> There might be other things going on, but the rbtree one seems to be a
> big one. I think perf needs to get its own rbtree header or something,
> instead of doing that insane "let's include random core kernel
> headers" thing.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216829

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> > Agreed, what I concern is that someone finds a bug and fixes one of them and 
> > another is not fixed.
> > 
> > I'll see the forked version and check if it can be merged into the kernel.
> 
> Ever since Linus complained about perf tools including kernel headers, I have 
> assumed we should have separate source code.  That email thread was not cc'ed to 
> a mailing list but here is a quote:
> 
> Em Sat, Jul 04, 2015 at 08:53:46AM -0700, Linus Torvalds escreveu:
>
> > So this is more fundamental, and looks like it's just due to perf abusing the 
> > kernel headers, and now that rbtree has rcu support ("rbtree: Make lockless 
> > searches non-fatal"), it gets tons of headers included that really don't work 
> > from user space.
> >
> > There might be other things going on, but the rbtree one seems to be a big 
> > one. I think perf needs to get its own rbtree header or something, instead of 
> > doing that insane "let's include random core kernel headers" thing.

Note that even plain copying and occasional back-merges isn't a bad solution: it's 
better than 'messy sharing' of code.

But we can also share code in a bit more organized fashion, and any of the two 
solutions I proposed solve these complications:

 - if we do the diff -u check warning during perf build then the forked versions
   won't stay forked for long. This is the simplest variant.

 - if we librarize this functionality into tools/lib/x86/decode/ (and make sure 
   it's a library that can be linked into the kernel) then we are back to shared 
   code.

The problem wasn't to share code per se, the problem was to share code in a messy 
way, without making it apparent that it's shared code: which made it easy to break 
the tools/perf build via harmless looking kernel side changes.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216924

> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> 
> On 01/09/15 14:38, 平松雅巳 / HIRAMATU，MASAMI wrote:
> >> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo Molnar
> >>
> >>
> >> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >>> Hi
> >>>
> >>> perf tools has a copy of the x86 instruction decoder for decoding
> >>> Intel PT. [...]
> >>
> >> So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses
> >> for kprobes et al - and the two versions already forked slightly:
> >>
> >> -#include "inat.h"
> >> -#include "insn.h"
> >> +#include <asm/inat.h>
> >> +#include <asm/insn.h>
> >>
> >> it would be nice to add a diff check to the perf build, and (non-fatally) warn
> >> during the build if the two versions depart from each other?
> >>
> >> This will make sure the two versions are fully in sync in the long run as well.
> >>
> >> ( Alternatively we could perhaps also librarize it into tools/lib/, and teach the
> >>   kernel build to pick that one up? )
> >
> > Agreed, what I concern is that someone finds a bug and fixes one of them and
> > another is not fixed.
> >
> > I'll see the forked version and check if it can be merged into the kernel.
> 
> Ever since Linus complained about perf tools including kernel headers, I
> have assumed we should have separate source code.  That email thread was not
> cc'ed to a mailing list but here is a quote:
> 
> Em Sat, Jul 04, 2015 at 08:53:46AM -0700, Linus Torvalds escreveu:
> > So this is more fundamental, and looks like it's just due to perf
> > abusing the kernel headers, and now that rbtree has rcu support
> > ("rbtree: Make lockless searches non-fatal"), it gets tons of headers
> > included that really don't work from user space.
> >
> > There might be other things going on, but the rbtree one seems to be a
> > big one. I think perf needs to get its own rbtree header or something,
> > instead of doing that insane "let's include random core kernel
> > headers" thing.


OK, now I see what happened...
Hmm, so at this point, I'll just port the test to arch/x86/tools/, since the
kernel should have that.

Thanks,



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216808

On 01/09/15 11:54, Ingo Molnar wrote:
> 
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
>> Hi
>>
>> perf tools has a copy of the x86 instruction decoder for decoding
>> Intel PT. [...]
> 
> So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses 
> for kprobes et al - and the two versions already forked slightly:
> 
> -#include "inat.h"
> -#include "insn.h"
> +#include <asm/inat.h>
> +#include <asm/insn.h>
> 
> it would be nice to add a diff check to the perf build, and (non-fatally) warn 
> during the build if the two versions depart from each other?

I had a go and came up with this.  Arnaldo, Jiri any comments?

diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
index 240730d682c1..1b8a32de8504 100644
--- a/tools/perf/util/intel-pt-decoder/Build
+++ b/tools/perf/util/intel-pt-decoder/Build
@@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt
 $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
 	@$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
 
-$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
+$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
+	@test -d ../../arch/x86 && (( \
+	diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \
+	diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \
+	diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \
+	diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \
+	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
+	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
+	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
+	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
 
 CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder -Wno-override-init


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216858

On Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter wrote:
> On 01/09/15 11:54, Ingo Molnar wrote:
> > 
> > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> > 
> >> Hi
> >>
> >> perf tools has a copy of the x86 instruction decoder for decoding
> >> Intel PT. [...]
> > 
> > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses 
> > for kprobes et al - and the two versions already forked slightly:
> > 
> > -#include "inat.h"
> > -#include "insn.h"
> > +#include <asm/inat.h>
> > +#include <asm/insn.h>
> > 
> > it would be nice to add a diff check to the perf build, and (non-fatally) warn 
> > during the build if the two versions depart from each other?
> 
> I had a go and came up with this.  Arnaldo, Jiri any comments?
> 
> diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> index 240730d682c1..1b8a32de8504 100644
> --- a/tools/perf/util/intel-pt-decoder/Build
> +++ b/tools/perf/util/intel-pt-decoder/Build
> @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt
>  $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
>  	@$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
>  
> -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +	@test -d ../../arch/x86 && (( \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \
> +	diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \
> +	diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
> +	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
> +	$(call rule_mkdir)
> +	$(call if_changed_dep,cc_o_c)
>  

seems ok, but it might be nicer to have make function for that
so we could use it on other places like rbtree.h

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216904

Em Tue, Sep 01, 2015 at 03:59:16PM +0200, Jiri Olsa escreveu:
> On Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter wrote:
> > On 01/09/15 11:54, Ingo Molnar wrote:
> > > it would be nice to add a diff check to the perf build, and (non-fatally) warn 
> > > during the build if the two versions depart from each other?

> > I had a go and came up with this.  Arnaldo, Jiri any comments?

> > diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> > +	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
> > +	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
> > +	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
> > +	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
> > +	$(call rule_mkdir)
> > +	$(call if_changed_dep,cc_o_c)
 
> seems ok, but it might be nicer to have make function for that
> so we could use it on other places like rbtree.h

That will pose some more hurdles, as there are things like
EXPORT_SYMBOL() and RCU stuff that are ok in the kernel sources, but not
in the tools/ copy...

I.e. fully sharing will put a new burden for kernel developers working
on the to-be-shared code, which is something that is not desired.

I was ok with, hey, tools/ broke because you're sharing code with the
kernel, as probably a tools/ developer would notice that and fix things,
but Linus advised against that.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216860

Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu:
> On 01/09/15 11:54, Ingo Molnar wrote:
> > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> perf tools has a copy of the x86 instruction decoder for decoding
> >> Intel PT. [...]

> > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses 
> > for kprobes et al - and the two versions already forked slightly:

> > -#include "inat.h"
> > -#include "insn.h"
> > +#include <asm/inat.h>
> > +#include <asm/insn.h>

> > it would be nice to add a diff check to the perf build, and (non-fatally) warn 
> > during the build if the two versions depart from each other?
 
> I had a go and came up with this.  Arnaldo, Jiri any comments?

It looks ok, but then, if the people doing the original work, i.e.
Masami, IIRC, manages to make these files something shared, then this
becomes moot, right?

We would go back to sharing stuff with the kernel, but this time around
we would be using something that everybody knows is being shared, which
doesn't elliminates the possibility that at some point changes made with
the kernel in mind would break the tools/ using code.

Perhaps it is better to keep copying what we want and introduce
infrastructure to check for differences and warn us as soon as possible
and do the copy, test if it doesn't break what we use, etc.

I.e. we wouldn't be putting any new burden on the "kernel people", i.e.
the burden of having to check that changed they made doesn't break
tools/ living code, nor any out of the blue breakage on tools/
developers when changes are made on the kernel "side".

I.e. have something like what you did, but not limited to these intel-pt
decoder bits, we share more than that :-)

So, I would apply your patch and move forward, at least these intel-pt
bits would be covered, Ingo?

- Arnaldo
 
> diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> index 240730d682c1..1b8a32de8504 100644
> --- a/tools/perf/util/intel-pt-decoder/Build
> +++ b/tools/perf/util/intel-pt-decoder/Build
> @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt
>  $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
>  	@$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
>  
> -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +	@test -d ../../arch/x86 && (( \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \
> +	diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \
> +	diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
> +	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
> +	$(call rule_mkdir)
> +	$(call if_changed_dep,cc_o_c)
>  
>  CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder -Wno-override-init
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217052

Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu:
> On 01/09/15 11:54, Ingo Molnar wrote:
> > 
> > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> > 
> >> Hi
> >>
> >> perf tools has a copy of the x86 instruction decoder for decoding
> >> Intel PT. [...]
> > 
> > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses 
> > for kprobes et al - and the two versions already forked slightly:
> > 
> > -#include "inat.h"
> > -#include "insn.h"
> > +#include <asm/inat.h>
> > +#include <asm/insn.h>
> > 
> > it would be nice to add a diff check to the perf build, and (non-fatally) warn 
> > during the build if the two versions depart from each other?
> 
> I had a go and came up with this.  Arnaldo, Jiri any comments?

So, I was going to try and merge this series, can you please collect the
Acks by Masami and Jiri and resubmit?

I'd say no need to stop this just to get a build function to use with
this, the test below should do the trick _for this specific instance_
and then, after we get this, you should use it as the initial usecase
for adding the build function, d'accord?

Jiri, are you ok with this?

- Arnaldo
 
> diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> index 240730d682c1..1b8a32de8504 100644
> --- a/tools/perf/util/intel-pt-decoder/Build
> +++ b/tools/perf/util/intel-pt-decoder/Build
> @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt
>  $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
>  	@$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
>  
> -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +	@test -d ../../arch/x86 && (( \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \
> +	diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \
> +	diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
> +	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
> +	$(call rule_mkdir)
> +	$(call if_changed_dep,cc_o_c)
>  
>  CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder -Wno-override-init
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217328

On Tue, Sep 01, 2015 at 04:57:16PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu:
> > On 01/09/15 11:54, Ingo Molnar wrote:
> > > 
> > > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > 
> > >> Hi
> > >>
> > >> perf tools has a copy of the x86 instruction decoder for decoding
> > >> Intel PT. [...]
> > > 
> > > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses 
> > > for kprobes et al - and the two versions already forked slightly:
> > > 
> > > -#include "inat.h"
> > > -#include "insn.h"
> > > +#include <asm/inat.h>
> > > +#include <asm/insn.h>
> > > 
> > > it would be nice to add a diff check to the perf build, and (non-fatally) warn 
> > > during the build if the two versions depart from each other?
> > 
> > I had a go and came up with this.  Arnaldo, Jiri any comments?
> 
> So, I was going to try and merge this series, can you please collect the
> Acks by Masami and Jiri and resubmit?
> 
> I'd say no need to stop this just to get a build function to use with
> this, the test below should do the trick _for this specific instance_
> and then, after we get this, you should use it as the initial usecase
> for adding the build function, d'accord?
> 
> Jiri, are you ok with this?

sure, np you can use my ack

jirka

> 
> - Arnaldo
>  
> > diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> > index 240730d682c1..1b8a32de8504 100644
> > --- a/tools/perf/util/intel-pt-decoder/Build
> > +++ b/tools/perf/util/intel-pt-decoder/Build
> > @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt
> >  $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
> >  	@$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
> >  
> > -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> > +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> > +	@test -d ../../arch/x86 && (( \
> > +	diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \
> > +	diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \
> > +	diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \
> > +	diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \
> > +	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
> > +	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
> > +	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
> > +	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
> > +	$(call rule_mkdir)
> > +	$(call if_changed_dep,cc_o_c)
> >  
> >  CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder -Wno-override-init
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217352

> From: Jiri Olsa [mailto:jolsa@redhat.com]
> 
> On Tue, Sep 01, 2015 at 04:57:16PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu:
> > > On 01/09/15 11:54, Ingo Molnar wrote:
> > > >
> > > > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > >
> > > >> Hi
> > > >>
> > > >> perf tools has a copy of the x86 instruction decoder for decoding
> > > >> Intel PT. [...]
> > > >
> > > > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses
> > > > for kprobes et al - and the two versions already forked slightly:
> > > >
> > > > -#include "inat.h"
> > > > -#include "insn.h"
> > > > +#include <asm/inat.h>
> > > > +#include <asm/insn.h>
> > > >
> > > > it would be nice to add a diff check to the perf build, and (non-fatally) warn
> > > > during the build if the two versions depart from each other?
> > >
> > > I had a go and came up with this.  Arnaldo, Jiri any comments?
> >
> > So, I was going to try and merge this series, can you please collect the
> > Acks by Masami and Jiri and resubmit?
> >
> > I'd say no need to stop this just to get a build function to use with
> > this, the test below should do the trick _for this specific instance_
> > and then, after we get this, you should use it as the initial usecase
> > for adding the build function, d'accord?
> >
> > Jiri, are you ok with this?
> 
> sure, np you can use my ack

I'm also OK for this patch. I just concern that is OK for Adrian too? 
Since this ensures all the copied code should be dead copy (not modified anymore),
if we want a different instruction decoding routine, we have to break the test
anyway.

Thank you,

> 
> jirka
> 
> >
> > - Arnaldo
> >
> > > diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> > > index 240730d682c1..1b8a32de8504 100644
> > > --- a/tools/perf/util/intel-pt-decoder/Build
> > > +++ b/tools/perf/util/intel-pt-decoder/Build
> > > @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt
> > >  $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
> > >  	@$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
> > >
> > > -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c
> $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> > > +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c
> util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> > > +	@test -d ../../arch/x86 && (( \
> > > +	diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \
> > > +	diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \
> > > +	diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \
> > > +	diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \
> > > +	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
> > > +	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
> > > +	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
> > > +	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
> > > +	$(call rule_mkdir)
> > > +	$(call if_changed_dep,cc_o_c)
> > >
> > >  CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder -Wno-override-init
> > >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217376

* 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> wrote:

> > sure, np you can use my ack
> 
> I'm also OK for this patch. I just concern that is OK for Adrian too? 
> Since this ensures all the copied code should be dead copy (not modified anymore),
> if we want a different instruction decoding routine, we have to break the test
> anyway.

So the idea would be to not break anything, only warn in a non-fatal question. 
This protects against unbisectable universes being created via simple git merges 
where updates meet but testing of tooling isn't done.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217501

PiBGcm9tOiBJbmdvIE1vbG5hciBbbWFpbHRvOm1pbmdvLmtlcm5lbC5vcmdAZ21haWwuY29tXSBP
biBCZWhhbGYgT2YgSW5nbyBNb2xuYXINCj4gDQo+ID4gPiBzdXJlLCBucCB5b3UgY2FuIHVzZSBt
eSBhY2sNCj4gPg0KPiA+IEknbSBhbHNvIE9LIGZvciB0aGlzIHBhdGNoLiBJIGp1c3QgY29uY2Vy
biB0aGF0IGlzIE9LIGZvciBBZHJpYW4gdG9vPw0KPiA+IFNpbmNlIHRoaXMgZW5zdXJlcyBhbGwg
dGhlIGNvcGllZCBjb2RlIHNob3VsZCBiZSBkZWFkIGNvcHkgKG5vdCBtb2RpZmllZCBhbnltb3Jl
KSwNCj4gPiBpZiB3ZSB3YW50IGEgZGlmZmVyZW50IGluc3RydWN0aW9uIGRlY29kaW5nIHJvdXRp
bmUsIHdlIGhhdmUgdG8gYnJlYWsgdGhlIHRlc3QNCj4gPiBhbnl3YXkuDQo+IA0KPiBTbyB0aGUg
aWRlYSB3b3VsZCBiZSB0byBub3QgYnJlYWsgYW55dGhpbmcsIG9ubHkgd2FybiBpbiBhIG5vbi1m
YXRhbCBxdWVzdGlvbi4NCj4gVGhpcyBwcm90ZWN0cyBhZ2FpbnN0IHVuYmlzZWN0YWJsZSB1bml2
ZXJzZXMgYmVpbmcgY3JlYXRlZCB2aWEgc2ltcGxlIGdpdCBtZXJnZXMNCj4gd2hlcmUgdXBkYXRl
cyBtZWV0IGJ1dCB0ZXN0aW5nIG9mIHRvb2xpbmcgaXNuJ3QgZG9uZS4NCg0KSSBzZWUsIHNvIEkg
Z2l2ZSBteSBhY2sgOikNCg0KQWNrZWQtYnk6IE1hc2FtaSBIaXJhbWF0c3UgPG1hc2FtaS5oaXJh
bWF0c3UucHRAaGl0YWNoaS5jb20+DQoNClRoYW5rIHlvdSENCg0KPiANCj4gVGhhbmtzLA0KPiAN
Cj4gCUluZ28NCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web