Re: [PATCH 7/7] x86/bugs: Replace CONFIG_SPECTRE_BHI_{ON,OFF} with CONFIG_MITIGATION_SPECTRE_BHI

From: Reinette Chatre <reinette.chatre@intel.com>
To: Ingo Molnar <mingo@kernel.org>, Josh Poimboeuf <jpoimboe@kernel.org>
Cc: <x86@kernel.org>, <linux-kernel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Daniel Sneddon <daniel.sneddon@linux.intel.com>,
	Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Alexandre Chartre <alexandre.chartre@oracle.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	Sean Christopherson <seanjc@google.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Nikolay Borisov <nik.borisov@suse.com>,
	"KP Singh" <kpsingh@kernel.org>, Waiman Long <longman@redhat.com>,
	"Borislav Petkov" <bp@alien8.de>,
	James Morse <james.morse@arm.com>,
	Dave Martin <Dave.Martin@arm.com>
Subject: Re: [PATCH 7/7] x86/bugs: Replace CONFIG_SPECTRE_BHI_{ON,OFF} with CONFIG_MITIGATION_SPECTRE_BHI
Date: Tue, 16 Apr 2024 22:35:58 -0700	[thread overview]
Message-ID: <eeb900f4-30ae-45e8-87a4-30c3faf7c1ba@intel.com> (raw)
In-Reply-To: <ZhecyLQsGZ9Iv8wU@gmail.com>

Hi Ingo,

On 4/11/2024 1:18 AM, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
>>>  static enum bhi_mitigations bhi_mitigation __ro_after_init =
>>> -	IS_ENABLED(CONFIG_SPECTRE_BHI_ON) ? BHI_MITIGATION_ON : BHI_MITIGATION_OFF;
>>> +	IS_ENABLED(CONFIG_MITIGATION_SPECTRE_BHI_ON) ? BHI_MITIGATION_ON : BHI_MITIGATION_OFF;
>>
>> Uhm, after this patch there's no CONFIG_MITIGATION_SPECTRE_BHI_ON 
>> anymore, which is kindof nasty, as IS_ENABLED() doesn't generate a build 
>> failure for non-existent Kconfig variables IIRC ...
>>
>> So AFAICT this patch turns on BHI unconditionally.
> 
> BTW., this is why IS_ENABLED() is a bad primitive IMO:
> 
> kepler:~/tip> for N in $(git grep -w IS_ENABLED arch/x86/ | sed 's/.*IS_ENABLED(//g' | sed 's/).*//g' | sort | uniq | sed 's/^CONFIG_//g'); do printf "# %40s: " $N; git grep -E "^config $N$" -- '**Kconfig**' | wc -l; done | grep -w 0
> #           RESCTRL_RMID_DEPENDS_ON_CLOSID: 0
> #                   NODE_NOT_IN_PAGE_FLAGS: 0
> 
> 1)
> 
> CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID doesn't exist anymore, but is used 
> widely:
> 
>  kepler:~/tip> git grep RESCTRL_RMID_DEPENDS_ON_CLOSID
>  arch/x86/kernel/cpu/resctrl/monitor.c: *     Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined.
>  arch/x86/kernel/cpu/resctrl/monitor.c:  if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>  arch/x86/kernel/cpu/resctrl/monitor.c:  if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>  arch/x86/kernel/cpu/resctrl/monitor.c:  if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>  arch/x86/kernel/cpu/resctrl/monitor.c:  if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>  arch/x86/kernel/cpu/resctrl/monitor.c:          if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>  arch/x86/kernel/cpu/resctrl/monitor.c:  if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c: if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> 
> Each of those uses is bogus, as the Kconfig symbol doesn't exist. (!)
> 
> AFAICT CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID was never defined within the 
> upstream kernel (!!).
> 
> AFAICT the first bogus CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID use was 
> introduced in this recent commit:
> 
>    b30a55df60c3 ("x86/resctrl: Track the number of dirty RMID a CLOSID has")
> 
> ... and was cargo-cult copied in other patches. It was never explained in 
> the changelog why it's used without a Kconfig entry anywhere.
> 
> Maybe in the future some other arch might (or might not) introduce 
> RESCTRL_RMID_DEPENDS_ON_CLOSID, but that doesn't justify this bad pattern 
> of dead code ...
> 

We are in the final stage [1] of untangling the resctrl filesystem code from the 
x86 specific code with the goal for the resctrl interface to also be used for Arm's
(Memory System Resource Partitioning and Monitoring) MPAM that is similar enough
to AMD's Platform Quality of Service (PQOS) and Intel's Resource Director Technology
(RDT) for which resctrl is currently used.

During this work we incrementally separated the resctrl filesystem from the x86
specific code. RESCTRL_RMID_DEPENDS_ON_CLOSID was introduced during this effort
to prepare the resctrl filesystem for a fundamental difference between x86 and Arm
and you are correct that this is dead code until the Arm support arrives. The Kconfig
entry is currently being reviewed [2] as part of the portion where the resctrl
filesystem structure starts to take shape but it remains without a user until the
Arm support arrives in the next part of this effort.

I do believe that there are precedent for subsystems obtaining support for features
a couple of kernel versions before users of the features appear. With Arm MPAM support
requiring this capability from the new "resctrl filesystem" it did seem reasonable at
the time to add the capability in preparation for Arm support.

Reinette

[1] https://lore.kernel.org/lkml/20240321165106.31602-1-james.morse@arm.com/
[2] https://lore.kernel.org/lkml/20240321165106.31602-30-james.morse@arm.com/