[PATCH] kvm tools: Add memory gap for larger RAM sizes

[PATCH] kvm tools: Add memory gap for larger RAM sizes

[Sasha Levin]

e820 is expected to leave a memory gap within the low 32
bits of RAM space. From the documentation of e820_setup_gap():
/*
 * Search for the biggest gap in the low 32 bits of the e820
 * memory space.  We pass this space to PCI to assign MMIO resources
 * for hotplug or unconfigured devices in.
 * Hopefully the BIOS let enough space left.
 */

Not leaving such gap causes errors and hangs during the boot
process.

This patch adds a memory gap between 0xe0000000 and 0x100000000
when using more than 0xe0000000 bytes for guest RAM.

This patch updates the e820 table, slot allocations
used for KVM_SET_USER_MEMORY_REGION.

Changes in V2:
 - Allocate RAM with the gap to avoid altering the translation code.
 - New patch description.

Changes in V3:
 - Remove unnecessary casts.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/bios.c             |   27 ++++++++++++++++++++-------
 tools/kvm/include/kvm/e820.h |    2 +-
 tools/kvm/include/kvm/kvm.h  |    2 ++
 tools/kvm/kvm.c              |   30 +++++++++++++++++++++++-------
 4 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/tools/kvm/bios.c b/tools/kvm/bios.c
index 2199c0c..3cd9b24 100644
--- a/tools/kvm/bios.c
+++ b/tools/kvm/bios.c
@@ -61,8 +61,6 @@ static void e820_setup(struct kvm *kvm)
 	size		= guest_flat_to_host(kvm, E820_MAP_SIZE);
 	mem_map		= guest_flat_to_host(kvm, E820_MAP_START);
 
-	*size		= E820_MEM_AREAS;
-
 	mem_map[i++]	= (struct e820_entry) {
 		.addr		= REAL_MODE_IVT_BEGIN,
 		.size		= EBDA_START - REAL_MODE_IVT_BEGIN,
@@ -78,13 +76,28 @@ static void e820_setup(struct kvm *kvm)
 		.size		= MB_BIOS_END - MB_BIOS_BEGIN,
 		.type		= E820_MEM_RESERVED,
 	};
-	mem_map[i++]	= (struct e820_entry) {
-		.addr		= BZ_KERNEL_START,
-		.size		= kvm->ram_size - BZ_KERNEL_START,
-		.type		= E820_MEM_USABLE,
-	};
+	if (kvm->ram_size < KVM_32BIT_GAP_START) {
+		mem_map[i++]	= (struct e820_entry) {
+			.addr		= BZ_KERNEL_START,
+			.size		= kvm->ram_size - BZ_KERNEL_START,
+			.type		= E820_MEM_USABLE,
+		};
+	} else {
+		mem_map[i++]	= (struct e820_entry) {
+			.addr		= BZ_KERNEL_START,
+			.size		= KVM_32BIT_GAP_START - BZ_KERNEL_START,
+			.type		= E820_MEM_USABLE,
+		};
+		mem_map[i++]	= (struct e820_entry) {
+			.addr		= 0x100000000ULL,
+			.size		= kvm->ram_size - KVM_32BIT_GAP_START,
+			.type		= E820_MEM_USABLE,
+		};
+	}
 
 	BUILD_BUG_ON(i > E820_MEM_AREAS);
+
+	*size			= i;
 }
 
 /**
diff --git a/tools/kvm/include/kvm/e820.h b/tools/kvm/include/kvm/e820.h
index 252ae1f..e0f5f2a 100644
--- a/tools/kvm/include/kvm/e820.h
+++ b/tools/kvm/include/kvm/e820.h
@@ -8,7 +8,7 @@
 #define E820_MEM_USABLE		1
 #define E820_MEM_RESERVED	2
 
-#define E820_MEM_AREAS		4
+#define E820_MEM_AREAS		5
 
 struct e820_entry {
 	u64	addr;	/* start of memory segment */
diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
index 3dab78d..5e2e64c 100644
--- a/tools/kvm/include/kvm/kvm.h
+++ b/tools/kvm/include/kvm/kvm.h
@@ -8,6 +8,8 @@
 #include <time.h>
 
 #define KVM_NR_CPUS		(255)
+#define KVM_32BIT_GAP_SIZE	(512 << 20)
+#define KVM_32BIT_GAP_START	((1ULL << 32) - KVM_32BIT_GAP_SIZE)
 
 struct kvm {
 	int			sys_fd;		/* For system ioctls(), i.e. /dev/kvm */
diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
index 65793f2..16d91d5 100644
--- a/tools/kvm/kvm.c
+++ b/tools/kvm/kvm.c
@@ -153,23 +153,33 @@ static bool kvm__cpu_supports_vm(void)
 	return regs.ecx & (1 << feature);
 }
 
-void kvm__init_ram(struct kvm *self)
+static void kvm_register_mem_slot(struct kvm *kvm, u32 slot, u64 guest_phys, u64 size, void *userspace_addr)
 {
 	struct kvm_userspace_memory_region mem;
 	int ret;
 
 	mem = (struct kvm_userspace_memory_region) {
-		.slot			= 0,
-		.guest_phys_addr	= 0x0UL,
-		.memory_size		= self->ram_size,
-		.userspace_addr		= (unsigned long) self->ram_start,
+		.slot			= slot,
+		.guest_phys_addr	= guest_phys,
+		.memory_size		= size,
+		.userspace_addr		= (u64)userspace_addr,
 	};
 
-	ret = ioctl(self->vm_fd, KVM_SET_USER_MEMORY_REGION, &mem);
+	ret = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &mem);
 	if (ret < 0)
 		die_perror("KVM_SET_USER_MEMORY_REGION ioctl");
 }
 
+void kvm__init_ram(struct kvm *self)
+{
+	if (self->ram_size < KVM_32BIT_GAP_START) {
+		kvm_register_mem_slot(self, 0, 0, self->ram_size, self->ram_start);
+	} else {
+		kvm_register_mem_slot(self, 0, 0, KVM_32BIT_GAP_START, self->ram_start);
+		kvm_register_mem_slot(self, 1, 0x100000000ULL, self->ram_size - KVM_32BIT_GAP_START, self->ram_start + 0x100000000ULL);
+	}
+}
+
 int kvm__max_cpus(struct kvm *self)
 {
 	int ret;
@@ -225,7 +235,13 @@ struct kvm *kvm__init(const char *kvm_dev, unsigned long ram_size)
 
 	self->ram_size		= ram_size;
 
-	self->ram_start = mmap(NULL, ram_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
+	if (self->ram_size < KVM_32BIT_GAP_START) {
+		self->ram_start = mmap(NULL, ram_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
+	} else {
+		self->ram_start = mmap(NULL, ram_size + KVM_32BIT_GAP_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
+		if (self->ram_start != MAP_FAILED)
+			mprotect(self->ram_start + KVM_32BIT_GAP_START, KVM_32BIT_GAP_SIZE, PROT_NONE);
+	}
 	if (self->ram_start == MAP_FAILED)
 		die("out of memory");
 
-- 
1.7.5.rc3

Re: [PATCH] kvm tools: Add memory gap for larger RAM sizes

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

>  }
>  
> +void kvm__init_ram(struct kvm *self)
> +{

Why is there no comment explaining what this function does and what the whole 
gap logic is about? The bug this problem caused was non-obvious, so any future 
developer reading this code will wonder what this is all about.

> +	if (self->ram_size < KVM_32BIT_GAP_START) {
> +		kvm_register_mem_slot(self, 0, 0, self->ram_size, self->ram_start);
> +	} else {
> +		kvm_register_mem_slot(self, 0, 0, KVM_32BIT_GAP_START, self->ram_start);
> +		kvm_register_mem_slot(self, 1, 0x100000000ULL, self->ram_size - KVM_32BIT_GAP_START, self->ram_start + 0x100000000ULL);
> +	}
> +}

Why not write it in a much more obvious and almost self-documenting way:

		/* First RAM range from zero to the PCI gap: */

		phys_start = 0;
		phys_size  = KVM_32BIT_GAP_START;
		host_mem   = self->ram_start;

		kvm_register_mem_slot(self, 0, phys_start, phys_size, host_mem);

		/* Second RAM range from 4GB to the end of RAM: */

		phys_start = 0x100000000ULL;
		phys_size  = self->ram_size - phys_size;
		host_mem   = self->ram_start + phys_start;

		kvm_register_mem_slot(self, 1, phys_start, phys_size, host_mem);

?

Btw., could we please also stop using 'self' for function parameters? It's 
utterly meaningless as a name and makes grepping pretty hard.

Use a consistent and meaningful convention please, such as:

	struct kvm_cpu *vcpu

And obviously CPU related methods will always have a vcpu parameter around.

Thanks,

	Ingo

Re: [PATCH] kvm tools: Add memory gap for larger RAM sizes

[Pekka Enberg]

On 5/11/11 2:13 PM, Ingo Molnar wrote:
> Btw., could we please also stop using 'self' for function parameters? It's
> utterly meaningless as a name and makes grepping pretty hard.

I blame perf! But sure we can do that.

                     Pekka

Re: [PATCH] kvm tools: Add memory gap for larger RAM sizes

[Ingo Molnar]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On 5/11/11 2:13 PM, Ingo Molnar wrote:
> >Btw., could we please also stop using 'self' for function parameters? It's
> >utterly meaningless as a name and makes grepping pretty hard.
> 
> I blame perf! But sure we can do that.

The blame is well put - we eliminated almost all instances of 'self' from perf, 
but somewhat sadly not all of them.

This was a brief infection perf caught - but it's highly contagious it appears!

Thanks,

	Ingo