Received-SPF: pass (google.com: domain of linux-kernel+bounces-39499-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <acac059f-6e6e-428b-907c-ef63c79a9410@amd.com>
Date: Thu, 25 Jan 2024 19:35:14 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] tracing: Include Microcode Revision in mce_record
 tracepoint
Content-Language: en-US
To: Sohil Mehta <sohil.mehta@intel.com>, linux-trace-kernel@vger.kernel.org,
 linux-edac@vger.kernel.org
Cc: rostedt@goodmis.org, tony.luck@intel.com, bp@alien8.de, x86@kernel.org,
 linux-kernel@vger.kernel.org, yazen.ghannam@amd.com,
 Avadhut Naik <avadhut.naik@amd.com>
References: <20240125184857.851355-1-avadhut.naik@amd.com>
 <20240125184857.851355-3-avadhut.naik@amd.com>
 <1792a925-172f-4a9e-ad59-dea474bc7cda@intel.com>
From: "Naik, Avadhut" <avadnaik@amd.com>
In-Reply-To: <1792a925-172f-4a9e-ad59-dea474bc7cda@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RFhadXVzcFdqUnZnZkJWSW1TZzlHbVgrUFdIRTJDWVZYaEsxUFNKanVEbVN0?=
 =?utf-8?B?dGtXNDNLaUxxZWp0TitsemI5bUNuS3lvbHBWcU1sakdRTlpoM3NDT0doNytQ?=
 =?utf-8?B?cUpId2FtWWJPRm5paTdhNmM0MndCYkpSczRPT1FOMVlXUXZOMkxQLzQ4dGVl?=
 =?utf-8?B?WEd0RmNCVEhkYkdwWStGZ0VaeHd6MFZac0RMODVtOThVMmtPbEdQZ01JWEVu?=
 =?utf-8?B?MUxGUFo2S3ZRMk5oYkZPbUlwQTBUMW9uZmRydStHRzNXMG4zbnhKemlEWEJw?=
 =?utf-8?B?QXc0a0Z3eHlWaUVkSFg3SThTNm1HcUNER3NLRXFHWXVnT3dZTCtsWWVGR0pm?=
 =?utf-8?B?L2RWTVQyM0RJSmp6OEp2MURSSng1ampSMy92WWdBUDUwN0NiRDdXYzJUaGFR?=
 =?utf-8?B?WUFrdjFHT08rUklUamx5YXFFT3YvTFUrcWVUd005Y2RLdlVHT016SjN6UUhk?=
 =?utf-8?B?V2NZTElabS9RcVdZNGllWE5ZNWZPNzY3TGJ0SzNZY1VtWDlrM1Z3ZEZhUlRB?=
 =?utf-8?B?Q0J2Z0FjUE9YaXRBaFpaNFBtSG0yL25jTEZUTUtaaURKanNqbER0OGFZYnh2?=
 =?utf-8?B?dElkR0Q4aWUzVjdqWk5KZVM1NVdRbXY1NzlGSjNMNG9rL0loM3o5cy80Yzd3?=
 =?utf-8?B?bjlVczJXMHFwcVFxTlNLYWlXMlRCeGl2Sk1CL1EvUElQV0RuaXFZRHJYbnN1?=
 =?utf-8?B?NVFPbTNjSVg2N0ozeG0zazVPamxpTVMxZkdSSThEdDU4ZUN0UXJ4ZG9KVTEx?=
 =?utf-8?B?dGNCN2FvVkVhdlE1T1R0SjZ2QlN0Sjh5UTA4ODhrbFZndEI0T08rQU56S2xq?=
 =?utf-8?B?S1Zja3VpOHk3MmE2bWMwUXM1aFl6OUNKNGF3RUM4K29FUzR2NUZ1S0l2cFBr?=
 =?utf-8?B?MDFJTUt4d2s1VXgrOTYrRDhhakxGRng3VkJtY2w5VTFUM3lJSlhDNGZSY2k0?=
 =?utf-8?B?TmIvY1RpM2ZDb1VXck5CSzVrL3lndk1BdHhSMWQ2ZDh5d1pXL29HaWJlcGpX?=
 =?utf-8?B?ci9WV0Y1cXZJNDNEK0NTMDAyS1hNRHZmeU8rbEdnNFpFdUh2cTdhWEQ1Q3VY?=
 =?utf-8?B?b1VCVlZUdFYwY2ZsWGx1M2dqcXVaTUN3OVBxSWxOckF1K2VnM3ZxTVpLMGo3?=
 =?utf-8?B?ZDc4TlpaK3ROSWFNVVhKdVpUYW5HRmFoN1MybmEvc0E1aWNydS95KzR1K2s4?=
 =?utf-8?B?R0ZtMzVHUTFFRWp6eVJTSHlydmFYclBPVGZxK2dyRWJRaXlHa2NQbElZWkFB?=
 =?utf-8?B?c3puUlViYXh0azJUL2w1NTV2OTJydU1lMjRac1VaYk0zQVFYZEtjWmx3Rkl6?=
 =?utf-8?B?UGJOQWZzZ0VGQk1UbUNXR3psY21XUCtlU1BPSzRrV0hTWWo5UVpPQlc2U3Z4?=
 =?utf-8?B?eU5wUlhHZ2gxcUlJUnFpUjl6NDBHblhZY0paaDVjc1BFNDJSL0JJM3dUU0M0?=
 =?utf-8?B?dXBFTXQ2c3RYT3ArWm1nV0d3d0tQUGN2SnVSYTVpb0YvUVU3cXNVNDVrV0Q5?=
 =?utf-8?B?bEVXbk5rOUM5TkIzSGtSMUQ2cUJFajgweXAxVHVZN21VREhaVFFoRkEvRlNP?=
 =?utf-8?B?b1B6WUN0bjEzOHF6Ty9UU1Z2dytxS3JROGpybFhJcWkrcnpsOUlSLzBqczNm?=
 =?utf-8?B?b0dsZ20xSUxoS3V3ejNYVHZNNTR0TWRYNTBDdUNWL2d3aFU4V2dmK3ZGRmYw?=
 =?utf-8?B?cVIwN3FpNWZuMXcrck5WZ3AwY2VxODZPMjc4dThrY24xa0ZCOWxRUitPOU5D?=
 =?utf-8?B?dHUrUitTa0xWSzk0cFg3UVd1Tnd3bGZWeUZSMjBYYXI1ZWVFVXByUVVvWkVr?=
 =?utf-8?B?ay96V0N3dkJHU1g3SFRIck93cnZOTWYwY3pUZ2xxU204Tk12TVNpVXljNGph?=
 =?utf-8?B?Z2cvbTQ5QzhrMEptZUF6NFJ6MTJnZ0ZMV0pYcnJUV2lpemd5cU9Ybm9lN1Ix?=
 =?utf-8?B?cm1WTzRrTWNJNHJ3K3hlVFVIVUFBdVp4NEFicjZNaVN6ajFXb3lWVi9jMUZB?=
 =?utf-8?B?dDBPQlNYd1hEQmVwSXYrZm1aczhoUnVJTll4MXhiZ3ZsendsZFM5TlVreWFP?=
 =?utf-8?B?MEJSYkIwU2xyblUxRTlhbVBoQ0Q4U2doYjdzL2hHcXVLMUU5eXRJRlBIQ01N?=
 =?utf-8?Q?F4IJIU1yK+N1fuvEpfVZ7PmOR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 862fa1c9-cfc3-43c7-f2e4-08dc1e0f0d1e
X-MS-Exchange-CrossTenant-AuthSource: CH3PR12MB8403.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 01:35:17.5446
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: O+RiGCPDyVAZNJAdMvT6JdDXBD/6A18C2gbTQwspozR/3FPwuduiY8cRMOmap/K0cOoVUwZFi2w3IZavxQXJUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB7823


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