linux-kernel - Re: [PATCH v2 2/2] tracing: Include Microcode Revision in mce

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]

Date: Thu, 25 Jan 2024 19:35:14 -0600
From: "Naik, Avadhut" <avadnaik@....com>
To: Sohil Mehta <sohil.mehta@...el.com>, linux-trace-kernel@...r.kernel.org,
 linux-edac@...r.kernel.org
Cc: rostedt@...dmis.org, tony.luck@...el.com, bp@...en8.de, x86@...nel.org,
 linux-kernel@...r.kernel.org, yazen.ghannam@....com,
 Avadhut Naik <avadhut.naik@....com>
Subject: Re: [PATCH v2 2/2] tracing: Include Microcode Revision in mce_record
 tracepoint



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@....com>
>> ---
> 
> A couple of nits below.
> 
> Apart from that the patch looks fine to me.
> 
> Reviewed-by: Sohil Mehta <sohil.mehta@...el.com>
> 
>>  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