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

Paul Brook
2010-02-28T01:34:59Z

>+ /*
>+ * 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