[PATCH v3 0/3] perf tool: 'config3' attribute support

[PATCH v3 0/3] perf tool: 'config3' attribute support

[Rob Herring]

This series is a fix to allow unknown 'configN' attrs and then extends 
perf tool to support a new config3 attr.

The config3 support is RFC for context as the kernel side support[1] is 
not yet accepted.

[1] https://lore.kernel.org/all/20220825-arm-spe-v8-7-v1-0-c75b8d92e692@kernel.org/

Signed-off-by: Rob Herring <robh@kernel.org>
---
Rob Herring (3):
      perf: Skip and warn on unknown format 'configN' attrs
      perf tools: Sync perf_event_attr::config3 addition
      perf: Add support for perf_event_attr::config3

 include/uapi/linux/perf_event.h       |  3 +++
 tools/include/uapi/linux/perf_event.h |  3 +++
 tools/perf/util/parse-events.c        |  9 +++++++++
 tools/perf/util/parse-events.h        |  1 +
 tools/perf/util/parse-events.l        |  1 +
 tools/perf/util/pmu.c                 | 16 ++++++++++++++++
 tools/perf/util/pmu.h                 |  3 +++
 tools/perf/util/pmu.l                 |  2 --
 tools/perf/util/pmu.y                 | 15 ++++-----------
 9 files changed, 40 insertions(+), 13 deletions(-)
---
base-commit: 1c23f9e627a7b412978b4e852793c5e3c3efc555
change-id: 20220914-arm-perf-tool-spe1-2-v2-cab789c5d598

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

[PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Rob Herring]

If the kernel exposes a new perf_event_attr field in a format attr, perf
will return an error stating the specified PMU can't be found. For
example, a format attr with 'config3:0-63' causes an error as config3 is
unknown to perf. This causes a compatibility issue between a newer
kernel with older perf tool.

Before this change with a kernel adding 'config3' I get:

$ perf record -e arm_spe// -- true
event syntax error: 'arm_spe//'
                     \___ Cannot find PMU `arm_spe'. Missing kernel support?
Run 'perf list' for a list of valid events

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

    -e, --event <event>   event selector. use 'perf list' to list
available events

After this change, I get:

$ perf record -e arm_spe// -- true
WARNING: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::config3' which is not supported by this version of perf!
[ perf record: Woken up 2 times to write data ]
[ perf record: Captured and wrote 0.091 MB perf.data ]

To support unknown configN formats, rework the YACC implementation to
pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
warning.

Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Move warning from format scanning to when PMU is selected
 - Add and use PERF_PMU_FORMAT_VALUE_CONFIG_END

v2: https://lore.kernel.org/all/20220909204509.2169512-1-robh@kernel.org/
 - Rework YACC code to handle configN formats in C code
 - Add a warning when an unknown configN attr is found

v1: https://lore.kernel.org/all/20220901184709.2179309-1-robh@kernel.org/

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f05e15acd33f..e2305fca0f95 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -215,6 +215,9 @@ __add_event(struct list_head *list, int *idx,
 	struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
 			       cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
 
+	if (pmu)
+		perf_pmu__warn_invalid_formats(pmu);
+
 	if (pmu && attr->type == PERF_TYPE_RAW)
 		perf_pmu__warn_invalid_config(pmu, attr->config, name);
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 89655d53117a..2a0d1415dd55 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1005,6 +1005,19 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
 	return NULL;
 }
 
+void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
+{
+	struct perf_pmu_format *format;
+
+	list_for_each_entry(format, &pmu->format, list)
+		if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
+			pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
+				   "which is not supported by this version of perf!\n",
+				   pmu->name, format->name, format->value);
+			return;
+		}
+}
+
 static struct perf_pmu *pmu_find(const char *name)
 {
 	struct perf_pmu *pmu;
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index a7b0f9507510..68e15c38ae71 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -17,6 +17,7 @@ enum {
 	PERF_PMU_FORMAT_VALUE_CONFIG,
 	PERF_PMU_FORMAT_VALUE_CONFIG1,
 	PERF_PMU_FORMAT_VALUE_CONFIG2,
+	PERF_PMU_FORMAT_VALUE_CONFIG_END,
 };
 
 #define PERF_PMU_FORMAT_BITS 64
@@ -139,6 +140,7 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu);
 
 void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
 				   const char *name);
+void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu);
 
 bool perf_pmu__has_hybrid(void);
 int perf_pmu__match(char *pattern, char *name, char *tok);
diff --git a/tools/perf/util/pmu.l b/tools/perf/util/pmu.l
index a15d9fbd7c0e..58b4926cfaca 100644
--- a/tools/perf/util/pmu.l
+++ b/tools/perf/util/pmu.l
@@ -27,8 +27,6 @@ num_dec         [0-9]+
 
 {num_dec}	{ return value(10); }
 config		{ return PP_CONFIG; }
-config1		{ return PP_CONFIG1; }
-config2		{ return PP_CONFIG2; }
 -		{ return '-'; }
 :		{ return ':'; }
 ,		{ return ','; }
diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
index bfd7e8509869..283efe059819 100644
--- a/tools/perf/util/pmu.y
+++ b/tools/perf/util/pmu.y
@@ -20,7 +20,7 @@ do { \
 
 %}
 
-%token PP_CONFIG PP_CONFIG1 PP_CONFIG2
+%token PP_CONFIG
 %token PP_VALUE PP_ERROR
 %type <num> PP_VALUE
 %type <bits> bit_term
@@ -47,18 +47,11 @@ PP_CONFIG ':' bits
 				      $3));
 }
 |
-PP_CONFIG1 ':' bits
+PP_CONFIG PP_VALUE ':' bits
 {
 	ABORT_ON(perf_pmu__new_format(format, name,
-				      PERF_PMU_FORMAT_VALUE_CONFIG1,
-				      $3));
-}
-|
-PP_CONFIG2 ':' bits
-{
-	ABORT_ON(perf_pmu__new_format(format, name,
-				      PERF_PMU_FORMAT_VALUE_CONFIG2,
-				      $3));
+				      $2,
+				      $4));
 }
 
 bits:

-- 
b4 0.10.0-dev

[PATCH v3 2/3] perf tools: Sync perf_event_attr::config3 addition

[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>
---
This patch is dependent on the kernel side landing first.

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

[PATCH v3 3/3] perf: Add support for perf_event_attr::config3

[Rob Herring]

perf_event_attr has gained a new field, config3, so add support for it
extending the existing configN support.

Signed-off-by: Rob Herring <robh@kernel.org>
---
This patch is dependent on the kernel side landing first.

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index e2305fca0f95..c843434981d0 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -913,6 +913,7 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
 	[PARSE_EVENTS__TERM_TYPE_CONFIG]		= "config",
 	[PARSE_EVENTS__TERM_TYPE_CONFIG1]		= "config1",
 	[PARSE_EVENTS__TERM_TYPE_CONFIG2]		= "config2",
+	[PARSE_EVENTS__TERM_TYPE_CONFIG3]		= "config3",
 	[PARSE_EVENTS__TERM_TYPE_NAME]			= "name",
 	[PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD]		= "period",
 	[PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ]		= "freq",
@@ -952,6 +953,7 @@ config_term_avail(int term_type, struct parse_events_error *err)
 	case PARSE_EVENTS__TERM_TYPE_CONFIG:
 	case PARSE_EVENTS__TERM_TYPE_CONFIG1:
 	case PARSE_EVENTS__TERM_TYPE_CONFIG2:
+	case PARSE_EVENTS__TERM_TYPE_CONFIG3:
 	case PARSE_EVENTS__TERM_TYPE_NAME:
 	case PARSE_EVENTS__TERM_TYPE_METRIC_ID:
 	case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
@@ -997,6 +999,10 @@ do {									   \
 		CHECK_TYPE_VAL(NUM);
 		attr->config2 = term->val.num;
 		break;
+	case PARSE_EVENTS__TERM_TYPE_CONFIG3:
+		CHECK_TYPE_VAL(NUM);
+		attr->config3 = term->val.num;
+		break;
 	case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
 		CHECK_TYPE_VAL(NUM);
 		break;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 7e6a601d9cd0..fa9460900abf 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -59,6 +59,7 @@ enum {
 	PARSE_EVENTS__TERM_TYPE_CONFIG,
 	PARSE_EVENTS__TERM_TYPE_CONFIG1,
 	PARSE_EVENTS__TERM_TYPE_CONFIG2,
+	PARSE_EVENTS__TERM_TYPE_CONFIG3,
 	PARSE_EVENTS__TERM_TYPE_NAME,
 	PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD,
 	PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 3a9ce96c8bce..51fe0a9fb3de 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -285,6 +285,7 @@ modifier_bp	[rwx]{1,3}
 config			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CONFIG); }
 config1			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CONFIG1); }
 config2			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CONFIG2); }
+config3			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CONFIG3); }
 name			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NAME); }
 period			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD); }
 freq			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ); }
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 2a0d1415dd55..6943a11b822c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1274,6 +1274,9 @@ static int pmu_config_term(const char *pmu_name,
 	case PERF_PMU_FORMAT_VALUE_CONFIG2:
 		vp = &attr->config2;
 		break;
+	case PERF_PMU_FORMAT_VALUE_CONFIG3:
+		vp = &attr->config3;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 68e15c38ae71..2e6bd1bf304a 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -17,6 +17,7 @@ enum {
 	PERF_PMU_FORMAT_VALUE_CONFIG,
 	PERF_PMU_FORMAT_VALUE_CONFIG1,
 	PERF_PMU_FORMAT_VALUE_CONFIG2,
+	PERF_PMU_FORMAT_VALUE_CONFIG3,
 	PERF_PMU_FORMAT_VALUE_CONFIG_END,
 };
 

-- 
b4 0.10.0-dev

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Namhyung Kim]

On Wed, Sep 14, 2022 at 1:09 PM Rob Herring <robh@kernel.org> wrote:
>
> If the kernel exposes a new perf_event_attr field in a format attr, perf
> will return an error stating the specified PMU can't be found. For
> example, a format attr with 'config3:0-63' causes an error as config3 is
> unknown to perf. This causes a compatibility issue between a newer
> kernel with older perf tool.
>
> Before this change with a kernel adding 'config3' I get:
>
> $ perf record -e arm_spe// -- true
> event syntax error: 'arm_spe//'
>                      \___ Cannot find PMU `arm_spe'. Missing kernel support?
> Run 'perf list' for a list of valid events
>
>  Usage: perf record [<options>] [<command>]
>     or: perf record [<options>] -- <command> [<options>]
>
>     -e, --event <event>   event selector. use 'perf list' to list
> available events
>
> After this change, I get:
>
> $ perf record -e arm_spe// -- true
> WARNING: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::config3' which is not supported by this version of perf!
> [ perf record: Woken up 2 times to write data ]
> [ perf record: Captured and wrote 0.091 MB perf.data ]
>
> To support unknown configN formats, rework the YACC implementation to
> pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> warning.

It only handles configN formats but it might add a completely different
name later, right?

Thanks,
Namhyung

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Rob Herring]

On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Sep 14, 2022 at 1:09 PM Rob Herring <robh@kernel.org> wrote:
> >
> > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > will return an error stating the specified PMU can't be found. For
> > example, a format attr with 'config3:0-63' causes an error as config3 is
> > unknown to perf. This causes a compatibility issue between a newer
> > kernel with older perf tool.
> >
> > Before this change with a kernel adding 'config3' I get:
> >
> > $ perf record -e arm_spe// -- true
> > event syntax error: 'arm_spe//'
> >                      \___ Cannot find PMU `arm_spe'. Missing kernel support?
> > Run 'perf list' for a list of valid events
> >
> >  Usage: perf record [<options>] [<command>]
> >     or: perf record [<options>] -- <command> [<options>]
> >
> >     -e, --event <event>   event selector. use 'perf list' to list
> > available events
> >
> > After this change, I get:
> >
> > $ perf record -e arm_spe// -- true
> > WARNING: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::config3' which is not supported by this version of perf!
> > [ perf record: Woken up 2 times to write data ]
> > [ perf record: Captured and wrote 0.091 MB perf.data ]
> >
> > To support unknown configN formats, rework the YACC implementation to
> > pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> > warning.
>
> It only handles configN formats but it might add a completely different
> name later, right?

Right. An unknown configN is a warning. An unknown name is still an
error as before. Given that sysfs format attrs are for mapping fields
which could be anything to "generic" perf_event_attr fields, how would
we ever have anything other than configN?

Rob

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Leo Yan]

Hi Rob,

On Wed, Sep 14, 2022 at 03:08:34PM -0500, Rob Herring wrote:

[...]

> +void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
> +{
> +	struct perf_pmu_format *format;
> +
> +	list_for_each_entry(format, &pmu->format, list)
> +		if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
> +			pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
> +				   "which is not supported by this version of perf!\n",
> +				   pmu->name, format->name, format->value);
> +			return;
> +		}
> +}

Though I saw you and Namhyung have discussion in underway, this patch
set is fine for me.  I validated the patches at my side (with a bit
hacking in Arm SPE driver for faking invert filter).  You could add my
tested tag for this patch set:

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

But I want to remind two things after I used "perf test" to validate
this patch set:

  $ ./perf test list
  6: Parse event definition strings
  6:1: Test event parsing
  6:2: Test parsing of "hybrid" CPU events
  6:3: Parsing of all PMU events from sysfs
  6:4: Parsing of given PMU events from sysfs
  6:5: Parsing of aliased events from sysfs
  6:6: Parsing of aliased events
  6:7: Parsing of terms (event modifiers)
  $ ./perf test -v 6

The first one is this patch set introduces segmentation fault for the
case "Parsing of aliased events" (See tests/parse-events.c).  But the
issue is caused by the test case itself; we need to add below line into
test_event_fake_pmu() for initialisation list header.

    INIT_LIST_HEAD(&perf_pmu__fake.format);

The second question is for testing config3 in "perf test".  You could
see the file tests/parse-events.c has included several test cases for
config/config1/config2.  It's good to add the same testing for config3
as well, please see test__checkevent_pmu() and test__checkterms_simple()
for relevant code.

Thanks,
Leo

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Namhyung Kim]

On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, Sep 14, 2022 at 1:09 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > > will return an error stating the specified PMU can't be found. For
> > > example, a format attr with 'config3:0-63' causes an error as config3 is
> > > unknown to perf. This causes a compatibility issue between a newer
> > > kernel with older perf tool.
> > >
> > > Before this change with a kernel adding 'config3' I get:
> > >
> > > $ perf record -e arm_spe// -- true
> > > event syntax error: 'arm_spe//'
> > >                      \___ Cannot find PMU `arm_spe'. Missing kernel support?
> > > Run 'perf list' for a list of valid events
> > >
> > >  Usage: perf record [<options>] [<command>]
> > >     or: perf record [<options>] -- <command> [<options>]
> > >
> > >     -e, --event <event>   event selector. use 'perf list' to list
> > > available events
> > >
> > > After this change, I get:
> > >
> > > $ perf record -e arm_spe// -- true
> > > WARNING: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::config3' which is not supported by this version of perf!
> > > [ perf record: Woken up 2 times to write data ]
> > > [ perf record: Captured and wrote 0.091 MB perf.data ]
> > >
> > > To support unknown configN formats, rework the YACC implementation to
> > > pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> > > warning.
> >
> > It only handles configN formats but it might add a completely different
> > name later, right?
>
> Right. An unknown configN is a warning. An unknown name is still an
> error as before. Given that sysfs format attrs are for mapping fields
> which could be anything to "generic" perf_event_attr fields, how would
> we ever have anything other than configN?

I'm not sure I'm following.  It could be anything other than configN.

But I don't object to this particular change as it's needed for current
work.  Later, we can fix the issue if another name comes in.

Thanks,
Namhyung

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Rob Herring]

On Thu, Sep 29, 2022 at 1:54 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Wed, Sep 14, 2022 at 1:09 PM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > > > will return an error stating the specified PMU can't be found. For
> > > > example, a format attr with 'config3:0-63' causes an error as config3 is
> > > > unknown to perf. This causes a compatibility issue between a newer
> > > > kernel with older perf tool.
> > > >
> > > > Before this change with a kernel adding 'config3' I get:
> > > >
> > > > $ perf record -e arm_spe// -- true
> > > > event syntax error: 'arm_spe//'
> > > >                      \___ Cannot find PMU `arm_spe'. Missing kernel support?
> > > > Run 'perf list' for a list of valid events
> > > >
> > > >  Usage: perf record [<options>] [<command>]
> > > >     or: perf record [<options>] -- <command> [<options>]
> > > >
> > > >     -e, --event <event>   event selector. use 'perf list' to list
> > > > available events
> > > >
> > > > After this change, I get:
> > > >
> > > > $ perf record -e arm_spe// -- true
> > > > WARNING: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::config3' which is not supported by this version of perf!
> > > > [ perf record: Woken up 2 times to write data ]
> > > > [ perf record: Captured and wrote 0.091 MB perf.data ]
> > > >
> > > > To support unknown configN formats, rework the YACC implementation to
> > > > pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> > > > warning.
> > >
> > > It only handles configN formats but it might add a completely different
> > > name later, right?
> >
> > Right. An unknown configN is a warning. An unknown name is still an
> > error as before. Given that sysfs format attrs are for mapping fields
> > which could be anything to "generic" perf_event_attr fields, how would
> > we ever have anything other than configN?
>
> I'm not sure I'm following.  It could be anything other than configN.

It's possible, yes, but likely or necessary? Probably not.

Let me try again. perf_event_attr:configX fields are pmu specific and
sysfs format files provide the mapping of their specific usage to
configX bits. If we add something to perf_event_attr that's not PMU
specific, but common for perf, then it's going to have a specific name
and no format entry. Right? If we add yet another PMU specific field,
why would we ever have a name other than 'config'? Anything different
has little benefit since format files provide the specific meaning and
it's up to the PMU driver to handle them.

Maybe someone comes up with something, but that seems a lot less
likely than another configN.

> But I don't object to this particular change as it's needed for current
> work.  Later, we can fix the issue if another name comes in.

Is that an Acked/Reviewed-by?

Rob

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Namhyung Kim]

On Thu, Sep 29, 2022 at 1:55 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Sep 29, 2022 at 1:54 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > On Wed, Sep 14, 2022 at 1:09 PM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > > > > will return an error stating the specified PMU can't be found. For
> > > > > example, a format attr with 'config3:0-63' causes an error as config3 is
> > > > > unknown to perf. This causes a compatibility issue between a newer
> > > > > kernel with older perf tool.
> > > > >
> > > > > Before this change with a kernel adding 'config3' I get:
> > > > >
> > > > > $ perf record -e arm_spe// -- true
> > > > > event syntax error: 'arm_spe//'
> > > > >                      \___ Cannot find PMU `arm_spe'. Missing kernel support?
> > > > > Run 'perf list' for a list of valid events
> > > > >
> > > > >  Usage: perf record [<options>] [<command>]
> > > > >     or: perf record [<options>] -- <command> [<options>]
> > > > >
> > > > >     -e, --event <event>   event selector. use 'perf list' to list
> > > > > available events
> > > > >
> > > > > After this change, I get:
> > > > >
> > > > > $ perf record -e arm_spe// -- true
> > > > > WARNING: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::config3' which is not supported by this version of perf!
> > > > > [ perf record: Woken up 2 times to write data ]
> > > > > [ perf record: Captured and wrote 0.091 MB perf.data ]
> > > > >
> > > > > To support unknown configN formats, rework the YACC implementation to
> > > > > pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> > > > > warning.
> > > >
> > > > It only handles configN formats but it might add a completely different
> > > > name later, right?
> > >
> > > Right. An unknown configN is a warning. An unknown name is still an
> > > error as before. Given that sysfs format attrs are for mapping fields
> > > which could be anything to "generic" perf_event_attr fields, how would
> > > we ever have anything other than configN?
> >
> > I'm not sure I'm following.  It could be anything other than configN.
>
> It's possible, yes, but likely or necessary? Probably not.
>
> Let me try again. perf_event_attr:configX fields are pmu specific and
> sysfs format files provide the mapping of their specific usage to
> configX bits. If we add something to perf_event_attr that's not PMU
> specific, but common for perf, then it's going to have a specific name
> and no format entry. Right? If we add yet another PMU specific field,
> why would we ever have a name other than 'config'? Anything different
> has little benefit since format files provide the specific meaning and
> it's up to the PMU driver to handle them.
>
> Maybe someone comes up with something, but that seems a lot less
> likely than another configN.

Fair enough.

>
> > But I don't object to this particular change as it's needed for current
> > work.  Later, we can fix the issue if another name comes in.
>
> Is that an Acked/Reviewed-by?

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Rob Herring]

On Thu, Sep 29, 2022 at 1:48 AM Leo Yan <leo.yan@linaro.org> wrote:
>
> Hi Rob,
>
> On Wed, Sep 14, 2022 at 03:08:34PM -0500, Rob Herring wrote:
>
> [...]
>
> > +void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
> > +{
> > +     struct perf_pmu_format *format;
> > +
> > +     list_for_each_entry(format, &pmu->format, list)
> > +             if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
> > +                     pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
> > +                                "which is not supported by this version of perf!\n",
> > +                                pmu->name, format->name, format->value);
> > +                     return;
> > +             }
> > +}
>
> Though I saw you and Namhyung have discussion in underway, this patch
> set is fine for me.  I validated the patches at my side (with a bit
> hacking in Arm SPE driver for faking invert filter).  You could add my
> tested tag for this patch set:
>
> Tested-by: Leo Yan <leo.yan@linaro.org>
>
> But I want to remind two things after I used "perf test" to validate
> this patch set:
>
>   $ ./perf test list
>   6: Parse event definition strings
>   6:1: Test event parsing
>   6:2: Test parsing of "hybrid" CPU events
>   6:3: Parsing of all PMU events from sysfs
>   6:4: Parsing of given PMU events from sysfs
>   6:5: Parsing of aliased events from sysfs
>   6:6: Parsing of aliased events
>   6:7: Parsing of terms (event modifiers)
>   $ ./perf test -v 6
>
> The first one is this patch set introduces segmentation fault for the
> case "Parsing of aliased events" (See tests/parse-events.c).  But the
> issue is caused by the test case itself; we need to add below line into
> test_event_fake_pmu() for initialisation list header.

Thanks.

>     INIT_LIST_HEAD(&perf_pmu__fake.format);

I ended up fixing this in perf_pmu__warn_invalid_formats() instead as
the test dealing with internal stuct pmu details didn't seem right:

+       /* fake pmu doesn't have format list */
+       if (pmu == &perf_pmu__fake)
+               return;
+

>
> The second question is for testing config3 in "perf test".  You could
> see the file tests/parse-events.c has included several test cases for
> config/config1/config2.  It's good to add the same testing for config3
> as well, please see test__checkevent_pmu() and test__checkterms_simple()
> for relevant code.

Okay, will add in the next version.

Rob

Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Leo Yan]

On Tue, Oct 04, 2022 at 12:07:30PM -0500, Rob Herring wrote:

[...]

> >     INIT_LIST_HEAD(&perf_pmu__fake.format);
> 
> I ended up fixing this in perf_pmu__warn_invalid_formats() instead as
> the test dealing with internal stuct pmu details didn't seem right:
> 
> +       /* fake pmu doesn't have format list */
> +       if (pmu == &perf_pmu__fake)
> +               return;
> +

Good point.  It would be even better to fix it in the first place
rather than checking fake PMU in perf_pmu__warn_invalid_formats(),
how about below fixing in util/pmu.c?

-struct perf_pmu perf_pmu__fake;
+struct perf_pmu perf_pmu__fake = {
+       .format = LIST_HEAD_INIT(perf_pmu__fake.format),
+};

Thanks,
Leo