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

[Namhyung Kim <namhyung@kernel.org>]

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
>