[PATCH v3 4/5] perf arm-spe: Support metadata version 2

[Leo Yan]

On 10/1/2024 2:53 PM, James Clark wrote:

[...]

>> @@ -1267,15 +1339,24 @@ int arm_spe_process_auxtrace_info(union perf_event
>> *event,
>>       const char *cpuid = perf_env__cpuid(session->evlist->env);
>>       u64 midr = strtol(cpuid, NULL, 16);
>>       struct arm_spe *spe;
>> -     int err;
>> +     u64 **metadata = NULL;
>> +     u64 metadata_ver;
>> +     int nr_cpu, err;
>>
>>       if (auxtrace_info->header.size < sizeof(struct
>> perf_record_auxtrace_info) +
>>                                       min_sz)
>>               return -EINVAL;
>>
>> +     metadata = arm_spe__alloc_metadata(auxtrace_info, &metadata_ver,
>> +                                        &nr_cpu);
>> +     if (!metadata && metadata_ver != 1) {
>> +             pr_err("Failed to parse Arm SPE metadata.\n");
>> +             return -EINVAL;
>> +     }
>> +
>>       spe = zalloc(sizeof(struct arm_spe));
>>       if (!spe)
>> -             return -ENOMEM;
>> +             goto err_free_metadata;
>>
> 
> Hi Leo,
> 
> There's a build error here:
> 
> util/arm-spe.c:1402:6: error: variable 'err' is used uninitialized
> whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>         if (!spe)
>             ^~~~

Thanks for catching this issue.

It is interesting that Aarch64 cross compiler GCC-13 and GCC-14 both do not
report this issue, I reproduced the error until I changed to x86_64 GCC-11.

>>       err = auxtrace_queues__init(&spe->queues);
>>       if (err)
>> @@ -1284,8 +1365,14 @@ int arm_spe_process_auxtrace_info(union perf_event
>> *event,
>>       spe->session = session;
>>       spe->machine = &session->machines.host; /* No kvm support */
>>       spe->auxtrace_type = auxtrace_info->type;
>> -     spe->pmu_type = auxtrace_info->priv[ARM_SPE_PMU_TYPE];
>> +     if (metadata_ver == 1)
>> +             spe->pmu_type = auxtrace_info->priv[ARM_SPE_PMU_TYPE];
>> +     else
>> +             spe->pmu_type = auxtrace_info->priv[ARM_SPE_SHARED_PMU_TYPE];
> 
> I thought V2 saved the type per-CPU, so I wasn't that clear on the
> SHARED_PMU_TYPE vs ARM_SPE_PMU_TYPE use here. Maybe it's just a naming
> issue? If it's a placeholder value to generate events can it be the same
> name as the V1 enum, that way it's clearer that it's the same thing.
> 
> Like: ARM_SPE_PMU_TYPE_V2

Yes, in the end we will use the PMU type saved in per-CPU metadata which will
be updated in the multi AUX event patch series. Before we reach this point,
let us keep a PMU type in the metadata header.

I will rename to ARM_SPE_PMU_TYPE_V2.

Thanks for review.

Leo

> Other than that, for the whole set:
> 
> Reviewed-by: James Clark <james.clark at linaro.org>
>