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 <[email protected]>
>>>
>>> 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 <[email protected]>
>> 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 <[email protected]>
>> Co-developed-by: Avadhut Naik <[email protected]>
>> Signed-off-by: Avadhut Naik <[email protected]>
>> Signed-off-by: Borislav Petkov (AMD) <[email protected]>
>> Link:
>> https://lore.kernel.org/r/[email protected]
>>
>> 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
