[kvm-unit-tests PATCH v2 0/6] x86: enable malloc and friends

[kvm-unit-tests PATCH v2 0/6] x86: enable malloc and friends

[Andrew Jones]

And do some other alloc related things too...

v2:
- introduced alloc_zeroed_page [Paolo]
- kept spinlocks in phys_alloc_init [Thomas]
- added more improvements to alloc/free_page, e.g locks [drew]
- force phys_alloc alignment to be set once [drew]
- new patch that renames alloc.c's state [drew]
- fix vmalloc-based memalign v1 bug. Didn't consider size offset [drew]
- added huge commit message for "lib/x86/vm: enable malloc and friends"
  to try to justify it [drew]

Didn't take Thomas' r-b for "move heap management to lib", because it
changed too much.


Andrew Jones (6):
  lib/x86/vm: collection of improvements
  lib/alloc: improve init
  lib/alloc: prepare to extend alloc.c's purpose
  x86: lib/alloc: move heap management to lib
  x86: lib/alloc: introduce alloc_zeroed_page
  lib/x86/vm: enable malloc and friends

 lib/alloc.c         | 160 +++++++++++++++++++++++++++++++++++++---------------
 lib/alloc.h         |  18 +++++-
 lib/arm/setup.c     |   2 +-
 lib/powerpc/setup.c |   2 +-
 lib/x86/asm/page.h  |   2 +
 lib/x86/vm.c        | 105 ++++++++++++++++++++--------------
 lib/x86/vm.h        |   2 +-
 x86/Makefile.common |   1 +
 x86/vmx.c           |  19 +++----
 x86/vmx_tests.c     |  28 ++++-----
 10 files changed, 217 insertions(+), 122 deletions(-)

-- 
2.7.4

[kvm-unit-tests PATCH v2 1/6] lib/x86/vm: collection of improvements

[Andrew Jones]

Ensure we're page aligned, add locking, just return if NULL is
passed to vfree().

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/x86/asm/page.h |  2 ++
 lib/x86/vm.c       | 44 +++++++++++++++++++++++++++++++++++++-------
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
index 5044a49ab0cc..dd999304f1f0 100644
--- a/lib/x86/asm/page.h
+++ b/lib/x86/asm/page.h
@@ -16,6 +16,8 @@
 
 #ifndef __ASSEMBLY__
 
+#define PAGE_ALIGN(addr)	ALIGN(addr, PAGE_SIZE)
+
 #ifdef __x86_64__
 #define LARGE_PAGE_SIZE	(512 * PAGE_SIZE)
 #else
diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index f7e778b3779c..baea17e7f475 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -1,37 +1,54 @@
 #include "fwcfg.h"
 #include "vm.h"
 #include "libcflat.h"
+#include "asm/spinlock.h"
 
+static struct spinlock heap_lock;
+static struct spinlock vm_lock;
 static void *free = 0;
 static void *vfree_top = 0;
 
 static void free_memory(void *mem, unsigned long size)
 {
+    assert(!((unsigned long)mem & ~PAGE_MASK));
+
+    spin_lock(&heap_lock);
+
+    free = NULL;
+
     while (size >= PAGE_SIZE) {
 	*(void **)mem = free;
 	free = mem;
 	mem += PAGE_SIZE;
 	size -= PAGE_SIZE;
     }
+
+    spin_unlock(&heap_lock);
 }
 
 void *alloc_page()
 {
     void *p;
 
+    spin_lock(&heap_lock);
+
     if (!free)
-	return 0;
+	return NULL;
 
     p = free;
     free = *(void **)free;
 
+    spin_unlock(&heap_lock);
+
     return p;
 }
 
 void free_page(void *page)
 {
+    spin_lock(&heap_lock);
     *(void **)page = free;
     free = page;
+    spin_unlock(&heap_lock);
 }
 
 extern char edata;
@@ -162,11 +179,13 @@ void *vmalloc(unsigned long size)
     void *mem, *p;
     unsigned pages;
 
-    size += sizeof(unsigned long);
+    size = PAGE_ALIGN(size + sizeof(unsigned long));
 
-    size = (size + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1);
+    spin_lock(&vm_lock);
     vfree_top -= size;
     mem = p = vfree_top;
+    spin_unlock(&vm_lock);
+
     pages = size / PAGE_SIZE;
     while (pages--) {
 	install_page(phys_to_virt(read_cr3()), virt_to_phys(alloc_page()), p);
@@ -179,12 +198,18 @@ void *vmalloc(unsigned long size)
 
 uint64_t virt_to_phys_cr3(void *mem)
 {
-    return (*get_pte(phys_to_virt(read_cr3()), mem) & PT_ADDR_MASK) + ((ulong)mem & (PAGE_SIZE - 1));
+    return (*get_pte(phys_to_virt(read_cr3()), mem) & PT_ADDR_MASK) + ((ulong)mem & ~PAGE_MASK);
 }
 
 void vfree(void *mem)
 {
-    unsigned long size = ((unsigned long *)mem)[-1];
+    unsigned long size;
+
+    if (mem == NULL)
+	return;
+
+    mem -= sizeof(unsigned long);
+    size = *(unsigned long *)mem;
 
     while (size) {
 	free_page(phys_to_virt(*get_pte(phys_to_virt(read_cr3()), mem) & PT_ADDR_MASK));
@@ -198,11 +223,14 @@ void *vmap(unsigned long long phys, unsigned long size)
     void *mem, *p;
     unsigned pages;
 
-    size = (size + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1);
-    vfree_top -= size;
+    size = PAGE_ALIGN(size);
     phys &= ~(unsigned long long)(PAGE_SIZE - 1);
 
+    spin_lock(&vm_lock);
+    vfree_top -= size;
     mem = p = vfree_top;
+    spin_unlock(&vm_lock);
+
     pages = size / PAGE_SIZE;
     while (pages--) {
 	install_page(phys_to_virt(read_cr3()), phys, p);
@@ -214,7 +242,9 @@ void *vmap(unsigned long long phys, unsigned long size)
 
 void *alloc_vpages(ulong nr)
 {
+	spin_lock(&vm_lock);
 	vfree_top -= PAGE_SIZE * nr;
+	spin_unlock(&vm_lock);
 	return vfree_top;
 }
 
-- 
2.7.4

[kvm-unit-tests PATCH v2 2/6] lib/alloc: improve init

[Andrew Jones]

Add some asserts to make sure phys_alloc_init is called only once
and before any other alloc calls. Also require that an alignment
is set up just once and *before* init, or never.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/alloc.c         | 26 ++++++++++++++++++--------
 lib/alloc.h         |  7 +++++--
 lib/arm/setup.c     |  2 +-
 lib/powerpc/setup.c |  2 +-
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/lib/alloc.c b/lib/alloc.c
index e1d7b8a380ff..5b3ef5d00f8a 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -43,18 +43,28 @@ void phys_alloc_show(void)
 void phys_alloc_init(phys_addr_t base_addr, phys_addr_t size)
 {
 	spin_lock(&lock);
+
+	if (!align_min)
+		align_min = DEFAULT_MINIMUM_ALIGNMENT;
+
+	assert(!top);
+	assert(!(base_addr & (align_min - 1)));
+	assert(size);
+
 	base = base_addr;
 	top = base + size;
-	align_min = DEFAULT_MINIMUM_ALIGNMENT;
-	nr_regions = 0;
+
 	spin_unlock(&lock);
 }
 
 void phys_alloc_set_minimum_alignment(phys_addr_t align)
 {
-	assert(align && !(align & (align - 1)));
 	spin_lock(&lock);
+
+	assert(!align_min);
+	assert(align && !(align & (align - 1)));
 	align_min = align;
+
 	spin_unlock(&lock);
 }
 
@@ -63,14 +73,14 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
 {
 	static bool warned = false;
 	phys_addr_t addr, size_orig = size;
-	u64 top_safe;
+	u64 top_safe = top;
 
-	spin_lock(&lock);
+	if (safe && sizeof(long) == 4)
+		top_safe = MIN(top, 1ULL << 32);
 
-	top_safe = top;
+	assert(top_safe && base < top_safe);
 
-	if (safe && sizeof(long) == 4)
-		top_safe = MIN(top_safe, 1ULL << 32);
+	spin_lock(&lock);
 
 	align = MAX(align, align_min);
 
diff --git a/lib/alloc.h b/lib/alloc.h
index c12bd15f7afc..bd3c4e8ff3f6 100644
--- a/lib/alloc.h
+++ b/lib/alloc.h
@@ -73,13 +73,16 @@ static inline void *memalign(size_t alignment, size_t size)
 
 /*
  * phys_alloc_init creates the initial free memory region of size @size
- * at @base. The minimum alignment is set to DEFAULT_MINIMUM_ALIGNMENT.
+ * at @base. If a minimum alignment has not been set then
+ * DEFAULT_MINIMUM_ALIGNMENT is used. phys_alloc_init may only be called
+ * once.
  */
 extern void phys_alloc_init(phys_addr_t base, phys_addr_t size);
 
 /*
  * phys_alloc_set_minimum_alignment sets the minimum alignment to
- * @align.
+ * @align. phys_alloc_set_minimum_alignment must be called before
+ * phys_alloc_init and only once.
  */
 extern void phys_alloc_set_minimum_alignment(phys_addr_t align);
 
diff --git a/lib/arm/setup.c b/lib/arm/setup.c
index 7e7b39f11dde..d02d1a731d20 100644
--- a/lib/arm/setup.c
+++ b/lib/arm/setup.c
@@ -94,8 +94,8 @@ static void mem_init(phys_addr_t freemem_start)
 	__phys_offset = mem.start;	/* PHYS_OFFSET */
 	__phys_end = mem.end;		/* PHYS_END */
 
-	phys_alloc_init(freemem_start, primary.end - freemem_start);
 	phys_alloc_set_minimum_alignment(SMP_CACHE_BYTES);
+	phys_alloc_init(freemem_start, primary.end - freemem_start);
 
 	mmu_enable_idmap();
 }
diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
index 9153f7bebc6a..b390dadaebbe 100644
--- a/lib/powerpc/setup.c
+++ b/lib/powerpc/setup.c
@@ -146,9 +146,9 @@ static void mem_init(phys_addr_t freemem_start)
 	__physical_start = mem.start;	/* PHYSICAL_START */
 	__physical_end = mem.end;	/* PHYSICAL_END */
 
-	phys_alloc_init(freemem_start, primary.end - freemem_start);
 	phys_alloc_set_minimum_alignment(__icache_bytes > __dcache_bytes
 					 ? __icache_bytes : __dcache_bytes);
+	phys_alloc_init(freemem_start, primary.end - freemem_start);
 }
 
 void setup(const void *fdt)
-- 
2.7.4

[kvm-unit-tests PATCH v2 3/6] lib/alloc: prepare to extend alloc.c's purpose

[Andrew Jones]

Pick better names for phys_alloc state to tidy up a bit
before we add new functions.

(No functional change.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/alloc.c | 97 ++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 51 insertions(+), 46 deletions(-)

diff --git a/lib/alloc.c b/lib/alloc.c
index 5b3ef5d00f8a..1d990a803825 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -10,62 +10,65 @@
 #define MIN(a, b)		((a) < (b) ? (a) : (b))
 #define MAX(a, b)		((a) > (b) ? (a) : (b))
 
-#define PHYS_ALLOC_NR_REGIONS	256
+#define PHYS_ALLOC_NR_LOGS	256
 
 struct phys_alloc_region {
 	phys_addr_t base;
 	phys_addr_t size;
 };
 
-static struct phys_alloc_region regions[PHYS_ALLOC_NR_REGIONS];
-static int nr_regions;
+static struct phys_alloc_region phys_alloc_log[PHYS_ALLOC_NR_LOGS];
+static int phys_alloc_nr_logs;
 
-static struct spinlock lock;
-static phys_addr_t base, top, align_min;
+static struct spinlock phys_alloc_lock;
+static phys_addr_t phys_alloc_base;
+static phys_addr_t phys_alloc_top;
+static phys_addr_t phys_alloc_align_min;
 
 void phys_alloc_show(void)
 {
 	int i;
 
-	spin_lock(&lock);
+	spin_lock(&phys_alloc_lock);
 	printf("phys_alloc minimum alignment: 0x%" PRIx64 "\n",
-		(u64)align_min);
-	for (i = 0; i < nr_regions; ++i)
+		(u64)phys_alloc_align_min);
+	for (i = 0; i < phys_alloc_nr_logs; ++i)
 		printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
-			(u64)regions[i].base,
-			(u64)(regions[i].base + regions[i].size - 1),
+			(u64)phys_alloc_log[i].base,
+			(u64)(phys_alloc_log[i].base +
+			      phys_alloc_log[i].size - 1),
 			"USED");
 	printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
-		(u64)base, (u64)(top - 1), "FREE");
-	spin_unlock(&lock);
+		(u64)phys_alloc_base, (u64)(phys_alloc_top - 1), "FREE");
+	spin_unlock(&phys_alloc_lock);
 }
 
-void phys_alloc_init(phys_addr_t base_addr, phys_addr_t size)
+void phys_alloc_init(phys_addr_t base, phys_addr_t size)
 {
-	spin_lock(&lock);
+	spin_lock(&phys_alloc_lock);
 
-	if (!align_min)
-		align_min = DEFAULT_MINIMUM_ALIGNMENT;
+	if (!phys_alloc_align_min)
+		phys_alloc_align_min = DEFAULT_MINIMUM_ALIGNMENT;
 
-	assert(!top);
-	assert(!(base_addr & (align_min - 1)));
+	assert(!phys_alloc_top);
+	assert(!(base & (phys_alloc_align_min - 1)));
 	assert(size);
 
-	base = base_addr;
-	top = base + size;
+	phys_alloc_base = base;
+	phys_alloc_top = base + size;
 
-	spin_unlock(&lock);
+	spin_unlock(&phys_alloc_lock);
 }
 
 void phys_alloc_set_minimum_alignment(phys_addr_t align)
 {
-	spin_lock(&lock);
+	spin_lock(&phys_alloc_lock);
 
-	assert(!align_min);
+	assert(!phys_alloc_align_min);
 	assert(align && !(align & (align - 1)));
-	align_min = align;
+	phys_alloc_align_min = align;
 
-	spin_unlock(&lock);
+	spin_unlock(&phys_alloc_lock);
 }
 
 static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
@@ -73,44 +76,45 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
 {
 	static bool warned = false;
 	phys_addr_t addr, size_orig = size;
-	u64 top_safe = top;
+	u64 top_safe = phys_alloc_top;
 
 	if (safe && sizeof(long) == 4)
-		top_safe = MIN(top, 1ULL << 32);
+		top_safe = MIN(phys_alloc_top, 1ULL << 32);
 
-	assert(top_safe && base < top_safe);
+	assert(top_safe && phys_alloc_base < top_safe);
 
-	spin_lock(&lock);
+	spin_lock(&phys_alloc_lock);
 
-	align = MAX(align, align_min);
+	align = MAX(align, phys_alloc_align_min);
 
-	addr = ALIGN(base, align);
-	size += addr - base;
+	addr = ALIGN(phys_alloc_base, align);
+	size += addr - phys_alloc_base;
 
-	if ((top_safe - base) < size) {
+	if ((top_safe - phys_alloc_base) < size) {
 		printf("phys_alloc: requested=0x%" PRIx64
 		       " (align=0x%" PRIx64 "), "
 		       "need=0x%" PRIx64 ", but free=0x%" PRIx64 ". "
 		       "top=0x%" PRIx64 ", top_safe=0x%" PRIx64 "\n",
-		       (u64)size_orig, (u64)align, (u64)size, top_safe - base,
-		       (u64)top, top_safe);
-		spin_unlock(&lock);
+		       (u64)size_orig, (u64)align, (u64)size,
+		       top_safe - phys_alloc_base, (u64)phys_alloc_top,
+		       top_safe);
+		spin_unlock(&phys_alloc_lock);
 		return INVALID_PHYS_ADDR;
 	}
 
-	base += size;
+	phys_alloc_base += size;
 
-	if (nr_regions < PHYS_ALLOC_NR_REGIONS) {
-		regions[nr_regions].base = addr;
-		regions[nr_regions].size = size_orig;
-		++nr_regions;
+	if (phys_alloc_nr_logs < PHYS_ALLOC_NR_LOGS) {
+		phys_alloc_log[phys_alloc_nr_logs].base = addr;
+		phys_alloc_log[phys_alloc_nr_logs].size = size_orig;
+		++phys_alloc_nr_logs;
 	} else if (!warned) {
 		printf("WARNING: phys_alloc: No free log entries, "
 		       "can no longer log allocations...\n");
 		warned = true;
 	}
 
-	spin_unlock(&lock);
+	spin_unlock(&phys_alloc_lock);
 
 	return addr;
 }
@@ -138,17 +142,18 @@ phys_addr_t phys_zalloc_aligned(phys_addr_t size, phys_addr_t align)
 
 phys_addr_t phys_alloc(phys_addr_t size)
 {
-	return phys_alloc_aligned(size, align_min);
+	return phys_alloc_aligned(size, phys_alloc_align_min);
 }
 
 phys_addr_t phys_zalloc(phys_addr_t size)
 {
-	return phys_zalloc_aligned(size, align_min);
+	return phys_zalloc_aligned(size, phys_alloc_align_min);
 }
 
 static void *early_malloc(size_t size)
 {
-	phys_addr_t addr = phys_alloc_aligned_safe(size, align_min, true);
+	phys_addr_t addr = phys_alloc_aligned_safe(size,
+					phys_alloc_align_min, true);
 	if (addr == INVALID_PHYS_ADDR)
 		return NULL;
 
@@ -158,7 +163,7 @@ static void *early_malloc(size_t size)
 static void *early_calloc(size_t nmemb, size_t size)
 {
 	phys_addr_t addr = phys_zalloc_aligned_safe(nmemb * size,
-						    align_min, true);
+					phys_alloc_align_min, true);
 	if (addr == INVALID_PHYS_ADDR)
 		return NULL;
 
-- 
2.7.4

[kvm-unit-tests PATCH v2 4/6] x86: lib/alloc: move heap management to lib

[Andrew Jones]

This will allow other arches to use {alloc,free}_page.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/alloc.c         | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/alloc.h         | 10 ++++++++++
 lib/x86/vm.c        | 48 ++----------------------------------------------
 x86/Makefile.common |  1 +
 4 files changed, 62 insertions(+), 46 deletions(-)

diff --git a/lib/alloc.c b/lib/alloc.c
index 1d990a803825..ce1198e2977f 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -5,6 +5,7 @@
  */
 #include "alloc.h"
 #include "asm/spinlock.h"
+#include "asm/page.h"
 #include "asm/io.h"
 
 #define MIN(a, b)		((a) < (b) ? (a) : (b))
@@ -150,6 +151,54 @@ phys_addr_t phys_zalloc(phys_addr_t size)
 	return phys_zalloc_aligned(size, phys_alloc_align_min);
 }
 
+static struct spinlock heap_lock;
+static void *heap_free_head;
+
+void heap_init(void *start, size_t size)
+{
+	void *p = start;
+
+	assert(!((unsigned long)start & ~PAGE_MASK));
+
+	spin_lock(&heap_lock);
+
+	heap_free_head = NULL;
+
+	while (size >= PAGE_SIZE) {
+		*(void **)p = heap_free_head;
+		heap_free_head = p;
+		p += PAGE_SIZE;
+		size -= PAGE_SIZE;
+	}
+
+	spin_unlock(&heap_lock);
+}
+
+void *alloc_page(void)
+{
+	void *p;
+
+	spin_lock(&heap_lock);
+
+	if (!heap_free_head)
+		return NULL;
+
+	p = heap_free_head;
+	heap_free_head = *(void **)heap_free_head;
+
+	spin_unlock(&heap_lock);
+
+	return p;
+}
+
+void free_page(void *page)
+{
+	spin_lock(&heap_lock);
+	*(void **)page = heap_free_head;
+	heap_free_head = page;
+	spin_unlock(&heap_lock);
+}
+
 static void *early_malloc(size_t size)
 {
 	phys_addr_t addr = phys_alloc_aligned_safe(size,
diff --git a/lib/alloc.h b/lib/alloc.h
index bd3c4e8ff3f6..a37330b3088a 100644
--- a/lib/alloc.h
+++ b/lib/alloc.h
@@ -118,4 +118,14 @@ extern phys_addr_t phys_zalloc(phys_addr_t size);
  */
 extern void phys_alloc_show(void);
 
+/*
+ * Heap page allocator
+ *
+ * After initializing with heap_init, {alloc,free}_page can be used
+ * to easily manage pages.
+ */
+extern void heap_init(void *start, size_t size);
+extern void *alloc_page(void);
+extern void free_page(void *page);
+
 #endif /* _ALLOC_H_ */
diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index baea17e7f475..4e399f80dd31 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -1,56 +1,12 @@
 #include "fwcfg.h"
 #include "vm.h"
 #include "libcflat.h"
+#include "alloc.h"
 #include "asm/spinlock.h"
 
-static struct spinlock heap_lock;
 static struct spinlock vm_lock;
-static void *free = 0;
 static void *vfree_top = 0;
 
-static void free_memory(void *mem, unsigned long size)
-{
-    assert(!((unsigned long)mem & ~PAGE_MASK));
-
-    spin_lock(&heap_lock);
-
-    free = NULL;
-
-    while (size >= PAGE_SIZE) {
-	*(void **)mem = free;
-	free = mem;
-	mem += PAGE_SIZE;
-	size -= PAGE_SIZE;
-    }
-
-    spin_unlock(&heap_lock);
-}
-
-void *alloc_page()
-{
-    void *p;
-
-    spin_lock(&heap_lock);
-
-    if (!free)
-	return NULL;
-
-    p = free;
-    free = *(void **)free;
-
-    spin_unlock(&heap_lock);
-
-    return p;
-}
-
-void free_page(void *page)
-{
-    spin_lock(&heap_lock);
-    *(void **)page = free;
-    free = page;
-    spin_unlock(&heap_lock);
-}
-
 extern char edata;
 static unsigned long end_of_memory;
 
@@ -170,7 +126,7 @@ void setup_vm()
 {
     assert(!end_of_memory);
     end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
-    free_memory(&edata, end_of_memory - (unsigned long)&edata);
+    heap_init(&edata, end_of_memory - (unsigned long)&edata);
     setup_mmu(end_of_memory);
 }
 
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 356d879a986b..88038e41af60 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -3,6 +3,7 @@
 all: test_cases
 
 cflatobjs += lib/pci.o
+cflatobjs += lib/alloc.o
 cflatobjs += lib/x86/io.o
 cflatobjs += lib/x86/smp.o
 cflatobjs += lib/x86/vm.o
-- 
2.7.4

[kvm-unit-tests PATCH v2 5/6] x86: lib/alloc: introduce alloc_zeroed_page

[Andrew Jones]

This allows us to remove a bunch of memsets.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/alloc.c     |  8 ++++++++
 lib/alloc.h     |  1 +
 lib/x86/vm.c    |  4 +---
 x86/vmx.c       | 19 +++++++------------
 x86/vmx_tests.c | 28 ++++++++++------------------
 5 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/lib/alloc.c b/lib/alloc.c
index ce1198e2977f..d797690cc458 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -191,6 +191,14 @@ void *alloc_page(void)
 	return p;
 }
 
+void *alloc_zeroed_page(void)
+{
+	void *p = alloc_page();
+
+	memset(p, 0, PAGE_SIZE);
+	return p;
+}
+
 void free_page(void *page)
 {
 	spin_lock(&heap_lock);
diff --git a/lib/alloc.h b/lib/alloc.h
index a37330b3088a..f61a5200c829 100644
--- a/lib/alloc.h
+++ b/lib/alloc.h
@@ -126,6 +126,7 @@ extern void phys_alloc_show(void);
  */
 extern void heap_init(void *start, size_t size);
 extern void *alloc_page(void);
+extern void *alloc_zeroed_page(void);
 extern void free_page(void *page);
 
 #endif /* _ALLOC_H_ */
diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 4e399f80dd31..8b95104ef80f 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -91,9 +91,7 @@ static void setup_mmu_range(unsigned long *cr3, unsigned long start,
 
 static void setup_mmu(unsigned long len)
 {
-    unsigned long *cr3 = alloc_page();
-
-    memset(cr3, 0, PAGE_SIZE);
+    unsigned long *cr3 = alloc_zeroed_page();
 
 #ifdef __x86_64__
     if (len < (1ul << 32))
diff --git a/x86/vmx.c b/x86/vmx.c
index 411ed3211d4d..5d333e077a02 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -29,6 +29,7 @@
  */
 
 #include "libcflat.h"
+#include "alloc.h"
 #include "processor.h"
 #include "vm.h"
 #include "desc.h"
@@ -276,9 +277,8 @@ static void split_large_ept_entry(unsigned long *ptep, int level)
 	assert(pte & EPT_LARGE_PAGE);
 	assert(level == 2 || level == 3);
 
-	new_pt = alloc_page();
+	new_pt = alloc_zeroed_page();
 	assert(new_pt);
-	memset(new_pt, 0, PAGE_SIZE);
 
 	prototype = pte & ~EPT_ADDR_MASK;
 	if (level == 2)
@@ -617,8 +617,7 @@ static void init_vmcs_guest(void)
 
 static int init_vmcs(struct vmcs **vmcs)
 {
-	*vmcs = alloc_page();
-	memset(*vmcs, 0, PAGE_SIZE);
+	*vmcs = alloc_zeroed_page();
 	(*vmcs)->revision_id = basic.revision;
 	/* vmclear first to init vmcs */
 	if (vmcs_clear(*vmcs)) {
@@ -656,8 +655,7 @@ static void init_vmx(void)
 	ulong fix_cr0_set, fix_cr0_clr;
 	ulong fix_cr4_set, fix_cr4_clr;
 
-	vmxon_region = alloc_page();
-	memset(vmxon_region, 0, PAGE_SIZE);
+	vmxon_region = alloc_zeroed_page();
 
 	fix_cr0_set =  rdmsr(MSR_IA32_VMX_CR0_FIXED0);
 	fix_cr0_clr =  rdmsr(MSR_IA32_VMX_CR0_FIXED1);
@@ -686,10 +684,8 @@ static void init_vmx(void)
 
 	*vmxon_region = basic.revision;
 
-	guest_stack = alloc_page();
-	memset(guest_stack, 0, PAGE_SIZE);
-	guest_syscall_stack = alloc_page();
-	memset(guest_syscall_stack, 0, PAGE_SIZE);
+	guest_stack = alloc_zeroed_page();
+	guest_syscall_stack = alloc_zeroed_page();
 }
 
 static void do_vmxon_off(void *data)
@@ -811,8 +807,7 @@ static void test_vmptrst(void)
 	int ret;
 	struct vmcs *vmcs1, *vmcs2;
 
-	vmcs1 = alloc_page();
-	memset(vmcs1, 0, PAGE_SIZE);
+	vmcs1 = alloc_zeroed_page();
 	init_vmcs(&vmcs1);
 	ret = vmcs_save(&vmcs2);
 	report("test vmptrst", (!ret) && (vmcs1 == vmcs2));
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 58736d789bd5..ebc220e8329c 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3,6 +3,7 @@
  *
  * Author : Arthur Chunqi Li <yzt356@gmail.com>
  */
+#include "alloc.h"
 #include "vmx.h"
 #include "msr.h"
 #include "processor.h"
@@ -223,8 +224,7 @@ void msr_bmp_init()
 	void *msr_bitmap;
 	u32 ctrl_cpu0;
 
-	msr_bitmap = alloc_page();
-	memset(msr_bitmap, 0x0, PAGE_SIZE);
+	msr_bitmap = alloc_zeroed_page();
 	ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
 	ctrl_cpu0 |= CPU_MSR_BITMAP;
 	vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
@@ -565,10 +565,8 @@ static int iobmp_init()
 {
 	u32 ctrl_cpu0;
 
-	io_bitmap_a = alloc_page();
-	io_bitmap_b = alloc_page();
-	memset(io_bitmap_a, 0x0, PAGE_SIZE);
-	memset(io_bitmap_b, 0x0, PAGE_SIZE);
+	io_bitmap_a = alloc_zeroed_page();
+	io_bitmap_b = alloc_zeroed_page();
 	ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
 	ctrl_cpu0 |= CPU_IO_BITMAP;
 	ctrl_cpu0 &= (~CPU_IO);
@@ -937,8 +935,7 @@ static int setup_ept()
 		return 1;
 	}
 	eptp |= (3 << EPTP_PG_WALK_LEN_SHIFT);
-	pml4 = alloc_page();
-	memset(pml4, 0, PAGE_SIZE);
+	pml4 = alloc_zeroed_page();
 	eptp |= virt_to_phys(pml4);
 	vmcs_write(EPTP, eptp);
 	support_2m = !!(ept_vpid.val & EPT_CAP_2M_PAGE);
@@ -972,10 +969,8 @@ static int ept_init()
 	vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
 	if (setup_ept())
 		return VMX_TEST_EXIT;
-	data_page1 = alloc_page();
-	data_page2 = alloc_page();
-	memset(data_page1, 0x0, PAGE_SIZE);
-	memset(data_page2, 0x0, PAGE_SIZE);
+	data_page1 = alloc_zeroed_page();
+	data_page2 = alloc_zeroed_page();
 	*((u32 *)data_page1) = MAGIC_VAL_1;
 	*((u32 *)data_page2) = MAGIC_VAL_2;
 	install_ept(pml4, (unsigned long)data_page1, (unsigned long)data_page2,
@@ -1538,12 +1533,9 @@ struct vmx_msr_entry *exit_msr_store, *entry_msr_load, *exit_msr_load;
 static int msr_switch_init(struct vmcs *vmcs)
 {
 	msr_bmp_init();
-	exit_msr_store = alloc_page();
-	exit_msr_load = alloc_page();
-	entry_msr_load = alloc_page();
-	memset(exit_msr_store, 0, PAGE_SIZE);
-	memset(exit_msr_load, 0, PAGE_SIZE);
-	memset(entry_msr_load, 0, PAGE_SIZE);
+	exit_msr_store = alloc_zeroed_page();
+	exit_msr_load = alloc_zeroed_page();
+	entry_msr_load = alloc_zeroed_page();
 	entry_msr_load[0].index = MSR_KERNEL_GS_BASE;
 	entry_msr_load[0].value = MSR_MAGIC;
 
-- 
2.7.4

[kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Andrew Jones]

We've had malloc, calloc, free, and memalign available for quite a
while, and arm and powerpc make use of them. x86 hasn't yet, which
is fine, but could lead to problems if common lib code wants to make
use of them. While arm and powerpc currently use early_alloc_ops,
built on phys_alloc, x86 does not initialize phys_alloc and has its
own memory management, vmalloc. This patch enables vmalloc to be
used as the underlying allocator, but there are two drawbacks:

 1) Every allocation will allocate at least one page.
 2) It can only be used with the MMU enabled.

Drawback (1) is probably fine for kvm-unit-tests. Drawback (2) may
not be, as common lib code may get invoked at any time by any unit
test, including ones that don't have the MMU enabled. However, as
we only switch alloc_ops to the vmalloc based implementations in
setup_vm, where the MMU is enabled, then they'll be safe to use for
any unit test that invokes setup_vm first. If the unit test does
not invoke setup_vm first then alloc_ops will use the default
implementations, the ones based on phys_alloc, and will result
in asserts firing if phys_alloc_init has not be called first.

While this patch may not enable anything right now, I think it
makes sense to enable these alloc ops before they're needed, because
then everything will most likely just work when a future common lib
function that uses malloc is introduced. If that common function
results in a unit test hitting an assert, then the test writer will
just need to decide if they want to use phys_alloc or vmalloc and
call the appropriate init function first.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/x86/vm.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 lib/x86/vm.h |  2 +-
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 8b95104ef80f..955c9b8afea5 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -120,20 +120,52 @@ static void setup_mmu(unsigned long len)
     printf("cr4 = %lx\n", read_cr4());
 }
 
+static void *vcalloc(size_t nmemb, size_t size)
+{
+    void *addr = vmalloc(nmemb * size);
+    memset(addr, 0, nmemb * size);
+    return addr;
+}
+
+static void *vmemalign(size_t alignment, size_t size)
+{
+    void *base, *addr;
+    size_t offset;
+
+    assert(alignment && !(alignment & (alignment - 1)));
+
+    alignment = ALIGN(alignment, sizeof(size_t));
+    base = vmalloc(size + alignment);
+    offset = ((size_t *)base)[-1];
+
+    addr = (void *)ALIGN((ulong)base, alignment);
+    ((size_t *)addr)[-1] = offset + (addr - base);
+    return addr;
+}
+
+static struct alloc_ops vm_alloc_ops = {
+    .malloc = vmalloc,
+    .calloc = vcalloc,
+    .free = vfree,
+    .memalign = vmemalign,
+};
+
 void setup_vm()
 {
     assert(!end_of_memory);
     end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
     heap_init(&edata, end_of_memory - (unsigned long)&edata);
     setup_mmu(end_of_memory);
+    alloc_ops = &vm_alloc_ops;
 }
 
-void *vmalloc(unsigned long size)
+void *vmalloc(size_t size)
 {
     void *mem, *p;
     unsigned pages;
+    size_t offset = sizeof(size_t) * 2;
 
-    size = PAGE_ALIGN(size + sizeof(unsigned long));
+    size = PAGE_ALIGN(size + offset);
 
     spin_lock(&vm_lock);
     vfree_top -= size;
@@ -145,8 +177,11 @@ void *vmalloc(unsigned long size)
 	install_page(phys_to_virt(read_cr3()), virt_to_phys(alloc_page()), p);
 	p += PAGE_SIZE;
     }
-    *(unsigned long *)mem = size;
-    mem += sizeof(unsigned long);
+
+    ((size_t *)mem)[0] = size;
+    ((size_t *)mem)[1] = offset;
+    mem += offset;
+
     return mem;
 }
 
@@ -157,13 +192,13 @@ uint64_t virt_to_phys_cr3(void *mem)
 
 void vfree(void *mem)
 {
-    unsigned long size;
+    size_t size;
 
     if (mem == NULL)
 	return;
 
-    mem -= sizeof(unsigned long);
-    size = *(unsigned long *)mem;
+    mem -= ((size_t *)mem)[-1]; /* offset */
+    size = *(size_t *)mem;
 
     while (size) {
 	free_page(phys_to_virt(*get_pte(phys_to_virt(read_cr3()), mem) & PT_ADDR_MASK));
diff --git a/lib/x86/vm.h b/lib/x86/vm.h
index 6a4384f5a48d..3c9a71d03cf9 100644
--- a/lib/x86/vm.h
+++ b/lib/x86/vm.h
@@ -7,7 +7,7 @@
 
 void setup_vm();
 
-void *vmalloc(unsigned long size);
+void *vmalloc(size_t size);
 void vfree(void *mem);
 void *vmap(unsigned long long phys, unsigned long size);
 void *alloc_vpage(void);
-- 
2.7.4

Re: [kvm-unit-tests PATCH v2 1/6] lib/x86/vm: collection of improvements

[Laurent Vivier]

On 02/11/2016 21:52, Andrew Jones wrote:
> Ensure we're page aligned, add locking, just return if NULL is
> passed to vfree().
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/x86/asm/page.h |  2 ++
>  lib/x86/vm.c       | 44 +++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
> index 5044a49ab0cc..dd999304f1f0 100644
> --- a/lib/x86/asm/page.h
> +++ b/lib/x86/asm/page.h
> @@ -16,6 +16,8 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#define PAGE_ALIGN(addr)	ALIGN(addr, PAGE_SIZE)
> +
>  #ifdef __x86_64__
>  #define LARGE_PAGE_SIZE	(512 * PAGE_SIZE)
>  #else
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index f7e778b3779c..baea17e7f475 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -1,37 +1,54 @@
>  #include "fwcfg.h"
>  #include "vm.h"
>  #include "libcflat.h"
> +#include "asm/spinlock.h"
>  
> +static struct spinlock heap_lock;
> +static struct spinlock vm_lock;
>  static void *free = 0;
>  static void *vfree_top = 0;
>  
>  static void free_memory(void *mem, unsigned long size)
>  {
> +    assert(!((unsigned long)mem & ~PAGE_MASK));
> +
> +    spin_lock(&heap_lock);
> +
> +    free = NULL;
> +
>      while (size >= PAGE_SIZE) {
>  	*(void **)mem = free;
>  	free = mem;
>  	mem += PAGE_SIZE;
>  	size -= PAGE_SIZE;
>      }
> +
> +    spin_unlock(&heap_lock);
>  }
>  
>  void *alloc_page()
>  {
>      void *p;
>  
> +    spin_lock(&heap_lock);
> +
>      if (!free)
> -	return 0;
> +	return NULL;

You must unlock heap_lock before return

Laurent

Re: [kvm-unit-tests PATCH v2 1/6] lib/x86/vm: collection of improvements

[Andrew Jones]

On Thu, Nov 03, 2016 at 10:40:14AM +0100, Laurent Vivier wrote:
> >  void *alloc_page()
> >  {
> >      void *p;
> >  
> > +    spin_lock(&heap_lock);
> > +
> >      if (!free)
> > -	return 0;
> > +	return NULL;
> 
> You must unlock heap_lock before return

Doh! Nice catch!

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 2/6] lib/alloc: improve init

[Laurent Vivier]

On 02/11/2016 21:52, Andrew Jones wrote:
> Add some asserts to make sure phys_alloc_init is called only once
> and before any other alloc calls. Also require that an alignment
> is set up just once and *before* init, or never.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  lib/alloc.c         | 26 ++++++++++++++++++--------
>  lib/alloc.h         |  7 +++++--
>  lib/arm/setup.c     |  2 +-
>  lib/powerpc/setup.c |  2 +-
>  4 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index e1d7b8a380ff..5b3ef5d00f8a 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -43,18 +43,28 @@ void phys_alloc_show(void)
>  void phys_alloc_init(phys_addr_t base_addr, phys_addr_t size)
>  {
>  	spin_lock(&lock);
> +
> +	if (!align_min)
> +		align_min = DEFAULT_MINIMUM_ALIGNMENT;
> +
> +	assert(!top);
> +	assert(!(base_addr & (align_min - 1)));
> +	assert(size);
> +
>  	base = base_addr;
>  	top = base + size;
> -	align_min = DEFAULT_MINIMUM_ALIGNMENT;
> -	nr_regions = 0;
> +
>  	spin_unlock(&lock);
>  }
>  
>  void phys_alloc_set_minimum_alignment(phys_addr_t align)
>  {
> -	assert(align && !(align & (align - 1)));
>  	spin_lock(&lock);
> +
> +	assert(!align_min);
> +	assert(align && !(align & (align - 1)));
>  	align_min = align;
> +
>  	spin_unlock(&lock);
>  }
>  
> @@ -63,14 +73,14 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
>  {
>  	static bool warned = false;
>  	phys_addr_t addr, size_orig = size;
> -	u64 top_safe;
> +	u64 top_safe = top;
>  
> -	spin_lock(&lock);
> +	if (safe && sizeof(long) == 4)
> +		top_safe = MIN(top, 1ULL << 32);
>  
> -	top_safe = top;
> +	assert(top_safe && base < top_safe);
>  
> -	if (safe && sizeof(long) == 4)
> -		top_safe = MIN(top_safe, 1ULL << 32);
> +	spin_lock(&lock);
>  
>  	align = MAX(align, align_min);
>  
> diff --git a/lib/alloc.h b/lib/alloc.h
> index c12bd15f7afc..bd3c4e8ff3f6 100644
> --- a/lib/alloc.h
> +++ b/lib/alloc.h
> @@ -73,13 +73,16 @@ static inline void *memalign(size_t alignment, size_t size)
>  
>  /*
>   * phys_alloc_init creates the initial free memory region of size @size
> - * at @base. The minimum alignment is set to DEFAULT_MINIMUM_ALIGNMENT.
> + * at @base. If a minimum alignment has not been set then
> + * DEFAULT_MINIMUM_ALIGNMENT is used. phys_alloc_init may only be called
> + * once.
>   */
>  extern void phys_alloc_init(phys_addr_t base, phys_addr_t size);
>  
>  /*
>   * phys_alloc_set_minimum_alignment sets the minimum alignment to
> - * @align.
> + * @align. phys_alloc_set_minimum_alignment must be called before
> + * phys_alloc_init and only once.
>   */
>  extern void phys_alloc_set_minimum_alignment(phys_addr_t align);
>  
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> index 7e7b39f11dde..d02d1a731d20 100644
> --- a/lib/arm/setup.c
> +++ b/lib/arm/setup.c
> @@ -94,8 +94,8 @@ static void mem_init(phys_addr_t freemem_start)
>  	__phys_offset = mem.start;	/* PHYS_OFFSET */
>  	__phys_end = mem.end;		/* PHYS_END */
>  
> -	phys_alloc_init(freemem_start, primary.end - freemem_start);
>  	phys_alloc_set_minimum_alignment(SMP_CACHE_BYTES);
> +	phys_alloc_init(freemem_start, primary.end - freemem_start);
>  
>  	mmu_enable_idmap();
>  }
> diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> index 9153f7bebc6a..b390dadaebbe 100644
> --- a/lib/powerpc/setup.c
> +++ b/lib/powerpc/setup.c
> @@ -146,9 +146,9 @@ static void mem_init(phys_addr_t freemem_start)
>  	__physical_start = mem.start;	/* PHYSICAL_START */
>  	__physical_end = mem.end;	/* PHYSICAL_END */
>  
> -	phys_alloc_init(freemem_start, primary.end - freemem_start);
>  	phys_alloc_set_minimum_alignment(__icache_bytes > __dcache_bytes
>  					 ? __icache_bytes : __dcache_bytes);
> +	phys_alloc_init(freemem_start, primary.end - freemem_start);
>  }
>  
>  void setup(const void *fdt)
> 

Re: [kvm-unit-tests PATCH v2 5/6] x86: lib/alloc: introduce alloc_zeroed_page

[Laurent Vivier]

On 02/11/2016 21:52, Andrew Jones wrote:
> This allows us to remove a bunch of memsets.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/alloc.c     |  8 ++++++++
>  lib/alloc.h     |  1 +
>  lib/x86/vm.c    |  4 +---
>  x86/vmx.c       | 19 +++++++------------
>  x86/vmx_tests.c | 28 ++++++++++------------------
>  5 files changed, 27 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index ce1198e2977f..d797690cc458 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -191,6 +191,14 @@ void *alloc_page(void)
>  	return p;
>  }
>  
> +void *alloc_zeroed_page(void)
> +{
> +	void *p = alloc_page();

alloc_page() can return NULL.

As most of the callers don't check the return value of
alloc_zeroed_page(), I think you should assert() here.


> +	memset(p, 0, PAGE_SIZE);
> +	return p;
> +}
> +
>  void free_page(void *page)
>  {
>  	spin_lock(&heap_lock);
> diff --git a/lib/alloc.h b/lib/alloc.h
> index a37330b3088a..f61a5200c829 100644
> --- a/lib/alloc.h
> +++ b/lib/alloc.h
> @@ -126,6 +126,7 @@ extern void phys_alloc_show(void);
>   */
>  extern void heap_init(void *start, size_t size);
>  extern void *alloc_page(void);
> +extern void *alloc_zeroed_page(void);
>  extern void free_page(void *page);
>  
>  #endif /* _ALLOC_H_ */
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 4e399f80dd31..8b95104ef80f 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -91,9 +91,7 @@ static void setup_mmu_range(unsigned long *cr3, unsigned long start,
>  
>  static void setup_mmu(unsigned long len)
>  {
> -    unsigned long *cr3 = alloc_page();
> -
> -    memset(cr3, 0, PAGE_SIZE);
> +    unsigned long *cr3 = alloc_zeroed_page();
>  
>  #ifdef __x86_64__
>      if (len < (1ul << 32))
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 411ed3211d4d..5d333e077a02 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -29,6 +29,7 @@
>   */
>  
>  #include "libcflat.h"
> +#include "alloc.h"
>  #include "processor.h"
>  #include "vm.h"
>  #include "desc.h"
> @@ -276,9 +277,8 @@ static void split_large_ept_entry(unsigned long *ptep, int level)
>  	assert(pte & EPT_LARGE_PAGE);
>  	assert(level == 2 || level == 3);
>  
> -	new_pt = alloc_page();
> +	new_pt = alloc_zeroed_page();
>  	assert(new_pt);

If alloc_zeroed_page() uses assert(), this one is useless, otherwise add
the same to all the other calls of alloc_zeroed_page() below.

> -	memset(new_pt, 0, PAGE_SIZE);
>  
>  	prototype = pte & ~EPT_ADDR_MASK;
>  	if (level == 2)
> @@ -617,8 +617,7 @@ static void init_vmcs_guest(void)
...

Thanks,
Laurent

Re: [kvm-unit-tests PATCH v2 3/6] lib/alloc: prepare to extend alloc.c's purpose

[Laurent Vivier]

On 02/11/2016 21:52, Andrew Jones wrote:
> Pick better names for phys_alloc state to tidy up a bit
> before we add new functions.
> 
> (No functional change.)
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  lib/alloc.c | 97 ++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 51 insertions(+), 46 deletions(-)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index 5b3ef5d00f8a..1d990a803825 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -10,62 +10,65 @@
>  #define MIN(a, b)		((a) < (b) ? (a) : (b))
>  #define MAX(a, b)		((a) > (b) ? (a) : (b))
>  
> -#define PHYS_ALLOC_NR_REGIONS	256
> +#define PHYS_ALLOC_NR_LOGS	256
>  
>  struct phys_alloc_region {
>  	phys_addr_t base;
>  	phys_addr_t size;
>  };
>  
> -static struct phys_alloc_region regions[PHYS_ALLOC_NR_REGIONS];
> -static int nr_regions;
> +static struct phys_alloc_region phys_alloc_log[PHYS_ALLOC_NR_LOGS];
> +static int phys_alloc_nr_logs;
>  
> -static struct spinlock lock;
> -static phys_addr_t base, top, align_min;
> +static struct spinlock phys_alloc_lock;
> +static phys_addr_t phys_alloc_base;
> +static phys_addr_t phys_alloc_top;
> +static phys_addr_t phys_alloc_align_min;
>  
>  void phys_alloc_show(void)
>  {
>  	int i;
>  
> -	spin_lock(&lock);
> +	spin_lock(&phys_alloc_lock);
>  	printf("phys_alloc minimum alignment: 0x%" PRIx64 "\n",
> -		(u64)align_min);
> -	for (i = 0; i < nr_regions; ++i)
> +		(u64)phys_alloc_align_min);
> +	for (i = 0; i < phys_alloc_nr_logs; ++i)
>  		printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
> -			(u64)regions[i].base,
> -			(u64)(regions[i].base + regions[i].size - 1),
> +			(u64)phys_alloc_log[i].base,
> +			(u64)(phys_alloc_log[i].base +
> +			      phys_alloc_log[i].size - 1),
>  			"USED");
>  	printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
> -		(u64)base, (u64)(top - 1), "FREE");
> -	spin_unlock(&lock);
> +		(u64)phys_alloc_base, (u64)(phys_alloc_top - 1), "FREE");
> +	spin_unlock(&phys_alloc_lock);
>  }
>  
> -void phys_alloc_init(phys_addr_t base_addr, phys_addr_t size)
> +void phys_alloc_init(phys_addr_t base, phys_addr_t size)
>  {
> -	spin_lock(&lock);
> +	spin_lock(&phys_alloc_lock);
>  
> -	if (!align_min)
> -		align_min = DEFAULT_MINIMUM_ALIGNMENT;
> +	if (!phys_alloc_align_min)
> +		phys_alloc_align_min = DEFAULT_MINIMUM_ALIGNMENT;
>  
> -	assert(!top);
> -	assert(!(base_addr & (align_min - 1)));
> +	assert(!phys_alloc_top);
> +	assert(!(base & (phys_alloc_align_min - 1)));
>  	assert(size);
>  
> -	base = base_addr;
> -	top = base + size;
> +	phys_alloc_base = base;
> +	phys_alloc_top = base + size;
>  
> -	spin_unlock(&lock);
> +	spin_unlock(&phys_alloc_lock);
>  }
>  
>  void phys_alloc_set_minimum_alignment(phys_addr_t align)
>  {
> -	spin_lock(&lock);
> +	spin_lock(&phys_alloc_lock);
>  
> -	assert(!align_min);
> +	assert(!phys_alloc_align_min);
>  	assert(align && !(align & (align - 1)));
> -	align_min = align;
> +	phys_alloc_align_min = align;
>  
> -	spin_unlock(&lock);
> +	spin_unlock(&phys_alloc_lock);
>  }
>  
>  static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
> @@ -73,44 +76,45 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
>  {
>  	static bool warned = false;
>  	phys_addr_t addr, size_orig = size;
> -	u64 top_safe = top;
> +	u64 top_safe = phys_alloc_top;
>  
>  	if (safe && sizeof(long) == 4)
> -		top_safe = MIN(top, 1ULL << 32);
> +		top_safe = MIN(phys_alloc_top, 1ULL << 32);
>  
> -	assert(top_safe && base < top_safe);
> +	assert(top_safe && phys_alloc_base < top_safe);
>  
> -	spin_lock(&lock);
> +	spin_lock(&phys_alloc_lock);
>  
> -	align = MAX(align, align_min);
> +	align = MAX(align, phys_alloc_align_min);
>  
> -	addr = ALIGN(base, align);
> -	size += addr - base;
> +	addr = ALIGN(phys_alloc_base, align);
> +	size += addr - phys_alloc_base;
>  
> -	if ((top_safe - base) < size) {
> +	if ((top_safe - phys_alloc_base) < size) {
>  		printf("phys_alloc: requested=0x%" PRIx64
>  		       " (align=0x%" PRIx64 "), "
>  		       "need=0x%" PRIx64 ", but free=0x%" PRIx64 ". "
>  		       "top=0x%" PRIx64 ", top_safe=0x%" PRIx64 "\n",
> -		       (u64)size_orig, (u64)align, (u64)size, top_safe - base,
> -		       (u64)top, top_safe);
> -		spin_unlock(&lock);
> +		       (u64)size_orig, (u64)align, (u64)size,
> +		       top_safe - phys_alloc_base, (u64)phys_alloc_top,
> +		       top_safe);
> +		spin_unlock(&phys_alloc_lock);
>  		return INVALID_PHYS_ADDR;
>  	}
>  
> -	base += size;
> +	phys_alloc_base += size;
>  
> -	if (nr_regions < PHYS_ALLOC_NR_REGIONS) {
> -		regions[nr_regions].base = addr;
> -		regions[nr_regions].size = size_orig;
> -		++nr_regions;
> +	if (phys_alloc_nr_logs < PHYS_ALLOC_NR_LOGS) {
> +		phys_alloc_log[phys_alloc_nr_logs].base = addr;
> +		phys_alloc_log[phys_alloc_nr_logs].size = size_orig;
> +		++phys_alloc_nr_logs;
>  	} else if (!warned) {
>  		printf("WARNING: phys_alloc: No free log entries, "
>  		       "can no longer log allocations...\n");
>  		warned = true;
>  	}
>  
> -	spin_unlock(&lock);
> +	spin_unlock(&phys_alloc_lock);
>  
>  	return addr;
>  }
> @@ -138,17 +142,18 @@ phys_addr_t phys_zalloc_aligned(phys_addr_t size, phys_addr_t align)
>  
>  phys_addr_t phys_alloc(phys_addr_t size)
>  {
> -	return phys_alloc_aligned(size, align_min);
> +	return phys_alloc_aligned(size, phys_alloc_align_min);
>  }
>  
>  phys_addr_t phys_zalloc(phys_addr_t size)
>  {
> -	return phys_zalloc_aligned(size, align_min);
> +	return phys_zalloc_aligned(size, phys_alloc_align_min);
>  }
>  
>  static void *early_malloc(size_t size)
>  {
> -	phys_addr_t addr = phys_alloc_aligned_safe(size, align_min, true);
> +	phys_addr_t addr = phys_alloc_aligned_safe(size,
> +					phys_alloc_align_min, true);
>  	if (addr == INVALID_PHYS_ADDR)
>  		return NULL;
>  
> @@ -158,7 +163,7 @@ static void *early_malloc(size_t size)
>  static void *early_calloc(size_t nmemb, size_t size)
>  {
>  	phys_addr_t addr = phys_zalloc_aligned_safe(nmemb * size,
> -						    align_min, true);
> +					phys_alloc_align_min, true);
>  	if (addr == INVALID_PHYS_ADDR)
>  		return NULL;
>  
> 

Re: [kvm-unit-tests PATCH v2 5/6] x86: lib/alloc: introduce alloc_zeroed_page

[Andrew Jones]

On Thu, Nov 03, 2016 at 01:02:03PM +0100, Laurent Vivier wrote:
> 
> 
> On 02/11/2016 21:52, Andrew Jones wrote:
> > This allows us to remove a bunch of memsets.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  lib/alloc.c     |  8 ++++++++
> >  lib/alloc.h     |  1 +
> >  lib/x86/vm.c    |  4 +---
> >  x86/vmx.c       | 19 +++++++------------
> >  x86/vmx_tests.c | 28 ++++++++++------------------
> >  5 files changed, 27 insertions(+), 33 deletions(-)
> > 
> > diff --git a/lib/alloc.c b/lib/alloc.c
> > index ce1198e2977f..d797690cc458 100644
> > --- a/lib/alloc.c
> > +++ b/lib/alloc.c
> > @@ -191,6 +191,14 @@ void *alloc_page(void)
> >  	return p;
> >  }
> >  
> > +void *alloc_zeroed_page(void)
> > +{
> > +	void *p = alloc_page();
> 
> alloc_page() can return NULL.
> 
> As most of the callers don't check the return value of
> alloc_zeroed_page(), I think you should assert() here.

Hmm... both returning NULL (which you're right I should have done here)
and asserting have their merits. If a unit test, for whatever reason,
wanted to allocate all memory, then it would impossible to know how many
alloc_page calls that would take before needing to stop to avoid the
assert. It's much easier to loop until NULL. However, most callers won't
do that, and, as you point out below, most callers are neglecting to
check for NULL.

So I think we need both. I propose renaming alloc_page to __alloc_page.
Unit tests that want to avoid the assert and check for NULL should use
that. But most callers should use a new alloc_page,

 void *alloc_page(void)
 {
     void *p = __alloc_page();
     assert(p);
     return p;
 }

alloc_zeroed_page will be built on alloc_page. __alloc_page users can
zero their own pages...

Thoughts on that?

> 
> 
> > +	memset(p, 0, PAGE_SIZE);
> > +	return p;
> > +}
> > +
> >  void free_page(void *page)
> >  {
> >  	spin_lock(&heap_lock);
> > diff --git a/lib/alloc.h b/lib/alloc.h
> > index a37330b3088a..f61a5200c829 100644
> > --- a/lib/alloc.h
> > +++ b/lib/alloc.h
> > @@ -126,6 +126,7 @@ extern void phys_alloc_show(void);
> >   */
> >  extern void heap_init(void *start, size_t size);
> >  extern void *alloc_page(void);
> > +extern void *alloc_zeroed_page(void);
> >  extern void free_page(void *page);
> >  
> >  #endif /* _ALLOC_H_ */
> > diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> > index 4e399f80dd31..8b95104ef80f 100644
> > --- a/lib/x86/vm.c
> > +++ b/lib/x86/vm.c
> > @@ -91,9 +91,7 @@ static void setup_mmu_range(unsigned long *cr3, unsigned long start,
> >  
> >  static void setup_mmu(unsigned long len)
> >  {
> > -    unsigned long *cr3 = alloc_page();
> > -
> > -    memset(cr3, 0, PAGE_SIZE);
> > +    unsigned long *cr3 = alloc_zeroed_page();
> >  
> >  #ifdef __x86_64__
> >      if (len < (1ul << 32))
> > diff --git a/x86/vmx.c b/x86/vmx.c
> > index 411ed3211d4d..5d333e077a02 100644
> > --- a/x86/vmx.c
> > +++ b/x86/vmx.c
> > @@ -29,6 +29,7 @@
> >   */
> >  
> >  #include "libcflat.h"
> > +#include "alloc.h"
> >  #include "processor.h"
> >  #include "vm.h"
> >  #include "desc.h"
> > @@ -276,9 +277,8 @@ static void split_large_ept_entry(unsigned long *ptep, int level)
> >  	assert(pte & EPT_LARGE_PAGE);
> >  	assert(level == 2 || level == 3);
> >  
> > -	new_pt = alloc_page();
> > +	new_pt = alloc_zeroed_page();
> >  	assert(new_pt);
> 
> If alloc_zeroed_page() uses assert(), this one is useless, otherwise add
> the same to all the other calls of alloc_zeroed_page() below.
> 
> > -	memset(new_pt, 0, PAGE_SIZE);
> >  
> >  	prototype = pte & ~EPT_ADDR_MASK;
> >  	if (level == 2)
> > @@ -617,8 +617,7 @@ static void init_vmcs_guest(void)
> ...
> 
> Thanks,
> Laurent
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH v2 5/6] x86: lib/alloc: introduce alloc_zeroed_page

[Laurent Vivier]

On 03/11/2016 13:47, Andrew Jones wrote:
> On Thu, Nov 03, 2016 at 01:02:03PM +0100, Laurent Vivier wrote:
>>
>>
>> On 02/11/2016 21:52, Andrew Jones wrote:
>>> This allows us to remove a bunch of memsets.
>>>
>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>>  lib/alloc.c     |  8 ++++++++
>>>  lib/alloc.h     |  1 +
>>>  lib/x86/vm.c    |  4 +---
>>>  x86/vmx.c       | 19 +++++++------------
>>>  x86/vmx_tests.c | 28 ++++++++++------------------
>>>  5 files changed, 27 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/lib/alloc.c b/lib/alloc.c
>>> index ce1198e2977f..d797690cc458 100644
>>> --- a/lib/alloc.c
>>> +++ b/lib/alloc.c
>>> @@ -191,6 +191,14 @@ void *alloc_page(void)
>>>  	return p;
>>>  }
>>>  
>>> +void *alloc_zeroed_page(void)
>>> +{
>>> +	void *p = alloc_page();
>>
>> alloc_page() can return NULL.
>>
>> As most of the callers don't check the return value of
>> alloc_zeroed_page(), I think you should assert() here.
> 
> Hmm... both returning NULL (which you're right I should have done here)
> and asserting have their merits. If a unit test, for whatever reason,
> wanted to allocate all memory, then it would impossible to know how many
> alloc_page calls that would take before needing to stop to avoid the
> assert. It's much easier to loop until NULL. However, most callers won't
> do that, and, as you point out below, most callers are neglecting to
> check for NULL.
> 
> So I think we need both. I propose renaming alloc_page to __alloc_page.
> Unit tests that want to avoid the assert and check for NULL should use
> that. But most callers should use a new alloc_page,
> 
>  void *alloc_page(void)
>  {
>      void *p = __alloc_page();
>      assert(p);
>      return p;
>  }
> 
> alloc_zeroed_page will be built on alloc_page. __alloc_page users can
> zero their own pages...
> 
> Thoughts on that?

It's nice to have both. Good idea.

Thanks,
Laurent

Re: [kvm-unit-tests PATCH v2 4/6] x86: lib/alloc: move heap management to lib

[Laurent Vivier]

On 02/11/2016 21:52, Andrew Jones wrote:
> This will allow other arches to use {alloc,free}_page.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/alloc.c         | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/alloc.h         | 10 ++++++++++
>  lib/x86/vm.c        | 48 ++----------------------------------------------
>  x86/Makefile.common |  1 +
>  4 files changed, 62 insertions(+), 46 deletions(-)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index 1d990a803825..ce1198e2977f 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -5,6 +5,7 @@
>   */
>  #include "alloc.h"
>  #include "asm/spinlock.h"
> +#include "asm/page.h"
>  #include "asm/io.h"
>  
>  #define MIN(a, b)		((a) < (b) ? (a) : (b))
> @@ -150,6 +151,54 @@ phys_addr_t phys_zalloc(phys_addr_t size)
>  	return phys_zalloc_aligned(size, phys_alloc_align_min);
>  }
>  
> +static struct spinlock heap_lock;
> +static void *heap_free_head;
> +
> +void heap_init(void *start, size_t size)
> +{
> +	void *p = start;

why do you introduce "p"? It's not obvious for me...

> +
> +	assert(!((unsigned long)start & ~PAGE_MASK));
> +
> +	spin_lock(&heap_lock);
> +
> +	heap_free_head = NULL;
> +
> +	while (size >= PAGE_SIZE) {
> +		*(void **)p = heap_free_head;
> +		heap_free_head = p;
> +		p += PAGE_SIZE;
> +		size -= PAGE_SIZE;
> +	}
> +
> +	spin_unlock(&heap_lock);
> +}
> +
> +void *alloc_page(void)
> +{
> +	void *p;
> +
> +	spin_lock(&heap_lock);
> +
> +	if (!heap_free_head)
> +		return NULL;

missing unlock propagated from PATCH 1

Laurent

Re: [kvm-unit-tests PATCH v2 4/6] x86: lib/alloc: move heap management to lib

[Andrew Jones]

On Thu, Nov 03, 2016 at 02:28:15PM +0100, Laurent Vivier wrote:
> 
> 
> On 02/11/2016 21:52, Andrew Jones wrote:
> > This will allow other arches to use {alloc,free}_page.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  lib/alloc.c         | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/alloc.h         | 10 ++++++++++
> >  lib/x86/vm.c        | 48 ++----------------------------------------------
> >  x86/Makefile.common |  1 +
> >  4 files changed, 62 insertions(+), 46 deletions(-)
> > 
> > diff --git a/lib/alloc.c b/lib/alloc.c
> > index 1d990a803825..ce1198e2977f 100644
> > --- a/lib/alloc.c
> > +++ b/lib/alloc.c
> > @@ -5,6 +5,7 @@
> >   */
> >  #include "alloc.h"
> >  #include "asm/spinlock.h"
> > +#include "asm/page.h"
> >  #include "asm/io.h"
> >  
> >  #define MIN(a, b)		((a) < (b) ? (a) : (b))
> > @@ -150,6 +151,54 @@ phys_addr_t phys_zalloc(phys_addr_t size)
> >  	return phys_zalloc_aligned(size, phys_alloc_align_min);
> >  }
> >  
> > +static struct spinlock heap_lock;
> > +static void *heap_free_head;
> > +
> > +void heap_init(void *start, size_t size)
> > +{
> > +	void *p = start;
> 
> why do you introduce "p"? It's not obvious for me...

Just naming. I prefer the input to be named start,
but the iterator to be named p.

> 
> > +
> > +	assert(!((unsigned long)start & ~PAGE_MASK));
> > +
> > +	spin_lock(&heap_lock);
> > +
> > +	heap_free_head = NULL;
> > +
> > +	while (size >= PAGE_SIZE) {
> > +		*(void **)p = heap_free_head;
> > +		heap_free_head = p;
> > +		p += PAGE_SIZE;
> > +		size -= PAGE_SIZE;
> > +	}
> > +
> > +	spin_unlock(&heap_lock);
> > +}
> > +
> > +void *alloc_page(void)
> > +{
> > +	void *p;
> > +
> > +	spin_lock(&heap_lock);
> > +
> > +	if (!heap_free_head)
> > +		return NULL;
> 
> missing unlock propagated from PATCH 1

Yup. Already fixed for v3

Thanks,
drew

> 
> Laurent
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Paolo Bonzini]

On 02/11/2016 21:52, Andrew Jones wrote:
> We've had malloc, calloc, free, and memalign available for quite a
> while, and arm and powerpc make use of them. x86 hasn't yet, which
> is fine, but could lead to problems if common lib code wants to make
> use of them. While arm and powerpc currently use early_alloc_ops,
> built on phys_alloc, x86 does not initialize phys_alloc and has its
> own memory management, vmalloc. This patch enables vmalloc to be
> used as the underlying allocator, but there are two drawbacks:
> 
>  1) Every allocation will allocate at least one page.
>  2) It can only be used with the MMU enabled.
> 
> Drawback (1) is probably fine for kvm-unit-tests. Drawback (2) may
> not be, as common lib code may get invoked at any time by any unit
> test, including ones that don't have the MMU enabled. However, as
> we only switch alloc_ops to the vmalloc based implementations in
> setup_vm, where the MMU is enabled, then they'll be safe to use for
> any unit test that invokes setup_vm first. If the unit test does
> not invoke setup_vm first then alloc_ops will use the default
> implementations, the ones based on phys_alloc, and will result
> in asserts firing if phys_alloc_init has not be called first.
> 
> While this patch may not enable anything right now, I think it
> makes sense to enable these alloc ops before they're needed, because
> then everything will most likely just work when a future common lib
> function that uses malloc is introduced. If that common function
> results in a unit test hitting an assert, then the test writer will
> just need to decide if they want to use phys_alloc or vmalloc and
> call the appropriate init function first.

So it seems to me that phys_alloc should be implemented _on top_ of
virtual memory.  Basically, the alloc_ops should not be
malloc/calloc/free/memalign.  The current
early_malloc/early_calloc/early_free/early_memalign should just be
renamed to malloc/calloc/free/memalign.

Instead, you only need an op which is

	void *morecore(unsigned long size);

The default implementation is basically "base += size; return base -
size;".  When you turn on virtual memory (hopefully, that's before any
allocation!) map_core is changed to point to a function that calls
install_page to fetch memory if needed.

Assuming lib/x86/vm.c is changed to allocate upwards (not downwards),
something like this would do:

	addr = malloc_free;
	available = malloc_top - addr;
	if (available < size) {
		if (vfree_bottom == malloc_top + 1) {
			// can reuse top of previous segment
			vsize = PAGE_ALIGN(size - available);
			vaddr = alloc_vpages(vsize >> PAGE_SHIFT);
			assert(vaddr == malloc_top + 1);
		} else {
			// ignore free space at end of previous segment
			vsize = PAGE_ALIGN(size);
			addr = vaddr =
				alloc_vpages(vsize >> PAGE_SHIFT);
		}
		malloc_top = vaddr + vsize - 1;
	}
	malloc_free = addr + size;
	return addr;

Paolo

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Andrew Jones]

On Thu, Nov 03, 2016 at 03:12:15PM +0100, Paolo Bonzini wrote:
> 
> 
> On 02/11/2016 21:52, Andrew Jones wrote:
> > We've had malloc, calloc, free, and memalign available for quite a
> > while, and arm and powerpc make use of them. x86 hasn't yet, which
> > is fine, but could lead to problems if common lib code wants to make
> > use of them. While arm and powerpc currently use early_alloc_ops,
> > built on phys_alloc, x86 does not initialize phys_alloc and has its
> > own memory management, vmalloc. This patch enables vmalloc to be
> > used as the underlying allocator, but there are two drawbacks:
> > 
> >  1) Every allocation will allocate at least one page.
> >  2) It can only be used with the MMU enabled.
> > 
> > Drawback (1) is probably fine for kvm-unit-tests. Drawback (2) may
> > not be, as common lib code may get invoked at any time by any unit
> > test, including ones that don't have the MMU enabled. However, as
> > we only switch alloc_ops to the vmalloc based implementations in
> > setup_vm, where the MMU is enabled, then they'll be safe to use for
> > any unit test that invokes setup_vm first. If the unit test does
> > not invoke setup_vm first then alloc_ops will use the default
> > implementations, the ones based on phys_alloc, and will result
> > in asserts firing if phys_alloc_init has not be called first.
> > 
> > While this patch may not enable anything right now, I think it
> > makes sense to enable these alloc ops before they're needed, because
> > then everything will most likely just work when a future common lib
> > function that uses malloc is introduced. If that common function
> > results in a unit test hitting an assert, then the test writer will
> > just need to decide if they want to use phys_alloc or vmalloc and
> > call the appropriate init function first.
> 
> So it seems to me that phys_alloc should be implemented _on top_ of
> virtual memory.

Actually I have it envisioned the other way around. phys_alloc should
only be for early allocations, before enabling the MMU (which is why
it's called "phys"_alloc). The only reason phys_alloc is still being
used for ARM after enabling the MMU is because, thanks to using an
identity map, it doesn't really matter. The only drawback of using it
is that I didn't want it to be too complicated, so I didn't implement
a way to free memory. If at some point we want a region of memory that
isn't identity mapped, and/or can also be freed, then we'll need a new
allocator. That new allocator would get its memory from a phys_alloc
allocation though. For example, we can now use the alloc/free_page
allocator like this

 phys_alloc_init(phys_base, total_size);
 heap_phys_base = phys_alloc_aligned(heap_size, PAGE_SIZE);
 mmu_enable(pgtable);
 mmu_set_range_ptes(pgtable, heap_virt_base, heap_phys_base,
                    heap_phys_base + heap_size, prot);
 heap_init(heap_virt_base, heap_size);
 alloc_ops = &heap_based_ops;

> Basically, the alloc_ops should not be
> malloc/calloc/free/memalign.  The current
> early_malloc/early_calloc/early_free/early_memalign should just be
> renamed to malloc/calloc/free/memalign.
> 
> Instead, you only need an op which is
> 
> 	void *morecore(unsigned long size);

It makes sense to try to reduce the number of ops we have. Certainly
malloc/calloc/memalign could all be based on a single op (they're
already all based on phys_alloc_aligned_safe). We still need one more
though for free(). I can fix that up, i.e. change alloc_ops to only
an alloc and a free. early_alloc will be phys_alloc_aligned_safe and
early_free will still just be {}.

> 
> The default implementation is basically "base += size; return base -
> size;".  When you turn on virtual memory (hopefully, that's before any
> allocation!) map_core is changed to point to a function that calls
> install_page to fetch memory if needed.

Yes, this is the idea we have today; before MMU, alloc_ops point to
phys_alloc (the simple default imp. you describe). After MMU, we can
remap alloc_ops to point to a new implementation (as I show above).

> 
> Assuming lib/x86/vm.c is changed to allocate upwards (not downwards),
> something like this would do:
> 
> 	addr = malloc_free;
> 	available = malloc_top - addr;
> 	if (available < size) {
> 		if (vfree_bottom == malloc_top + 1) {
> 			// can reuse top of previous segment
> 			vsize = PAGE_ALIGN(size - available);
> 			vaddr = alloc_vpages(vsize >> PAGE_SHIFT);
> 			assert(vaddr == malloc_top + 1);
> 		} else {
> 			// ignore free space at end of previous segment
> 			vsize = PAGE_ALIGN(size);
> 			addr = vaddr =
> 				alloc_vpages(vsize >> PAGE_SHIFT);
> 		}
> 		malloc_top = vaddr + vsize - 1;
> 	}
> 	malloc_free = addr + size;
> 	return addr;

I agree lib/x86/vm.c needs to change it's virtual address allocation
scheme. It allocates down and never allows an address to be reused
(but maybe that's by design to avoid TLB flushes?) I didn't feel like
tackling that right now though. I'm not sure what the function you've
written should be used for. To me, it looks like it's an extension of
the virtual address management already available. Where do alloc_page()
and install_page() come in, like vmalloc has?

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Paolo Bonzini]

On 03/11/2016 15:58, Andrew Jones wrote:
> Actually I have it envisioned the other way around. phys_alloc should
> only be for early allocations, before enabling the MMU (which is why
> it's called "phys"_alloc).

Yes, I agree.  Putting your other suggestions together, you'd have for
early allocation:

   phys        // provide contiguous physical addresses
    |
   morecore    // provide contiguous RAM
    |
   malloc      // full API

Later, you'd switch morecore to also take care of installing PTEs:

    page     virt
      \       /
      morecore
          |
       malloc

Now morecore talks to both page and virt.  page (alloc_page) is
initialized with whatever memory wasn't allocated by phys, and takes up
its job.  virt (alloc_vpages) allocates virtual address space.  morecore
is now taking not necessarily consecutive physical memory at page
granularity, assigning consecutive virtual address to it, and if needed
slicing pages into smaller allocations.

morecore is not an external API, it's just malloc's pluggable interface.
 page/virt/malloc are all external APIs.  phys could be, but nothing
uses phys_alloc_aligned and phys_zalloc_aligned so we might as well zap it.

Note that you can choose separately at each layer whether to implement
freeing or not.  I think we only need to have it in "page" and "malloc"
(though in the latter it can be dummy, as it is now, since "malloc" is
just a small veneer around morecore).

>> 	addr = malloc_free;
>> 	available = malloc_top - addr;
>> 	if (available < size) {
>> 		if (vfree_bottom == malloc_top + 1) {
>> 			// can reuse top of previous segment
>> 			vsize = PAGE_ALIGN(size - available);
>> 			vaddr = alloc_vpages(vsize >> PAGE_SHIFT);
>> 			assert(vaddr == malloc_top + 1);
>> 		} else {
>> 			// ignore free space at end of previous segment
>> 			vsize = PAGE_ALIGN(size);
>> 			addr = vaddr =
>> 				alloc_vpages(vsize >> PAGE_SHIFT);
>> 		}
>> 		malloc_top = vaddr + vsize - 1;
>> 	}
>> 	malloc_free = addr + size;
>> 	return addr;
> 
> I agree lib/x86/vm.c needs to change it's virtual address allocation
> scheme. It allocates down and never allows an address to be reused
> (but maybe that's by design to avoid TLB flushes?) I didn't feel like
> tackling that right now though. I'm not sure what the function you've
> written should be used for. To me, it looks like it's an extension of
> the virtual address management already available. Where do alloc_page()
> and install_page() come in, like vmalloc has?

The alloc_page()/install_page() loop would be before setting malloc_top.

Paolo

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Andrew Jones]

On Thu, Nov 03, 2016 at 05:00:01PM +0100, Paolo Bonzini wrote:
> 
> 
> On 03/11/2016 15:58, Andrew Jones wrote:
> > Actually I have it envisioned the other way around. phys_alloc should
> > only be for early allocations, before enabling the MMU (which is why
> > it's called "phys"_alloc).
> 
> Yes, I agree.  Putting your other suggestions together, you'd have for
> early allocation:
> 
>    phys        // provide contiguous physical addresses
>     |
>    morecore    // provide contiguous RAM
>     |
>    malloc      // full API
> 
> Later, you'd switch morecore to also take care of installing PTEs:
> 
>     page     virt
>       \       /
>       morecore
>           |
>        malloc

Yup, I think we're on the same page (no pun intended). However, as
morecore needs to take both size and alignment arguments in order
to be able to cope with any offsets that a memalign call would create,
then I was planning to just keep it named early_memalign.

> 
> Now morecore talks to both page and virt.  page (alloc_page) is
> initialized with whatever memory wasn't allocated by phys, and takes up
> its job.  virt (alloc_vpages) allocates virtual address space.  morecore
> is now taking not necessarily consecutive physical memory at page
> granularity, assigning consecutive virtual address to it, and if needed
> slicing pages into smaller allocations.

vmalloc does all that except the slicing up feature, which is left for
a rainy day... So I think the implementation I have here for vmemalign
is still valid for x86's "morecore".

> 
> morecore is not an external API, it's just malloc's pluggable interface.
>  page/virt/malloc are all external APIs.  phys could be, but nothing

Right, both vmemalign and early_memalign are internal only (static) and
alloc_page and alloc_vpages and malloc/calloc/memalign/free are external.
x86 also has vmalloc/vfree (the building block for vmemalign) which allow
callers to explicitly ask for virtual addresses corresponding to a new
allocation. We should keep that external because malloc doesn't imply
you're getting PTEs set, as it depends on the stage/degree of test init.

> uses phys_alloc_aligned and phys_zalloc_aligned so we might as well zap it.

These two are useful for finding/getting the "whatever memory wasn't
allocated by phys" you state above. To do that, you simple allocate one
more region containing the amount of memory you want the new allocator
to have with one of them. I'll zap phys_alloc and phys_zalloc though.

> 
> Note that you can choose separately at each layer whether to implement
> freeing or not.  I think we only need to have it in "page" and "malloc"
> (though in the latter it can be dummy, as it is now, since "malloc" is
> just a small veneer around morecore).

Yup, check.

> 
> >> 	addr = malloc_free;
> >> 	available = malloc_top - addr;
> >> 	if (available < size) {
> >> 		if (vfree_bottom == malloc_top + 1) {
> >> 			// can reuse top of previous segment
> >> 			vsize = PAGE_ALIGN(size - available);
> >> 			vaddr = alloc_vpages(vsize >> PAGE_SHIFT);
> >> 			assert(vaddr == malloc_top + 1);
> >> 		} else {
> >> 			// ignore free space at end of previous segment
> >> 			vsize = PAGE_ALIGN(size);
> >> 			addr = vaddr =
> >> 				alloc_vpages(vsize >> PAGE_SHIFT);
> >> 		}
> >> 		malloc_top = vaddr + vsize - 1;
> >> 	}
> >> 	malloc_free = addr + size;
> >> 	return addr;
> > 
> > I agree lib/x86/vm.c needs to change it's virtual address allocation
> > scheme. It allocates down and never allows an address to be reused
> > (but maybe that's by design to avoid TLB flushes?) I didn't feel like
> > tackling that right now though. I'm not sure what the function you've
> > written should be used for. To me, it looks like it's an extension of
> > the virtual address management already available. Where do alloc_page()
> > and install_page() come in, like vmalloc has?
> 
> The alloc_page()/install_page() loop would be before setting malloc_top.

Ok, so if we don't want to rework vmalloc/alloc_vpages for this series
to use a different virtual address allocation scheme (I don't ;-), then
I think this part can be left for another time.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 4/6] x86: lib/alloc: move heap management to lib

[Paolo Bonzini]

On 02/11/2016 21:52, Andrew Jones wrote:
> This will allow other arches to use {alloc,free}_page.

Let's make a new file lib/pagealloc.c.  This could potentially grow to
something like a buddy allocator.

Paolo

> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/alloc.c         | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/alloc.h         | 10 ++++++++++
>  lib/x86/vm.c        | 48 ++----------------------------------------------
>  x86/Makefile.common |  1 +
>  4 files changed, 62 insertions(+), 46 deletions(-)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index 1d990a803825..ce1198e2977f 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -5,6 +5,7 @@
>   */
>  #include "alloc.h"
>  #include "asm/spinlock.h"
> +#include "asm/page.h"
>  #include "asm/io.h"
>  
>  #define MIN(a, b)		((a) < (b) ? (a) : (b))
> @@ -150,6 +151,54 @@ phys_addr_t phys_zalloc(phys_addr_t size)
>  	return phys_zalloc_aligned(size, phys_alloc_align_min);
>  }
>  
> +static struct spinlock heap_lock;
> +static void *heap_free_head;
> +
> +void heap_init(void *start, size_t size)
> +{
> +	void *p = start;
> +
> +	assert(!((unsigned long)start & ~PAGE_MASK));
> +
> +	spin_lock(&heap_lock);
> +
> +	heap_free_head = NULL;
> +
> +	while (size >= PAGE_SIZE) {
> +		*(void **)p = heap_free_head;
> +		heap_free_head = p;
> +		p += PAGE_SIZE;
> +		size -= PAGE_SIZE;
> +	}
> +
> +	spin_unlock(&heap_lock);
> +}
> +
> +void *alloc_page(void)
> +{
> +	void *p;
> +
> +	spin_lock(&heap_lock);
> +
> +	if (!heap_free_head)
> +		return NULL;
> +
> +	p = heap_free_head;
> +	heap_free_head = *(void **)heap_free_head;
> +
> +	spin_unlock(&heap_lock);
> +
> +	return p;
> +}
> +
> +void free_page(void *page)
> +{
> +	spin_lock(&heap_lock);
> +	*(void **)page = heap_free_head;
> +	heap_free_head = page;
> +	spin_unlock(&heap_lock);
> +}
> +
>  static void *early_malloc(size_t size)
>  {
>  	phys_addr_t addr = phys_alloc_aligned_safe(size,
> diff --git a/lib/alloc.h b/lib/alloc.h
> index bd3c4e8ff3f6..a37330b3088a 100644
> --- a/lib/alloc.h
> +++ b/lib/alloc.h
> @@ -118,4 +118,14 @@ extern phys_addr_t phys_zalloc(phys_addr_t size);
>   */
>  extern void phys_alloc_show(void);
>  
> +/*
> + * Heap page allocator
> + *
> + * After initializing with heap_init, {alloc,free}_page can be used
> + * to easily manage pages.
> + */
> +extern void heap_init(void *start, size_t size);
> +extern void *alloc_page(void);
> +extern void free_page(void *page);
> +
>  #endif /* _ALLOC_H_ */
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index baea17e7f475..4e399f80dd31 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -1,56 +1,12 @@
>  #include "fwcfg.h"
>  #include "vm.h"
>  #include "libcflat.h"
> +#include "alloc.h"
>  #include "asm/spinlock.h"
>  
> -static struct spinlock heap_lock;
>  static struct spinlock vm_lock;
> -static void *free = 0;
>  static void *vfree_top = 0;
>  
> -static void free_memory(void *mem, unsigned long size)
> -{
> -    assert(!((unsigned long)mem & ~PAGE_MASK));
> -
> -    spin_lock(&heap_lock);
> -
> -    free = NULL;
> -
> -    while (size >= PAGE_SIZE) {
> -	*(void **)mem = free;
> -	free = mem;
> -	mem += PAGE_SIZE;
> -	size -= PAGE_SIZE;
> -    }
> -
> -    spin_unlock(&heap_lock);
> -}
> -
> -void *alloc_page()
> -{
> -    void *p;
> -
> -    spin_lock(&heap_lock);
> -
> -    if (!free)
> -	return NULL;
> -
> -    p = free;
> -    free = *(void **)free;
> -
> -    spin_unlock(&heap_lock);
> -
> -    return p;
> -}
> -
> -void free_page(void *page)
> -{
> -    spin_lock(&heap_lock);
> -    *(void **)page = free;
> -    free = page;
> -    spin_unlock(&heap_lock);
> -}
> -
>  extern char edata;
>  static unsigned long end_of_memory;
>  
> @@ -170,7 +126,7 @@ void setup_vm()
>  {
>      assert(!end_of_memory);
>      end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
> -    free_memory(&edata, end_of_memory - (unsigned long)&edata);
> +    heap_init(&edata, end_of_memory - (unsigned long)&edata);
>      setup_mmu(end_of_memory);
>  }
>  
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 356d879a986b..88038e41af60 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -3,6 +3,7 @@
>  all: test_cases
>  
>  cflatobjs += lib/pci.o
> +cflatobjs += lib/alloc.o
>  cflatobjs += lib/x86/io.o
>  cflatobjs += lib/x86/smp.o
>  cflatobjs += lib/x86/vm.o
> 

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Paolo Bonzini]

On 03/11/2016 17:51, Andrew Jones wrote:
> On Thu, Nov 03, 2016 at 05:00:01PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 03/11/2016 15:58, Andrew Jones wrote:
>>> Actually I have it envisioned the other way around. phys_alloc should
>>> only be for early allocations, before enabling the MMU (which is why
>>> it's called "phys"_alloc).
>>
>> Yes, I agree.  Putting your other suggestions together, you'd have for
>> early allocation:
>>
>>    phys        // provide contiguous physical addresses
>>     |
>>    morecore    // provide contiguous RAM
>>     |
>>    malloc      // full API
>>
>> Later, you'd switch morecore to also take care of installing PTEs:
>>
>>     page     virt
>>       \       /
>>       morecore
>>           |
>>        malloc
> 
> Yup, I think we're on the same page (no pun intended). However, as
> morecore needs to take both size and alignment arguments in order
> to be able to cope with any offsets that a memalign call would create,
> then I was planning to just keep it named early_memalign.

You're right, it needs alignment too.  So yeah, early_memalign's code
can be repurposed as the early allocator's morecore callback.  For the
current trivial malloc implementation, malloc would be implemented as
ops->morecore(size, align_min).  I think we agree on the overall idea
and on having only one function pointer.

>> Now morecore talks to both page and virt.  page (alloc_page) is
>> initialized with whatever memory wasn't allocated by phys, and takes up
>> its job.  virt (alloc_vpages) allocates virtual address space.  morecore
>> is now taking not necessarily consecutive physical memory at page
>> granularity, assigning consecutive virtual address to it, and if needed
>> slicing pages into smaller allocations.
> 
> vmalloc does all that except the slicing up feature, which is left for
> a rainy day... So I think the implementation I have here for vmemalign
> is still valid for x86's "morecore".

Fair enough, the slicing can come later.  Can you do v3 that (on top of
what you have in this series):

1) changes alloc_ops to only have the .memalign callback, with fixed
implementations of malloc/memalign/free functions;

2) makes vmalloc&vmap use alloc_vpages (i.e. use the "virt" allocator);

3) make alloc_vpages go bottom to top (not strictly necessary but nice);

4) replaces vmalloc/vfree with memalign(align=4096)/free in x86/;

5) moves the virt allocator and the vmalloc ops to a new file
lib/vmalloc.c (with install_page() remaining as the sole hook into
arch-specific code), removing what's now dead from lib/vmalloc.c.

I think your current patches fit between (3) and (4), since they end
with the vmemalign function and the ops struct in lib/x86/vm.c.

Thanks,

Paolo

>> morecore is not an external API, it's just malloc's pluggable interface.
>>  page/virt/malloc are all external APIs.  phys could be, but nothing
> 
> Right, both vmemalign and early_memalign are internal only (static) and
> alloc_page and alloc_vpages and malloc/calloc/memalign/free are external.
> x86 also has vmalloc/vfree (the building block for vmemalign) which allow
> callers to explicitly ask for virtual addresses corresponding to a new
> allocation. We should keep that external because malloc doesn't imply
> you're getting PTEs set, as it depends on the stage/degree of test init.
> 
>> uses phys_alloc_aligned and phys_zalloc_aligned so we might as well zap it.
> 
> These two are useful for finding/getting the "whatever memory wasn't
> allocated by phys" you state above. To do that, you simple allocate one
> more region containing the amount of memory you want the new allocator
> to have with one of them. I'll zap phys_alloc and phys_zalloc though.
> 
>>
>> Note that you can choose separately at each layer whether to implement
>> freeing or not.  I think we only need to have it in "page" and "malloc"
>> (though in the latter it can be dummy, as it is now, since "malloc" is
>> just a small veneer around morecore).
> 
> Yup, check.
> 
>>
>>>> 	addr = malloc_free;
>>>> 	available = malloc_top - addr;
>>>> 	if (available < size) {
>>>> 		if (vfree_bottom == malloc_top + 1) {
>>>> 			// can reuse top of previous segment
>>>> 			vsize = PAGE_ALIGN(size - available);
>>>> 			vaddr = alloc_vpages(vsize >> PAGE_SHIFT);
>>>> 			assert(vaddr == malloc_top + 1);
>>>> 		} else {
>>>> 			// ignore free space at end of previous segment
>>>> 			vsize = PAGE_ALIGN(size);
>>>> 			addr = vaddr =
>>>> 				alloc_vpages(vsize >> PAGE_SHIFT);
>>>> 		}
>>>> 		malloc_top = vaddr + vsize - 1;
>>>> 	}
>>>> 	malloc_free = addr + size;
>>>> 	return addr;
>>>
>>> I agree lib/x86/vm.c needs to change it's virtual address allocation
>>> scheme. It allocates down and never allows an address to be reused
>>> (but maybe that's by design to avoid TLB flushes?) I didn't feel like
>>> tackling that right now though. I'm not sure what the function you've
>>> written should be used for. To me, it looks like it's an extension of
>>> the virtual address management already available. Where do alloc_page()
>>> and install_page() come in, like vmalloc has?
>>
>> The alloc_page()/install_page() loop would be before setting malloc_top.
> 
> Ok, so if we don't want to rework vmalloc/alloc_vpages for this series
> to use a different virtual address allocation scheme (I don't ;-), then
> I think this part can be left for another time.
> 
> Thanks,
> drew
> 

Re: [kvm-unit-tests PATCH v2 4/6] x86: lib/alloc: move heap management to lib

[Andrew Jones]

On Thu, Nov 03, 2016 at 06:21:37PM +0100, Paolo Bonzini wrote:
> 
> 
> On 02/11/2016 21:52, Andrew Jones wrote:
> > This will allow other arches to use {alloc,free}_page.
> 
> Let's make a new file lib/pagealloc.c.  This could potentially grow to
> something like a buddy allocator.

I'd be fine with the buddy allocator getting thrown in alloc.c too,
but also don't mind more files. Will do for v3.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Andrew Jones]

On Thu, Nov 03, 2016 at 06:34:45PM +0100, Paolo Bonzini wrote:
> You're right, it needs alignment too.  So yeah, early_memalign's code
> can be repurposed as the early allocator's morecore callback.  For the
> current trivial malloc implementation, malloc would be implemented as
> ops->morecore(size, align_min).  I think we agree on the overall idea
> and on having only one function pointer.

Yup, already done. But 2, not 1, pointers. Still need 'free' too.

> 
> >> Now morecore talks to both page and virt.  page (alloc_page) is
> >> initialized with whatever memory wasn't allocated by phys, and takes up
> >> its job.  virt (alloc_vpages) allocates virtual address space.  morecore
> >> is now taking not necessarily consecutive physical memory at page
> >> granularity, assigning consecutive virtual address to it, and if needed
> >> slicing pages into smaller allocations.
> > 
> > vmalloc does all that except the slicing up feature, which is left for
> > a rainy day... So I think the implementation I have here for vmemalign
> > is still valid for x86's "morecore".
> 
> Fair enough, the slicing can come later.  Can you do v3 that (on top of
> what you have in this series):
> 
> 1) changes alloc_ops to only have the .memalign callback, with fixed
> implementations of malloc/memalign/free functions;

done

> 
> 2) makes vmalloc&vmap use alloc_vpages (i.e. use the "virt" allocator);

OK

> 
> 3) make alloc_vpages go bottom to top (not strictly necessary but nice);

OK, from what base though? 0x8, i.e. just skip NULL?

> 
> 4) replaces vmalloc/vfree with memalign(align=4096)/free in x86/;

I thought about this already, but I wasn't sure we wanted to abandon
the explicit "I want virt mem" vmalloc naming. memalign/malloc will
be vmalloc for any unit test that does setup_vm, but otherwise (if
x86 adopted phys_alloc) it wouldn't. Maybe x86 should just never
adopt phys_alloc, leading to asserts when setup_vm hasn't been
called?

> 
> 5) moves the virt allocator and the vmalloc ops to a new file
> lib/vmalloc.c (with install_page() remaining as the sole hook into
> arch-specific code), removing what's now dead from lib/vmalloc.c.

Yeah, this is a good next step. Will do.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Paolo Bonzini]

On 03/11/2016 19:12, Andrew Jones wrote:
> On Thu, Nov 03, 2016 at 06:34:45PM +0100, Paolo Bonzini wrote:
>> You're right, it needs alignment too.  So yeah, early_memalign's code
>> can be repurposed as the early allocator's morecore callback.  For the
>> current trivial malloc implementation, malloc would be implemented as
>> ops->morecore(size, align_min).  I think we agree on the overall idea
>> and on having only one function pointer.
> 
> Yup, already done. But 2, not 1, pointers. Still need 'free' too.

Didn't we say that the morecore allocator would only grow?

>> 3) make alloc_vpages go bottom to top (not strictly necessary but nice);
> 
> OK, from what base though? 0x8, i.e. just skip NULL?

Well, the right one would be "after the last thing that phys allocated".
 For x86 however you'd have to init the phys allocator to edata.  But
let's leave this for later.

>>
>> 4) replaces vmalloc/vfree with memalign(align=4096)/free in x86/;
> 
> I thought about this already, but I wasn't sure we wanted to abandon
> the explicit "I want virt mem" vmalloc naming. memalign/malloc will
> be vmalloc for any unit test that does setup_vm, but otherwise (if
> x86 adopted phys_alloc) it wouldn't. Maybe x86 should just never
> adopt phys_alloc, leading to asserts when setup_vm hasn't been
> called?

I don't think the tests want virtual memory specifically.  If they want,
they use things like vmap.

Paolo

>>
>> 5) moves the virt allocator and the vmalloc ops to a new file
>> lib/vmalloc.c (with install_page() remaining as the sole hook into
>> arch-specific code), removing what's now dead from lib/vmalloc.c.
> 
> Yeah, this is a good next step. Will do.
> 
> Thanks,
> drew
> 

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Andrew Jones]

On Thu, Nov 03, 2016 at 07:20:20PM +0100, Paolo Bonzini wrote:
> 
> 
> On 03/11/2016 19:12, Andrew Jones wrote:
> > On Thu, Nov 03, 2016 at 06:34:45PM +0100, Paolo Bonzini wrote:
> >> You're right, it needs alignment too.  So yeah, early_memalign's code
> >> can be repurposed as the early allocator's morecore callback.  For the
> >> current trivial malloc implementation, malloc would be implemented as
> >> ops->morecore(size, align_min).  I think we agree on the overall idea
> >> and on having only one function pointer.
> > 
> > Yup, already done. But 2, not 1, pointers. Still need 'free' too.
> 
> Didn't we say that the morecore allocator would only grow?

morecore[phys_alloc] only grows, free -> {}
morecore[vmemalign] can do both, free -> vfree

> 
> >> 3) make alloc_vpages go bottom to top (not strictly necessary but nice);
> > 
> > OK, from what base though? 0x8, i.e. just skip NULL?
> 
> Well, the right one would be "after the last thing that phys allocated".
>  For x86 however you'd have to init the phys allocator to edata.  But
> let's leave this for later.

Hmm, I see what's going on now in x86. First, setup_vm should be doing
setup_mmu *before* 'free_memory' (what I renamed to heap_init), because
it assumes it's using virtual address with page_alloc/free. Second, x86
creates an identity map up to end_of_memory, and that's the last address
the heap knows about too. So, if we want to allocate free virtual
addresses upwards, then we should start from end_of_memory.

> 
> >>
> >> 4) replaces vmalloc/vfree with memalign(align=4096)/free in x86/;
> > 
> > I thought about this already, but I wasn't sure we wanted to abandon
> > the explicit "I want virt mem" vmalloc naming. memalign/malloc will
> > be vmalloc for any unit test that does setup_vm, but otherwise (if
> > x86 adopted phys_alloc) it wouldn't. Maybe x86 should just never
> > adopt phys_alloc, leading to asserts when setup_vm hasn't been
> > called?
> 
> I don't think the tests want virtual memory specifically.  If they want,
> they use things like vmap.

OK

> 
> Paolo
> 
> >>
> >> 5) moves the virt allocator and the vmalloc ops to a new file
> >> lib/vmalloc.c (with install_page() remaining as the sole hook into
> >> arch-specific code), removing what's now dead from lib/vmalloc.c.
> > 
> > Yeah, this is a good next step. Will do.

This one looks like we'll need to cleanup the install_page parameter
expectations a bit in order to keep arch-specific stuff out of the callers,
but that should be doable.

drew

Re: [kvm-unit-tests PATCH v2 6/6] lib/x86/vm: enable malloc and friends

[Paolo Bonzini]

On 03/11/2016 19:42, Andrew Jones wrote:
> On Thu, Nov 03, 2016 at 07:20:20PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 03/11/2016 19:12, Andrew Jones wrote:
>>> On Thu, Nov 03, 2016 at 06:34:45PM +0100, Paolo Bonzini wrote:
>>>> You're right, it needs alignment too.  So yeah, early_memalign's code
>>>> can be repurposed as the early allocator's morecore callback.  For the
>>>> current trivial malloc implementation, malloc would be implemented as
>>>> ops->morecore(size, align_min).  I think we agree on the overall idea
>>>> and on having only one function pointer.
>>>
>>> Yup, already done. But 2, not 1, pointers. Still need 'free' too.
>>
>> Didn't we say that the morecore allocator would only grow?
> 
> morecore[phys_alloc] only grows, free -> {}
> morecore[vmemalign] can do both, free -> vfree

Though there'd be no caller of .free, and vfree is sitting mostly unused
right now.  You decide.

> Hmm, I see what's going on now in x86. First, setup_vm should be doing
> setup_mmu *before* 'free_memory' (what I renamed to heap_init), because
> it assumes it's using virtual address with page_alloc/free. Second, x86
> creates an identity map up to end_of_memory, and that's the last address
> the heap knows about too. So, if we want to allocate free virtual
> addresses upwards, then we should start from end_of_memory.

Right, from 2G.  There's even a comment. :)

>>>> 5) moves the virt allocator and the vmalloc ops to a new file
>>>> lib/vmalloc.c (with install_page() remaining as the sole hook into
>>>> arch-specific code), removing what's now dead from lib/vmalloc.c.
>>>
>>> Yeah, this is a good next step. Will do.
> 
> This one looks like we'll need to cleanup the install_page parameter
> expectations a bit in order to keep arch-specific stuff out of the callers,
> but that should be doable.

The first argument can go---its always phys_to_virt(read_cr3())---and
with that it should be fine.

Paolo