[PATCH] M386 flush_one_tlb invlpg

[PATCH] M386 flush_one_tlb invlpg

[Hugh Dickins]

CONFIG_M386 kernel running on PPro+ processor with X86_FEATURE_PGE may
set _PAGE_GLOBAL bit: then __flush_tlb_one must use invlpg instruction.

The need for this was shown by a recent HyperThreading discussion.
Marc Dietrich reported (LKML 22 Aug) that CONFIG_M386 CONFIG_SMP kernel
on P4 Xeon did not support HT: his dmesg showed acpi_tables_init failed
from bad table data due to unflushed TLB: he confirms patch fixes it.

No tears would be shed if CONFIG_M386 could not support HT, but bad TLB
is trouble.  Same CONFIG_M386 bug affects CONFIG_HIGHMEM's kmap_atomic,
and potentially dmi_scan (now using set_fixmap via bt_ioremap).  Though
it's true that none of these uses really needs _PAGE_GLOBAL bit itself.

I just sent the 2.4.20-pre4 asm-i386/pgtable.h patch to Marcelo:
here's patch against 2.5.31 or current BK: please apply.

Hugh

--- 2.5.31/include/asm-i386/tlbflush.h	Tue Jun  4 14:27:14 2002
+++ linux/include/asm-i386/tlbflush.h	Tue Aug 27 15:30:53 2002
@@ -45,11 +45,19 @@
 			__flush_tlb();					\
 	} while (0)
 
-#ifndef CONFIG_X86_INVLPG
-#define __flush_tlb_one(addr) __flush_tlb()
+#define __flush_tlb_single(addr) \
+	__asm__ __volatile__("invlpg %0": :"m" (*(char *) addr))
+
+#ifdef CONFIG_X86_INVLPG
+# define __flush_tlb_one(addr) __flush_tlb_single(addr)
 #else
-#define __flush_tlb_one(addr) \
-__asm__ __volatile__("invlpg %0": :"m" (*(char *) addr))
+# define __flush_tlb_one(addr)						\
+	do {								\
+		if (cpu_has_pge)					\
+			__flush_tlb_single(addr);			\
+		else							\
+			__flush_tlb();					\
+	} while (0)
 #endif
 
 /*

Re: [PATCH] M386 flush_one_tlb invlpg

[Linus Torvalds]

On Tue, 27 Aug 2002, Hugh Dickins wrote:
> 
> I just sent the 2.4.20-pre4 asm-i386/pgtable.h patch to Marcelo:
> here's patch against 2.5.31 or current BK: please apply.

This test is senseless, in my opinion:

> +		if (cpu_has_pge)					\
> +			__flush_tlb_single(addr);			\

The test _should_ be for something like

	if (cpu_has_invlpg)
		__flush_tlb_single(addr);

since we want to use the invlpg instruction regardless of any PGE issues 
if it is available.

There's another issue, which is the fact that I do not believe that invlpg 
is even guaranteed to invalidate a G page at all - although obviously all 
current CPU's seem to work that way. However, I don't see that documented 
anywhere. It might make sense to mark the places that expect to invalidate 
a global page explicitly, and call that function "flush_one_global_rlb()" 
(even if it - at least for now - does the same thing as the regular single 
invalidate).

		Linus

Re: [PATCH] M386 flush_one_tlb invlpg

[Luca Barbieri]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

> There's another issue, which is the fact that I do not believe that invlpg 
> is even guaranteed to invalidate a G page at all - although obviously all 
> current CPU's seem to work that way. However, I don't see that documented 
> anywhere.
You haven't read the P4 system architecture manual, section 3.11.
It explicitly says that invlpg ignores the G flag.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] M386 flush_one_tlb invlpg

[Linus Torvalds]

On 27 Aug 2002, Luca Barbieri wrote:
>
> You haven't read the P4 system architecture manual, section 3.11.
> It explicitly says that invlpg ignores the G flag.

Ahh, good. Then my only issue is the mismatched test..

		Linus

Re: [PATCH] M386 flush_one_tlb invlpg

[Brian Gerst]

Linus Torvalds wrote:
> On Tue, 27 Aug 2002, Hugh Dickins wrote:
> 
>>I just sent the 2.4.20-pre4 asm-i386/pgtable.h patch to Marcelo:
>>here's patch against 2.5.31 or current BK: please apply.
> 
> 
> This test is senseless, in my opinion:
> 
> 
>>+		if (cpu_has_pge)					\
>>+			__flush_tlb_single(addr);			\
> 
> 
> The test _should_ be for something like
> 
> 	if (cpu_has_invlpg)
> 		__flush_tlb_single(addr);
> 
> since we want to use the invlpg instruction regardless of any PGE issues 
> if it is available.
> 
> There's another issue, which is the fact that I do not believe that invlpg 
> is even guaranteed to invalidate a G page at all - although obviously all 
> current CPU's seem to work that way. However, I don't see that documented 
> anywhere. 

P4 System Programming Guide, Section 10.9:
The INVLPG instruction invalidates the TLB for a specific page. This 
instruction is the most efficient in cases where software only needs to 
invalidate a specific page, because it improves performance over 
invalidating the whole TLB. This instruction is not affected by the 
state of the G flag in a page-directory or page-table entry.

Re: [PATCH] M386 flush_one_tlb invlpg

[Andrea Arcangeli]

On Tue, Aug 27, 2002 at 10:45:29PM +0200, Luca Barbieri wrote:
> You haven't read the P4 system architecture manual, section 3.11.
> It explicitly says that invlpg ignores the G flag.

btw, we just depend on it for example on every kmap_atomic.

Andrea

Re: [PATCH] M386 flush_one_tlb invlpg

[Hugh Dickins]

On Tue, 27 Aug 2002, Linus Torvalds wrote:
> 
> This test is senseless, in my opinion:
> 
> > +		if (cpu_has_pge)					\
> > +			__flush_tlb_single(addr);			\
> 
> The test _should_ be for something like
> 
> 	if (cpu_has_invlpg)
> 		__flush_tlb_single(addr);
> 
> since we want to use the invlpg instruction regardless of any PGE issues 

New patch below defines cpu_has_invlpg as (boot_cpu_data.x86 > 3).
But I do feel safer with that original cpu_has_pge test, which was
using a decent capability flag, and only changed behaviour of the
CONFIG_M386 __flush_tlb_one when it's necessary.

Isn't CONFIG_M386 about maximum safe applicability, rather than speed?
Am I imagining it, or were there a few i386 + i486 SMP machines built?
Or might there be some i486 clone which didn't really implement invlpg,
which could be used with a CONFIG_M386 kernel before this change,
but not after?  But perhaps I'm just dreaming up excuses for my
senselessness - your call.

Hugh

CONFIG_M386 kernel running on PPro+ processor with X86_FEATURE_PGE may
set _PAGE_GLOBAL bit: then __flush_tlb_one must use invlpg instruction.

--- 2.5.32/include/asm-i386/tlbflush.h	Tue May 28 21:41:36 2002
+++ linux/include/asm-i386/tlbflush.h	Wed Aug 28 20:48:44 2002
@@ -45,11 +45,21 @@
 			__flush_tlb();					\
 	} while (0)
 
-#ifndef CONFIG_X86_INVLPG
-#define __flush_tlb_one(addr) __flush_tlb()
+#define cpu_has_invlpg	(boot_cpu_data.x86 > 3)
+
+#define __flush_tlb_single(addr) \
+	__asm__ __volatile__("invlpg %0": :"m" (*(char *) addr))
+
+#ifdef CONFIG_X86_INVLPG
+# define __flush_tlb_one(addr) __flush_tlb_single(addr)
 #else
-#define __flush_tlb_one(addr) \
-__asm__ __volatile__("invlpg %0": :"m" (*(char *) addr))
+# define __flush_tlb_one(addr)						\
+	do {								\
+		if (cpu_has_invlpg)					\
+			__flush_tlb_single(addr);			\
+		else							\
+			__flush_tlb();					\
+	} while (0)
 #endif
 
 /*

Re: [PATCH] M386 flush_one_tlb invlpg

[Alan Cox]

On Wed, 2002-08-28 at 21:30, Hugh Dickins wrote:
> New patch below defines cpu_has_invlpg as (boot_cpu_data.x86 > 3).
> But I do feel safer with that original cpu_has_pge test, which was
> using a decent capability flag, and only changed behaviour of the
> CONFIG_M386 __flush_tlb_one when it's necessary.
> 
> Isn't CONFIG_M386 about maximum safe applicability, rather than speed?
> Am I imagining it, or were there a few i386 + i486 SMP machines built?
> Or might there be some i486 clone which didn't really implement invlpg,
> which could be used with a CONFIG_M386 kernel before this change,
> but not after?  But perhaps I'm just dreaming up excuses for my
> senselessness - your call.

To answer that

There are no SMP i386 boxes that support Intel MP 1.1
There are a few SMP 486 boxes using MP 1.1

The nx586 processor is a '586' class CPU that has neither wp nor invlpg
by default. I believe however that it reports family '3' if it has the
hypercode loaded which lacks invlpg