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

Hidetoshi Seto
2009-04-08T08:20:41Z

Andi Kleen wrote:
> From: Huang Ying <ying.huang@intel.com>
>
> Return rip/cs if MCG_STATUS_EIPV is set in mce_get_rip(). Remain m->cs
> if RIP is read from rip_msr.

It means we use "Error IP" as "Return IP" if RIPV=0 but EIPV=1 ...?
Sounds strange.

>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>
> ---
> arch/x86/kernel/cpu/mcheck/mce_64.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> Index: linux/arch/x86/kernel/cpu/mcheck/mce_64.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/cpu/mcheck/mce_64.c 2009-04-07 16:09:59.000000000 +0200
> +++ linux/arch/x86/kernel/cpu/mcheck/mce_64.c 2009-04-07 16:43:15.000000000 +0200
> @@ -175,7 +175,7 @@
>
> static inline void mce_get_rip(struct mce *m, struct pt_regs *regs)
> {
> - if (regs && (m->mcgstatus & MCG_STATUS_RIPV)) {
> + if (regs && (m->mcgstatus & (MCG_STATUS_RIPV | MCG_STATUS_EIPV))) {
> m->ip = regs->ip;
> m->cs = regs->cs;
> } else {
> @@ -186,7 +186,6 @@
> /* Assume the RIP in the MSR is exact. Is this true? */
> m->mcgstatus |= MCG_STATUS_EIPV;

Why this "forcing EIPV=1" still required?
I think remaining this line will make something wrong.

> rdmsrl(rip_msr, m->ip);
> - m->cs = 0;
> }
> }

The mce_get_rip() is called from inside of a for-loop.
And assume that we start with RIPV=0 and EIPV=0:

Before applying this patch:

if (rip_msr) { (m->ip, m->cs) = ((data from msr), 0); }
else { (m->ip, m->cs) = (0, 0); }

And After:

1st call:
if (rip_msr) { (m->ip, m->cs) = ((data from msr), 0); }
else { (m->ip, m->cs) = (0, 0); }
2nd call and later:
if (rip_msr) { (m->ip, m->cs) = ((data from msr), regs->cs); }
else { (m->ip, m->cs) = (0, 0); }

Plus, after applying [3/28] of your patchset for 2.6.31 (that
removes "m->mcgstatus |= MCG_STATUS_EIPV"), it will be again:

if (rip_msr) { (m->ip, m->cs) = ((data from msr), 0); }
else { (m->ip, m->cs) = (0, 0); }

So I bet this patch does not work stand alone.

Given that:

1) the ip retrieved by mce_get_rip() is now only used for input of
mce_log().

2) code in mce_log():

if (m->ip) {
printk(KERN_EMERG "RIP%s %02x:<%016Lx> ",
!(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "",
m->cs, m->ip);
if (m->cs == __KERNEL_CS)
print_symbol("{%s}", m->ip);
printk("\n");
}

3) code in mce_cap_init():

/* Use accurate RIP reporting if available. */
if ((cap & MCG_EXT_P) && (MCG_NUM_EXT(cap) >= 9))
rip_msr = MSR_IA32_MCG_EIP;

I guess it would make much sense if we stop mixing RIP and EIP and rename
the mce_get_rip() to mce_get_eip(), and the rip_msr to eip_msr too.

And then it would be acceptable if we print RIP with "!INEXACT!" annotation
instead of printing precise EIP in case of RIPV=0 but EIPV=1.

Thanks,
H.Seto