Messages in this thread | | | Date | Thu, 12 Nov 2015 11:55:52 -0800 | From | "Luck, Tony" <> | Subject | Re: [PATCH 2/3] x86, ras: Extend machine check recovery code to annotated ring0 areas |
| |
On Wed, Nov 11, 2015 at 08:19:35PM -0800, Andy Lutomirski wrote: > >@@ -1132,9 +1133,15 @@ void do_machine_check(struct pt_regs *regs, long error_code) > > if (no_way_out) > > mce_panic("Fatal machine check on current CPU", &m, msg); > > if (worst == MCE_AR_SEVERITY) { > >- recover_paddr = m.addr; > >- if (!(m.mcgstatus & MCG_STATUS_RIPV)) > >- flags |= MF_MUST_KILL; > >+ if ((m.cs & 3) == 3) { > >+ recover_paddr = m.addr; > >+ if (!(m.mcgstatus & MCG_STATUS_RIPV)) > >+ flags |= MF_MUST_KILL; > >+ } else if (fixup_mcexception(regs)) { > >+ regs->ax = BIT(63) | m.addr; > >+ } else > >+ mce_panic("Failed kernel mode recovery", > >+ &m, NULL); > > Maybe I'm misunderstanding this, but presumably you shouldn't call > fixup_mcexception unless you've first verified RIPV (i.e. that the ip you're > looking up in the table is valid).
Good point. We can only arrive here with a AR_SEVERITY from some kernel code if the code in mce_severity.c assigned that severity. But it doesn't currently look at RIPV ... I should make it do that. Actually I'll check for both RIPV and EIPV: we don't need to look for a fixup entry for all the innocent bystander cpus that got dragged into the exception handler because the exception was broadcast to everyone.
> Also... I find the general flow of this code very hard to follow. It's > critical that an MCE hitting kernel mode not get as far as > ist_begin_non_atomic. It was already hard enough to tell that the code > follows that rule, and now it's even harder. Would it make sense to add > clear assertions that m.cs == regs->cs and that user_mode(regs) when you get > to the end? Simplifying the control flow might also be nice.
Yes. This is a mess now. It works (because we only set recover_paddr in the user mode case ... so we'll take the "goto done" for the kernel case). But I agree that this is far from obvious.
> > } else if (kill_it) { > > force_sig(SIGBUS, current); > > } > > > > I would argue that this should happen in the non-atomic section. It's > probably okay as long as we came from user mode, but it's more obviously > safe in the non-atomic section.
Will look at relocating this too when I restructure the tail of the function.
Thanks for the review.
-Tony
| |