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

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

>> 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.


	/* 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.

> 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