The logic of not doing a cache flush if the CPU declares cache self
snooping support is repeated across the x86/cacheinfo code.  Extract it
into its own function.

Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
---
 arch/x86/kernel/cpu/cacheinfo.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 254c0b2e1d72..ac47d1b4f775 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -640,6 +640,17 @@ int populate_cache_leaves(unsigned int cpu)
 static unsigned long saved_cr4;
 static DEFINE_RAW_SPINLOCK(cache_disable_lock);
 
+/*
+ * Cache flushing is the most time-consuming step when programming the
+ * MTRRs.  On many Intel CPUs without known erratas, it can be skipped
+ * if the CPU declares cache self-snooping support.
+ */
+static void maybe_flush_caches(void)
+{
+	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
+		wbinvd();
+}
+
 void cache_disable(void) __acquires(cache_disable_lock)
 {
 	unsigned long cr0;
@@ -657,14 +668,7 @@ void cache_disable(void) __acquires(cache_disable_lock)
 	cr0 = read_cr0() | X86_CR0_CD;
 	write_cr0(cr0);
 
-	/*
-	 * Cache flushing is the most time-consuming step when programming
-	 * the MTRRs. Fortunately, as per the Intel Software Development
-	 * Manual, we can skip it if the processor supports cache self-
-	 * snooping.
-	 */
-	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
-		wbinvd();
+	maybe_flush_caches();
 
 	/* Save value of CR4 and clear Page Global Enable (bit 7) */
 	if (cpu_feature_enabled(X86_FEATURE_PGE)) {
@@ -679,9 +683,7 @@ void cache_disable(void) __acquires(cache_disable_lock)
 	if (cpu_feature_enabled(X86_FEATURE_MTRR))
 		mtrr_disable();
 
-	/* Again, only flush caches if we have to. */
-	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
-		wbinvd();
+	maybe_flush_caches();
 }
 
 void cache_enable(void) __releases(cache_disable_lock)
-- 
2.48.1

On 04/03/2025 8:51 am, Ahmed S. Darwish wrote:
> The logic of not doing a cache flush if the CPU declares cache self
> snooping support is repeated across the x86/cacheinfo code.  Extract it
> into its own function.
>
> Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>

I know you're just refactoring code, but the SDM has basically reverted
this statement about it being safe to skip WBINVD based on SELFSNOOP.

It turns out not to be safe in cases where the underlying physical
memory changes from cacheable to unchangeable.  By skipping the WBINVD
as part of changing the memory type, you end up with spurious writebacks
at a later point when the memory is expected to be UC.  Apparently this
is a problem for CLX devices, hence the change in the SDM.

~Andrew

Hi Andrew,

On Tue, 04 Mar 2025, Andrew Cooper wrote:
>
> On 04/03/2025 8:51 am, Ahmed S. Darwish wrote:
> > The logic of not doing a cache flush if the CPU declares cache self
> > snooping support is repeated across the x86/cacheinfo code.  Extract it
> > into its own function.
> >
> > Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
>
> I know you're just refactoring code, but the SDM has basically reverted
> this statement about it being safe to skip WBINVD based on SELFSNOOP.
>

Still, thanks a lot for sharing :)

> It turns out not to be safe in cases where the underlying physical
> memory changes from cacheable to unchangeable.  By skipping the WBINVD
> as part of changing the memory type, you end up with spurious writebacks
> at a later point when the memory is expected to be UC.  Apparently this
> is a problem for CLX devices, hence the change in the SDM.

While writing that refactoring patch, I indeed noticed that there is an
errata list of CPUs where X86_FEATURE_SELFSNOOP is force disabled, thus
ensuring WBINVD is never skipped:

    static void check_memory_type_self_snoop_errata(...)
    {
     	switch (c->x86_vfm) {
     	case INTEL_CORE_YONAH:
     	case INTEL_CORE2_MEROM:
     	case INTEL_CORE2_MEROM_L:
     	case INTEL_CORE2_PENRYN:
     	case INTEL_CORE2_DUNNINGTON:
     	case INTEL_NEHALEM:
     	case INTEL_NEHALEM_G:
     	case INTEL_NEHALEM_EP:
     	case INTEL_NEHALEM_EX:
     	case INTEL_WESTMERE:
     	case INTEL_WESTMERE_EP:
     	case INTEL_SANDYBRIDGE:
     		setup_clear_cpu_cap(X86_FEATURE_SELFSNOOP);
     	}
    }

That's why I added "CPUs without known erratas" in the comments:

    /*
     * Cache flushing is the most time-consuming step when programming
     * the MTRRs.  On many Intel CPUs without known erratas, it can be
     * skipped if the CPU declares cache self-snooping support.
     */
    static void maybe_flush_caches(void)
    {
           if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
                   wbinvd();
    }

But, interestingly, CLX devices (intel-family.h CASCADELAKE_X /
SKYLAKE_X) are not part of the kernel's Self Snoop errata list above.

@Thomas, @Ingo, any ideas?

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH

On 05/03/2025 6:40 pm, Ahmed S. Darwish wrote:
> Hi Andrew,
>
> On Tue, 04 Mar 2025, Andrew Cooper wrote:
>> On 04/03/2025 8:51 am, Ahmed S. Darwish wrote:
>>> The logic of not doing a cache flush if the CPU declares cache self
>>> snooping support is repeated across the x86/cacheinfo code.  Extract it
>>> into its own function.
>>>
>>> Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
>> I know you're just refactoring code, but the SDM has basically reverted
>> this statement about it being safe to skip WBINVD based on SELFSNOOP.
>>
> Still, thanks a lot for sharing :)
>
>> It turns out not to be safe in cases where the underlying physical
>> memory changes from cacheable to unchangeable.  By skipping the WBINVD
>> as part of changing the memory type, you end up with spurious writebacks
>> at a later point when the memory is expected to be UC.  Apparently this
>> is a problem for CLX devices, hence the change in the SDM.
> While writing that refactoring patch, I indeed noticed that there is an
> errata list of CPUs where X86_FEATURE_SELFSNOOP is force disabled, thus
> ensuring WBINVD is never skipped:
>
>     static void check_memory_type_self_snoop_errata(...)
>     {
>      	switch (c->x86_vfm) {
>      	case INTEL_CORE_YONAH:
>      	case INTEL_CORE2_MEROM:
>      	case INTEL_CORE2_MEROM_L:
>      	case INTEL_CORE2_PENRYN:
>      	case INTEL_CORE2_DUNNINGTON:
>      	case INTEL_NEHALEM:
>      	case INTEL_NEHALEM_G:
>      	case INTEL_NEHALEM_EP:
>      	case INTEL_NEHALEM_EX:
>      	case INTEL_WESTMERE:
>      	case INTEL_WESTMERE_EP:
>      	case INTEL_SANDYBRIDGE:
>      		setup_clear_cpu_cap(X86_FEATURE_SELFSNOOP);
>      	}
>     }
>
> That's why I added "CPUs without known erratas" in the comments:
>
>     /*
>      * Cache flushing is the most time-consuming step when programming
>      * the MTRRs.  On many Intel CPUs without known erratas, it can be
>      * skipped if the CPU declares cache self-snooping support.
>      */
>     static void maybe_flush_caches(void)
>     {
>            if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
>                    wbinvd();
>     }
>
> But, interestingly, CLX devices (intel-family.h CASCADELAKE_X /
> SKYLAKE_X) are not part of the kernel's Self Snoop errata list above.

CLX (Cascade Lake) != CXL (Compute eXpress Link).

CXL is the new PCIe.  (So say the CXL consortium at least.)

~Andrew

On Wed, 05 Mar 2025, Andrew Cooper wrote:
...
> It turns out not to be safe in cases where the underlying physical
> memory changes from cacheable to unchangeable.  By skipping the WBINVD
> as part of changing the memory type, you end up with spurious writebacks
> at a later point when the memory is expected to be UC.  Apparently this
> is a problem for CLX devices, hence the change in the SDM.
...
>
> CLX (Cascade Lake) != CXL (Compute eXpress Link).
>
> CXL is the new PCIe.  (So say the CXL consortium at least.)
>

Oh, sorry, you wrote "CLX devices" above, not CXL... Only thing my poor
brain could come up with was CASCADELAKE_X :)

On 05/03/2025 6:58 pm, Ahmed S. Darwish wrote:
> On Wed, 05 Mar 2025, Andrew Cooper wrote:
> ...
>> It turns out not to be safe in cases where the underlying physical
>> memory changes from cacheable to unchangeable.  By skipping the WBINVD
>> as part of changing the memory type, you end up with spurious writebacks
>> at a later point when the memory is expected to be UC.  Apparently this
>> is a problem for CLX devices, hence the change in the SDM.
> ...
>> CLX (Cascade Lake) != CXL (Compute eXpress Link).
>>
>> CXL is the new PCIe.  (So say the CXL consortium at least.)
>>
> Oh, sorry, you wrote "CLX devices" above, not CXL... Only thing my poor
> brain could come up with was CASCADELAKE_X :)

Oops, so I did.  Sorry.

~Andrew