Re: [PATCH v2 12/14] perf arm-spe: Add more sub classes for operation packet

From: Leo Yan <leo.yan@linaro.org>
To: "André Przywara" <andre.przywara@arm.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	Wei Li <liwei391@huawei.com>, James Clark <james.clark@arm.com>,
	Dave Martin <Dave.Martin@arm.com>,
	linux-kernel@vger.kernel.org, Al Grant <Al.Grant@arm.com>
Subject: Re: [PATCH v2 12/14] perf arm-spe: Add more sub classes for operation packet
Date: Wed, 21 Oct 2020 13:16:56 +0800	[thread overview]
Message-ID: <20201021051656.GF7226@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <de7d783c-29c6-0f71-922d-bcefe8f1962a@arm.com>

On Tue, Oct 20, 2020 at 10:54:57PM +0100, André Przywara wrote:
> On 29/09/2020 14:39, Leo Yan wrote:
> 
> Hi,
> 
> > For the operation type packet payload with load/store class, it misses
> > to support these sub classes:
> > 
> >   - A load/store targeting the general-purpose registers;
> >   - A load/store targeting unspecified registers;
> >   - The ARMv8.4 nested virtualisation extension can redirect system
> >     register accesses to a memory page controlled by the hypervisor.
> >     The SPE profiling feature in newer implementations can tag those
> >     memory accesses accordingly.
> > 
> > Add the bit pattern describing load/store sub classes, so that the perf
> > tool can decode it properly.
> > 
> > Inspired by Andre Przywara, refined the commit log and code for more
> > clear description.
> > 
> > Co-developed-by: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  .../util/arm-spe-decoder/arm-spe-pkt-decoder.c    | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > 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 a848c784f4cf..57a2d5494838 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
> > @@ -378,6 +378,21 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >  				ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
> >  				if (ret < 0)
> >  					return ret;
> > +			} else if ((payload & SPE_OP_PKT_LDST_SUBCLASS_MASK) ==
> 
> These three and the one above use the same mask, should this go into a
> switch case? Move this block to the end, then do:
> 	switch (payload & SPE_OP_PKT_LDST_SUBCLASS_MASK) {
> 	case SPE_OP_PKT_LDST_SUBCLASS_GP_REG:
> 		...
> 	case SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG:
> 		...
> Maybe even assign just a string pointer inside, then have one snprintf.
> Haven't checked it that *really* looks better, though.

Good point, will follow up this suggestion.

> Also those later checks are quite indented, shall those be moved to
> helper functions? Again just an idea ....

Yeah, I also considered to divide into small functions to handle
different types of packets so can avoid deep indention.  Will tweak
patches for this.

Very appreciate for your detailed reviewing and suggestions throughout
the patch set!

Leo