linux-kernel - Re: [PATCH v11 06/12] x86/mm: use INVLPGB for kernel TLB flushes

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]

Message-ID: <5f4d58fe-4fa1-4b59-81a7-e8c8d3030d0a@intel.com>
Date: Tue, 18 Feb 2025 14:27:31 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Rik van Riel <riel@...riel.com>, Yosry Ahmed <yosry.ahmed@...ux.dev>
Cc: Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
 linux-kernel@...r.kernel.org, bp@...en8.de, dave.hansen@...ux.intel.com,
 zhengqi.arch@...edance.com, nadav.amit@...il.com, thomas.lendacky@....com,
 kernel-team@...a.com, linux-mm@...ck.org, akpm@...ux-foundation.org,
 jackmanb@...gle.com, jannh@...gle.com, mhklinux@...look.com,
 andrew.cooper3@...rix.com, Manali Shukla <Manali.Shukla@....com>
Subject: Re: [PATCH v11 06/12] x86/mm: use INVLPGB for kernel TLB flushes

On 2/18/25 10:00, Rik van Riel wrote:
> On Sat, 2025-02-15 at 02:08 +0000, Yosry Ahmed wrote:
>> So I think what Dave wants (and I agree) is:
>> 	if (!broadcast_kernel_range_flush(info))
>> 		ipi_kernel_range_flush(info)
>>
>> Where ipi_kernel_range_flush() contains old_thing1() and oldthing2().

That's OK-ish. But it still smells of hacking in the new concept without
refactoring things properly.

Let's logically inline the code that we've got.  I think it actually
ends up looking something like this:

	if (cpu_feature_enabled(X86_FEATURE_INVLPGB)) {
		if (info->end == TLB_FLUSH_ALL) {
			invlpgb_flush_all();
		} else {
			for_each(addr)
				invlpgb_flush_addr_nosync(addr, nr);
		}
	} else {
		if (info->end == TLB_FLUSH_ALL)
 			on_each_cpu(do_flush_tlb_all, NULL, 1);
	 	else
 			on_each_cpu(do_kernel_range_flush, info, 1);
	}

Where we've got two inputs:

	1. INVLPGB support (or not)
	2. TLB_FLUSH_ALL (basically ranged or full flush)

So I think we should group by *one* of those. The above groups by
INVLPGB support and this groups by TLB_FLUSH_ALL:

	if (info->end == TLB_FLUSH_ALL) {
		if (cpu_feature_enabled(X86_FEATURE_INVLPGB)) {
			invlpgb_flush_all();
		} else {
			on_each_cpu(do_flush_tlb_all, NULL, 1);
		}
	} else {
		if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
 			for_each(addr)
				invlpgb_flush_addr_nosync(addr, nr);
	 	else
 			on_each_cpu(do_kernel_range_flush, info, 1);
	}

So, if we create some helpers that give some consistent naming:

static tlb_flush_all_ipi(...)
{
	on_each_cpu(do_flush_tlb_all, NULL, 1);
}

static tlb_flush_all(...)
{
	if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
		invlpgb_flush_all(...);
	else
		tlb_flush_all_ipi(...);
}

and then also create the ranged equivalents (which internally have the
same cpu_feature_enabled() check):

	tlb_flush_range_ipi(...)
	invlpgb_flush_range(...)

Then we can have the top-level code be:

	if (info->end == TLB_FLUSH_ALL)
		tlb_flush_all(info);
	else
		tlb_flush_range(info);

That actually looks way nicer than what we have today. For bonus points,
if a third way of flushing the TLB showed up, it would slot right in:

 static tlb_flush_all(...)
 {
	if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
		invlpgb_flush_all(...);
+	else if cpu_feature_enabled(X86_FEATURE_RAR))
+		rar_flush_all(...);
	else
		tlb_flush_all_ipi(...);
 }

That's *exactly* the way we want the code to read. At the higher level,
it's deciding based on the generic thing that *everybody* cares about:
ranged or full flush. Then, at the lower level, it's deciding how to
implement that high-level flush concept.