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

Commit Message

Avadhut Naik Oct. 22, 2024, 7:36 p.m. UTC

From: Yazen Ghannam <[e-mail]>

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.

[Yazen: Add Avadhut as co-developer for wrapper changes.]

Co-developed-by: Avadhut Naik <[e-mail]>
Signed-off-by: Avadhut Naik <[e-mail]>
Signed-off-by: Yazen Ghannam <[e-mail]>
Signed-off-by: Avadhut Naik <[e-mail]>
---
Changes in v2:
[1] https://lore.kernel.org/linux-edac/[e-mail]/
[2] https://lore.kernel.org/linux-edac/[e-mail]/

1. Drop dependencies on sets [1] and [2] above and rebase on top of
tip/master.

Changes in v3:
1. Modify commit message per feedback provided.
2. Remove call to memset() for the string frutext. Instead, just ensure
that it is NULL terminated.
2. Fix SoB chain to properly reflect the patch path.

Changes in v4:
1. Rebase on top of tip/master to avoid merge conflicts.

Changes in v5:
1. No changes except rebasing on top of tip/master.

Changes in v6:
1. No changes except rebasing on top of tip/master.

Changes in v7:
1. No changes except rebasing on top of tip/master.
---
 arch/x86/include/asm/mce.h     |  2 ++
 arch/x86/kernel/cpu/mce/amd.c  |  1 +
 arch/x86/kernel/cpu/mce/apei.c |  2 ++
 arch/x86/kernel/cpu/mce/core.c |  3 +++
 drivers/edac/mce_amd.c         | 21 ++++++++++++++-------
 5 files changed, 22 insertions(+), 7 deletions(-)

Comments

Qiuxu Zhuo Oct. 24, 2024, 5:49 a.m. UTC | #1

> From: Avadhut Naik <[e-mail]>
> [...]
> Subject: [PATCH v7 5/5] EDAC/mce_amd: Add support for FRU Text in MCA
> 
> From: Yazen Ghannam <[e-mail]>
> 
> 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.
> 
> [Yazen: Add Avadhut as co-developer for wrapper changes.]
> 
> Co-developed-by: Avadhut Naik <[e-mail]>
> Signed-off-by: Avadhut Naik <[e-mail]>
> Signed-off-by: Yazen Ghannam <[e-mail]>
> Signed-off-by: Avadhut Naik <[e-mail]>

LGTM.

    Reviewed-by: Qiuxu Zhuo <[e-mail]>

Borislav Petkov Oct. 30, 2024, 4:05 p.m. UTC | #2

On Tue, Oct 22, 2024 at 07:36:31PM +0000, Avadhut Naik wrote:
> @@ -853,8 +850,18 @@ 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);
> +			} else {
> +				pr_emerg(HW_ERR "Syndrome1: 0x%016llx, Syndrome2: 0x%016llx",
> +					 err->vendor.amd.synd1, err->vendor.amd.synd2);
> +			}
>  		}

Right, so let's turn this into:

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index bc5e67306f77..edc2c8033de8 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -208,8 +208,6 @@ static void __print_mce(struct mce_hw_err *err)
 			pr_cont("SYND2 %llx ", err->vendor.amd.synd2);
 		if (m->ipid)
 			pr_cont("IPID %llx ", m->ipid);
-		if (err->vendor.amd.config)
-			pr_cont("CONFIG %llx ", err->vendor.amd.config);
 	}
 
 	pr_cont("\n");
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index d69a1466f0bc..62fcd92bf9d2 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -858,9 +858,6 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 				memcpy(&frutext[8], &err->vendor.amd.synd2, 8);
 
 				pr_emerg(HW_ERR "FRU Text: %s", frutext);
-			} else {
-				pr_emerg(HW_ERR "Syndrome1: 0x%016llx, Syndrome2: 0x%016llx",
-					 err->vendor.amd.synd1, err->vendor.amd.synd2);
 			}
 		}
 
and simply treat synd1 and synd2 as FRU text. I don't want to expose
mca_config to userspace yet but use it in the RAS code only. If a case appears
that we want to really expose it to userspace, we can talk about a proper
design then.

This patch doesn't make it part of the tracepoint either so...

Borislav Petkov Oct. 30, 2024, 4:15 p.m. UTC | #3

On Tue, Oct 22, 2024 at 07:36:31PM +0000, Avadhut Naik wrote:
> From: Yazen Ghannam <[e-mail]>
> 
> 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 <[e-mail]>
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. ]
    
    Signed-off-by: Yazen Ghannam <[e-mail]>
    Co-developed-by: Avadhut Naik <[e-mail]>
    Signed-off-by: Avadhut Naik <[e-mail]>
    Signed-off-by: Borislav Petkov (AMD) <[e-mail]>
    Link: https://lore.kernel.org/r/[e-mail]

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 */
 		} 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");

Yazen Ghannam Oct. 30, 2024, 4:31 p.m. UTC | #4

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

Thanks,
Yazen

Naik, Avadhut Oct. 30, 2024, 4:49 p.m. UTC | #5

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

Borislav Petkov Oct. 30, 2024, 4:50 p.m. UTC | #6

On Wed, Oct 30, 2024 at 12:31:47PM -0400, Yazen Ghannam wrote:
> The entire struct mce_hw_err gets exposed through the mce tracepoint in
> patch 3 of this set.

Bah, crap. Lemme go back and take a second stab at this.

Borislav Petkov Oct. 30, 2024, 6:01 p.m. UTC | #7

On Wed, Oct 30, 2024 at 05:50:02PM +0100, Borislav Petkov wrote:
> Bah, crap. Lemme go back and take a second stab at this.

Second try.

The reason why I don't want to expose MCA_CONFIG to userspace is, well,
userspace doesn't need to know any "management" information the hw gives. It
either gets FRU text in that tracepoint or it doesn't. But it doesn't need to
know what MCA_CONFIG said or didn't say.

Ok?

Author: Yazen Ghannam <[e-mail]>
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.). Handle the case where an MCA bank covers
    multiple devices, for example, a Unified Memory Controller (UMC) bank
    that manages two DIMMs.
    
      [ Yazen: Add Avadhut as co-developer for wrapper changes. ]
      [ bp: Do not expose MCA_CONFIG to userspace yet. ]
    
    Signed-off-by: Yazen Ghannam <[e-mail]>
    Co-developed-by: Avadhut Naik <[e-mail]>
    Signed-off-by: Avadhut Naik <[e-mail]>
    Signed-off-by: Borislav Petkov (AMD) <[e-mail]>
    Link: https://lore.kernel.org/r/[e-mail]

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 4d936ee20e24..4543cf2eb5e8 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
 
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 194d9fd47d20..50d74d3bf0f5 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);
+	u32 mca_config_lo = 0, dummy;
 	int ecc;
 
 	if (m->kflags & MCE_HANDLED_CEC)
@@ -814,11 +815,9 @@ 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);
+		rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(m->bank), &mca_config_lo, &dummy);
 
-		if (!rdmsr_safe(addr, &low, &high) &&
-		    (low & MCI_CONFIG_MCAX))
+		if (mca_config_lo & MCI_CONFIG_MCAX)
 			pr_cont("|%s", ((m->status & MCI_STATUS_TCC) ? "TCC" : "-"));
 
 		pr_cont("|%s", ((m->status & MCI_STATUS_SYNDV) ? "SyndV" : "-"));
@@ -853,8 +852,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_lo & 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");

Naik, Avadhut Oct. 30, 2024, 7:57 p.m. UTC | #8

On 10/30/2024 13:01, Borislav Petkov wrote:
> On Wed, Oct 30, 2024 at 05:50:02PM +0100, Borislav Petkov wrote:
>> Bah, crap. Lemme go back and take a second stab at this.
> 
> Second try.
> 
> The reason why I don't want to expose MCA_CONFIG to userspace is, well,
> userspace doesn't need to know any "management" information the hw gives. It
> either gets FRU text in that tracepoint or it doesn't. But it doesn't need to
> know what MCA_CONFIG said or didn't say.
> 
> Ok?
> 
So, for now, in the kernel, we log SYND1/2 registers only when they contain
FRUText.
While in the userspace, since MCA_CONFIG is not in the picture, we always
interpret SYND1/2 data as FRUText.
Rasdaemon might need to be tweaked accordingly. Will take care of it.
Overall, sounds good.

Do you want me send out a revised version with these changes?

> Author: Yazen Ghannam <[e-mail]>
> 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.). Handle the case where an MCA bank covers
>     multiple devices, for example, a Unified Memory Controller (UMC) bank
>     that manages two DIMMs.
>     
>       [ Yazen: Add Avadhut as co-developer for wrapper changes. ]
>       [ bp: Do not expose MCA_CONFIG to userspace yet. ]
>     
>     Signed-off-by: Yazen Ghannam <[e-mail]>
>     Co-developed-by: Avadhut Naik <[e-mail]>
>     Signed-off-by: Avadhut Naik <[e-mail]>
>     Signed-off-by: Borislav Petkov (AMD) <[e-mail]>
>     Link: https://lore.kernel.org/r/[e-mail]
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 4d936ee20e24..4543cf2eb5e8 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
>  
> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> index 194d9fd47d20..50d74d3bf0f5 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);
> +	u32 mca_config_lo = 0, dummy;
>  	int ecc;
>  
>  	if (m->kflags & MCE_HANDLED_CEC)
> @@ -814,11 +815,9 @@ 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);
> +		rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(m->bank), &mca_config_lo, &dummy);
>  
> -		if (!rdmsr_safe(addr, &low, &high) &&
> -		    (low & MCI_CONFIG_MCAX))
> +		if (mca_config_lo & MCI_CONFIG_MCAX)
>  			pr_cont("|%s", ((m->status & MCI_STATUS_TCC) ? "TCC" : "-"));
>  
>  		pr_cont("|%s", ((m->status & MCI_STATUS_SYNDV) ? "SyndV" : "-"));
> @@ -853,8 +852,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_lo & 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");
>

Yazen Ghannam Oct. 30, 2024, 8:46 p.m. UTC | #9

On Wed, Oct 30, 2024 at 02:57:33PM -0500, Naik, Avadhut wrote:
> 
> 
> On 10/30/2024 13:01, Borislav Petkov wrote:
> > On Wed, Oct 30, 2024 at 05:50:02PM +0100, Borislav Petkov wrote:
> >> Bah, crap. Lemme go back and take a second stab at this.
> > 
> > Second try.
> > 
> > The reason why I don't want to expose MCA_CONFIG to userspace is, well,
> > userspace doesn't need to know any "management" information the hw gives. It
> > either gets FRU text in that tracepoint or it doesn't. But it doesn't need to
> > know what MCA_CONFIG said or didn't say.
> > 
> > Ok?
> > 
> So, for now, in the kernel, we log SYND1/2 registers only when they contain
> FRUText.
> While in the userspace, since MCA_CONFIG is not in the picture, we always
> interpret SYND1/2 data as FRUText.
> Rasdaemon might need to be tweaked accordingly. Will take care of it.
> Overall, sounds good.
>

Sounds good to me too.

Thanks,
Yazen

> Do you want me send out a revised version with these changes?
> 
> > Author: Yazen Ghannam <[e-mail]>
> > 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.). Handle the case where an MCA bank covers
> >     multiple devices, for example, a Unified Memory Controller (UMC) bank
> >     that manages two DIMMs.
> >     
> >       [ Yazen: Add Avadhut as co-developer for wrapper changes. ]
> >       [ bp: Do not expose MCA_CONFIG to userspace yet. ]
> >     
> >     Signed-off-by: Yazen Ghannam <[e-mail]>
> >     Co-developed-by: Avadhut Naik <[e-mail]>
> >     Signed-off-by: Avadhut Naik <[e-mail]>
> >     Signed-off-by: Borislav Petkov (AMD) <[e-mail]>
> >     Link: https://lore.kernel.org/r/[e-mail]
> > 
> > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> > index 4d936ee20e24..4543cf2eb5e8 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
> >  
> > diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> > index 194d9fd47d20..50d74d3bf0f5 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);
> > +	u32 mca_config_lo = 0, dummy;
> >  	int ecc;
> >  
> >  	if (m->kflags & MCE_HANDLED_CEC)
> > @@ -814,11 +815,9 @@ 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);
> > +		rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(m->bank), &mca_config_lo, &dummy);
> >  
> > -		if (!rdmsr_safe(addr, &low, &high) &&
> > -		    (low & MCI_CONFIG_MCAX))
> > +		if (mca_config_lo & MCI_CONFIG_MCAX)
> >  			pr_cont("|%s", ((m->status & MCI_STATUS_TCC) ? "TCC" : "-"));
> >  
> >  		pr_cont("|%s", ((m->status & MCI_STATUS_SYNDV) ? "SyndV" : "-"));
> > @@ -853,8 +852,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_lo & 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");
> > 
> 
> -- 
> Thanks,
> Avadhut Naik

Borislav Petkov Oct. 30, 2024, 9:23 p.m. UTC | #10

On Wed, Oct 30, 2024 at 02:57:33PM -0500, Naik, Avadhut wrote:
> So, for now, in the kernel, we log SYND1/2 registers only when they contain
> FRUText.  While in the userspace, since MCA_CONFIG is not in the picture, we
> always interpret SYND1/2 data as FRUText.  Rasdaemon might need to be
> tweaked accordingly. Will take care of it.  Overall, sounds good.

Thanks.

> Do you want me send out a revised version with these changes?

Nah, I will queue them soon as I have all the bits here already.

Thx.

Patch

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c2466b20fe79..72a69ad7d692 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
 
@@ -213,6 +214,7 @@  struct mce_hw_err {
 		struct {
 			u64 synd1;		/* MCA_SYND1 MSR */
 			u64 synd2;		/* MCA_SYND2 MSR */
+			u64 config;		/* MCA_CONFIG MSR */
 		} 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..bc5e67306f77 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -208,6 +208,8 @@  static void __print_mce(struct mce_hw_err *err)
 			pr_cont("SYND2 %llx ", err->vendor.amd.synd2);
 		if (m->ipid)
 			pr_cont("IPID %llx ", m->ipid);
+		if (err->vendor.amd.config)
+			pr_cont("CONFIG %llx ", err->vendor.amd.config);
 	}
 
 	pr_cont("\n");
@@ -681,6 +683,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..d69a1466f0bc 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,18 @@  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);
+			} else {
+				pr_emerg(HW_ERR "Syndrome1: 0x%016llx, Syndrome2: 0x%016llx",
+					 err->vendor.amd.synd1, err->vendor.amd.synd2);
+			}
 		}
 
 		pr_cont("\n");