Re: [PATCH 02/31] x86: MCE: Improve mce_get_rip v3

Hidetoshi Seto
2009-05-27T04:29:34Z

Andi Kleen wrote:
> From: Huang Ying <ying.huang@intel.com>
>
> Assume RIP is valid when either EIPV or RIPV are set.

Bad description.
If RIP means "restart IP" that is valid only if RIPV is set,
this sentence doesn't make sense completely.

Followings will be better:
"Assume IP stored on the stack indicates the address of instruction
at the time of the MCE when either EIPV or RIPV are set."

> This influences
> whether the machine check exception handler decides to return or panic.

I suppose you are pointing logics in:

mce_get_rip(&m, regs);
:
panicm = m;
:
/*
* If the EIPV bit is set, it means the saved IP is the
* instruction which caused the MCE.
*/
if (m.mcgstatus & MCG_STATUS_EIPV)
user_space = panicm.ip && (panicm.cs & 3);

/*
* If we know that the error was in user space, send a
* SIGBUS. Otherwise, panic if tolerance is low.
*
* force_sig() takes an awful lot of locks and has a slight
* risk of deadlocking.
*/
if (user_space) {
force_sig(SIGBUS, current);
} else if (panic_on_oops || tolerant < 2) {
mce_panic("Uncorrected machine check",
&panicm, mcestart);
}

So EIPV without RIPV will be no ip and will result in panic,
while expected result is SIGBUS.

> This fixes a test case in the mce-test suite and is more compliant
> to the specification.

It would be better to describe about the test case that need this fix.

> This currently only makes a difference in a artificial testing
> scenario with the mce-test test suite.
>
> Also in addition do not force the RIP to be valid with the exact
> register MSRs.

I think the forced one is EIP:
> - m->mcgstatus |= MCG_STATUS_EIPV;

And please note that it keep use CS on stack even if MSR is available.

I made an alternative patch for this, with no functional change.
Please consider replacing.

Thanks,
H.Seto

> [AK: combination of patches from Huang Ying and Hidetoshi Seto, with
> new description by me]
> 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.c | 13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 249e3cf..3f158d7 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -247,21 +247,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 the 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)) {
> +
> + 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;
> + if (rip_msr)
> m->ip = mce_rdmsrl(rip_msr);
> - m->cs = 0;
> - }
> }
>
> /*