Re: [PATCH 4/6] x86-mce: Add spinlocks to prevent duplicated MCP and CMCI reports.

[Borislav Petkov <bp@alien8.de>]

On Thu, Jul 10, 2014 at 09:12:24PM +0200, Borislav Petkov wrote:
> I'll think about it more tomorrow - my brain is twisted enough for
> today. :-)

Ok, new day, new luck. :-)

So, following yesterday's discussion, our problem is IMHO that shared
banks could be read multiple times before they're finally cleared,
leading to repeated MCE records.

Now, staring at machine_check_poll, the processing is controlled by one
bit - MCI_STATUS_VAL - which decides what happens next.

So how about we change processing around this one bit: we let only one
reader access MSR_IA32_MCx_STATUS(i) and clear it right afterwards by
saving its contents to m.status previously.

Concurrent callers of machine_check_poll will not read the MCI_STATUS
MSR and since they look at the local copy m.status which is 0, they'll
go to the next bank.

And this for the cost of a locked CMPXCHG when we have to inc
poll_reader which should be cheaper than disabling IRQs everytime.

I.e., something like that. Hmm...

---
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 09edd0b65fef..5483b507025a 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -19,6 +19,7 @@ struct mce_bank {
 	unsigned char init;				/* initialise bank? */
 	struct device_attribute attr;			/* device attribute */
 	char			attrname[ATTR_LEN];	/* attribute name */
+	atomic_t		poll_reader;		/* sync for polled shared banks */
 };
 
 int mce_severity(struct mce *a, int tolerant, char **msg);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index bb92f38153b2..443861da86e4 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -609,9 +609,20 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		m.addr = 0;
 		m.bank = i;
 		m.tsc = 0;
+		m.status = 0;
 
 		barrier();
-		m.status = mce_rdmsrl(MSR_IA32_MCx_STATUS(i));
+
+		if (atomic_add_unless(&mce_banks[i].poll_reader, 1, 1)) {
+			m.status = mce_rdmsrl(MSR_IA32_MCx_STATUS(i));
+
+			if (m.status & MCI_STATUS_VAL)
+				/* clear status register for this bank */
+				mce_wrmsrl(MSR_IA32_MCx_STATUS(i), 0);
+
+			atomic_dec(&mce_banks[i].poll_reader);
+		}
+
 		if (!(m.status & MCI_STATUS_VAL))
 			continue;
 
@@ -637,17 +648,12 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		if (!(flags & MCP_DONTLOG) && !mca_cfg.dont_log_ce)
 			mce_log(&m);
 
-		/*
-		 * Clear state for this bank.
-		 */
-		mce_wrmsrl(MSR_IA32_MCx_STATUS(i), 0);
 	}
 
 	/*
 	 * Don't clear MCG_STATUS here because it's only defined for
 	 * exceptions.
 	 */
-
 	sync_core();
 }
 EXPORT_SYMBOL_GPL(machine_check_poll);



-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--