Part-way to writing memory
authorRonald G. Minnich <rminnich@google.com>
Fri, 10 Jan 2014 22:45:38 +0000 (14:45 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Fri, 17 Jan 2014 22:57:12 +0000 (14:57 -0800)
We're getting a NULL pointer in vm_set_memory_region,
but part of it is working.

I have moved some of the infrastructure over to error()/waserror(), and
I'm now remembering why I like getting useful error messages
instead of the conventional UNIX bring-me-another-rock
behavior of EINVAL.

Signed-off-by: Ronald G. Minnich <rminnich@google.com>
kern/arch/x86/vmx.c
kern/drivers/dev/vm.c

index 70e9302..c7814a6 100644 (file)
@@ -1392,6 +1392,7 @@ out:
 int vm_set_memory_region(struct litevm *litevm,
                                           struct litevm_memory_region *mem)
 {
+       ERRSTACK(2);
        int r;
        gfn_t base_gfn;
        unsigned long npages;
@@ -1404,13 +1405,14 @@ int vm_set_memory_region(struct litevm *litevm,
        r = -EINVAL;
        /* General sanity checks */
        if (mem->memory_size & (PAGE_SIZE - 1))
-               goto out;
+               error("mem->memory_size %lld is not page-aligned", mem->memory_size);
        if (mem->guest_phys_addr & (PAGE_SIZE - 1))
-               goto out;
+               error("guest_phys_addr 0x%llx is not page-aligned", mem->guest_phys_addr);
        if (mem->slot >= LITEVM_MEMORY_SLOTS)
-               goto out;
+               error("Slot %d is >= %d", mem->slot, LITEVM_MEMORY_SLOTS);
        if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
-               goto out;
+               error("0x%x + 0x%x is < 0x%x", 
+                     mem->guest_phys_addr, mem->memory_size, mem->guest_phys_addr);
 
        memslot = &litevm->memslots[mem->slot];
        base_gfn = mem->guest_phys_addr >> PAGE_SHIFT;
@@ -1419,9 +1421,19 @@ int vm_set_memory_region(struct litevm *litevm,
        if (!npages)
                mem->flags &= ~LITEVM_MEM_LOG_DIRTY_PAGES;
 
+       /* this is actually a very tricky for loop. The use of
+        * error is a bit dangerous, so we don't use it much.
+        * consider a rewrite. Would be nice if akaros could do the
+        * allocation of a bunch of pages for us.
+        */
 raced:
        spin_lock(&litevm->lock);
 
+       if (waserror()){
+               spin_unlock(&litevm->lock);
+               nexterror();
+       }
+               
        memory_config_version = litevm->memory_config_version;
        new = old = *memslot;
 
@@ -1432,7 +1444,8 @@ raced:
        /* Disallow changing a memory slot's size. */
        r = -EINVAL;
        if (npages && old.npages && npages != old.npages)
-               goto out_unlock;
+               error("npages is %d, old.npages is %d, can't change",
+                     npages, old.npages);
 
        /* Check for overlaps */
        r = -EEXIST;
@@ -1443,13 +1456,14 @@ raced:
                        continue;
                if (!((base_gfn + npages <= s->base_gfn) ||
                      (base_gfn >= s->base_gfn + s->npages)))
-                       goto out_unlock;
+                       error("Overlap");
        }
        /*
         * Do memory allocations outside lock.  memory_config_version will
         * detect any races.
         */
        spin_unlock(&litevm->lock);
+       poperror();
 
        /* Deallocate if slot is being removed */
        if (!npages)
@@ -1525,9 +1539,13 @@ raced:
 
 out_unlock:
        spin_unlock(&litevm->lock);
+       printk("out_unlock\n");
+       error("well, something went badly.");
 out_free:
+       printk("out_free\n");
        litevm_free_physmem_slot(&new, &old);
 out:
+       printk("vm_set_memory_region: return %d\n", r);
        return r;
 }
 
index bbfdcf8..cc3deb8 100644 (file)
@@ -78,6 +78,7 @@ readn(struct chan *c, void *vp, long n)
        p = vp;
        while(n > 0) {
                nn = devtab[c->type]->read(c, p, n, c->offset);
+               printk("readn: Got %d@%lld\n", nn, c->offset);
                if(nn == 0)
                        error("%s: wanted %d, got %d", Eshort, total, want);
                c->offset += nn;
@@ -266,15 +267,6 @@ static struct chan *vmopen(struct chan *c, int omode)
        case Qimage:
                /* the purpose of opening is to hold a kref on the proc_vm */
                v = c->aux;
-               /* this isn't a valid pointer yet, since our chan doesn't have a
-                * ref.  since the time that walk gave our chan the qid, the chan
-                * could have been closed, and the vm decref'd and freed.  the
-                * qid is essentially an uncounted reference, and we need to go to
-                * the source to attempt to get a real ref.  Unfortunately, this is
-                * another scan of the list, same as devsrv.  We could speed it up
-                * by storing an "on_list" bool in the vm_is. */
-               //if (!vm_i)
-               //      error("Unable to open vm, concurrent closing");
                break;
        }
        c->mode = openmode(omode);
@@ -350,7 +342,7 @@ static long vmwrite(struct chan *c, void *ubuf, long n, int64_t unused)
        struct cmdbuf *cb;
        struct litevm *vm;
        uint64_t hexval;
-
+       printd("vmwrite(%p, %p, %d)\n", c, ubuf, n);
        switch (TYPE(c->qid)) {
        case Qtopdir:
        case Qvmdir:
@@ -381,12 +373,13 @@ static long vmwrite(struct chan *c, void *ubuf, long n, int64_t unused)
                        vmr.flags = strtoul(cb->f[3], NULL, 0);
                        vmr.guest_phys_addr = strtoul(cb->f[4], NULL, 0);
                        filesize = strtoul(cb->f[5], NULL, 0);
-                       vmr.memory_size = (filesize + 4095) & ~4096ULL;
+                       vmr.memory_size = (filesize + 4095) & ~4095ULL;
+
                        file = namec(cb->f[1], Aopen, OREAD, 0);
-                       if (! file)
-                               error("%s: can't open", cb->f[1], NULL, 0);
+                       printk("after namec file is %p\n", file);
                        if (waserror()){
                                cclose(file);
+                               nexterror();
                        }
                        /* at some point we want to mmap from the kernel
                         * but we don't have that yet. This all needs
@@ -399,11 +392,14 @@ static long vmwrite(struct chan *c, void *ubuf, long n, int64_t unused)
                        }
 
                        readn(file, v, filesize);
-                       cclose(file);
                        vmr.init_data = v;
 
                        if (vm_set_memory_region(vm, &vmr))
                                error("vm_set_memory_region failed");
+                       poperror();
+                       poperror();
+                       kfree(v);
+                       cclose(file);
 
                } else if (!strcmp(cb->f[0], "region")) {
                        void *v;