Re: [PATCH v2 1/6] x86/irq: Add enumeration of NMI source reporting CPU feature

[Xin Li]

On 6/11/2024 9:54 AM, Jacob Pan wrote:
> The lack of a mechanism to pinpoint the origins of Non-Maskable Interrupts
> (NMIs) necessitates that the NMI vector 2 handler consults each NMI source
> handler individually. This approach leads to inefficiencies, delays, and
> the occurrence of unnecessary NMIs, thereby also constraining the potential
> applications of NMIs.
>
> A new CPU feature, known as NMI source reporting, has been introduced as
> part of the Flexible Return and Event Delivery (FRED) spec. This feature
> enables the NMI vector 2 handler to directly obtain information about the
> NMI source from the FRED event data.
>
> The functionality of NMI source reporting is tied to the FRED. Although it
> is enumerated by a unique CPUID feature bit, it cannot be turned off
> independently once FRED is activated.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> ---
> v2: Removed NMI source from static CPU ID dependency table (HPA)
> ---
>   arch/x86/Kconfig                         | 9 +++++++++
>   arch/x86/include/asm/cpufeatures.h       | 1 +
>   arch/x86/include/asm/disabled-features.h | 8 +++++++-
>   arch/x86/kernel/traps.c                  | 4 +++-
>   4 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1d7122a1883e..b8b15f20b94e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -511,12 +511,21 @@ config X86_CPU_RESCTRL
>   config X86_FRED
>   	bool "Flexible Return and Event Delivery"
>   	depends on X86_64
> +	select X86_NMI_SOURCE
>   	help
>   	  When enabled, try to use Flexible Return and Event Delivery
>   	  instead of the legacy SYSCALL/SYSENTER/IDT architecture for
>   	  ring transitions and exception/interrupt handling if the
>   	  system supports it.
>   
> +config X86_NMI_SOURCE

Lets reuse X86_FRED instead of adding another hard config option. See
below.

> +	def_bool n
> +	help
> +	  Once enabled, information on NMI originator/source can be provided
> +	  via FRED event data. This makes NMI processing more efficient in that
> +	  NMI handler does not need to check for every possible source at
> +	  runtime when NMI is delivered.
> +
>   config X86_BIGSMP
>   	bool "Support for big SMP systems with more than 8 CPUs"
>   	depends on SMP && X86_32

...

> diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
> index c492bdc97b05..3856c4737d65 100644
> --- a/arch/x86/include/asm/disabled-features.h
> +++ b/arch/x86/include/asm/disabled-features.h
> @@ -123,6 +123,12 @@
>   # define DISABLE_FRED	(1 << (X86_FEATURE_FRED & 31))
>   #endif
>   
> +#ifdef CONFIG_X86_NMI_SOURCE
> +# define DISABLE_NMI_SOURCE	0
> +#else
> +# define DISABLE_NMI_SOURCE	(1 << (X86_FEATURE_NMI_SOURCE & 31))
> +#endif
> +
>   #ifdef CONFIG_KVM_AMD_SEV
>   #define DISABLE_SEV_SNP		0
>   #else
> @@ -145,7 +151,7 @@
>   #define DISABLED_MASK10	0
>   #define DISABLED_MASK11	(DISABLE_RETPOLINE|DISABLE_RETHUNK|DISABLE_UNRET| \
>   			 DISABLE_CALL_DEPTH_TRACKING|DISABLE_USER_SHSTK)
> -#define DISABLED_MASK12	(DISABLE_FRED|DISABLE_LAM)
> +#define DISABLED_MASK12	(DISABLE_FRED|DISABLE_LAM|DISABLE_NMI_SOURCE)
>   #define DISABLED_MASK13	0
>   #define DISABLED_MASK14	0
>   #define DISABLED_MASK15	0
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4fa0b17e5043..465f04e4a79f 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -1427,8 +1427,10 @@ early_param("fred", fred_setup);
>   
>   void __init trap_init(void)
>   {
> -	if (cpu_feature_enabled(X86_FEATURE_FRED) && !enable_fred)
> +	if (cpu_feature_enabled(X86_FEATURE_FRED) && !enable_fred) {
>   		setup_clear_cpu_cap(X86_FEATURE_FRED);
> +		setup_clear_cpu_cap(X86_FEATURE_NMI_SOURCE);
> +	}

With this, no need to add DISABLE_NMI_SOURCE to disabled-features.h:

1) If FRED is not available, NMI source won't be too.
2) If FRED is available but not enabled, all features relying on FRED
should be cleared. We probably should move the feature bits clearing
code into a static function when more such features are added in future.

>   
>   	/* Init cpu_entry_area before IST entries are set up */
>   	setup_cpu_entry_areas();

Thanks!
    Xin