Re: [PATCH v7 5/5] EDAC/mce_amd: Add support for FRU Text in MCA

["Naik, Avadhut" <avadnaik@amd.com>]

On 10/30/2024 11:31, Yazen Ghannam wrote:
> On Wed, Oct 30, 2024 at 05:15:50PM +0100, Borislav Petkov wrote:
>> On Tue, Oct 22, 2024 at 07:36:31PM +0000, Avadhut Naik wrote:
>>> From: Yazen Ghannam <yazen.ghannam@amd.com>
>>>
>>> A new "FRU Text in MCA" feature is defined where the Field Replaceable
>>> Unit (FRU) Text for a device is represented by a string in the new
>>> MCA_SYND1 and MCA_SYND2 registers. This feature is supported per MCA
>>> bank, and it is advertised by the McaFruTextInMca bit (MCA_CONFIG[9]).
>>>
>>> The FRU Text is populated dynamically for each individual error state
>>> (MCA_STATUS, MCA_ADDR, et al.). This handles the case where an MCA bank
>>> covers multiple devices, for example, a Unified Memory Controller (UMC)
>>> bank that manages two DIMMs.
>>>
>>> Since MCA_CONFIG[9] is instrumental in decoding FRU Text, it has to be
>>> exported through the mce_record tracepoint so that userspace tools like
>>> the rasdaemon can determine if FRU Text has been reported through the
>>> MCA_SYND1 and MCA_SYND2 registers and output it.
>>
>> IOW:
>>
>> Author: Yazen Ghannam <yazen.ghannam@amd.com>
>> Date:   Tue Oct 22 19:36:31 2024 +0000
>>
>>     EDAC/mce_amd: Add support for FRU text in MCA
>>     
>>     A new "FRU Text in MCA" feature is defined where the Field Replaceable
>>     Unit (FRU) Text for a device is represented by a string in the new
>>     MCA_SYND1 and MCA_SYND2 registers. This feature is supported per MCA
>>     bank, and it is advertised by the McaFruTextInMca bit (MCA_CONFIG[9]).
>>     
>>     The FRU Text is populated dynamically for each individual error state
>>     (MCA_STATUS, MCA_ADDR, et al.). This handles the case where an MCA bank
>>     covers multiple devices, for example, a Unified Memory Controller (UMC)
>>     bank that manages two DIMMs.
>>     
>>     If SYND1 and SYND2 are !NULL, then userspace can assume that they
>>     contain FRU text information. If they will report other information in
>>     the future, then a way of communicating the info type contained must be
>>     devised.
>>     
>>       [ Yazen: Add Avadhut as co-developer for wrapper changes. ]
>>       [ bp: Do not expose MCA_CONFIG to userspace yet. ]
> 
> The entire struct mce_hw_err gets exposed through the mce tracepoint in
> patch 3 of this set.
> 
>>     
>>     Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
>>     Co-developed-by: Avadhut Naik <avadhut.naik@amd.com>
>>     Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
>>     Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
>>     Link: https://lore.kernel.org/r/20241022194158.110073-6-avadhut.naik@amd.com
>>
>> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
>> index 4d936ee20e24..649a901ad563 100644
>> --- a/arch/x86/include/asm/mce.h
>> +++ b/arch/x86/include/asm/mce.h
>> @@ -61,6 +61,7 @@
>>   *  - TCC bit is present in MCx_STATUS.
>>   */
>>  #define MCI_CONFIG_MCAX		0x1
>> +#define MCI_CONFIG_FRUTEXT	BIT_ULL(9)
>>  #define MCI_IPID_MCATYPE	0xFFFF0000
>>  #define MCI_IPID_HWID		0xFFF
>>  
>> @@ -212,6 +213,7 @@ struct mce_hw_err {
>>  		struct {
>>  			u64 synd1;		/* MCA_SYND1 MSR */
>>  			u64 synd2;		/* MCA_SYND2 MSR */
>> +			u64 config;		/* MCA_CONFIG MSR */
> 
> Anything that is added here will automatically show up in the
> tracepoint, since it's a dynamic array. That was one of the reasons to
> do the wrapper struct idea, right?
> 
>>  		} amd;
>>  	} vendor;
>>  };
>> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
>> index 6ca80fff1fea..65ace034af08 100644
>> --- a/arch/x86/kernel/cpu/mce/amd.c
>> +++ b/arch/x86/kernel/cpu/mce/amd.c
>> @@ -796,6 +796,7 @@ static void __log_error(unsigned int bank, u64 status, u64 addr, u64 misc)
>>  
>>  	if (mce_flags.smca) {
>>  		rdmsrl(MSR_AMD64_SMCA_MCx_IPID(bank), m->ipid);
>> +		rdmsrl(MSR_AMD64_SMCA_MCx_CONFIG(bank), err.vendor.amd.config);
>>  
>>  		if (m->status & MCI_STATUS_SYNDV) {
>>  			rdmsrl(MSR_AMD64_SMCA_MCx_SYND(bank), m->synd);
>> diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
>> index 0a89947e47bc..19a1c72fc2bf 100644
>> --- a/arch/x86/kernel/cpu/mce/apei.c
>> +++ b/arch/x86/kernel/cpu/mce/apei.c
>> @@ -155,6 +155,8 @@ int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
>>  		fallthrough;
>>  	/* MCA_CONFIG */
>>  	case 4:
>> +		err.vendor.amd.config = *(i_mce + 3);
>> +		fallthrough;
>>  	/* MCA_MISC0 */
>>  	case 3:
>>  		m->misc = *(i_mce + 2);
>> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
>> index fca23fe16abe..edc2c8033de8 100644
>> --- a/arch/x86/kernel/cpu/mce/core.c
>> +++ b/arch/x86/kernel/cpu/mce/core.c
>> @@ -681,6 +681,7 @@ static noinstr void mce_read_aux(struct mce_hw_err *err, int i)
>>  
>>  	if (mce_flags.smca) {
>>  		m->ipid = mce_rdmsrl(MSR_AMD64_SMCA_MCx_IPID(i));
>> +		err->vendor.amd.config = mce_rdmsrl(MSR_AMD64_SMCA_MCx_CONFIG(i));
>>  
>>  		if (m->status & MCI_STATUS_SYNDV) {
>>  			m->synd = mce_rdmsrl(MSR_AMD64_SMCA_MCx_SYND(i));
>> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
>> index 194d9fd47d20..62fcd92bf9d2 100644
>> --- a/drivers/edac/mce_amd.c
>> +++ b/drivers/edac/mce_amd.c
>> @@ -795,6 +795,7 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>>  	struct mce *m = (struct mce *)data;
>>  	struct mce_hw_err *err = to_mce_hw_err(m);
>>  	unsigned int fam = x86_family(m->cpuid);
>> +	u64 mca_config = err->vendor.amd.config;
>>  	int ecc;
>>  
>>  	if (m->kflags & MCE_HANDLED_CEC)
>> @@ -814,11 +815,7 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>>  		((m->status & MCI_STATUS_PCC)	? "PCC"	  : "-"));
>>  
>>  	if (boot_cpu_has(X86_FEATURE_SMCA)) {
>> -		u32 low, high;
>> -		u32 addr = MSR_AMD64_SMCA_MCx_CONFIG(m->bank);
>> -
>> -		if (!rdmsr_safe(addr, &low, &high) &&
>> -		    (low & MCI_CONFIG_MCAX))
>> +		if (mca_config & MCI_CONFIG_MCAX)
>>  			pr_cont("|%s", ((m->status & MCI_STATUS_TCC) ? "TCC" : "-"));
>>  
>>  		pr_cont("|%s", ((m->status & MCI_STATUS_SYNDV) ? "SyndV" : "-"));
>> @@ -853,8 +850,15 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>>  
>>  		if (m->status & MCI_STATUS_SYNDV) {
>>  			pr_cont(", Syndrome: 0x%016llx\n", m->synd);
>> -			pr_emerg(HW_ERR "Syndrome1: 0x%016llx, Syndrome2: 0x%016llx",
>> -				 err->vendor.amd.synd1, err->vendor.amd.synd2);
>> +			if (mca_config & MCI_CONFIG_FRUTEXT) {
>> +				char frutext[17];
>> +
>> +				frutext[16] = '\0';
>> +				memcpy(&frutext[0], &err->vendor.amd.synd1, 8);
>> +				memcpy(&frutext[8], &err->vendor.amd.synd2, 8);
>> +
>> +				pr_emerg(HW_ERR "FRU Text: %s", frutext);
>> +			}
>>  		}
>>  
>>  		pr_cont("\n");
>>
>>
> 
> The only changes I see are dropping a couple of kernel prints. I think
> that's probably okay. But I don't think that's what you intend by not
> exposing MCA_CONFIG to user space.
> 

Like Yazen mentioned, we dont explicitly need to add MCA_CONFIG to the tracepoint
Just adding it to struct mce_hw_err should be enough as it would then be
exported as vendor-specific data.

Additionally, userspace tools like rasdaemon would need MCA_CONFIG to undertake
FRU Text decoding from SYND1/2 registers.

> Thanks,
> Yazen

-- 
Thanks,
Avadhut Naik