From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NlXxG-0001A4-2a for qemu-devel@nongnu.org; Sat, 27 Feb 2010 20:28:22 -0500 Received: from [199.232.76.173] (port=56092 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NlXxE-00019w-NM for qemu-devel@nongnu.org; Sat, 27 Feb 2010 20:28:20 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NlXxD-0006pn-V6 for qemu-devel@nongnu.org; Sat, 27 Feb 2010 20:28:20 -0500 Received: from mx20.gnu.org ([199.232.41.8]:56630) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NlXxD-0006pj-No for qemu-devel@nongnu.org; Sat, 27 Feb 2010 20:28:19 -0500 Received: from mail.codesourcery.com ([38.113.113.100]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NlXxD-0004Mp-0C for qemu-devel@nongnu.org; Sat, 27 Feb 2010 20:28:19 -0500 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 References: <20100224211117.958583246@amt.cnet> <20100224211507.913712224@amt.cnet> In-Reply-To: <20100224211507.913712224@amt.cnet> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201002280128.16649.paul@codesourcery.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: john cooper , Marcelo Tosatti , avi@redhat.com, kvm@vger.kernel.org >+ /* >+ * 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