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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f14f9dc5-7a82-4ec3-8acc-53a0c7b5a2b6@arm.com>
Date: Fri, 30 Aug 2024 08:54:42 +0100
From: Leo Yan <leo.yan@....com>
To: James Clark <james.clark@...aro.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Will Deacon
 <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
 <mike.leach@...aro.org>, John Garry <john.g.garry@...cle.com>,
 Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>,
 "Liang, Kan" <kan.liang@...ux.intel.com>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 Yicong Yang <yangyicong@...ilicon.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 coresight@...ts.linaro.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v1 9/9] perf arm-spe: Dump metadata with version 2

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