Re: [PATCH v1 9/9] perf arm-spe: Dump metadata with version 2

[Leo Yan <leo.yan@arm.com>]

On 8/28/24 17:20, James Clark wrote:
>> +static void arm_spe_print_info(struct arm_spe *spe, __u64 *arr)
>>   {
>> +     unsigned int i, cpu, header_size, cpu_num, per_cpu_size;
>> +
>>       if (!dump_trace)
>>               return;
>>
>> -     fprintf(stdout, arm_spe_info_fmts[ARM_SPE_PMU_TYPE], arr[ARM_SPE_PMU_TYPE]);
>> +     if (spe->metadata_ver == 1) {
>> +             cpu_num = 0;
>> +             header_size = ARM_SPE_AUXTRACE_V1_PRIV_MAX;
>> +             per_cpu_size = 0;
>> +     } else if (spe->metadata_ver == 2) {
> 
> Assuming future version updates are backwards compatible and only add
> new info this should be spe->metadata_ver >= 2, otherwise version bumps
> end up causing errors when files get passed around.
> 
> I know there are arguments about what should and shouldn't be supported
> when opening new files on old perfs, but in this case it's easy to only
> add new info to the aux header and leave the old stuff intact.
> 
>> +             cpu_num = arr[ARM_SPE_CPU_NUM];
>> +             header_size = ARM_SPE_AUXTRACE_V2_PRIV_MAX;
>> +             per_cpu_size = ARM_SPE_AUXTRACE_V2_PRIV_PER_CPU_MAX;
> 
> I think for coresight we also save the size of each per-cpu block rather
> than use a constant, that way new items can be appended without breaking
> readers.

Good point for adding a 'size' field for each per-cpu block.

My understanding is we need to make the metadata format to be self-described.
E.g. the metadata header contains the size for itself, and every per CPU
metadata block also contains a 'size' field.  Based on this, a general code
can be used to processing different metadata versions.
  
> That kind of leads to another point that this mechanism is mostly
> duplicated from coresight. It saves a main header version, then per-cpu
> groups of variable size with named elements. I'm not saying we should
> definitely try to share the code, but it's worth keeping in mind.

Agreed. I will refine a bit, for better matching this direction.

Thanks for suggestion.

Leo