[PATCH 0/5] Updates to AMD MCE driver per Scalable MCA spec

[PATCH 0/5] Updates to AMD MCE driver per Scalable MCA spec

[Aravind Gopalakrishnan]

The patchset contains updates to the MCE driver based
on the Scalable MCA specification.

Patches 1-3 include some minor changes to existing code
and have been tested for regressions on older families.

Patches 4-5 is new code and only runs on processors
with ScalableMCA feature enabled (for future)

Patch 1: Order of mce_amd_feature_init() was incorrect as
	 it should be called after we gather features from
	 cpuid bits. Fixing that in this patch
Patch 2: We do not require shared bank verification on ZP.
	 Modifying code here to return early if we are on a processor
	 that supports SMCA feature.
Patch 3: The number of blocks per bank is reduced from Fam17h onwards.
	 Fixing code to reflect this architectural change
Patch 4: LVT offset for thresholding is now programmed in different MSR
	 as opposed to per-bank MISC register in earlier processors.
	 Fixing code here to obtain LVT offset from correct MSR.
Patch 5: OS is required to set MCAXEn bit in the per-bank CONFIG MSR
	 to acknowledge the use of new MSR range for MCA.
	 Doing that here and also creating definitions for the new
	 MSR range in msr-index.
	 
Note: checkpatch generates warnings for Patch 5. But I have not
wrapped text around the character limit as it looked ugly.
(I overshot it by only a character or two)

Aravind Gopalakrishnan (5):
  x86, mce: Fix order of AMD MCE init function call
  x86/mcheck/AMD: Do not perform shared bank check for future processors
  x86/mcheck/AMD: Reduce number of blocks scanned per bank
  x86/mcheck/AMD: Fix LVT offset configuration for thresholding
  x86/mcheck/AMD: Set MCAX Enable bit

 arch/x86/include/asm/msr-index.h     | 23 +++++++++++++++++
 arch/x86/kernel/cpu/mcheck/mce.c     |  2 +-
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 50 ++++++++++++++++++++++++++++++++++--
 3 files changed, 72 insertions(+), 3 deletions(-)

-- 
2.7.0

[PATCH 1/5] x86, mce: Fix order of AMD MCE init function call

[Aravind Gopalakrishnan]

In mce_amd_feature_init() we take decisions based
on mce_flags being set or not. So the feature
detection using cpuid should naturally be ordered before
we call mce_amd_feature_init()

Fixing that here.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a006f4c..b718080 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1617,10 +1617,10 @@ static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
 	case X86_VENDOR_AMD: {
 		u32 ebx = cpuid_ebx(0x80000007);
 
-		mce_amd_feature_init(c);
 		mce_flags.overflow_recov = !!(ebx & BIT(0));
 		mce_flags.succor	 = !!(ebx & BIT(1));
 		mce_flags.smca		 = !!(ebx & BIT(3));
+		mce_amd_feature_init(c);
 
 		break;
 		}
-- 
2.7.0

[PATCH 2/5] x86/mcheck/AMD: Do not perform shared bank check for future processors

[Aravind Gopalakrishnan]

Fam17h and above should not require a check to see if a bank
is shared or not. For shared banks, there will always be only
one core that has visibility over the MSRs and only that
particular core will be allowed to write to the MSRs

Fixing the code to return early if we detect Fam17h or above.
No change in functionality for earlier processors

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index e99b150..da570a8 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -84,6 +84,14 @@ struct thresh_restart {
 
 static inline bool is_shared_bank(int bank)
 {
+	/*
+	 * For Fam17h and above, we shouldn't require this check.
+	 * Only the core that can see valid values on the MSRs has
+	 * control over the respective MCA bank
+	 */
+	if (mce_flags.smca)
+		return 0;
+
 	/* Bank 4 is for northbridge reporting and is thus shared */
 	return (bank == 4);
 }
-- 
2.7.0

[PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Aravind Gopalakrishnan]

>From Fam17h onwards, the number of extended MISC register
blocks is reduced to 4. It is an architectural change
from what we had on earlier processors.

Changing the value of NRBLOCKS here to reflect that change.

Although theoritically the total number of extended MCx_MISC
registers was 8 in earlier processor families, in practice
we only had to use the extra registers for MC4. And only 2 of
those were used. So this change does not affect older processors.
Tested it on Fam10h, Fam15h systems and works fine.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index da570a8..e650fdc 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -28,7 +28,7 @@
 #include <asm/msr.h>
 #include <asm/trace/irq_vectors.h>
 
-#define NR_BLOCKS         9
+#define NR_BLOCKS         5
 #define THRESHOLD_MAX     0xFFF
 #define INT_TYPE_APIC     0x00020000
 #define MASK_VALID_HI     0x80000000
-- 
2.7.0

Re: [PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Borislav Petkov]

On Thu, Jan 14, 2016 at 04:05:38PM -0600, Aravind Gopalakrishnan wrote:
> From Fam17h onwards, the number of extended MISC register
> blocks is reduced to 4. It is an architectural change
> from what we had on earlier processors.
> 
> Changing the value of NRBLOCKS here to reflect that change.
> 
> Although theoritically the total number of extended MCx_MISC
> registers was 8 in earlier processor families, in practice
> we only had to use the extra registers for MC4. And only 2 of
> those were used. So this change does not affect older processors.
> Tested it on Fam10h, Fam15h systems and works fine.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index da570a8..e650fdc 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -28,7 +28,7 @@
>  #include <asm/msr.h>
>  #include <asm/trace/irq_vectors.h>
>  
> -#define NR_BLOCKS         9
> +#define NR_BLOCKS         5

This doesn't look necessary to me. We do check MCi_MISC[BlkPtr] before
accessing that MSR.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Aravind Gopalakrishnan]

On 1/14/2016 4:37 PM, Borislav Petkov wrote:
> On Thu, Jan 14, 2016 at 04:05:38PM -0600, Aravind Gopalakrishnan wrote:
>>   
>> -#define NR_BLOCKS         9
>> +#define NR_BLOCKS         5
> This doesn't look necessary to me. We do check MCi_MISC[BlkPtr] before
> accessing that MSR.
>

True. But that BlkPtr logic also will undergo changes as it's 
interpretation for future processors is different.

We are guaranteed to have all the MISC registers (all 5 of them) going 
forward.
But we shouldn't be accessing MSRs beyond the 5th extended MISC register 
for each bank as that is the architectural boundary.
Hence the change here.

Thanks,
-Aravind.

Re: [PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Borislav Petkov]

On Thu, Jan 14, 2016 at 04:48:22PM -0600, Aravind Gopalakrishnan wrote:
> True. But that BlkPtr logic also will undergo changes as it's interpretation
> for future processors is different.

But there still must be a bit there which says "this register is valid",
like MCi_MISC[63].

And so I'd very much prefer checking a bit (or bits) instead of relying
on defines.

> We are guaranteed to have all the MISC registers (all 5 of them) going
> forward.

Guarantees are worth nothing.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Aravind Gopalakrishnan]

On 1/14/2016 4:53 PM, Borislav Petkov wrote:
> On Thu, Jan 14, 2016 at 04:48:22PM -0600, Aravind Gopalakrishnan wrote:
>> True. But that BlkPtr logic also will undergo changes as it's interpretation
>> for future processors is different.
> But there still must be a bit there which says "this register is valid",
> like MCi_MISC[63].

There is a bit to say if it's valid or not.

> And so I'd very much prefer checking a bit (or bits) instead of relying
> on defines.
>
>

But we'd still need to know the last available MISC register for a bank 
to know when to end the loop right?

Currently we loop over all the possible blocks-
  for (block = 0; block < NR_BLOCKS; ++block) {
  <code>...

  increment block address;

  <code>..
  }

Here, we know we have to stop at block number 8 as that is the last MISC 
register that is present for a bank.
In the same manner, we'd still have to know the last possible MISC 
register for future processors..

Thanks,
-Aravind.

Re: [PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Borislav Petkov]

On Thu, Jan 14, 2016 at 05:08:30PM -0600, Aravind Gopalakrishnan wrote:
> In the same manner, we'd still have to know the last possible MISC
> register for future processors..

I was going to suggest that we should probably *count* the MISC
registers upfront so that we know exactly how many are we dealing with
instead of relying on macros but that would be overengineering it for no
good reason. And we're checking the valid bits and so on, so we're good.

So ok, I'm persuaded.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Gopalakrishnan, Aravind]

On 1/15/2016 5:14 AM, Borislav Petkov wrote:
>
> So ok, I'm persuaded.
>
> Thanks.
>

Okay, I'll send V2 with the changes on the other patches.

Thanks,

-Aravind.

Re: [PATCH 3/5] x86/mcheck/AMD: Reduce number of blocks scanned per bank

[Aravind Gopalakrishnan]

On 1/15/2016 5:14 AM, Borislav Petkov wrote:
> And we're checking the valid bits and so on, so we're good.
>
> So ok, I'm persuaded.
>
> Thanks.
>

Ok, I'll send V2 with the changes.

Thanks,
-Aravind.

[PATCH 4/5] x86/mcheck/AMD: Fix LVT offset configuration for thresholding

[Aravind Gopalakrishnan]

For processor families with  SMCA feature, the LVT offset
for threshold interrupts is configured only in MSR 0xC0000410
and not in each per bank MISC register as was done in earlier
families.

Fixing the code here to obtain the LVT offset from the correct
MSR for those families which have SMCA feature enabled.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index e650fdc..4383d75 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -49,6 +49,9 @@
 #define DEF_LVT_OFF		0x2
 #define DEF_INT_TYPE_APIC	0x2
 
+/* SMCA settings */
+#define SMCA_THR_LVT_OFF	0xF000
+
 static const char * const th_names[] = {
 	"load_store",
 	"insn_fetch",
@@ -143,6 +146,15 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
 	}
 
 	if (apic != msr) {
+		/*
+		 * For SMCA enabled processors, LVT offset is programmed at
+		 * different MSR and BIOS provides the value.
+		 * The original field where LVT offset was set is Reserved.
+		 * So, return early here.
+		 */
+		if (mce_flags.smca)
+			return 0;
+
 		pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
 		       "for bank %d, block %d (MSR%08X=0x%x%08x)\n",
 		       b->cpu, apic, b->bank, b->block, b->address, hi, lo);
@@ -301,7 +313,21 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 				goto init;
 
 			b.interrupt_enable = 1;
-			new	= (high & MASK_LVTOFF_HI) >> 20;
+
+			if (mce_flags.smca) {
+				u32 smca_low = 0, smca_high = 0;
+
+				/* Gather LVT offset for thresholding */
+				if (rdmsr_safe(MSR_CU_DEF_ERR,
+					       &smca_low,
+					       &smca_high))
+					break;
+
+				new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
+			} else {
+				new	= (high & MASK_LVTOFF_HI) >> 20;
+			}
+
 			offset  = setup_APIC_mce_threshold(offset, new);
 
 			if ((offset == new) &&
-- 
2.7.0

[PATCH 5/5] x86/mcheck/AMD: Set MCAX Enable bit

[Aravind Gopalakrishnan]

It is required for OS to acknowledge that it is using
the MCAX register set and its associated fields by setting
the 'McaXEnable' bit in each bank's MCi_CONFIG register. If
it is not set, then all UC errors will cause a system panic.

So setting the bit here and also defining the new MSR range for
SMCA enabled proccessors in msr-index

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/include/asm/msr-index.h     | 23 +++++++++++++++++++++++
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 12 ++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index b05402e..88505f8 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -264,6 +264,29 @@
 #define MSR_IA32_MC0_CTL2		0x00000280
 #define MSR_IA32_MCx_CTL2(x)		(MSR_IA32_MC0_CTL2 + (x))
 
+/* SMCA defined MSR register set for AMD64 */
+#define MSR_AMD64_SMCA_MC0_CTL		0xc0002000
+#define MSR_AMD64_SMCA_MC0_STATUS	0xc0002001
+#define MSR_AMD64_SMCA_MC0_ADDR		0xc0002002
+#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_SYND		0xc0002006
+#define MSR_AMD64_SMCA_MC0_DESTAT	0xc0002008
+#define MSR_AMD64_SMCA_MC0_DEADDR	0xc0002009
+#define MSR_AMD64_SMCA_MC0_MISC1	0xc000200a
+
+#define MSR_AMD64_SMCA_MCx_CTL(x)	(MSR_AMD64_SMCA_MC0_CTL + 0x10*(x))
+#define MSR_AMD64_SMCA_MCx_STATUS(x)	(MSR_AMD64_SMCA_MC0_STATUS + 0x10*(x))
+#define MSR_AMD64_SMCA_MCx_ADDR(x)	(MSR_AMD64_SMCA_MC0_ADDR + 0x10*(x))
+#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_SYND(x)	(MSR_AMD64_SMCA_MC0_SYND + 0x10*(x))
+#define MSR_AMD64_SMCA_MCx_DESTAT(x)	(MSR_AMD64_SMCA_MC0_DESTAT + 0x10*(x))
+#define MSR_AMD64_SMCA_MCx_DEADDR(x)	(MSR_AMD64_SMCA_MC0_DEADDR + 0x10*(x))
+#define MSR_AMD64_SMCA_MCx_MISCy(x, y)	((MSR_AMD64_SMCA_MC0_MISC1 + y) + (0x10*(x)))
+
 #define MSR_P6_PERFCTR0			0x000000c1
 #define MSR_P6_PERFCTR1			0x000000c2
 #define MSR_P6_EVNTSEL0			0x00000186
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 4383d75..ae6fcca 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -51,6 +51,7 @@
 
 /* SMCA settings */
 #define SMCA_THR_LVT_OFF	0xF000
+#define SMCA_MCAX_EN_OFF	0x1
 
 static const char * const th_names[] = {
 	"load_store",
@@ -316,6 +317,17 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 
 			if (mce_flags.smca) {
 				u32 smca_low = 0, smca_high = 0;
+				u32 smca_addr = 0;
+
+				/* Set MCAXEnable bit for each bank */
+				smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
+				if (rdmsr_safe(smca_addr,
+					       &smca_low,
+					       &smca_high))
+					continue;
+
+				smca_high = (smca_high & ~SMCA_MCAX_EN_OFF) | 0x1;
+				wrmsr(smca_addr, smca_low, smca_high);
 
 				/* Gather LVT offset for thresholding */
 				if (rdmsr_safe(MSR_CU_DEF_ERR,
-- 
2.7.0

Re: [PATCH 5/5] x86/mcheck/AMD: Set MCAX Enable bit

[Borislav Petkov]

On Thu, Jan 14, 2016 at 04:05:40PM -0600, Aravind Gopalakrishnan wrote:
> It is required for OS to acknowledge that it is using
> the MCAX register set and its associated fields by setting
> the 'McaXEnable' bit in each bank's MCi_CONFIG register. If
> it is not set, then all UC errors will cause a system panic.
> 
> So setting the bit here and also defining the new MSR range for
> SMCA enabled proccessors in msr-index
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/include/asm/msr-index.h     | 23 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 12 ++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index b05402e..88505f8 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -264,6 +264,29 @@
>  #define MSR_IA32_MC0_CTL2		0x00000280
>  #define MSR_IA32_MCx_CTL2(x)		(MSR_IA32_MC0_CTL2 + (x))
>  
> +/* SMCA defined MSR register set for AMD64 */
> +#define MSR_AMD64_SMCA_MC0_CTL		0xc0002000
> +#define MSR_AMD64_SMCA_MC0_STATUS	0xc0002001
> +#define MSR_AMD64_SMCA_MC0_ADDR		0xc0002002
> +#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_SYND		0xc0002006
> +#define MSR_AMD64_SMCA_MC0_DESTAT	0xc0002008
> +#define MSR_AMD64_SMCA_MC0_DEADDR	0xc0002009
> +#define MSR_AMD64_SMCA_MC0_MISC1	0xc000200a
> +
> +#define MSR_AMD64_SMCA_MCx_CTL(x)	(MSR_AMD64_SMCA_MC0_CTL + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_STATUS(x)	(MSR_AMD64_SMCA_MC0_STATUS + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_ADDR(x)	(MSR_AMD64_SMCA_MC0_ADDR + 0x10*(x))
> +#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_SYND(x)	(MSR_AMD64_SMCA_MC0_SYND + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_DESTAT(x)	(MSR_AMD64_SMCA_MC0_DESTAT + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_DEADDR(x)	(MSR_AMD64_SMCA_MC0_DEADDR + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_MISCy(x, y)	((MSR_AMD64_SMCA_MC0_MISC1 + y) + (0x10*(x)))

Please add MSRs only with the respective patch that uses them.

AFAICT, you need to add only MSR_AMD64_SMCA_MCx_CONFIG() here.

> +
>  #define MSR_P6_PERFCTR0			0x000000c1
>  #define MSR_P6_PERFCTR1			0x000000c2
>  #define MSR_P6_EVNTSEL0			0x00000186
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 4383d75..ae6fcca 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -51,6 +51,7 @@
>  
>  /* SMCA settings */
>  #define SMCA_THR_LVT_OFF	0xF000
> +#define SMCA_MCAX_EN_OFF	0x1

SMCA *and* MCAX.

SMCA_EN_OFF is not enough?

>  
>  static const char * const th_names[] = {
>  	"load_store",
> @@ -316,6 +317,17 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>  
>  			if (mce_flags.smca) {
>  				u32 smca_low = 0, smca_high = 0;
> +				u32 smca_addr = 0;
> +
> +				/* Set MCAXEnable bit for each bank */
> +				smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
					     ^^^^^^^^^^^^^^^^^^^^^^^^^


> +				if (rdmsr_safe(smca_addr,
> +					       &smca_low,
> +					       &smca_high))
> +					continue;
> +
> +				smca_high = (smca_high & ~SMCA_MCAX_EN_OFF) | 0x1;

So this can simply be:

				smca_high |= SMCA_MCAX_EN_OFF;

?

> +				wrmsr(smca_addr, smca_low, smca_high);
>  
>  				/* Gather LVT offset for thresholding */
>  				if (rdmsr_safe(MSR_CU_DEF_ERR,

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 5/5] x86/mcheck/AMD: Set MCAX Enable bit

[Aravind Gopalakrishnan]

On 1/14/2016 4:46 PM, Borislav Petkov wrote:
> On Thu, Jan 14, 2016 at 04:05:40PM -0600, Aravind Gopalakrishnan wrote:
>>   
>> +/* SMCA defined MSR register set for AMD64 */
>> +#define MSR_AMD64_SMCA_MC0_CTL		0xc0002000
>> +#define MSR_AMD64_SMCA_MC0_STATUS	0xc0002001
>> +#define MSR_AMD64_SMCA_MC0_ADDR		0xc0002002
>> +#define MSR_AMD64_SMCA_MC0_MISC0	0xc0002003
>> +#define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
>>
>> +
>> +#define MSR_AMD64_SMCA_MCx_CTL(x)	(MSR_AMD64_SMCA_MC0_CTL + 0x10*(x))
>> +#define MSR_AMD64_SMCA_MCx_STATUS(x)	(MSR_AMD64_SMCA_MC0_STATUS + 0x10*(x))
>> +#define MSR_AMD64_SMCA_MCx_ADDR(x)	(MSR_AMD64_SMCA_MC0_ADDR + 0x10*(x))
>> +#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))
>>
> Please add MSRs only with the respective patch that uses them.
>
> AFAICT, you need to add only MSR_AMD64_SMCA_MCx_CONFIG() here.

Ok, Will fix this.

>>   
>>   /* SMCA settings */
>>   #define SMCA_THR_LVT_OFF	0xF000
>> +#define SMCA_MCAX_EN_OFF	0x1
> SMCA *and* MCAX.
>
> SMCA_EN_OFF is not enough?

Well McaX is name of the field in the MSR. I retained the "SMCA" prefix 
as these are all still part of the ScalableMCA changes.
I would prefer if "MCAX" is retained as it is indicative of which bit we 
are touching. So how about just MCAX_EN_OFF ?

>>   
>>
>> +
>> +				smca_high = (smca_high & ~SMCA_MCAX_EN_OFF) | 0x1;
> So this can simply be:
>
> 				smca_high |= SMCA_MCAX_EN_OFF;
>
> ?

Yes. Will fix this.

Thanks,
-Aravind.

Re: [PATCH 5/5] x86/mcheck/AMD: Set MCAX Enable bit

[Borislav Petkov]

On Thu, Jan 14, 2016 at 04:53:58PM -0600, Aravind Gopalakrishnan wrote:
> Well McaX is name of the field in the MSR. I retained the "SMCA" prefix as

What does that McaX mean, btw?

> these are all still part of the ScalableMCA changes.
> I would prefer if "MCAX" is retained as it is indicative of which bit we are
> touching. So how about just MCAX_EN_OFF ?

If we're going to have a bunch of defines belonging to SMCA, then we're
better having them all start with SMCA_ after all, I guess.

But please make sure you have comments over their definitions explaining
what those bits are. When an outsider is reading those patches and SMCA,
MCAX start appearing left and right, his head most likely starts to
spin.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 5/5] x86/mcheck/AMD: Set MCAX Enable bit

[Aravind Gopalakrishnan]

On 1/14/2016 4:58 PM, Borislav Petkov wrote:
> On Thu, Jan 14, 2016 at 04:53:58PM -0600, Aravind Gopalakrishnan wrote:
>> Well McaX is name of the field in the MSR. I retained the "SMCA" prefix as
> What does that McaX mean, btw?

McaX indicates-
* we have MCA MSRs in a new address range
* there are more registers per bank (hence the definitions to registers 
like MCx_IPID, MCx_CONFIG etc)


>> these are all still part of the ScalableMCA changes.
>> I would prefer if "MCAX" is retained as it is indicative of which bit we are
>> touching. So how about just MCAX_EN_OFF ?
> If we're going to have a bunch of defines belonging to SMCA, then we're
> better having them all start with SMCA_ after all, I guess.
>
> But please make sure you have comments over their definitions explaining
> what those bits are. When an outsider is reading those patches and SMCA,
> MCAX start appearing left and right, his head most likely starts to
> spin.
>

Sure, I shall add some comments around the definitions for V2.

Thanks,
-Aravind.