Re: [PATCH RFC] NMI Re-introduce un[set]_nmi_callback

[Ingo Molnar <mingo@elte.hu>]

* Andi Kleen <andi@firstfloor.org> wrote:

> > Add "kdump" to the list. It will also be broken if we decide to let 
> > one driver hijack the NMI handler.
> 
> kdump is a special case, similar to the NMI button panic mode. It 
> should be always only active when the user configured it. When the 
> user configured it should be always the fallback and override any 
> other drivers.

if by 'any other drivers' you mean all other notifiers then that's wrong 
- kdump must still come after many other NMI sources.

Basically, the sane order is this:

highest priority:
 
  instruction patching callbacks. (kprobes, mmiotrace, kmemcheck) These 
  are abstractions that are essential for the kernel to properly 
  function/execute. We dont ever want them to be overriden.

high priority:

  CPU-generated profiling callbacks. (nmi-lapic watchdog, performance 
  counter generated NMIs) These are 'good' NMI sources that can (well, 
  'could') identify themselves, and they can also come in very 
  frequently so we want to execute them early.

mid priority:

  optional/interactive debug facilities. (kdump, KGDB, trace dump, NMI 
  button).
  The user enables them optionally and wants them catch all non-expected 
  or interactive NMI events.

normal priority:

  various platform drivers. Infrequent NMI sources. It's what we use to 
  make unexpected events slightly more informative when the user does 
  not configure any explicit debugging helper.

lowest priority:

  fallback legacy platform handlers - 61H reads, etc.

All in one, the patch submitted here is wrong as a generic facility. One 
valid aspect of the patch is that the port 61H reads (and other 
architecture code chipset ops) should execute as a regular notifier and 
with low priority.

as it does not really solve anything in a structured way, it allows 
platform drivers to install a super-high priority notifier, creating 
needless duplication and confusion. The exact reasons for the changes 
should be listed instead and proper (and separate) patches done for each 
reason, along the suggestions in this thread - i believe all issues were 
covered in one way or another.

	Ingo