Fix handling of NMIs from unknown sources (BMC, hypervisor)
ClosedPublic
Actions

Authored by vangyzen on Apr 24 2020, 4:45 PM.

Details

Reviewers

kib
avg
jhb

Commits

rS360328: Fix handling of NMIs from unknown sources (BMC, hypervisor)

Summary

Release kernels have no KDB backends enabled, so they discard an NMI
if it is not due to a hardware failure. This includes NMIs from
IPMI BMCs and hypervisors.

Furthermore, the interaction of panic_on_nmi, kdb_on_nmi, and
debugger_on_panic is confusing.

Respond to all NMIs according to panic_on_nmi and debugger_on_panic.
Remove kdb_on_nmi. Expand the meaning of panic_on_nmi by making
it a bitfield. There are currently two bits, one for NMIs due to
hardware failure, and one for all others. Leave room for more.

If panic_on_nmi and debugger_on_panic are both true, don't actually panic,
but directly enter the debugger, to allow someone to leave the debugger
and [hopefully] resume normal execution.

Test Plan

I tested on bhyve with bhyvectl --inject-nmi using all combinations
of the relevant sysctls.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30722
Build 28450: arc lint + arc unit

Event Timeline

vangyzen created this revision.Apr 24 2020, 4:45 PM

Herald added subscribers: dab, badger. · View Herald TranscriptApr 24 2020, 4:45 PM

Harbormaster completed remote builds in B30701: Diff 70945.Apr 24 2020, 4:45 PM

kib added inline comments.Apr 24 2020, 6:21 PM

sys/x86/x86/cpu_machdep.c
829	0xff is unexplainable, why not 3 ?
864	Why keep this line under #ifdef KDB? IMO it would be cleaner have `if (!claimed && panic_on_nmi != 0) panic();` statement unconditional at the very end of the function.

vangyzen marked 2 inline comments as done.Apr 24 2020, 6:45 PM

vangyzen added inline comments.

sys/x86/x86/cpu_machdep.c
829	I'm allowing for future expansion without further churn in the interface. For example, we can query the IPMI BMC to determine whether an NMI is due to a watchdog timeout. We could use another bit to distinguish those. If an admin wants the box to panic for all NMIs, even those we don't distinguish yet, he/she can set this to 0xff.
864	Effectively, this is the final `!claimed && panic_on_nmi` case. I suppose I could remove the `& 2` from the condition. If I move the `!claimed` after the `#endif`, I can imagine some analysis tools complaining that the test is useless in the `!KDB` case, which it is.

kib added inline comments.Apr 25 2020, 12:43 PM

sys/x86/x86/cpu_machdep.c
864	Well, no, I like it with `& 2`, and I think it should stay this way. IMO a deficiency in the analysis tool should not preclude us from structuring code in the natural way. This pattern of compiling out something that makes the code looks strange after processing all ifdefs is too common.

follow kib's suggestion

Harbormaster completed remote builds in B30722: Diff 70983.Apr 25 2020, 1:44 PM

vangyzen marked an inline comment as done.Apr 25 2020, 1:46 PM

kib added inline comments.Apr 25 2020, 2:28 PM

sys/x86/x86/cpu_machdep.c
865	I meant to put this (!claimed && panic_on_nmi !=) after the block for '2', i.e. after the closing brace at line 867. In this form indeed it is somewhat strange.