Re: [PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
On Thu, Mar 28, 2024 at 01:17:43AM -0500, Naik, Avadhut wrote: > SOCKET -> Socket > PROCESSOR -> Processor > MICROCODE -> Microcode SOCKET -> socket PROCESSOR -> processor MICROCODE -> microcode And yeah, the acronyms need to obviously stay in all caps. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
[PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
On 3/27/2024 17:35, Borislav Petkov wrote:
> On Wed, Mar 27, 2024 at 03:31:01PM -0700, Sohil Mehta wrote:
>> On 3/27/2024 1:54 PM, Avadhut Naik wrote:
>>
>>> - 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: %x",
>>
>> Nit: s/MICROCODE REVISION/MICROCODE/g
>>
>> You could probably get rid of the word REVISION in the interest of
>> brevity similar to __print_mce().
>
> You *definitely* want to do that - good catch.
>
Will do.
> And TBH, all the screaming words aren't helping either... :)
>
Are you suggesting to change the ALL CAPS format of words which are
not acronyms to normal Capitalization style? Like Sohit suggested
in his other mail on this thread?
Somewhat like below:
SOCKET -> Socket
PROCESSOR -> Processor
MICROCODE -> Microcode
--
Thanks,
Avadhut Naik
Re: [PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
On 3/27/2024 17:31, Sohil Mehta wrote:
> On 3/27/2024 1:54 PM, Avadhut Naik wrote:
>
>> -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: %x",
>
> Nit: s/MICROCODE REVISION/MICROCODE/g
>
> You could probably get rid of the word REVISION in the interest of
> brevity similar to __print_mce().
>
> 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);
>
>
Okay. Will remove "REVISION".
> -Sohil
--
Thanks,
Avadhut Naik
Re: [PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
> > You *definitely* want to do that - good catch. > > And TBH, all the screaming words aren't helping either... :) > :) I thought the same as well. But, I felt inconsistently screaming words might be worse. Maybe just update all the words that are not acronyms (such as Processor, Time, Socket, etc.)
Re: [PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
On Wed, Mar 27, 2024 at 03:31:01PM -0700, Sohil Mehta wrote:
> On 3/27/2024 1:54 PM, Avadhut Naik wrote:
>
> > - 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: %x",
>
> Nit: s/MICROCODE REVISION/MICROCODE/g
>
> You could probably get rid of the word REVISION in the interest of
> brevity similar to __print_mce().
You *definitely* want to do that - good catch.
And TBH, all the screaming words aren't helping either... :)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
On 3/27/2024 1:54 PM, Avadhut Naik wrote:
> - 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:
> %x",
Nit: s/MICROCODE REVISION/MICROCODE/g
You could probably get rid of the word REVISION in the interest of
brevity similar to __print_mce().
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);
-Sohil
RE: [PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
> Export microcode version through the tracepoint to prevent ambiguity over > the active version on the system when the MCE was received. > > Signed-off-by: Avadhut Naik > Reviewed-by: Sohil Mehta > Reviewed-by: Steven Rostedt (Google) Reviewed-by: Tony Luck
[PATCH v4 2/2] tracing: Include Microcode Revision in mce_record tracepoint
Currently, the microcode field (Microcode Revision) of struct mce is not
exported to userspace through the mce_record tracepoint.
Knowing the microcode version on which the MCE was received is critical
information for debugging. If the version is not recorded, later attempts
to acquire the version might result in discrepancies since it can be
changed at runtime.
Export microcode version through the tracepoint to prevent ambiguity over
the active version on the system when the MCE was received.
Signed-off-by: Avadhut Naik
Reviewed-by: Sohil Mehta
Reviewed-by: Steven Rostedt (Google)
---
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 959ba7b775b1..69438550252c 100644
--- a/include/trace/events/mce.h
+++ b/include/trace/events/mce.h
@@ -42,6 +42,7 @@ TRACE_EVENT(mce_record,
__field(u8, cs )
__field(u8, bank)
__field(u8, cpuvendor )
+ __field(u32,microcode )
),
TP_fast_assign(
@@ -63,9 +64,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:
%x",
__entry->cpu,
__entry->mcgcap, __entry->mcgstatus,
__entry->bank, __entry->status,
@@ -77,7 +79,8 @@ TRACE_EVENT(mce_record,
__entry->cpuvendor, __entry->cpuid,
__entry->walltime,
__entry->socketid,
- __entry->apicid)
+ __entry->apicid,
+ __entry->microcode)
);
#endif /* _TRACE_MCE_H */
--
2.34.1
