[PATCH 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR} registers

[PATCH 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR} registers

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

We have support for the new SMCA MCA_DE{STAT,ADDR} registers in Linux. So
we've used these registers in place of MCA_{STATUS,ADDR} on SMCA systems.
However, the guidance for current implementations of SMCA is to continue
using MCA_{STATUS,ADDR} and to use MCA_DE{STAT,ADDR} only if a Deferred
error was not found in the former registers. This also means we shouldn't
clear MCA_CONFIG[LogDeferredInMcaStat].

Redo the AMD Deferred error interrupt handler to follow the guidance for
current SMCA systems. Also, don't break after finding the first error.

Don't clear MCA_CONFIG[LogDeferredInMcaStat] during AMD mcheck init.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 47 ++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 524cc57..4e459e0 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -472,20 +472,6 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 		smca_high |= BIT(0);
 
 		/*
-		 * SMCA logs Deferred Error information in MCA_DE{STAT,ADDR}
-		 * registers with the option of additionally logging to
-		 * MCA_{STATUS,ADDR} if MCA_CONFIG[LogDeferredInMcaStat] is set.
-		 *
-		 * This bit is usually set by BIOS to retain the old behavior
-		 * for OSes that don't use the new registers. Linux supports the
-		 * new registers so let's disable that additional logging here.
-		 *
-		 * MCA_CONFIG[LogDeferredInMcaStat] is bit 34 (bit 2 in the high
-		 * portion of the MSR).
-		 */
-		smca_high &= ~BIT(2);
-
-		/*
 		 * SMCA sets the Deferred Error Interrupt type per bank.
 		 *
 		 * MCA_CONFIG[DeferredIntTypeSupported] is bit 5, and tells us
@@ -756,7 +742,8 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 EXPORT_SYMBOL_GPL(umc_normaddr_to_sysaddr);
 
 static void
-__log_error(unsigned int bank, bool deferred_err, bool threshold_err, u64 misc)
+__log_error(unsigned int bank, bool deferred_err, bool use_smca_destat,
+			       bool threshold_err, u64 misc)
 {
 	u32 msr_status = msr_ops.status(bank);
 	u32 msr_addr = msr_ops.addr(bank);
@@ -765,7 +752,7 @@ __log_error(unsigned int bank, bool deferred_err, bool threshold_err, u64 misc)
 
 	WARN_ON_ONCE(deferred_err && threshold_err);
 
-	if (deferred_err && mce_flags.smca) {
+	if (deferred_err && use_smca_destat) {
 		msr_status = MSR_AMD64_SMCA_MCx_DESTAT(bank);
 		msr_addr = MSR_AMD64_SMCA_MCx_DEADDR(bank);
 	}
@@ -807,6 +794,10 @@ __log_error(unsigned int bank, bool deferred_err, bool threshold_err, u64 misc)
 
 	mce_log(&m);
 
+	/* We should still clear MCA_DESTAT even if we used MCA_STATUS. */
+	if (mce_flags.smca && !use_smca_destat)
+		wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(bank), 0);
+
 	wrmsrl(msr_status, 0);
 }
 
@@ -832,25 +823,29 @@ asmlinkage __visible void __irq_entry smp_trace_deferred_error_interrupt(void)
 	exiting_ack_irq();
 }
 
+static inline bool check_deferred_status(u64 status)
+{
+	return ((status & MCI_STATUS_VAL) && (status & MCI_STATUS_DEFERRED));
+}
+
 /* APIC interrupt handler for deferred errors */
 static void amd_deferred_error_interrupt(void)
 {
 	unsigned int bank;
-	u32 msr_status;
 	u64 status;
 
 	for (bank = 0; bank < mca_cfg.banks; ++bank) {
-		msr_status = (mce_flags.smca) ? MSR_AMD64_SMCA_MCx_DESTAT(bank)
-					      : msr_ops.status(bank);
+		rdmsrl(msr_ops.status(bank), status);
 
-		rdmsrl(msr_status, status);
+		if (check_deferred_status(status)) {
+			__log_error(bank, true, false, false, 0);
 
-		if (!(status & MCI_STATUS_VAL) ||
-		    !(status & MCI_STATUS_DEFERRED))
-			continue;
+		} else if (mce_flags.smca) {
+			rdmsrl(MSR_AMD64_SMCA_MCx_DESTAT(bank), status);
 
-		__log_error(bank, true, false, 0);
-		break;
+			if (check_deferred_status(status))
+				__log_error(bank, true, true, false, 0);
+		}
 	}
 }
 
@@ -904,7 +899,7 @@ static void amd_threshold_interrupt(void)
 	return;
 
 log:
-	__log_error(bank, false, true, ((u64)high << 32) | low);
+	__log_error(bank, false, false, true, ((u64)high << 32) | low);
 
 	/* Reset threshold block after logging error. */
 	memset(&tr, 0, sizeof(tr));
-- 
2.7.4

[PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Scalable MCA systems have a new MCA_CONFIG register that we use to
configure each bank. We currently use this when we set up thresholding.
However, this is logically separate.

Move setup of MCA_CONFIG into a separate function.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 48 ++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 4e459e0..95870b3 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -433,7 +433,7 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 			int offset, u32 misc_high)
 {
 	unsigned int cpu = smp_processor_id();
-	u32 smca_low, smca_high, smca_addr;
+	u32 smca_low, smca_high;
 	struct threshold_block b;
 	int new;
 
@@ -457,7 +457,29 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 		goto set_offset;
 	}
 
-	smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
+	/* Gather LVT offset for thresholding: */
+	if (rdmsr_safe(MSR_CU_DEF_ERR, &smca_low, &smca_high))
+		goto out;
+
+	new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
+
+set_offset:
+	offset = setup_APIC_mce_threshold(offset, new);
+
+	if ((offset == new) && (mce_threshold_vector != amd_threshold_interrupt))
+		mce_threshold_vector = amd_threshold_interrupt;
+
+done:
+	mce_threshold_block_init(&b, offset);
+
+out:
+	return offset;
+}
+
+static void set_smca_config(unsigned int bank)
+{
+	u32 smca_low, smca_high;
+	u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
 
 	if (!rdmsr_safe(smca_addr, &smca_low, &smca_high)) {
 		/*
@@ -487,24 +509,6 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 
 		wrmsr(smca_addr, smca_low, smca_high);
 	}
-
-	/* Gather LVT offset for thresholding: */
-	if (rdmsr_safe(MSR_CU_DEF_ERR, &smca_low, &smca_high))
-		goto out;
-
-	new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
-
-set_offset:
-	offset = setup_APIC_mce_threshold(offset, new);
-
-	if ((offset == new) && (mce_threshold_vector != amd_threshold_interrupt))
-		mce_threshold_vector = amd_threshold_interrupt;
-
-done:
-	mce_threshold_block_init(&b, offset);
-
-out:
-	return offset;
 }
 
 /* cpu init entry point, called from mce.c with preempt off */
@@ -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 	int offset = -1;
 
 	for (bank = 0; bank < mca_cfg.banks; ++bank) {
-		if (mce_flags.smca)
+		if (mce_flags.smca) {
 			get_smca_bank_info(bank);
+			set_smca_config(bank);
+		}
 
 		for (block = 0; block < NR_BLOCKS; ++block) {
 			address = get_block_address(cpu, address, low, high, bank, block);
-- 
2.7.4

Re: [PATCH 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR} registers

[Borislav Petkov]

On Wed, Mar 22, 2017 at 02:29:30PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> We have support for the new SMCA MCA_DE{STAT,ADDR} registers in Linux. So
> we've used these registers in place of MCA_{STATUS,ADDR} on SMCA systems.
> However, the guidance for current implementations of SMCA is to continue
> using MCA_{STATUS,ADDR} and to use MCA_DE{STAT,ADDR} only if a Deferred
> error was not found in the former registers. This also means we shouldn't
> clear MCA_CONFIG[LogDeferredInMcaStat].
> 
> Redo the AMD Deferred error interrupt handler to follow the guidance for
> current SMCA systems. Also, don't break after finding the first error.
> 
> Don't clear MCA_CONFIG[LogDeferredInMcaStat] during AMD mcheck init.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 47 ++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 524cc57..4e459e0 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -472,20 +472,6 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
>  		smca_high |= BIT(0);
>  
>  		/*
> -		 * SMCA logs Deferred Error information in MCA_DE{STAT,ADDR}
> -		 * registers with the option of additionally logging to
> -		 * MCA_{STATUS,ADDR} if MCA_CONFIG[LogDeferredInMcaStat] is set.
> -		 *
> -		 * This bit is usually set by BIOS to retain the old behavior
> -		 * for OSes that don't use the new registers. Linux supports the
> -		 * new registers so let's disable that additional logging here.
> -		 *
> -		 * MCA_CONFIG[LogDeferredInMcaStat] is bit 34 (bit 2 in the high
> -		 * portion of the MSR).
> -		 */
> -		smca_high &= ~BIT(2);
> -
> -		/*
>  		 * SMCA sets the Deferred Error Interrupt type per bank.
>  		 *
>  		 * MCA_CONFIG[DeferredIntTypeSupported] is bit 5, and tells us
> @@ -756,7 +742,8 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
>  EXPORT_SYMBOL_GPL(umc_normaddr_to_sysaddr);
>  
>  static void
> -__log_error(unsigned int bank, bool deferred_err, bool threshold_err, u64 misc)
> +__log_error(unsigned int bank, bool deferred_err, bool use_smca_destat,
> +			       bool threshold_err, u64 misc)

So I had to paste the function signature in a separate vim window and
keep looking between those arguments' names and the function calls.

Because if I look at:

	__log_error(bank, true, false, false, 0);

I absolutely have no clue what that code does. So we need to think of
something better. From the looks of it, I guess we dealt with a single
__log_error() function long enough. Perhaps it is time for separation:

log_error_deferred
log_error_smca
log_error...

and have a function __log_error() which all three call to do the work
which all three share.

That should make the code readable again IMO. __log_error() as it is now
is hard to follow anyway.

>  {
>  	u32 msr_status = msr_ops.status(bank);
>  	u32 msr_addr = msr_ops.addr(bank);
> @@ -765,7 +752,7 @@ __log_error(unsigned int bank, bool deferred_err, bool threshold_err, u64 misc)
>  
>  	WARN_ON_ONCE(deferred_err && threshold_err);
>  
> -	if (deferred_err && mce_flags.smca) {
> +	if (deferred_err && use_smca_destat) {
>  		msr_status = MSR_AMD64_SMCA_MCx_DESTAT(bank);
>  		msr_addr = MSR_AMD64_SMCA_MCx_DEADDR(bank);
>  	}
> @@ -807,6 +794,10 @@ __log_error(unsigned int bank, bool deferred_err, bool threshold_err, u64 misc)
>  
>  	mce_log(&m);
>  
> +	/* We should still clear MCA_DESTAT even if we used MCA_STATUS. */
> +	if (mce_flags.smca && !use_smca_destat)
> +		wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(bank), 0);
> +
>  	wrmsrl(msr_status, 0);
>  }
>  
> @@ -832,25 +823,29 @@ asmlinkage __visible void __irq_entry smp_trace_deferred_error_interrupt(void)
>  	exiting_ack_irq();
>  }
>  
> +static inline bool check_deferred_status(u64 status)

This function name does not tell me anything.

	if (is_deferred_error(status))

tells me more.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Wed, Mar 22, 2017 at 02:29:31PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Scalable MCA systems have a new MCA_CONFIG register that we use to
> configure each bank. We currently use this when we set up thresholding.
> However, this is logically separate.
> 
> Move setup of MCA_CONFIG into a separate function.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 48 ++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 21 deletions(-)

...

>  /* cpu init entry point, called from mce.c with preempt off */
> @@ -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>  	int offset = -1;
>  
>  	for (bank = 0; bank < mca_cfg.banks; ++bank) {
> -		if (mce_flags.smca)
> +		if (mce_flags.smca) {
>  			get_smca_bank_info(bank);
> +			set_smca_config(bank);

Or simply bundle those two which do something SMCA-aware per bank into a single:

	smca_configure(bank);

which reads almost like a sentence.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

RE: [PATCH 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR} registers

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@suse.de]
> Sent: Monday, March 27, 2017 1:27 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>

...

> >  static void
> > -__log_error(unsigned int bank, bool deferred_err, bool threshold_err,
> > u64 misc)
> > +__log_error(unsigned int bank, bool deferred_err, bool use_smca_destat,
> > +			       bool threshold_err, u64 misc)
> 
> So I had to paste the function signature in a separate vim window and keep
> looking between those arguments' names and the function calls.
> 
> Because if I look at:
> 
> 	__log_error(bank, true, false, false, 0);
> 
> I absolutely have no clue what that code does. So we need to think of
> something better. From the looks of it, I guess we dealt with a single
> __log_error() function long enough. Perhaps it is time for separation:
> 
> log_error_deferred
> log_error_smca
> log_error...
> 
> and have a function __log_error() which all three call to do the work which all
> three share.
> 
> That should make the code readable again IMO. __log_error() as it is now is
> hard to follow anyway.
> 

Okay, will do.

> > @@ -832,25 +823,29 @@ asmlinkage __visible void __irq_entry
> smp_trace_deferred_error_interrupt(void)
> >  	exiting_ack_irq();
> >  }
> >
> > +static inline bool check_deferred_status(u64 status)
> 
> This function name does not tell me anything.
> 
> 	if (is_deferred_error(status))
> 
> tells me more.
> 

Okay.

Thanks,
Yazen

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@suse.de]
> Sent: Tuesday, March 28, 2017 3:23 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Tony Luck <tony.luck@intel.com>;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Wed, Mar 22, 2017 at 02:29:31PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > Scalable MCA systems have a new MCA_CONFIG register that we use to
> > configure each bank. We currently use this when we set up thresholding.
> > However, this is logically separate.
> >
> > Move setup of MCA_CONFIG into a separate function.
> >
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> >  arch/x86/kernel/cpu/mcheck/mce_amd.c | 48
> > ++++++++++++++++++++----------------
> >  1 file changed, 27 insertions(+), 21 deletions(-)
> 
> ...
> 
> >  /* cpu init entry point, called from mce.c with preempt off */ @@
> > -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
> >  	int offset = -1;
> >
> >  	for (bank = 0; bank < mca_cfg.banks; ++bank) {
> > -		if (mce_flags.smca)
> > +		if (mce_flags.smca) {
> >  			get_smca_bank_info(bank);
> > +			set_smca_config(bank);
> 
> Or simply bundle those two which do something SMCA-aware per bank into a
> single:
> 
> 	smca_configure(bank);
> 
> which reads almost like a sentence.
> 

I'd like to keep the functions separate since they're logically independent. I can
define something like smca_configure() as a wrapper function that can contain
current and future SMCA related functions. Is this okay?

Thanks,
Yazen

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Tue, Apr 04, 2017 at 01:34:42PM +0000, Ghannam, Yazen wrote:
> I'd like to keep the functions separate since they're logically independent. I can
> define something like smca_configure() as a wrapper function that can contain
> current and future SMCA related functions. Is this okay?

Are you planning to call the one and not the other in some path?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, April 04, 2017 9:46 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Tony Luck <tony.luck@intel.com>;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Tue, Apr 04, 2017 at 01:34:42PM +0000, Ghannam, Yazen wrote:
> > I'd like to keep the functions separate since they're logically
> > independent. I can define something like smca_configure() as a wrapper
> > function that can contain current and future SMCA related functions. Is this
> okay?
> 
> Are you planning to call the one and not the other in some path?

No, not at the moment. This patch is just to help with readability.

Thanks,
Yazen

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Tue, Apr 04, 2017 at 02:37:34PM +0000, Ghannam, Yazen wrote:
> No, not at the moment. This patch is just to help with readability.

Then no need to have two separate functions just because stuff is
logically independent. Add a comment over the SMCA configuration part in
case we want to carve it out later.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, April 04, 2017 11:01 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Tony Luck <tony.luck@intel.com>;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Tue, Apr 04, 2017 at 02:37:34PM +0000, Ghannam, Yazen wrote:
> > No, not at the moment. This patch is just to help with readability.
> 
> Then no need to have two separate functions just because stuff is logically
> independent. Add a comment over the SMCA configuration part in case we
> want to carve it out later.
> 

Okay, will do.

Thanks,
Yazen