Re: [PATCH] [3/4] x86: MCE: Improve mce_get_rip

[Huang Ying <ying.huang@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]

Add some description for the patch, hope that to be more clear.

Best Regards,
Huang Ying
--------------------------------------------->
mce_get_rip() is used to get IP when MCE is generated, usually from
the stack. But the IP on the stack is not always valid.
MCG_STATUS_RIPV indicates program can restart from the IP on the stack,
so if it is set, the IP is valid. MCG_STATUS_EIPV indicate IP on the
stack is directly associated with the error, so if it is set, the IP
is valid too.

In current implementation, no IP will be returned (and then reported)
if MCG_STATUS_RIPV is not set and MCG_STATUS_EIPV is set. This patch
fixes this issue by returning IP on the stack when MCG_STATUS_EIPV is
set.

In some CPU, a MSR (rip_msr) provides another way to get IP when MCE
is generated. This is used by mce_get_rip() too.

There is no MSR for CS, in current implementation, if rip_msr is used
to get IP, reported CS is set to 0. But in fact, the CS on the stack
can be trusted if MCG_STATUS_RIPV or MCG_STATUS_EIPV is set. This
patch fixes this issue by keeping reported CS when rip_msr is used.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---
 arch/x86/kernel/cpu/mcheck/mce_64.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

--- a/arch/x86/kernel/cpu/mcheck/mce_64.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_64.c
@@ -173,21 +173,22 @@ int mce_available(struct cpuinfo_x86 *c)
 	return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
 }
 
+/*
+ * Get the address of instruction at the time of the machine check error.
+ */
 static inline void mce_get_rip(struct mce *m, struct pt_regs *regs)
 {
-	if (regs && (m->mcgstatus & MCG_STATUS_RIPV)) {
+	/* Use value on the stack if it is meaningful. */
+	if (regs && (m->mcgstatus & (MCG_STATUS_RIPV | MCG_STATUS_EIPV))) {
 		m->ip = regs->ip;
 		m->cs = regs->cs;
 	} else {
 		m->ip = 0;
 		m->cs = 0;
 	}
-	if (rip_msr) {
-		/* Assume the RIP in the MSR is exact. Is this true? */
-		m->mcgstatus |= MCG_STATUS_EIPV;
+	/* Use accurate value if available. */
+	if (rip_msr)
 		rdmsrl(rip_msr, m->ip);
-		m->cs = 0;
-	}
 }
 
 /*
@@ -569,7 +570,10 @@ static int mce_cap_init(void)
 		memset(bank, 0xff, banks * sizeof(u64));
 	}
 
-	/* Use accurate RIP reporting if available. */
+	/*
+	 * Use Extended Machine Check State Register to get accurate state of
+	 * the RIP register at the time of the machine check if available.
+	 */
 	if ((cap & (1<<9)) && ((cap >> 16) & 0xff) >= 9)
 		rip_msr = MSR_IA32_MCG_EIP;
 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]