[PATCH 0/5] perf: arm64: Support ARMv8.3-SPE extensions

[PATCH 0/5] perf: arm64: Support ARMv8.3-SPE extensions

[Andre Przywara]

The "ARMv8.3-SPE extensions" add some bits to SPE to cover newer
architecture features, most prominently SVE.

Add the new bits where needed, mostly to perf's SPE packet decoder.

Cheers,
Andre

Andre Przywara (5):
  arm64: spe: Allow new bits in SPE filter register
  perf: arm_spe: Add new event packet bits
  perf: arm_spe: Add nested virt event decoding
  perf: arm_spe: Decode memory tagging properties
  perf: arm_spe: Decode SVE events

 arch/arm64/include/asm/sysreg.h               |  2 +-
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 86 +++++++++++++++++--
 2 files changed, 81 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH 1/5] arm64: spe: Allow new bits in SPE filter register

[Andre Przywara]

The ARMv8.3-SPE extension adds some new bits for the event filter.

Remove bits 11, 17 and 18 from the RES0 mask, so they can be used
correctly.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/include/asm/sysreg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 554a7e8ecb07..efca4ee28671 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -281,7 +281,7 @@
 #define SYS_PMSFCR_EL1_ST_SHIFT		18
 
 #define SYS_PMSEVFR_EL1			sys_reg(3, 0, 9, 9, 5)
-#define SYS_PMSEVFR_EL1_RES0		0x0000ffff00ff0f55UL
+#define SYS_PMSEVFR_EL1_RES0		0x0000ffff00f90755UL
 
 #define SYS_PMSLATFR_EL1		sys_reg(3, 0, 9, 9, 6)
 #define SYS_PMSLATFR_EL1_MINLAT_SHIFT	0
-- 
2.17.1

[PATCH 2/5] perf: arm_spe: Add new event packet bits

[Andre Przywara]

The ARMv8.3-SPE extension adds some new bits to the event packet
fields.

Handle bits 11 (alignment), 17 and 18 (SVE predication) when decoding
the SPE buffer content.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../util/arm-spe-decoder/arm-spe-pkt-decoder.c  | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index b94001b756c7..e633bb5b8e65 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
@@ -346,6 +346,23 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 				buf += ret;
 				blen -= ret;
 			}
+			if (payload & BIT(11)) {
+				ret = snprintf(buf, buf_len, " ALIGNMENT");
+				buf += ret;
+				blen -= ret;
+			}
+		}
+		if (idx > 2) {
+			if (payload & BIT(17)) {
+				ret = snprintf(buf, buf_len, " SVE-PARTIAL-PRED");
+				buf += ret;
+				blen -= ret;
+			}
+			if (payload & BIT(18)) {
+				ret = snprintf(buf, buf_len, " SVE-EMPTY-PRED");
+				buf += ret;
+				blen -= ret;
+			}
 		}
 		if (ret < 0)
 			return ret;
-- 
2.17.1

[PATCH 3/5] perf: arm_spe: Add nested virt event decoding

[Andre Przywara]

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 this load/store type, so that the perf
tool can decode it properly.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index e633bb5b8e65..943e4155b246 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
@@ -398,6 +398,10 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 					buf += ret;
 					blen -= ret;
 				}
+			} else if ((payload & 0xfe) == 0x30) {
+				ret = snprintf(buf, buf_len, " NV-SYSREG");
+				buf += ret;
+				blen -= ret;
 			} else if (payload & 0x4) {
 				ret = snprintf(buf, buf_len, " SIMD-FP");
 				buf += ret;
-- 
2.17.1

[PATCH 4/5] perf: arm_spe: Decode memory tagging properties

[Andre Przywara]

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.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../util/arm-spe-decoder/arm-spe-pkt-decoder.c  | 17 ++++++++++++-----
 1 file changed, 12 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 943e4155b246..a033f34846a6 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,13 +8,14 @@
 #include <string.h>
 #include <endian.h>
 #include <byteswap.h>
+#include <linux/bits.h>
 
 #include "arm-spe-pkt-decoder.h"
 
-#define BIT(n)		(1ULL << (n))
-
 #define NS_FLAG		BIT(63)
 #define EL_FLAG		(BIT(62) | BIT(61))
+#define CH_FLAG		BIT(62)
+#define PAT_FLAG	GENMASK_ULL(59, 56)
 
 #define SPE_HEADER0_PAD			0x0
 #define SPE_HEADER0_END			0x1
@@ -447,10 +448,16 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 			return snprintf(buf, buf_len, "%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 3:	ns = !!(packet->payload & NS_FLAG);
+		case 3:	{
+			int ch = !!(packet->payload & CH_FLAG);
+			int pat = (packet->payload & PAT_FLAG) >> 56;
+
+			ns = !!(packet->payload & NS_FLAG);
 			payload &= ~(0xffULL << 56);
-			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
-					payload, ns);
+			return snprintf(buf, buf_len,
+					"PA 0x%llx ns=%d ch=%d, pat=%x",
+					payload, ns, ch, pat);
+			}
 		default: return 0;
 		}
 	case ARM_SPE_CONTEXT:
-- 
2.17.1

[PATCH 5/5] perf: arm_spe: Decode SVE events

[Andre Przywara]

The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
that introduces very long vector operations (up to 2048 bits).
The SPE profiling feature can tag SVE instructions with additional
properties like predication or the effective vector length.

Decode the new operation type bits in the SPE decoder to allow the perf
tool to correctly report about SVE instructions.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
 1 file changed, 47 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 a033f34846a6..f0c369259554 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
@@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	}
 	case ARM_SPE_OP_TYPE:
 		switch (idx) {
-		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
+		case 0: {
+			size_t blen = buf_len;
+
+			if ((payload & 0x89) == 0x08) {
+				ret = snprintf(buf, buf_len, "SVE");
+				buf += ret;
+				blen -= ret;
+				if (payload & 0x2)
+					ret = snprintf(buf, buf_len, " FP");
+				else
+					ret = snprintf(buf, buf_len, " INT");
+				buf += ret;
+				blen -= ret;
+				if (payload & 0x4) {
+					ret = snprintf(buf, buf_len, " PRED");
+					buf += ret;
+					blen -= ret;
+				}
+				/* Bits [7..4] encode the vector length */
+				ret = snprintf(buf, buf_len, " EVLEN%d",
+					       32 << ((payload >> 4) & 0x7));
+				buf += ret;
+				blen -= ret;
+				return buf_len - blen;
+			}
+
+			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
 					"COND-SELECT" : "INSN-OTHER");
+			}
 		case 1:	{
 			size_t blen = buf_len;
 
@@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 				ret = snprintf(buf, buf_len, " NV-SYSREG");
 				buf += ret;
 				blen -= ret;
+			} else if ((payload & 0x0a) == 0x08) {
+				ret = snprintf(buf, buf_len, " SVE");
+				buf += ret;
+				blen -= ret;
+				if (payload & 0x4) {
+					ret = snprintf(buf, buf_len, " PRED");
+					buf += ret;
+					blen -= ret;
+				}
+				if (payload & 0x80) {
+					ret = snprintf(buf, buf_len, " SG");
+					buf += ret;
+					blen -= ret;
+				}
+				/* Bits [7..4] encode the vector length */
+				ret = snprintf(buf, buf_len, " EVLEN%d",
+					       32 << ((payload >> 4) & 0x7));
+				buf += ret;
+				blen -= ret;
 			} else if (payload & 0x4) {
 				ret = snprintf(buf, buf_len, " SIMD-FP");
 				buf += ret;
-- 
2.17.1

Re: [PATCH 1/5] arm64: spe: Allow new bits in SPE filter register

[Leo Yan]

Hi Andre,

On Tue, Sep 22, 2020 at 11:12:21AM +0100, Andre Przywara wrote:
> The ARMv8.3-SPE extension adds some new bits for the event filter.
> 
> Remove bits 11, 17 and 18 from the RES0 mask, so they can be used
> correctly.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm64/include/asm/sysreg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 554a7e8ecb07..efca4ee28671 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -281,7 +281,7 @@
>  #define SYS_PMSFCR_EL1_ST_SHIFT		18
>  
>  #define SYS_PMSEVFR_EL1			sys_reg(3, 0, 9, 9, 5)
> -#define SYS_PMSEVFR_EL1_RES0		0x0000ffff00ff0f55UL
> +#define SYS_PMSEVFR_EL1_RES0		0x0000ffff00f90755UL

This patch is duplicate with Wei Li's patch [1].  You could see there
have some discussion and Will gave suggestions [2] for the patch, this
would be a good start point to continue this work.

Thanks,
Leo

[1] https://www.spinics.net/lists/arm-kernel/msg825364.html
[2] https://www.spinics.net/lists/arm-kernel/msg835733.html

>  
>  #define SYS_PMSLATFR_EL1		sys_reg(3, 0, 9, 9, 6)
>  #define SYS_PMSLATFR_EL1_MINLAT_SHIFT	0
> -- 
> 2.17.1
> 

Re: [PATCH 2/5] perf: arm_spe: Add new event packet bits

[Leo Yan]

On Tue, Sep 22, 2020 at 11:12:22AM +0100, Andre Przywara wrote:
> The ARMv8.3-SPE extension adds some new bits to the event packet
> fields.
> 
> Handle bits 11 (alignment), 17 and 18 (SVE predication) when decoding
> the SPE buffer content.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../util/arm-spe-decoder/arm-spe-pkt-decoder.c  | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index b94001b756c7..e633bb5b8e65 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
> @@ -346,6 +346,23 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  				buf += ret;
>  				blen -= ret;
>  			}
> +			if (payload & BIT(11)) {
> +				ret = snprintf(buf, buf_len, " ALIGNMENT");
> +				buf += ret;
> +				blen -= ret;
> +			}
> +		}
> +		if (idx > 2) {
> +			if (payload & BIT(17)) {
> +				ret = snprintf(buf, buf_len, " SVE-PARTIAL-PRED");
> +				buf += ret;
> +				blen -= ret;
> +			}
> +			if (payload & BIT(18)) {
> +				ret = snprintf(buf, buf_len, " SVE-EMPTY-PRED");
> +				buf += ret;
> +				blen -= ret;
> +			}

From patch 02 to patch 05, some changes have been included in the
patch set "perf arm-spe: Refactor decoding & dumping flow".  I
refactored the Arm SPE decoder so uses macros to replace the hard code
numbers for packet formats.  So I'd like your changes could rebase on
this refactor patch set, thus can reuse the predefined macros for
decoding.

For this patch, it has been included in the patch [2].  You could see
your implementation is difference for handling "ALIGNMENT", it misses
to check "idx > 2".  It would be very helpful if you could review
patch [2].

Thanks,
Leo

[1] https://lore.kernel.org/patchwork/cover/1288406/
[2] https://lore.kernel.org/patchwork/patch/1288413/

>  		}
>  		if (ret < 0)
>  			return ret;
> -- 
> 2.17.1
> 

Re: [PATCH 3/5] perf: arm_spe: Add nested virt event decoding

[Leo Yan]

On Tue, Sep 22, 2020 at 11:12:23AM +0100, Andre Przywara wrote:
> 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 this load/store type, so that the perf
> tool can decode it properly.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index e633bb5b8e65..943e4155b246 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
> @@ -398,6 +398,10 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  					buf += ret;
>  					blen -= ret;
>  				}
> +			} else if ((payload & 0xfe) == 0x30) {
> +				ret = snprintf(buf, buf_len, " NV-SYSREG");
> +				buf += ret;
> +				blen -= ret;

This change has been included in the patch "perf arm-spe: Add more sub
classes for operation packet" [1].

Thanks,
Leo

[1] https://lore.kernel.org/patchwork/patch/1288412/

>  			} else if (payload & 0x4) {
>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>  				buf += ret;
> -- 
> 2.17.1
> 

Re: [PATCH 4/5] perf: arm_spe: Decode memory tagging properties

[Leo Yan]

On Tue, Sep 22, 2020 at 11:12:24AM +0100, Andre Przywara wrote:
> 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.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../util/arm-spe-decoder/arm-spe-pkt-decoder.c  | 17 ++++++++++++-----
>  1 file changed, 12 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 943e4155b246..a033f34846a6 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,13 +8,14 @@
>  #include <string.h>
>  #include <endian.h>
>  #include <byteswap.h>
> +#include <linux/bits.h>
>  
>  #include "arm-spe-pkt-decoder.h"
>  
> -#define BIT(n)		(1ULL << (n))
> -
>  #define NS_FLAG		BIT(63)
>  #define EL_FLAG		(BIT(62) | BIT(61))
> +#define CH_FLAG		BIT(62)
> +#define PAT_FLAG	GENMASK_ULL(59, 56)
>  
>  #define SPE_HEADER0_PAD			0x0
>  #define SPE_HEADER0_END			0x1
> @@ -447,10 +448,16 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  			return snprintf(buf, buf_len, "%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 3:	ns = !!(packet->payload & NS_FLAG);
> +		case 3:	{
> +			int ch = !!(packet->payload & CH_FLAG);
> +			int pat = (packet->payload & PAT_FLAG) >> 56;
> +
> +			ns = !!(packet->payload & NS_FLAG);
>  			payload &= ~(0xffULL << 56);
> -			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
> -					payload, ns);
> +			return snprintf(buf, buf_len,
> +					"PA 0x%llx ns=%d ch=%d, pat=%x",
> +					payload, ns, ch, pat);
> +			}

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

>  		default: return 0;
>  		}
>  	case ARM_SPE_CONTEXT:
> -- 
> 2.17.1
> 

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Leo Yan]

Hi Andre,

On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> that introduces very long vector operations (up to 2048 bits).
> The SPE profiling feature can tag SVE instructions with additional
> properties like predication or the effective vector length.
> 
> Decode the new operation type bits in the SPE decoder to allow the perf
> tool to correctly report about SVE instructions.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>  1 file changed, 47 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 a033f34846a6..f0c369259554 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
> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	}
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> +		case 0: {
> +			size_t blen = buf_len;
> +
> +			if ((payload & 0x89) == 0x08) {
> +				ret = snprintf(buf, buf_len, "SVE");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x2)
> +					ret = snprintf(buf, buf_len, " FP");
> +				else
> +					ret = snprintf(buf, buf_len, " INT");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));
> +				buf += ret;
> +				blen -= ret;
> +				return buf_len - blen;
> +			}
> +
> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>  					"COND-SELECT" : "INSN-OTHER");
> +			}
>  		case 1:	{
>  			size_t blen = buf_len;
>  
> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>  				buf += ret;
>  				blen -= ret;
> +			} else if ((payload & 0x0a) == 0x08) {
> +				ret = snprintf(buf, buf_len, " SVE");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				if (payload & 0x80) {
> +					ret = snprintf(buf, buf_len, " SG");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));
> +				buf += ret;
> +				blen -= ret;

The changes in this patch has been included in the patch [1].

So my summary for patches 02 ~ 05, except patch 04, other changes has
been included in the patch set "perf arm-spe: Refactor decoding &
dumping flow".

I'd like to add your patch 04 into the patch set "perf arm-spe:
Refactor decoding & dumping flow" and I will respin the patch set v2 on
the latest perf/core branch and send out to review.

For patch 01, you could continue to try to land it in the kernel.
(Maybe consolidate a bit with Wei?).

Do you think this is okay for you?

Thanks,
Leo

[1] https://lore.kernel.org/patchwork/patch/1288413/

>  			} else if (payload & 0x4) {
>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>  				buf += ret;
> -- 
> 2.17.1
> 

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[André Przywara]

On 27/09/2020 04:30, Leo Yan wrote:

Hi Leo,

> On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
>> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
>> that introduces very long vector operations (up to 2048 bits).
>> The SPE profiling feature can tag SVE instructions with additional
>> properties like predication or the effective vector length.
>>
>> Decode the new operation type bits in the SPE decoder to allow the perf
>> tool to correctly report about SVE instructions.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>>  1 file changed, 47 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 a033f34846a6..f0c369259554 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
>> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  	}
>>  	case ARM_SPE_OP_TYPE:
>>  		switch (idx) {
>> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>> +		case 0: {
>> +			size_t blen = buf_len;
>> +
>> +			if ((payload & 0x89) == 0x08) {
>> +				ret = snprintf(buf, buf_len, "SVE");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x2)
>> +					ret = snprintf(buf, buf_len, " FP");
>> +				else
>> +					ret = snprintf(buf, buf_len, " INT");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
>> +				buf += ret;
>> +				blen -= ret;
>> +				return buf_len - blen;
>> +			}
>> +
>> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>>  					"COND-SELECT" : "INSN-OTHER");
>> +			}
>>  		case 1:	{
>>  			size_t blen = buf_len;
>>  
>> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>>  				buf += ret;
>>  				blen -= ret;
>> +			} else if ((payload & 0x0a) == 0x08) {
>> +				ret = snprintf(buf, buf_len, " SVE");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				if (payload & 0x80) {
>> +					ret = snprintf(buf, buf_len, " SG");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
>> +				buf += ret;
>> +				blen -= ret;
> 
> The changes in this patch has been included in the patch [1].
> 
> So my summary for patches 02 ~ 05, except patch 04, other changes has
> been included in the patch set "perf arm-spe: Refactor decoding &
> dumping flow".

Ah, my sincere apologies, I totally missed Wei's and your series on this
(although I did some research on "prior art").

> I'd like to add your patch 04 into the patch set "perf arm-spe:
> Refactor decoding & dumping flow" and I will respin the patch set v2 on
> the latest perf/core branch and send out to review.
> 
> For patch 01, you could continue to try to land it in the kernel.
> (Maybe consolidate a bit with Wei?).
> 
> Do you think this is okay for you?

Yes, sounds like a plan. So Wei's original series is now fully
integrated into your 13-patch rework, right?

Is "[RESEND,v1,xx/13] ..." the latest revision of your series?
Do you plan on sending a v2 anytime soon? Or shall I do review on the
existing one?

Cheers,
Andre

> 
> [1] https://lore.kernel.org/patchwork/patch/1288413/
> 
>>  			} else if (payload & 0x4) {
>>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>>  				buf += ret;
>> -- 
>> 2.17.1
>>

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Leo Yan]

Hi Andre,

On Mon, Sep 28, 2020 at 11:15:53AM +0100, André Przywara wrote:

[...]

> > So my summary for patches 02 ~ 05, except patch 04, other changes has
> > been included in the patch set "perf arm-spe: Refactor decoding &
> > dumping flow".
> 
> Ah, my sincere apologies, I totally missed Wei's and your series on this
> (although I did some research on "prior art").

No worries!

> > I'd like to add your patch 04 into the patch set "perf arm-spe:
> > Refactor decoding & dumping flow" and I will respin the patch set v2 on
> > the latest perf/core branch and send out to review.
> > 
> > For patch 01, you could continue to try to land it in the kernel.
> > (Maybe consolidate a bit with Wei?).
> > 
> > Do you think this is okay for you?
> 
> Yes, sounds like a plan. So Wei's original series is now fully
> integrated into your 13-patch rework, right?

Thanks for confirmation.

You could see Wei's patch set has 4 patches [1].  I only picked the
patch 02 [2] from Wei's patch set into my refactoring patch set; the
patch 01 is for enabling driver for SVE events, the patches 03/04
introduced new synthesized events.

Patches 03 / 04 should be considered carefully and it's good to prove
these synthesized events will be useful for user cases before upstream
them.  The reason is AFAIK a good direction to generate SPE trace data
for memory events [3], and for SVE, I think we should firstly consider
if can reuse the memory event for profiling rather than adding new
synthesized events.

So I prefer to give priority for patches 01 / 02.

> Is "[RESEND,v1,xx/13] ..." the latest revision of your series?

Yes.

> Do you plan on sending a v2 anytime soon? Or shall I do review on the
> existing one?

For saving time, let me respin patch set v2 and send to LKML (hope in
next 1~2 days).  Then you could review patch set v2.

Thanks,
Leo

[1] https://lore.kernel.org/patchwork/cover/1278778/
[2] https://lore.kernel.org/patchwork/patch/1278780/
[3] https://lore.kernel.org/patchwork/cover/1298085/

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Dave Martin]

On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> that introduces very long vector operations (up to 2048 bits).

(8192, in fact, though don't expect to see that on real hardware any
time soon...  qemu and the Arm fast model can do it, though.)

> The SPE profiling feature can tag SVE instructions with additional
> properties like predication or the effective vector length.
> 
> Decode the new operation type bits in the SPE decoder to allow the perf
> tool to correctly report about SVE instructions.


I don't know anything about SPE, so just commenting on a few minor
things that catch my eye here.

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>  1 file changed, 47 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 a033f34846a6..f0c369259554 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
> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	}
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> +		case 0: {
> +			size_t blen = buf_len;
> +
> +			if ((payload & 0x89) == 0x08) {
> +				ret = snprintf(buf, buf_len, "SVE");
> +				buf += ret;
> +				blen -= ret;

(Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
the s*printf() family -- if this assumption is widespread in perf tool
a lready that I guess just go with the flow.)

I wonder if this snprintf+increment+decrement sequence could be wrapped
up as a helper, rather than having to be repeated all over the place.

> +				if (payload & 0x2)
> +					ret = snprintf(buf, buf_len, " FP");
> +				else
> +					ret = snprintf(buf, buf_len, " INT");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));

Isn't this just extracting 3 bits (0x7)?  And what unit are we aiming
for here: is it the number of bytes per vector, or something else?  I'm
confused by the fact that this will go up in steps of 32, which doesn't
seem to match up to the architecure.

I notice that bit 7 has to be zero to get into this if() though.

> +				buf += ret;
> +				blen -= ret;
> +				return buf_len - blen;
> +			}
> +
> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>  					"COND-SELECT" : "INSN-OTHER");
> +			}
>  		case 1:	{
>  			size_t blen = buf_len;
>  
> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>  				buf += ret;
>  				blen -= ret;
> +			} else if ((payload & 0x0a) == 0x08) {
> +				ret = snprintf(buf, buf_len, " SVE");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				if (payload & 0x80) {
> +					ret = snprintf(buf, buf_len, " SG");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));

Same comment as above.  Maybe have a common helper for decoding the
vector length bits so it can be fixed in a single place?

> +				buf += ret;
> +				blen -= ret;
>  			} else if (payload & 0x4) {
>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>  				buf += ret;

Cheers
---Dave

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[André Przywara]

On 28/09/2020 14:21, Dave Martin wrote:

Hi Dave,

> On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
>> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
>> that introduces very long vector operations (up to 2048 bits).
> 
> (8192, in fact, though don't expect to see that on real hardware any
> time soon...  qemu and the Arm fast model can do it, though.)
> 
>> The SPE profiling feature can tag SVE instructions with additional
>> properties like predication or the effective vector length.
>>
>> Decode the new operation type bits in the SPE decoder to allow the perf
>> tool to correctly report about SVE instructions.
> 
> 
> I don't know anything about SPE, so just commenting on a few minor
> things that catch my eye here.

Many thanks for taking a look!
Please note that I actually missed a prior submission by Wei, so the
code changes here will end up in:
https://lore.kernel.org/patchwork/patch/1288413/

But your two points below magically apply to his patch as well, so....

> 
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>>  1 file changed, 47 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 a033f34846a6..f0c369259554 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
>> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  	}
>>  	case ARM_SPE_OP_TYPE:
>>  		switch (idx) {
>> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>> +		case 0: {
>> +			size_t blen = buf_len;
>> +
>> +			if ((payload & 0x89) == 0x08) {
>> +				ret = snprintf(buf, buf_len, "SVE");
>> +				buf += ret;
>> +				blen -= ret;
> 
> (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> the s*printf() family -- if this assumption is widespread in perf tool
> a lready that I guess just go with the flow.)

Yeah, some parts of the code in here check for -1, actually, but doing
this on every call to snprintf would push this current code over the
edge - and I cowardly avoided a refactoring ;-)

Please note that his is perf userland, and also we are printing constant
strings here.
Although admittedly this starts to sounds like an excuse now ...

> I wonder if this snprintf+increment+decrement sequence could be wrapped
> up as a helper, rather than having to be repeated all over the place.

Yes, I was hoping nobody would notice ;-)

>> +				if (payload & 0x2)
>> +					ret = snprintf(buf, buf_len, " FP");
>> +				else
>> +					ret = snprintf(buf, buf_len, " INT");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
> 
> Isn't this just extracting 3 bits (0x7)? 

Ah, right, the comment is wrong. It's actually bits [6:4].

> And what unit are we aiming
> for here: is it the number of bytes per vector, or something else?  I'm
> confused by the fact that this will go up in steps of 32, which doesn't
> seem to match up to the architecure.

So this is how SPE encodes the effective vector length in its payload:
the format is described in section "D10.2.7 Operation Type packet" in a
(recent) ARMv8 ARM. I put the above statement in a C file and ran all
input values through it, it produced the exact *bit* length values as in
the spec.

Is there any particular pattern you are concerned about?
I admit this is somewhat hackish, I can do an extra function to put some
comments in there.

> 
> I notice that bit 7 has to be zero to get into this if() though.
> 
>> +				buf += ret;
>> +				blen -= ret;
>> +				return buf_len - blen;
>> +			}
>> +
>> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>>  					"COND-SELECT" : "INSN-OTHER");
>> +			}
>>  		case 1:	{
>>  			size_t blen = buf_len;
>>  
>> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>>  				buf += ret;
>>  				blen -= ret;
>> +			} else if ((payload & 0x0a) == 0x08) {
>> +				ret = snprintf(buf, buf_len, " SVE");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				if (payload & 0x80) {
>> +					ret = snprintf(buf, buf_len, " SG");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
> 
> Same comment as above.  Maybe have a common helper for decoding the
> vector length bits so it can be fixed in a single place?

Yup. Although I wonder if this is the smallest of the problems with this
function going forward.

Cheers,
Andre

> 
>> +				buf += ret;
>> +				blen -= ret;
>>  			} else if (payload & 0x4) {
>>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>>  				buf += ret;
> 
> Cheers
> ---Dave
> 

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Dave Martin]

On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> On 28/09/2020 14:21, Dave Martin wrote:
> 
> Hi Dave,
> 
> > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> >> that introduces very long vector operations (up to 2048 bits).
> > 
> > (8192, in fact, though don't expect to see that on real hardware any
> > time soon...  qemu and the Arm fast model can do it, though.)
> > 
> >> The SPE profiling feature can tag SVE instructions with additional
> >> properties like predication or the effective vector length.
> >>
> >> Decode the new operation type bits in the SPE decoder to allow the perf
> >> tool to correctly report about SVE instructions.
> > 
> > 
> > I don't know anything about SPE, so just commenting on a few minor
> > things that catch my eye here.
> 
> Many thanks for taking a look!
> Please note that I actually missed a prior submission by Wei, so the
> code changes here will end up in:
> https://lore.kernel.org/patchwork/patch/1288413/
> 
> But your two points below magically apply to his patch as well, so....
> 
> > 
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
> >>  1 file changed, 47 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 a033f34846a6..f0c369259554 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
> >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>  	}
> >>  	case ARM_SPE_OP_TYPE:
> >>  		switch (idx) {
> >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> >> +		case 0: {
> >> +			size_t blen = buf_len;
> >> +
> >> +			if ((payload & 0x89) == 0x08) {
> >> +				ret = snprintf(buf, buf_len, "SVE");
> >> +				buf += ret;
> >> +				blen -= ret;
> > 
> > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > the s*printf() family -- if this assumption is widespread in perf tool
> > a lready that I guess just go with the flow.)
> 
> Yeah, some parts of the code in here check for -1, actually, but doing
> this on every call to snprintf would push this current code over the
> edge - and I cowardly avoided a refactoring ;-)
> 
> Please note that his is perf userland, and also we are printing constant
> strings here.
> Although admittedly this starts to sounds like an excuse now ...
> 
> > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > up as a helper, rather than having to be repeated all over the place.
> 
> Yes, I was hoping nobody would notice ;-)

It's probably not worth losing sleep over.

snprintf(3) says, under NOTES:

	Until glibc 2.0.6, they would return -1 when the output was
	truncated.

which is probably ancient enough history that we don't care.  C11 does
say that a negative return value can happen "if an encoding error
occurred".  _Probably_ not a problem if perf tool never calls
setlocale(), but ...


> >> +				if (payload & 0x2)
> >> +					ret = snprintf(buf, buf_len, " FP");
> >> +				else
> >> +					ret = snprintf(buf, buf_len, " INT");
> >> +				buf += ret;
> >> +				blen -= ret;
> >> +				if (payload & 0x4) {
> >> +					ret = snprintf(buf, buf_len, " PRED");
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				/* Bits [7..4] encode the vector length */
> >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> >> +					       32 << ((payload >> 4) & 0x7));
> > 
> > Isn't this just extracting 3 bits (0x7)? 
> 
> Ah, right, the comment is wrong. It's actually bits [6:4].
> 
> > And what unit are we aiming
> > for here: is it the number of bytes per vector, or something else?  I'm
> > confused by the fact that this will go up in steps of 32, which doesn't
> > seem to match up to the architecure.
> 
> So this is how SPE encodes the effective vector length in its payload:
> the format is described in section "D10.2.7 Operation Type packet" in a
> (recent) ARMv8 ARM. I put the above statement in a C file and ran all
> input values through it, it produced the exact *bit* length values as in
> the spec.
> 
> Is there any particular pattern you are concerned about?
> I admit this is somewhat hackish, I can do an extra function to put some
> comments in there.

Mostly I'm curious because the encoding doesn't match the SVE
architecture: SVE requires 4 bits to specify the vector length, not 3.
This might have been a deliberate limitation in the SPE spec., but it
raises questions about what should happen when 3 bits is not enough.

For SVE, valid vector lengths are 16 bytes * n
or equivalently 128 bits * n), where 1 <= n <= 16.

The code here though cannot print EVLEN16 or EVLEN48 etc.  This might
not be a bug, but I'd like to understand where it comes from...

> 
> > 
> > I notice that bit 7 has to be zero to get into this if() though.
> > 
> >> +				buf += ret;
> >> +				blen -= ret;
> >> +				return buf_len - blen;
> >> +			}
> >> +
> >> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> >>  					"COND-SELECT" : "INSN-OTHER");
> >> +			}
> >>  		case 1:	{
> >>  			size_t blen = buf_len;
> >>  
> >> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
> >>  				buf += ret;
> >>  				blen -= ret;
> >> +			} else if ((payload & 0x0a) == 0x08) {
> >> +				ret = snprintf(buf, buf_len, " SVE");
> >> +				buf += ret;
> >> +				blen -= ret;
> >> +				if (payload & 0x4) {
> >> +					ret = snprintf(buf, buf_len, " PRED");
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				if (payload & 0x80) {
> >> +					ret = snprintf(buf, buf_len, " SG");
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				/* Bits [7..4] encode the vector length */
> >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> >> +					       32 << ((payload >> 4) & 0x7));
> > 
> > Same comment as above.  Maybe have a common helper for decoding the
> > vector length bits so it can be fixed in a single place?
> 
> Yup. Although I wonder if this is the smallest of the problems with this
> function going forward.
> 
> Cheers,
> Andre

Fair enough.

Cheers
---Dave

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Leo Yan]

On Mon, Sep 28, 2020 at 03:47:56PM +0100, Dave Martin wrote:
> On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> > On 28/09/2020 14:21, Dave Martin wrote:
> > 
> > Hi Dave,
> > 
> > > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> > >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> > >> that introduces very long vector operations (up to 2048 bits).
> > > 
> > > (8192, in fact, though don't expect to see that on real hardware any
> > > time soon...  qemu and the Arm fast model can do it, though.)
> > > 
> > >> The SPE profiling feature can tag SVE instructions with additional
> > >> properties like predication or the effective vector length.
> > >>
> > >> Decode the new operation type bits in the SPE decoder to allow the perf
> > >> tool to correctly report about SVE instructions.
> > > 
> > > 
> > > I don't know anything about SPE, so just commenting on a few minor
> > > things that catch my eye here.
> > 
> > Many thanks for taking a look!
> > Please note that I actually missed a prior submission by Wei, so the
> > code changes here will end up in:
> > https://lore.kernel.org/patchwork/patch/1288413/
> > 
> > But your two points below magically apply to his patch as well, so....
> > 
> > > 
> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > >> ---
> > >>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
> > >>  1 file changed, 47 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 a033f34846a6..f0c369259554 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
> > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > >>  	}
> > >>  	case ARM_SPE_OP_TYPE:
> > >>  		switch (idx) {
> > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > >> +		case 0: {
> > >> +			size_t blen = buf_len;
> > >> +
> > >> +			if ((payload & 0x89) == 0x08) {
> > >> +				ret = snprintf(buf, buf_len, "SVE");
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > > 
> > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > the s*printf() family -- if this assumption is widespread in perf tool
> > > a lready that I guess just go with the flow.)
> > 
> > Yeah, some parts of the code in here check for -1, actually, but doing
> > this on every call to snprintf would push this current code over the
> > edge - and I cowardly avoided a refactoring ;-)
> > 
> > Please note that his is perf userland, and also we are printing constant
> > strings here.
> > Although admittedly this starts to sounds like an excuse now ...
> > 
> > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > up as a helper, rather than having to be repeated all over the place.
> > 
> > Yes, I was hoping nobody would notice ;-)
> 
> It's probably not worth losing sleep over.
> 
> snprintf(3) says, under NOTES:
> 
> 	Until glibc 2.0.6, they would return -1 when the output was
> 	truncated.
> 
> which is probably ancient enough history that we don't care.  C11 does
> say that a negative return value can happen "if an encoding error
> occurred".  _Probably_ not a problem if perf tool never calls
> setlocale(), but ...

I have one patch which tried to fix the snprintf+increment sequence
[1], to be honest, the change seems urgly for me.  I agree it's better
to use a helper to wrap up.

[1] https://lore.kernel.org/patchwork/patch/1288410/

> > >> +				if (payload & 0x2)
> > >> +					ret = snprintf(buf, buf_len, " FP");
> > >> +				else
> > >> +					ret = snprintf(buf, buf_len, " INT");
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > >> +				if (payload & 0x4) {
> > >> +					ret = snprintf(buf, buf_len, " PRED");
> > >> +					buf += ret;
> > >> +					blen -= ret;
> > >> +				}
> > >> +				/* Bits [7..4] encode the vector length */
> > >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> > >> +					       32 << ((payload >> 4) & 0x7));
> > > 
> > > Isn't this just extracting 3 bits (0x7)? 
> > 
> > Ah, right, the comment is wrong. It's actually bits [6:4].
> > 
> > > And what unit are we aiming
> > > for here: is it the number of bytes per vector, or something else?  I'm
> > > confused by the fact that this will go up in steps of 32, which doesn't
> > > seem to match up to the architecure.
> > 
> > So this is how SPE encodes the effective vector length in its payload:
> > the format is described in section "D10.2.7 Operation Type packet" in a
> > (recent) ARMv8 ARM. I put the above statement in a C file and ran all
> > input values through it, it produced the exact *bit* length values as in
> > the spec.
> > 
> > Is there any particular pattern you are concerned about?
> > I admit this is somewhat hackish, I can do an extra function to put some
> > comments in there.
> 
> Mostly I'm curious because the encoding doesn't match the SVE
> architecture: SVE requires 4 bits to specify the vector length, not 3.
> This might have been a deliberate limitation in the SPE spec., but it
> raises questions about what should happen when 3 bits is not enough.
> 
> For SVE, valid vector lengths are 16 bytes * n
> or equivalently 128 bits * n), where 1 <= n <= 16.
> 
> The code here though cannot print EVLEN16 or EVLEN48 etc.  This might
> not be a bug, but I'd like to understand where it comes from...

In the SPE's spec, the defined values for EVL are:

  0b'000 -> EVLEN: 32 bits.
  0b'001 -> EVLEN: 64 bits.
  0b'010 -> EVLEN: 128 bits.
  0b'011 -> EVLEN: 256 bits.
  0b'100 -> EVLEN: 512 bits.
  0b'101 -> EVLEN: 1024 bits.
  0b'110 -> EVLEN: 2048 bits.

Note that 0b'111 is reserved.  In theory, I think SPE Operation packet
can support up to 4196 bits (32 << 7) when the EVL field is 0b'111; but
it's impossible to express vector length for 8192 bits as you mentioned.

Thanks,
Leo

> > > I notice that bit 7 has to be zero to get into this if() though.
> > > 
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > >> +				return buf_len - blen;
> > >> +			}
> > >> +
> > >> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > >>  					"COND-SELECT" : "INSN-OTHER");
> > >> +			}
> > >>  		case 1:	{
> > >>  			size_t blen = buf_len;
> > >>  
> > >> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > >>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
> > >>  				buf += ret;
> > >>  				blen -= ret;
> > >> +			} else if ((payload & 0x0a) == 0x08) {
> > >> +				ret = snprintf(buf, buf_len, " SVE");
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > >> +				if (payload & 0x4) {
> > >> +					ret = snprintf(buf, buf_len, " PRED");
> > >> +					buf += ret;
> > >> +					blen -= ret;
> > >> +				}
> > >> +				if (payload & 0x80) {
> > >> +					ret = snprintf(buf, buf_len, " SG");
> > >> +					buf += ret;
> > >> +					blen -= ret;
> > >> +				}
> > >> +				/* Bits [7..4] encode the vector length */
> > >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> > >> +					       32 << ((payload >> 4) & 0x7));
> > > 
> > > Same comment as above.  Maybe have a common helper for decoding the
> > > vector length bits so it can be fixed in a single place?
> > 
> > Yup. Although I wonder if this is the smallest of the problems with this
> > function going forward.
> > 
> > Cheers,
> > Andre
> 
> Fair enough.
> 
> Cheers
> ---Dave

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Dave Martin]

On Tue, Sep 29, 2020 at 10:19:02AM +0800, Leo Yan wrote:
> On Mon, Sep 28, 2020 at 03:47:56PM +0100, Dave Martin wrote:
> > On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> > > On 28/09/2020 14:21, Dave Martin wrote:
> > > 
> > > Hi Dave,
> > > 
> > > > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> > > >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> > > >> that introduces very long vector operations (up to 2048 bits).
> > > > 
> > > > (8192, in fact, though don't expect to see that on real hardware any
> > > > time soon...  qemu and the Arm fast model can do it, though.)

[...]

> > Mostly I'm curious because the encoding doesn't match the SVE
> > architecture: SVE requires 4 bits to specify the vector length, not 3.
> > This might have been a deliberate limitation in the SPE spec., but it
> > raises questions about what should happen when 3 bits is not enough.
> > 
> > For SVE, valid vector lengths are 16 bytes * n
> > or equivalently 128 bits * n), where 1 <= n <= 16.
> > 
> > The code here though cannot print EVLEN16 or EVLEN48 etc.  This might
> > not be a bug, but I'd like to understand where it comes from...
> 
> In the SPE's spec, the defined values for EVL are:
> 
>   0b'000 -> EVLEN: 32 bits.
>   0b'001 -> EVLEN: 64 bits.
>   0b'010 -> EVLEN: 128 bits.
>   0b'011 -> EVLEN: 256 bits.
>   0b'100 -> EVLEN: 512 bits.
>   0b'101 -> EVLEN: 1024 bits.
>   0b'110 -> EVLEN: 2048 bits.
> 
> Note that 0b'111 is reserved.  In theory, I think SPE Operation packet
> can support up to 4196 bits (32 << 7) when the EVL field is 0b'111; but

OK, having looked at the spec I can now confirm that this look correct.
I was expecting a more direct correspondence between the SVE ISA and
these events, but it looks like SPE may report on a finer granularity
than whole instructions, hence showing effective vector lengths smaller
than 32; also SPE rounds the reported effective vector length up to a
power of two, which allows the full range of lengths to be reported via
the 3-bit EVL field.

> it's impossible to express vector length for 8192 bits as you mentioned.

Yes, ignore my comment about 8192-bit vectors: I was confusing myself
(the Linux API extensions support up to 8192 _bytes_ per vector in order
to have some expansion room just in case; however the SVE architecture
limits vectors to at most 2048 bits).

So I don't see any obvious issues.

It might be a good idea to explicitly reject the encoding 0b111, since
we can't be certain what it is going to mean -- however, I don't have a
strong opinion on this.

Cheers
---Dave

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Dave Martin]

On Tue, Sep 29, 2020 at 10:19:02AM +0800, Leo Yan wrote:
> On Mon, Sep 28, 2020 at 03:47:56PM +0100, Dave Martin wrote:
> > On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> > > On 28/09/2020 14:21, Dave Martin wrote:
> > > 
> > > Hi Dave,
> > > 
> > > > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> > > >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> > > >> that introduces very long vector operations (up to 2048 bits).
> > > > 
> > > > (8192, in fact, though don't expect to see that on real hardware any
> > > > time soon...  qemu and the Arm fast model can do it, though.)
> > > > 
> > > >> The SPE profiling feature can tag SVE instructions with additional
> > > >> properties like predication or the effective vector length.
> > > >>
> > > >> Decode the new operation type bits in the SPE decoder to allow the perf
> > > >> tool to correctly report about SVE instructions.
> > > > 
> > > > 
> > > > I don't know anything about SPE, so just commenting on a few minor
> > > > things that catch my eye here.
> > > 
> > > Many thanks for taking a look!
> > > Please note that I actually missed a prior submission by Wei, so the
> > > code changes here will end up in:
> > > https://lore.kernel.org/patchwork/patch/1288413/
> > > 
> > > But your two points below magically apply to his patch as well, so....
> > > 
> > > > 
> > > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > >> ---
> > > >>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
> > > >>  1 file changed, 47 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 a033f34846a6..f0c369259554 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
> > > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > > >>  	}
> > > >>  	case ARM_SPE_OP_TYPE:
> > > >>  		switch (idx) {
> > > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > > >> +		case 0: {
> > > >> +			size_t blen = buf_len;
> > > >> +
> > > >> +			if ((payload & 0x89) == 0x08) {
> > > >> +				ret = snprintf(buf, buf_len, "SVE");
> > > >> +				buf += ret;
> > > >> +				blen -= ret;
> > > > 
> > > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > > the s*printf() family -- if this assumption is widespread in perf tool
> > > > a lready that I guess just go with the flow.)
> > > 
> > > Yeah, some parts of the code in here check for -1, actually, but doing
> > > this on every call to snprintf would push this current code over the
> > > edge - and I cowardly avoided a refactoring ;-)
> > > 
> > > Please note that his is perf userland, and also we are printing constant
> > > strings here.
> > > Although admittedly this starts to sounds like an excuse now ...
> > > 
> > > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > > up as a helper, rather than having to be repeated all over the place.
> > > 
> > > Yes, I was hoping nobody would notice ;-)
> > 
> > It's probably not worth losing sleep over.
> > 
> > snprintf(3) says, under NOTES:
> > 
> > 	Until glibc 2.0.6, they would return -1 when the output was
> > 	truncated.
> > 
> > which is probably ancient enough history that we don't care.  C11 does
> > say that a negative return value can happen "if an encoding error
> > occurred".  _Probably_ not a problem if perf tool never calls
> > setlocale(), but ...
> 
> I have one patch which tried to fix the snprintf+increment sequence
> [1], to be honest, the change seems urgly for me.  I agree it's better
> to use a helper to wrap up.
> 
> [1] https://lore.kernel.org/patchwork/patch/1288410/

Sure, putting explicit checks all over the place makes a lot of noise in
the code.

I was wondering whether something along the following lines would work:

	/* ... */

	if (payload & SVE_EVT_PKT_GEN_EXCEPTION)
		buf_appendf_err(&buf, &buf_len, &ret, " EXCEPTION-GEN");
	if (payload & SVE_EVT_PKT_ARCH_RETIRED)
		buf_appendf_err(&buf, &buf_len, &ret, " RETIRED");
	if (payload & SVE_EVT_PKT_L1D_ACCESS)
		buf_appendf_err(&buf, &buf_len, &ret, " L1D-ACCESS");

	/* ... */

	if (ret)
		return ret;

[...]

Best to keep such refactoring independent of this series though.

Cheers
---Dave

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Leo Yan]

Hi Dave,

On Wed, Sep 30, 2020 at 11:34:11AM +0100, Dave Martin wrote:

[...]

> > > > >> 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 a033f34846a6..f0c369259554 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
> > > > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > > > >>  	}
> > > > >>  	case ARM_SPE_OP_TYPE:
> > > > >>  		switch (idx) {
> > > > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > > > >> +		case 0: {
> > > > >> +			size_t blen = buf_len;
> > > > >> +
> > > > >> +			if ((payload & 0x89) == 0x08) {
> > > > >> +				ret = snprintf(buf, buf_len, "SVE");
> > > > >> +				buf += ret;
> > > > >> +				blen -= ret;
> > > > > 
> > > > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > > > the s*printf() family -- if this assumption is widespread in perf tool
> > > > > a lready that I guess just go with the flow.)
> > > > 
> > > > Yeah, some parts of the code in here check for -1, actually, but doing
> > > > this on every call to snprintf would push this current code over the
> > > > edge - and I cowardly avoided a refactoring ;-)
> > > > 
> > > > Please note that his is perf userland, and also we are printing constant
> > > > strings here.
> > > > Although admittedly this starts to sounds like an excuse now ...
> > > > 
> > > > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > > > up as a helper, rather than having to be repeated all over the place.
> > > > 
> > > > Yes, I was hoping nobody would notice ;-)
> > > 
> > > It's probably not worth losing sleep over.
> > > 
> > > snprintf(3) says, under NOTES:
> > > 
> > > 	Until glibc 2.0.6, they would return -1 when the output was
> > > 	truncated.
> > > 
> > > which is probably ancient enough history that we don't care.  C11 does
> > > say that a negative return value can happen "if an encoding error
> > > occurred".  _Probably_ not a problem if perf tool never calls
> > > setlocale(), but ...
> > 
> > I have one patch which tried to fix the snprintf+increment sequence
> > [1], to be honest, the change seems urgly for me.  I agree it's better
> > to use a helper to wrap up.
> > 
> > [1] https://lore.kernel.org/patchwork/patch/1288410/
> 
> Sure, putting explicit checks all over the place makes a lot of noise in
> the code.
> 
> I was wondering whether something along the following lines would work:
> 
> 	/* ... */
> 
> 	if (payload & SVE_EVT_PKT_GEN_EXCEPTION)
> 		buf_appendf_err(&buf, &buf_len, &ret, " EXCEPTION-GEN");
> 	if (payload & SVE_EVT_PKT_ARCH_RETIRED)
> 		buf_appendf_err(&buf, &buf_len, &ret, " RETIRED");
> 	if (payload & SVE_EVT_PKT_L1D_ACCESS)
> 		buf_appendf_err(&buf, &buf_len, &ret, " L1D-ACCESS");
> 
> 	/* ... */
> 
> 	if (ret)
> 		return ret;
> 
> [...]

I have sent out the patch v2 [1] and Cc'ed you; I used a similiar API
definition with your suggestion:

  static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
 				  const char *fmt, ...)

Only a difference is when return from arm_spe_pkt_snprintf(), will check
the return value and directly bail out when detect failure.  Your input
will be considered for next spin.

> Best to keep such refactoring independent of this series though.

Yeah, the patch set [2] is quite heavy; after get some reviewing,
maybe need to consider to split into 2 or even 3 small patch sets.

Thanks a lot for your suggestions!

Leo

[1] https://lore.kernel.org/patchwork/patch/1314603/
[2] https://lore.kernel.org/patchwork/cover/1314599/

Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

[Dave Martin]

On Wed, Sep 30, 2020 at 07:04:53PM +0800, Leo Yan wrote:

[...]

> > > > > >> 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 a033f34846a6..f0c369259554 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
> > > > > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > > > > >>  	}
> > > > > >>  	case ARM_SPE_OP_TYPE:
> > > > > >>  		switch (idx) {
> > > > > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > > > > >> +		case 0: {
> > > > > >> +			size_t blen = buf_len;
> > > > > >> +
> > > > > >> +			if ((payload & 0x89) == 0x08) {
> > > > > >> +				ret = snprintf(buf, buf_len, "SVE");
> > > > > >> +				buf += ret;
> > > > > >> +				blen -= ret;
> > > > > > 
> > > > > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > > > > the s*printf() family -- if this assumption is widespread in perf tool
> > > > > > a lready that I guess just go with the flow.)
> > > > > 
> > > > > Yeah, some parts of the code in here check for -1, actually, but doing
> > > > > this on every call to snprintf would push this current code over the
> > > > > edge - and I cowardly avoided a refactoring ;-)
> > > > > 
> > > > > Please note that his is perf userland, and also we are printing constant
> > > > > strings here.
> > > > > Although admittedly this starts to sounds like an excuse now ...
> > > > > 
> > > > > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > > > > up as a helper, rather than having to be repeated all over the place.
> > > > > 
> > > > > Yes, I was hoping nobody would notice ;-)
> > > > 
> > > > It's probably not worth losing sleep over.
> > > > 
> > > > snprintf(3) says, under NOTES:
> > > > 
> > > > 	Until glibc 2.0.6, they would return -1 when the output was
> > > > 	truncated.
> > > > 
> > > > which is probably ancient enough history that we don't care.  C11 does
> > > > say that a negative return value can happen "if an encoding error
> > > > occurred".  _Probably_ not a problem if perf tool never calls
> > > > setlocale(), but ...
> > > 
> > > I have one patch which tried to fix the snprintf+increment sequence
> > > [1], to be honest, the change seems urgly for me.  I agree it's better
> > > to use a helper to wrap up.
> > > 
> > > [1] https://lore.kernel.org/patchwork/patch/1288410/
> > 
> > Sure, putting explicit checks all over the place makes a lot of noise in
> > the code.
> > 
> > I was wondering whether something along the following lines would work:
> > 
> > 	/* ... */
> > 
> > 	if (payload & SVE_EVT_PKT_GEN_EXCEPTION)
> > 		buf_appendf_err(&buf, &buf_len, &ret, " EXCEPTION-GEN");
> > 	if (payload & SVE_EVT_PKT_ARCH_RETIRED)
> > 		buf_appendf_err(&buf, &buf_len, &ret, " RETIRED");
> > 	if (payload & SVE_EVT_PKT_L1D_ACCESS)
> > 		buf_appendf_err(&buf, &buf_len, &ret, " L1D-ACCESS");
> > 
> > 	/* ... */
> > 
> > 	if (ret)
> > 		return ret;
> > 
> > [...]
> 
> I have sent out the patch v2 [1] and Cc'ed you; I used a similiar API
> definition with your suggestion:
> 
>   static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
>  				  const char *fmt, ...)
> 
> Only a difference is when return from arm_spe_pkt_snprintf(), will check
> the return value and directly bail out when detect failure.  Your input
> will be considered for next spin.
> 
> > Best to keep such refactoring independent of this series though.
> 
> Yeah, the patch set [2] is quite heavy; after get some reviewing,
> maybe need to consider to split into 2 or even 3 small patch sets.
> 
> Thanks a lot for your suggestions!
>
> Leo

No problem, your approach seems reasonable to me.

Cheers
---Dave

> [1] https://lore.kernel.org/patchwork/patch/1314603/
> [2] https://lore.kernel.org/patchwork/cover/1314599/
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] perf: arm_spe: Decode memory tagging properties

[Arnaldo Carvalho de Melo]

Em Tue, Oct 13, 2020 at 11:51:03AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sun, Sep 27, 2020 at 11:19:18AM +0800, Leo Yan escreveu:
> > On Tue, Sep 22, 2020 at 11:12:24AM +0100, Andre Przywara wrote:
> > > 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.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  .../util/arm-spe-decoder/arm-spe-pkt-decoder.c  | 17 ++++++++++++-----
> > >  1 file changed, 12 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 943e4155b246..a033f34846a6 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,13 +8,14 @@
> > >  #include <string.h>
> > >  #include <endian.h>
> > >  #include <byteswap.h>
> > > +#include <linux/bits.h>
> > >  
> > >  #include "arm-spe-pkt-decoder.h"
> > >  
> > > -#define BIT(n)		(1ULL << (n))
> > > -
> > >  #define NS_FLAG		BIT(63)
> > >  #define EL_FLAG		(BIT(62) | BIT(61))
> > > +#define CH_FLAG		BIT(62)
> > > +#define PAT_FLAG	GENMASK_ULL(59, 56)
> > >  
> > >  #define SPE_HEADER0_PAD			0x0
> > >  #define SPE_HEADER0_END			0x1
> > > @@ -447,10 +448,16 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > >  			return snprintf(buf, buf_len, "%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 3:	ns = !!(packet->payload & NS_FLAG);
> > > +		case 3:	{
> > > +			int ch = !!(packet->payload & CH_FLAG);
> > > +			int pat = (packet->payload & PAT_FLAG) >> 56;
> > > +
> > > +			ns = !!(packet->payload & NS_FLAG);
> > >  			payload &= ~(0xffULL << 56);
> > > -			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
> > > -					payload, ns);
> > > +			return snprintf(buf, buf_len,
> > > +					"PA 0x%llx ns=%d ch=%d, pat=%x",
> > > +					payload, ns, ch, pat);
> > > +			}
> > 
> > Reviewed-by: Leo Yan <leo.yan@linaro.org>
> 
> Thanks, applied.

I take that back, I'm applying Leo's series that Andre reviewed instead.

- Arnaldo