x86/mce: Handle varying MCA bank counts

x86/mce: Handle varying MCA bank counts

[Yazen Ghannam]

> -----Original Message-----
> From: Luck, Tony <tony.luck@intel.com>
> Sent: Friday, July 27, 2018 5:09 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org; bp@suse.de;
> x86@kernel.org
> Subject: Re: [PATCH] x86/mce: Handle varying MCA bank counts
> 
> On Fri, Jul 27, 2018 at 04:40:09PM -0500, Yazen Ghannam wrote:
> > -	/* Don't support asymmetric configurations today */
> > -	WARN_ON(mca_cfg.banks != 0 && b != mca_cfg.banks);
> > -	mca_cfg.banks = b;
> > +	mca_cfg.banks = max(mca_cfg.banks, b);
> 
> Should we change mca_cfg.banks to be a per-cpu variable?
> 
> 	DEFINE_PER_CPU(int, mce_num_banks);
> 
> That would make it easier to make sure the places
> that scan all banks only look at the ones that exist.
> 

I agree and I'd like to do that in future patches. But it'll require changes in
a few other places, and possibly breaking more assumptions in the code.

I'd like to address this issue in a smaller patch and submit it to the stable
branches. And we can redo things as per_cpu going forward.

Thanks,
Yazen
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

x86/mce: Handle varying MCA bank counts

[Luck, Tony]

On Fri, Jul 27, 2018 at 04:40:09PM -0500, Yazen Ghannam wrote:
> -	/* Don't support asymmetric configurations today */
> -	WARN_ON(mca_cfg.banks != 0 && b != mca_cfg.banks);
> -	mca_cfg.banks = b;
> +	mca_cfg.banks = max(mca_cfg.banks, b);

Should we change mca_cfg.banks to be a per-cpu variable?

	DEFINE_PER_CPU(int, mce_num_banks);

That would make it easier to make sure the places
that scan all banks only look at the ones that exist.

-Tony
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

x86/mce: Handle varying MCA bank counts

[Yazen Ghannam]

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

Linux reads MCG_CAP[Count] to find the number of MCA banks visible to a
CPU. Currently, this is assumed to be the same for all CPUs and a
warning is shown if there is a difference. The number of banks is
overwritten with the MCG_CAP[Count] value of each following CPU that
boots.

According to the Intel SDM and AMD APM, the MCG_CAP[Count] value gives
the number of banks that are available to a "processor implementation".
The AMD BKDGs/PPRs further clarify that this value is per core. This
value has historically been the same for every core in the system, but
that is not an architectural requirement.

Future AMD systems may have different MCG_CAP[Count] values per core,
so the assumption that all CPUs will have the same MCG_CAP[Count] value
will no longer be valid.

Also, the first CPU to boot will allocate the struct mce_banks[] array
using the number of banks based on its MCG_CAP[Count] value. The machine
check handler and other functions use the global number of banks to
iterate and index into the mce_banks[] array. So it's possible to use an
out-of-bounds index on an asymmetric system where a following CPU sees a
MCG_CAP[Count] value greater than its predecessors.

For example, CPU0 sees MCG_CAP[Count]=2. It sets mca_cfg.banks=2 and
allocates mce_banks[] with 2 elements. CPU1 sees MCG_CAP[Count]=3 and
sets mca_cfg.banks=3, but mce_banks[] is already allocated and remains
having 2 elements.

Allocate the mce_banks[] array to the maximum number of banks. This will
avoid the potential out-of-bounds index since we cap the value of
mca_cfg.banks to MAX_NR_BANKS.

Set the value of mca_cfg.banks equal to the max of the previous value
and the value for the current CPU. This way mca_cfg.banks will always
represent the max number of banks detected on any CPU in the system.
This will ensure that all CPUs will access all the banks that are
visible to them. A CPU that can access fewer than the max number of
banks will find the registers of the extra banks to be read-as-zero.

Print the number of MCA banks that we're using. Do this in
mcheck_late_init() so that we print the final value after all CPUs have
been initialized.

Get bank count from target CPU when doing injection with mce-inject
module.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce-inject.c | 14 +++++++-------
 arch/x86/kernel/cpu/mcheck/mce.c        | 21 +++++++--------------
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce-inject.c b/arch/x86/kernel/cpu/mcheck/mce-inject.c
index c805a06e14c3..5dda56d56dd3 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-inject.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c
@@ -46,8 +46,6 @@
 static struct mce i_mce;
 static struct dentry *dfs_inj;
 
-static u8 n_banks;
-
 #define MAX_FLAG_OPT_SIZE	4
 #define NBCFG			0x44
 
@@ -567,9 +565,15 @@ static void do_inject(void)
 static int inj_bank_set(void *data, u64 val)
 {
 	struct mce *m = (struct mce *)data;
+	u64 cap;
+	u8 n_banks;
+
+	/* Get bank count on target CPU so we can handle non-uniform values. */
+	rdmsrl_on_cpu(m->extcpu, MSR_IA32_MCG_CAP, &cap);
+	n_banks = cap & MCG_BANKCNT_MASK;
 
 	if (val >= n_banks) {
-		pr_err("Non-existent MCE bank: %llu\n", val);
+		pr_err("MCA bank %llu non-existent on CPU%d\n", val, m->extcpu);
 		return -EINVAL;
 	}
 
@@ -659,10 +663,6 @@ static struct dfs_node {
 static int __init debugfs_init(void)
 {
 	unsigned int i;
-	u64 cap;
-
-	rdmsrl(MSR_IA32_MCG_CAP, cap);
-	n_banks = cap & MCG_BANKCNT_MASK;
 
 	dfs_inj = debugfs_create_dir("mce-inject", NULL);
 	if (!dfs_inj)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 4b767284b7f5..4238c65a0cce 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1479,13 +1479,12 @@ EXPORT_SYMBOL_GPL(mce_notify_irq);
 static int __mcheck_cpu_mce_banks_init(void)
 {
 	int i;
-	u8 num_banks = mca_cfg.banks;
 
-	mce_banks = kcalloc(num_banks, sizeof(struct mce_bank), GFP_KERNEL);
+	mce_banks = kcalloc(MAX_NR_BANKS, sizeof(struct mce_bank), GFP_KERNEL);
 	if (!mce_banks)
 		return -ENOMEM;
 
-	for (i = 0; i < num_banks; i++) {
+	for (i = 0; i < MAX_NR_BANKS; i++) {
 		struct mce_bank *b = &mce_banks[i];
 
 		b->ctl = -1ULL;
@@ -1499,24 +1498,16 @@ static int __mcheck_cpu_mce_banks_init(void)
  */
 static int __mcheck_cpu_cap_init(void)
 {
-	unsigned b;
+	u8 b;
 	u64 cap;
 
 	rdmsrl(MSR_IA32_MCG_CAP, cap);
 
 	b = cap & MCG_BANKCNT_MASK;
-	if (!mca_cfg.banks)
-		pr_info("CPU supports %d MCE banks\n", b);
-
-	if (b > MAX_NR_BANKS) {
-		pr_warn("Using only %u machine check banks out of %u\n",
-			MAX_NR_BANKS, b);
+	if (WARN_ON_ONCE(b > MAX_NR_BANKS))
 		b = MAX_NR_BANKS;
-	}
 
-	/* Don't support asymmetric configurations today */
-	WARN_ON(mca_cfg.banks != 0 && b != mca_cfg.banks);
-	mca_cfg.banks = b;
+	mca_cfg.banks = max(mca_cfg.banks, b);
 
 	if (!mce_banks) {
 		int err = __mcheck_cpu_mce_banks_init();
@@ -2502,6 +2493,8 @@ EXPORT_SYMBOL_GPL(mcsafe_key);
 
 static int __init mcheck_late_init(void)
 {
+	pr_info("Using %d MCE banks\n", mca_cfg.banks);
+
 	if (mca_cfg.recovery)
 		static_branch_inc(&mcsafe_key);