linux-kernel - Re: [PATCH V2 7/9] tools/perf: Update instruction tracking with add instruction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7c8af115-56fb-4b1f-818c-f2a7d532166b@csgroup.eu>
Date: Tue, 7 May 2024 09:58:48 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, "acme@...nel.org"
	<acme@...nel.org>, "jolsa@...nel.org" <jolsa@...nel.org>,
	"adrian.hunter@...el.com" <adrian.hunter@...el.com>, "irogers@...gle.com"
	<irogers@...gle.com>, "namhyung@...nel.org" <namhyung@...nel.org>,
	"segher@...nel.crashing.org" <segher@...nel.crashing.org>
CC: "linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"maddy@...ux.ibm.com" <maddy@...ux.ibm.com>, "kjain@...ux.ibm.com"
	<kjain@...ux.ibm.com>, "disgoel@...ux.vnet.ibm.com"
	<disgoel@...ux.vnet.ibm.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "akanksha@...ux.ibm.com"
	<akanksha@...ux.ibm.com>
Subject: Re: [PATCH V2 7/9] tools/perf: Update instruction tracking with add
 instruction



Le 06/05/2024 à 14:19, Athira Rajeev a écrit :
> Update instruction tracking with add instruction. Apart from "mr"
> instruction, the register state is carried on by other insns, ie,
> "add, addi, addis". Since these are not memory instructions and doesn't
> fall in the range of (32 to 63), add these as part of nmemonic table.
> For now, add* instructions are added. There is possibility of getting
> more added here. But to extract regs, still the binary code will be
> used. So associate this with "load_store_ops" itself and no other
> changes required.

Looks fragile.

How do you handle addc, adde, addic ?
And also addme, addze, which only have two operands instead of 3 ?

> 
> Signed-off-by: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
> ---
>   .../perf/arch/powerpc/annotate/instructions.c | 21 +++++++++++++++++++
>   tools/perf/util/disasm.c                      |  1 +
>   2 files changed, 22 insertions(+)
> 
> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c b/tools/perf/arch/powerpc/annotate/instructions.c
> index cce7023951fe..1f35d8a65bb4 100644
> --- a/tools/perf/arch/powerpc/annotate/instructions.c
> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
> @@ -1,6 +1,17 @@
>   // SPDX-License-Identifier: GPL-2.0
>   #include <linux/compiler.h>
>   
> +/*
> + * powerpc instruction nmemonic table to associate load/store instructions with
> + * move_ops. mov_ops is used to identify add/mr to do instruction tracking.
> + */
> +static struct ins powerpc__instructions[] = {
> +	{ .name = "mr",         .ops = &load_store_ops,  },
> +	{ .name = "addi",       .ops = &load_store_ops,   },
> +	{ .name = "addis",      .ops = &load_store_ops,  },
> +	{ .name = "add",        .ops = &load_store_ops,  },
> +};
> +
>   static struct ins_ops *powerpc__associate_instruction_ops(struct arch *arch, const char *name)
>   {
>   	int i;
> @@ -75,6 +86,9 @@ static void update_insn_state_powerpc(struct type_state *state,
>   	if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
>   		return;
>   
> +	if (!strncmp(dl->ins.name, "add", 3))
> +		goto regs_check;
> +
>   	if (strncmp(dl->ins.name, "mr", 2))
>   		return;
>   
> @@ -85,6 +99,7 @@ static void update_insn_state_powerpc(struct type_state *state,
>   		dst->reg1 = src_reg;
>   	}
>   
> +regs_check:
>   	if (!has_reg_type(state, dst->reg1))
>   		return;
>   
> @@ -115,6 +130,12 @@ static void update_insn_state_powerpc(struct type_state *state __maybe_unused, s
>   static int powerpc__annotate_init(struct arch *arch, char *cpuid __maybe_unused)
>   {
>   	if (!arch->initialized) {
> +		arch->nr_instructions = ARRAY_SIZE(powerpc__instructions);
> +		arch->instructions = calloc(arch->nr_instructions, sizeof(struct ins));
> +		if (!arch->instructions)
> +			return -ENOMEM;
> +		memcpy(arch->instructions, powerpc__instructions, sizeof(struct ins) * arch->nr_instructions);
> +		arch->nr_instructions_allocated = arch->nr_instructions;
>   		arch->initialized = true;
>   		arch->associate_instruction_ops = powerpc__associate_instruction_ops;
>   		arch->objdump.comment_char      = '#';
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index ac6b8b8da38a..32cf506a9010 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -36,6 +36,7 @@ static struct ins_ops mov_ops;
>   static struct ins_ops nop_ops;
>   static struct ins_ops lock_ops;
>   static struct ins_ops ret_ops;
> +static struct ins_ops load_store_ops;
>   
>   static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>   			   struct ins_operands *ops, int max_ins_name);