Incorporate new understanding of/with Intel SMP spec.

Dropped cmpxchg in favor of xchg, to match lecture notes.

Use xchg to release lock, for future protection and to
keep gcc from acting clever.

5 files changed, 25 insertions, 36 deletions

diff --git a/TRICKS b/TRICKS
index b56a38c..6883588 100644
--- a/TRICKS
+++ b/TRICKS
@@ -102,5 +102,11 @@ after observing the earlier writes by CPU0.
 So any reads in B are guaranteed to observe the
 effects of writes in A.
 
-Not sure about the second one yet.
+According to the Intel manual behavior spec, the
+second condition requires a serialization instruction
+in release, to avoid reads in A happening after giving
+up lk.  No Intel SMP processor in existence actually
+moves reads down after writes, but the language in
+the spec allows it.  There is no telling whether future
+processors will need it.
 
diff --git a/main.c b/main.c
index 2108d95..6ec6d19 100644
--- a/main.c
+++ b/main.c
@@ -50,8 +50,9 @@ mpmain(void)
   if(cpu() != mp_bcpu())
     lapic_init(cpu());
   setupsegs(0);
-  cpus[cpu()].booted = 1;
+  xchg(&cpus[cpu()].booted, 1);
 
+  cprintf("cpu%d: scheduling\n");
   scheduler();
 }
 
diff --git a/proc.h b/proc.h
index fa60452..502361d 100644
--- a/proc.h
+++ b/proc.h
@@ -56,7 +56,7 @@ struct cpu {
   struct context context;     // Switch here to enter scheduler
   struct taskstate ts;        // Used by x86 to find stack for interrupt
   struct segdesc gdt[NSEGS];  // x86 global descriptor table
-  volatile int booted;        // Has the CPU started?
+  volatile uint booted;        // Has the CPU started?
   int ncli;                   // Depth of pushcli nesting.
   int intena;                 // Were interrupts enabled before pushcli? 
 };
diff --git a/spinlock.c b/spinlock.c
index c00c978..ba5bb4a 100644
--- a/spinlock.c
+++ b/spinlock.c
@@ -10,12 +10,6 @@
 
 extern int use_console_lock;
 
-// Barrier to gcc's instruction reordering.
-static void inline gccbarrier(void)
-{
-  asm volatile("" : : : "memory");
-}
-
 void
 initlock(struct spinlock *lock, char *name)
 {
@@ -35,7 +29,10 @@ acquire(struct spinlock *lock)
   if(holding(lock))
     panic("acquire");
 
-  while(cmpxchg(0, 1, &lock->locked) == 1)
+  // The xchg is atomic.
+  // It also serializes, so that reads after acquire are not
+  // reordered before it.  
+  while(xchg(&lock->locked, 1) == 1)
     ;
 
   // Record info about lock acquisition for debugging.
@@ -56,8 +53,12 @@ release(struct spinlock *lock)
   lock->pcs[0] = 0;
   lock->cpu = 0xffffffff;
 
-  gccbarrier();  // Keep gcc from moving lock->locked = 0 earlier.
-  lock->locked = 0;
+  // The xchg serializes, so that reads before release are 
+  // not reordered after it.  (This reordering would be allowed
+  // by the Intel manuals, but does not happen on current 
+  // Intel processors.  The xchg being asm volatile also keeps
+  // gcc from delaying the above assignments.)
+  xchg(&lock->locked, 0);
 
   popcli();
 }
diff --git a/x86.h b/x86.h
index a1c66b5..1f2c881 100644
--- a/x86.h
+++ b/x86.h
@@ -96,35 +96,16 @@ write_eflags(uint eflags)
   asm volatile("pushl %0; popfl" : : "r" (eflags));
 }
 
-// XXX: Kill this if not used.
-static inline void
-cpuid(uint info, uint *eaxp, uint *ebxp, uint *ecxp, uint *edxp)
-{
-  uint eax, ebx, ecx, edx;
-
-  asm volatile("cpuid" :
-               "=a" (eax), "=b" (ebx), "=c" (ecx), "=d" (edx) :
-               "a" (info));
-  if(eaxp)
-    *eaxp = eax;
-  if(ebxp)
-    *ebxp = ebx;
-  if(ecxp)
-    *ecxp = ecx;
-  if(edxp)
-    *edxp = edx;
-}
-
 static inline uint
-cmpxchg(uint oldval, uint newval, volatile uint* lock_addr)
+xchg(volatile uint *addr, uint newval)
 {
   uint result;
   
   // The + in "+m" denotes a read-modify-write operand.
-  asm volatile("lock; cmpxchgl %2, %0" :
-                       "+m" (*lock_addr), "=a" (result) :
-                       "r"(newval), "1"(oldval) :
-                       "cc");
+  asm volatile("lock; xchgl %0, %1" :
+               "+m" (*addr), "=a" (result) :
+               "1" (newval) :
+               "cc");
   return result;
 }