Received-SPF: pass (google.com: domain of linux-kernel+bounces-166515-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 2 May 2024 10:41:14 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: linux-kernel@vger.kernel.org, "Chang S. Bae" <chang.seok.bae@intel.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Nikolay Borisov <nik.borisov@suse.com>,
	Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org,
	Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
	Ian Rogers <irogers@google.com>, linux-perf-users@vger.kernel.org
Subject: Re: [PATCH 02/10] x86/insn: Fix PUSH instruction in x86 instruction
 decoder opcode map
Message-ID: <ZjOX-mE_CUOisltJ@x1>
References: <20240502105853.5338-1-adrian.hunter@intel.com>
 <20240502105853.5338-3-adrian.hunter@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240502105853.5338-3-adrian.hunter@intel.com>

On Thu, May 02, 2024 at 01:58:45PM +0300, Adrian Hunter wrote:
> The x86 instruction decoder is used not only for decoding kernel
> instructions. It is also used by perf uprobes (user space probes) and by
> perf tools Intel Processor Trace decoding. Consequently, it needs to
> support instructions executed by user space also.

I wonder if we should do it this way, i.e. updating both tools/ and
kernel source code in the same patch.

I think the best is to update the kernel bits, then, after that is
merged, do the tooling part.

To avoid possible, yet unlikely, clashes in linux-next, for instance.

- Arnaldo
 
> Opcode 0x68 PUSH instruction is currently defined as 64-bit operand size
> only i.e. (d64). That was based on Intel SDM Opcode Map. However that is
> contradicted by the Instruction Set Reference section for PUSH in the
> same manual.
> 
> Remove 64-bit operand size only annotation from opcode 0x68 PUSH
> instruction.
> 
> Example:
> 
>   $ cat pushw.s
>   .global  _start
>   .text
>   _start:
>           pushw   $0x1234
>           mov     $0x1,%eax   # system call number (sys_exit)
>           int     $0x80
>   $ as -o pushw.o pushw.s
>   $ ld -s -o pushw pushw.o
>   $ objdump -d pushw | tail -4
>   0000000000401000 <.text>:
>     401000:       66 68 34 12             pushw  $0x1234
>     401004:       b8 01 00 00 00          mov    $0x1,%eax
>     401009:       cd 80                   int    $0x80
>   $ perf record -e intel_pt//u ./pushw
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.014 MB perf.data ]
> 
>  Before:
> 
>   $ perf script --insn-trace=disasm
>   Warning:
>   1 instruction trace errors
>            pushw   10349 [000] 10586.869237014:            401000 [unknown] (/home/ahunter/git/misc/rtit-tests/pushw)           pushw $0x1234
>            pushw   10349 [000] 10586.869237014:            401006 [unknown] (/home/ahunter/git/misc/rtit-tests/pushw)           addb %al, (%rax)
>            pushw   10349 [000] 10586.869237014:            401008 [unknown] (/home/ahunter/git/misc/rtit-tests/pushw)           addb %cl, %ch
>            pushw   10349 [000] 10586.869237014:            40100a [unknown] (/home/ahunter/git/misc/rtit-tests/pushw)           addb $0x2e, (%rax)
>    instruction trace error type 1 time 10586.869237224 cpu 0 pid 10349 tid 10349 ip 0x40100d code 6: Trace doesn't match instruction
> 
>  After:
> 
>   $ perf script --insn-trace=disasm
>              pushw   10349 [000] 10586.869237014:            401000 [unknown] (./pushw)           pushw $0x1234
>              pushw   10349 [000] 10586.869237014:            401004 [unknown] (./pushw)           movl $1, %eax
> 
> Fixes: eb13296cfaf6 ("x86: Instruction decoder API")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  arch/x86/lib/x86-opcode-map.txt       | 2 +-
>  tools/arch/x86/lib/x86-opcode-map.txt | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/lib/x86-opcode-map.txt b/arch/x86/lib/x86-opcode-map.txt
> index c94988d5130d..4ea2e6adb477 100644
> --- a/arch/x86/lib/x86-opcode-map.txt
> +++ b/arch/x86/lib/x86-opcode-map.txt
> @@ -148,7 +148,7 @@ AVXcode:
>  65: SEG=GS (Prefix)
>  66: Operand-Size (Prefix)
>  67: Address-Size (Prefix)
> -68: PUSH Iz (d64)
> +68: PUSH Iz
>  69: IMUL Gv,Ev,Iz
>  6a: PUSH Ib (d64)
>  6b: IMUL Gv,Ev,Ib
> diff --git a/tools/arch/x86/lib/x86-opcode-map.txt b/tools/arch/x86/lib/x86-opcode-map.txt
> index c94988d5130d..4ea2e6adb477 100644
> --- a/tools/arch/x86/lib/x86-opcode-map.txt
> +++ b/tools/arch/x86/lib/x86-opcode-map.txt
> @@ -148,7 +148,7 @@ AVXcode:
>  65: SEG=GS (Prefix)
>  66: Operand-Size (Prefix)
>  67: Address-Size (Prefix)
> -68: PUSH Iz (d64)
> +68: PUSH Iz
>  69: IMUL Gv,Ev,Iz
>  6a: PUSH Ib (d64)
>  6b: IMUL Gv,Ev,Ib
> -- 
> 2.34.1