Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1610010 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR}  registers Borislav Petkov <bp@suse.de> - 2017-03-27 19:30 +0200
    RE: [PATCH 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR}  registers "Ghannam, Yazen" <Yazen.Ghannam@amd.com> - 2017-04-04 15:40 +0200

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

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)
-- 

[toc] | [next] | [standalone]

#1616026

> -----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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web