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 18:08:14 -0700, Sohil Mehta <sohil.mehta@intel.com>
wrote:

> >> 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?
> >   
> 
> I thought there was a common function for all features. I expected it to
> go through each feature and clear the ones whose dependency is missing.
> But I can't find it easily. Maybe someone else knows this better.
> 
> However, anytime do_clear_cpu_cap() is called for any feature it does
> the below and scans the cpuid_deps table to clear all features with
> missing dependencies. That would cause X86_FEATURE_NMI_SOURCE to be
> cleared one way or another.
I don't think this is true. For a simplified example:
cpuid_deps has the following feature-depends pairs.
[1, 3]
[2, 3]
now, do_clear_cpu_cap(c, 2)

Before the loop below __set_bit(feature, disable), bit 2 is set. 

Since there is no other features depend on 2, the loop below will not clear
any other features. no?

> 
> 
> 	/* Loop until we get a stable state. */
> 	do {
> 		changed = false;
> 		for (d = cpuid_deps; d->feature; d++) {
> 			if (!test_bit(d->depends, disable))
> 				continue;
> 			if (__test_and_set_bit(d->feature, disable))
> 				continue;
> 
> 			changed = true;
> 			clear_feature(c, d->feature);
> 		}
> 	} while (changed);
> 
> 
> > In the next version, I will add runtime disable if HW malfunctions.
> > i.e. no valid bitmask.
> >   
> 
> I don't think we do this for other features that have a missing
> dependency. It doesn't seem NMI source is any different from them.
> 
NMI source is an optimization with a fallback path *always* available. In
that sense, it can be disabled at runtime without losing functionality.

The closest analogy I can think of are timers for clocksources where we use
higher ranking/cheaper timers first, and only resort to other timers in
case the primary/optimal one fails.

e.g.
root@984fee003c4f:~/jacob# cat
/sys/devices/system/clocksource/clocksource0/available_clocksource  
tsc hpet acpi_pm


Thanks,

Jacob