Received-SPF: pass (google.com: domain of linux-kernel+bounces-206920-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\))
Subject: Re: [PATCH V3 06/14] tools/perf: Update parameters for reg extract
 functions to use raw instruction on powerpc
From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
In-Reply-To: <ZmFclbqQytaZt1Ep@google.com>
Date: Sat, 8 Jun 2024 12:38:08 +0530
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Ian Rogers <irogers@google.com>,
        Segher Boessenkool <segher@kernel.crashing.org>,
        christophe.leroy@csgroup.eu, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        akanksha@linux.ibm.com, maddy@linux.ibm.com, kjain@linux.ibm.com,
        disgoel@linux.vnet.ibm.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <3AAD3E17-571C-4775-BF4B-34EDFCC94079@linux.vnet.ibm.com>
References: <20240601060941.13692-1-atrajeev@linux.vnet.ibm.com>
 <20240601060941.13692-7-atrajeev@linux.vnet.ibm.com>
 <ZmFclbqQytaZt1Ep@google.com>
To: Namhyung Kim <namhyung@kernel.org>


> On 6 Jun 2024, at 12:22=E2=80=AFPM, Namhyung Kim <namhyung@kernel.org> =
wrote:
>=20
> On Sat, Jun 01, 2024 at 11:39:33AM +0530, Athira Rajeev wrote:
>> Use the raw instruction code and macros to identify memory =
instructions,
>> extract register fields and also offset. The implementation addresses
>> the D-form, X-form, DS-form instructions. Two main functions are =
added.
>> New parse function "load_store__parse" as instruction ops parser for
>> memory instructions. Unlink other parser (like mov__parse), this =
parser
>> fills in the "raw_insn" field for source/target and new added =
"mem_ref"
>> field. Also set if it is multi_regs and opcode as well. No other =
fields
>> are set because, here there is no need to parse the disassembled
>> code and arch specific macros will take care of extracting offset and
>> regs which is easier and will be precise.
>>=20
>> In powerpc, all instructions with a primary opcode from 32 to 63
>> are memory instructions. Update "ins__find" function to have =
"raw_insn"
>> also as a parameter. Don't use the "extract_reg_offset", instead use
>> newly added function "get_arch_regs" which will set these fields: =
reg1,
>> reg2, offset depending of where it is source or target ops.
>>=20
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> .../perf/arch/powerpc/annotate/instructions.c | 16 +++++
>> tools/perf/arch/powerpc/util/dwarf-regs.c     | 44 +++++++++++++
>> tools/perf/util/annotate.c                    | 25 +++++++-
>> tools/perf/util/disasm.c                      | 64 =
+++++++++++++++++--
>> tools/perf/util/disasm.h                      |  4 +-
>> tools/perf/util/include/dwarf-regs.h          |  3 +
>> 6 files changed, 147 insertions(+), 9 deletions(-)
>>=20
>> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c =
b/tools/perf/arch/powerpc/annotate/instructions.c
>> index d57fd023ef9c..10fea5e5cf4c 100644
>> --- a/tools/perf/arch/powerpc/annotate/instructions.c
>> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
>> @@ -49,6 +49,22 @@ static struct ins_ops =
*powerpc__associate_instruction_ops(struct arch *arch, con
>> return ops;
>> }
>>=20
>> +#define PPC_OP(op)      (((op) >> 26) & 0x3F)
>> +
>> +static struct ins_ops *check_ppc_insn(int raw_insn)
>> +{
>> + int opcode =3D PPC_OP(raw_insn);
>> +
>> + /*
>> +  * Instructions with opcode 32 to 63 are memory
>> +  * instructions in powerpc
>> +  */
>> + if ((opcode & 0x20))
>> + return &load_store_ops;
>> +
>> + return NULL;
>> +}
>> +
>> static int powerpc__annotate_init(struct arch *arch, char *cpuid =
__maybe_unused)
>> {
>> if (!arch->initialized) {
>> diff --git a/tools/perf/arch/powerpc/util/dwarf-regs.c =
b/tools/perf/arch/powerpc/util/dwarf-regs.c
>> index 430623ca5612..38b74fa01d8b 100644
>> --- a/tools/perf/arch/powerpc/util/dwarf-regs.c
>> +++ b/tools/perf/arch/powerpc/util/dwarf-regs.c
>> @@ -107,3 +107,47 @@ int regs_query_register_offset(const char *name)
>> #define PPC_DS(DS) ((DS) & 0xfffc)
>> #define OP_LD 58
>> #define OP_STD 62
>> +
>> +static int get_source_reg(unsigned int raw_insn)
>> +{
>> + return PPC_RA(raw_insn);
>> +}
>> +
>> +static int get_target_reg(unsigned int raw_insn)
>> +{
>> + return PPC_RT(raw_insn);
>> +}
>> +
>> +static int get_offset_opcode(int raw_insn __maybe_unused)
>=20
> The argument is used below, no need for __maybe_unused.
>=20
>> +{
>> + int opcode =3D PPC_OP(raw_insn);
>> +
>> + /* DS- form */
>> + if ((opcode =3D=3D OP_LD) || (opcode =3D=3D OP_STD))
>> + return PPC_DS(raw_insn);
>> + else
>> + return PPC_D(raw_insn);
>> +}
>> +
>> +/*
>> + * Fills the required fields for op_loc depending on if it
>> + * is a source or target.
>> + * D form: ins RT,D(RA) -> src_reg1 =3D RA, offset =3D D, dst_reg1 =3D=
 RT
>> + * DS form: ins RT,DS(RA) -> src_reg1 =3D RA, offset =3D DS, =
dst_reg1 =3D RT
>> + * X form: ins RT,RA,RB -> src_reg1 =3D RA, src_reg2 =3D RB, =
dst_reg1 =3D RT
>> + */
>> +void get_arch_regs(int raw_insn __maybe_unused, int is_source =
__maybe_unused,
>> + struct annotated_op_loc *op_loc __maybe_unused)
>=20
> Ditto.

Yes, right. Will fix it both the places.

Thanks
Athira
>=20
> Thanks,
> Namhyung

>=20
>=20
>> +{
>> + if (is_source)
>> + op_loc->reg1 =3D get_source_reg(raw_insn);
>> + else
>> + op_loc->reg1 =3D get_target_reg(raw_insn);
>> +
>> + if (op_loc->multi_regs)
>> + op_loc->reg2 =3D PPC_RB(raw_insn);
>> +
>> + /* TODO: Implement offset handling for X Form */
>> + if ((op_loc->mem_ref) && (PPC_OP(raw_insn) !=3D 31))
>> + op_loc->offset =3D get_offset_opcode(raw_insn);
>> +}
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 1451caf25e77..2b8cc759ae35 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -2079,6 +2079,12 @@ static int extract_reg_offset(struct arch =
*arch, const char *str,
>> return 0;
>> }
>>=20
>> +__weak void get_arch_regs(int raw_insn __maybe_unused, int is_source =
__maybe_unused,
>> + struct annotated_op_loc *op_loc __maybe_unused)
>> +{
>> + return;
>> +}
>> +
>> /**
>>  * annotate_get_insn_location - Get location of instruction
>>  * @arch: the architecture info
>> @@ -2123,20 +2129,33 @@ int annotate_get_insn_location(struct arch =
*arch, struct disasm_line *dl,
>> for_each_insn_op_loc(loc, i, op_loc) {
>> const char *insn_str =3D ops->source.raw;
>> bool multi_regs =3D ops->source.multi_regs;
>> + bool mem_ref =3D ops->source.mem_ref;
>>=20
>> if (i =3D=3D INSN_OP_TARGET) {
>> insn_str =3D ops->target.raw;
>> multi_regs =3D ops->target.multi_regs;
>> + mem_ref =3D ops->target.mem_ref;
>> }
>>=20
>> /* Invalidate the register by default */
>> op_loc->reg1 =3D -1;
>> op_loc->reg2 =3D -1;
>>=20
>> - if (insn_str =3D=3D NULL)
>> - continue;
>> + if (insn_str =3D=3D NULL) {
>> + if (!arch__is(arch, "powerpc"))
>> + continue;
>> + }
>>=20
>> - if (strchr(insn_str, arch->objdump.memory_ref_char)) {
>> + /*
>> +  * For powerpc, call get_arch_regs function which extracts the
>> +  * required fields for op_loc, ie reg1, reg2, offset from the
>> +  * raw instruction.
>> +  */
>> + if (arch__is(arch, "powerpc")) {
>> + op_loc->mem_ref =3D mem_ref;
>> + op_loc->multi_regs =3D multi_regs;
>> + get_arch_regs(ops->raw_insn, !i, op_loc);
>> + } else if (strchr(insn_str, arch->objdump.memory_ref_char)) {
>> op_loc->mem_ref =3D true;
>> op_loc->multi_regs =3D multi_regs;
>> extract_reg_offset(arch, insn_str, op_loc);
>> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
>> index 61f0f1656f82..252cb0d1f5d1 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -37,6 +37,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;
>>=20
>> static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>>    struct ins_operands *ops, int max_ins_name);
>> @@ -517,7 +518,7 @@ static int lock__parse(struct arch *arch, struct =
ins_operands *ops, struct map_s
>> if (disasm_line__parse(ops->raw, &ops->locked.ins.name, =
&ops->locked.ops->raw) < 0)
>> goto out_free_ops;
>>=20
>> - ops->locked.ins.ops =3D ins__find(arch, ops->locked.ins.name);
>> + ops->locked.ins.ops =3D ins__find(arch, ops->locked.ins.name, 0);
>>=20
>> if (ops->locked.ins.ops =3D=3D NULL)
>> goto out_free_ops;
>> @@ -672,6 +673,47 @@ static struct ins_ops mov_ops =3D {
>> .scnprintf =3D mov__scnprintf,
>> };
>>=20
>> +static int load_store__scnprintf(struct ins *ins, char *bf, size_t =
size,
>> + struct ins_operands *ops, int max_ins_name)
>> +{
>> + return scnprintf(bf, size, "%-*s %s", max_ins_name, ins->name,
>> + ops->raw);
>> +}
>> +
>> +/*
>> + * Sets the fields: "raw_insn", opcode, multi_regs and "mem_ref".
>> + * "mem_ref" is set for ops->source which is later used to
>> + * fill the objdump->memory_ref-char field. This ops is currently
>> + * used by powerpc and since binary instruction code is used to
>> + * extract opcode, regs and offset, no other parsing is needed here
>> + */
>> +static int load_store__parse(struct arch *arch __maybe_unused, =
struct ins_operands *ops,
>> + struct map_symbol *ms __maybe_unused)
>> +{
>> + ops->source.raw_insn =3D ops->raw_insn;
>> + ops->source.mem_ref =3D true;
>> + ops->source.opcode =3D ops->opcode;
>> + ops->source.multi_regs =3D false;
>> +
>> + if (!ops->source.raw_insn)
>> + return -1;
>> +
>> + ops->target.raw_insn =3D ops->raw_insn;
>> + ops->target.mem_ref =3D false;
>> + ops->target.multi_regs =3D false;
>> + ops->target.opcode =3D ops->opcode;
>> +
>> + if (!ops->target.raw_insn)
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +static struct ins_ops load_store_ops =3D {
>> + .parse     =3D load_store__parse,
>> + .scnprintf =3D load_store__scnprintf,
>> +};
>> +
>> static int dec__parse(struct arch *arch __maybe_unused, struct =
ins_operands *ops, struct map_symbol *ms __maybe_unused)
>> {
>> char *target, *comment, *s, prev;
>> @@ -762,11 +804,23 @@ static void ins__sort(struct arch *arch)
>> qsort(arch->instructions, nmemb, sizeof(struct ins), ins__cmp);
>> }
>>=20
>> -static struct ins_ops *__ins__find(struct arch *arch, const char =
*name)
>> +static struct ins_ops *__ins__find(struct arch *arch, const char =
*name, int raw_insn)
>> {
>> struct ins *ins;
>> const int nmemb =3D arch->nr_instructions;
>>=20
>> + if (arch__is(arch, "powerpc")) {
>> + /*
>> +  * For powerpc, identify the instruction ops
>> +  * from the opcode using raw_insn.
>> +  */
>> + struct ins_ops *ops;
>> +
>> + ops =3D check_ppc_insn(raw_insn);
>> + if (ops)
>> + return ops;
>> + }
>> +
>> if (!arch->sorted_instructions) {
>> ins__sort(arch);
>> arch->sorted_instructions =3D true;
>> @@ -796,9 +850,9 @@ static struct ins_ops *__ins__find(struct arch =
*arch, const char *name)
>> return ins ? ins->ops : NULL;
>> }
>>=20
>> -struct ins_ops *ins__find(struct arch *arch, const char *name)
>> +struct ins_ops *ins__find(struct arch *arch, const char *name, int =
raw_insn)
>> {
>> - struct ins_ops *ops =3D __ins__find(arch, name);
>> + struct ins_ops *ops =3D __ins__find(arch, name, raw_insn);
>>=20
>> if (!ops && arch->associate_instruction_ops)
>> ops =3D arch->associate_instruction_ops(arch, name);
>> @@ -808,7 +862,7 @@ struct ins_ops *ins__find(struct arch *arch, =
const char *name)
>>=20
>> static void disasm_line__init_ins(struct disasm_line *dl, struct arch =
*arch, struct map_symbol *ms)
>> {
>> - dl->ins.ops =3D ins__find(arch, dl->ins.name);
>> + dl->ins.ops =3D ins__find(arch, dl->ins.name, dl->ops.raw_insn);
>>=20
>> if (!dl->ins.ops)
>> return;
>> diff --git a/tools/perf/util/disasm.h b/tools/perf/util/disasm.h
>> index a391e1bb81f7..831ebcc329cd 100644
>> --- a/tools/perf/util/disasm.h
>> +++ b/tools/perf/util/disasm.h
>> @@ -62,6 +62,7 @@ struct ins_operands {
>> bool offset_avail;
>> bool outside;
>> bool multi_regs;
>> + bool mem_ref;
>> } target;
>> union {
>> struct {
>> @@ -71,6 +72,7 @@ struct ins_operands {
>> int raw_insn;
>> u64 addr;
>> bool multi_regs;
>> + bool mem_ref;
>> } source;
>> struct {
>> struct ins     ins;
>> @@ -104,7 +106,7 @@ struct annotate_args {
>> struct arch *arch__find(const char *name);
>> bool arch__is(struct arch *arch, const char *name);
>>=20
>> -struct ins_ops *ins__find(struct arch *arch, const char *name);
>> +struct ins_ops *ins__find(struct arch *arch, const char *name, int =
raw_insn);
>> int ins__scnprintf(struct ins *ins, char *bf, size_t size,
>>    struct ins_operands *ops, int max_ins_name);
>>=20
>> diff --git a/tools/perf/util/include/dwarf-regs.h =
b/tools/perf/util/include/dwarf-regs.h
>> index 01fb25a1150a..7ea39362ecaf 100644
>> --- a/tools/perf/util/include/dwarf-regs.h
>> +++ b/tools/perf/util/include/dwarf-regs.h
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0 */
>> #ifndef _PERF_DWARF_REGS_H_
>> #define _PERF_DWARF_REGS_H_
>> +#include "annotate.h"
>>=20
>> #define DWARF_REG_PC  0xd3af9c /* random number */
>> #define DWARF_REG_FB  0xd3affb /* random number */
>> @@ -31,6 +32,8 @@ static inline int get_dwarf_regnum(const char *name =
__maybe_unused,
>> }
>> #endif
>>=20
>> +void get_arch_regs(int raw_insn, int is_source, struct =
annotated_op_loc *op_loc);
>> +
>> #ifdef HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET
>> /*
>>  * Arch should support fetching the offset of a register in pt_regs
>> --=20
>> 2.43.0