Re: [PATCH v2 2/2] tracing: Include Microcode Revision in mce_record tracepoint

[Naik, Avadhut]

On 1/25/2024 3:03 PM, Sohil Mehta wrote:
> On 1/25/2024 10:48 AM, Avadhut Naik wrote:
>> Currently, the microcode field (Microcode Revision) of struct mce is not
>> exported to userspace through the mce_record tracepoint.
>>
>> Export it through the tracepoint as it may provide useful information for
>> debug and analysis.
>>
>> Signed-off-by: Avadhut Naik <avadhut.naik@xxxxxxx>
>> ---
> 
> A couple of nits below.
> 
> Apart from that the patch looks fine to me.
> 
> Reviewed-by: Sohil Mehta <sohil.mehta@xxxxxxxxx>
> 
>>  include/trace/events/mce.h | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/trace/events/mce.h b/include/trace/events/mce.h
>> index 657b93ec8176..203baccd3c5c 100644
>> --- a/include/trace/events/mce.h
>> +++ b/include/trace/events/mce.h
>> @@ -34,6 +34,7 @@ TRACE_EVENT(mce_record,
>>  		__field(	u8,		cs		)
>>  		__field(	u8,		bank		)
>>  		__field(	u8,		cpuvendor	)
>> +		__field(	u32,	microcode	)
> 
> Tab alignment is inconsistent.
> 
>>  	),
>>  
>>  	TP_fast_assign(
>> @@ -55,9 +56,10 @@ TRACE_EVENT(mce_record,
>>  		__entry->cs		= m->cs;
>>  		__entry->bank		= m->bank;
>>  		__entry->cpuvendor	= m->cpuvendor;
>> +		__entry->microcode	= m->microcode;
>>  	),
>>  
>> -	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PPIN: %llx, PROCESSOR: %u:%x, TIME: %llu, SOCKET: %u, APIC: %x",
>> +	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PPIN: %llx, PROCESSOR: %u:%x, TIME: %llu, SOCKET: %u, APIC: %x, MICROCODE REVISION: %u",
> 
> Should microcode by printed as a decimal or an hexadecimal? Elsewhere
> such as __print_mce(), it is printed as an hexadecimal:
> 
>         /*
>          * Note this output is parsed by external tools and old fields
>          * should not be changed.
>          */
>         pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x
> microcode %x\n",
>                 m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid,
>                 m->microcode);
> 
> 
Had kept the field as decimal since I considered that version should be a decimal
number instead of a hex value. Hadn't noticed the above log in core.c file.
Thanks for pointing it out.

Since we now have precedent that microcode version values are being reported in hex,
will change the above format specifier to %x.
Will just submit a new version, addressing the tab alignments too.
> 
> 
>>  		__entry->cpu,
>>  		__entry->mcgcap, __entry->mcgstatus,
>>  		__entry->bank, __entry->status,
>> @@ -69,7 +71,8 @@ TRACE_EVENT(mce_record,
>>  		__entry->cpuvendor, __entry->cpuid,
>>  		__entry->walltime,
>>  		__entry->socketid,
>> -		__entry->apicid)
>> +		__entry->apicid,
>> +		__entry->microcode)
>>  );
>>  
>>  #endif /* _TRACE_MCE_H */
> 

-- 
Thanks,
Avadhut Naik