[PATCH] Use global TLB flushes in MTRR code

[PATCH] Use global TLB flushes in MTRR code

[Andi Kleen]

[probably stable material too]

Use global TLB flushes in MTRR code

Obviously kernel mappings should be flushed here too.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/cpu/mtrr/generic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux/arch/x86/kernel/cpu/mtrr/generic.c
===================================================================
--- linux.orig/arch/x86/kernel/cpu/mtrr/generic.c
+++ linux/arch/x86/kernel/cpu/mtrr/generic.c
@@ -356,7 +356,7 @@ static void prepare_set(void) __acquires
 	}
 
 	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
-	__flush_tlb();
+	__flush_tlb_all();
 
 	/*  Save MTRR state */
 	rdmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
@@ -368,7 +368,7 @@ static void prepare_set(void) __acquires
 static void post_set(void) __releases(set_atomicity_lock)
 {
 	/*  Flush TLBs (no need to flush caches - they are disabled)  */
-	__flush_tlb();
+	__flush_tlb_all();
 
 	/* Intel (P6) standard MTRRs */
 	mtrr_wrmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);

Re: [PATCH] Use global TLB flushes in MTRR code

[Oliver Pinter]

and old-stable (eg 2.6.22)?

On 2/7/08, Andi Kleen <andi@firstfloor.org> wrote:
> [probably stable material too]
>
> Use global TLB flushes in MTRR code
>
> Obviously kernel mappings should be flushed here too.
>
> Signed-off-by: Andi Kleen <ak@suse.de>
>
> ---
>  arch/x86/kernel/cpu/mtrr/generic.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: linux/arch/x86/kernel/cpu/mtrr/generic.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/cpu/mtrr/generic.c
> +++ linux/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -356,7 +356,7 @@ static void prepare_set(void) __acquires
>  	}
>
>  	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
> -	__flush_tlb();
> +	__flush_tlb_all();
>
>  	/*  Save MTRR state */
>  	rdmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
> @@ -368,7 +368,7 @@ static void prepare_set(void) __acquires
>  static void post_set(void) __releases(set_atomicity_lock)
>  {
>  	/*  Flush TLBs (no need to flush caches - they are disabled)  */
> -	__flush_tlb();
> +	__flush_tlb_all();
>
>  	/* Intel (P6) standard MTRRs */
>  	mtrr_wrmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Thanks,
Oliver

Re: [PATCH] Use global TLB flushes in MTRR code

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> [probably stable material too]
> 
> Use global TLB flushes in MTRR code
> 
> Obviously kernel mappings should be flushed here too.

no, your patch is not needed:

>  	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
> -	__flush_tlb();
> +	__flush_tlb_all();

read the complete code:

        /*  Save value of CR4 and clear Page Global Enable (bit 7)  */
        if ( cpu_has_pge ) {
                cr4 = read_cr4();
                write_cr4(cr4 & ~X86_CR4_PGE);
        }

        /* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
        __flush_tlb();

first first turn off PGE and do a cr3 flush - that gets rid of all TLBs.

but even if it didnt get rid of it, the mirror image function, 
post_set(), we turn PGE back on in the cr4 _after_ we've flushed the 
TLBs via the cr3 - that will flush all TLBs again.

	Ingo

Re: [PATCH] Use global TLB flushes in MTRR code

[Andi Kleen]

On Thu, Feb 07, 2008 at 08:13:37PM +0100, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > [probably stable material too]
> > 
> > Use global TLB flushes in MTRR code
> > 
> > Obviously kernel mappings should be flushed here too.
> 
> no, your patch is not needed:

Yes you're right.  Thanks makes more sense.  The weird style
fooled me.

It seems to come from the pseudo code in the Intel manual,
but I think it would be still cleaner/more idiomatic to use two
__flush_tlb_all() and remove the explicit code. The only drawback would 
be some more cr4 accesses, which does not seem like a big issue.

Updated patch follows.

-Andi

---

Use standard global TLB flushes in MTRR code

This is more idiomatic and it does not really make sense for this 
code to implement a own TLB flushing variant.

The control registers will be read/written a few times more, but 
that should not really matter for this code.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/cpu/mtrr/generic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux/arch/x86/kernel/cpu/mtrr/generic.c
===================================================================
--- linux.orig/arch/x86/kernel/cpu/mtrr/generic.c
+++ linux/arch/x86/kernel/cpu/mtrr/generic.c
@@ -349,14 +349,8 @@ static void prepare_set(void) __acquires
 	write_cr0(cr0);
 	wbinvd();
 
-	/*  Save value of CR4 and clear Page Global Enable (bit 7)  */
-	if ( cpu_has_pge ) {
-		cr4 = read_cr4();
-		write_cr4(cr4 & ~X86_CR4_PGE);
-	}
-
-	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
-	__flush_tlb();
+	/* Flush all TLBs */
+	__flush_tlb_all();
 
 	/*  Save MTRR state */
 	rdmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
@@ -368,7 +362,7 @@ static void prepare_set(void) __acquires
 static void post_set(void) __releases(set_atomicity_lock)
 {
 	/*  Flush TLBs (no need to flush caches - they are disabled)  */
-	__flush_tlb();
+	__flush_tlb_all();
 
 	/* Intel (P6) standard MTRRs */
 	mtrr_wrmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
@@ -376,9 +370,9 @@ static void post_set(void) __releases(se
 	/*  Enable caches  */
 	write_cr0(read_cr0() & 0xbfffffff);
 
-	/*  Restore value of CR4  */
-	if ( cpu_has_pge )
-		write_cr4(cr4);
+	/* Flush TLBs again to handle prefetches etc. */
+	__flush_tlb_all();
+
 	spin_unlock(&set_atomicity_lock);
 }
 

Re: [PATCH] Use global TLB flushes in MTRR code

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> On Thu, Feb 07, 2008 at 08:13:37PM +0100, Ingo Molnar wrote:
> > 
> > * Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > [probably stable material too]
> > > 
> > > Use global TLB flushes in MTRR code
> > > 
> > > Obviously kernel mappings should be flushed here too.
> > 
> > no, your patch is not needed:
> 
> Yes you're right.  Thanks makes more sense.  The weird style fooled 
> me.
> 
> It seems to come from the pseudo code in the Intel manual, but I think 
> it would be still cleaner/more idiomatic to use two __flush_tlb_all() 
> and remove the explicit code. The only drawback would be some more cr4 
> accesses, which does not seem like a big issue.
> 
> Updated patch follows.

... and this patch of yours breaks MTRR setting subtly:

> -	/*  Save value of CR4 and clear Page Global Enable (bit 7)  */
> -	if ( cpu_has_pge ) {
> -		cr4 = read_cr4();
> -		write_cr4(cr4 & ~X86_CR4_PGE);
> -	}
> -
> -	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
> -	__flush_tlb();
> +	/* Flush all TLBs */
> +	__flush_tlb_all();

because it's not just an open-coded __tlb_flush_all(), it _disables PGE 
and keeps it so while the MTRR's are changed on all CPUs_.

Your patch adds __flush_tlb_all() which re-enables the PGE bit in cr4, 
see asm-x86/tlbflush.h:

        /* clear PGE */
        write_cr4(cr4 & ~X86_CR4_PGE);
        /* write old PGE again and flush TLBs */
        write_cr4(cr4);

so we'll keep PGE enabled during the MTRR setting - which changes 
behavior.

	Ingo

Re: [PATCH] Use global TLB flushes in MTRR code

[Andi Kleen]

> because it's not just an open-coded __tlb_flush_all(), it _disables PGE 
> and keeps it so while the MTRR's are changed on all CPUs_.

Yes and? 

> 
> Your patch adds __flush_tlb_all() which re-enables the PGE bit in cr4, 
> see asm-x86/tlbflush.h:
> 
>         /* clear PGE */
>         write_cr4(cr4 & ~X86_CR4_PGE);
>         /* write old PGE again and flush TLBs */
>         write_cr4(cr4);
> 
> so we'll keep PGE enabled during the MTRR setting - which changes 
> behavior.

It changes behaviour in some minor ways but I don't think it makes any 
difference. PGE only influences TLB flushes (according to its 
specification) and all the TLB flushes still run with PGE disabled.

-Andi

Re: [PATCH] Use global TLB flushes in MTRR code

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> > because it's not just an open-coded __tlb_flush_all(), it _disables PGE 
> > and keeps it so while the MTRR's are changed on all CPUs_.
> 
> Yes and? 

your first patch was outright wrong then you declared the second one a 
"cleanup" while it changes behavior: bad in my book ;-)

> > Your patch adds __flush_tlb_all() which re-enables the PGE bit in cr4, 
> > see asm-x86/tlbflush.h:
> > 
> >         /* clear PGE */
> >         write_cr4(cr4 & ~X86_CR4_PGE);
> >         /* write old PGE again and flush TLBs */
> >         write_cr4(cr4);
> > 
> > so we'll keep PGE enabled during the MTRR setting - which changes 
> > behavior.
> 
> It changes behaviour in some minor ways but I don't think it makes any 
> difference. PGE only influences TLB flushes (according to its 
> specification) and all the TLB flushes still run with PGE disabled.

now that i pointed out the difference, your position changed to "changes 
behavior in minor ways" ;-)

This is fragile code and almost nothing in the MTRR area is "minor", we 
are just not touching this code unless it's really justified.

	Ingo

Re: [PATCH] Use global TLB flushes in MTRR code

[Andi Kleen]

On Sat, Feb 09, 2008 at 10:40:37AM +0100, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > because it's not just an open-coded __tlb_flush_all(), it _disables PGE 
> > > and keeps it so while the MTRR's are changed on all CPUs_.
> > 
> > Yes and? 
> 
> your first patch was outright wrong then you declared the second one a 
> "cleanup" while it changes behavior: bad in my book ;-)

I didn't claim that it didn't change the code -- you know
that I'm not a white space warrior, so I normally don't bother
with these kinds of patches -- just that it uses the usual Linux 
idioms for global TLB flushing.

> > > Your patch adds __flush_tlb_all() which re-enables the PGE bit in cr4, 
> > > see asm-x86/tlbflush.h:
> > > 
> > >         /* clear PGE */
> > >         write_cr4(cr4 & ~X86_CR4_PGE);
> > >         /* write old PGE again and flush TLBs */
> > >         write_cr4(cr4);
> > > 
> > > so we'll keep PGE enabled during the MTRR setting - which changes 
> > > behavior.
> > 
> > It changes behaviour in some minor ways but I don't think it makes any 
> > difference. PGE only influences TLB flushes (according to its 
> > specification) and all the TLB flushes still run with PGE disabled.
> 
> now that i pointed out the difference, your position changed to "changes 
> behavior in minor ways" ;-)

The instruction stream changes (more cr* accesses), but the actual flushes 
do not.  There is the exact same number of global TLB flushes (three
as requested by the Intel manual); just instead of in weird open coded 
style they are done in standard Linux style. 

I think it's an improvement because the old code fooled me at least,
so it's not 100% obvious.

> This is fragile code and almost nothing in the MTRR area is "minor", we 
> are just not touching this code unless it's really justified.

I don't think that's true actually; at least it doesn't match my experience
from maintaing that code for quite some time. MTRR was never particularly
fragile, just ugly.

Anyways I personally won't be fooled by that code again, so if 
you're not interested in (IMHO) cleaner and more readable and 
more maintaintable code then it's fine for me to not apply the patch.

-Andi

Re: [PATCH] Use global TLB flushes in MTRR code

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> > > It changes behaviour in some minor ways but I don't think it makes 
> > > any difference. PGE only influences TLB flushes (according to its 
> > > specification) and all the TLB flushes still run with PGE 
> > > disabled.
> > 
> > now that i pointed out the difference, your position changed to 
> > "changes behavior in minor ways" ;-)
> 
> The instruction stream changes (more cr* accesses), but the actual 
> flushes do not. [...]

You are still dead wrong. You refuse to realize the _obvious_ and 
_fundamental_ thing that the PGE bit does: it enables global TLBs while 
we change MTRRs!

That would be a completely new and totally untested modus operandi for a 
large array of x86 CPUs. You should read the Intel manual about the 
recommended way to change MTRRs (Vol. 3A 10-41) - it describes disabling 
the PGE during MTRR changes if it's enabled. The primary purpose of that 
is of course to flush the TLB entries - but still it's documented that 
way and the current code does it that way. It would be rather stupid for 
us to do otherwise: we simply cannot guarantee that x86 CPUs that 
implement MTRRs and PGE have no undocumented or _unknown_ erratas in 
this area. It _might_ be fine, while what you say is that it _is_ fine - 
which we simply cannot know. (And yes, there are documented CPU erratas 
related to global TLBs and MTRR's, so this area is far from being an 
unwritten page.)

So i refuse to apply such a pointless and risky "cleanup" patch from you 
to arch/x86 [which increases code size as well], _especially_ given how 
many times i've explained this very issue to you already and especially 
because you continue to mislead about the true effects of the patch.

	Ingo

Re: [PATCH] Use global TLB flushes in MTRR code

[Andi Kleen]

> That would be a completely new and totally untested modus operandi for a 
> large array of x86 CPUs. You should read the Intel manual about the 
> recommended way to change MTRRs (Vol. 3A 10-41) - it describes disabling 

I did -- that was the base I was working from.

BTW if you had read it closely you would have noticed that
the code as written currently violates the specification in a non trivial
way currently. If you worry so much about it that might be a good
area to investigate.

> the PGE during MTRR changes if it's enabled. The primary purpose of that 
> is of course to flush the TLB entries - but still it's documented that 
> way and the current code does it that way. It would be rather stupid for 

There are no global TLBs around during the MTRR change period because 
they have been all flushed just before. If any get prefetched they
will be in the TLB no matter if PGE is on or not.  That is because even
with PGE disabled TLBs are active. Then as soon as the MTRR changing
is done all TLBs (global or not) will be flushed.

[I admit I should have put that rationale into the original changelog,
then my thinking would have been clearer]

> us to do otherwise: we simply cannot guarantee that x86 CPUs that 
> implement MTRRs and PGE have no undocumented or _unknown_ erratas in 
> this area. It _might_ be fine, while what you say is that it _is_ fine - 
> which we simply cannot know. (And yes, there are documented CPU erratas 
> related to global TLBs and MTRR's, so this area is far from being an 
> unwritten page.)

If there are any global TLBs around they will be flushed at the 
end (even two times) 

Anyways the only theoretical problem I could think of if there is some CPU
that does not flush all global TLBs on a global TLB flush, but if that
really happens the only sane thing we could on that CPU is to never
use global TLBs. But I'm not aware of any x86 CPU that broken around
(are you?)

> So i refuse to apply such a pointless and risky "cleanup" patch from you 

Ok I cannot argue with you being so overly cautious. Caution
is a non exact heuristic that is hard to argue against.

I would appreciate though if you could clearly lay out which areas you
consider so risky that you think they cannot be changed anymore.

Does this cover all TLB flushes in general? Or only in some 
specific circumstances? If yes in which? 

Thanks.

That is just I can avoid stepping on your toes for those areas
for merely cleanups. They tend to be not important enough to warrant
a longer argument.

> to arch/x86 [which increases code size as well], 

Yes, a whole ~10 bytes or so in two non duplicated functions :-) I like
the code size argument as well as the next guy, but if you use it 
for rounding errors like this it blunts.

-Andi