diff mbox series

[v2,1/5] perf arm-spe: Define metadata header version 2

Message ID 20240914215458.751802-2-leo.yan@arm.com (mailing list archive)
State New, archived
Headers show
Series perf arm-spe: Introduce metadata version 2 | expand

Commit Message

Leo Yan Sept. 14, 2024, 9:54 p.m. UTC 
The first version's metadata header structure doesn't include a field to
indicate a header version, which is not friendly for extension.

Define the metadata version 2 format with a new header structure and
extend per CPU's metadata. In the meantime, the old metadata header will
still be supported for backward compatibility.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/arch/arm64/util/arm-spe.c |  4 +--
 tools/perf/util/arm-spe.c            |  2 +-
 tools/perf/util/arm-spe.h            | 38 +++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 4 deletions(-)

Comments

Namhyung Kim Sept. 27, 2024, 6:32 a.m. UTC | #1 
On Sat, Sep 14, 2024 at 10:54:54PM +0100, Leo Yan wrote:
> The first version's metadata header structure doesn't include a field to
> indicate a header version, which is not friendly for extension.
> 
> Define the metadata version 2 format with a new header structure and
> extend per CPU's metadata. In the meantime, the old metadata header will
> still be supported for backward compatibility.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/arch/arm64/util/arm-spe.c |  4 +--
>  tools/perf/util/arm-spe.c            |  2 +-
>  tools/perf/util/arm-spe.h            | 38 +++++++++++++++++++++++++++-
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> index 2be99fdf997d..c2d5c8ca4900 100644
> --- a/tools/perf/arch/arm64/util/arm-spe.c
> +++ b/tools/perf/arch/arm64/util/arm-spe.c
> @@ -41,7 +41,7 @@ static size_t
>  arm_spe_info_priv_size(struct auxtrace_record *itr __maybe_unused,
>  		       struct evlist *evlist __maybe_unused)
>  {
> -	return ARM_SPE_AUXTRACE_PRIV_SIZE;
> +	return ARM_SPE_AUXTRACE_V1_PRIV_SIZE;
>  }
>  
>  static int arm_spe_info_fill(struct auxtrace_record *itr,
> @@ -53,7 +53,7 @@ static int arm_spe_info_fill(struct auxtrace_record *itr,
>  			container_of(itr, struct arm_spe_recording, itr);
>  	struct perf_pmu *arm_spe_pmu = sper->arm_spe_pmu;
>  
> -	if (priv_size != ARM_SPE_AUXTRACE_PRIV_SIZE)
> +	if (priv_size != ARM_SPE_AUXTRACE_V1_PRIV_SIZE)
>  		return -EINVAL;
>  
>  	if (!session->evlist->core.nr_mmaps)
> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index 138ffc71b32d..70989b1bae47 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -1262,7 +1262,7 @@ int arm_spe_process_auxtrace_info(union perf_event *event,
>  				  struct perf_session *session)
>  {
>  	struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
> -	size_t min_sz = sizeof(u64) * ARM_SPE_AUXTRACE_PRIV_MAX;
> +	size_t min_sz = ARM_SPE_AUXTRACE_V1_PRIV_SIZE;
>  	struct perf_record_time_conv *tc = &session->time_conv;
>  	const char *cpuid = perf_env__cpuid(session->evlist->env);
>  	u64 midr = strtol(cpuid, NULL, 16);
> diff --git a/tools/perf/util/arm-spe.h b/tools/perf/util/arm-spe.h
> index 4f4900c18f3e..5416d4e1d15f 100644
> --- a/tools/perf/util/arm-spe.h
> +++ b/tools/perf/util/arm-spe.h
> @@ -12,10 +12,46 @@
>  enum {
>  	ARM_SPE_PMU_TYPE,
>  	ARM_SPE_PER_CPU_MMAPS,
> +	ARM_SPE_AUXTRACE_V1_PRIV_MAX,
> +};
> +
> +#define ARM_SPE_AUXTRACE_V1_PRIV_SIZE	\
> +	(ARM_SPE_AUXTRACE_V1_PRIV_MAX * sizeof(u64))
> +
> +enum {
> +	/*
> +	 * The old metadata format (defined above) does not include a
> +	 * field for version number. Version 1 is reserved and starts
> +	 * from version 2.
> +	 */
> +	ARM_SPE_HEADER_VERSION,
> +	/* Number of sizeof(u64) */
> +	ARM_SPE_HEADER_SIZE,
> +	/* PMU type shared by CPUs */
> +	ARM_SPE_SHARED_PMU_TYPE,
> +	/* Number of CPUs */
> +	ARM_SPE_CPUS_NUM,
>  	ARM_SPE_AUXTRACE_PRIV_MAX,
>  };

Why don't you define something like struct arm_spe_header_v2 ?

Thanks,
Namhyung

>  
> -#define ARM_SPE_AUXTRACE_PRIV_SIZE (ARM_SPE_AUXTRACE_PRIV_MAX * sizeof(u64))
> +enum {
> +	/* Magic number */
> +	ARM_SPE_MAGIC,
> +	/* CPU logical number in system */
> +	ARM_SPE_CPU,
> +	/* Number of parameters */
> +	ARM_SPE_CPU_NR_PARAMS,
> +	/* CPU MIDR */
> +	ARM_SPE_CPU_MIDR,
> +	/* Associated PMU type */
> +	ARM_SPE_CPU_PMU_TYPE,
> +	/* Minimal interval */
> +	ARM_SPE_CAP_MIN_IVAL,
> +	ARM_SPE_CPU_PRIV_MAX,
> +};
> +
> +#define ARM_SPE_HEADER_CURRENT_VERSION	2
> +
>  
>  union perf_event;
>  struct perf_session;
> -- 
> 2.34.1
>
Leo Yan Sept. 27, 2024, 8:05 a.m. UTC | #2 
On 9/27/24 07:32, Namhyung Kim wrote:

[...]

>> diff --git a/tools/perf/util/arm-spe.h b/tools/perf/util/arm-spe.h
>> index 4f4900c18f3e..5416d4e1d15f 100644
>> --- a/tools/perf/util/arm-spe.h
>> +++ b/tools/perf/util/arm-spe.h
>> @@ -12,10 +12,46 @@
>>   enum {
>>        ARM_SPE_PMU_TYPE,
>>        ARM_SPE_PER_CPU_MMAPS,
>> +     ARM_SPE_AUXTRACE_V1_PRIV_MAX,
>> +};
>> +
>> +#define ARM_SPE_AUXTRACE_V1_PRIV_SIZE        \
>> +     (ARM_SPE_AUXTRACE_V1_PRIV_MAX * sizeof(u64))
>> +
>> +enum {
>> +     /*
>> +      * The old metadata format (defined above) does not include a
>> +      * field for version number. Version 1 is reserved and starts
>> +      * from version 2.
>> +      */
>> +     ARM_SPE_HEADER_VERSION,
>> +     /* Number of sizeof(u64) */
>> +     ARM_SPE_HEADER_SIZE,
>> +     /* PMU type shared by CPUs */
>> +     ARM_SPE_SHARED_PMU_TYPE,
>> +     /* Number of CPUs */
>> +     ARM_SPE_CPUS_NUM,
>>        ARM_SPE_AUXTRACE_PRIV_MAX,
>>   };
> 
> Why don't you define something like struct arm_spe_header_v2 ?

Here the target is to make metadata to be self-described and can be
easily extended for new version (but I would not expect updating header
will happen frequently).

The fields ARM_SPE_HEADER_VERSION and ARM_SPE_HEADER_SIZE give a
header version and a header size. This allows us to easily extend new
enum items for new header. A benefit is we can use general code for
decoding because we don't need to process structure for a specific
version.

Thanks for review!

Leo
diff mbox series

Patch

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 2be99fdf997d..c2d5c8ca4900 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -41,7 +41,7 @@  static size_t
 arm_spe_info_priv_size(struct auxtrace_record *itr __maybe_unused,
 		       struct evlist *evlist __maybe_unused)
 {
-	return ARM_SPE_AUXTRACE_PRIV_SIZE;
+	return ARM_SPE_AUXTRACE_V1_PRIV_SIZE;
 }
 
 static int arm_spe_info_fill(struct auxtrace_record *itr,
@@ -53,7 +53,7 @@  static int arm_spe_info_fill(struct auxtrace_record *itr,
 			container_of(itr, struct arm_spe_recording, itr);
 	struct perf_pmu *arm_spe_pmu = sper->arm_spe_pmu;
 
-	if (priv_size != ARM_SPE_AUXTRACE_PRIV_SIZE)
+	if (priv_size != ARM_SPE_AUXTRACE_V1_PRIV_SIZE)
 		return -EINVAL;
 
 	if (!session->evlist->core.nr_mmaps)
diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
index 138ffc71b32d..70989b1bae47 100644
--- a/tools/perf/util/arm-spe.c
+++ b/tools/perf/util/arm-spe.c
@@ -1262,7 +1262,7 @@  int arm_spe_process_auxtrace_info(union perf_event *event,
 				  struct perf_session *session)
 {
 	struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
-	size_t min_sz = sizeof(u64) * ARM_SPE_AUXTRACE_PRIV_MAX;
+	size_t min_sz = ARM_SPE_AUXTRACE_V1_PRIV_SIZE;
 	struct perf_record_time_conv *tc = &session->time_conv;
 	const char *cpuid = perf_env__cpuid(session->evlist->env);
 	u64 midr = strtol(cpuid, NULL, 16);
diff --git a/tools/perf/util/arm-spe.h b/tools/perf/util/arm-spe.h
index 4f4900c18f3e..5416d4e1d15f 100644
--- a/tools/perf/util/arm-spe.h
+++ b/tools/perf/util/arm-spe.h
@@ -12,10 +12,46 @@ 
 enum {
 	ARM_SPE_PMU_TYPE,
 	ARM_SPE_PER_CPU_MMAPS,
+	ARM_SPE_AUXTRACE_V1_PRIV_MAX,
+};
+
+#define ARM_SPE_AUXTRACE_V1_PRIV_SIZE	\
+	(ARM_SPE_AUXTRACE_V1_PRIV_MAX * sizeof(u64))
+
+enum {
+	/*
+	 * The old metadata format (defined above) does not include a
+	 * field for version number. Version 1 is reserved and starts
+	 * from version 2.
+	 */
+	ARM_SPE_HEADER_VERSION,
+	/* Number of sizeof(u64) */
+	ARM_SPE_HEADER_SIZE,
+	/* PMU type shared by CPUs */
+	ARM_SPE_SHARED_PMU_TYPE,
+	/* Number of CPUs */
+	ARM_SPE_CPUS_NUM,
 	ARM_SPE_AUXTRACE_PRIV_MAX,
 };
 
-#define ARM_SPE_AUXTRACE_PRIV_SIZE (ARM_SPE_AUXTRACE_PRIV_MAX * sizeof(u64))
+enum {
+	/* Magic number */
+	ARM_SPE_MAGIC,
+	/* CPU logical number in system */
+	ARM_SPE_CPU,
+	/* Number of parameters */
+	ARM_SPE_CPU_NR_PARAMS,
+	/* CPU MIDR */
+	ARM_SPE_CPU_MIDR,
+	/* Associated PMU type */
+	ARM_SPE_CPU_PMU_TYPE,
+	/* Minimal interval */
+	ARM_SPE_CAP_MIN_IVAL,
+	ARM_SPE_CPU_PRIV_MAX,
+};
+
+#define ARM_SPE_HEADER_CURRENT_VERSION	2
+
 
 union perf_event;
 struct perf_session;