Messages in this thread | | | Subject | Re: [PATCH v2 12/14] perf arm-spe: Add more sub classes for operation packet | From | André Przywara <> | Date | Tue, 20 Oct 2020 22:54:57 +0100 |
| |
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.
Also those later checks are quite indented, shall those be moved to helper functions? Again just an idea ....
Cheers, Andre
> + SPE_OP_PKT_LDST_SUBCLASS_GP_REG) { > + ret = arm_spe_pkt_snprintf(&buf, &blen, " GP-REG"); > + if (ret < 0) > + return ret; > + } else if ((payload & SPE_OP_PKT_LDST_SUBCLASS_MASK) == > + SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG) { > + ret = arm_spe_pkt_snprintf(&buf, &blen, " UNSPEC-REG"); > + if (ret < 0) > + return ret; > + } else if ((payload & SPE_OP_PKT_LDST_SUBCLASS_MASK) == > + SPE_OP_PKT_LDST_SUBCLASS_NV_SYSREG) { > + ret = arm_spe_pkt_snprintf(&buf, &blen, " NV-SYSREG"); > + if (ret < 0) > + return ret; > } > > return buf_len - blen; >
| |