[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [patch uq/master 2/2] Add option to use file backed gue
From: |
Paul Brook |
Subject: |
Re: [Qemu-devel] [patch uq/master 2/2] Add option to use file backed guest memory |
Date: |
Sun, 28 Feb 2010 01:28:16 +0000 |
User-agent: |
KMail/1.12.4 (Linux/2.6.32-trunk-amd64; KDE/4.3.4; x86_64; ; ) |
>+ /*
>+ * 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
>address@hidden -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