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

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

On Fri, 21 Jun 2024 15:23:51 -0700, Sohil Mehta <sohil.mehta@intel.com>
wrote:

> > 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?
My understanding is that cpuid_deps is to ensure CPU features are
cleared according to its dependency chain. Not for HW bugs/quirks.

> 
> 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      },
>         {}
>  };
If FRED is never reported by CPUID, then there would not be any calls to
setup_clear_cpu_cap(X86_FEATURE_FRED), so this table does not help clear
the dependent NMI_SOURCE, right?

In the next version, I will add runtime disable if HW malfunctions. i.e. no
valid bitmask.

Maybe we can also add a big WARN_ON like this:
if (WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_FRED) &&
		cpu_feature_enabled(X86_FEATURE_NMI_SOURCE)) 
	setup_clear_cpu_cap(X86_FEATURE_NMI_SOURCE);

Thanks,

Jacob