Skip to contentSkip to conversation

linux-2.6.0-test2_mtrr-race-fix_A0

RFC PATCH New Mathias Fröhlich 4 participants 8 messages Aug 6, 2003 – Aug 7, 2003

lore
Share & tools
lore

Mathias Fröhlich

author
#0 lore raw

Hi,

You should not remove the barrier past mtrr change. If you do that, it is possible that cpu's run with inconsistent mtrrs. This can have bad sideeffects since at least the cache snooping protocol used by intel uses assumptions about the cachability of memory regions. Those information about the cachability is also taken from the mtrrs as far as I remember. This intel cpu developer manual, which documented the early PII and PPro chips, recommended this algorithm. Since actual intel cpus use the same old cpu to chipset bus protocol, this old documentation most propably still applies.

So the conclusion is that as far as you don't know the exact way all those SMP protocols between chipsets and CPUs with all the possible sideeffects very well, dont't change this behavour.

But I'm shure that fixes to the stack allocated variable problem are welcome :)

Greetings

Mathias Fröhlich

Signature · 2 lines
Mathias Fröhlich, email: [email protected]
old email was: [email protected]
#1 lore raw
Quoted 11 lines · Mathias Fröhlich

Hi,

You should not remove the barrier past mtrr change. If you do that, it is possible that cpu's run with inconsistent mtrrs. This can have bad sideeffects since at least the cache snooping protocol used by intel uses assumptions about the cachability of memory regions. Those information about the cachability is also taken from the mtrrs as far as I remember. This intel cpu developer manual, which documented the early PII and PPro chips, recommended this algorithm. Since actual intel cpus use the same old cpu to chipset bus protocol, this old documentation most propably still applies.

Hmm. I should dig up that doc. Its a little hazy in my mind, but I think I understand your description. I'm glad you caught this, as I can't imagine the subtle bugs that might have popped up.

Well, let me look at it again and see if I can come up with a proper fix.

Thanks for the knowledgeable feedback and sanity checking! -john

#2 lore raw
Quoted 17 lines · john stultz
Quoted 11 lines · Mathias Fröhlich

Hi,

You should not remove the barrier past mtrr change. If you do that, it is possible that cpu's run with inconsistent mtrrs. This can have bad sideeffects since at least the cache snooping protocol used by intel uses assumptions about the cachability of memory regions. Those information about the cachability is also taken from the mtrrs as far as I remember. This intel cpu developer manual, which documented the early PII and PPro chips, recommended this algorithm. Since actual intel cpus use the same old cpu to chipset bus protocol, this old documentation most propably still applies.

Hmm. I should dig up that doc. Its a little hazy in my mind, but I think I understand your description. I'm glad you caught this, as I can't imagine the subtle bugs that might have popped up.

Quoted 5 lines

Well, let me look at it again and see if I can come up with a proper fix.

Thanks for the knowledgeable feedback and sanity checking! -john

I added an extra sync up from the caller after the last gate change so it is the last one to touch the automatic data.

Mark.

===== arch/i386/kernel/cpu/mtrr/main.c 1.29 vs edited =====

arch/i386/kernel/cpu/mtrr/main.c +12 -1
@@ -169,6 +169,7 @@
 		cpu_relax();
 		barrier();
 	}
+	atomic_dec(&data->count);
 	local_irq_restore(flags);
 }
 

@@ -256,8 +257,18 @@
 		cpu_relax();
 		barrier();
 	}
-	local_irq_restore(flags);
+	atomic_set(&data.count, num_booting_cpus() - 1);
 	atomic_set(&data.gate,0);
+
+	/* 
+	 * Wait here for everyone to have seen the gate change
+	 * So we're the last ones to touch 'data'
+	 */
+	while(atomic_read(&data.count)) {
+		cpu_relax();
+		barrier();
+	}
+	local_irq_restore(flags);
 }
 
 /**
Signature · 1 line
Mark Haverkamp <[email protected]>
#3 lore raw
Quoted 6 lines · Mark Haverkamp
Quoted 3 lines · john stultz

Well, let me look at it again and see if I can come up with a proper fix.

I added an extra sync up from the caller after the last gate change so it is the last one to touch the automatic data.

Ah, you beat me to it!

I'm actually testing the very same change (comments differ a touch, but that's ok).

Looks good. If everyone is happy I'd say resubmit it to Andrew.

thanks
-john

#4 lore raw

PATCH 1/1 linux-2.6.0-test2_mtrr-race-fix_A0

Quoted 10 lines · john stultz
Quoted 6 lines · Mark Haverkamp
Quoted 3 lines · john stultz

Well, let me look at it again and see if I can come up with a proper fix.

I added an extra sync up from the caller after the last gate change so it is the last one to touch the automatic data.

Ah, you beat me to it!

Quoted 7 lines

I'm actually testing the very same change (comments differ a touch, but that's ok).

Looks good. If everyone is happy I'd say resubmit it to Andrew.

thanks
-john

I'd like to submit this patch for inclusion. It adds an extra sync up in set_mtrr and ipi_handler to make sure that set_mtrr is the last to touch the automatic set_mtrr_data.

===== arch/i386/kernel/cpu/mtrr/main.c 1.29 vs edited =====

arch/i386/kernel/cpu/mtrr/main.c +12 -1
@@ -169,6 +169,7 @@
 		cpu_relax();
 		barrier();
 	}
+	atomic_dec(&data->count);
 	local_irq_restore(flags);
 }
 

@@ -256,8 +257,18 @@
 		cpu_relax();
 		barrier();
 	}
-	local_irq_restore(flags);
+	atomic_set(&data.count, num_booting_cpus() - 1);
 	atomic_set(&data.gate,0);
+
+	/* 
+	 * Wait here for everyone to have seen the gate change
+	 * So we're the last ones to touch 'data'
+	 */
+	while(atomic_read(&data.count)) {
+		cpu_relax();
+		barrier();
+	}
+	local_irq_restore(flags);
 }
 
 /**
Signature · 1 line
Mark Haverkamp <[email protected]>
#5 lore raw
In reply to john stultz
Quoted 12 lines · john stultz
Quoted 9 lines · Mathias Fröhlich

You should not remove the barrier past mtrr change. If you do that, it is possible that cpu's run with inconsistent mtrrs. This can have bad sideeffects since at least the cache snooping protocol used by intel uses assumptions about the cachability of memory regions. Those information about the cachability is also taken from the mtrrs as far as I remember. This intel cpu developer manual, which documented the early PII and PPro chips, recommended this algorithm. Since actual intel cpus use the same old cpu to chipset bus protocol, this old documentation most propably still applies.

Quoted 6 lines

Hmm. I should dig up that doc. Its a little hazy in my mind, but I think I understand your description. I'm glad you caught this, as I can't imagine the subtle bugs that might have popped up.

Well, let me look at it again and see if I can come up with a proper fix.

The intel manual (10-36 Memory Cache Control - vol3) actually recommends the following procedure I was a bit anal about explicitely setting and clearing flags and used the specific TLB flush via cr3->reg->cr3. John could you give this a spin on your afflicted systems?

Index: linux-2.5/arch/i386/kernel/cpu/mtrr/generic.c
===================================================================
RCS file: /home/cvs/linux-2.5/arch/i386/kernel/cpu/mtrr/generic.c,v retrieving revision 1.8

arch/i386/kernel/cpu/mtrr/generic.c +17 -9
diff -u -p -B -r1.8 generic.c
@@ -8,6 +8,7 @@
 #include <asm/msr.h>
 #include <asm/system.h>
 #include <asm/cpufeature.h>
+#include <asm/tlbflush.h>
 #include "mtrr.h"
 
 struct mtrr_state {

@@ -241,19 +242,22 @@ static void prepare_set(void)
 	   more invasive changes to the way the kernel boots  */
 	spin_lock(&set_atomicity_lock);
 
+	/*  Enter the no-fill (CD=1, NW=0) cache mode and flush caches. */
+	cr0 = read_cr0() | 0x40000000;	/* set CD flag */
+	cr0 &= ~(0x20000000);		/* clear NW flag */
+	wbinvd();
+	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 & (unsigned char) ~(1 << 7));
 	}
 
-	/*  Disable and flush caches. Note that wbinvd flushes the TLBs as
-	    a side-effect  */
-	cr0 = read_cr0() | 0x40000000;
-	wbinvd();
-	write_cr0(cr0);
-	wbinvd();
-
+	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
+	__flush_tlb();
+	
 	/*  Save MTRR state */
 	rdmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
 

@@ -263,14 +267,18 @@ static void prepare_set(void)
 
 static void post_set(void)
 {
+	unsigned long cr0;
+
 	/*  Flush caches and TLBs  */
 	wbinvd();
+	__flush_tlb();
 
 	/* Intel (P6) standard MTRRs */
 	wrmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
 		
-	/*  Enable caches  */
-	write_cr0(read_cr0() & 0xbfffffff);
+	/*  Enable caches, clear previously set CD flag */
+	cr0 = read_cr0() & ~(0x40000000);
+	write_cr0(cr0);
 
 	/*  Restore value of CR4  */
 	if ( cpu_has_pge )
#6 lore raw
Quoted 4 lines · Zwane Mwaikambo

The intel manual (10-36 Memory Cache Control - vol3) actually recommends the following procedure I was a bit anal about explicitely setting and clearing flags and used the specific TLB flush via cr3->reg->cr3. John could you give this a spin on your afflicted systems?

I'm not quite sure I follow. Really I've never looked at the mtrr code before, so forgive my daftness. I just found a deadlock that was locking my box caused by the synchronization between ipi_handler() and set_mtrr() (which Mark's patch seems to properly fix).

How does this change affect the deadlock? Is this just a separate issue? thanks
-john

#7 lore raw
Quoted 6 lines · john stultz
Quoted 4 lines · Zwane Mwaikambo

The intel manual (10-36 Memory Cache Control - vol3) actually recommends the following procedure I was a bit anal about explicitely setting and clearing flags and used the specific TLB flush via cr3->reg->cr3. John could you give this a spin on your afflicted systems?

Quoted 7 lines

I'm not quite sure I follow. Really I've never looked at the mtrr code before, so forgive my daftness. I just found a deadlock that was locking my box caused by the synchronization between ipi_handler() and set_mtrr() (which Mark's patch seems to properly fix).

How does this change affect the deadlock? Is this just a separate issue? thanks

Mostly seperate issue, but seeing as we where there, there is a slight deviation from recommended procedure. Sorry about that, you may ignore it.

Thanks
Zwane

Signature · 1 line
function.linuxpower.ca