Re: [Qemu-devel] [patch uq/master 2/2] Add option to use file backed guest memory

[Paul Brook <paul@codesourcery.com>]

>+    /*
>+     * ftruncate is not supported by hugetlbfs in older
>+     * hosts, so don't bother checking for errors.
>+     * If anything goes wrong with it under other filesystems,
>+     * mmap will fail.
>+     */
>+    if (ftruncate(fd, memory))
>+       perror("ftruncate");

Code does not match comment.

>+    if (asprintf(&filename, "%s/kvm.XXXXXX", path) == -1) {
>+       return NULL;
>+    }

This isn't kvm any more :-)

>+    flags = mem_prealloc ? MAP_POPULATE|MAP_SHARED : MAP_PRIVATE;

Missing spaces round logic operator (plus several other occurrences).

>+static void *file_ram_alloc(ram_addr_t memory, const char *path)
>+{
>+    return NULL;
>+}

Silently ignoring commandline options is bad. 
Especially as the other option you added (-mem-prealloc) causes an error if 
not supported.

>+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
>+        fprintf(stderr, "kvm: host lacks mmu notifiers, disabling
> -mem-path\n"); +        return NULL;
>+    }

Code does not match error message.  Users are liable to see this many times.

>+    new_block->host = file_ram_alloc(size, mem_path);

IMHO it would be better to check the mem_path != NULL here, rather that 
burying the check in file_ram_alloc.

>+    if (memory < hpagesize) {
>+        return NULL;
>+    }

Ah, so it's actually "allocate memory in $path, if you feel like it". Good job 
we aren't relying on this for correctness.  At minimum I recommend documenting 
this heuristic.

>+    if (!new_block->host) {
> #if defined(TARGET_S390X) && defined(CONFIG_KVM)
>-    /* XXX S390 KVM requires the topmost vma of the RAM to be < 256GB */

By my reading this implies -mempath is probably broken on s390 KVM?

>+DEF("mem-path", HAS_ARG, QEMU_OPTION_mempath,
>+    "-mem-path FILE  provide backing storage for guest RAM\n")
>+STEXI
>+@item -mem-path @var{path}
>+Allocate guest RAM from a temporarily created file in @var{path}.
>+ETEXI

You should mention that this is only useful when PATH happens to be a linux 
hugetlbfs mount.

>+#ifdef MAP_POPULATE
>+            case QEMU_OPTION_mem_prealloc:
>+                mem_prealloc = !mem_prealloc;
>+#endif

This looks highly suspect.  Having redundant options toggle the sate seems 
like a particularly bad UI.

Paul