Re: [PATCH 2/2] perf arm-spe: Parse more SPE fields and store source

[Ali Saidi <alisaidi@amazon.com>]

Hi German,

On 28/01/2022 19:20, German Gomez wrote:
>Hi Ali,
>
>On 25/01/2022 19:20, Ali Saidi wrote:
>> Decode more SPE events and op types to allow for processing by perf
>> scripts. For example looking for branches which may indicate candidates
>> for conversion to a CSEL, store exclusives that are candidates for
>> conversion to LSE atomics and record the source information for memory
>> ops.
>> [snip]
>> +				if (SPE_OP_PKT_IS_LDST_ATOMIC(payload)) {
>> +					if (payload & SPE_OP_PKT_AT)
>> +						decoder->record.op |= ARM_SPE_LDST_ATOMIC;
>
>In "utils/arm-spe.c" we check "if (record->op == ARM_SPE_LD)" so this
>ORing could break some of the generated samples.

Yep, you're correct. Interestingly I can only find one use of record->op using
equivalence instead of a logical and so perhaps it's best to fix this one use.

...
>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
>> index 69b31084d6be..113e427afe99 100644
>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
>> @@ -22,11 +22,18 @@ enum arm_spe_sample_type {
>>  	ARM_SPE_TLB_MISS	= 1 << 5,
>>  	ARM_SPE_BRANCH_MISS	= 1 << 6,
>>  	ARM_SPE_REMOTE_ACCESS	= 1 << 7,
>> +	ARM_SPE_BR_NOT_TAKEN	= 1 << 8,
>
>Can you rename it to ARM_SPE_BRANCH_NOT_TAKEN for consistency?

No problem

>
>>  };
>>  
>>  enum arm_spe_op_type {
>>  	ARM_SPE_LD		= 1 << 0,
>>  	ARM_SPE_ST		= 1 << 1,
>> +	ARM_SPE_LDST_EXCL	= 1 << 2,
>> +	ARM_SPE_LDST_ATOMIC	= 1 << 3,
>> +	ARM_SPE_LDST_ACQREL	= 1 << 4,
>> +	ARM_SPE_BR		= 1 << 5,
>> +	ARM_SPE_BR_COND		= 1 << 6,
>> +	ARM_SPE_BR_IND		= 1 << 7,
>
>I'm not sure if we should keep everything in one enum/bitmask. I'm also
>looking at adding more of the data from the packets to the record, and
>considering refactoring the record structure. I'll share here when I
>have something.

One straight forward way to do this would be to make it a u16 field that was 
SPE operation-type header and operation-type payload with some accessors instead
of trying to re-encode the operation type into a new format.

Thanks,
Ali