[PATCH v3 00/20] perf arm-spe: Refactor decoding & dumping flow

[PATCH v3 00/20] perf arm-spe: Refactor decoding & dumping flow

[Leo Yan]

This is patch set v3 for refactoring Arm SPE trace decoding and dumping.
In this version, it mainly addressed the comments and suggestions from
mailing list (mainly from Andre Przywara, thanks!).

This patch set is to refactor the Arm SPE decoding with:

- Patches 01, 02 are minor cleans up for header, typos;
- Patches 03, 04 and 05 are used to fix and polish the packet and
  payload length calculation;
- Patch 06 is to add a helper to wrap up printing strings, this can
  avoid bunch of duplicate code lines;
- Patches 07 ~ 18 are used to refactor decoding for different types
  packet one by one (address packet, context packet, counter packet,
  event packet, operation packet); it also introduces separate functions
  for parsing specific packet, this can allow the code more readable and
  easier to manage and extend code;
- Patch 19 comes from Andre to dump memory tagging;
- Patch 20 comes from Wei Li to add decoding for ARMv8.3 SVE extension.

This patch set is cleanly applied on the top of perf/core branch
with commit 7cf726a59435 ("Merge tag 'linux-kselftest-kunit-5.10-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest"),
And I tested this patch set on Hisilicon D06 platform with commands
"perf script" and "perf script -D".

Changes from v2:
- Tried best to address Andre's comments and refined patches;
- Added new patches 08, 11, 13, 16 for introducing new functions for
  packets parsing (Andre);
- Removed size condition checking for event packet (Andre);
- Used PKT_XXX_GET() form to replace PKT_XXX_MASK()/PKT_XXX_SHIFT()
  (Andre).

Changes from v1:
- Heavily rewrote the patch 05 for refactoring printing strings; this
  is fundamental change, so adjusted the sequence for patches and moved
  the printing string patch ahead from patch 10 (v1) to patch 05;
- Changed to use GENMASK_ULL() for bits mask;
- Added Andre's patch 13 for dumping memory tagging;
- Refined patch 12 for adding sub classes for Operation packet, merged
  some commit log from Andre's patch, which allows commit log and code
  to be more clear; Added "Co-developed-by: Andre Przywara" tag to
  reflect this.


Andre Przywara (1):
  perf arm_spe: Decode memory tagging properties

Leo Yan (18):
  perf arm-spe: Include bitops.h for BIT() macro
  perf arm-spe: Fix a typo in comment
  perf arm-spe: Refactor payload size calculation
  perf arm-spe: Refactor arm_spe_get_events()
  perf arm-spe: Fix packet length handling
  perf arm-spe: Refactor printing string to buffer
  perf arm-spe: Refactor packet header parsing
  perf arm-spe: Add new function arm_spe_pkt_desc_addr()
  perf arm-spe: Refactor address packet handling
  perf arm-spe: Refactor context packet handling
  perf arm-spe: Add new function arm_spe_pkt_desc_counter()
  perf arm-spe: Refactor counter packet handling
  perf arm-spe: Add new function arm_spe_pkt_desc_event()
  perf arm-spe: Refactor event type handling
  perf arm-spe: Remove size condition checking for events
  perf arm-spe: Add new function arm_spe_pkt_desc_op_type()
  perf arm-spe: Refactor operation packet handling
  perf arm-spe: Add more sub classes for operation packet

Wei Li (1):
  perf arm-spe: Add support for ARMv8.3-SPE

 .../util/arm-spe-decoder/arm-spe-decoder.c    |  43 +-
 .../util/arm-spe-decoder/arm-spe-decoder.h    |  17 -
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 658 +++++++++++-------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 132 +++-
 4 files changed, 536 insertions(+), 314 deletions(-)

-- 
2.17.1

[PATCH v3 01/20] perf arm-spe: Include bitops.h for BIT() macro

[Leo Yan]

Include header linux/bitops.h, directly use its BIT() macro and remove
the self defined macros.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-decoder.c     | 5 +----
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 3 +--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index 93e063f22be5..cc18a1e8c212 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -12,6 +12,7 @@
 #include <string.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <linux/bitops.h>
 #include <linux/compiler.h>
 #include <linux/zalloc.h>
 
@@ -21,10 +22,6 @@
 
 #include "arm-spe-decoder.h"
 
-#ifndef BIT
-#define BIT(n)		(1UL << (n))
-#endif
-
 static u64 arm_spe_calc_ip(int index, u64 payload)
 {
 	u8 *addr = (u8 *)&payload;
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 b94001b756c7..46ddb53a6457 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
@@ -8,11 +8,10 @@
 #include <string.h>
 #include <endian.h>
 #include <byteswap.h>
+#include <linux/bitops.h>
 
 #include "arm-spe-pkt-decoder.h"
 
-#define BIT(n)		(1ULL << (n))
-
 #define NS_FLAG		BIT(63)
 #define EL_FLAG		(BIT(62) | BIT(61))
 
-- 
2.17.1

[PATCH v3 02/20] perf arm-spe: Fix a typo in comment

[Leo Yan]

Fix a typo: s/iff/if.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 2 +-
 1 file changed, 1 insertion(+), 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 46ddb53a6457..7c7b5eb09fba 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
@@ -142,7 +142,7 @@ static int arm_spe_get_events(const unsigned char *buf, size_t len,
 
 	/* we use index to identify Events with a less number of
 	 * comparisons in arm_spe_pkt_desc(): E.g., the LLC-ACCESS,
-	 * LLC-REFILL, and REMOTE-ACCESS events are identified iff
+	 * LLC-REFILL, and REMOTE-ACCESS events are identified if
 	 * index > 1.
 	 */
 	packet->index = ret - 1;
-- 
2.17.1

[PATCH v3 03/20] perf arm-spe: Refactor payload size calculation

[Leo Yan]

This patch defines macro to extract "sz" field from header, and renames
the function payloadlen() to arm_spe_payload_len().

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 18 +++++++++---------
 .../util/arm-spe-decoder/arm-spe-pkt-decoder.h |  3 +++
 2 files changed, 12 insertions(+), 9 deletions(-)

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 7c7b5eb09fba..4294c133a465 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
@@ -69,22 +69,22 @@ const char *arm_spe_pkt_name(enum arm_spe_pkt_type type)
 	return arm_spe_packet_name[type];
 }
 
-/* return ARM SPE payload size from its encoding,
- * which is in bits 5:4 of the byte.
- * 00 : byte
- * 01 : halfword (2)
- * 10 : word (4)
- * 11 : doubleword (8)
+/*
+ * Extracts the field "sz" from header bits and converts to bytes:
+ *   00 : byte (1)
+ *   01 : halfword (2)
+ *   10 : word (4)
+ *   11 : doubleword (8)
  */
-static int payloadlen(unsigned char byte)
+static unsigned int arm_spe_payload_len(unsigned char hdr)
 {
-	return 1 << ((byte & 0x30) >> 4);
+	return 1 << SPE_HEADER_SZ(hdr);
 }
 
 static int arm_spe_get_payload(const unsigned char *buf, size_t len,
 			       struct arm_spe_pkt *packet)
 {
-	size_t payload_len = payloadlen(buf[0]);
+	size_t payload_len = arm_spe_payload_len(buf[0]);
 
 	if (len < 1 + payload_len)
 		return ARM_SPE_NEED_MORE_BYTES;
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 4c870521b8eb..e9ea8e3ead5d 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
@@ -9,6 +9,7 @@
 
 #include <stddef.h>
 #include <stdint.h>
+#include <linux/bits.h>
 
 #define ARM_SPE_PKT_DESC_MAX		256
 
@@ -36,6 +37,8 @@ struct arm_spe_pkt {
 	uint64_t		payload;
 };
 
+#define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)
+
 #define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
 #define SPE_ADDR_PKT_HDR_INDEX_BRANCH		(0x1)
 #define SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT	(0x2)
-- 
2.17.1

Re: [PATCH v3 03/20] perf arm-spe: Refactor payload size calculation

[André Przywara]

On 22/10/2020 15:57, Leo Yan wrote:

Hi Leo,

> This patch defines macro to extract "sz" field from header, and renames
> the function payloadlen() to arm_spe_payload_len().
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 18 +++++++++---------
>  .../util/arm-spe-decoder/arm-spe-pkt-decoder.h |  3 +++
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> 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 7c7b5eb09fba..4294c133a465 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
> @@ -69,22 +69,22 @@ const char *arm_spe_pkt_name(enum arm_spe_pkt_type type)
>  	return arm_spe_packet_name[type];
>  }
>  
> -/* return ARM SPE payload size from its encoding,
> - * which is in bits 5:4 of the byte.
> - * 00 : byte
> - * 01 : halfword (2)
> - * 10 : word (4)
> - * 11 : doubleword (8)
> +/*
> + * Extracts the field "sz" from header bits and converts to bytes:
> + *   00 : byte (1)
> + *   01 : halfword (2)
> + *   10 : word (4)
> + *   11 : doubleword (8)
>   */
> -static int payloadlen(unsigned char byte)
> +static unsigned int arm_spe_payload_len(unsigned char hdr)
>  {
> -	return 1 << ((byte & 0x30) >> 4);
> +	return 1 << SPE_HEADER_SZ(hdr);

I know, I know, I asked for this, but now looking again at it - and
after having seen the whole series:
This is now really trivial, and there are just two users? And
SPE_HEADER_SZ() is only used in here?

So either you just stuff the "1U << .." into the callers of
arm_spe_payload_len(), or indeed put all of this into one macro (as you
had originally).

Apologies for this forth and back, but I didn't realise how this is
really used eventually, and I just saw the transition from function to
macro.

But please use 1U << .., signed shifts are treacherous.

>  }
>  
>  static int arm_spe_get_payload(const unsigned char *buf, size_t len,
>  			       struct arm_spe_pkt *packet)
>  {
> -	size_t payload_len = payloadlen(buf[0]);
> +	size_t payload_len = arm_spe_payload_len(buf[0]);
>  
>  	if (len < 1 + payload_len)
>  		return ARM_SPE_NEED_MORE_BYTES;
> 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 4c870521b8eb..e9ea8e3ead5d 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
> @@ -9,6 +9,7 @@
>  
>  #include <stddef.h>
>  #include <stdint.h>
> +#include <linux/bits.h>
>  
>  #define ARM_SPE_PKT_DESC_MAX		256
>  
> @@ -36,6 +37,8 @@ struct arm_spe_pkt {
>  	uint64_t		payload;
>  };
>  
> +#define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)

If you should keep this, please put parentheses around "val".

Cheers,
Andre

> +
>  #define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
>  #define SPE_ADDR_PKT_HDR_INDEX_BRANCH		(0x1)
>  #define SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT	(0x2)
> 

Re: [PATCH v3 03/20] perf arm-spe: Refactor payload size calculation

[Leo Yan]

Hi Andre,

On Fri, Oct 23, 2020 at 06:08:53PM +0100, André Przywara wrote:
> On 22/10/2020 15:57, Leo Yan wrote:
> 
> Hi Leo,
> 
> > This patch defines macro to extract "sz" field from header, and renames
> > the function payloadlen() to arm_spe_payload_len().
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 18 +++++++++---------
> >  .../util/arm-spe-decoder/arm-spe-pkt-decoder.h |  3 +++
> >  2 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > 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 7c7b5eb09fba..4294c133a465 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
> > @@ -69,22 +69,22 @@ const char *arm_spe_pkt_name(enum arm_spe_pkt_type type)
> >  	return arm_spe_packet_name[type];
> >  }
> >  
> > -/* return ARM SPE payload size from its encoding,
> > - * which is in bits 5:4 of the byte.
> > - * 00 : byte
> > - * 01 : halfword (2)
> > - * 10 : word (4)
> > - * 11 : doubleword (8)
> > +/*
> > + * Extracts the field "sz" from header bits and converts to bytes:
> > + *   00 : byte (1)
> > + *   01 : halfword (2)
> > + *   10 : word (4)
> > + *   11 : doubleword (8)
> >   */
> > -static int payloadlen(unsigned char byte)
> > +static unsigned int arm_spe_payload_len(unsigned char hdr)
> >  {
> > -	return 1 << ((byte & 0x30) >> 4);
> > +	return 1 << SPE_HEADER_SZ(hdr);
> 
> I know, I know, I asked for this, but now looking again at it - and
> after having seen the whole series:
> This is now really trivial, and there are just two users? And
> SPE_HEADER_SZ() is only used in here?
> 
> So either you just stuff the "1U << .." into the callers of
> arm_spe_payload_len(), or indeed put all of this into one macro (as you
> had originally).

Okay, will stuff the "1U << .." into the function.

> Apologies for this forth and back, but I didn't realise how this is
> really used eventually, and I just saw the transition from function to
> macro.

Actually you considered more than me and I learned many details from the
reivewing process, so no worries :)

> But please use 1U << .., signed shifts are treacherous.
> 
> >  }
> >  
> >  static int arm_spe_get_payload(const unsigned char *buf, size_t len,
> >  			       struct arm_spe_pkt *packet)
> >  {
> > -	size_t payload_len = payloadlen(buf[0]);
> > +	size_t payload_len = arm_spe_payload_len(buf[0]);
> >  
> >  	if (len < 1 + payload_len)
> >  		return ARM_SPE_NEED_MORE_BYTES;
> > 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 4c870521b8eb..e9ea8e3ead5d 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
> > @@ -9,6 +9,7 @@
> >  
> >  #include <stddef.h>
> >  #include <stdint.h>
> > +#include <linux/bits.h>
> >  
> >  #define ARM_SPE_PKT_DESC_MAX		256
> >  
> > @@ -36,6 +37,8 @@ struct arm_spe_pkt {
> >  	uint64_t		payload;
> >  };
> >  
> > +#define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)
> 
> If you should keep this, please put parentheses around "val".

I went through your suggestions in this patch and other patches, looks
reasonable to me, will follow them and respin patch set.

Thanks a lot for timely reviewing!

Leo

[PATCH v3 04/20] perf arm-spe: Refactor arm_spe_get_events()

[Leo Yan]

In function arm_spe_get_events(), the event packet's 'index' is assigned
as payload length, but the flow is not directive: it firstly gets the
packet length from the return value of arm_spe_get_payload(), the value
includes header length (1) and payload length:

  int ret = arm_spe_get_payload(buf, len, packet);

and then reduces header length from packet length, so finally get the
payload length:

  packet->index = ret - 1;

To simplify the code, this patch directly assigns payload length to
event packet's index; and at the end it calls arm_spe_get_payload() to
return the payload value.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

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 4294c133a465..f3bb8bf102aa 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
@@ -136,8 +136,6 @@ static int arm_spe_get_timestamp(const unsigned char *buf, size_t len,
 static int arm_spe_get_events(const unsigned char *buf, size_t len,
 			      struct arm_spe_pkt *packet)
 {
-	int ret = arm_spe_get_payload(buf, len, packet);
-
 	packet->type = ARM_SPE_EVENTS;
 
 	/* we use index to identify Events with a less number of
@@ -145,9 +143,9 @@ static int arm_spe_get_events(const unsigned char *buf, size_t len,
 	 * LLC-REFILL, and REMOTE-ACCESS events are identified if
 	 * index > 1.
 	 */
-	packet->index = ret - 1;
+	packet->index = arm_spe_payload_len(buf[0]);
 
-	return ret;
+	return arm_spe_get_payload(buf, len, packet);
 }
 
 static int arm_spe_get_data_source(const unsigned char *buf, size_t len,
-- 
2.17.1

Re: [PATCH v3 04/20] perf arm-spe: Refactor arm_spe_get_events()

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:
> In function arm_spe_get_events(), the event packet's 'index' is assigned
> as payload length, but the flow is not directive: it firstly gets the
> packet length from the return value of arm_spe_get_payload(), the value
> includes header length (1) and payload length:
> 
>   int ret = arm_spe_get_payload(buf, len, packet);
> 
> and then reduces header length from packet length, so finally get the
> payload length:
> 
>   packet->index = ret - 1;
> 
> To simplify the code, this patch directly assigns payload length to
> event packet's index; and at the end it calls arm_spe_get_payload() to
> return the payload value.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> 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 4294c133a465..f3bb8bf102aa 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
> @@ -136,8 +136,6 @@ static int arm_spe_get_timestamp(const unsigned char *buf, size_t len,
>  static int arm_spe_get_events(const unsigned char *buf, size_t len,
>  			      struct arm_spe_pkt *packet)
>  {
> -	int ret = arm_spe_get_payload(buf, len, packet);
> -
>  	packet->type = ARM_SPE_EVENTS;
>  
>  	/* we use index to identify Events with a less number of
> @@ -145,9 +143,9 @@ static int arm_spe_get_events(const unsigned char *buf, size_t len,
>  	 * LLC-REFILL, and REMOTE-ACCESS events are identified if
>  	 * index > 1.
>  	 */
> -	packet->index = ret - 1;
> +	packet->index = arm_spe_payload_len(buf[0]);
>  
> -	return ret;
> +	return arm_spe_get_payload(buf, len, packet);
>  }
>  
>  static int arm_spe_get_data_source(const unsigned char *buf, size_t len,
> 

[PATCH v3 05/20] perf arm-spe: Fix packet length handling

[Leo Yan]

When processing address packet and counter packet, if the packet
contains extended header, it misses to account the extra one byte for
header length calculation, thus returns the wrong packet length.

To correct the packet length calculation, one possible fixing is simply
to plus extra 1 for extended header, but will spread some duplicate code
in the flows for processing address packet and counter packet.
Alternatively, we can refine the function arm_spe_get_payload() to not
only support short header and allow it to support extended header, and
rely on it for the packet length calculation.

So this patch refactors function arm_spe_get_payload() with a new
argument 'ext_hdr' for support extended header; the packet processing
flows can invoke this function to unify the packet length calculation.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 34 +++++++------------
 1 file changed, 12 insertions(+), 22 deletions(-)

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 f3bb8bf102aa..e9ec7edb51a0 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
@@ -82,14 +82,15 @@ static unsigned int arm_spe_payload_len(unsigned char hdr)
 }
 
 static int arm_spe_get_payload(const unsigned char *buf, size_t len,
+			       unsigned char ext_hdr,
 			       struct arm_spe_pkt *packet)
 {
-	size_t payload_len = arm_spe_payload_len(buf[0]);
+	size_t payload_len = arm_spe_payload_len(buf[ext_hdr]);
 
-	if (len < 1 + payload_len)
+	if (len < 1 + ext_hdr + payload_len)
 		return ARM_SPE_NEED_MORE_BYTES;
 
-	buf++;
+	buf += 1 + ext_hdr;
 
 	switch (payload_len) {
 	case 1: packet->payload = *(uint8_t *)buf; break;
@@ -99,7 +100,7 @@ static int arm_spe_get_payload(const unsigned char *buf, size_t len,
 	default: return ARM_SPE_BAD_PACKET;
 	}
 
-	return 1 + payload_len;
+	return 1 + ext_hdr + payload_len;
 }
 
 static int arm_spe_get_pad(struct arm_spe_pkt *packet)
@@ -130,7 +131,7 @@ static int arm_spe_get_timestamp(const unsigned char *buf, size_t len,
 				 struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_TIMESTAMP;
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_events(const unsigned char *buf, size_t len,
@@ -145,14 +146,14 @@ static int arm_spe_get_events(const unsigned char *buf, size_t len,
 	 */
 	packet->index = arm_spe_payload_len(buf[0]);
 
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_data_source(const unsigned char *buf, size_t len,
 				   struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_DATA_SOURCE;
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_context(const unsigned char *buf, size_t len,
@@ -160,8 +161,7 @@ static int arm_spe_get_context(const unsigned char *buf, size_t len,
 {
 	packet->type = ARM_SPE_CONTEXT;
 	packet->index = buf[0] & 0x3;
-
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_op_type(const unsigned char *buf, size_t len,
@@ -169,41 +169,31 @@ static int arm_spe_get_op_type(const unsigned char *buf, size_t len,
 {
 	packet->type = ARM_SPE_OP_TYPE;
 	packet->index = buf[0] & 0x3;
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_counter(const unsigned char *buf, size_t len,
 			       const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
-	if (len < 2)
-		return ARM_SPE_NEED_MORE_BYTES;
-
 	packet->type = ARM_SPE_COUNTER;
 	if (ext_hdr)
 		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
 	else
 		packet->index = buf[0] & 0x7;
 
-	packet->payload = le16_to_cpu(*(uint16_t *)(buf + 1));
-
-	return 1 + ext_hdr + 2;
+	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
 
 static int arm_spe_get_addr(const unsigned char *buf, size_t len,
 			    const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
-	if (len < 8)
-		return ARM_SPE_NEED_MORE_BYTES;
-
 	packet->type = ARM_SPE_ADDRESS;
 	if (ext_hdr)
 		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
 	else
 		packet->index = buf[0] & 0x7;
 
-	memcpy_le64(&packet->payload, buf + 1, 8);
-
-	return 1 + ext_hdr + 8;
+	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
 
 static int arm_spe_do_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v3 06/20] perf arm-spe: Refactor printing string to buffer

[Leo Yan]

When outputs strings to the decoding buffer with function snprintf(),
SPE decoder needs to detects if any error returns from snprintf() and if
so needs to directly bail out.  If snprintf() returns success, it needs
to update buffer pointer and reduce the buffer length so can continue to
output the next string into the consequent memory space.

This complex logics are spreading in the function arm_spe_pkt_desc() so
there has many duplicate codes for handling error detecting, increment
buffer pointer and decrement buffer size.

To avoid the duplicate code, this patch introduces a new helper function
arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
the caller arm_spe_pkt_desc() will call it and simply check the returns
value.

This patch also moves the variable 'blen' as the function's local
variable, this allows to remove the unnecessary braces and improve the
readability.

Suggested-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 247 ++++++++++--------
 1 file changed, 135 insertions(+), 112 deletions(-)

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 e9ec7edb51a0..c7b6dc016f11 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
@@ -9,6 +9,7 @@
 #include <endian.h>
 #include <byteswap.h>
 #include <linux/bitops.h>
+#include <stdarg.h>
 
 #include "arm-spe-pkt-decoder.h"
 
@@ -258,192 +259,214 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
 	return ret;
 }
 
+static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
+				const char *fmt, ...)
+{
+	va_list ap;
+	int ret;
+
+	va_start(ap, fmt);
+	ret = vsnprintf(*buf_p, *blen, fmt, ap);
+	va_end(ap);
+
+	if (ret < 0)
+		return ret;
+
+	*buf_p += ret;
+	*blen -= ret;
+	return ret;
+}
+
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
 	int ret, ns, el, idx = packet->index;
 	unsigned long long payload = packet->payload;
 	const char *name = arm_spe_pkt_name(packet->type);
+	size_t blen = buf_len;
 
 	switch (packet->type) {
 	case ARM_SPE_BAD:
 	case ARM_SPE_PAD:
 	case ARM_SPE_END:
-		return snprintf(buf, buf_len, "%s", name);
-	case ARM_SPE_EVENTS: {
-		size_t blen = buf_len;
-
-		ret = 0;
-		ret = snprintf(buf, buf_len, "EV");
-		buf += ret;
-		blen -= ret;
+		return arm_spe_pkt_snprintf(&buf, &blen, "%s", name);
+	case ARM_SPE_EVENTS:
+		ret = arm_spe_pkt_snprintf(&buf, &blen, "EV");
+		if (ret < 0)
+			return ret;
+
 		if (payload & 0x1) {
-			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
+			if (ret < 0)
+				return ret;
 		}
 		if (payload & 0x2) {
-			ret = snprintf(buf, buf_len, " RETIRED");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
+			if (ret < 0)
+				return ret;
 		}
 		if (payload & 0x4) {
-			ret = snprintf(buf, buf_len, " L1D-ACCESS");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
+			if (ret < 0)
+				return ret;
 		}
 		if (payload & 0x8) {
-			ret = snprintf(buf, buf_len, " L1D-REFILL");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
+			if (ret < 0)
+				return ret;
 		}
 		if (payload & 0x10) {
-			ret = snprintf(buf, buf_len, " TLB-ACCESS");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
+			if (ret < 0)
+				return ret;
 		}
 		if (payload & 0x20) {
-			ret = snprintf(buf, buf_len, " TLB-REFILL");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
+			if (ret < 0)
+				return ret;
 		}
 		if (payload & 0x40) {
-			ret = snprintf(buf, buf_len, " NOT-TAKEN");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
+			if (ret < 0)
+				return ret;
 		}
 		if (payload & 0x80) {
-			ret = snprintf(buf, buf_len, " MISPRED");
-			buf += ret;
-			blen -= ret;
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
+			if (ret < 0)
+				return ret;
 		}
 		if (idx > 1) {
 			if (payload & 0x100) {
-				ret = snprintf(buf, buf_len, " LLC-ACCESS");
-				buf += ret;
-				blen -= ret;
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
+				if (ret < 0)
+					return ret;
 			}
 			if (payload & 0x200) {
-				ret = snprintf(buf, buf_len, " LLC-REFILL");
-				buf += ret;
-				blen -= ret;
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
+				if (ret < 0)
+					return ret;
 			}
 			if (payload & 0x400) {
-				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
-				buf += ret;
-				blen -= ret;
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
+				if (ret < 0)
+					return ret;
 			}
 		}
-		if (ret < 0)
-			return ret;
-		blen -= ret;
 		return buf_len - blen;
-	}
+
 	case ARM_SPE_OP_TYPE:
 		switch (idx) {
-		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
-					"COND-SELECT" : "INSN-OTHER");
-		case 1:	{
-			size_t blen = buf_len;
-
-			if (payload & 0x1)
-				ret = snprintf(buf, buf_len, "ST");
-			else
-				ret = snprintf(buf, buf_len, "LD");
-			buf += ret;
-			blen -= ret;
+		case 0:
+			return arm_spe_pkt_snprintf(&buf, &blen,
+					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
+		case 1:
+			ret = arm_spe_pkt_snprintf(&buf, &blen,
+						   payload & 0x1 ? "ST" : "LD");
+			if (ret < 0)
+				return ret;
+
 			if (payload & 0x2) {
 				if (payload & 0x4) {
-					ret = snprintf(buf, buf_len, " AT");
-					buf += ret;
-					blen -= ret;
+					ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
+					if (ret < 0)
+						return ret;
 				}
 				if (payload & 0x8) {
-					ret = snprintf(buf, buf_len, " EXCL");
-					buf += ret;
-					blen -= ret;
+					ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
+					if (ret < 0)
+						return ret;
 				}
 				if (payload & 0x10) {
-					ret = snprintf(buf, buf_len, " AR");
-					buf += ret;
-					blen -= ret;
+					ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
+					if (ret < 0)
+						return ret;
 				}
 			} else if (payload & 0x4) {
-				ret = snprintf(buf, buf_len, " SIMD-FP");
-				buf += ret;
-				blen -= ret;
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
+				if (ret < 0)
+					return ret;
 			}
+
+			return buf_len - blen;
+
+		case 2:
+			ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
 			if (ret < 0)
 				return ret;
-			blen -= ret;
-			return buf_len - blen;
-		}
-		case 2:	{
-			size_t blen = buf_len;
 
-			ret = snprintf(buf, buf_len, "B");
-			buf += ret;
-			blen -= ret;
 			if (payload & 0x1) {
-				ret = snprintf(buf, buf_len, " COND");
-				buf += ret;
-				blen -= ret;
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
+				if (ret < 0)
+					return ret;
 			}
 			if (payload & 0x2) {
-				ret = snprintf(buf, buf_len, " IND");
-				buf += ret;
-				blen -= ret;
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
+				if (ret < 0)
+					return ret;
 			}
-			if (ret < 0)
-				return ret;
-			blen -= ret;
+
 			return buf_len - blen;
-			}
-		default: return 0;
+
+		default:
+			return 0;
 		}
 	case ARM_SPE_DATA_SOURCE:
 	case ARM_SPE_TIMESTAMP:
-		return snprintf(buf, buf_len, "%s %lld", name, payload);
+		return arm_spe_pkt_snprintf(&buf, &blen, "%s %lld", name, payload);
 	case ARM_SPE_ADDRESS:
 		switch (idx) {
 		case 0:
 		case 1: ns = !!(packet->payload & NS_FLAG);
 			el = (packet->payload & EL_FLAG) >> 61;
 			payload &= ~(0xffULL << 56);
-			return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d",
+			return arm_spe_pkt_snprintf(&buf, &blen,
+					"%s 0x%llx el%d ns=%d",
 				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
-		case 2:	return snprintf(buf, buf_len, "VA 0x%llx", payload);
+		case 2:
+			return arm_spe_pkt_snprintf(&buf, &blen,
+						    "VA 0x%llx", payload);
 		case 3:	ns = !!(packet->payload & NS_FLAG);
 			payload &= ~(0xffULL << 56);
-			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
-					payload, ns);
-		default: return 0;
+			return arm_spe_pkt_snprintf(&buf, &blen,
+						    "PA 0x%llx ns=%d", payload, ns);
+		default:
+			return 0;
 		}
 	case ARM_SPE_CONTEXT:
-		return snprintf(buf, buf_len, "%s 0x%lx el%d", name,
-				(unsigned long)payload, idx + 1);
-	case ARM_SPE_COUNTER: {
-		size_t blen = buf_len;
-
-		ret = snprintf(buf, buf_len, "%s %d ", name,
-			       (unsigned short)payload);
-		buf += ret;
-		blen -= ret;
-		switch (idx) {
-		case 0:	ret = snprintf(buf, buf_len, "TOT"); break;
-		case 1:	ret = snprintf(buf, buf_len, "ISSUE"); break;
-		case 2:	ret = snprintf(buf, buf_len, "XLAT"); break;
-		default: ret = 0;
-		}
+		return arm_spe_pkt_snprintf(&buf, &blen, "%s 0x%lx el%d",
+					    name, (unsigned long)payload, idx + 1);
+	case ARM_SPE_COUNTER:
+		ret = arm_spe_pkt_snprintf(&buf, &blen, "%s %d ", name,
+					   (unsigned short)payload);
 		if (ret < 0)
 			return ret;
-		blen -= ret;
+
+		switch (idx) {
+		case 0:
+			ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT");
+			if (ret < 0)
+				return ret;
+			break;
+		case 1:
+			ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE");
+			if (ret < 0)
+				return ret;
+			break;
+		case 2:
+			ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT");
+			if (ret < 0)
+				return ret;
+			break;
+		default:
+			break;
+		}
+
 		return buf_len - blen;
-	}
+
 	default:
 		break;
 	}
 
-	return snprintf(buf, buf_len, "%s 0x%llx (%d)",
-			name, payload, packet->index);
+	return arm_spe_pkt_snprintf(&buf, &blen, "%s 0x%llx (%d)",
+				    name, payload, packet->index);
 }
-- 
2.17.1

[PATCH v3 07/20] perf arm-spe: Refactor packet header parsing

[Leo Yan]

The packet header parsing uses the hard coded values and it uses nested
if-else statements.

To improve the readability, this patch refactors the macros for packet
header format so it removes the hard coded values.  Furthermore, based
on the new mask macros it reduces the nested if-else statements and
changes to use the flat conditions checking, this is directive and can
easily map to the descriptions in ARMv8-a architecture reference manual
(ARM DDI 0487E.a), chapter 'D10.1.5 Statistical Profiling Extension
protocol packet headers'.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 92 +++++++++----------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 20 ++++
 2 files changed, 61 insertions(+), 51 deletions(-)

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 c7b6dc016f11..6f2329990729 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
@@ -16,28 +16,6 @@
 #define NS_FLAG		BIT(63)
 #define EL_FLAG		(BIT(62) | BIT(61))
 
-#define SPE_HEADER0_PAD			0x0
-#define SPE_HEADER0_END			0x1
-#define SPE_HEADER0_ADDRESS		0x30 /* address packet (short) */
-#define SPE_HEADER0_ADDRESS_MASK	0x38
-#define SPE_HEADER0_COUNTER		0x18 /* counter packet (short) */
-#define SPE_HEADER0_COUNTER_MASK	0x38
-#define SPE_HEADER0_TIMESTAMP		0x71
-#define SPE_HEADER0_TIMESTAMP		0x71
-#define SPE_HEADER0_EVENTS		0x2
-#define SPE_HEADER0_EVENTS_MASK		0xf
-#define SPE_HEADER0_SOURCE		0x3
-#define SPE_HEADER0_SOURCE_MASK		0xf
-#define SPE_HEADER0_CONTEXT		0x24
-#define SPE_HEADER0_CONTEXT_MASK	0x3c
-#define SPE_HEADER0_OP_TYPE		0x8
-#define SPE_HEADER0_OP_TYPE_MASK	0x3c
-#define SPE_HEADER1_ALIGNMENT		0x0
-#define SPE_HEADER1_ADDRESS		0xb0 /* address packet (extended) */
-#define SPE_HEADER1_ADDRESS_MASK	0xf8
-#define SPE_HEADER1_COUNTER		0x98 /* counter packet (extended) */
-#define SPE_HEADER1_COUNTER_MASK	0xf8
-
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define le16_to_cpu bswap_16
 #define le32_to_cpu bswap_32
@@ -200,46 +178,58 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
 static int arm_spe_do_get_packet(const unsigned char *buf, size_t len,
 				 struct arm_spe_pkt *packet)
 {
-	unsigned int byte;
+	unsigned int hdr;
+	unsigned char ext_hdr = 0;
 
 	memset(packet, 0, sizeof(struct arm_spe_pkt));
 
 	if (!len)
 		return ARM_SPE_NEED_MORE_BYTES;
 
-	byte = buf[0];
-	if (byte == SPE_HEADER0_PAD)
+	hdr = buf[0];
+
+	if (hdr == SPE_HEADER0_PAD)
 		return arm_spe_get_pad(packet);
-	else if (byte == SPE_HEADER0_END) /* no timestamp at end of record */
+
+	if (hdr == SPE_HEADER0_END) /* no timestamp at end of record */
 		return arm_spe_get_end(packet);
-	else if (byte & 0xc0 /* 0y11xxxxxx */) {
-		if (byte & 0x80) {
-			if ((byte & SPE_HEADER0_ADDRESS_MASK) == SPE_HEADER0_ADDRESS)
-				return arm_spe_get_addr(buf, len, 0, packet);
-			if ((byte & SPE_HEADER0_COUNTER_MASK) == SPE_HEADER0_COUNTER)
-				return arm_spe_get_counter(buf, len, 0, packet);
-		} else
-			if (byte == SPE_HEADER0_TIMESTAMP)
-				return arm_spe_get_timestamp(buf, len, packet);
-			else if ((byte & SPE_HEADER0_EVENTS_MASK) == SPE_HEADER0_EVENTS)
-				return arm_spe_get_events(buf, len, packet);
-			else if ((byte & SPE_HEADER0_SOURCE_MASK) == SPE_HEADER0_SOURCE)
-				return arm_spe_get_data_source(buf, len, packet);
-			else if ((byte & SPE_HEADER0_CONTEXT_MASK) == SPE_HEADER0_CONTEXT)
-				return arm_spe_get_context(buf, len, packet);
-			else if ((byte & SPE_HEADER0_OP_TYPE_MASK) == SPE_HEADER0_OP_TYPE)
-				return arm_spe_get_op_type(buf, len, packet);
-	} else if ((byte & 0xe0) == 0x20 /* 0y001xxxxx */) {
-		/* 16-bit header */
-		byte = buf[1];
-		if (byte == SPE_HEADER1_ALIGNMENT)
+
+	if (hdr == SPE_HEADER0_TIMESTAMP)
+		return arm_spe_get_timestamp(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_EVENTS)
+		return arm_spe_get_events(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_SOURCE)
+		return arm_spe_get_data_source(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_CONTEXT)
+		return arm_spe_get_context(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_OP_TYPE)
+		return arm_spe_get_op_type(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_EXTENDED) {
+		/* 16-bit extended format header */
+		ext_hdr = 1;
+
+		hdr = buf[1];
+		if (hdr == SPE_HEADER1_ALIGNMENT)
 			return arm_spe_get_alignment(buf, len, packet);
-		else if ((byte & SPE_HEADER1_ADDRESS_MASK) == SPE_HEADER1_ADDRESS)
-			return arm_spe_get_addr(buf, len, 1, packet);
-		else if ((byte & SPE_HEADER1_COUNTER_MASK) == SPE_HEADER1_COUNTER)
-			return arm_spe_get_counter(buf, len, 1, packet);
 	}
 
+	/*
+	 * The short format header's byte 0 or the extended format header's
+	 * byte 1 has been assigned to 'hdr', which uses the same encoding for
+	 * address packet and counter packet, so don't need to distinguish if
+	 * it's short format or extended format and handle in once.
+	 */
+	if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_ADDRESS)
+		return arm_spe_get_addr(buf, len, ext_hdr, packet);
+
+	if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_COUNTER)
+		return arm_spe_get_counter(buf, len, ext_hdr, packet);
+
 	return ARM_SPE_BAD_PACKET;
 }
 
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 e9ea8e3ead5d..68552ff8a8f7 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
@@ -37,6 +37,26 @@ struct arm_spe_pkt {
 	uint64_t		payload;
 };
 
+/* Short header (HEADER0) and extended header (HEADER1) */
+#define SPE_HEADER0_PAD				0x0
+#define SPE_HEADER0_END				0x1
+#define SPE_HEADER0_TIMESTAMP			0x71
+/* Mask for event & data source */
+#define SPE_HEADER0_MASK1			(GENMASK_ULL(7, 6) | GENMASK_ULL(3, 0))
+#define SPE_HEADER0_EVENTS			0x42
+#define SPE_HEADER0_SOURCE			0x43
+/* Mask for context & operation */
+#define SPE_HEADER0_MASK2			GENMASK_ULL(7, 2)
+#define SPE_HEADER0_CONTEXT			0x64
+#define SPE_HEADER0_OP_TYPE			0x48
+/* Mask for extended format */
+#define SPE_HEADER0_EXTENDED			0x20
+/* Mask for address & counter */
+#define SPE_HEADER0_MASK3			GENMASK_ULL(7, 3)
+#define SPE_HEADER0_ADDRESS			0xb0
+#define SPE_HEADER0_COUNTER			0x98
+#define SPE_HEADER1_ALIGNMENT			0x0
+
 #define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)
 
 #define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
-- 
2.17.1

Re: [PATCH v3 07/20] perf arm-spe: Refactor packet header parsing

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> The packet header parsing uses the hard coded values and it uses nested
> if-else statements.
> 
> To improve the readability, this patch refactors the macros for packet
> header format so it removes the hard coded values.  Furthermore, based
> on the new mask macros it reduces the nested if-else statements and
> changes to use the flat conditions checking, this is directive and can
> easily map to the descriptions in ARMv8-a architecture reference manual
> (ARM DDI 0487E.a), chapter 'D10.1.5 Statistical Profiling Extension
> protocol packet headers'.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Compared against the previous version, which I had checked already
against the manual. Thanks for the fixes!

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks,
Andre


> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 92 +++++++++----------
>  .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 20 ++++
>  2 files changed, 61 insertions(+), 51 deletions(-)
> 
> 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 c7b6dc016f11..6f2329990729 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
> @@ -16,28 +16,6 @@
>  #define NS_FLAG		BIT(63)
>  #define EL_FLAG		(BIT(62) | BIT(61))
>  
> -#define SPE_HEADER0_PAD			0x0
> -#define SPE_HEADER0_END			0x1
> -#define SPE_HEADER0_ADDRESS		0x30 /* address packet (short) */
> -#define SPE_HEADER0_ADDRESS_MASK	0x38
> -#define SPE_HEADER0_COUNTER		0x18 /* counter packet (short) */
> -#define SPE_HEADER0_COUNTER_MASK	0x38
> -#define SPE_HEADER0_TIMESTAMP		0x71
> -#define SPE_HEADER0_TIMESTAMP		0x71
> -#define SPE_HEADER0_EVENTS		0x2
> -#define SPE_HEADER0_EVENTS_MASK		0xf
> -#define SPE_HEADER0_SOURCE		0x3
> -#define SPE_HEADER0_SOURCE_MASK		0xf
> -#define SPE_HEADER0_CONTEXT		0x24
> -#define SPE_HEADER0_CONTEXT_MASK	0x3c
> -#define SPE_HEADER0_OP_TYPE		0x8
> -#define SPE_HEADER0_OP_TYPE_MASK	0x3c
> -#define SPE_HEADER1_ALIGNMENT		0x0
> -#define SPE_HEADER1_ADDRESS		0xb0 /* address packet (extended) */
> -#define SPE_HEADER1_ADDRESS_MASK	0xf8
> -#define SPE_HEADER1_COUNTER		0x98 /* counter packet (extended) */
> -#define SPE_HEADER1_COUNTER_MASK	0xf8
> -
>  #if __BYTE_ORDER == __BIG_ENDIAN
>  #define le16_to_cpu bswap_16
>  #define le32_to_cpu bswap_32
> @@ -200,46 +178,58 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
>  static int arm_spe_do_get_packet(const unsigned char *buf, size_t len,
>  				 struct arm_spe_pkt *packet)
>  {
> -	unsigned int byte;
> +	unsigned int hdr;
> +	unsigned char ext_hdr = 0;
>  
>  	memset(packet, 0, sizeof(struct arm_spe_pkt));
>  
>  	if (!len)
>  		return ARM_SPE_NEED_MORE_BYTES;
>  
> -	byte = buf[0];
> -	if (byte == SPE_HEADER0_PAD)
> +	hdr = buf[0];
> +
> +	if (hdr == SPE_HEADER0_PAD)
>  		return arm_spe_get_pad(packet);
> -	else if (byte == SPE_HEADER0_END) /* no timestamp at end of record */
> +
> +	if (hdr == SPE_HEADER0_END) /* no timestamp at end of record */
>  		return arm_spe_get_end(packet);
> -	else if (byte & 0xc0 /* 0y11xxxxxx */) {
> -		if (byte & 0x80) {
> -			if ((byte & SPE_HEADER0_ADDRESS_MASK) == SPE_HEADER0_ADDRESS)
> -				return arm_spe_get_addr(buf, len, 0, packet);
> -			if ((byte & SPE_HEADER0_COUNTER_MASK) == SPE_HEADER0_COUNTER)
> -				return arm_spe_get_counter(buf, len, 0, packet);
> -		} else
> -			if (byte == SPE_HEADER0_TIMESTAMP)
> -				return arm_spe_get_timestamp(buf, len, packet);
> -			else if ((byte & SPE_HEADER0_EVENTS_MASK) == SPE_HEADER0_EVENTS)
> -				return arm_spe_get_events(buf, len, packet);
> -			else if ((byte & SPE_HEADER0_SOURCE_MASK) == SPE_HEADER0_SOURCE)
> -				return arm_spe_get_data_source(buf, len, packet);
> -			else if ((byte & SPE_HEADER0_CONTEXT_MASK) == SPE_HEADER0_CONTEXT)
> -				return arm_spe_get_context(buf, len, packet);
> -			else if ((byte & SPE_HEADER0_OP_TYPE_MASK) == SPE_HEADER0_OP_TYPE)
> -				return arm_spe_get_op_type(buf, len, packet);
> -	} else if ((byte & 0xe0) == 0x20 /* 0y001xxxxx */) {
> -		/* 16-bit header */
> -		byte = buf[1];
> -		if (byte == SPE_HEADER1_ALIGNMENT)
> +
> +	if (hdr == SPE_HEADER0_TIMESTAMP)
> +		return arm_spe_get_timestamp(buf, len, packet);
> +
> +	if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_EVENTS)
> +		return arm_spe_get_events(buf, len, packet);
> +
> +	if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_SOURCE)
> +		return arm_spe_get_data_source(buf, len, packet);
> +
> +	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_CONTEXT)
> +		return arm_spe_get_context(buf, len, packet);
> +
> +	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_OP_TYPE)
> +		return arm_spe_get_op_type(buf, len, packet);
> +
> +	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_EXTENDED) {
> +		/* 16-bit extended format header */
> +		ext_hdr = 1;
> +
> +		hdr = buf[1];
> +		if (hdr == SPE_HEADER1_ALIGNMENT)
>  			return arm_spe_get_alignment(buf, len, packet);
> -		else if ((byte & SPE_HEADER1_ADDRESS_MASK) == SPE_HEADER1_ADDRESS)
> -			return arm_spe_get_addr(buf, len, 1, packet);
> -		else if ((byte & SPE_HEADER1_COUNTER_MASK) == SPE_HEADER1_COUNTER)
> -			return arm_spe_get_counter(buf, len, 1, packet);
>  	}
>  
> +	/*
> +	 * The short format header's byte 0 or the extended format header's
> +	 * byte 1 has been assigned to 'hdr', which uses the same encoding for
> +	 * address packet and counter packet, so don't need to distinguish if
> +	 * it's short format or extended format and handle in once.
> +	 */
> +	if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_ADDRESS)
> +		return arm_spe_get_addr(buf, len, ext_hdr, packet);
> +
> +	if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_COUNTER)
> +		return arm_spe_get_counter(buf, len, ext_hdr, packet);
> +
>  	return ARM_SPE_BAD_PACKET;
>  }
>  
> 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 e9ea8e3ead5d..68552ff8a8f7 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
> @@ -37,6 +37,26 @@ struct arm_spe_pkt {
>  	uint64_t		payload;
>  };
>  
> +/* Short header (HEADER0) and extended header (HEADER1) */
> +#define SPE_HEADER0_PAD				0x0
> +#define SPE_HEADER0_END				0x1
> +#define SPE_HEADER0_TIMESTAMP			0x71
> +/* Mask for event & data source */
> +#define SPE_HEADER0_MASK1			(GENMASK_ULL(7, 6) | GENMASK_ULL(3, 0))
> +#define SPE_HEADER0_EVENTS			0x42
> +#define SPE_HEADER0_SOURCE			0x43
> +/* Mask for context & operation */
> +#define SPE_HEADER0_MASK2			GENMASK_ULL(7, 2)
> +#define SPE_HEADER0_CONTEXT			0x64
> +#define SPE_HEADER0_OP_TYPE			0x48
> +/* Mask for extended format */
> +#define SPE_HEADER0_EXTENDED			0x20
> +/* Mask for address & counter */
> +#define SPE_HEADER0_MASK3			GENMASK_ULL(7, 3)
> +#define SPE_HEADER0_ADDRESS			0xb0
> +#define SPE_HEADER0_COUNTER			0x98
> +#define SPE_HEADER1_ALIGNMENT			0x0
> +
>  #define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)
>  
>  #define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
> 

[PATCH v3 08/20] perf arm-spe: Add new function arm_spe_pkt_desc_addr()

[Leo Yan]

This patch moves out the address parsing code from arm_spe_pkt_desc()
and uses the new introduced function arm_spe_pkt_desc_addr() to process
address packet.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 49 ++++++++++++-------
 1 file changed, 30 insertions(+), 19 deletions(-)

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 6f2329990729..550cd7648c73 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
@@ -267,10 +267,38 @@ static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
 	return ret;
 }
 
+static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
+				 char *buf, size_t buf_len)
+{
+	int ns, el, idx = packet->index;
+	u64 payload = packet->payload;
+
+	switch (idx) {
+	case 0:
+	case 1:
+		ns = !!(packet->payload & NS_FLAG);
+		el = (packet->payload & EL_FLAG) >> 61;
+		payload &= ~(0xffULL << 56);
+		return arm_spe_pkt_snprintf(&buf, &buf_len,
+				"%s 0x%llx el%d ns=%d",
+				(idx == 1) ? "TGT" : "PC", payload, el, ns);
+	case 2:
+		return arm_spe_pkt_snprintf(&buf, &buf_len,
+					    "VA 0x%llx", payload);
+	case 3:
+		ns = !!(packet->payload & NS_FLAG);
+		payload &= ~(0xffULL << 56);
+		return arm_spe_pkt_snprintf(&buf, &buf_len,
+					    "PA 0x%llx ns=%d", payload, ns);
+	default:
+		return 0;
+	}
+}
+
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
-	int ret, ns, el, idx = packet->index;
+	int ret, idx = packet->index;
 	unsigned long long payload = packet->payload;
 	const char *name = arm_spe_pkt_name(packet->type);
 	size_t blen = buf_len;
@@ -404,24 +432,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	case ARM_SPE_TIMESTAMP:
 		return arm_spe_pkt_snprintf(&buf, &blen, "%s %lld", name, payload);
 	case ARM_SPE_ADDRESS:
-		switch (idx) {
-		case 0:
-		case 1: ns = !!(packet->payload & NS_FLAG);
-			el = (packet->payload & EL_FLAG) >> 61;
-			payload &= ~(0xffULL << 56);
-			return arm_spe_pkt_snprintf(&buf, &blen,
-					"%s 0x%llx el%d ns=%d",
-				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
-		case 2:
-			return arm_spe_pkt_snprintf(&buf, &blen,
-						    "VA 0x%llx", payload);
-		case 3:	ns = !!(packet->payload & NS_FLAG);
-			payload &= ~(0xffULL << 56);
-			return arm_spe_pkt_snprintf(&buf, &blen,
-						    "PA 0x%llx ns=%d", payload, ns);
-		default:
-			return 0;
-		}
+		return arm_spe_pkt_desc_addr(packet, buf, buf_len);
 	case ARM_SPE_CONTEXT:
 		return arm_spe_pkt_snprintf(&buf, &blen, "%s 0x%lx el%d",
 					    name, (unsigned long)payload, idx + 1);
-- 
2.17.1

Re: [PATCH v3 08/20] perf arm-spe: Add new function arm_spe_pkt_desc_addr()

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:
> This patch moves out the address parsing code from arm_spe_pkt_desc()
> and uses the new introduced function arm_spe_pkt_desc_addr() to process
> address packet.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Can confirm the move is correct.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 49 ++++++++++++-------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> 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 6f2329990729..550cd7648c73 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
> @@ -267,10 +267,38 @@ static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
>  	return ret;
>  }
>  
> +static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
> +				 char *buf, size_t buf_len)
> +{
> +	int ns, el, idx = packet->index;
> +	u64 payload = packet->payload;
> +
> +	switch (idx) {
> +	case 0:
> +	case 1:
> +		ns = !!(packet->payload & NS_FLAG);
> +		el = (packet->payload & EL_FLAG) >> 61;
> +		payload &= ~(0xffULL << 56);
> +		return arm_spe_pkt_snprintf(&buf, &buf_len,
> +				"%s 0x%llx el%d ns=%d",
> +				(idx == 1) ? "TGT" : "PC", payload, el, ns);
> +	case 2:
> +		return arm_spe_pkt_snprintf(&buf, &buf_len,
> +					    "VA 0x%llx", payload);
> +	case 3:
> +		ns = !!(packet->payload & NS_FLAG);
> +		payload &= ~(0xffULL << 56);
> +		return arm_spe_pkt_snprintf(&buf, &buf_len,
> +					    "PA 0x%llx ns=%d", payload, ns);
> +	default:
> +		return 0;
> +	}
> +}
> +
>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  		     size_t buf_len)
>  {
> -	int ret, ns, el, idx = packet->index;
> +	int ret, idx = packet->index;
>  	unsigned long long payload = packet->payload;
>  	const char *name = arm_spe_pkt_name(packet->type);
>  	size_t blen = buf_len;
> @@ -404,24 +432,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	case ARM_SPE_TIMESTAMP:
>  		return arm_spe_pkt_snprintf(&buf, &blen, "%s %lld", name, payload);
>  	case ARM_SPE_ADDRESS:
> -		switch (idx) {
> -		case 0:
> -		case 1: ns = !!(packet->payload & NS_FLAG);
> -			el = (packet->payload & EL_FLAG) >> 61;
> -			payload &= ~(0xffULL << 56);
> -			return arm_spe_pkt_snprintf(&buf, &blen,
> -					"%s 0x%llx el%d ns=%d",
> -				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
> -		case 2:
> -			return arm_spe_pkt_snprintf(&buf, &blen,
> -						    "VA 0x%llx", payload);
> -		case 3:	ns = !!(packet->payload & NS_FLAG);
> -			payload &= ~(0xffULL << 56);
> -			return arm_spe_pkt_snprintf(&buf, &blen,
> -						    "PA 0x%llx ns=%d", payload, ns);
> -		default:
> -			return 0;
> -		}
> +		return arm_spe_pkt_desc_addr(packet, buf, buf_len);
>  	case ARM_SPE_CONTEXT:
>  		return arm_spe_pkt_snprintf(&buf, &blen, "%s 0x%lx el%d",
>  					    name, (unsigned long)payload, idx + 1);
> 

[PATCH v3 09/20] perf arm-spe: Refactor address packet handling

[Leo Yan]

This patch is to refactor address packet handling, it defines macros for
address packet's header and payload, these macros are used by decoder
and the dump flow.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../util/arm-spe-decoder/arm-spe-decoder.c    | 29 ++++++++--------
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 26 +++++++-------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 34 ++++++++++++-------
 3 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index cc18a1e8c212..776b3e6628bb 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -24,36 +24,35 @@
 
 static u64 arm_spe_calc_ip(int index, u64 payload)
 {
-	u8 *addr = (u8 *)&payload;
-	int ns, el;
+	u64 ns, el;
 
 	/* Instruction virtual address or Branch target address */
 	if (index == SPE_ADDR_PKT_HDR_INDEX_INS ||
 	    index == SPE_ADDR_PKT_HDR_INDEX_BRANCH) {
-		ns = addr[7] & SPE_ADDR_PKT_NS;
-		el = (addr[7] & SPE_ADDR_PKT_EL_MASK) >> SPE_ADDR_PKT_EL_OFFSET;
+		ns = SPE_ADDR_PKT_GET_NS(payload);
+		el = SPE_ADDR_PKT_GET_EL(payload);
+
+		/* Clean highest byte */
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 
 		/* Fill highest byte for EL1 or EL2 (VHE) mode */
 		if (ns && (el == SPE_ADDR_PKT_EL1 || el == SPE_ADDR_PKT_EL2))
-			addr[7] = 0xff;
-		/* Clean highest byte for other cases */
-		else
-			addr[7] = 0x0;
+			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
 
 	/* Data access virtual address */
 	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT) {
 
+		/* Clean tags */
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
+
 		/* Fill highest byte if bits [48..55] is 0xff */
-		if (addr[6] == 0xff)
-			addr[7] = 0xff;
-		/* Otherwise, cleanup tags */
-		else
-			addr[7] = 0x0;
+		if (SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload) == 0xffULL)
+			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
 
 	/* Data access physical address */
 	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS) {
-		/* Cleanup byte 7 */
-		addr[7] = 0x0;
+		/* Clean highest byte */
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 	} else {
 		pr_err("unsupported address packet index: 0x%x\n", index);
 	}
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 550cd7648c73..156f98d6b8b2 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
@@ -13,9 +13,6 @@
 
 #include "arm-spe-pkt-decoder.h"
 
-#define NS_FLAG		BIT(63)
-#define EL_FLAG		(BIT(62) | BIT(61))
-
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define le16_to_cpu bswap_16
 #define le32_to_cpu bswap_32
@@ -167,10 +164,11 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
 			    const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_ADDRESS;
+
 	if (ext_hdr)
-		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
+		packet->index = SPE_ADDR_PKT_HDR_EXTENDED_INDEX(buf[0], buf[1]);
 	else
-		packet->index = buf[0] & 0x7;
+		packet->index = SPE_ADDR_PKT_HDR_SHORT_INDEX(buf[0]);
 
 	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
@@ -274,20 +272,20 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 	u64 payload = packet->payload;
 
 	switch (idx) {
-	case 0:
-	case 1:
-		ns = !!(packet->payload & NS_FLAG);
-		el = (packet->payload & EL_FLAG) >> 61;
-		payload &= ~(0xffULL << 56);
+	case SPE_ADDR_PKT_HDR_INDEX_INS:
+	case SPE_ADDR_PKT_HDR_INDEX_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);
 		return arm_spe_pkt_snprintf(&buf, &buf_len,
 				"%s 0x%llx el%d ns=%d",
 				(idx == 1) ? "TGT" : "PC", payload, el, ns);
-	case 2:
+	case SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT:
 		return arm_spe_pkt_snprintf(&buf, &buf_len,
 					    "VA 0x%llx", payload);
-	case 3:
-		ns = !!(packet->payload & NS_FLAG);
-		payload &= ~(0xffULL << 56);
+	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
+		ns = !!SPE_ADDR_PKT_GET_NS(payload);
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 		return arm_spe_pkt_snprintf(&buf, &buf_len,
 					    "PA 0x%llx ns=%d", payload, ns);
 	default:
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 68552ff8a8f7..4111550d2bde 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
@@ -59,19 +59,27 @@ struct arm_spe_pkt {
 
 #define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)
 
-#define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
-#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_NS				BIT(7)
-#define SPE_ADDR_PKT_CH				BIT(6)
-#define SPE_ADDR_PKT_EL_OFFSET			(5)
-#define SPE_ADDR_PKT_EL_MASK			(0x3 << SPE_ADDR_PKT_EL_OFFSET)
-#define SPE_ADDR_PKT_EL0			(0)
-#define SPE_ADDR_PKT_EL1			(1)
-#define SPE_ADDR_PKT_EL2			(2)
-#define SPE_ADDR_PKT_EL3			(3)
+/* Address packet header */
+#define SPE_ADDR_PKT_HDR_SHORT_INDEX(h)		((h) & GENMASK_ULL(2, 0))
+#define SPE_ADDR_PKT_HDR_EXTENDED_INDEX(h0, h1)	(((h0) & GENMASK_ULL(1, 0)) << 3 | \
+						 SPE_ADDR_PKT_HDR_SHORT_INDEX(h1))
+#define SPE_ADDR_PKT_HDR_INDEX_INS		0x0
+#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
+
+/* Address packet payload */
+#define SPE_ADDR_PKT_ADDR_BYTE7_SHIFT		56
+#define SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(v)	((v) & GENMASK_ULL(55, 0))
+#define SPE_ADDR_PKT_ADDR_GET_BYTE_6(v)		(((v) & GENMASK_ULL(55, 48)) >> 48)
+
+#define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT(63)) >> 63)
+#define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
+
+#define SPE_ADDR_PKT_EL0			0
+#define SPE_ADDR_PKT_EL1			1
+#define SPE_ADDR_PKT_EL2			2
+#define SPE_ADDR_PKT_EL3			3
 
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
-- 
2.17.1

Re: [PATCH v3 09/20] perf arm-spe: Refactor address packet handling

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi Leo,

> This patch is to refactor address packet handling, it defines macros for
> address packet's header and payload, these macros are used by decoder
> and the dump flow.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  .../util/arm-spe-decoder/arm-spe-decoder.c    | 29 ++++++++--------
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 26 +++++++-------
>  .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 34 ++++++++++++-------
>  3 files changed, 47 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> index cc18a1e8c212..776b3e6628bb 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> @@ -24,36 +24,35 @@
>  
>  static u64 arm_spe_calc_ip(int index, u64 payload)
>  {
> -	u8 *addr = (u8 *)&payload;
> -	int ns, el;
> +	u64 ns, el;
>  
>  	/* Instruction virtual address or Branch target address */
>  	if (index == SPE_ADDR_PKT_HDR_INDEX_INS ||
>  	    index == SPE_ADDR_PKT_HDR_INDEX_BRANCH) {
> -		ns = addr[7] & SPE_ADDR_PKT_NS;
> -		el = (addr[7] & SPE_ADDR_PKT_EL_MASK) >> SPE_ADDR_PKT_EL_OFFSET;
> +		ns = SPE_ADDR_PKT_GET_NS(payload);
> +		el = SPE_ADDR_PKT_GET_EL(payload);
> +
> +		/* Clean highest byte */
> +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
>  
>  		/* Fill highest byte for EL1 or EL2 (VHE) mode */
>  		if (ns && (el == SPE_ADDR_PKT_EL1 || el == SPE_ADDR_PKT_EL2))
> -			addr[7] = 0xff;
> -		/* Clean highest byte for other cases */
> -		else
> -			addr[7] = 0x0;
> +			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
>  
>  	/* Data access virtual address */
>  	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT) {
>  
> +		/* Clean tags */
> +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> +
>  		/* Fill highest byte if bits [48..55] is 0xff */

Do you know where this comes from? If yes, can you replace the comment
with the reason, so that it says *why* and not *what* the code does?

> -		if (addr[6] == 0xff)
> -			addr[7] = 0xff;
> -		/* Otherwise, cleanup tags */
> -		else
> -			addr[7] = 0x0;
> +		if (SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload) == 0xffULL)
> +			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
>  
>  	/* Data access physical address */
>  	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS) {
> -		/* Cleanup byte 7 */
> -		addr[7] = 0x0;
> +		/* Clean highest byte */
> +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
>  	} else {
>  		pr_err("unsupported address packet index: 0x%x\n", index);
>  	}
> 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 550cd7648c73..156f98d6b8b2 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
> @@ -13,9 +13,6 @@
>  
>  #include "arm-spe-pkt-decoder.h"
>  
> -#define NS_FLAG		BIT(63)
> -#define EL_FLAG		(BIT(62) | BIT(61))
> -
>  #if __BYTE_ORDER == __BIG_ENDIAN
>  #define le16_to_cpu bswap_16
>  #define le32_to_cpu bswap_32
> @@ -167,10 +164,11 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
>  			    const unsigned char ext_hdr, struct arm_spe_pkt *packet)
>  {
>  	packet->type = ARM_SPE_ADDRESS;
> +
>  	if (ext_hdr)
> -		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
> +		packet->index = SPE_ADDR_PKT_HDR_EXTENDED_INDEX(buf[0], buf[1]);
>  	else
> -		packet->index = buf[0] & 0x7;
> +		packet->index = SPE_ADDR_PKT_HDR_SHORT_INDEX(buf[0]);
>  
>  	return arm_spe_get_payload(buf, len, ext_hdr, packet);
>  }
> @@ -274,20 +272,20 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  	u64 payload = packet->payload;
>  
>  	switch (idx) {
> -	case 0:
> -	case 1:
> -		ns = !!(packet->payload & NS_FLAG);
> -		el = (packet->payload & EL_FLAG) >> 61;
> -		payload &= ~(0xffULL << 56);
> +	case SPE_ADDR_PKT_HDR_INDEX_INS:
> +	case SPE_ADDR_PKT_HDR_INDEX_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);
>  		return arm_spe_pkt_snprintf(&buf, &buf_len,
>  				"%s 0x%llx el%d ns=%d",
>  				(idx == 1) ? "TGT" : "PC", payload, el, ns);
> -	case 2:
> +	case SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT:
>  		return arm_spe_pkt_snprintf(&buf, &buf_len,
>  					    "VA 0x%llx", payload);
> -	case 3:
> -		ns = !!(packet->payload & NS_FLAG);
> -		payload &= ~(0xffULL << 56);
> +	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
> +		ns = !!SPE_ADDR_PKT_GET_NS(payload);
> +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
>  		return arm_spe_pkt_snprintf(&buf, &buf_len,
>  					    "PA 0x%llx ns=%d", payload, ns);
>  	default:
> 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 68552ff8a8f7..4111550d2bde 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
> @@ -59,19 +59,27 @@ struct arm_spe_pkt {
>  
>  #define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)
>  
> -#define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
> -#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_NS				BIT(7)
> -#define SPE_ADDR_PKT_CH				BIT(6)
> -#define SPE_ADDR_PKT_EL_OFFSET			(5)
> -#define SPE_ADDR_PKT_EL_MASK			(0x3 << SPE_ADDR_PKT_EL_OFFSET)
> -#define SPE_ADDR_PKT_EL0			(0)
> -#define SPE_ADDR_PKT_EL1			(1)
> -#define SPE_ADDR_PKT_EL2			(2)
> -#define SPE_ADDR_PKT_EL3			(3)
> +/* Address packet header */
> +#define SPE_ADDR_PKT_HDR_SHORT_INDEX(h)		((h) & GENMASK_ULL(2, 0))
> +#define SPE_ADDR_PKT_HDR_EXTENDED_INDEX(h0, h1)	(((h0) & GENMASK_ULL(1, 0)) << 3 | \
> +						 SPE_ADDR_PKT_HDR_SHORT_INDEX(h1))

Did you consider sharing those two with the identical definition for the
extended counter packet? This extended packet seems more like a generic
concept, regardless of the packet type.

> +#define SPE_ADDR_PKT_HDR_INDEX_INS		0x0
> +#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
> +
> +/* Address packet payload */
> +#define SPE_ADDR_PKT_ADDR_BYTE7_SHIFT		56
> +#define SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(v)	((v) & GENMASK_ULL(55, 0))
> +#define SPE_ADDR_PKT_ADDR_GET_BYTE_6(v)		(((v) & GENMASK_ULL(55, 48)) >> 48)
> +
> +#define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT(63)) >> 63)

You need BIT_ULL(63) here to make this work on 32-bit systems.

Cheers,
Andre

> +#define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
> +
> +#define SPE_ADDR_PKT_EL0			0
> +#define SPE_ADDR_PKT_EL1			1
> +#define SPE_ADDR_PKT_EL2			2
> +#define SPE_ADDR_PKT_EL3			3
>  
>  const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
>  
> 

Re: [PATCH v3 09/20] perf arm-spe: Refactor address packet handling

[Leo Yan]

On Fri, Oct 23, 2020 at 06:53:35PM +0100, André Przywara wrote:
> On 22/10/2020 15:58, Leo Yan wrote:
> 
> Hi Leo,
> 
> > This patch is to refactor address packet handling, it defines macros for
> > address packet's header and payload, these macros are used by decoder
> > and the dump flow.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  .../util/arm-spe-decoder/arm-spe-decoder.c    | 29 ++++++++--------
> >  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 26 +++++++-------
> >  .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 34 ++++++++++++-------
> >  3 files changed, 47 insertions(+), 42 deletions(-)
> > 
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > index cc18a1e8c212..776b3e6628bb 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > @@ -24,36 +24,35 @@
> >  
> >  static u64 arm_spe_calc_ip(int index, u64 payload)
> >  {
> > -	u8 *addr = (u8 *)&payload;
> > -	int ns, el;
> > +	u64 ns, el;
> >  
> >  	/* Instruction virtual address or Branch target address */
> >  	if (index == SPE_ADDR_PKT_HDR_INDEX_INS ||
> >  	    index == SPE_ADDR_PKT_HDR_INDEX_BRANCH) {
> > -		ns = addr[7] & SPE_ADDR_PKT_NS;
> > -		el = (addr[7] & SPE_ADDR_PKT_EL_MASK) >> SPE_ADDR_PKT_EL_OFFSET;
> > +		ns = SPE_ADDR_PKT_GET_NS(payload);
> > +		el = SPE_ADDR_PKT_GET_EL(payload);
> > +
> > +		/* Clean highest byte */
> > +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> >  
> >  		/* Fill highest byte for EL1 or EL2 (VHE) mode */
> >  		if (ns && (el == SPE_ADDR_PKT_EL1 || el == SPE_ADDR_PKT_EL2))
> > -			addr[7] = 0xff;
> > -		/* Clean highest byte for other cases */
> > -		else
> > -			addr[7] = 0x0;
> > +			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
> >  
> >  	/* Data access virtual address */
> >  	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT) {
> >  
> > +		/* Clean tags */
> > +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> > +
> >  		/* Fill highest byte if bits [48..55] is 0xff */
> 
> Do you know where this comes from? If yes, can you replace the comment
> with the reason, so that it says *why* and not *what* the code does?

I think I know where this comes from :)

Armv8 ARM defines the Address packet payload, the data virtual address
highest byte (byte7 for bits [63:56]) is assignment for TAG; if connect
with the kernel document Documentation/arm64/memory.rst, we can see the
prededined the "AArch64 Linux memory layout with 4KB pages + 4 levels",
though the payload's byte7 is for TAG, but we can check bits [48..55]
(byte6) and if it's 0xff, then we need fill the byte7 so can create a
valid virtual address for data accessing, this finally can allow perf
tool to find correct symbol for data structure.

  Start                 End                     Size            Use                      
  -----------------------------------------------------------------------                
  0000000000000000      0000ffffffffffff         256TB          user                     
  ffff000000000000      ffff7fffffffffff         128TB          kernel logical memory map
  ffff800000000000      ffff9fffffffffff          32TB          kasan shadow region      
  ffffa00000000000      ffffa00007ffffff         128MB          bpf jit region           
  ffffa00008000000      ffffa0000fffffff         128MB          modules                  
  ffffa00010000000      fffffdffbffeffff         ~93TB          vmalloc                  
  fffffdffbfff0000      fffffdfffe5f8fff        ~998MB          [guard region]           
  fffffdfffe5f9000      fffffdfffe9fffff        4124KB          fixed mappings           
  fffffdfffea00000      fffffdfffebfffff           2MB          [guard region]           
  fffffdfffec00000      fffffdffffbfffff          16MB          PCI I/O space            
  fffffdffffc00000      fffffdffffdfffff           2MB          [guard region]           
  fffffdffffe00000      ffffffffffdfffff           2TB          vmemmap                  
  ffffffffffe00000      ffffffffffffffff           2MB          [guard region]           

But I find it misses to support the "AArch64 Linux memory layout with
64KB pages + 3 levels (52-bit with HW support)":

  Start                 End                     Size            Use                      
  -----------------------------------------------------------------------                
  0000000000000000      000fffffffffffff           4PB          user                     
  fff0000000000000      fff7ffffffffffff           2PB          kernel logical memory map
  fff8000000000000      fffd9fffffffffff        1440TB          [gap]                    
  fffda00000000000      ffff9fffffffffff         512TB          kasan shadow region      
  ffffa00000000000      ffffa00007ffffff         128MB          bpf jit region           
  ffffa00008000000      ffffa0000fffffff         128MB          modules                  
  ffffa00010000000      fffff81ffffeffff         ~88TB          vmalloc                  
  fffff81fffff0000      fffffc1ffe58ffff          ~3TB          [guard region]           
  fffffc1ffe590000      fffffc1ffe9fffff        4544KB          fixed mappings           
  fffffc1ffea00000      fffffc1ffebfffff           2MB          [guard region]           
  fffffc1ffec00000      fffffc1fffbfffff          16MB          PCI I/O space            
  fffffc1fffc00000      fffffc1fffdfffff           2MB          [guard region]           
  fffffc1fffe00000      ffffffffffdfffff        3968GB          vmemmap                  
  ffffffffffe00000      ffffffffffffffff           2MB          [guard region]           

So if detect byte6 is "0xf0", "0xf8, "0xfd", we also need to fixup the
byte7 and fill 0xff to it.

If you think this is right way to process address packet, I will use a
separate to fix comment and support 64KB page fixup.

> > -		if (addr[6] == 0xff)
> > -			addr[7] = 0xff;
> > -		/* Otherwise, cleanup tags */
> > -		else
> > -			addr[7] = 0x0;
> > +		if (SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload) == 0xffULL)
> > +			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
> >  
> >  	/* Data access physical address */
> >  	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS) {
> > -		/* Cleanup byte 7 */
> > -		addr[7] = 0x0;
> > +		/* Clean highest byte */
> > +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> >  	} else {
> >  		pr_err("unsupported address packet index: 0x%x\n", index);
> >  	}
> > 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 550cd7648c73..156f98d6b8b2 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
> > @@ -13,9 +13,6 @@
> >  
> >  #include "arm-spe-pkt-decoder.h"
> >  
> > -#define NS_FLAG		BIT(63)
> > -#define EL_FLAG		(BIT(62) | BIT(61))
> > -
> >  #if __BYTE_ORDER == __BIG_ENDIAN
> >  #define le16_to_cpu bswap_16
> >  #define le32_to_cpu bswap_32
> > @@ -167,10 +164,11 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
> >  			    const unsigned char ext_hdr, struct arm_spe_pkt *packet)
> >  {
> >  	packet->type = ARM_SPE_ADDRESS;
> > +
> >  	if (ext_hdr)
> > -		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
> > +		packet->index = SPE_ADDR_PKT_HDR_EXTENDED_INDEX(buf[0], buf[1]);
> >  	else
> > -		packet->index = buf[0] & 0x7;
> > +		packet->index = SPE_ADDR_PKT_HDR_SHORT_INDEX(buf[0]);
> >  
> >  	return arm_spe_get_payload(buf, len, ext_hdr, packet);
> >  }
> > @@ -274,20 +272,20 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
> >  	u64 payload = packet->payload;
> >  
> >  	switch (idx) {
> > -	case 0:
> > -	case 1:
> > -		ns = !!(packet->payload & NS_FLAG);
> > -		el = (packet->payload & EL_FLAG) >> 61;
> > -		payload &= ~(0xffULL << 56);
> > +	case SPE_ADDR_PKT_HDR_INDEX_INS:
> > +	case SPE_ADDR_PKT_HDR_INDEX_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);
> >  		return arm_spe_pkt_snprintf(&buf, &buf_len,
> >  				"%s 0x%llx el%d ns=%d",
> >  				(idx == 1) ? "TGT" : "PC", payload, el, ns);
> > -	case 2:
> > +	case SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT:
> >  		return arm_spe_pkt_snprintf(&buf, &buf_len,
> >  					    "VA 0x%llx", payload);
> > -	case 3:
> > -		ns = !!(packet->payload & NS_FLAG);
> > -		payload &= ~(0xffULL << 56);
> > +	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
> > +		ns = !!SPE_ADDR_PKT_GET_NS(payload);
> > +		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> >  		return arm_spe_pkt_snprintf(&buf, &buf_len,
> >  					    "PA 0x%llx ns=%d", payload, ns);
> >  	default:
> > 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 68552ff8a8f7..4111550d2bde 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
> > @@ -59,19 +59,27 @@ struct arm_spe_pkt {
> >  
> >  #define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)
> >  
> > -#define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
> > -#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_NS				BIT(7)
> > -#define SPE_ADDR_PKT_CH				BIT(6)
> > -#define SPE_ADDR_PKT_EL_OFFSET			(5)
> > -#define SPE_ADDR_PKT_EL_MASK			(0x3 << SPE_ADDR_PKT_EL_OFFSET)
> > -#define SPE_ADDR_PKT_EL0			(0)
> > -#define SPE_ADDR_PKT_EL1			(1)
> > -#define SPE_ADDR_PKT_EL2			(2)
> > -#define SPE_ADDR_PKT_EL3			(3)
> > +/* Address packet header */
> > +#define SPE_ADDR_PKT_HDR_SHORT_INDEX(h)		((h) & GENMASK_ULL(2, 0))
> > +#define SPE_ADDR_PKT_HDR_EXTENDED_INDEX(h0, h1)	(((h0) & GENMASK_ULL(1, 0)) << 3 | \
> > +						 SPE_ADDR_PKT_HDR_SHORT_INDEX(h1))
> 
> Did you consider sharing those two with the identical definition for the
> extended counter packet? This extended packet seems more like a generic
> concept, regardless of the packet type.

TBH, I have considered for this :)  An exception is the Context packet,
it uses different format for index (bits [1:0]).

I think I can consolidate the index for Address packet and conter
packet for sharing definitions.

  #define SPE_PKT_HDR_SHORT_INDEX(h)		((h) & GENMASK_ULL(2, 0))
  #define SPE_PKT_HDR_EXTENDED_INDEX(h0, h1)	(((h0) & GENMASK_ULL(1, 0)) << 3 | \
  						 SPE_PKT_HDR_SHORT_INDEX(h1))

> > +#define SPE_ADDR_PKT_HDR_INDEX_INS		0x0
> > +#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
> > +
> > +/* Address packet payload */
> > +#define SPE_ADDR_PKT_ADDR_BYTE7_SHIFT		56
> > +#define SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(v)	((v) & GENMASK_ULL(55, 0))
> > +#define SPE_ADDR_PKT_ADDR_GET_BYTE_6(v)		(((v) & GENMASK_ULL(55, 48)) >> 48)
> > +
> > +#define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT(63)) >> 63)
> 
> You need BIT_ULL(63) here to make this work on 32-bit systems.

Will do.  Thanks!

Leo

[PATCH v3 10/20] perf arm-spe: Refactor context packet handling

[Leo Yan]

Minor refactoring to use macro for index mask.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 2 +-
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 3 +++
 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 156f98d6b8b2..1fc07c693640 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
@@ -136,7 +136,7 @@ static int arm_spe_get_context(const unsigned char *buf, size_t len,
 			       struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_CONTEXT;
-	packet->index = buf[0] & 0x3;
+	packet->index = SPE_CTX_PKT_HDR_INDEX(buf[0]);
 	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
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 4111550d2bde..8808f2d0b6e4 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
@@ -81,6 +81,9 @@ struct arm_spe_pkt {
 #define SPE_ADDR_PKT_EL2			2
 #define SPE_ADDR_PKT_EL3			3
 
+/* Context packet header */
+#define SPE_CTX_PKT_HDR_INDEX(h)		((h) & GENMASK_ULL(1, 0))
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v3 11/20] perf arm-spe: Add new function arm_spe_pkt_desc_counter()

[Leo Yan]

This patch moves out the counter packet parsing code from
arm_spe_pkt_desc() to the new function arm_spe_pkt_desc_counter().

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 64 +++++++++++--------
 1 file changed, 37 insertions(+), 27 deletions(-)

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 1fc07c693640..023bcc9be3cc 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
@@ -293,6 +293,42 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 	}
 }
 
+static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
+				    char *buf, size_t buf_len)
+{
+	u64 payload = packet->payload;
+	const char *name = arm_spe_pkt_name(packet->type);
+	size_t blen = buf_len;
+	int ret;
+
+	ret = arm_spe_pkt_snprintf(&buf, &blen, "%s %d ", name,
+				   (unsigned short)payload);
+	if (ret < 0)
+		return ret;
+
+	switch (packet->index) {
+	case 0:
+		ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT");
+		if (ret < 0)
+			return ret;
+		break;
+	case 1:
+		ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE");
+		if (ret < 0)
+			return ret;
+		break;
+	case 2:
+		ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT");
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		break;
+	}
+
+	return buf_len - blen;
+}
+
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
@@ -435,33 +471,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		return arm_spe_pkt_snprintf(&buf, &blen, "%s 0x%lx el%d",
 					    name, (unsigned long)payload, idx + 1);
 	case ARM_SPE_COUNTER:
-		ret = arm_spe_pkt_snprintf(&buf, &blen, "%s %d ", name,
-					   (unsigned short)payload);
-		if (ret < 0)
-			return ret;
-
-		switch (idx) {
-		case 0:
-			ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT");
-			if (ret < 0)
-				return ret;
-			break;
-		case 1:
-			ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE");
-			if (ret < 0)
-				return ret;
-			break;
-		case 2:
-			ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT");
-			if (ret < 0)
-				return ret;
-			break;
-		default:
-			break;
-		}
-
-		return buf_len - blen;
-
+		return arm_spe_pkt_desc_counter(packet, buf, buf_len);
 	default:
 		break;
 	}
-- 
2.17.1

Re: [PATCH v3 11/20] perf arm-spe: Add new function arm_spe_pkt_desc_counter()

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:
> This patch moves out the counter packet parsing code from
> arm_spe_pkt_desc() to the new function arm_spe_pkt_desc_counter().
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Confirmed by diff'ing '-' vs. '+' to not introduce an actual change.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 64 +++++++++++--------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
> 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 1fc07c693640..023bcc9be3cc 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
> @@ -293,6 +293,42 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  	}
>  }
>  
> +static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
> +				    char *buf, size_t buf_len)
> +{
> +	u64 payload = packet->payload;
> +	const char *name = arm_spe_pkt_name(packet->type);
> +	size_t blen = buf_len;
> +	int ret;
> +
> +	ret = arm_spe_pkt_snprintf(&buf, &blen, "%s %d ", name,
> +				   (unsigned short)payload);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (packet->index) {
> +	case 0:
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT");
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case 1:
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE");
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case 2:
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT");
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return buf_len - blen;
> +}
> +
>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  		     size_t buf_len)
>  {
> @@ -435,33 +471,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  		return arm_spe_pkt_snprintf(&buf, &blen, "%s 0x%lx el%d",
>  					    name, (unsigned long)payload, idx + 1);
>  	case ARM_SPE_COUNTER:
> -		ret = arm_spe_pkt_snprintf(&buf, &blen, "%s %d ", name,
> -					   (unsigned short)payload);
> -		if (ret < 0)
> -			return ret;
> -
> -		switch (idx) {
> -		case 0:
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT");
> -			if (ret < 0)
> -				return ret;
> -			break;
> -		case 1:
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE");
> -			if (ret < 0)
> -				return ret;
> -			break;
> -		case 2:
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT");
> -			if (ret < 0)
> -				return ret;
> -			break;
> -		default:
> -			break;
> -		}
> -
> -		return buf_len - blen;
> -
> +		return arm_spe_pkt_desc_counter(packet, buf, buf_len);
>  	default:
>  		break;
>  	}
> 

[PATCH v3 12/20] perf arm-spe: Refactor counter packet handling

[Leo Yan]

This patch defines macros for counter packet header, and uses macros to
replace hard code values in functions arm_spe_get_counter() and
arm_spe_pkt_desc().

In the function arm_spe_get_counter(), adds a new line for more
readable.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 11 ++++++-----
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h |  8 ++++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

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 023bcc9be3cc..6eebd30f3d78 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
@@ -152,10 +152,11 @@ static int arm_spe_get_counter(const unsigned char *buf, size_t len,
 			       const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_COUNTER;
+
 	if (ext_hdr)
-		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
+		packet->index = SPE_CNT_PKT_HDR_EXTENDED_INDEX(buf[0], buf[1]);
 	else
-		packet->index = buf[0] & 0x7;
+		packet->index = SPE_CNT_PKT_HDR_SHORT_INDEX(buf[0]);
 
 	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
@@ -307,17 +308,17 @@ static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
 		return ret;
 
 	switch (packet->index) {
-	case 0:
+	case SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT:
 		ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT");
 		if (ret < 0)
 			return ret;
 		break;
-	case 1:
+	case SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT:
 		ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE");
 		if (ret < 0)
 			return ret;
 		break;
-	case 2:
+	case SPE_CNT_PKT_HDR_INDEX_TRANS_LAT:
 		ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT");
 		if (ret < 0)
 			return ret;
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 8808f2d0b6e4..8a291f451ef8 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
@@ -84,6 +84,14 @@ struct arm_spe_pkt {
 /* Context packet header */
 #define SPE_CTX_PKT_HDR_INDEX(h)		((h) & GENMASK_ULL(1, 0))
 
+/* Counter packet header */
+#define SPE_CNT_PKT_HDR_SHORT_INDEX(h)		((h) & GENMASK_ULL(2, 0))
+#define SPE_CNT_PKT_HDR_EXTENDED_INDEX(h0, h1)	(((h0) & GENMASK_ULL(1, 0)) << 3 | \
+						 SPE_ADDR_PKT_HDR_SHORT_INDEX(h1))
+#define SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT		0x0
+#define SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT		0x1
+#define SPE_CNT_PKT_HDR_INDEX_TRANS_LAT		0x2
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

Re: [PATCH v3 12/20] perf arm-spe: Refactor counter packet handling

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> This patch defines macros for counter packet header, and uses macros to
> replace hard code values in functions arm_spe_get_counter() and
> arm_spe_pkt_desc().
> 
> In the function arm_spe_get_counter(), adds a new line for more
> readable.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 11 ++++++-----
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h |  8 ++++++++
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> 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 023bcc9be3cc..6eebd30f3d78 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
> @@ -152,10 +152,11 @@ static int arm_spe_get_counter(const unsigned char *buf, size_t len,
>  			       const unsigned char ext_hdr, struct arm_spe_pkt *packet)
>  {
>  	packet->type = ARM_SPE_COUNTER;
> +
>  	if (ext_hdr)
> -		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
> +		packet->index = SPE_CNT_PKT_HDR_EXTENDED_INDEX(buf[0], buf[1]);
>  	else
> -		packet->index = buf[0] & 0x7;
> +		packet->index = SPE_CNT_PKT_HDR_SHORT_INDEX(buf[0]);
>  
>  	return arm_spe_get_payload(buf, len, ext_hdr, packet);
>  }
> @@ -307,17 +308,17 @@ static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
>  		return ret;
>  
>  	switch (packet->index) {
> -	case 0:
> +	case SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT:
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT");
>  		if (ret < 0)
>  			return ret;
>  		break;
> -	case 1:
> +	case SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT:
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE");
>  		if (ret < 0)
>  			return ret;
>  		break;
> -	case 2:
> +	case SPE_CNT_PKT_HDR_INDEX_TRANS_LAT:
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT");
>  		if (ret < 0)
>  			return ret;
> 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 8808f2d0b6e4..8a291f451ef8 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
> @@ -84,6 +84,14 @@ struct arm_spe_pkt {
>  /* Context packet header */
>  #define SPE_CTX_PKT_HDR_INDEX(h)		((h) & GENMASK_ULL(1, 0))
>  
> +/* Counter packet header */
> +#define SPE_CNT_PKT_HDR_SHORT_INDEX(h)		((h) & GENMASK_ULL(2, 0))
> +#define SPE_CNT_PKT_HDR_EXTENDED_INDEX(h0, h1)	(((h0) & GENMASK_ULL(1, 0)) << 3 | \
> +						 SPE_ADDR_PKT_HDR_SHORT_INDEX(h1))

I would still like to see this merged with the SPE_ADDR_PKT_HDR_*_INDEX
definition in patch 10/20.

Otherwise this patch is fine.

Cheers,
Andre

> +#define SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT		0x0
> +#define SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT		0x1
> +#define SPE_CNT_PKT_HDR_INDEX_TRANS_LAT		0x2
> +
>  const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
>  
>  int arm_spe_get_packet(const unsigned char *buf, size_t len,
> 

[PATCH v3 13/20] perf arm-spe: Add new function arm_spe_pkt_desc_event()

[Leo Yan]

This patch moves out the event packet parsing from arm_spe_pkt_desc()
to the new function arm_spe_pkt_desc_event().

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 136 ++++++++++--------
 1 file changed, 73 insertions(+), 63 deletions(-)

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 6eebd30f3d78..8a6b50f32a52 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
@@ -266,6 +266,78 @@ static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
 	return ret;
 }
 
+static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
+				  char *buf, size_t buf_len)
+{
+	u64 payload = packet->payload;
+	size_t blen = buf_len;
+	int ret;
+
+	ret = arm_spe_pkt_snprintf(&buf, &blen, "EV");
+	if (ret < 0)
+		return ret;
+
+	if (payload & 0x1) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & 0x2) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & 0x4) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & 0x8) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & 0x10) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & 0x20) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & 0x40) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & 0x80) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
+		if (ret < 0)
+			return ret;
+	}
+	if (packet->index > 1) {
+		if (payload & 0x100) {
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
+			if (ret < 0)
+				return ret;
+		}
+		if (payload & 0x200) {
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
+			if (ret < 0)
+				return ret;
+		}
+		if (payload & 0x400) {
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return buf_len - blen;
+}
+
 static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 				 char *buf, size_t buf_len)
 {
@@ -344,69 +416,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	case ARM_SPE_END:
 		return arm_spe_pkt_snprintf(&buf, &blen, "%s", name);
 	case ARM_SPE_EVENTS:
-		ret = arm_spe_pkt_snprintf(&buf, &blen, "EV");
-		if (ret < 0)
-			return ret;
-
-		if (payload & 0x1) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & 0x2) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & 0x4) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & 0x8) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & 0x10) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & 0x20) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & 0x40) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & 0x80) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
-			if (ret < 0)
-				return ret;
-		}
-		if (idx > 1) {
-			if (payload & 0x100) {
-				ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
-				if (ret < 0)
-					return ret;
-			}
-			if (payload & 0x200) {
-				ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
-				if (ret < 0)
-					return ret;
-			}
-			if (payload & 0x400) {
-				ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
-				if (ret < 0)
-					return ret;
-			}
-		}
-		return buf_len - blen;
-
+		return arm_spe_pkt_desc_event(packet, buf, buf_len);
 	case ARM_SPE_OP_TYPE:
 		switch (idx) {
 		case 0:
-- 
2.17.1

Re: [PATCH v3 13/20] perf arm-spe: Add new function arm_spe_pkt_desc_event()

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> This patch moves out the event packet parsing from arm_spe_pkt_desc()
> to the new function arm_spe_pkt_desc_event().
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

diff -w says this is correct, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks!
Andre

> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 136 ++++++++++--------
>  1 file changed, 73 insertions(+), 63 deletions(-)
> 
> 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 6eebd30f3d78..8a6b50f32a52 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
> @@ -266,6 +266,78 @@ static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
>  	return ret;
>  }
>  
> +static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
> +				  char *buf, size_t buf_len)
> +{
> +	u64 payload = packet->payload;
> +	size_t blen = buf_len;
> +	int ret;
> +
> +	ret = arm_spe_pkt_snprintf(&buf, &blen, "EV");
> +	if (ret < 0)
> +		return ret;
> +
> +	if (payload & 0x1) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & 0x2) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & 0x4) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & 0x8) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & 0x10) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & 0x20) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & 0x40) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & 0x80) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (packet->index > 1) {
> +		if (payload & 0x100) {
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
> +			if (ret < 0)
> +				return ret;
> +		}
> +		if (payload & 0x200) {
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
> +			if (ret < 0)
> +				return ret;
> +		}
> +		if (payload & 0x400) {
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
> +	return buf_len - blen;
> +}
> +
>  static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  				 char *buf, size_t buf_len)
>  {
> @@ -344,69 +416,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	case ARM_SPE_END:
>  		return arm_spe_pkt_snprintf(&buf, &blen, "%s", name);
>  	case ARM_SPE_EVENTS:
> -		ret = arm_spe_pkt_snprintf(&buf, &blen, "EV");
> -		if (ret < 0)
> -			return ret;
> -
> -		if (payload & 0x1) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & 0x2) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & 0x4) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & 0x8) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & 0x10) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & 0x20) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & 0x40) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & 0x80) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (idx > 1) {
> -			if (payload & 0x100) {
> -				ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
> -				if (ret < 0)
> -					return ret;
> -			}
> -			if (payload & 0x200) {
> -				ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
> -				if (ret < 0)
> -					return ret;
> -			}
> -			if (payload & 0x400) {
> -				ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
> -				if (ret < 0)
> -					return ret;
> -			}
> -		}
> -		return buf_len - blen;
> -
> +		return arm_spe_pkt_desc_event(packet, buf, buf_len);
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
>  		case 0:
> 

[PATCH v3 14/20] perf arm-spe: Refactor event type handling

[Leo Yan]

Move the enums of event types to arm-spe-pkt-decoder.h, thus function
arm_spe_pkt_desc() can them for bitmasks.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../util/arm-spe-decoder/arm-spe-decoder.h    | 17 --------------
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 22 +++++++++----------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 18 +++++++++++++++
 3 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
index a5111a8d4360..24727b8ca7ff 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
@@ -13,23 +13,6 @@
 
 #include "arm-spe-pkt-decoder.h"
 
-enum arm_spe_events {
-	EV_EXCEPTION_GEN	= 0,
-	EV_RETIRED		= 1,
-	EV_L1D_ACCESS		= 2,
-	EV_L1D_REFILL		= 3,
-	EV_TLB_ACCESS		= 4,
-	EV_TLB_WALK		= 5,
-	EV_NOT_TAKEN		= 6,
-	EV_MISPRED		= 7,
-	EV_LLC_ACCESS		= 8,
-	EV_LLC_MISS		= 9,
-	EV_REMOTE_ACCESS	= 10,
-	EV_ALIGNMENT		= 11,
-	EV_PARTIAL_PREDICATE	= 17,
-	EV_EMPTY_PREDICATE	= 18,
-};
-
 enum arm_spe_sample_type {
 	ARM_SPE_L1D_ACCESS	= 1 << 0,
 	ARM_SPE_L1D_MISS	= 1 << 1,
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 8a6b50f32a52..58a1390b7a43 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
@@ -277,58 +277,58 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 	if (ret < 0)
 		return ret;
 
-	if (payload & 0x1) {
+	if (payload & BIT(EV_EXCEPTION_GEN)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
 		if (ret < 0)
 			return ret;
 	}
-	if (payload & 0x2) {
+	if (payload & BIT(EV_RETIRED)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
 		if (ret < 0)
 			return ret;
 	}
-	if (payload & 0x4) {
+	if (payload & BIT(EV_L1D_ACCESS)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
 		if (ret < 0)
 			return ret;
 	}
-	if (payload & 0x8) {
+	if (payload & BIT(EV_L1D_REFILL)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
 		if (ret < 0)
 			return ret;
 	}
-	if (payload & 0x10) {
+	if (payload & BIT(EV_TLB_ACCESS)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
 		if (ret < 0)
 			return ret;
 	}
-	if (payload & 0x20) {
+	if (payload & BIT(EV_TLB_WALK)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
 		if (ret < 0)
 			return ret;
 	}
-	if (payload & 0x40) {
+	if (payload & BIT(EV_NOT_TAKEN)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
 		if (ret < 0)
 			return ret;
 	}
-	if (payload & 0x80) {
+	if (payload & BIT(EV_MISPRED)) {
 		ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
 		if (ret < 0)
 			return ret;
 	}
 	if (packet->index > 1) {
-		if (payload & 0x100) {
+		if (payload & BIT(EV_LLC_ACCESS)) {
 			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
 			if (ret < 0)
 				return ret;
 		}
-		if (payload & 0x200) {
+		if (payload & BIT(EV_LLC_MISS)) {
 			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
 			if (ret < 0)
 				return ret;
 		}
-		if (payload & 0x400) {
+		if (payload & BIT(EV_REMOTE_ACCESS)) {
 			ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
 			if (ret < 0)
 				return ret;
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 8a291f451ef8..12c344454cf1 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
@@ -92,6 +92,24 @@ struct arm_spe_pkt {
 #define SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT		0x1
 #define SPE_CNT_PKT_HDR_INDEX_TRANS_LAT		0x2
 
+/* Event packet payload */
+enum arm_spe_events {
+	EV_EXCEPTION_GEN	= 0,
+	EV_RETIRED		= 1,
+	EV_L1D_ACCESS		= 2,
+	EV_L1D_REFILL		= 3,
+	EV_TLB_ACCESS		= 4,
+	EV_TLB_WALK		= 5,
+	EV_NOT_TAKEN		= 6,
+	EV_MISPRED		= 7,
+	EV_LLC_ACCESS		= 8,
+	EV_LLC_MISS		= 9,
+	EV_REMOTE_ACCESS	= 10,
+	EV_ALIGNMENT		= 11,
+	EV_PARTIAL_PREDICATE	= 17,
+	EV_EMPTY_PREDICATE	= 18,
+};
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

Re: [PATCH v3 14/20] perf arm-spe: Refactor event type handling

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> Move the enums of event types to arm-spe-pkt-decoder.h, thus function
> arm_spe_pkt_desc() can them for bitmasks.
> 
> Suggested-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

The move is fine, and I checked the bitmasks as well.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  .../util/arm-spe-decoder/arm-spe-decoder.h    | 17 --------------
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 22 +++++++++----------
>  .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 18 +++++++++++++++
>  3 files changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> index a5111a8d4360..24727b8ca7ff 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> @@ -13,23 +13,6 @@
>  
>  #include "arm-spe-pkt-decoder.h"
>  
> -enum arm_spe_events {
> -	EV_EXCEPTION_GEN	= 0,
> -	EV_RETIRED		= 1,
> -	EV_L1D_ACCESS		= 2,
> -	EV_L1D_REFILL		= 3,
> -	EV_TLB_ACCESS		= 4,
> -	EV_TLB_WALK		= 5,
> -	EV_NOT_TAKEN		= 6,
> -	EV_MISPRED		= 7,
> -	EV_LLC_ACCESS		= 8,
> -	EV_LLC_MISS		= 9,
> -	EV_REMOTE_ACCESS	= 10,
> -	EV_ALIGNMENT		= 11,
> -	EV_PARTIAL_PREDICATE	= 17,
> -	EV_EMPTY_PREDICATE	= 18,
> -};
> -
>  enum arm_spe_sample_type {
>  	ARM_SPE_L1D_ACCESS	= 1 << 0,
>  	ARM_SPE_L1D_MISS	= 1 << 1,
> 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 8a6b50f32a52..58a1390b7a43 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
> @@ -277,58 +277,58 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (payload & 0x1) {
> +	if (payload & BIT(EV_EXCEPTION_GEN)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (payload & 0x2) {
> +	if (payload & BIT(EV_RETIRED)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (payload & 0x4) {
> +	if (payload & BIT(EV_L1D_ACCESS)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (payload & 0x8) {
> +	if (payload & BIT(EV_L1D_REFILL)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (payload & 0x10) {
> +	if (payload & BIT(EV_TLB_ACCESS)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (payload & 0x20) {
> +	if (payload & BIT(EV_TLB_WALK)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (payload & 0x40) {
> +	if (payload & BIT(EV_NOT_TAKEN)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (payload & 0x80) {
> +	if (payload & BIT(EV_MISPRED)) {
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
>  		if (ret < 0)
>  			return ret;
>  	}
>  	if (packet->index > 1) {
> -		if (payload & 0x100) {
> +		if (payload & BIT(EV_LLC_ACCESS)) {
>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
>  			if (ret < 0)
>  				return ret;
>  		}
> -		if (payload & 0x200) {
> +		if (payload & BIT(EV_LLC_MISS)) {
>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
>  			if (ret < 0)
>  				return ret;
>  		}
> -		if (payload & 0x400) {
> +		if (payload & BIT(EV_REMOTE_ACCESS)) {
>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
>  			if (ret < 0)
>  				return ret;
> 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 8a291f451ef8..12c344454cf1 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
> @@ -92,6 +92,24 @@ struct arm_spe_pkt {
>  #define SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT		0x1
>  #define SPE_CNT_PKT_HDR_INDEX_TRANS_LAT		0x2
>  
> +/* Event packet payload */
> +enum arm_spe_events {
> +	EV_EXCEPTION_GEN	= 0,
> +	EV_RETIRED		= 1,
> +	EV_L1D_ACCESS		= 2,
> +	EV_L1D_REFILL		= 3,
> +	EV_TLB_ACCESS		= 4,
> +	EV_TLB_WALK		= 5,
> +	EV_NOT_TAKEN		= 6,
> +	EV_MISPRED		= 7,
> +	EV_LLC_ACCESS		= 8,
> +	EV_LLC_MISS		= 9,
> +	EV_REMOTE_ACCESS	= 10,
> +	EV_ALIGNMENT		= 11,
> +	EV_PARTIAL_PREDICATE	= 17,
> +	EV_EMPTY_PREDICATE	= 18,
> +};
> +
>  const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
>  
>  int arm_spe_get_packet(const unsigned char *buf, size_t len,
> 

[PATCH v3 15/20] perf arm-spe: Remove size condition checking for events

[Leo Yan]

In the Armv8 ARM (ARM DDI 0487F.c), chapter "D10.2.6 Events packet", it
describes the event bit is valid with specific payload requirement.  For
example, the Last Level cache access event, the bit is defined as:

  E[8], byte 1 bit [0], when SZ == 0b01 , when SZ == 0b10 ,
  		     or when SZ == 0b11

It requires the payload size is at least 2 bytes, when byte 1 (start
counting from 0) is valid, E[8] (bit 0 in byte 1) can be used for LLC
access event type.  For safety, the code checks the condition for
payload size firstly, if meet the requirement for payload size, then
continue to parse event type.

If review function arm_spe_get_payload(), it has used cast, so any bytes
beyond the valid size have been set to zeros.

For this reason, we don't need to check payload size anymore afterwards
when parse events, thus this patch removes payload size conditions.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../util/arm-spe-decoder/arm-spe-decoder.c    |  9 ++----
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 30 +++++++++----------
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index 776b3e6628bb..a5d7509d5daa 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -178,16 +178,13 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
 			if (payload & BIT(EV_TLB_ACCESS))
 				decoder->record.type |= ARM_SPE_TLB_ACCESS;
 
-			if ((idx == 2 || idx == 4 || idx == 8) &&
-			    (payload & BIT(EV_LLC_MISS)))
+			if (payload & BIT(EV_LLC_MISS))
 				decoder->record.type |= ARM_SPE_LLC_MISS;
 
-			if ((idx == 2 || idx == 4 || idx == 8) &&
-			    (payload & BIT(EV_LLC_ACCESS)))
+			if (payload & BIT(EV_LLC_ACCESS))
 				decoder->record.type |= ARM_SPE_LLC_ACCESS;
 
-			if ((idx == 2 || idx == 4 || idx == 8) &&
-			    (payload & BIT(EV_REMOTE_ACCESS)))
+			if (payload & BIT(EV_REMOTE_ACCESS))
 				decoder->record.type |= ARM_SPE_REMOTE_ACCESS;
 
 			if (payload & BIT(EV_MISPRED))
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 58a1390b7a43..2cb019999016 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
@@ -317,22 +317,20 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 		if (ret < 0)
 			return ret;
 	}
-	if (packet->index > 1) {
-		if (payload & BIT(EV_LLC_ACCESS)) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & BIT(EV_LLC_MISS)) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
-			if (ret < 0)
-				return ret;
-		}
-		if (payload & BIT(EV_REMOTE_ACCESS)) {
-			ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
-			if (ret < 0)
-				return ret;
-		}
+	if (payload & BIT(EV_LLC_ACCESS)) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & BIT(EV_LLC_MISS)) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & BIT(EV_REMOTE_ACCESS)) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
+		if (ret < 0)
+			return ret;
 	}
 
 	return buf_len - blen;
-- 
2.17.1

Re: [PATCH v3 15/20] perf arm-spe: Remove size condition checking for events

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:
> In the Armv8 ARM (ARM DDI 0487F.c), chapter "D10.2.6 Events packet", it
> describes the event bit is valid with specific payload requirement.  For
> example, the Last Level cache access event, the bit is defined as:
> 
>   E[8], byte 1 bit [0], when SZ == 0b01 , when SZ == 0b10 ,
>   		     or when SZ == 0b11
> 
> It requires the payload size is at least 2 bytes, when byte 1 (start
> counting from 0) is valid, E[8] (bit 0 in byte 1) can be used for LLC
> access event type.  For safety, the code checks the condition for
> payload size firstly, if meet the requirement for payload size, then
> continue to parse event type.
> 
> If review function arm_spe_get_payload(), it has used cast, so any bytes
> beyond the valid size have been set to zeros.
> 
> For this reason, we don't need to check payload size anymore afterwards
> when parse events, thus this patch removes payload size conditions.
> 
> Suggested-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Thanks, that looks better now!

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  .../util/arm-spe-decoder/arm-spe-decoder.c    |  9 ++----
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 30 +++++++++----------
>  2 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> index 776b3e6628bb..a5d7509d5daa 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> @@ -178,16 +178,13 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
>  			if (payload & BIT(EV_TLB_ACCESS))
>  				decoder->record.type |= ARM_SPE_TLB_ACCESS;
>  
> -			if ((idx == 2 || idx == 4 || idx == 8) &&
> -			    (payload & BIT(EV_LLC_MISS)))
> +			if (payload & BIT(EV_LLC_MISS))
>  				decoder->record.type |= ARM_SPE_LLC_MISS;
>  
> -			if ((idx == 2 || idx == 4 || idx == 8) &&
> -			    (payload & BIT(EV_LLC_ACCESS)))
> +			if (payload & BIT(EV_LLC_ACCESS))
>  				decoder->record.type |= ARM_SPE_LLC_ACCESS;
>  
> -			if ((idx == 2 || idx == 4 || idx == 8) &&
> -			    (payload & BIT(EV_REMOTE_ACCESS)))
> +			if (payload & BIT(EV_REMOTE_ACCESS))
>  				decoder->record.type |= ARM_SPE_REMOTE_ACCESS;
>  
>  			if (payload & BIT(EV_MISPRED))
> 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 58a1390b7a43..2cb019999016 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
> @@ -317,22 +317,20 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
>  		if (ret < 0)
>  			return ret;
>  	}
> -	if (packet->index > 1) {
> -		if (payload & BIT(EV_LLC_ACCESS)) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & BIT(EV_LLC_MISS)) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
> -			if (ret < 0)
> -				return ret;
> -		}
> -		if (payload & BIT(EV_REMOTE_ACCESS)) {
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
> -			if (ret < 0)
> -				return ret;
> -		}
> +	if (payload & BIT(EV_LLC_ACCESS)) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & BIT(EV_LLC_MISS)) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & BIT(EV_REMOTE_ACCESS)) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	return buf_len - blen;
> 

[PATCH v3 16/20] perf arm-spe: Add new function arm_spe_pkt_desc_op_type()

[Leo Yan]

The operation type packet is complex and contains subclass; the parsing
flow causes deep indentation; for more readable, this patch introduces
a new function arm_spe_pkt_desc_op_type() which is used for operation
type parsing.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 122 ++++++++++--------
 1 file changed, 66 insertions(+), 56 deletions(-)

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 2cb019999016..19d05d9734ab 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
@@ -336,6 +336,70 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 	return buf_len - blen;
 }
 
+static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
+				    char *buf, size_t buf_len)
+{
+	int ret, idx = packet->index;
+	unsigned long long payload = packet->payload;
+	size_t blen = buf_len;
+
+	switch (idx) {
+	case 0:
+		return arm_spe_pkt_snprintf(&buf, &blen,
+				payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
+	case 1:
+		ret = arm_spe_pkt_snprintf(&buf, &blen,
+					   payload & 0x1 ? "ST" : "LD");
+		if (ret < 0)
+			return ret;
+
+		if (payload & 0x2) {
+			if (payload & 0x4) {
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
+				if (ret < 0)
+					return ret;
+			}
+			if (payload & 0x8) {
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
+				if (ret < 0)
+					return ret;
+			}
+			if (payload & 0x10) {
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
+				if (ret < 0)
+					return ret;
+			}
+		} else if (payload & 0x4) {
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
+			if (ret < 0)
+				return ret;
+		}
+
+		return buf_len - blen;
+
+	case 2:
+		ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
+		if (ret < 0)
+			return ret;
+
+		if (payload & 0x1) {
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
+			if (ret < 0)
+				return ret;
+		}
+		if (payload & 0x2) {
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
+			if (ret < 0)
+				return ret;
+		}
+
+		return buf_len - blen;
+
+	default:
+		return 0;
+	}
+}
+
 static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 				 char *buf, size_t buf_len)
 {
@@ -403,7 +467,7 @@ static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
-	int ret, idx = packet->index;
+	int idx = packet->index;
 	unsigned long long payload = packet->payload;
 	const char *name = arm_spe_pkt_name(packet->type);
 	size_t blen = buf_len;
@@ -416,61 +480,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	case ARM_SPE_EVENTS:
 		return arm_spe_pkt_desc_event(packet, buf, buf_len);
 	case ARM_SPE_OP_TYPE:
-		switch (idx) {
-		case 0:
-			return arm_spe_pkt_snprintf(&buf, &blen,
-					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
-		case 1:
-			ret = arm_spe_pkt_snprintf(&buf, &blen,
-						   payload & 0x1 ? "ST" : "LD");
-			if (ret < 0)
-				return ret;
-
-			if (payload & 0x2) {
-				if (payload & 0x4) {
-					ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
-					if (ret < 0)
-						return ret;
-				}
-				if (payload & 0x8) {
-					ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
-					if (ret < 0)
-						return ret;
-				}
-				if (payload & 0x10) {
-					ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
-					if (ret < 0)
-						return ret;
-				}
-			} else if (payload & 0x4) {
-				ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
-				if (ret < 0)
-					return ret;
-			}
-
-			return buf_len - blen;
-
-		case 2:
-			ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
-			if (ret < 0)
-				return ret;
-
-			if (payload & 0x1) {
-				ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
-				if (ret < 0)
-					return ret;
-			}
-			if (payload & 0x2) {
-				ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
-				if (ret < 0)
-					return ret;
-			}
-
-			return buf_len - blen;
-
-		default:
-			return 0;
-		}
+		return arm_spe_pkt_desc_op_type(packet, buf, buf_len);
 	case ARM_SPE_DATA_SOURCE:
 	case ARM_SPE_TIMESTAMP:
 		return arm_spe_pkt_snprintf(&buf, &blen, "%s %lld", name, payload);
-- 
2.17.1

Re: [PATCH v3 16/20] perf arm-spe: Add new function arm_spe_pkt_desc_op_type()

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> The operation type packet is complex and contains subclass; the parsing
> flow causes deep indentation; for more readable, this patch introduces
> a new function arm_spe_pkt_desc_op_type() which is used for operation
> type parsing.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Compared '-' and '+' with diff -w, no changes.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks,
Andre

> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 122 ++++++++++--------
>  1 file changed, 66 insertions(+), 56 deletions(-)
> 
> 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 2cb019999016..19d05d9734ab 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
> @@ -336,6 +336,70 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
>  	return buf_len - blen;
>  }
>  
> +static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
> +				    char *buf, size_t buf_len)
> +{
> +	int ret, idx = packet->index;
> +	unsigned long long payload = packet->payload;
> +	size_t blen = buf_len;
> +
> +	switch (idx) {
> +	case 0:
> +		return arm_spe_pkt_snprintf(&buf, &blen,
> +				payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
> +	case 1:
> +		ret = arm_spe_pkt_snprintf(&buf, &blen,
> +					   payload & 0x1 ? "ST" : "LD");
> +		if (ret < 0)
> +			return ret;
> +
> +		if (payload & 0x2) {
> +			if (payload & 0x4) {
> +				ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
> +				if (ret < 0)
> +					return ret;
> +			}
> +			if (payload & 0x8) {
> +				ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
> +				if (ret < 0)
> +					return ret;
> +			}
> +			if (payload & 0x10) {
> +				ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
> +				if (ret < 0)
> +					return ret;
> +			}
> +		} else if (payload & 0x4) {
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		return buf_len - blen;
> +
> +	case 2:
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
> +		if (ret < 0)
> +			return ret;
> +
> +		if (payload & 0x1) {
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
> +			if (ret < 0)
> +				return ret;
> +		}
> +		if (payload & 0x2) {
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		return buf_len - blen;
> +
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  				 char *buf, size_t buf_len)
>  {
> @@ -403,7 +467,7 @@ static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  		     size_t buf_len)
>  {
> -	int ret, idx = packet->index;
> +	int idx = packet->index;
>  	unsigned long long payload = packet->payload;
>  	const char *name = arm_spe_pkt_name(packet->type);
>  	size_t blen = buf_len;
> @@ -416,61 +480,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	case ARM_SPE_EVENTS:
>  		return arm_spe_pkt_desc_event(packet, buf, buf_len);
>  	case ARM_SPE_OP_TYPE:
> -		switch (idx) {
> -		case 0:
> -			return arm_spe_pkt_snprintf(&buf, &blen,
> -					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
> -		case 1:
> -			ret = arm_spe_pkt_snprintf(&buf, &blen,
> -						   payload & 0x1 ? "ST" : "LD");
> -			if (ret < 0)
> -				return ret;
> -
> -			if (payload & 0x2) {
> -				if (payload & 0x4) {
> -					ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
> -					if (ret < 0)
> -						return ret;
> -				}
> -				if (payload & 0x8) {
> -					ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
> -					if (ret < 0)
> -						return ret;
> -				}
> -				if (payload & 0x10) {
> -					ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
> -					if (ret < 0)
> -						return ret;
> -				}
> -			} else if (payload & 0x4) {
> -				ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
> -				if (ret < 0)
> -					return ret;
> -			}
> -
> -			return buf_len - blen;
> -
> -		case 2:
> -			ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
> -			if (ret < 0)
> -				return ret;
> -
> -			if (payload & 0x1) {
> -				ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
> -				if (ret < 0)
> -					return ret;
> -			}
> -			if (payload & 0x2) {
> -				ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
> -				if (ret < 0)
> -					return ret;
> -			}
> -
> -			return buf_len - blen;
> -
> -		default:
> -			return 0;
> -		}
> +		return arm_spe_pkt_desc_op_type(packet, buf, buf_len);
>  	case ARM_SPE_DATA_SOURCE:
>  	case ARM_SPE_TIMESTAMP:
>  		return arm_spe_pkt_snprintf(&buf, &blen, "%s %lld", name, payload);
> 

[PATCH v3 17/20] perf arm-spe: Refactor operation packet handling

[Leo Yan]

Defines macros for operation packet header and formats (support sub
classes for 'other', 'branch', 'load and store', etc).  Uses these
macros for operation packet decoding and dumping.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 34 +++++++++++--------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 26 ++++++++++++++
 2 files changed, 45 insertions(+), 15 deletions(-)

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 19d05d9734ab..59b538563d31 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
@@ -144,7 +144,7 @@ static int arm_spe_get_op_type(const unsigned char *buf, size_t len,
 			       struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_OP_TYPE;
-	packet->index = buf[0] & 0x3;
+	packet->index = SPE_OP_PKT_HDR_CLASS(buf[0]);
 	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
@@ -339,37 +339,39 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 				    char *buf, size_t buf_len)
 {
-	int ret, idx = packet->index;
+	int ret, class = packet->index;
 	unsigned long long payload = packet->payload;
 	size_t blen = buf_len;
 
-	switch (idx) {
-	case 0:
+	switch (class) {
+	case SPE_OP_PKT_HDR_CLASS_OTHER:
 		return arm_spe_pkt_snprintf(&buf, &blen,
-				payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
-	case 1:
+			payload & SPE_OP_PKT_COND ? "COND-SELECT" : "INSN-OTHER");
+	case SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC:
 		ret = arm_spe_pkt_snprintf(&buf, &blen,
-					   payload & 0x1 ? "ST" : "LD");
+					   payload & SPE_OP_PKT_ST ? "ST" : "LD");
 		if (ret < 0)
 			return ret;
 
-		if (payload & 0x2) {
-			if (payload & 0x4) {
+		if (SPE_OP_PKT_LDST_SUBCLASS_ATOMIC_GET(payload) ==
+					SPE_OP_PKT_LDST_SUBCLASS_ATOMIC) {
+			if (payload & SPE_OP_PKT_AT) {
 				ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
 				if (ret < 0)
 					return ret;
 			}
-			if (payload & 0x8) {
+			if (payload & SPE_OP_PKT_EXCL) {
 				ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
 				if (ret < 0)
 					return ret;
 			}
-			if (payload & 0x10) {
+			if (payload & SPE_OP_PKT_AR) {
 				ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
 				if (ret < 0)
 					return ret;
 			}
-		} else if (payload & 0x4) {
+		} else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
+					SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
 			ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
 			if (ret < 0)
 				return ret;
@@ -377,17 +379,19 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 
 		return buf_len - blen;
 
-	case 2:
+	case SPE_OP_PKT_HDR_CLASS_BR_ERET:
 		ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
 		if (ret < 0)
 			return ret;
 
-		if (payload & 0x1) {
+		if (payload & SPE_OP_PKT_COND) {
 			ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
 			if (ret < 0)
 				return ret;
 		}
-		if (payload & 0x2) {
+
+		if (SPE_OP_PKT_BRANCH_SUBCLASS_GET(payload) ==
+				SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT) {
 			ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
 			if (ret < 0)
 				return ret;
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 12c344454cf1..31dbb8c0fde3 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
@@ -110,6 +110,32 @@ enum arm_spe_events {
 	EV_EMPTY_PREDICATE	= 18,
 };
 
+/* Operation packet header */
+#define SPE_OP_PKT_HDR_CLASS(h)			((h) & GENMASK_ULL(1, 0))
+#define SPE_OP_PKT_HDR_CLASS_OTHER		0x0
+#define SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC	0x1
+#define SPE_OP_PKT_HDR_CLASS_BR_ERET		0x2
+
+#define SPE_OP_PKT_COND				BIT(0)
+
+#define SPE_OP_PKT_LDST_SUBCLASS_GET(v)		((v) & GENMASK_ULL(7, 1))
+#define SPE_OP_PKT_LDST_SUBCLASS_GP_REG		0x0
+#define SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP	0x4
+#define SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG	0x10
+#define SPE_OP_PKT_LDST_SUBCLASS_NV_SYSREG	0x30
+
+#define SPE_OP_PKT_LDST_SUBCLASS_ATOMIC_GET(v)	((v) & (GENMASK_ULL(7, 5) | GENMASK_ULL(1, 1)))
+#define SPE_OP_PKT_LDST_SUBCLASS_ATOMIC		0x2
+
+#define SPE_OP_PKT_AR				BIT(4)
+#define SPE_OP_PKT_EXCL				BIT(3)
+#define SPE_OP_PKT_AT				BIT(2)
+#define SPE_OP_PKT_ST				BIT(0)
+
+#define SPE_OP_PKT_BRANCH_SUBCLASS_GET(v)	((v) & GENMASK_ULL(7, 1))
+#define SPE_OP_PKT_BRANCH_SUBCLASS_DIRECT	0x0
+#define SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT	0x2
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

Re: [PATCH v3 17/20] perf arm-spe: Refactor operation packet handling

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> Defines macros for operation packet header and formats (support sub
> classes for 'other', 'branch', 'load and store', etc).  Uses these
> macros for operation packet decoding and dumping.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 34 +++++++++++--------
>  .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 26 ++++++++++++++
>  2 files changed, 45 insertions(+), 15 deletions(-)
> 
> 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 19d05d9734ab..59b538563d31 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
> @@ -144,7 +144,7 @@ static int arm_spe_get_op_type(const unsigned char *buf, size_t len,
>  			       struct arm_spe_pkt *packet)
>  {
>  	packet->type = ARM_SPE_OP_TYPE;
> -	packet->index = buf[0] & 0x3;
> +	packet->index = SPE_OP_PKT_HDR_CLASS(buf[0]);
>  	return arm_spe_get_payload(buf, len, 0, packet);
>  }
>  
> @@ -339,37 +339,39 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
>  static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
>  				    char *buf, size_t buf_len)
>  {
> -	int ret, idx = packet->index;
> +	int ret, class = packet->index;
>  	unsigned long long payload = packet->payload;
>  	size_t blen = buf_len;
>  
> -	switch (idx) {
> -	case 0:
> +	switch (class) {
> +	case SPE_OP_PKT_HDR_CLASS_OTHER:
>  		return arm_spe_pkt_snprintf(&buf, &blen,
> -				payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
> -	case 1:
> +			payload & SPE_OP_PKT_COND ? "COND-SELECT" : "INSN-OTHER");
> +	case SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC:
>  		ret = arm_spe_pkt_snprintf(&buf, &blen,
> -					   payload & 0x1 ? "ST" : "LD");
> +					   payload & SPE_OP_PKT_ST ? "ST" : "LD");
>  		if (ret < 0)
>  			return ret;
>  
> -		if (payload & 0x2) {
> -			if (payload & 0x4) {
> +		if (SPE_OP_PKT_LDST_SUBCLASS_ATOMIC_GET(payload) ==
> +					SPE_OP_PKT_LDST_SUBCLASS_ATOMIC) {

This looks somewhat hard to read, and those symbols are only used once?
So what about combining this down in the header so that you can use:
		if (SPE_OP_PKT_IS_LDST_ATOMIC(payload)) {

> +			if (payload & SPE_OP_PKT_AT) {
>  				ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
>  				if (ret < 0)
>  					return ret;
>  			}
> -			if (payload & 0x8) {
> +			if (payload & SPE_OP_PKT_EXCL) {
>  				ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
>  				if (ret < 0)
>  					return ret;
>  			}
> -			if (payload & 0x10) {
> +			if (payload & SPE_OP_PKT_AR) {
>  				ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
>  				if (ret < 0)
>  					return ret;
>  			}
> -		} else if (payload & 0x4) {
> +		} else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
> +					SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
>  			if (ret < 0)
>  				return ret;
> @@ -377,17 +379,19 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
>  
>  		return buf_len - blen;
>  
> -	case 2:
> +	case SPE_OP_PKT_HDR_CLASS_BR_ERET:
>  		ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
>  		if (ret < 0)
>  			return ret;
>  
> -		if (payload & 0x1) {
> +		if (payload & SPE_OP_PKT_COND) {
>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
>  			if (ret < 0)
>  				return ret;
>  		}
> -		if (payload & 0x2) {
> +
> +		if (SPE_OP_PKT_BRANCH_SUBCLASS_GET(payload) ==
> +				SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT) {

Same here, it's the only user of both symbols, so maybe:
		if (SPE_OP_PKT_IS_INDIRECT_BRANCH(payload)) {

Cheers,
Andre

>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
>  			if (ret < 0)
>  				return ret;
> 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 12c344454cf1..31dbb8c0fde3 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
> @@ -110,6 +110,32 @@ enum arm_spe_events {
>  	EV_EMPTY_PREDICATE	= 18,
>  };
>  
> +/* Operation packet header */
> +#define SPE_OP_PKT_HDR_CLASS(h)			((h) & GENMASK_ULL(1, 0))
> +#define SPE_OP_PKT_HDR_CLASS_OTHER		0x0
> +#define SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC	0x1
> +#define SPE_OP_PKT_HDR_CLASS_BR_ERET		0x2
> +
> +#define SPE_OP_PKT_COND				BIT(0)
> +
> +#define SPE_OP_PKT_LDST_SUBCLASS_GET(v)		((v) & GENMASK_ULL(7, 1))
> +#define SPE_OP_PKT_LDST_SUBCLASS_GP_REG		0x0
> +#define SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP	0x4
> +#define SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG	0x10
> +#define SPE_OP_PKT_LDST_SUBCLASS_NV_SYSREG	0x30
> +
> +#define SPE_OP_PKT_LDST_SUBCLASS_ATOMIC_GET(v)	((v) & (GENMASK_ULL(7, 5) | GENMASK_ULL(1, 1)))
> +#define SPE_OP_PKT_LDST_SUBCLASS_ATOMIC		0x2
> +
> +#define SPE_OP_PKT_AR				BIT(4)
> +#define SPE_OP_PKT_EXCL				BIT(3)
> +#define SPE_OP_PKT_AT				BIT(2)
> +#define SPE_OP_PKT_ST				BIT(0)
> +
> +#define SPE_OP_PKT_BRANCH_SUBCLASS_GET(v)	((v) & GENMASK_ULL(7, 1))
> +#define SPE_OP_PKT_BRANCH_SUBCLASS_DIRECT	0x0
> +#define SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT	0x2
> +
>  const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
>  
>  int arm_spe_get_packet(const unsigned char *buf, size_t len,
> 

[PATCH v3 18/20] perf arm-spe: Add more sub classes for operation packet

[Leo Yan]

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>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 28 +++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

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 59b538563d31..c1a3b0afd1de 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
@@ -370,11 +370,35 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 				if (ret < 0)
 					return ret;
 			}
-		} else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
-					SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
+		}
+
+		switch (SPE_OP_PKT_LDST_SUBCLASS_GET(payload)) {
+		case SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP:
 			ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
 			if (ret < 0)
 				return ret;
+
+			break;
+		case SPE_OP_PKT_LDST_SUBCLASS_GP_REG:
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " GP-REG");
+			if (ret < 0)
+				return ret;
+
+			break;
+		case SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG:
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " UNSPEC-REG");
+			if (ret < 0)
+				return ret;
+
+			break;
+		case SPE_OP_PKT_LDST_SUBCLASS_NV_SYSREG:
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " NV-SYSREG");
+			if (ret < 0)
+				return ret;
+
+			break;
+		default:
+			break;
 		}
 
 		return buf_len - blen;
-- 
2.17.1

Re: [PATCH v3 18/20] perf arm-spe: Add more sub classes for operation packet

[André Przywara]

On 22/10/2020 15:58, 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>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 28 +++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> 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 59b538563d31..c1a3b0afd1de 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
> @@ -370,11 +370,35 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
>  				if (ret < 0)
>  					return ret;
>  			}
> -		} else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
> -					SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
> +		}
> +
> +		switch (SPE_OP_PKT_LDST_SUBCLASS_GET(payload)) {
> +		case SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP:
>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
>  			if (ret < 0)
>  				return ret;
> +
> +			break;
> +		case SPE_OP_PKT_LDST_SUBCLASS_GP_REG:
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " GP-REG");
> +			if (ret < 0)
> +				return ret;
> +
> +			break;
> +		case SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG:
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " UNSPEC-REG");
> +			if (ret < 0)
> +				return ret;
> +
> +			break;
> +		case SPE_OP_PKT_LDST_SUBCLASS_NV_SYSREG:
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " NV-SYSREG");
> +			if (ret < 0)
> +				return ret;
> +
> +			break;
> +		default:
> +			break;
>  		}
>  
>  		return buf_len - blen;
> 

[PATCH v3 19/20] perf arm_spe: Decode memory tagging properties

[Leo Yan]

From: Andre Przywara <andre.przywara@arm.com>

When SPE records a physical address, it can additionally tag the event
with information from the Memory Tagging architecture extension.

Decode the two additional fields in the SPE event payload.

[leoy: Refined patch to use predefined macros]

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 +++++-
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 2 ++
 2 files changed, 7 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 c1a3b0afd1de..74ac12cbec69 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
@@ -432,6 +432,7 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 				 char *buf, size_t buf_len)
 {
 	int ns, el, idx = packet->index;
+	int ch, pat;
 	u64 payload = packet->payload;
 
 	switch (idx) {
@@ -448,9 +449,12 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 					    "VA 0x%llx", payload);
 	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
 		ns = !!SPE_ADDR_PKT_GET_NS(payload);
+		ch = !!SPE_ADDR_PKT_GET_CH(payload);
+		pat = SPE_ADDR_PKT_GET_PAT(payload);
 		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 		return arm_spe_pkt_snprintf(&buf, &buf_len,
-					    "PA 0x%llx ns=%d", payload, ns);
+					    "PA 0x%llx ns=%d ch=%d, pat=%x",
+					    payload, ns, ch, pat);
 	default:
 		return 0;
 	}
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 31dbb8c0fde3..d69af0d618ea 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
@@ -75,6 +75,8 @@ struct arm_spe_pkt {
 
 #define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT(63)) >> 63)
 #define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
+#define SPE_ADDR_PKT_GET_CH(v)			(((v) & BIT(62)) >> 62)
+#define SPE_ADDR_PKT_GET_PAT(v)			(((v) & GENMASK_ULL(59, 56)) >> 56)
 
 #define SPE_ADDR_PKT_EL0			0
 #define SPE_ADDR_PKT_EL1			1
-- 
2.17.1

Re: [PATCH v3 19/20] perf arm_spe: Decode memory tagging properties

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> From: Andre Przywara <andre.przywara@arm.com>
> 
> When SPE records a physical address, it can additionally tag the event
> with information from the Memory Tagging architecture extension.
> 
> Decode the two additional fields in the SPE event payload.
> 
> [leoy: Refined patch to use predefined macros]
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 +++++-
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 2 ++
>  2 files changed, 7 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 c1a3b0afd1de..74ac12cbec69 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
> @@ -432,6 +432,7 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  				 char *buf, size_t buf_len)
>  {
>  	int ns, el, idx = packet->index;
> +	int ch, pat;
>  	u64 payload = packet->payload;
>  
>  	switch (idx) {
> @@ -448,9 +449,12 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  					    "VA 0x%llx", payload);
>  	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
>  		ns = !!SPE_ADDR_PKT_GET_NS(payload);
> +		ch = !!SPE_ADDR_PKT_GET_CH(payload);
> +		pat = SPE_ADDR_PKT_GET_PAT(payload);
>  		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
>  		return arm_spe_pkt_snprintf(&buf, &buf_len,
> -					    "PA 0x%llx ns=%d", payload, ns);
> +					    "PA 0x%llx ns=%d ch=%d, pat=%x",
> +					    payload, ns, ch, pat);
>  	default:
>  		return 0;
>  	}
> 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 31dbb8c0fde3..d69af0d618ea 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
> @@ -75,6 +75,8 @@ struct arm_spe_pkt {
>  
>  #define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT(63)) >> 63)
>  #define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
> +#define SPE_ADDR_PKT_GET_CH(v)			(((v) & BIT(62)) >> 62)

You need BIT_ULL() here to make this work on 32-bit systems.

Cheers,
Andre

> +#define SPE_ADDR_PKT_GET_PAT(v)			(((v) & GENMASK_ULL(59, 56)) >> 56)
>  
>  #define SPE_ADDR_PKT_EL0			0
>  #define SPE_ADDR_PKT_EL1			1
> 

[PATCH v3 20/20] perf arm-spe: Add support for ARMv8.3-SPE

[Leo Yan]

From: Wei Li <liwei391@huawei.com>

This patch is to support Armv8.3 extension for SPE, it adds alignment
field in the Events packet and it supports the Scalable Vector Extension
(SVE) for Operation packet and Events packet with two additions:

  - The vector length for SVE operations in the Operation Type packet;
  - The incomplete predicate and empty predicate fields in the Events
    packet.

Signed-off-by: Wei Li <liwei391@huawei.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 74 ++++++++++++++++++-
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 18 +++++
 2 files changed, 90 insertions(+), 2 deletions(-)

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 74ac12cbec69..6da4cfbc9914 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
@@ -332,6 +332,21 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 		if (ret < 0)
 			return ret;
 	}
+	if (payload & BIT(EV_ALIGNMENT)) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " ALIGNMENT");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & BIT(EV_PARTIAL_PREDICATE)) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " SVE-PARTIAL-PRED");
+		if (ret < 0)
+			return ret;
+	}
+	if (payload & BIT(EV_EMPTY_PREDICATE)) {
+		ret = arm_spe_pkt_snprintf(&buf, &blen, " SVE-EMPTY-PRED");
+		if (ret < 0)
+			return ret;
+	}
 
 	return buf_len - blen;
 }
@@ -345,8 +360,43 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 
 	switch (class) {
 	case SPE_OP_PKT_HDR_CLASS_OTHER:
-		return arm_spe_pkt_snprintf(&buf, &blen,
-			payload & SPE_OP_PKT_COND ? "COND-SELECT" : "INSN-OTHER");
+		if (SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP_GET(payload) ==
+				SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP) {
+
+			ret = arm_spe_pkt_snprintf(&buf, &blen, "SVE-OTHER");
+			if (ret < 0)
+				return ret;
+
+			/* SVE effective vector length */
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " EVLEN %d",
+						   SPE_OP_PKG_SVE_EVL(payload));
+			if (ret < 0)
+				return ret;
+
+			if (payload & SPE_OP_PKT_SVE_FP) {
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " FP");
+				if (ret < 0)
+					return ret;
+			}
+			if (payload & SPE_OP_PKT_SVE_PRED) {
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " PRED");
+				if (ret < 0)
+					return ret;
+			}
+		} else {
+			ret = arm_spe_pkt_snprintf(&buf, &blen, "OTHER");
+			if (ret < 0)
+				return ret;
+
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " %s",
+					payload & SPE_OP_PKT_COND ?
+					"COND-SELECT" : "INSN-OTHER");
+			if (ret < 0)
+				return ret;
+		}
+
+		return buf_len - blen;
+
 	case SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC:
 		ret = arm_spe_pkt_snprintf(&buf, &blen,
 					   payload & SPE_OP_PKT_ST ? "ST" : "LD");
@@ -401,6 +451,26 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 			break;
 		}
 
+		if (SPE_OP_PKT_LDST_SUBCLASS_SVE_GET(payload) ==
+				SPE_OP_PKT_LDST_SUBCLASS_SVE) {
+			/* SVE effective vector length */
+			ret = arm_spe_pkt_snprintf(&buf, &blen, " EVLEN %d",
+						   SPE_OP_PKG_SVE_EVL(payload));
+			if (ret < 0)
+				return ret;
+
+			if (payload & SPE_OP_PKT_SVE_PRED) {
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " PRED");
+				if (ret < 0)
+					return ret;
+			}
+			if (payload & SPE_OP_PKT_SVE_SG) {
+				ret = arm_spe_pkt_snprintf(&buf, &blen, " SG");
+				if (ret < 0)
+					return ret;
+			}
+		}
+
 		return buf_len - blen;
 
 	case SPE_OP_PKT_HDR_CLASS_BR_ERET:
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 d69af0d618ea..04bc09f3ea17 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
@@ -118,6 +118,9 @@ enum arm_spe_events {
 #define SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC	0x1
 #define SPE_OP_PKT_HDR_CLASS_BR_ERET		0x2
 
+#define SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP_GET(v)	((v) & (BIT(7) | BIT(3) | BIT(0)))
+#define SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP	0x8
+
 #define SPE_OP_PKT_COND				BIT(0)
 
 #define SPE_OP_PKT_LDST_SUBCLASS_GET(v)		((v) & GENMASK_ULL(7, 1))
@@ -134,6 +137,21 @@ enum arm_spe_events {
 #define SPE_OP_PKT_AT				BIT(2)
 #define SPE_OP_PKT_ST				BIT(0)
 
+#define SPE_OP_PKT_LDST_SUBCLASS_SVE_GET(v)	((v) & (GENMASK_ULL(3, 3) | GENMASK_ULL(1, 1)))
+#define SPE_OP_PKT_LDST_SUBCLASS_SVE		0x8
+
+#define SPE_OP_PKT_SVE_SG			BIT(7)
+/*
+ * SVE effective vector length (EVL) is stored in byte 0 bits [6:4];
+ * the length is rounded up to a power of two and use 32 as one step,
+ * so EVL calculation is:
+ *
+ *   32 * (2 ^ bits [6:4]) = 32 << (bits [6:4])
+ */
+#define SPE_OP_PKG_SVE_EVL(v)			(32 << (((v) & GENMASK_ULL(6, 4)) >> 4))
+#define SPE_OP_PKT_SVE_PRED			BIT(2)
+#define SPE_OP_PKT_SVE_FP			BIT(1)
+
 #define SPE_OP_PKT_BRANCH_SUBCLASS_GET(v)	((v) & GENMASK_ULL(7, 1))
 #define SPE_OP_PKT_BRANCH_SUBCLASS_DIRECT	0x0
 #define SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT	0x2
-- 
2.17.1

Re: [PATCH v3 20/20] perf arm-spe: Add support for ARMv8.3-SPE

[André Przywara]

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> From: Wei Li <liwei391@huawei.com>
> 
> This patch is to support Armv8.3 extension for SPE, it adds alignment
> field in the Events packet and it supports the Scalable Vector Extension
> (SVE) for Operation packet and Events packet with two additions:
> 
>   - The vector length for SVE operations in the Operation Type packet;
>   - The incomplete predicate and empty predicate fields in the Events
>     packet.
> 
> Signed-off-by: Wei Li <liwei391@huawei.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 74 ++++++++++++++++++-
>  .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 18 +++++
>  2 files changed, 90 insertions(+), 2 deletions(-)
> 
> 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 74ac12cbec69..6da4cfbc9914 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
> @@ -332,6 +332,21 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
>  		if (ret < 0)
>  			return ret;
>  	}
> +	if (payload & BIT(EV_ALIGNMENT)) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " ALIGNMENT");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & BIT(EV_PARTIAL_PREDICATE)) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " SVE-PARTIAL-PRED");
> +		if (ret < 0)
> +			return ret;
> +	}
> +	if (payload & BIT(EV_EMPTY_PREDICATE)) {
> +		ret = arm_spe_pkt_snprintf(&buf, &blen, " SVE-EMPTY-PRED");
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	return buf_len - blen;
>  }
> @@ -345,8 +360,43 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
>  
>  	switch (class) {
>  	case SPE_OP_PKT_HDR_CLASS_OTHER:
> -		return arm_spe_pkt_snprintf(&buf, &blen,
> -			payload & SPE_OP_PKT_COND ? "COND-SELECT" : "INSN-OTHER");
> +		if (SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP_GET(payload) ==
> +				SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP) {

Same comment as in the other patch, can you combine those two into one
symbol?

> +
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, "SVE-OTHER");
> +			if (ret < 0)
> +				return ret;
> +
> +			/* SVE effective vector length */
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " EVLEN %d",
> +						   SPE_OP_PKG_SVE_EVL(payload));
> +			if (ret < 0)
> +				return ret;
> +
> +			if (payload & SPE_OP_PKT_SVE_FP) {
> +				ret = arm_spe_pkt_snprintf(&buf, &blen, " FP");
> +				if (ret < 0)
> +					return ret;
> +			}
> +			if (payload & SPE_OP_PKT_SVE_PRED) {
> +				ret = arm_spe_pkt_snprintf(&buf, &blen, " PRED");
> +				if (ret < 0)
> +					return ret;
> +			}
> +		} else {
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, "OTHER");
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " %s",
> +					payload & SPE_OP_PKT_COND ?
> +					"COND-SELECT" : "INSN-OTHER");
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		return buf_len - blen;
> +
>  	case SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC:
>  		ret = arm_spe_pkt_snprintf(&buf, &blen,
>  					   payload & SPE_OP_PKT_ST ? "ST" : "LD");
> @@ -401,6 +451,26 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
>  			break;
>  		}
>  
> +		if (SPE_OP_PKT_LDST_SUBCLASS_SVE_GET(payload) ==
> +				SPE_OP_PKT_LDST_SUBCLASS_SVE) {

Same here, could be combined into one symbol.

> +			/* SVE effective vector length */
> +			ret = arm_spe_pkt_snprintf(&buf, &blen, " EVLEN %d",
> +						   SPE_OP_PKG_SVE_EVL(payload));
> +			if (ret < 0)
> +				return ret;
> +
> +			if (payload & SPE_OP_PKT_SVE_PRED) {
> +				ret = arm_spe_pkt_snprintf(&buf, &blen, " PRED");
> +				if (ret < 0)
> +					return ret;
> +			}
> +			if (payload & SPE_OP_PKT_SVE_SG) {
> +				ret = arm_spe_pkt_snprintf(&buf, &blen, " SG");
> +				if (ret < 0)
> +					return ret;
> +			}
> +		}
> +
>  		return buf_len - blen;
>  
>  	case SPE_OP_PKT_HDR_CLASS_BR_ERET:
> 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 d69af0d618ea..04bc09f3ea17 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
> @@ -118,6 +118,9 @@ enum arm_spe_events {
>  #define SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC	0x1
>  #define SPE_OP_PKT_HDR_CLASS_BR_ERET		0x2
>  
> +#define SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP_GET(v)	((v) & (BIT(7) | BIT(3) | BIT(0)))
> +#define SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP	0x8
> +
>  #define SPE_OP_PKT_COND				BIT(0)
>  
>  #define SPE_OP_PKT_LDST_SUBCLASS_GET(v)		((v) & GENMASK_ULL(7, 1))
> @@ -134,6 +137,21 @@ enum arm_spe_events {
>  #define SPE_OP_PKT_AT				BIT(2)
>  #define SPE_OP_PKT_ST				BIT(0)
>  
> +#define SPE_OP_PKT_LDST_SUBCLASS_SVE_GET(v)	((v) & (GENMASK_ULL(3, 3) | GENMASK_ULL(1, 1)))
> +#define SPE_OP_PKT_LDST_SUBCLASS_SVE		0x8
> +
> +#define SPE_OP_PKT_SVE_SG			BIT(7)
> +/*
> + * SVE effective vector length (EVL) is stored in byte 0 bits [6:4];
> + * the length is rounded up to a power of two and use 32 as one step,
> + * so EVL calculation is:
> + *
> + *   32 * (2 ^ bits [6:4]) = 32 << (bits [6:4])
> + */

Thanks for adding the comment!

Cheers,
Andre

> +#define SPE_OP_PKG_SVE_EVL(v)			(32 << (((v) & GENMASK_ULL(6, 4)) >> 4))
> +#define SPE_OP_PKT_SVE_PRED			BIT(2)
> +#define SPE_OP_PKT_SVE_FP			BIT(1)
> +
>  #define SPE_OP_PKT_BRANCH_SUBCLASS_GET(v)	((v) & GENMASK_ULL(7, 1))
>  #define SPE_OP_PKT_BRANCH_SUBCLASS_DIRECT	0x0
>  #define SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT	0x2
> 

Re: [PATCH v3 20/20] perf arm-spe: Add support for ARMv8.3-SPE

[Leo Yan]

On Mon, Oct 26, 2020 at 06:17:20PM +0000, André Przywara wrote:
> On 22/10/2020 15:58, Leo Yan wrote:
> 
> Hi,
> 
> > From: Wei Li <liwei391@huawei.com>
> > 
> > This patch is to support Armv8.3 extension for SPE, it adds alignment
> > field in the Events packet and it supports the Scalable Vector Extension
> > (SVE) for Operation packet and Events packet with two additions:
> > 
> >   - The vector length for SVE operations in the Operation Type packet;
> >   - The incomplete predicate and empty predicate fields in the Events
> >     packet.
> > 
> > Signed-off-by: Wei Li <liwei391@huawei.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 74 ++++++++++++++++++-
> >  .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 18 +++++
> >  2 files changed, 90 insertions(+), 2 deletions(-)
> > 
> > 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 74ac12cbec69..6da4cfbc9914 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
> > @@ -332,6 +332,21 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
> >  		if (ret < 0)
> >  			return ret;
> >  	}
> > +	if (payload & BIT(EV_ALIGNMENT)) {
> > +		ret = arm_spe_pkt_snprintf(&buf, &blen, " ALIGNMENT");
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	if (payload & BIT(EV_PARTIAL_PREDICATE)) {
> > +		ret = arm_spe_pkt_snprintf(&buf, &blen, " SVE-PARTIAL-PRED");
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	if (payload & BIT(EV_EMPTY_PREDICATE)) {
> > +		ret = arm_spe_pkt_snprintf(&buf, &blen, " SVE-EMPTY-PRED");
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >  
> >  	return buf_len - blen;
> >  }
> > @@ -345,8 +360,43 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
> >  
> >  	switch (class) {
> >  	case SPE_OP_PKT_HDR_CLASS_OTHER:
> > -		return arm_spe_pkt_snprintf(&buf, &blen,
> > -			payload & SPE_OP_PKT_COND ? "COND-SELECT" : "INSN-OTHER");
> > +		if (SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP_GET(payload) ==
> > +				SPE_OP_PKT_OTHER_SUBCLASS_SVE_OP) {
> 
> Same comment as in the other patch, can you combine those two into one
> symbol?

Thanks for the suggestion, have refined patches for this and sent out
patch set v4 for reviewing.

Leo