From 9981bb227093373649acd019d378dfccb54189c8 Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Wed, 10 Jul 2019 08:57:51 -0400
Subject: [PATCH 1/9] tweak some comments.

---
 kernel/proc.c       | 20 ++++++++------------
 kernel/proc.h       | 32 ++++++++++++++++++--------------
 kernel/trampoline.S |  2 +-
 kernel/trap.c       |  2 +-
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/kernel/proc.c b/kernel/proc.c
index a947f7f..5c2d4ce 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -6,20 +6,16 @@
 #include "proc.h"
 #include "defs.h"
 
-struct proc proc[NPROC];
-
 struct cpu cpus[NCPU];
 
+struct proc proc[NPROC];
+
 struct proc *initproc;
 
-struct spinlock pid_lock;
 int nextpid = 1;
+struct spinlock pid_lock;
 
 extern void forkret(void);
-
-// for returning  out of the kernel
-extern void sysexit(void);
-
 static void wakeup1(struct proc *chan);
 
 extern char trampout[]; // trampoline.S
@@ -287,8 +283,8 @@ fork(void)
   return pid;
 }
 
-// Pass p's abandoned children to init. p and p's parent
-// are locked.
+// Pass p's abandoned children to init.
+// Caller must hold p->lock and parent->lock.
 void
 reparent(struct proc *p, struct proc *parent) {
   struct proc *pp;
@@ -536,7 +532,7 @@ sleep(void *chan, struct spinlock *lk)
 
 //PAGEBREAK!
 // Wake up p, used by exit()
-// Caller should lock p.
+// Caller must hold p->lock.
 static void
 wakeup1(struct proc *p)
 {
@@ -545,8 +541,8 @@ wakeup1(struct proc *p)
   }
 }
 
-// Wake up all processes sleeping on chan. Never
-// called when holding a p->lock
+// Wake up all processes sleeping on chan.
+// Must be called without any p->lock.
 void
 wakeup(void *chan)
 {
diff --git a/kernel/proc.h b/kernel/proc.h
index 687fdd1..373b605 100644
--- a/kernel/proc.h
+++ b/kernel/proc.h
@@ -18,33 +18,37 @@ struct context {
   uint64 s11;
 };
 
-// Per-CPU state
+// Per-CPU state.
 struct cpu {
-  struct proc *proc;           // The process running on this cpu or null
-  struct context scheduler;   // swtch() here to enter scheduler
-  int noff;                    // Depth of push_off() nesting.
-  int intena;                  // Were interrupts enabled before push_off()?
+  struct proc *proc;          // The process running on this cpu, or null.
+  struct context scheduler;   // swtch() here to enter scheduler().
+  int noff;                   // Depth of push_off() nesting.
+  int intena;                 // Were interrupts enabled before push_off()?
 };
 
 extern struct cpu cpus[NCPU];
 
 //PAGEBREAK: 17
 
-// per-process data for the early trap handling code in trampoline.S.
+// per-process data for the trap handling code in trampoline.S.
 // sits in a page by itself just under the trampoline page in the
 // user page table. not specially mapped in the kernel page table.
 // the sscratch register points here.
-// trampoline.S saves user registers, then restores kernel_sp and
-// kernel_satp.
-// includes callee-saved registers like s0-s11 because the
+// trampin in trampoline.S saves user registers in the trapframe,
+// then initializes registers from the trapframe's
+// kernel_sp, kernel_hartid, kernel_satp, and jumps to kernel_trap.
+// usertrapret() and trampout in trampoline.S set up
+// the trapframe's kernel_*, restore user registers from the
+// trapframe, switch to the user page table, and enter user space.
+// the trapframe includes callee-saved user registers like s0-s11 because the
 // return-to-user path via usertrapret() doesn't return through
 // the entire kernel call stack.
 struct trapframe {
-  /*   0 */ uint64 kernel_satp;
-  /*   8 */ uint64 kernel_sp;
-  /*  16 */ uint64 kernel_trap; // usertrap()
-  /*  24 */ uint64 epc; // saved user program counter
-  /*  32 */ uint64 hartid;
+  /*   0 */ uint64 kernel_satp;   // kernel page table
+  /*   8 */ uint64 kernel_sp;     // top of process's kernel stack
+  /*  16 */ uint64 kernel_trap;   // usertrap()
+  /*  24 */ uint64 epc;           // saved user program counter
+  /*  32 */ uint64 kernel_hartid; // saved kernel tp
   /*  40 */ uint64 ra;
   /*  48 */ uint64 sp;
   /*  56 */ uint64 gp;
diff --git a/kernel/trampoline.S b/kernel/trampoline.S
index dd4eb02..b992ea6 100644
--- a/kernel/trampoline.S
+++ b/kernel/trampoline.S
@@ -120,7 +120,7 @@ trampin:
         # restore kernel stack pointer from p->tf->kernel_sp
         ld sp, 8(a0)
 
-        # make tp hold the current hartid, from p->tf->hartid
+        # make tp hold the current hartid, from p->tf->kernel_hartid
         ld tp, 32(a0)
 
         # remember the address of usertrap(), p->tf->kernel_trap
diff --git a/kernel/trap.c b/kernel/trap.c
index 018b7db..27cfd32 100644
--- a/kernel/trap.c
+++ b/kernel/trap.c
@@ -100,7 +100,7 @@ usertrapret(void)
   p->tf->kernel_satp = r_satp();
   p->tf->kernel_sp = (uint64)p->kstack + PGSIZE;
   p->tf->kernel_trap = (uint64)usertrap;
-  p->tf->hartid = r_tp();
+  p->tf->kernel_hartid = r_tp();
 
   // set up the registers that trampoline.S's sret will use
   // to get to user space.

From 5eb1685700a7665814f1bcf63fc26f5dbf0f719a Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Wed, 10 Jul 2019 09:24:50 -0400
Subject: [PATCH 2/9] have kill() lock before looking at p->pid document
 wait()'s use of np->parent w/o holding lock.

---
 kernel/proc.c    | 49 ++++++++++++++++++++++++++----------------------
 kernel/syscall.c | 16 ++--------------
 kernel/trap.c    |  3 +++
 3 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/kernel/proc.c b/kernel/proc.c
index 5c2d4ce..1c222d3 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -367,19 +367,24 @@ wait(void)
     // Scan through table looking for exited children.
     havekids = 0;
     for(np = proc; np < &proc[NPROC]; np++){
-      if(np->parent != p)
-        continue;
-      acquire(&np->lock);
-      havekids = 1;
-      if(np->state == ZOMBIE){
-        // Found one.
-        pid = np->pid;
-        freeproc(np);
+      // this code uses np->parent without holding np->lock.
+      // acquiring the lock first would cause a deadlock,
+      // since np might be an ancestor, and we already hold p->lock.
+      if(np->parent == p){
+        // np->parent can't change here because only the parent
+        // changes it, and we're the parent.
+        acquire(&np->lock);
+        havekids = 1;
+        if(np->state == ZOMBIE){
+          // Found one.
+          pid = np->pid;
+          freeproc(np);
+          release(&np->lock);
+          release(&p->lock);
+          return pid;
+        }
         release(&np->lock);
-        release(&p->lock);
-        return pid;
       }
-      release(&np->lock);
     }
 
     // No point waiting if we don't have any children.
@@ -397,10 +402,10 @@ wait(void)
 // Per-CPU process scheduler.
 // Each CPU calls scheduler() after setting itself up.
 // Scheduler never returns.  It loops, doing:
-//  - choose a process to run
-//  - swtch to start running that process
+//  - choose a process to run.
+//  - swtch to start running that process.
 //  - eventually that process transfers control
-//      via swtch back to the scheduler.
+//    via swtch back to the scheduler.
 void
 scheduler(void)
 {
@@ -409,7 +414,7 @@ scheduler(void)
   
   c->proc = 0;
   for(;;){
-    // Enable interrupts on this processor.
+    // Give devices a brief chance to interrupt.
     intr_on();
 
     for(p = proc; p < &proc[NPROC]; p++) {
@@ -508,7 +513,7 @@ sleep(void *chan, struct spinlock *lk)
   // change p->state and then call sched.
   // Once we hold p->lock, we can be
   // guaranteed that we won't miss any wakeup
-  // (wakeup runs with p->lock locked),
+  // (wakeup locks p->lock),
   // so it's okay to release lk.
   if(lk != &p->lock){  //DOC: sleeplock0
     acquire(&p->lock);  //DOC: sleeplock1
@@ -559,24 +564,24 @@ wakeup(void *chan)
 
 // Kill the process with the given pid.
 // Process won't exit until it returns
-// to user space (see trap in trap.c).
+// to user space (see usertrap() in trap.c).
 int
 kill(int pid)
 {
   struct proc *p;
 
   for(p = proc; p < &proc[NPROC]; p++){
+    acquire(&p->lock);
     if(p->pid == pid){
-      acquire(&p->lock);
-      if(p->pid != pid)
-        panic("kill");
       p->killed = 1;
-      // Wake process from sleep if necessary.
-      if(p->state == SLEEPING)
+      if(p->state == SLEEPING){
+        // Wake process from sleep().
         p->state = RUNNABLE;
+      }
       release(&p->lock);
       return 0;
     }
+    release(&p->lock);
   }
   return -1;
 }
diff --git a/kernel/syscall.c b/kernel/syscall.c
index a054da2..adbad73 100644
--- a/kernel/syscall.c
+++ b/kernel/syscall.c
@@ -163,8 +163,8 @@ static uint64 (*syscalls[])(void) = {
 [SYS_close]   sys_close,
 };
 
-static void
-dosyscall(void)
+void
+syscall(void)
 {
   int num;
   struct proc *p = myproc();
@@ -180,15 +180,3 @@ dosyscall(void)
     p->tf->a0 = -1;
   }
 }
-
-void
-syscall()
-{
-    if(myproc()->killed)
-      exit();
-    dosyscall();
-    if(myproc()->killed)
-      exit();
-    return;
-}
-
diff --git a/kernel/trap.c b/kernel/trap.c
index 27cfd32..eb7f6cf 100644
--- a/kernel/trap.c
+++ b/kernel/trap.c
@@ -53,6 +53,9 @@ usertrap(void)
   if(r_scause() == 8){
     // system call
 
+    if(p->killed)
+      exit();
+
     // sepc points to the ecall instruction,
     // but we want to return to the next instruction.
     p->tf->epc += 4;

From c0266a877a25fdc0d73016bc7e7bf0f1800b3e0e Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Wed, 10 Jul 2019 09:28:00 -0400
Subject: [PATCH 3/9] document which proc fields are protected by p->lock

---
 kernel/proc.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/proc.h b/kernel/proc.h
index 373b605..1524c74 100644
--- a/kernel/proc.h
+++ b/kernel/proc.h
@@ -87,16 +87,20 @@ enum procstate { UNUSED, SLEEPING, RUNNABLE, RUNNING, ZOMBIE };
 // Per-process state
 struct proc {
   struct spinlock lock;
+
+  // p->lock must be held when using these:
+  enum procstate state;        // Process state
+  struct proc *parent;         // Parent process
+  void *chan;                  // If non-zero, sleeping on chan
+  int killed;                  // If non-zero, have been killed
+  int pid;                     // Process ID
+
+  // these are private to the process, so p->lock need not be held.
   char *kstack;                // Bottom of kernel stack for this process
   uint64 sz;                   // Size of process memory (bytes)
   pagetable_t pagetable;       // Page table
-  enum procstate state;        // Process state
-  int pid;                     // Process ID
-  struct proc *parent;         // Parent process
   struct trapframe *tf;        // data page for trampoline.S
   struct context context;      // swtch() here to run process
-  void *chan;                  // If non-zero, sleeping on chan
-  int killed;                  // If non-zero, have been killed
   struct file *ofile[NOFILE];  // Open files
   struct inode *cwd;           // Current directory
   char name[16];               // Process name (debugging)

From 061e3be6f89cc0b7479fc2bba8b1348f7a9d070d Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Wed, 10 Jul 2019 10:13:08 -0400
Subject: [PATCH 4/9] more comment cleanup

---
 kernel/proc.c | 54 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/kernel/proc.c b/kernel/proc.c
index 1c222d3..46ac128 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -73,7 +73,7 @@ allocpid() {
 // Look in the process table for an UNUSED proc.
 // If found, initialize state required to run in the kernel,
 // and return with p->lock held.
-// Otherwise return 0.
+// If there are no free procs, return 0.
 static struct proc*
 allocproc(void)
 {
@@ -94,6 +94,7 @@ found:
 
   // Allocate a page for the kernel stack.
   if((p->kstack = kalloc()) == 0){
+    release(&p->lock);
     return 0;
   }
 
@@ -101,6 +102,7 @@ found:
   if((p->tf = (struct trapframe *)kalloc()) == 0){
     kfree(p->kstack);
     p->kstack = 0;
+    release(&p->lock);
     return 0;
   }
 
@@ -141,16 +143,13 @@ freeproc(struct proc *p)
 }
 
 // Create a page table for a given process,
-// with no users pages, but with trampoline pages.
-// Called both when creating a process, and
-// by exec() when building tentative new memory image,
-// which might fail.
+// with no user pages, but with trampoline pages.
 pagetable_t
 proc_pagetable(struct proc *p)
 {
   pagetable_t pagetable;
 
-  // An empty user page table.
+  // An empty page table.
   pagetable = uvmcreate();
 
   // map the trampoline code (for system call return)
@@ -168,9 +167,7 @@ proc_pagetable(struct proc *p)
 }
 
 // Free a process's page table, and free the
-// physical memory the page table refers to.
-// Called both when a process exits and from
-// exec() if it fails.
+// physical memory it refers to.
 void
 proc_freepagetable(pagetable_t pagetable, uint64 sz)
 {
@@ -183,9 +180,12 @@ proc_freepagetable(pagetable_t pagetable, uint64 sz)
 // a user program that calls exec("/init")
 // od -t xC initcode
 uchar initcode[] = {
-  0x17, 0x05, 0x00, 0x00, 0x13, 0x05, 0x05, 0x02, 0x97, 0x05, 0x00, 0x00, 0x93, 0x85, 0x05, 0x02,
-  0x9d, 0x48, 0x73, 0x00, 0x00, 0x00, 0x89, 0x48, 0x73, 0x00, 0x00, 0x00, 0xef, 0xf0, 0xbf, 0xff,
-  0x2f, 0x69, 0x6e, 0x69, 0x74, 0x00, 0x00, 0x01, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+  0x17, 0x05, 0x00, 0x00, 0x13, 0x05, 0x05, 0x02,
+  0x97, 0x05, 0x00, 0x00, 0x93, 0x85, 0x05, 0x02,
+  0x9d, 0x48, 0x73, 0x00, 0x00, 0x00, 0x89, 0x48,
+  0x73, 0x00, 0x00, 0x00, 0xef, 0xf0, 0xbf, 0xff,
+  0x2f, 0x69, 0x6e, 0x69, 0x74, 0x00, 0x00, 0x01,
+  0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   0x00, 0x00, 0x00
 };
 
@@ -199,12 +199,14 @@ userinit(void)
   p = allocproc();
   initproc = p;
   
+  // allocate one user page and copy init's instructions
+  // and data into it.
   uvminit(p->pagetable, initcode, sizeof(initcode));
   p->sz = PGSIZE;
 
   // prepare for the very first "return" from kernel to user.
-  p->tf->epc = 0;
-  p->tf->sp = PGSIZE;
+  p->tf->epc = 0;      // user program counter
+  p->tf->sp = PGSIZE;  // user stack pointer
 
   safestrcpy(p->name, "initcode", sizeof(p->name));
   p->cwd = namei("/");
@@ -214,7 +216,7 @@ userinit(void)
   release(&p->lock);
 }
 
-// Grow current process's memory by n bytes.
+// Grow or shrink user memory by n bytes.
 // Return 0 on success, -1 on failure.
 int
 growproc(int n)
@@ -236,8 +238,8 @@ growproc(int n)
   return 0;
 }
 
-// Create a new process, copying p as the parent.
-// Sets up child kernel stack to return as if from system call.
+// Create a new process, copying the parent.
+// Sets up child kernel stack to return as if from fork() system call.
 int
 fork(void)
 {
@@ -308,7 +310,6 @@ reparent(struct proc *p, struct proc *parent) {
   }
 }
 
-
 // Exit the current process.  Does not return.
 // An exited process remains in the zombie state
 // until its parent calls wait().
@@ -339,6 +340,7 @@ exit(void)
     
   acquire(&p->lock);
 
+  // Give our children to init.
   reparent(p, p->parent);
 
   p->state = ZOMBIE;
@@ -362,7 +364,10 @@ wait(void)
   int havekids, pid;
   struct proc *p = myproc();
 
+  // hold p->lock for the whole time to avoid lost
+  // wakeups from a child's exit().
   acquire(&p->lock);
+
   for(;;){
     // Scan through table looking for exited children.
     havekids = 0;
@@ -371,8 +376,8 @@ wait(void)
       // acquiring the lock first would cause a deadlock,
       // since np might be an ancestor, and we already hold p->lock.
       if(np->parent == p){
-        // np->parent can't change here because only the parent
-        // changes it, and we're the parent.
+        // np->parent can't change between the check and the acquire()
+        // because only the parent changes it, and we're the parent.
         acquire(&np->lock);
         havekids = 1;
         if(np->state == ZOMBIE){
@@ -393,7 +398,7 @@ wait(void)
       return -1;
     }
     
-    // Wait for children to exit.  (See wakeup1 call in reparent.)
+    // Wait for a child to exit.
     sleep(p, &p->lock);  //DOC: wait-sleep
   }
 }
@@ -436,7 +441,7 @@ scheduler(void)
   }
 }
 
-// Enter scheduler.  Must hold only p->lock
+// Switch to scheduler.  Must hold only p->lock
 // and have changed proc->state. Saves and restores
 // intena because intena is a property of this
 // kernel thread, not this CPU. It should
@@ -519,6 +524,7 @@ sleep(void *chan, struct spinlock *lk)
     acquire(&p->lock);  //DOC: sleeplock1
     release(lk);
   }
+
   // Go to sleep.
   p->chan = chan;
   p->state = SLEEPING;
@@ -536,7 +542,7 @@ sleep(void *chan, struct spinlock *lk)
 }
 
 //PAGEBREAK!
-// Wake up p, used by exit()
+// Wake up p, used by exit().
 // Caller must hold p->lock.
 static void
 wakeup1(struct proc *p)
@@ -563,7 +569,7 @@ wakeup(void *chan)
 }
 
 // Kill the process with the given pid.
-// Process won't exit until it returns
+// The victim won't exit until it tries to return
 // to user space (see usertrap() in trap.c).
 int
 kill(int pid)

From e6addf25bb1332f8722ea28f2b431d93984f89e5 Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Wed, 10 Jul 2019 10:30:33 -0400
Subject: [PATCH 5/9] feeble attempt at build instructions

---
 README | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/README b/README
index 73e134e..d79cfde 100644
--- a/README
+++ b/README
@@ -31,7 +31,7 @@ Toomey, Stephen Tu, Pablo Ventura, Xi Wang, Keiichi Watanabe, Nicolas
 Wolovick, wxdao, Grant Wu, Jindong Zhang, Icenowy Zheng, and Zou Chang Wei.
 
 The code in the files that constitute xv6 is
-Copyright 2006-2018 Frans Kaashoek, Robert Morris, and Russ Cox.
+Copyright 2006-2019 Frans Kaashoek, Robert Morris, and Russ Cox.
 
 ERROR REPORTS
 
@@ -42,9 +42,7 @@ simplifications and clarifications than new features.
 
 BUILDING AND RUNNING XV6
 
-To build xv6 on an x86 ELF machine (like Linux or FreeBSD), run
-"make". On non-x86 or non-ELF machines (like OS X, even on x86), you
-will need to install a cross-compiler gcc suite capable of producing
-x86 ELF binaries (see https://pdos.csail.mit.edu/6.828/).
-Then run "make TOOLPREFIX=i386-jos-elf-". Now install the QEMU PC
-simulator and run "make qemu".
+You will need a RISC-V "newlib" tool chain from
+https://github.com/riscv/riscv-gnu-toolchain, and qemu compiled for
+riscv64-softmmu. Once they are installed, and in your shell
+search path, you can run "make qemu".

From 4bc900e78bdbff3ba22ccccd26833cf70fd300b1 Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Wed, 10 Jul 2019 14:54:34 -0400
Subject: [PATCH 6/9] nits

---
 kernel/proc.c     | 4 +++-
 kernel/spinlock.c | 4 +---
 kernel/spinlock.h | 2 --
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/proc.c b/kernel/proc.c
index 46ac128..0655783 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -64,8 +64,10 @@ allocpid() {
   int pid;
   
   acquire(&pid_lock);
-  pid = nextpid++;
+  pid = nextpid;
+  nextpid = nextpid + 1;
   release(&pid_lock);
+
   return pid;
 }
 
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index 5a44a46..52bd504 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -18,8 +18,6 @@ initlock(struct spinlock *lk, char *name)
 
 // Acquire the lock.
 // Loops (spins) until the lock is acquired.
-// Holding a lock for a long time may cause
-// other CPUs to waste time spinning to acquire it.
 void
 acquire(struct spinlock *lk)
 {
@@ -81,7 +79,7 @@ holding(struct spinlock *lk)
 }
 
 // push_off/pop_off are like intr_off()/intr_on() except that they are matched:
-// it takes two pop_off to undo two push_off.  Also, if interrupts
+// it takes two pop_off()s to undo two push_off()s.  Also, if interrupts
 // are initially off, then push_off, pop_off leaves them off.
 
 void
diff --git a/kernel/spinlock.h b/kernel/spinlock.h
index 5f244c9..4392820 100644
--- a/kernel/spinlock.h
+++ b/kernel/spinlock.h
@@ -5,7 +5,5 @@ struct spinlock {
   // For debugging:
   char *name;        // Name of lock.
   struct cpu *cpu;   // The cpu holding the lock.
-  struct cpu *last_release;
-  uint64 last_pc;
 };
 

From 7797a384236cee31b924d27d8f814ef9543662cd Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Thu, 11 Jul 2019 05:41:59 -0400
Subject: [PATCH 7/9] another test, to help with locking exercises

---
 kernel/kalloc.c   |  4 +++-
 kernel/spinlock.c |  9 +++++----
 user/usertests.c  | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/kernel/kalloc.c b/kernel/kalloc.c
index afadb02..d72e0ab 100644
--- a/kernel/kalloc.c
+++ b/kernel/kalloc.c
@@ -55,8 +55,9 @@ kfree(void *pa)
   // Fill with junk to catch dangling refs.
   memset(pa, 1, PGSIZE);
 
-  acquire(&kmem.lock);
   r = (struct run*)pa;
+
+  acquire(&kmem.lock);
   r->next = kmem.freelist;
   kmem.freelist = r;
   release(&kmem.lock);
@@ -75,6 +76,7 @@ kalloc(void)
   if(r)
     kmem.freelist = r->next;
   release(&kmem.lock);
+
   if(r)
     memset((char*)r, 5, PGSIZE); // fill with junk
   return (void*)r;
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index 52bd504..83512bb 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -25,7 +25,7 @@ acquire(struct spinlock *lk)
   if(holding(lk))
     panic("acquire");
 
-  // On RISC-V, this turns into an atomic swap:
+  // On RISC-V, sync_lock_test_and_set turns into an atomic swap:
   //   a5 = 1
   //   s1 = &lk->locked
   //   amoswap.w.aq a5, a5, (s1)
@@ -57,9 +57,10 @@ release(struct spinlock *lk)
   __sync_synchronize();
 
   // Release the lock, equivalent to lk->locked = 0.
-  // This code can't use a C assignment, since it might
-  // not be atomic.
-  // On RISC-V, this turns into an atomic swap:
+  // This code doesn't use a C assignment, since the C standard
+  // implies that an assignment might be implemented with
+  // multiple store instructions.
+  // On RISC-V, sync_lock_release turns into an atomic swap:
   //   s1 = &lk->locked
   //   amoswap.w zero, zero, (s1)
   __sync_lock_release(&lk->locked);
diff --git a/user/usertests.c b/user/usertests.c
index 9d46b1a..5cc5099 100644
--- a/user/usertests.c
+++ b/user/usertests.c
@@ -506,6 +506,44 @@ twochildren(void)
   printf(1, "twochildren ok\n");
 }
 
+// concurrent forks to try to expose locking bugs.
+void
+forkfork(void)
+{
+  int ppid = getpid();
+  
+  printf(1, "forkfork test\n");
+
+  for(int i = 0; i < 2; i++){
+    int pid = fork();
+    if(pid < 0){
+      printf(1, "fork failed");
+      exit();
+    }
+    if(pid == 0){
+      for(int j = 0; j < 200; j++){
+        int pid1 = fork();
+        if(pid1 < 0){
+          printf(1, "fork failed\n");
+          kill(ppid);
+          exit();
+        }
+        if(pid1 == 0){
+          exit();
+        }
+        wait();
+      }
+      exit();
+    }
+  }
+
+  for(int i = 0; i < 2; i++){
+    wait();
+  }
+
+  printf(1, "forkfork ok\n");
+}
+
 void
 forkforkfork(void)
 {
@@ -1858,6 +1896,7 @@ main(int argc, char *argv[])
 
   reparent();
   twochildren();
+  forkfork();
   forkforkfork();
   
   argptest();

From 6bbc2b2245c5b006824eb42ef33d5b296158a693 Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Thu, 11 Jul 2019 10:38:56 -0400
Subject: [PATCH 8/9] cosmetic changes

---
 kernel/trap.c        | 20 ++++++++++++++------
 kernel/virtio_disk.c | 16 ++++++++--------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/kernel/trap.c b/kernel/trap.c
index eb7f6cf..ea5799f 100644
--- a/kernel/trap.c
+++ b/kernel/trap.c
@@ -158,6 +158,15 @@ kerneltrap()
   w_sstatus(sstatus);
 }
 
+void
+clockintr()
+{
+  acquire(&tickslock);
+  ticks++;
+  wakeup(&ticks);
+  release(&tickslock);
+}
+
 // check if it's an external interrupt or software interrupt,
 // and handle it.
 // returns 2 if timer interrupt,
@@ -182,16 +191,15 @@ devintr()
     plic_complete(irq);
     return 1;
   } else if(scause == 0x8000000000000001){
-    // software interrupt from a machine-mode timer interrupt.
+    // software interrupt from a machine-mode timer interrupt,
+    // forwarded by machinevec in kernelvec.S.
 
     if(cpuid() == 0){
-      acquire(&tickslock);
-      ticks++;
-      wakeup(&ticks);
-      release(&tickslock);
+      clockintr();
     }
     
-    // acknowledge.
+    // acknowledge the software interrupt by clearing
+    // the SSIP bit in sip.
     w_sip(r_sip() & ~2);
 
     return 2;
diff --git a/kernel/virtio_disk.c b/kernel/virtio_disk.c
index 8637b56..1a29ce7 100644
--- a/kernel/virtio_disk.c
+++ b/kernel/virtio_disk.c
@@ -20,7 +20,7 @@
 // the address of virtio mmio register r.
 #define R(r) ((volatile uint32 *)(VIRTIO0 + (r)))
 
-struct spinlock virtio_disk_lock;
+struct spinlock vdisk_lock;
 
 // memory for virtio descriptors &c for queue 0.
 // this is a global instead of allocated because it has
@@ -49,7 +49,7 @@ virtio_disk_init(void)
 {
   uint32 status = 0;
 
-  initlock(&virtio_disk_lock, "virtio_disk");
+  initlock(&vdisk_lock, "virtio_disk");
 
   if(*R(VIRTIO_MMIO_MAGIC_VALUE) != 0x74726976 ||
      *R(VIRTIO_MMIO_VERSION) != 1 ||
@@ -168,7 +168,7 @@ virtio_disk_rw(struct buf *b)
 {
   uint64 sector = b->blockno * (BSIZE / 512);
 
-  acquire(&virtio_disk_lock);
+  acquire(&vdisk_lock);
 
   // the spec says that legacy block operations use three
   // descriptors: one for type/reserved/sector, one for
@@ -180,7 +180,7 @@ virtio_disk_rw(struct buf *b)
     if(alloc3_desc(idx) == 0) {
       break;
     }
-    sleep(&free[0], &virtio_disk_lock);
+    sleep(&free[0], &vdisk_lock);
   }
   
   // format the three descriptors.
@@ -234,16 +234,16 @@ virtio_disk_rw(struct buf *b)
 
   // Wait for virtio_disk_intr() to say request has finished.
   while((b->flags & (B_VALID|B_DIRTY)) != B_VALID){
-    sleep(b, &virtio_disk_lock);
+    sleep(b, &vdisk_lock);
   }
 
-  release(&virtio_disk_lock);
+  release(&vdisk_lock);
 }
 
 void
 virtio_disk_intr()
 {
-  acquire(&virtio_disk_lock);
+  acquire(&vdisk_lock);
 
   while((used_idx % NUM) != (used->id % NUM)){
     int id = used->elems[used_idx].id;
@@ -262,5 +262,5 @@ virtio_disk_intr()
     used_idx = (used_idx + 1) % NUM;
   }
 
-  release(&virtio_disk_lock);
+  release(&vdisk_lock);
 }

From ebc39372096280a4a5957d3e3836c859e5d78a79 Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Tue, 16 Jul 2019 17:02:21 -0400
Subject: [PATCH 9/9] conservatively call sfence.vma before every satp load.

---
 kernel/proc.c       |  2 +-
 kernel/riscv.h      | 11 +++++++++++
 kernel/trampoline.S |  4 +++-
 kernel/vm.c         |  1 +
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/kernel/proc.c b/kernel/proc.c
index 0655783..6ba3fec 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -544,7 +544,7 @@ sleep(void *chan, struct spinlock *lk)
 }
 
 //PAGEBREAK!
-// Wake up p, used by exit().
+// Wake up p if it is sleeping in wait(); used by exit().
 // Caller must hold p->lock.
 static void
 wakeup1(struct proc *p)
diff --git a/kernel/riscv.h b/kernel/riscv.h
index e5c0f64..e35f3bc 100644
--- a/kernel/riscv.h
+++ b/kernel/riscv.h
@@ -312,6 +312,17 @@ r_ra()
   return x;
 }
 
+// tell the machine to finish any previous writes to
+// PTEs, so that a subsequent use of a virtual
+// address or load of the SATP will see those writes.
+// perhaps this also flushes the TLB.
+static inline void
+sfence_vma()
+{
+  // the zero, zero means flush all TLB entries.
+  asm volatile("sfence.vma zero, zero");
+}
+
 
 #define PGSIZE 4096 // bytes per page
 #define PGSHIFT 12  // bits of offset within a page
diff --git a/kernel/trampoline.S b/kernel/trampoline.S
index b992ea6..471a29c 100644
--- a/kernel/trampoline.S
+++ b/kernel/trampoline.S
@@ -17,7 +17,8 @@ trampout:
 	# a0: p->tf in user page table
         # a1: new value for satp, for user page table
 
-        # switch to user page table
+	# switch to user page table.
+	sfence.vma zero, zero
         csrw satp, a1
 
         # put the saved user a0 in sscratch, so we
@@ -128,6 +129,7 @@ trampin:
 
         # restore kernel page table from p->tf->kernel_satp
         ld t1, 0(a0)
+	sfence.vma zero, zero
         csrw satp, t1
 
         # a0 is no longer valid, since the kernel page
diff --git a/kernel/vm.c b/kernel/vm.c
index bdb53c2..412ec8c 100644
--- a/kernel/vm.c
+++ b/kernel/vm.c
@@ -61,6 +61,7 @@ kvminit()
 void
 kvminithart()
 {
+  sfence_vma();
   w_satp(MAKE_SATP(kernel_pagetable));
 }