Date:   Mon, 30 Jan 2023 10:09:46 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Rob Herring <robh@kernel.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        James Clark <james.clark@arm.com>,
        Will Deacon <will@kernel.org>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf arm-spe: Add raw decoding for SPEv1.2 previous
 branch address
Message-ID: <Y9cm6sLGTA6hZrIu@leoy-yangtze.lan>
References: <20230127205951.673253-1-robh@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230127205951.673253-1-robh@kernel.org>
Precedence: bulk

On Fri, Jan 27, 2023 at 02:59:51PM -0600, Rob Herring wrote:
> Arm SPEv1.2 adds a new optional address packet type: previous branch
> target. The recorded address is the target virtual address of the most
> recently taken branch in program order.
> 
> Add support for decoding the address packet in raw dumps.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 4 +++-
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index 2f311189c6e8..e92e1a7c8a8e 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -422,16 +422,18 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  	int ch, pat;
>  	u64 payload = packet->payload;
>  	int err = 0;
> +	static const char *idx_name[] = {"PC", "TGT", "VA", "PA", "LBR" };

I know you use the "LBR" which is a naming convention from X86 arch,
Armv8 ARM defines the name "PBT", so "PBT" would be easier for us to
match code with architecture specification.

"LBR" is also fine for me, it's up to you if you want to update for
this:

Reviewed-by: Leo Yan <leo.yan@linaro.org>


>  	switch (idx) {
>  	case SPE_ADDR_PKT_HDR_INDEX_INS:
>  	case SPE_ADDR_PKT_HDR_INDEX_BRANCH:
> +	case SPE_ADDR_PKT_HDR_INDEX_PREV_BRANCH:
>  		ns = !!SPE_ADDR_PKT_GET_NS(payload);
>  		el = SPE_ADDR_PKT_GET_EL(payload);
>  		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
>  		arm_spe_pkt_out_string(&err, &buf, &buf_len,
>  				"%s 0x%llx el%d ns=%d",
> -				(idx == 1) ? "TGT" : "PC", payload, el, ns);
> +				idx_name[idx], payload, el, ns);
>  		break;
>  	case SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT:
>  		arm_spe_pkt_out_string(&err, &buf, &buf_len,
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> index 9b970e7bf1e2..f75ed3a8a050 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> @@ -65,6 +65,7 @@ struct arm_spe_pkt {
>  #define SPE_ADDR_PKT_HDR_INDEX_BRANCH		0x1
>  #define SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT	0x2
>  #define SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS	0x3
> +#define SPE_ADDR_PKT_HDR_INDEX_PREV_BRANCH	0x4
>  
>  /* Address packet payload */
>  #define SPE_ADDR_PKT_ADDR_BYTE7_SHIFT		56
> -- 
> 2.39.0
>