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

Groups > linux.kernel > #1340588 > unrolled thread

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

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 2/4] x86/mce/AMD: Fix logic to obtain block address Borislav Petkov <bp@alien8.de> - 2016-02-23 13:40 +0100
    Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> - 2016-02-24 00:00 +0100
      Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address Borislav Petkov <bp@alien8.de> - 2016-02-24 12:40 +0100
        Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address Boris Petkov <bp@alien8.de> - 2016-02-24 21:40 +0100

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

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.

[toc] | [next] | [standalone]

#1341152

On 2/23/16 6:39 AM, Borislav Petkov wrote:
> On Tue, Feb 16, 2016 at 03:45:09PM -0600, Aravind Gopalakrishnan wrote:
>>   
>>   /* '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.

I think MSR_AMD64_SMCA_MC0_MISC0 would be required in mce.c as well.
So might be better to retain it here.

MSR_AMD64_SMCA_MC0_MISC1 might be required only in mce_amd.c, So, I'll 
move it to mce.h

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

Will fix.

>> +			u32 smca_low, smca_high;
> s/smca_//

Will fix.

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

Will fix it like so-
addr = MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);

(It comes up to 81 chars, but will ignore checkpatch in this case..)

Thanks,
-Aravind.

[toc] | [prev] | [next] | [standalone]

#1341920

On Tue, Feb 23, 2016 at 04:56:38PM -0600, Aravind Gopalakrishnan wrote:
> I think MSR_AMD64_SMCA_MC0_MISC0 would be required in mce.c as well.
> So might be better to retain it here.

Actually, I'm thinking, these all are - even if used in multiple files
- all MCE-specific and therefore used in MCE/RAS-specific code. So they
all should go into mce.h. Everything RAS includes that header so they're
perfectly fine there...

> (It comes up to 81 chars, but will ignore checkpatch in this case..)

The 80-cols rule is not a hard one. Here's some food for thought:

https://lkml.kernel.org/r/20160219095132.GA9681@gmail.com

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1342433

Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:

>Should I change that as well and move it to mce.h?

Yes please. 


-- 
Sent from a small device: formatting sux and brevity is inevitable. 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web