[PATCH] tracing: Include PPIN in mce_record tracepoint

[Naik, Avadhut]

Hi,

On 1/23/2024 6:12 PM, Tony Luck wrote:
> On Tue, Jan 23, 2024 at 05:51:50PM -0600, Avadhut Naik wrote:
>> Machine Check Error information from struct mce is exported to userspace
>> through the mce_record tracepoint.
>>
>> Currently, however, the PPIN (Protected Processor Inventory Number) field
>> of struct mce is not exported through the tracepoint.
>>
>> Export PPIN through the tracepoint as it may provide useful information
>> for debug and analysis.
> 
> Awesome. I've been meaning to update the tracepoint for ages, but
> it never gets to the top of the queue.
> 
> But some questions:
> 
> 1) Are tracepoints a user visible ABI? Adding a new field in the middle
> feels like it might be problematic. I asked this question many years
> ago and Steven Rostedt said there was some tracing library in the works
> that would make this OK for appplications using that library.
> 

I think they can be user visible through the "trace" and "trace_pipe" in
/sys/kernel/debug/tracing. But you will have to enable the events you want
to trace through /sys/kernel/debug/tracing/events/<event-name>/enable.

AFAIK, this (adding field in the middle) shouldn't be problematic as we
have the tracepoint format available in debugfs. For e.g. with this patch,
the format is as follows:

[root avadnaik]# cat /sys/kernel/debug/tracing/events/mce/mce_record/format 
name: mce_record
ID: 113
format:
        field:unsigned short common_type;       offset:0;       size:2; signed:0;
        field:unsigned char common_flags;       offset:2;       size:1; signed:0;
        field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
        field:int common_pid;   offset:4;       size:4; signed:1;

        field:u64 mcgcap;       offset:8;       size:8; signed:0;
        field:u64 mcgstatus;    offset:16;      size:8; signed:0;
        field:u64 status;       offset:24;      size:8; signed:0;
        field:u64 addr; offset:32;      size:8; signed:0;
        field:u64 misc; offset:40;      size:8; signed:0;
        field:u64 synd; offset:48;      size:8; signed:0;
        field:u64 ipid; offset:56;      size:8; signed:0;
        field:u64 ip;   offset:64;      size:8; signed:0;
        field:u64 tsc;  offset:72;      size:8; signed:0;
        field:u64 ppin; offset:80;      size:8; signed:0;
        field:u64 walltime;     offset:88;      size:8; signed:0;
        field:u32 cpu;  offset:96;      size:4; signed:0;
        field:u32 cpuid;        offset:100;     size:4; signed:0;
        field:u32 apicid;       offset:104;     size:4; signed:0;
        field:u32 socketid;     offset:108;     size:4; signed:0;
        field:u8 cs;    offset:112;     size:1; signed:0;
        field:u8 bank;  offset:113;     size:1; signed:0;
        field:u8 cpuvendor;     offset:114;     size:1; signed:0;

print fmt: "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", REC->cpu, REC->mcgcap, REC->mcgstatus, REC->bank, REC->status, REC->ipid, REC->addr, REC->misc, REC->synd, REC->cs, REC->ip, REC->tsc, REC->ppin, REC->cpuvendor, REC->cpuid, REC->walltime, REC->socketid, REC->apicid


Just quickly tried with rasdaemon and things seem to be okay.

Also, not a cent percent sure, but the library you are mentioning of, I think
its the libtraceevent library and IIUC, it utilizes the above tracepoint format.

> 2) While you are adding to the tracepoint, should we batch up all
> the useful changes that have been made to "struct mce". I think the
> new fields that might be of use are:
> 
>         __u64 synd;             /* MCA_SYND MSR: only valid on SMCA systems */
>         __u64 ipid;             /* MCA_IPID MSR: only valid on SMCA systems */
>         __u64 ppin;             /* Protected Processor Inventory Number */
>         __u32 microcode;        /* Microcode revision */
> 

synd and ipid are already a part of mce_record tracepoint. (They too have been
added in the middle). Will add the microcode field in the next version.

>>
>> Signed-off-by: Avadhut Naik <avadhut.naik@xxxxxxx>
>> ---
>>  include/trace/events/mce.h | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/trace/events/mce.h b/include/trace/events/mce.h
>> index 1391ada0da3b..657b93ec8176 100644
>> --- a/include/trace/events/mce.h
>> +++ b/include/trace/events/mce.h
>> @@ -25,6 +25,7 @@ TRACE_EVENT(mce_record,
>>  		__field(	u64,		ipid		)
>>  		__field(	u64,		ip		)
>>  		__field(	u64,		tsc		)
>> +		__field(	u64,		ppin	)
>>  		__field(	u64,		walltime	)
>>  		__field(	u32,		cpu		)
>>  		__field(	u32,		cpuid		)
>> @@ -45,6 +46,7 @@ TRACE_EVENT(mce_record,
>>  		__entry->ipid		= m->ipid;
>>  		__entry->ip		= m->ip;
>>  		__entry->tsc		= m->tsc;
>> +		__entry->ppin		= m->ppin;
>>  		__entry->walltime	= m->time;
>>  		__entry->cpu		= m->extcpu;
>>  		__entry->cpuid		= m->cpuid;
>> @@ -55,7 +57,7 @@ TRACE_EVENT(mce_record,
>>  		__entry->cpuvendor	= m->cpuvendor;
>>  	),
> 
> ... rest of patch trimmed.
> 
> -Tony

-- 
Thanks,
Avadhut Naik