[PATCH RFC v1 0/3] perf: Arm SPEv1.2 support

[PATCH RFC v1 0/3] perf: Arm SPEv1.2 support

[Rob Herring]

This series adds support for Arm SPEv1.2 which is part of the
Armv8.7/Armv9.2 architecture. There's 2 new features that affect the 
kernel: a new event filter bit, branch 'not taken', and an inverted 
event filter register. 

RFC as this is compile tested only.

---
Rob Herring (3):
      perf: arm_spe: Support new SPEv1.2/v8.7 'not taken' event
      perf: Add perf_event_attr::config3
      perf: arm_spe: Add support for SPEv1.2 inverted event filtering

 arch/arm64/include/asm/sysreg.h       |  5 ++++
 drivers/perf/arm_spe_pmu.c            | 47 ++++++++++++++++++++++++++++++++++-
 include/uapi/linux/perf_event.h       |  3 +++
 tools/include/uapi/linux/perf_event.h |  3 +++
 4 files changed, 57 insertions(+), 1 deletion(-)
---
base-commit: 1c23f9e627a7b412978b4e852793c5e3c3efc555
change-id: 20220825-arm-spe-v8-7-fedf04e16f23

Best regards,
--
Rob Herring <robh@kernel.org>

-- 
b4 0.10.0-dev

[PATCH RFC v1 1/3] perf: arm_spe: Support new SPEv1.2/v8.7 'not taken' event

[Rob Herring]

Arm SPEv1.2 (Armv8.7/v9.2) adds a new event, 'not taken', in bit 6 of
the PMSEVFR_EL1 register. Update arm_spe_pmsevfr_res0() to support the
additional event.

Signed-off-by: Rob Herring <robh@kernel.org>

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7c71358d44c4..57904c11aece 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -312,6 +312,8 @@
 	 BIT_ULL(6) | BIT_ULL(4) | BIT_ULL(2) | BIT_ULL(0))
 #define SYS_PMSEVFR_EL1_RES0_8_3	\
 	(SYS_PMSEVFR_EL1_RES0_8_2 & ~(BIT_ULL(18) | BIT_ULL(17) | BIT_ULL(11)))
+#define SYS_PMSEVFR_EL1_RES0_8_7	\
+	(SYS_PMSEVFR_EL1_RES0_8_3 & ~BIT_ULL(6))
 
 #define SYS_PMSLATFR_EL1		sys_reg(3, 0, 9, 9, 6)
 #define SYS_PMSLATFR_EL1_MINLAT_SHIFT	0
@@ -847,6 +849,7 @@
 
 #define ID_AA64DFR0_PMSVER_8_2		0x1
 #define ID_AA64DFR0_PMSVER_8_3		0x2
+#define ID_AA64DFR0_PMSVER_8_7		0x3
 
 #define ID_DFR0_PERFMON_SHIFT		24
 
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index b65a7d9640e1..a75b03b5c8f9 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -677,9 +677,11 @@ static u64 arm_spe_pmsevfr_res0(u16 pmsver)
 	case ID_AA64DFR0_PMSVER_8_2:
 		return SYS_PMSEVFR_EL1_RES0_8_2;
 	case ID_AA64DFR0_PMSVER_8_3:
+		return SYS_PMSEVFR_EL1_RES0_8_3;
+	case ID_AA64DFR0_PMSVER_8_7:
 	/* Return the highest version we support in default */
 	default:
-		return SYS_PMSEVFR_EL1_RES0_8_3;
+		return SYS_PMSEVFR_EL1_RES0_8_7;
 	}
 }
 

-- 
b4 0.10.0-dev

[PATCH RFC v1 2/3] perf: Add perf_event_attr::config3

[Rob Herring]

Arm SPEv1.2 adds another 64-bits of event filtering control. As the
existing perf_event_attr::configN fields are all used up for SPE PMU, an
additional field is needed. Add a new 'config3' field.

Signed-off-by: Rob Herring <robh@kernel.org>

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 03b370062741..b53f9b958235 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -333,6 +333,7 @@ enum perf_event_read_format {
 #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
 #define PERF_ATTR_SIZE_VER6	120	/* add: aux_sample_size */
 #define PERF_ATTR_SIZE_VER7	128	/* add: sig_data */
+#define PERF_ATTR_SIZE_VER8	136	/* add: config3 */
 
 /*
  * Hardware event_id to monitor via a performance monitoring event:
@@ -474,6 +475,8 @@ struct perf_event_attr {
 	 * truncated accordingly on 32 bit architectures.
 	 */
 	__u64	sig_data;
+
+	__u64	config3; /* extension of config2 */
 };
 
 /*
diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index 581ed4bdc062..7fad17853310 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -333,6 +333,7 @@ enum perf_event_read_format {
 #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
 #define PERF_ATTR_SIZE_VER6	120	/* add: aux_sample_size */
 #define PERF_ATTR_SIZE_VER7	128	/* add: sig_data */
+#define PERF_ATTR_SIZE_VER8	136	/* add: config3 */
 
 /*
  * Hardware event_id to monitor via a performance monitoring event:
@@ -474,6 +475,8 @@ struct perf_event_attr {
 	 * truncated accordingly on 32 bit architectures.
 	 */
 	__u64	sig_data;
+
+	__u64	config3; /* extension of config2 */
 };
 
 /*

-- 
b4 0.10.0-dev

Re: [PATCH RFC v1 2/3] perf: Add perf_event_attr::config3

[Arnaldo Carvalho de Melo]

Em Thu, Aug 25, 2022 at 01:08:01PM -0500, Rob Herring escreveu:
> Arm SPEv1.2 adds another 64-bits of event filtering control. As the
> existing perf_event_attr::configN fields are all used up for SPE PMU, an
> additional field is needed. Add a new 'config3' field.

Try not to have tools/ and kernel code in the same patch, else you'll
burden kernel developers into testing tools/, which so far has been
refrained.

First you get the kernel bits in, then tooling.

Locally of course you test it together.

- Arnaldo
 
> Signed-off-by: Rob Herring <robh@kernel.org>
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 03b370062741..b53f9b958235 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -333,6 +333,7 @@ enum perf_event_read_format {
>  #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
>  #define PERF_ATTR_SIZE_VER6	120	/* add: aux_sample_size */
>  #define PERF_ATTR_SIZE_VER7	128	/* add: sig_data */
> +#define PERF_ATTR_SIZE_VER8	136	/* add: config3 */
>  
>  /*
>   * Hardware event_id to monitor via a performance monitoring event:
> @@ -474,6 +475,8 @@ struct perf_event_attr {
>  	 * truncated accordingly on 32 bit architectures.
>  	 */
>  	__u64	sig_data;
> +
> +	__u64	config3; /* extension of config2 */
>  };
>  
>  /*
> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
> index 581ed4bdc062..7fad17853310 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
> @@ -333,6 +333,7 @@ enum perf_event_read_format {
>  #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
>  #define PERF_ATTR_SIZE_VER6	120	/* add: aux_sample_size */
>  #define PERF_ATTR_SIZE_VER7	128	/* add: sig_data */
> +#define PERF_ATTR_SIZE_VER8	136	/* add: config3 */
>  
>  /*
>   * Hardware event_id to monitor via a performance monitoring event:
> @@ -474,6 +475,8 @@ struct perf_event_attr {
>  	 * truncated accordingly on 32 bit architectures.
>  	 */
>  	__u64	sig_data;
> +
> +	__u64	config3; /* extension of config2 */
>  };
>  
>  /*
> 
> -- 
> b4 0.10.0-dev

-- 

- Arnaldo

Re: [PATCH RFC v1 2/3] perf: Add perf_event_attr::config3

[Rob Herring]

On Thu, Aug 25, 2022 at 2:31 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Aug 25, 2022 at 01:08:01PM -0500, Rob Herring escreveu:
> > Arm SPEv1.2 adds another 64-bits of event filtering control. As the
> > existing perf_event_attr::configN fields are all used up for SPE PMU, an
> > additional field is needed. Add a new 'config3' field.
>
> Try not to have tools/ and kernel code in the same patch, else you'll
> burden kernel developers into testing tools/, which so far has been
> refrained.

I knew that, but assumed the header was special...

Rob

Re: [PATCH RFC v1 2/3] perf: Add perf_event_attr::config3

[Arnaldo Carvalho de Melo]

Em Thu, Aug 25, 2022 at 02:43:42PM -0500, Rob Herring escreveu:
> On Thu, Aug 25, 2022 at 2:31 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > Em Thu, Aug 25, 2022 at 01:08:01PM -0500, Rob Herring escreveu:
> > > Arm SPEv1.2 adds another 64-bits of event filtering control. As the
> > > existing perf_event_attr::configN fields are all used up for SPE PMU, an
> > > additional field is needed. Add a new 'config3' field.

> > Try not to have tools/ and kernel code in the same patch, else you'll
> > burden kernel developers into testing tools/, which so far has been
> > refrained.
 
> I knew that, but assumed the header was special...

So, we have tools/perf/check-headers.sh for that purpose, so that tools
developers can have the opportunity to see that the kernel ABI changed,
think about it, check what is needed to best support the new kernel ABI,
etc.

I really love when kernel developers take the time and help with
supporting new features by working on the tools/ bits, that would be
perfect!

But, as we saw just a few days ago, when changes were made in the kernel
and in tools/ in tandem, tools/ got broken just before -rc2:

  "perf tools: Fix compile error for x86"
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cfd2b5c1106fa20254d9f24970232cdf24860005>

So it seems better to first do the kernel work, then do the user part,
to avoid burdening kernel developers with tools/ work.

- Arnaldo

Re: [PATCH RFC v1 2/3] perf: Add perf_event_attr::config3

[Rob Herring]

On Thu, Aug 25, 2022 at 1:08 PM Rob Herring <robh@kernel.org> wrote:
>
> Arm SPEv1.2 adds another 64-bits of event filtering control. As the
> existing perf_event_attr::configN fields are all used up for SPE PMU, an
> additional field is needed. Add a new 'config3' field.

In testing this, just exposing 'config3' in the format attributes
causes the SPE PMU to be disabled. So we can't add this without
breaking existing perf. Shouldn't perf just skip any format fields it
doesn't know about?

Rob

Re: [PATCH RFC v1 2/3] perf: Add perf_event_attr::config3

[Arnaldo Carvalho de Melo]

On August 31, 2022 4:06:40 PM GMT-03:00, Rob Herring <robh@kernel.org> wrote:
>On Thu, Aug 25, 2022 at 1:08 PM Rob Herring <robh@kernel.org> wrote:
>>
>> Arm SPEv1.2 adds another 64-bits of event filtering control. As the
>> existing perf_event_attr::configN fields are all used up for SPE PMU, an
>> additional field is needed. Add a new 'config3' field.
>
>In testing this, just exposing 'config3' in the format attributes
>causes the SPE PMU to be disabled. So we can't add this without
>breaking existing perf. Shouldn't perf just skip any format fields it
>doesn't know about?

Something it doesn't know about, even more at the main perf_event_attr level, may preclude it to grok new records.

"May" is the key word, as perf_event_hdr has a size field, maybe sometimes it's possible to do as you say.

If you're interested propose patches to make perf ignore things when possible.

- Arnaldo

[PATCH RFC v1 3/3] perf: arm_spe: Add support for SPEv1.2 inverted event filtering

[Rob Herring]

Arm SPEv1.2 (Arm v8.7/v9.2) adds a new feature called Inverted Event
Filter which excludes samples matching the event filter. The feature
mirrors the existing event filter in PMSEVFR_EL1 adding a new register,
PMSNEVFR_EL1, which has the same event bit assignments.

Signed-off-by: Rob Herring <robh@kernel.org>

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 57904c11aece..9744da888818 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -258,6 +258,7 @@
 #define SYS_PMSIDR_EL1_ARCHINST_SHIFT	3
 #define SYS_PMSIDR_EL1_LDS_SHIFT	4
 #define SYS_PMSIDR_EL1_ERND_SHIFT	5
+#define SYS_PMSIDR_EL1_FNE_SHIFT	6
 #define SYS_PMSIDR_EL1_INTERVAL_SHIFT	8
 #define SYS_PMSIDR_EL1_INTERVAL_MASK	0xfUL
 #define SYS_PMSIDR_EL1_MAXSIZE_SHIFT	12
@@ -302,6 +303,7 @@
 #define SYS_PMSFCR_EL1_FE_SHIFT		0
 #define SYS_PMSFCR_EL1_FT_SHIFT		1
 #define SYS_PMSFCR_EL1_FL_SHIFT		2
+#define SYS_PMSFCR_EL1_FNE_SHIFT	3
 #define SYS_PMSFCR_EL1_B_SHIFT		16
 #define SYS_PMSFCR_EL1_LD_SHIFT		17
 #define SYS_PMSFCR_EL1_ST_SHIFT		18
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index a75b03b5c8f9..724409a88423 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -82,6 +82,7 @@ struct arm_spe_pmu {
 #define SPE_PMU_FEAT_ARCH_INST			(1UL << 3)
 #define SPE_PMU_FEAT_LDS			(1UL << 4)
 #define SPE_PMU_FEAT_ERND			(1UL << 5)
+#define SPE_PMU_FEAT_INV_FILT_EVT		(1UL << 6)
 #define SPE_PMU_FEAT_DEV_PROBED			(1UL << 63)
 	u64					features;
 
@@ -199,6 +200,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
 #define ATTR_CFG_FLD_min_latency_LO		0
 #define ATTR_CFG_FLD_min_latency_HI		11
 
+#define ATTR_CFG_FLD_inv_event_filter_CFG	config3	/* PMSNEVFR_EL1 */
+#define ATTR_CFG_FLD_inv_event_filter_LO	0
+#define ATTR_CFG_FLD_inv_event_filter_HI	63
+
 /* Why does everything I do descend into this? */
 #define __GEN_PMU_FORMAT_ATTR(cfg, lo, hi)				\
 	(lo) == (hi) ? #cfg ":" #lo "\n" : #cfg ":" #lo "-" #hi
@@ -229,6 +234,7 @@ GEN_PMU_FORMAT_ATTR(branch_filter);
 GEN_PMU_FORMAT_ATTR(load_filter);
 GEN_PMU_FORMAT_ATTR(store_filter);
 GEN_PMU_FORMAT_ATTR(event_filter);
+GEN_PMU_FORMAT_ATTR(inv_event_filter);
 GEN_PMU_FORMAT_ATTR(min_latency);
 
 static struct attribute *arm_spe_pmu_formats_attr[] = {
@@ -240,12 +246,27 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
 	&format_attr_load_filter.attr,
 	&format_attr_store_filter.attr,
 	&format_attr_event_filter.attr,
+	&format_attr_inv_event_filter.attr,
 	&format_attr_min_latency.attr,
 	NULL,
 };
 
+static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
+						  struct attribute *attr,
+						  int unused)
+	{
+	struct device *dev = kobj_to_dev(kobj);
+	struct arm_spe_pmu *spe_pmu = dev_get_drvdata(dev);
+
+	if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
+		return 0;
+
+	return attr->mode;
+}
+
 static const struct attribute_group arm_spe_pmu_format_group = {
 	.name	= "format",
+	.is_visible = arm_spe_pmu_format_attr_is_visible,
 	.attrs	= arm_spe_pmu_formats_attr,
 };
 
@@ -341,6 +362,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
 	if (ATTR_CFG_GET_FLD(attr, event_filter))
 		reg |= BIT(SYS_PMSFCR_EL1_FE_SHIFT);
 
+	if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
+		reg |= BIT(SYS_PMSFCR_EL1_FNE_SHIFT);
+
 	if (ATTR_CFG_GET_FLD(attr, min_latency))
 		reg |= BIT(SYS_PMSFCR_EL1_FL_SHIFT);
 
@@ -353,6 +377,12 @@ static u64 arm_spe_event_to_pmsevfr(struct perf_event *event)
 	return ATTR_CFG_GET_FLD(attr, event_filter);
 }
 
+static u64 arm_spe_event_to_pmsnevfr(struct perf_event *event)
+{
+	struct perf_event_attr *attr = &event->attr;
+	return ATTR_CFG_GET_FLD(attr, inv_event_filter);
+}
+
 static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
 {
 	struct perf_event_attr *attr = &event->attr;
@@ -703,6 +733,9 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
 	if (arm_spe_event_to_pmsevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
 		return -EOPNOTSUPP;
 
+	if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
+		return -EOPNOTSUPP;
+
 	if (attr->exclude_idle)
 		return -EOPNOTSUPP;
 
@@ -721,6 +754,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
 	    !(spe_pmu->features & SPE_PMU_FEAT_FILT_EVT))
 		return -EOPNOTSUPP;
 
+	if ((reg & BIT(SYS_PMSFCR_EL1_FNE_SHIFT)) &&
+	    !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
+		return -EOPNOTSUPP;
+
 	if ((reg & BIT(SYS_PMSFCR_EL1_FT_SHIFT)) &&
 	    !(spe_pmu->features & SPE_PMU_FEAT_FILT_TYP))
 		return -EOPNOTSUPP;
@@ -757,6 +794,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
 	reg = arm_spe_event_to_pmsevfr(event);
 	write_sysreg_s(reg, SYS_PMSEVFR_EL1);
 
+	reg = arm_spe_event_to_pmsnevfr(event);
+	write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
+
 	reg = arm_spe_event_to_pmslatfr(event);
 	write_sysreg_s(reg, SYS_PMSLATFR_EL1);
 
@@ -991,6 +1031,9 @@ static void __arm_spe_pmu_dev_probe(void *info)
 	if (reg & BIT(SYS_PMSIDR_EL1_FE_SHIFT))
 		spe_pmu->features |= SPE_PMU_FEAT_FILT_EVT;
 
+	if (reg & BIT(SYS_PMSIDR_EL1_FNE_SHIFT))
+		spe_pmu->features |= SPE_PMU_FEAT_INV_FILT_EVT;
+
 	if (reg & BIT(SYS_PMSIDR_EL1_FT_SHIFT))
 		spe_pmu->features |= SPE_PMU_FEAT_FILT_TYP;
 

-- 
b4 0.10.0-dev

Re: [PATCH RFC v1 3/3] perf: arm_spe: Add support for SPEv1.2 inverted event filtering

[James Clark]

On 25/08/2022 19:08, Rob Herring wrote:
> Arm SPEv1.2 (Arm v8.7/v9.2) adds a new feature called Inverted Event
> Filter which excludes samples matching the event filter. The feature
> mirrors the existing event filter in PMSEVFR_EL1 adding a new register,
> PMSNEVFR_EL1, which has the same event bit assignments.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 57904c11aece..9744da888818 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -258,6 +258,7 @@
>  #define SYS_PMSIDR_EL1_ARCHINST_SHIFT	3
>  #define SYS_PMSIDR_EL1_LDS_SHIFT	4
>  #define SYS_PMSIDR_EL1_ERND_SHIFT	5
> +#define SYS_PMSIDR_EL1_FNE_SHIFT	6
>  #define SYS_PMSIDR_EL1_INTERVAL_SHIFT	8
>  #define SYS_PMSIDR_EL1_INTERVAL_MASK	0xfUL
>  #define SYS_PMSIDR_EL1_MAXSIZE_SHIFT	12
> @@ -302,6 +303,7 @@
>  #define SYS_PMSFCR_EL1_FE_SHIFT		0
>  #define SYS_PMSFCR_EL1_FT_SHIFT		1
>  #define SYS_PMSFCR_EL1_FL_SHIFT		2
> +#define SYS_PMSFCR_EL1_FNE_SHIFT	3
>  #define SYS_PMSFCR_EL1_B_SHIFT		16
>  #define SYS_PMSFCR_EL1_LD_SHIFT		17
>  #define SYS_PMSFCR_EL1_ST_SHIFT		18
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index a75b03b5c8f9..724409a88423 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -82,6 +82,7 @@ struct arm_spe_pmu {
>  #define SPE_PMU_FEAT_ARCH_INST			(1UL << 3)
>  #define SPE_PMU_FEAT_LDS			(1UL << 4)
>  #define SPE_PMU_FEAT_ERND			(1UL << 5)
> +#define SPE_PMU_FEAT_INV_FILT_EVT		(1UL << 6)
>  #define SPE_PMU_FEAT_DEV_PROBED			(1UL << 63)
>  	u64					features;
>  
> @@ -199,6 +200,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
>  #define ATTR_CFG_FLD_min_latency_LO		0
>  #define ATTR_CFG_FLD_min_latency_HI		11
>  
> +#define ATTR_CFG_FLD_inv_event_filter_CFG	config3	/* PMSNEVFR_EL1 */
> +#define ATTR_CFG_FLD_inv_event_filter_LO	0
> +#define ATTR_CFG_FLD_inv_event_filter_HI	63
> +
>  /* Why does everything I do descend into this? */
>  #define __GEN_PMU_FORMAT_ATTR(cfg, lo, hi)				\
>  	(lo) == (hi) ? #cfg ":" #lo "\n" : #cfg ":" #lo "-" #hi
> @@ -229,6 +234,7 @@ GEN_PMU_FORMAT_ATTR(branch_filter);
>  GEN_PMU_FORMAT_ATTR(load_filter);
>  GEN_PMU_FORMAT_ATTR(store_filter);
>  GEN_PMU_FORMAT_ATTR(event_filter);
> +GEN_PMU_FORMAT_ATTR(inv_event_filter);
>  GEN_PMU_FORMAT_ATTR(min_latency);
>  
>  static struct attribute *arm_spe_pmu_formats_attr[] = {
> @@ -240,12 +246,27 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
>  	&format_attr_load_filter.attr,
>  	&format_attr_store_filter.attr,
>  	&format_attr_event_filter.attr,
> +	&format_attr_inv_event_filter.attr,
>  	&format_attr_min_latency.attr,
>  	NULL,
>  };
>  
> +static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
> +						  struct attribute *attr,
> +						  int unused)
> +	{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct arm_spe_pmu *spe_pmu = dev_get_drvdata(dev);
> +
> +	if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
>  static const struct attribute_group arm_spe_pmu_format_group = {
>  	.name	= "format",
> +	.is_visible = arm_spe_pmu_format_attr_is_visible,
>  	.attrs	= arm_spe_pmu_formats_attr,
>  };
>  
> @@ -341,6 +362,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
>  	if (ATTR_CFG_GET_FLD(attr, event_filter))
>  		reg |= BIT(SYS_PMSFCR_EL1_FE_SHIFT);
>  
> +	if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
> +		reg |= BIT(SYS_PMSFCR_EL1_FNE_SHIFT);
> +
>  	if (ATTR_CFG_GET_FLD(attr, min_latency))
>  		reg |= BIT(SYS_PMSFCR_EL1_FL_SHIFT);
>  
> @@ -353,6 +377,12 @@ static u64 arm_spe_event_to_pmsevfr(struct perf_event *event)
>  	return ATTR_CFG_GET_FLD(attr, event_filter);
>  }
>  
> +static u64 arm_spe_event_to_pmsnevfr(struct perf_event *event)
> +{
> +	struct perf_event_attr *attr = &event->attr;
> +	return ATTR_CFG_GET_FLD(attr, inv_event_filter);
> +}
> +
>  static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
>  {
>  	struct perf_event_attr *attr = &event->attr;
> @@ -703,6 +733,9 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>  	if (arm_spe_event_to_pmsevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
>  		return -EOPNOTSUPP;
>  
> +	if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
> +		return -EOPNOTSUPP;
> +
>  	if (attr->exclude_idle)
>  		return -EOPNOTSUPP;
>  
> @@ -721,6 +754,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>  	    !(spe_pmu->features & SPE_PMU_FEAT_FILT_EVT))
>  		return -EOPNOTSUPP;
>  
> +	if ((reg & BIT(SYS_PMSFCR_EL1_FNE_SHIFT)) &&
> +	    !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> +		return -EOPNOTSUPP;
> +
>  	if ((reg & BIT(SYS_PMSFCR_EL1_FT_SHIFT)) &&
>  	    !(spe_pmu->features & SPE_PMU_FEAT_FILT_TYP))
>  		return -EOPNOTSUPP;
> @@ -757,6 +794,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>  	reg = arm_spe_event_to_pmsevfr(event);
>  	write_sysreg_s(reg, SYS_PMSEVFR_EL1);
>  
> +	reg = arm_spe_event_to_pmsnevfr(event);
> +	write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
> +

I think this needs to check if the feature is present before writing
otherwise you get a crash, pasted below. Otherwise it looks ok to me.

  kernel BUG at arch/arm64/kernel/traps.c:497!
  Internal error: Oops - BUG: 0 [#1] SMP
  Modules linked in: coresight_tmc coresight_stm coresight_etm4x
coresight_replicator coresight_funnel coresight_tpiu coresight arm_spe_pmu
  CPU: 1 PID: 656 Comm: perf-exec Not tainted 6.0.0-rc3+ #43
  pstate: 004000c9 (nzcv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : do_undefinstr+0x2fc/0x320
  lr : do_undefinstr+0x250/0x320
  sp : ffff80000b543730
  x29: ffff80000b543730 x28: ffff008002748e80 x27: ffff00800be55e60
  x26: ffff80000a4cfce0 x25: ffff00837df9c59c x24: ffff00837df9c598
  x23: 00000000404000c9 x22: 00000000d5189921 x21: ffff80000a8080e8
  x20: ffff008002748e80 x19: ffff80000b5437c0 x18: 0000000000000000
  x17: 0000000000000000 x16: ffff8000082d2720 x15: fffffc0200462a80
  x14: 0000000000000000 x13: 0000029000000290 x12: 003000000000000c
  x11: 00000000ffffffff x10: 00000000ffffffff x9 : ffff8000082ead7c
  x8 : 0000000000000000 x7 : 0000000000000007 x6 : 0000000000010000
  x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000d5380000
  x2 : ffff80000a426640 x1 : ffff80000a8080f8 x0 : 00000000404000c9
  Call trace:
   do_undefinstr+0x2fc/0x320
   el1_undef+0x30/0x50
   el1h_64_sync_handler+0x80/0xd0
   el1h_64_sync+0x64/0x68
   arm_spe_pmu_start+0x64/0xe8 [arm_spe_pmu]
   arm_spe_pmu_add+0x78/0x98 [arm_spe_pmu]
   event_sched_in.isra.0+0xa8/0x1a8
   merge_sched_in+0x1f4/0x3e8
   visit_groups_merge.constprop.0+0x1ac/0x480
   ctx_sched_in+0x1b0/0x1c8
   perf_event_sched_in+0x60/0x90
   ctx_resched+0x68/0xb0
   perf_event_exec+0x130/0x460
   begin_new_exec+0x5e4/0x1100
   load_elf_binary+0x3bc/0x16d8
   bprm_execve+0x280/0x620
   do_execveat_common.isra.0+0x1a4/0x240
   __arm64_sys_execve+0x48/0x60
   invoke_syscall+0x4c/0x110
   el0_svc_common.constprop.0+0x4c/0xf8
   do_el0_svc+0x34/0xc0
   el0_svc+0x2c/0x88
   el0t_64_sync_handler+0xb8/0xc0
   el0t_64_sync+0x18c/0x190
  Code: 33103ec0 2a0003f6 17ffff7a f9001bf7 (d4210000)

>  	reg = arm_spe_event_to_pmslatfr(event);
>  	write_sysreg_s(reg, SYS_PMSLATFR_EL1);
>  
> @@ -991,6 +1031,9 @@ static void __arm_spe_pmu_dev_probe(void *info)
>  	if (reg & BIT(SYS_PMSIDR_EL1_FE_SHIFT))
>  		spe_pmu->features |= SPE_PMU_FEAT_FILT_EVT;
>  
> +	if (reg & BIT(SYS_PMSIDR_EL1_FNE_SHIFT))
> +		spe_pmu->features |= SPE_PMU_FEAT_INV_FILT_EVT;
> +
>  	if (reg & BIT(SYS_PMSIDR_EL1_FT_SHIFT))
>  		spe_pmu->features |= SPE_PMU_FEAT_FILT_TYP;
>  
> 

Re: [PATCH RFC v1 3/3] perf: arm_spe: Add support for SPEv1.2 inverted event filtering

[Rob Herring]

On Mon, Sep 5, 2022 at 9:55 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 25/08/2022 19:08, Rob Herring wrote:
> > Arm SPEv1.2 (Arm v8.7/v9.2) adds a new feature called Inverted Event
> > Filter which excludes samples matching the event filter. The feature
> > mirrors the existing event filter in PMSEVFR_EL1 adding a new register,
> > PMSNEVFR_EL1, which has the same event bit assignments.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 57904c11aece..9744da888818 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -258,6 +258,7 @@
> >  #define SYS_PMSIDR_EL1_ARCHINST_SHIFT        3
> >  #define SYS_PMSIDR_EL1_LDS_SHIFT     4
> >  #define SYS_PMSIDR_EL1_ERND_SHIFT    5
> > +#define SYS_PMSIDR_EL1_FNE_SHIFT     6
> >  #define SYS_PMSIDR_EL1_INTERVAL_SHIFT        8
> >  #define SYS_PMSIDR_EL1_INTERVAL_MASK 0xfUL
> >  #define SYS_PMSIDR_EL1_MAXSIZE_SHIFT 12
> > @@ -302,6 +303,7 @@
> >  #define SYS_PMSFCR_EL1_FE_SHIFT              0
> >  #define SYS_PMSFCR_EL1_FT_SHIFT              1
> >  #define SYS_PMSFCR_EL1_FL_SHIFT              2
> > +#define SYS_PMSFCR_EL1_FNE_SHIFT     3
> >  #define SYS_PMSFCR_EL1_B_SHIFT               16
> >  #define SYS_PMSFCR_EL1_LD_SHIFT              17
> >  #define SYS_PMSFCR_EL1_ST_SHIFT              18
> > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > index a75b03b5c8f9..724409a88423 100644
> > --- a/drivers/perf/arm_spe_pmu.c
> > +++ b/drivers/perf/arm_spe_pmu.c
> > @@ -82,6 +82,7 @@ struct arm_spe_pmu {
> >  #define SPE_PMU_FEAT_ARCH_INST                       (1UL << 3)
> >  #define SPE_PMU_FEAT_LDS                     (1UL << 4)
> >  #define SPE_PMU_FEAT_ERND                    (1UL << 5)
> > +#define SPE_PMU_FEAT_INV_FILT_EVT            (1UL << 6)
> >  #define SPE_PMU_FEAT_DEV_PROBED                      (1UL << 63)
> >       u64                                     features;
> >
> > @@ -199,6 +200,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
> >  #define ATTR_CFG_FLD_min_latency_LO          0
> >  #define ATTR_CFG_FLD_min_latency_HI          11
> >
> > +#define ATTR_CFG_FLD_inv_event_filter_CFG    config3 /* PMSNEVFR_EL1 */
> > +#define ATTR_CFG_FLD_inv_event_filter_LO     0
> > +#define ATTR_CFG_FLD_inv_event_filter_HI     63
> > +
> >  /* Why does everything I do descend into this? */
> >  #define __GEN_PMU_FORMAT_ATTR(cfg, lo, hi)                           \
> >       (lo) == (hi) ? #cfg ":" #lo "\n" : #cfg ":" #lo "-" #hi
> > @@ -229,6 +234,7 @@ GEN_PMU_FORMAT_ATTR(branch_filter);
> >  GEN_PMU_FORMAT_ATTR(load_filter);
> >  GEN_PMU_FORMAT_ATTR(store_filter);
> >  GEN_PMU_FORMAT_ATTR(event_filter);
> > +GEN_PMU_FORMAT_ATTR(inv_event_filter);
> >  GEN_PMU_FORMAT_ATTR(min_latency);
> >
> >  static struct attribute *arm_spe_pmu_formats_attr[] = {
> > @@ -240,12 +246,27 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
> >       &format_attr_load_filter.attr,
> >       &format_attr_store_filter.attr,
> >       &format_attr_event_filter.attr,
> > +     &format_attr_inv_event_filter.attr,
> >       &format_attr_min_latency.attr,
> >       NULL,
> >  };
> >
> > +static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
> > +                                               struct attribute *attr,
> > +                                               int unused)
> > +     {
> > +     struct device *dev = kobj_to_dev(kobj);
> > +     struct arm_spe_pmu *spe_pmu = dev_get_drvdata(dev);
> > +
> > +     if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> > +             return 0;
> > +
> > +     return attr->mode;
> > +}
> > +
> >  static const struct attribute_group arm_spe_pmu_format_group = {
> >       .name   = "format",
> > +     .is_visible = arm_spe_pmu_format_attr_is_visible,
> >       .attrs  = arm_spe_pmu_formats_attr,
> >  };
> >
> > @@ -341,6 +362,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
> >       if (ATTR_CFG_GET_FLD(attr, event_filter))
> >               reg |= BIT(SYS_PMSFCR_EL1_FE_SHIFT);
> >
> > +     if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
> > +             reg |= BIT(SYS_PMSFCR_EL1_FNE_SHIFT);
> > +
> >       if (ATTR_CFG_GET_FLD(attr, min_latency))
> >               reg |= BIT(SYS_PMSFCR_EL1_FL_SHIFT);
> >
> > @@ -353,6 +377,12 @@ static u64 arm_spe_event_to_pmsevfr(struct perf_event *event)
> >       return ATTR_CFG_GET_FLD(attr, event_filter);
> >  }
> >
> > +static u64 arm_spe_event_to_pmsnevfr(struct perf_event *event)
> > +{
> > +     struct perf_event_attr *attr = &event->attr;
> > +     return ATTR_CFG_GET_FLD(attr, inv_event_filter);
> > +}
> > +
> >  static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
> >  {
> >       struct perf_event_attr *attr = &event->attr;
> > @@ -703,6 +733,9 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> >       if (arm_spe_event_to_pmsevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
> >               return -EOPNOTSUPP;
> >
> > +     if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
> > +             return -EOPNOTSUPP;
> > +
> >       if (attr->exclude_idle)
> >               return -EOPNOTSUPP;
> >
> > @@ -721,6 +754,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> >           !(spe_pmu->features & SPE_PMU_FEAT_FILT_EVT))
> >               return -EOPNOTSUPP;
> >
> > +     if ((reg & BIT(SYS_PMSFCR_EL1_FNE_SHIFT)) &&
> > +         !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> > +             return -EOPNOTSUPP;
> > +
> >       if ((reg & BIT(SYS_PMSFCR_EL1_FT_SHIFT)) &&
> >           !(spe_pmu->features & SPE_PMU_FEAT_FILT_TYP))
> >               return -EOPNOTSUPP;
> > @@ -757,6 +794,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
> >       reg = arm_spe_event_to_pmsevfr(event);
> >       write_sysreg_s(reg, SYS_PMSEVFR_EL1);
> >
> > +     reg = arm_spe_event_to_pmsnevfr(event);
> > +     write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
> > +
>
> I think this needs to check if the feature is present before writing
> otherwise you get a crash, pasted below. Otherwise it looks ok to me.

Yes, that's the 1 fix I've needed in my testing.

Rob

Re: [PATCH RFC v1 3/3] perf: arm_spe: Add support for SPEv1.2 inverted event filtering

[Leo Yan]

Hi Rob,

On Thu, Aug 25, 2022 at 01:08:02PM -0500, Rob Herring wrote:

[...]

> +static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
> +						  struct attribute *attr,
> +						  int unused)
> +	{

Unexpected indentation. Just remind, when I used ./scripts/checkpatch.pl
to check this patch, I can see 1 error and 2 warnings.

As James pointed out the undefined instruction abort, which can be
produced on my Ampere AVA platform (Neoverse N1 CPUs).

Thanks,
Leo