clean up virtio code

kernel/memlayout.h

@@ -23,8 +23,8 @@
 #define UART0_IRQ 10
 
 // virtio mmio interface
-#define VIRTIO 0x10001000
+#define VIRTIO0 0x10001000
-#define VIRTIO_IRQ 1 // really the first of 8 units
+#define VIRTIO0_IRQ 1
 
 // local interrupt controller, which contains the timer.
 #define CLINT 0x2000000L

kernel/plic.c

@@ -13,7 +13,7 @@ plicinit(void)
 {
   // set desired IRQ priorities non-zero (otherwise disabled).
   *(uint32*)(PLIC + UART0_IRQ*4) = 1;
-  *(uint32*)(PLIC + VIRTIO_IRQ*4) = 1;
+  *(uint32*)(PLIC + VIRTIO0_IRQ*4) = 1;
 }
 
 void
@@ -22,7 +22,7 @@ plicinithart(void)
   int hart = cpuid();
   
   // set uart's enable bit for this hart's S-mode. 
-  *(uint32*)PLIC_SENABLE(hart)= (1 << UART0_IRQ) | (1 << VIRTIO_IRQ);
+  *(uint32*)PLIC_SENABLE(hart)= (1 << UART0_IRQ) | (1 << VIRTIO0_IRQ);
 
   // set this hart's S-mode priority threshold to 0.
   *(uint32*)PLIC_SPRIORITY(hart) = 0;

kernel/trap.c

@@ -159,7 +159,7 @@ devintr()
 
     if(irq == UART0_IRQ){
       uartintr();
-    } else if(irq == VIRTIO_IRQ){
+    } else if(irq == VIRTIO0_IRQ){
       virtio_disk_intr();
     }
 

kernel/virtio.h

@@ -4,12 +4,15 @@
 // only tested with qemu.
 // this is the "legacy" virtio interface.
 //
+// the virtio spec:
+// https://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.pdf
+//
 
 // virtio mmio control registers, mapped starting at 0x10001000.
 // from qemu virtio_mmio.h
 #define VIRTIO_MMIO_MAGIC_VALUE		0x000 // 0x74726976
-#define VIRTIO_MMIO_VERSION		0x004 // 1 -- version, 1 is legacy
+#define VIRTIO_MMIO_VERSION		0x004 // version; 1 is legacy
-#define VIRTIO_MMIO_DEVICE_ID		0x008 // 2 -- block device type
+#define VIRTIO_MMIO_DEVICE_ID		0x008 // device type; 1 is net, 2 is disk
 #define VIRTIO_MMIO_VENDOR_ID		0x00c // 0x554d4551
 #define VIRTIO_MMIO_DEVICE_FEATURES	0x010
 #define VIRTIO_MMIO_DRIVER_FEATURES	0x020

kernel/virtio_disk.c

@@ -17,8 +17,8 @@
 #include "buf.h"
 #include "virtio.h"
 
-// the address of a virtio mmio register.
+// the address of virtio mmio register r.
-#define R(off) ((volatile uint32 *)(VIRTIO + (off)))
+#define R(r) ((volatile uint32 *)(VIRTIO0 + (r)))
 
 struct spinlock virtio_disk_lock;
 
@@ -45,6 +45,7 @@ static uint16 used_idx; // we've looked this far in used[2..NUM].
 // indexed by first descriptor index of chain.
 static struct {
   struct buf *b;
+  char status;
 } info[NUM];
 
 void
@@ -54,8 +55,6 @@ virtio_disk_init(void)
 
   initlock(&virtio_disk_lock, "virtio_disk");
 
-  // qemu's virtio-mmio.c
-
   if(*R(VIRTIO_MMIO_MAGIC_VALUE) != 0x74726976 ||
      *R(VIRTIO_MMIO_VERSION) != 1 ||
      *R(VIRTIO_MMIO_DEVICE_ID) != 2 ||
@@ -90,9 +89,7 @@ virtio_disk_init(void)
 
   *R(VIRTIO_MMIO_GUEST_PAGE_SIZE) = PGSIZE;
 
-  // qemu's hw/virtio/virtio.c
+  // initialize queue 0.
-
-  // initialize queue 0
   *R(VIRTIO_MMIO_QUEUE_SEL) = 0;
   uint32 max = *R(VIRTIO_MMIO_QUEUE_NUM_MAX);
   if(max == 0)
@@ -113,6 +110,8 @@ virtio_disk_init(void)
 
   for(int i = 0; i < NUM; i++)
     free[i] = 1;
+
+  // plic.c and trap.c arrange for interrupts from VIRTIO0_IRQ.
 }
 
 // find a free descriptor, mark it non-free, return its index.
@@ -128,14 +127,30 @@ alloc_desc()
   return -1;
 }
 
-void
+// mark a descriptor as free.
+static void
 free_desc(int i)
 {
   if(i >= NUM)
     panic("virtio_disk_intr 1");
   if(free[i])
     panic("virtio_disk_intr 2");
+  desc[i].addr = 0;
   free[i] = 1;
+  wakeup(&free[0]);
+}
+
+// free a chain of descriptors.
+static void
+free_chain(int i)
+{
+  while(1){
+    free_desc(i);
+    if(desc[i].flags & VRING_DESC_F_NEXT)
+      i = desc[i].next;
+    else
+      break;
+  }
 }
 
 void
@@ -145,7 +160,7 @@ virtio_disk_rw(struct buf *b)
 
   acquire(&virtio_disk_lock);
 
-  // the spec says that legacy block operations always use three
+  // the spec says that legacy block operations use three
   // descriptors: one for type/reserved/sector, one for
   // the data, one for a 1-byte status result.
 
@@ -158,7 +173,6 @@ virtio_disk_rw(struct buf *b)
       if(idx[i] < 0){
         for(int j = 0; j < i; j++)
           free_desc(idx[j]);
-        wakeup(&free[0]);
         done = 0;
         break;
       }
@@ -198,8 +212,8 @@ virtio_disk_rw(struct buf *b)
   desc[idx[1]].flags |= VRING_DESC_F_NEXT;
   desc[idx[1]].next = idx[2];
 
-  char status = 0;
+  info[idx[0]].status = 0;
-  desc[idx[2]].addr = (uint64) &status;
+  desc[idx[2]].addr = (uint64) &info[idx[0]].status;
   desc[idx[2]].len = 1;
   desc[idx[2]].flags = VRING_DESC_F_WRITE; // device writes the status
   desc[idx[2]].next = 0;
@@ -232,16 +246,16 @@ virtio_disk_intr()
   // uint16 flags
   // uint16 idx
   // array of VRingUsedElem
-
+  volatile uint16 *idxp = (uint16 *)(used + 2);
-  // XXX spec says to read INTERRUPT_STATUS and
+  volatile struct VRingUsedElem *e0 = (struct VRingUsedElem *)(used + 4);
-  // write INTERRUPT_ACK
 
   acquire(&virtio_disk_lock);
-  
-  while((used_idx % NUM) != (*(volatile uint16 *)(used+2) % NUM)){
-    struct VRingUsedElem *ue = (struct VRingUsedElem *) (used + 4 + 8*used_idx);
 
-    // XXX check the one-byte status in the 3rd descriptor.
+  while((used_idx % NUM) != (*idxp % NUM)){
+    volatile struct VRingUsedElem *ue = &e0[used_idx];
+
+    if(info[ue->id].status != 0)
+      panic("virtio_disk_intr status");
 
     info[ue->id].b->flags |= B_VALID;
     info[ue->id].b->flags &= ~B_DIRTY;
@@ -249,17 +263,7 @@ virtio_disk_intr()
     wakeup(info[ue->id].b);
 
     info[ue->id].b = 0;
-
+    free_chain(ue->id);
-    uint i = ue->id;
-    while(1){
-      desc[i].addr = 0;
-      free_desc(i);
-      if(desc[i].flags & VRING_DESC_F_NEXT)
-        i = desc[i].next;
-      else
-        break;
-    }
-    wakeup(&free[0]);
 
     used_idx = (used_idx + 1) % NUM;
   }

kernel/vm.c

@@ -31,8 +31,8 @@ kvminit()
            UART0, PTE_R | PTE_W);
 
   // virtio mmio disk interface
-  mappages(kernel_pagetable, VIRTIO, PGSIZE,
+  mappages(kernel_pagetable, VIRTIO0, PGSIZE,
-           VIRTIO, PTE_R | PTE_W);
+           VIRTIO0, PTE_R | PTE_W);
 
   // CLINT
   mappages(kernel_pagetable, CLINT, 0x10000,