[probably stable material too]
Use global TLB flushes in MTRR code
Obviously kernel mappings should be flushed here too.
Signed-off-by: Andi Kleen <[email protected]>
---
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);
and old-stable (eg 2.6.22)?
On 2/7/08, Andi Kleen <[email protected]> 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 <[email protected]>
>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Thanks,
Oliver
* Andi Kleen <[email protected]> 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
On Thu, Feb 07, 2008 at 08:13:37PM +0100, Ingo Molnar wrote:
>
> * Andi Kleen <[email protected]> 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 <[email protected]>
---
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);
}
* Andi Kleen <[email protected]> wrote:
> On Thu, Feb 07, 2008 at 08:13:37PM +0100, Ingo Molnar wrote:
> >
> > * Andi Kleen <[email protected]> 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
> 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
* Andi Kleen <[email protected]> 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
On Sat, Feb 09, 2008 at 10:40:37AM +0100, Ingo Molnar wrote:
>
> * Andi Kleen <[email protected]> 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
* Andi Kleen <[email protected]> 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
> 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