Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Borislav Petkov <bp@alien8.de>]

On Tue, Feb 16, 2016 at 03:45:09PM -0600, Aravind Gopalakrishnan wrote:
> In upcoming processors, the BLKPTR field is no longer used
> to indicate the MSR number of the additional register.
> Insted, it simply indicates the prescence of additional MSRs.
> 
> Fixing the logic here to gather MSR address from
> MSR_AMD64_SMCA_MCx_MISC() for newer processors
> and we fall back to existing logic for older processors.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/include/asm/msr-index.h     |  4 ++
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 94 +++++++++++++++++++++++++-----------
>  2 files changed, 69 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 93bccbc..ca49e928e 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -265,10 +265,14 @@
>  #define MSR_IA32_MCx_CTL2(x)		(MSR_IA32_MC0_CTL2 + (x))
>  
>  /* 'SMCA': AMD64 Scalable MCA */
> +#define MSR_AMD64_SMCA_MC0_MISC0	0xc0002003
>  #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
>  #define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
> +#define MSR_AMD64_SMCA_MC0_MISC1	0xc000200a
> +#define MSR_AMD64_SMCA_MCx_MISC(x)	(MSR_AMD64_SMCA_MC0_MISC0 + 0x10*(x))
>  #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
>  #define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_MISCy(x, y)	((MSR_AMD64_SMCA_MC0_MISC1 + y) + (0x10*(x)))

Are those MSRs going to be used in multiple files? If not, they should
all go to mce.h.

>  #define MSR_P6_PERFCTR0			0x000000c1
>  #define MSR_P6_PERFCTR1			0x000000c2
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 8169103..4bdc836 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -286,6 +286,58 @@ static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
>  	wrmsr(MSR_CU_DEF_ERR, low, high);
>  }
>  
> +static u32 get_block_address(u32 current_addr,
> +			     u32 low,
> +			     u32 high,
> +			     unsigned int bank,
> +			     unsigned int block)

Use arg formatting like the rest of functions in the file please.

> +{
> +	u32 addr = 0, offset = 0;
> +
> +	if (mce_flags.smca) {
> +		if (!block) {
> +			addr = MSR_AMD64_SMCA_MCx_MISC(bank);
> +		} else {
> +			/*
> +			 * For SMCA enabled processors, BLKPTR field
> +			 * of the first MISC register (MCx_MISC0) indicates
> +			 * presence of additional MISC register set (MISC1-4)
> +			 */
> +			u32 smca_low, smca_high;

s/smca_//

> +
> +			if (rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(bank),
> +				       &smca_low, &smca_high) ||
> +			    !(smca_low & MCI_CONFIG_MCAX))
> +				goto nextaddr_out;
> +
> +			if (!rdmsr_safe(MSR_AMD64_SMCA_MCx_MISC(bank),
> +					&smca_low, &smca_high) &&
> +			    (smca_low & MASK_BLKPTR_LO))
> +				addr = MSR_AMD64_SMCA_MCx_MISCy(bank,
> +								block - 1);

unnecessary line break.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.