x86: Cleanup NMI handling

[PATCH 0/9] x86: Cleanup NMI handling

Posted by Sohil Mehta 8 months, 3 weeks ago

While reworking the NMI-source patches[1], I spent quite a few days
understanding the existing NMI handling code. The intention of this series is
to fix the inconsistencies and redundancies that I encountered.

This series also includes improved documentation to make the code easier to
follow. It does not introduce any significant functional changes.

I have tried to split the patches logically to make them easier to review. Let
me know if some of them should be combined. The patches are in no particular
order and can be picked up independently. 

The documentation updates in patches (5, 6, 7) are to the best of my ability.
However, I would really appreciate extra eyes to ensure it is precise.

The updated NMI-source patches will follow this series sometime later.

@Ingo, I tried to format the commit references the way you prefer:

  Commit:
    feeaf5512947 ("x86: Move sysctls into arch/x86")

However, checkpatch seems to dislike it, so I reformatted them as below:

  Commit feeaf5512947 ("x86: Move sysctls into arch/x86")

Is there a specific guideline for x86?

[1]: https://lore.kernel.org/lkml/20240709143906.1040477-1-jacob.jun.pan@linux.intel.com/

Sohil Mehta (9):
  x86/nmi: Simplify unknown NMI panic handling
  x86/nmi: Consolidate NMI panic variables
  x86/nmi: Use a macro to initialize NMI descriptors
  x86/nmi: Remove export of local_touch_nmi()
  x86/nmi: Fix comment in unknown NMI handling
  x86/nmi: Improve and relocate NMI handler comments
  x86/nmi: Improve NMI header documentation
  x86/nmi: Clean up NMI selftest
  x86/nmi: Improve NMI duration console print

 arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
 arch/x86/include/asm/x86_init.h |  1 +
 arch/x86/kernel/dumpstack.c     |  2 -
 arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
 arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
 arch/x86/kernel/setup.c         | 37 ++++++--------
 include/linux/panic.h           |  2 -
 7 files changed, 122 insertions(+), 108 deletions(-)

-- 
2.43.0

Re: [PATCH 0/9] x86: Cleanup NMI handling

Posted by H. Peter Anvin 8 months, 2 weeks ago

On March 27, 2025 4:46:20 PM PDT, Sohil Mehta <sohil.mehta@intel.com> wrote:
>While reworking the NMI-source patches[1], I spent quite a few days
>understanding the existing NMI handling code. The intention of this series is
>to fix the inconsistencies and redundancies that I encountered.
>
>This series also includes improved documentation to make the code easier to
>follow. It does not introduce any significant functional changes.
>
>I have tried to split the patches logically to make them easier to review. Let
>me know if some of them should be combined. The patches are in no particular
>order and can be picked up independently. 
>
>The documentation updates in patches (5, 6, 7) are to the best of my ability.
>However, I would really appreciate extra eyes to ensure it is precise.
>
>The updated NMI-source patches will follow this series sometime later.
>
>@Ingo, I tried to format the commit references the way you prefer:
>
>  Commit:
>    feeaf5512947 ("x86: Move sysctls into arch/x86")
>
>However, checkpatch seems to dislike it, so I reformatted them as below:
>
>  Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
>
>Is there a specific guideline for x86?
>
>[1]: https://lore.kernel.org/lkml/20240709143906.1040477-1-jacob.jun.pan@linux.intel.com/
>
>Sohil Mehta (9):
>  x86/nmi: Simplify unknown NMI panic handling
>  x86/nmi: Consolidate NMI panic variables
>  x86/nmi: Use a macro to initialize NMI descriptors
>  x86/nmi: Remove export of local_touch_nmi()
>  x86/nmi: Fix comment in unknown NMI handling
>  x86/nmi: Improve and relocate NMI handler comments
>  x86/nmi: Improve NMI header documentation
>  x86/nmi: Clean up NMI selftest
>  x86/nmi: Improve NMI duration console print
>
> arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
> arch/x86/include/asm/x86_init.h |  1 +
> arch/x86/kernel/dumpstack.c     |  2 -
> arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
> arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
> arch/x86/kernel/setup.c         | 37 ++++++--------
> include/linux/panic.h           |  2 -
> 7 files changed, 122 insertions(+), 108 deletions(-)
>

Talking about NMI, one thing I would like to see done would be to explicitly enable NMI at the point where we are ready to take them. 

If we enter the kernel with NMIs disabled, using IDT they will get spuriously reenabled the first time we do IRET, but using FRED they could stay disabled until we enter user space.

It also seems "cleaner".

We ought to already be doing something with the RTC NMI gate register, so that ought to be in the same logical spot, too.

Re: [PATCH 0/9] x86: Cleanup NMI handling

Posted by Nikolay Borisov 8 months, 2 weeks ago


On 28.03.25 г. 1:46 ч., Sohil Mehta wrote:
> While reworking the NMI-source patches[1], I spent quite a few days
> understanding the existing NMI handling code. The intention of this series is
> to fix the inconsistencies and redundancies that I encountered.
> 
> This series also includes improved documentation to make the code easier to
> follow. It does not introduce any significant functional changes.
> 
> I have tried to split the patches logically to make them easier to review. Let
> me know if some of them should be combined. The patches are in no particular
> order and can be picked up independently.
> 
> The documentation updates in patches (5, 6, 7) are to the best of my ability.
> However, I would really appreciate extra eyes to ensure it is precise.
> 
> The updated NMI-source patches will follow this series sometime later.
> 
> @Ingo, I tried to format the commit references the way you prefer:
> 
>    Commit:
>      feeaf5512947 ("x86: Move sysctls into arch/x86")
> 
> However, checkpatch seems to dislike it, so I reformatted them as below:
> 
>    Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
> 
> Is there a specific guideline for x86?
> 
> [1]: https://lore.kernel.org/lkml/20240709143906.1040477-1-jacob.jun.pan@linux.intel.com/
> 
> Sohil Mehta (9):
>    x86/nmi: Simplify unknown NMI panic handling
>    x86/nmi: Consolidate NMI panic variables
>    x86/nmi: Use a macro to initialize NMI descriptors
>    x86/nmi: Remove export of local_touch_nmi()
>    x86/nmi: Fix comment in unknown NMI handling
>    x86/nmi: Improve and relocate NMI handler comments
>    x86/nmi: Improve NMI header documentation
>    x86/nmi: Clean up NMI selftest
>    x86/nmi: Improve NMI duration console print
> 
>   arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
>   arch/x86/include/asm/x86_init.h |  1 +
>   arch/x86/kernel/dumpstack.c     |  2 -
>   arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
>   arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
>   arch/x86/kernel/setup.c         | 37 ++++++--------
>   include/linux/panic.h           |  2 -
>   7 files changed, 122 insertions(+), 108 deletions(-)
> 

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

Re: [PATCH 0/9] x86: Cleanup NMI handling

Posted by Peter Zijlstra 8 months, 2 weeks ago

On Thu, Mar 27, 2025 at 11:46:20PM +0000, Sohil Mehta wrote:

> Sohil Mehta (9):
>   x86/nmi: Simplify unknown NMI panic handling
>   x86/nmi: Consolidate NMI panic variables
>   x86/nmi: Use a macro to initialize NMI descriptors
>   x86/nmi: Remove export of local_touch_nmi()
>   x86/nmi: Fix comment in unknown NMI handling
>   x86/nmi: Improve and relocate NMI handler comments
>   x86/nmi: Improve NMI header documentation
>   x86/nmi: Clean up NMI selftest
>   x86/nmi: Improve NMI duration console print
> 
>  arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
>  arch/x86/include/asm/x86_init.h |  1 +
>  arch/x86/kernel/dumpstack.c     |  2 -
>  arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
>  arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
>  arch/x86/kernel/setup.c         | 37 ++++++--------
>  include/linux/panic.h           |  2 -
>  7 files changed, 122 insertions(+), 108 deletions(-)

These seem sane,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>