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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 27 Jun 2024 15:23:22 -0700
From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
To: Sohil Mehta <sohil.mehta@...el.com>
Cc: X86 Kernel <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
 "Thomas Gleixner" <tglx@...utronix.de>, Dave Hansen
 <dave.hansen@...el.com>, "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar
 <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 <linux-perf-users@...r.kernel.org>, Peter Zijlstra <peterz@...radead.org>,
 Andi Kleen <andi.kleen@...el.com>, Xin Li <xin3.li@...el.com>,
 jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v2 1/6] x86/irq: Add enumeration of NMI source reporting
 CPU feature

On Fri, 21 Jun 2024 18:08:14 -0700, Sohil Mehta <sohil.mehta@...el.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@...fee003c4f:~/jacob# cat
/sys/devices/system/clocksource/clocksource0/available_clocksource  
tsc hpet acpi_pm

Thanks,

Jacob