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

[Sohil Mehta <sohil.mehta@intel.com>]

Hi Jacob,

On 6/11/2024 9:54 AM, Jacob Pan wrote:

> 
> 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@linux.intel.com>
> ---
> v2: Removed NMI source from static CPU ID dependency table (HPA)

I am not sure if this would work in all scenarios. See below.
Sorry, I couldn't chime-in during the v1 review when this was suggested.


> 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);
> +	}
>  
>  	/* Init cpu_entry_area before IST entries are set up */
>  	setup_cpu_entry_areas();

I think this relies on the fact that whenever X86_FEATURE_NMI_SOURCE is
set, X86_FEATURE_FRED will also be set by the hardware. Though this
might be the expected behavior, hardware sometimes messes up and the
dependency entry in the static table would probably help catch that.

IIUC, when X86_FEATURE_NMI_SOURCE is set and X86_FEATURE_FRED is
cleared, cpu_feature_enabled(X86_FEATURE_FRED) will fail and the above
check would not end up clearing X86_FEATURE_NMI_SOURCE.

Isn't the following entry necessary to detect a misconfiguration or is
the purpose of the cpuid_deps table something else?

diff --git a/arch/x86/kernel/cpu/cpuid-deps.c
b/arch/x86/kernel/cpu/cpuid-deps.c
index b7d9f530ae16..39526041e91a 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -84,6 +84,7 @@ static const struct cpuid_dep cpuid_deps[] = {
        { X86_FEATURE_SHSTK,                    X86_FEATURE_XSAVES    },
        { X86_FEATURE_FRED,                     X86_FEATURE_LKGS      },
        { X86_FEATURE_FRED,                     X86_FEATURE_WRMSRNS   },
+       { X86_FEATURE_NMI_SOURCE,		X86_FEATURE_FRED      },
        {}
 };