Received-SPF: pass (google.com: domain of linux-kernel+bounces-206918-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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 10/14] tools/perf: Update instruction tracking for
 powerpc
From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
In-Reply-To: <ZmFc6kPnh-zIrIG1@google.com>
Date: Sat, 8 Jun 2024 12:35:42 +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: <100DACC9-508E-4B09-97AA-A771267DF1E8@linux.vnet.ibm.com>
References: <20240601060941.13692-1-atrajeev@linux.vnet.ibm.com>
 <20240601060941.13692-11-atrajeev@linux.vnet.ibm.com>
 <ZmFc6kPnh-zIrIG1@google.com>
To: Namhyung Kim <namhyung@kernel.org>


> On 6 Jun 2024, at 12:23=E2=80=AFPM, Namhyung Kim <namhyung@kernel.org> =
wrote:
>=20
> On Sat, Jun 01, 2024 at 11:39:37AM +0530, Athira Rajeev wrote:
>> Add instruction tracking function "update_insn_state_powerpc" for
>> powerpc. Example sequence in powerpc:
>>=20
>> ld      r10,264(r3)
>> mr      r31,r3
>> <<after some sequence>
>> ld      r9,312(r31)
>>=20
>> Consider ithe sample is pointing to: "ld r9,312(r31)".
>> Here the memory reference is hit at "312(r31)" where 312 is the =
offset
>> and r31 is the source register. Previous instruction sequence shows =
that
>> register state of r3 is moved to r31. So to identify the data type =
for r31
>> access, the previous instruction ("mr") needs to be tracked and the
>> state type entry has to be updated. Current instruction tracking =
support
>> in perf tools infrastructure is specific to x86. Patch adds this =
support
>> for powerpc as well.
>>=20
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> .../perf/arch/powerpc/annotate/instructions.c | 65 =
+++++++++++++++++++
>> tools/perf/util/annotate-data.c               |  9 ++-
>> tools/perf/util/disasm.c                      |  1 +
>> 3 files changed, 74 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c =
b/tools/perf/arch/powerpc/annotate/instructions.c
>> index db72148eb857..3ecf5a986037 100644
>> --- a/tools/perf/arch/powerpc/annotate/instructions.c
>> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
>> @@ -231,6 +231,71 @@ static struct ins_ops *check_ppc_insn(int =
raw_insn)
>> return NULL;
>> }
>>=20
>> +/*
>> + * Instruction tracking function to track register state moves.
>> + * Example sequence:
>> + *    ld      r10,264(r3)
>> + *    mr      r31,r3
>> + *    <<after some sequence>
>> + *    ld      r9,312(r31)
>> + *
>> + * Previous instruction sequence shows that register state of r3
>> + * is moved to r31. update_insn_state_powerpc tracks these state
>> + * changes
>> + */
>> +#ifdef HAVE_DWARF_SUPPORT
>> +static void update_insn_state_powerpc(struct type_state *state,
>> + struct data_loc_info *dloc, Dwarf_Die * cu_die __maybe_unused,
>> + struct disasm_line *dl)
>> +{
>> + struct annotated_insn_loc loc;
>> + struct annotated_op_loc *src =3D &loc.ops[INSN_OP_SOURCE];
>> + struct annotated_op_loc *dst =3D &loc.ops[INSN_OP_TARGET];
>> + struct type_state_reg *tsr;
>> + u32 insn_offset =3D dl->al.offset;
>> +
>> + if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
>> + return;
>> +
>> + /*
>> +  * Value 444 for bits 21:30 is for "mr"
>> +  * instruction. "mr" is extended OR. So set the
>> +  * source and destination reg correctly
>> +  */
>> + if (PPC_21_30(dl->ops.raw_insn) =3D=3D 444) {
>> + int src_reg =3D src->reg1;
>> +
>> + src->reg1 =3D dst->reg1;
>> + dst->reg1 =3D src_reg;
>> + }
>> +
>> + if (!has_reg_type(state, dst->reg1))
>> + return;
>> +
>> + tsr =3D &state->regs[dst->reg1];
>> +
>> + if (!has_reg_type(state, src->reg1) ||
>> + !state->regs[src->reg1].ok) {
>> + tsr->ok =3D false;
>> + return;
>> + }
>> +
>> + tsr->type =3D state->regs[src->reg1].type;
>> + tsr->kind =3D state->regs[src->reg1].kind;
>> + tsr->ok =3D true;
>> +
>> + pr_debug("mov [%x] reg%d -> reg%d",
>=20
> pr_debug_dtp() ?

Sure, will change this in V4

Thanks
Athira
>=20
> Thanks,
> Namhyung
>=20
>=20
>> + insn_offset, src->reg1, dst->reg1);
>> + pr_debug_type_name(&tsr->type, tsr->kind);
>> +}
>> +#else /* HAVE_DWARF_SUPPORT */
>> +static void update_insn_state_powerpc(struct type_state *state =
__maybe_unused, struct data_loc_info *dloc __maybe_unused,
>> + Dwarf_Die * cu_die __maybe_unused, struct disasm_line *dl =
__maybe_unused)
>> +{
>> + return;
>> +}
>> +#endif /* HAVE_DWARF_SUPPORT */
>> +
>> static int powerpc__annotate_init(struct arch *arch, char *cpuid =
__maybe_unused)
>> {
>> if (!arch->initialized) {
>> diff --git a/tools/perf/util/annotate-data.c =
b/tools/perf/util/annotate-data.c
>> index 7a48c3d72b89..734acdd8c4b7 100644
>> --- a/tools/perf/util/annotate-data.c
>> +++ b/tools/perf/util/annotate-data.c
>> @@ -1080,6 +1080,13 @@ static int find_data_type_insn(struct =
data_loc_info *dloc,
>> return ret;
>> }
>>=20
>> +static int arch_supports_insn_tracking(struct data_loc_info *dloc)
>> +{
>> + if ((arch__is(dloc->arch, "x86")) || (arch__is(dloc->arch, =
"powerpc")))
>> + return 1;
>> + return 0;
>> +}
>> +
>> /*
>>  * Construct a list of basic blocks for each scope with variables and =
try to find
>>  * the data type by updating a type state table through instructions.
>> @@ -1094,7 +1101,7 @@ static int find_data_type_block(struct =
data_loc_info *dloc,
>> int ret =3D -1;
>>=20
>> /* TODO: other architecture support */
>> - if (!arch__is(dloc->arch, "x86"))
>> + if (!arch_supports_insn_tracking(dloc))
>> return -1;
>>=20
>> prev_dst_ip =3D dst_ip =3D dloc->ip;
>> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
>> index 57af4dc42a58..d8b357055302 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -155,6 +155,7 @@ static struct arch architectures[] =3D {
>> {
>> .name =3D "powerpc",
>> .init =3D powerpc__annotate_init,
>> + .update_insn_state =3D update_insn_state_powerpc,
>> },
>> {
>> .name =3D "riscv64",
>> --=20
>> 2.43.0