From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60430) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbYy7-0004rD-1k for qemu-devel@nongnu.org; Wed, 30 Oct 2013 12:50:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VbYy1-0004Jq-0E for qemu-devel@nongnu.org; Wed, 30 Oct 2013 12:50:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55305) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbYy0-0004IF-No for qemu-devel@nongnu.org; Wed, 30 Oct 2013 12:50:00 -0400 Date: Wed, 30 Oct 2013 17:49:49 +0100 From: Igor Mammedov Message-ID: <20131030174949.2fb0d2c2@nial.usersys.redhat.com> In-Reply-To: <20131029213844.GB32615@amt.cnet> References: <20131028140406.GA18025@amt.cnet> <1383070729-19427-1-git-send-email-imammedo@redhat.com> <20131029213844.GB32615@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH] pc: align gpa<->hpa on 1GB boundary by splitting RAM on several regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcelo Tosatti Cc: aarcange@redhat.com, peter.maydell@linaro.org, gleb@redhat.com, quintela@redhat.com, jan.kiszka@siemens.com, qemu-devel@nongnu.org, aliguori@amazon.com, pbonzini@redhat.com, afaerber@suse.de, rth@twiddle.net On Tue, 29 Oct 2013 19:38:44 -0200 Marcelo Tosatti wrote: > On Tue, Oct 29, 2013 at 07:18:49PM +0100, Igor Mammedov wrote: > > Otherwise 1GB TLBs cannot be cached for the range. > > This fails to back non-1GB-aligned gpas, but 2MB aligned, with 2MB large > pages. With current command line only one hugetlbfs mount point is possible, so it will back with whatever alignment specified hugetlbfs mount point has. Anything that doesn't fit into page aligned region goes to tail using non hugepage baked phys_mem_set_alloc()=qemu_anon_ram_alloc() allocator. > > Since hugetlbfs allocation is static, it requires the user to inform > different 1GB and 2MB sized hugetlbfs mount points (with proper number > of corresponding hugetlbfs pages allocated). This is incompatible with > the current command line, and i'd like to see this problem handled in a > way that is command line backwards compatible. patch doesn't change that, it uses provided hugetlbfs and fallbacks (hunk 2) to phys_mem_alloc if requested memory region is not hugepage size aligned. So there is no any CLI change, only fixing memory leak. > Also, if the argument for one-to-one mapping between dimms and linear host > virtual address sections holds, it means virtual DIMMs must be > partitioned into whatever hugepage alignment is necessary (and in > that case, why they can't be partitioned similarly with the memory > region aliases?). Because during hotplug a new memory region of desired size is allocated and it could be mapped directly without any aliasing. And if some day we convert adhoc initial memory allocation to dimm devices there is no reason to alloc one huge block and then invent means how to alias hole somewhere else, we could just reuse memdev/dimm and allocate several memory regions with desired properties each represented by a memdev/dimm pair. one-one mapping simplifies design and interface with ACPI part during memory hotplug. for hotplug case flow could look like: memdev_add id=x1,size=1Gb,mem-path=/hugetlbfs/1gb,other-host-related-stuff-options #memdev could enforce size to be backend aligned device_add dimm,id=y1,backend=x1,addr=xxxxxx #dimm could get alignment from associated memdev or fail if addr #doesn't meet alignment of memdev backend memdev_add id=x2,size=2mb,mem-path=/hugetlbfs/2mb device_add dimm,id=y2,backend=x2,addr=yyyyyyy memdev_add id=x3,size=1mb device_add dimm,id=y3,backend=x3,addr=xxxxxxx linear memory block is allocated at runtime (user has to make sure that enough hugepages are available) by each memdev_add command and that RAM memory region is mapped into GPA by virtual DIMM as is, there wouldn't be any need for aliasing. Now back to intial memory and bright future we are looking forward to (i.e. ability to create machine from configuration file without adhoc codding like(pc_memory_init)): legacy cmdline "-m 4512 -mem-path /hugetlbfs/1gb" could be automatically translated into: -memdev id=x1,size=3g,mem-path=/hugetlbfs/1gb -device dimm,backend=x1,addr=0 -memdev id=x2,size=1g,mem-path=/hugetlbfs/1gb -device dimm,backend=x2,addr=4g -memdev id=x3,size=512m -device dimm,backend=x3,addr=5g or user could drop legacy CLI and assume fine grained control over memory configuration: -memdev id=x1,size=3g,mem-path=/hugetlbfs/1gb -device dimm,backend=x1,addr=0 -memdev id=x2,size=1g,mem-path=/hugetlbfs/1gb -device dimm,backend=x2,addr=4g -memdev id=x3,size=512m,mem-path=/hugetlbfs/2mb -device dimm,backend=x3,addr=5g So if we are going to break migration compatibility for new machine type lets do a way that could painlessly changed to memdev/device in future. > > PS: > > as side effect we are not wasting ~1Gb of memory if > > 1Gb hugepages are used and -m "hpagesize(in Mb)*n + 1" > > This is how hugetlbfs works. You waste 1GB hugepage if an extra > byte is requested. it looks more a bug than feature, why do it if leak could be avoided as shown below? > > > Signed-off-by: Igor Mammedov > > --- > > PS2: > > As RFC it's yet without compatibility changes noted by Paolo > > > > --- > > exec.c | 8 ++++++- > > hw/i386/pc.c | 50 ++++++++++++++++++++++++++++---------------- > > include/exec/cpu-common.h | 1 + > > 3 files changed, 40 insertions(+), 19 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 9b6ea50..a4e5c80 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -882,7 +882,7 @@ void qemu_mutex_unlock_ramlist(void) > > > > #define HUGETLBFS_MAGIC 0x958458f6 > > > > -static long gethugepagesize(const char *path) > > +long gethugepagesize(const char *path) > > { > > struct statfs fs; > > int ret; > > @@ -925,6 +925,12 @@ static void *file_ram_alloc(RAMBlock *block, > > return NULL; > > } > > > > + /* refuse to use huge pages if requested size isn't page aligned > > + * to avoid wasting memory */ > > + if (memory != (memory & ~(hpagesize-1))) { > > + return NULL; > > + } > > + > > if (kvm_enabled() && !kvm_has_sync_mmu()) { > > fprintf(stderr, "host lacks kvm mmu notifiers, -mem-path unsupported\n"); > > return NULL; > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 0c313fe..1611fa7 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1116,32 +1116,46 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > > { > > int linux_boot, i; > > MemoryRegion *ram, *option_rom_mr; > > - MemoryRegion *ram_below_4g, *ram_above_4g; > > FWCfgState *fw_cfg; > > + unsigned long hpagesize = gethugepagesize(mem_path); > > + ram_addr_t below_4g_mem_size_alined, below_4g_mem_size_tail, above_4g_mem_size_alined, above_4g_mem_size_tail; > > > > linux_boot = (kernel_filename != NULL); > > > > - /* Allocate RAM. We allocate it as a single memory region and use > > - * aliases to address portions of it, mostly for backwards compatibility > > - * with older qemus that used qemu_ram_alloc(). > > - */ > > + *ram_memory = g_malloc(sizeof(**ram_memory)); > > + memory_region_init(*ram_memory, NULL, "pc.ram", > > + above_4g_mem_size == 0 ? below_4g_mem_size: 0x100000000ULL + above_4g_mem_size); > > + memory_region_add_subregion(system_memory, 0, *ram_memory); > > + > > + below_4g_mem_size_alined = below_4g_mem_size & ~(hpagesize - 1); > > ram = g_malloc(sizeof(*ram)); > > - memory_region_init_ram(ram, NULL, "pc.ram", > > - below_4g_mem_size + above_4g_mem_size); > > + memory_region_init_ram(ram, NULL, "pc.ram.low.aligned", below_4g_mem_size_alined); > > + memory_region_add_subregion(*ram_memory, 0, ram); > > vmstate_register_ram_global(ram); > > - *ram_memory = ram; > > - ram_below_4g = g_malloc(sizeof(*ram_below_4g)); > > - memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", ram, > > - 0, below_4g_mem_size); > > - memory_region_add_subregion(system_memory, 0, ram_below_4g); > > - if (above_4g_mem_size > 0) { > > - ram_above_4g = g_malloc(sizeof(*ram_above_4g)); > > - memory_region_init_alias(ram_above_4g, NULL, "ram-above-4g", ram, > > - below_4g_mem_size, above_4g_mem_size); > > - memory_region_add_subregion(system_memory, 0x100000000ULL, > > - ram_above_4g); > > + > > + below_4g_mem_size_tail = below_4g_mem_size - below_4g_mem_size_alined; > > + if (below_4g_mem_size_tail) { > > + ram = g_malloc(sizeof(*ram)); > > + memory_region_init_ram(ram, NULL, "pc.ram.low.unaligned", below_4g_mem_size_tail); > > + memory_region_add_subregion(*ram_memory, below_4g_mem_size_alined, ram); > > + vmstate_register_ram_global(ram); > > } > > > > + if (above_4g_mem_size > 0) { > > + above_4g_mem_size_alined = above_4g_mem_size & ~(hpagesize - 1); > > + ram = g_malloc(sizeof(*ram)); > > + memory_region_init_ram(ram, NULL, "pc.ram.high.aligned", above_4g_mem_size_alined); > > + memory_region_add_subregion(*ram_memory, 0x100000000ULL, ram); > > + vmstate_register_ram_global(ram); > > + > > + above_4g_mem_size_tail = above_4g_mem_size - above_4g_mem_size_alined; > > + if (above_4g_mem_size_tail) { > > + ram = g_malloc(sizeof(*ram)); > > + memory_region_init_ram(ram, NULL, "pc.ram.high.unaligned", above_4g_mem_size_tail); > > + memory_region_add_subregion(*ram_memory, 0x100000000ULL + above_4g_mem_size_alined, ram); > > + vmstate_register_ram_global(ram); > > + } > > + } > > > > /* Initialize PC system firmware */ > > pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw); > > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h > > index 40e15e4..f89a37c 100644 > > --- a/include/exec/cpu-common.h > > +++ b/include/exec/cpu-common.h > > @@ -57,6 +57,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev); > > #ifdef __linux__ > > uint32_t qemu_get_ram_hpagesize(ram_addr_t addr); > > #endif > > +long gethugepagesize(const char *path); > > > > void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf, > > int len, int is_write); > > -- > > 1.7.1