[PATCH v4 2/4] x86/mce, EDAC/mce_amd: Add support for new MCA_SYND{1,2} registers

[Avadhut Naik <avadnaik@amd.com>]

On 8/21/24 12:40, Steven Rostedt wrote:
> On Thu, 15 Aug 2024 16:16:33 -0500
> Avadhut Naik <avadhut.naik@amd.com> wrote:
> 
>> diff --git a/include/trace/events/mce.h b/include/trace/events/mce.h
>> index 65aba1afcd07..1e7d5696b3ba 100644
>> --- a/include/trace/events/mce.h
>> +++ b/include/trace/events/mce.h
>> @@ -43,6 +43,8 @@ TRACE_EVENT(mce_record,
>>  		__field(	u8,		bank		)
>>  		__field(	u8,		cpuvendor	)
>>  		__field(	u32,		microcode	)
>> +		__field(	u8,		len	)
> 
> You don't need to save the length. It's already saved in the
> __dynamic_array meta data.
> 
That helps! Will remove the len field.

>> +		__dynamic_array(u8, v_data, sizeof(err->vendor))
>>  	),
>>  
>>  	TP_fast_assign(
>> @@ -65,9 +67,11 @@ TRACE_EVENT(mce_record,
>>  		__entry->bank		= err->m.bank;
>>  		__entry->cpuvendor	= err->m.cpuvendor;
>>  		__entry->microcode	= err->m.microcode;
>> +		__entry->len		= sizeof(err->vendor);
>> +		memcpy(__get_dynamic_array(v_data), &err->vendor, sizeof(err->vendor));
>>  	),
>>  
>> -	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR: %016Lx, MISC: %016Lx, SYND: %016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PPIN: %llx, vendor: %u, CPUID: %x, time: %llu, socket: %u, APIC: %x, microcode: %x",
>> +	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016llx, IPID: %016llx, ADDR: %016llx, MISC: %016llx, SYND: %016llx, RIP: %02x:<%016llx>, TSC: %llx, PPIN: %llx, vendor: %u, CPUID: %x, time: %llu, socket: %u, APIC: %x, microcode: %x, vendor data: %s",
>>  		__entry->cpu,
>>  		__entry->mcgcap, __entry->mcgstatus,
>>  		__entry->bank, __entry->status,
>> @@ -83,7 +87,8 @@ TRACE_EVENT(mce_record,
>>  		__entry->walltime,
>>  		__entry->socketid,
>>  		__entry->apicid,
>> -		__entry->microcode)
>> +		__entry->microcode,
>> +		__print_array(__get_dynamic_array(v_data), __entry->len / 8, 8))
> 
> You can replace the __entry->len with:
> 
> 		__print_array(__get_dynamic_array(v_data), __get_dynamic_array_len(v_data) / 8, 8))
> 
> Hmm, I should add a helper function that would do the same with just:
> 
> 		__print_dynamic_array(vdata, 8);
> 
> Where it will have __print_dynamic_array(array-name, el-size)
> 
> -- Steve
> 
Thanks for the patch! Will incorporate it this set.

I did, however, have another question. This was actually raised in an earlier
version of this set by Boris and Tony. Not very sure about the answer though.

Is there a way through which we can make the tracepoints smarter i.e. have
conditional checks in them?

To provide some more context, as of now, the v_data dynamic array above will
contain vendor-specific data only on AMD systems. On Intel systems, IIUC,
its contents will be 0x0.

Consequently, the tracepoint output on Intel systems will have some thing like
"0x0,0x0,0x0" at the end.

Is there some way through which we can work around this? Can we perform a
vendor check inside the tracepoint so that the above dynamic array is logged
only on AMD systems and not on Intel systems?

-- 
Thanks,
Avadhut Naik