[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240627152102.592a20f5@jacob-builder>
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
Powered by blists - more mailing lists