[kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator

[kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator

[Claudio Imbrenda]

My previous patchseries was rushed and not polished enough. Furthermore it
introduced some regressions.

This patchseries fixes hopefully all the issues reported, and introduces
some new features.

It also simplifies the code and hopefully makes it more readable.

Fixed:
* allocated memory is now zeroed by default

New features:
* per-allocation flags to specify not just the area (like before) but also
  other parameters
  - zero flag: the allocation will be zeroed
  - fresh flag: the returned memory has never been read or written to before
* default flags: it's possible to specify which flags should be enabled
  automatically at each allocation; by default the zero flag is set.


I would appreciate if people could test these patches, especially on
strange, unusual or exotic hardware (I tested only on s390x)


GitLab:
  https://gitlab.com/imbrenda/kvm-unit-tests/-/tree/page_allocator_fixes
CI:
  https://gitlab.com/imbrenda/kvm-unit-tests/-/pipelines/229689192


Claudio Imbrenda (12):
  lib/x86: fix page.h to include the generic header
  lib/list.h: add list_add_tail
  lib/vmalloc: add some asserts and improvements
  lib/asm: Fix definitions of memory areas
  lib/alloc_page: fix and improve the page allocator
  lib/alloc.h: remove align_min from struct alloc_ops
  lib/alloc_page: Optimization to skip known empty freelists
  lib/alloc_page: rework metadata format
  lib/alloc: replace areas with more generic flags
  lib/alloc_page: Wire up ZERO_FLAG
  lib/alloc_page: Properly handle requests for fresh blocks
  lib/alloc_page: default flags and zero pages by default

 lib/asm-generic/memory_areas.h |   9 +-
 lib/arm/asm/memory_areas.h     |  11 +-
 lib/arm64/asm/memory_areas.h   |  11 +-
 lib/powerpc/asm/memory_areas.h |  11 +-
 lib/ppc64/asm/memory_areas.h   |  11 +-
 lib/s390x/asm/memory_areas.h   |  13 +-
 lib/x86/asm/memory_areas.h     |  27 +--
 lib/x86/asm/page.h             |   4 +-
 lib/alloc.h                    |   1 -
 lib/alloc_page.h               |  66 ++++++--
 lib/list.h                     |   9 +
 lib/alloc_page.c               | 291 ++++++++++++++++++++-------------
 lib/alloc_phys.c               |   9 +-
 lib/s390x/smp.c                |   2 +-
 lib/vmalloc.c                  |  21 +--
 15 files changed, 286 insertions(+), 210 deletions(-)

-- 
2.26.2

[kvm-unit-tests PATCH v1 01/12] lib/x86: fix page.h to include the generic header

[Claudio Imbrenda]

Bring x86 in line with the other architectures and include the generic header
at asm-generic/page.h .
This provides the macros PAGE_SHIFT, PAGE_SIZE, PAGE_MASK, virt_to_pfn, and
pfn_to_virt.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/x86/asm/page.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
index 1359eb7..2cf8881 100644
--- a/lib/x86/asm/page.h
+++ b/lib/x86/asm/page.h
@@ -13,9 +13,7 @@
 typedef unsigned long pteval_t;
 typedef unsigned long pgd_t;
 
-#define PAGE_SHIFT	12
-#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <asm-generic/page.h>
 
 #ifndef __ASSEMBLY__
 
-- 
2.26.2

[kvm-unit-tests PATCH v1 02/12] lib/list.h: add list_add_tail

[Claudio Imbrenda]

Add a list_add_tail wrapper function to allow adding elements to the end
of a list.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/list.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/list.h b/lib/list.h
index 18d9516..7f9717e 100644
--- a/lib/list.h
+++ b/lib/list.h
@@ -50,4 +50,13 @@ static inline void list_add(struct linked_list *head, struct linked_list *li)
 	head->next = li;
 }
 
+/*
+ * Add the given element before the given list head.
+ */
+static inline void list_add_tail(struct linked_list *head, struct linked_list *li)
+{
+	assert(head);
+	list_add(head->prev, li);
+}
+
 #endif
-- 
2.26.2

[kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements

[Claudio Imbrenda]

Add some asserts to make sure the state is consistent.

Simplify and improve the readability of vm_free.

Fixes: 3f6fee0d4da4 ("lib/vmalloc: vmalloc support for handling allocation metadata")

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/vmalloc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 986a34c..7a49adf 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -162,13 +162,14 @@ static void *vm_memalign(size_t alignment, size_t size)
 static void vm_free(void *mem)
 {
 	struct metadata *m;
-	uintptr_t ptr, end;
+	uintptr_t ptr, page, i;
 
 	/* the pointer is not page-aligned, it was a single-page allocation */
 	if (!IS_ALIGNED((uintptr_t)mem, PAGE_SIZE)) {
 		assert(GET_MAGIC(mem) == VM_MAGIC);
-		ptr = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
-		free_page(phys_to_virt(ptr));
+		page = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
+		assert(page);
+		free_page(phys_to_virt(page));
 		return;
 	}
 
@@ -176,13 +177,14 @@ static void vm_free(void *mem)
 	m = GET_METADATA(mem);
 	assert(m->magic == VM_MAGIC);
 	assert(m->npages > 0);
+	assert(m->npages < BIT_ULL(BITS_PER_LONG - PAGE_SHIFT));
 	/* free all the pages including the metadata page */
-	ptr = (uintptr_t)mem - PAGE_SIZE;
-	end = ptr + m->npages * PAGE_SIZE;
-	for ( ; ptr < end; ptr += PAGE_SIZE)
-		free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
-	/* free the last one separately to avoid overflow issues */
-	free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
+	ptr = (uintptr_t)m & PAGE_MASK;
+	for (i = 0 ; i < m->npages + 1; i++, ptr += PAGE_SIZE) {
+		page = virt_to_pte_phys(page_root, (void *)ptr) & PAGE_MASK;
+		assert(page);
+		free_page(phys_to_virt(page));
+	}
 }
 
 static struct alloc_ops vmalloc_ops = {
-- 
2.26.2

[kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas

[Claudio Imbrenda]

Fix the definitions of the memory areas.

Bring the headers in line with the rest of the asm headers, by having the
appropriate #ifdef _ASM$ARCH_ guarding the headers.

Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/asm-generic/memory_areas.h |  9 ++++-----
 lib/arm/asm/memory_areas.h     | 11 +++--------
 lib/arm64/asm/memory_areas.h   | 11 +++--------
 lib/powerpc/asm/memory_areas.h | 11 +++--------
 lib/ppc64/asm/memory_areas.h   | 11 +++--------
 lib/s390x/asm/memory_areas.h   | 13 ++++++-------
 lib/x86/asm/memory_areas.h     | 27 ++++++++++++++++-----------
 lib/alloc_page.h               |  3 +++
 lib/alloc_page.c               |  4 +---
 9 files changed, 42 insertions(+), 58 deletions(-)

diff --git a/lib/asm-generic/memory_areas.h b/lib/asm-generic/memory_areas.h
index 927baa7..3074afe 100644
--- a/lib/asm-generic/memory_areas.h
+++ b/lib/asm-generic/memory_areas.h
@@ -1,11 +1,10 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
+#define __ASM_GENERIC_MEMORY_AREAS_H__
 
 #define AREA_NORMAL_PFN 0
 #define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
+#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
 
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define MAX_AREAS 1
 
 #endif
diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
index 927baa7..c723310 100644
--- a/lib/arm/asm/memory_areas.h
+++ b/lib/arm/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMARM_MEMORY_AREAS_H_
+#define _ASMARM_MEMORY_AREAS_H_
 
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
 
 #endif
diff --git a/lib/arm64/asm/memory_areas.h b/lib/arm64/asm/memory_areas.h
index 927baa7..18e8ca8 100644
--- a/lib/arm64/asm/memory_areas.h
+++ b/lib/arm64/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMARM64_MEMORY_AREAS_H_
+#define _ASMARM64_MEMORY_AREAS_H_
 
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
 
 #endif
diff --git a/lib/powerpc/asm/memory_areas.h b/lib/powerpc/asm/memory_areas.h
index 927baa7..76d1738 100644
--- a/lib/powerpc/asm/memory_areas.h
+++ b/lib/powerpc/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
+#define _ASMPOWERPC_MEMORY_AREAS_H_
 
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
 
 #endif
diff --git a/lib/ppc64/asm/memory_areas.h b/lib/ppc64/asm/memory_areas.h
index 927baa7..b9fd46b 100644
--- a/lib/ppc64/asm/memory_areas.h
+++ b/lib/ppc64/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMPPC64_MEMORY_AREAS_H_
+#define _ASMPPC64_MEMORY_AREAS_H_
 
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
 
 #endif
diff --git a/lib/s390x/asm/memory_areas.h b/lib/s390x/asm/memory_areas.h
index 4856a27..827bfb3 100644
--- a/lib/s390x/asm/memory_areas.h
+++ b/lib/s390x/asm/memory_areas.h
@@ -1,16 +1,15 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMS390X_MEMORY_AREAS_H_
+#define _ASMS390X_MEMORY_AREAS_H_
 
-#define AREA_NORMAL_PFN BIT(31-12)
+#define AREA_NORMAL_PFN (1 << 19)
 #define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
+#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
 
 #define AREA_LOW_PFN 0
 #define AREA_LOW_NUMBER 1
-#define AREA_LOW 2
+#define AREA_LOW (1 << AREA_LOW_NUMBER)
 
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define MAX_AREAS 2
 
 #define AREA_DMA31 AREA_LOW
 
diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
index 952f5bd..e84016f 100644
--- a/lib/x86/asm/memory_areas.h
+++ b/lib/x86/asm/memory_areas.h
@@ -1,21 +1,26 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASM_X86_MEMORY_AREAS_H_
+#define _ASM_X86_MEMORY_AREAS_H_
 
 #define AREA_NORMAL_PFN BIT(36-12)
 #define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
+#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
 
-#define AREA_PAE_HIGH_PFN BIT(32-12)
-#define AREA_PAE_HIGH_NUMBER 1
-#define AREA_PAE_HIGH 2
+#define AREA_HIGH_PFN BIT(32-12)
+#define AREA_HIGH_NUMBER 1
+#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
 
-#define AREA_LOW_PFN 0
+#define AREA_LOW_PFN BIT(24-12)
 #define AREA_LOW_NUMBER 2
-#define AREA_LOW 4
+#define AREA_LOW (1 << AREA_LOW_NUMBER)
 
-#define AREA_PAE (AREA_PAE | AREA_LOW)
+#define AREA_LOWEST_PFN 0
+#define AREA_LOWEST_NUMBER 3
+#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
 
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define MAX_AREAS 4
+
+#define AREA_DMA24 AREA_LOWEST
+#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
+#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
 
 #endif
diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index 816ff5d..b6aace5 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -10,6 +10,9 @@
 
 #include <asm/memory_areas.h>
 
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
 /* Returns true if the page allocator has been initialized */
 bool page_alloc_initialized(void);
 
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 685ab1e..ed0ff02 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -19,8 +19,6 @@
 #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
 #define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
 
-#define MAX_AREAS	6
-
 #define ORDER_MASK	0x3f
 #define ALLOC_MASK	0x40
 #define SPECIAL_MASK	0x80
@@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
 		return;
 	}
 #ifdef AREA_HIGH_PFN
-	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN), base_pfn, &top_pfn);
+	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN, base_pfn, &top_pfn);
 #endif
 	__page_alloc_init_area(AREA_NORMAL_NUMBER, AREA_NORMAL_PFN, base_pfn, &top_pfn);
 #ifdef AREA_LOW_PFN
-- 
2.26.2

[kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator

[Claudio Imbrenda]

This patch introduces some improvements to the code, mostly readability
improvements, but also some semantic details, and improvements in the
documentation.

* introduce and use pfn_t to semantically tag parameters as PFNs
* remove the PFN macro, use virt_to_pfn instead
* rename area_or_metadata_contains and area_contains to area_contains_pfn
  and usable_area_contains_pfn respectively
* fix/improve comments in lib/alloc_page.h
* move some wrapper functions to the header

Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
Fixes: 34c950651861 ("lib/alloc_page: allow reserving arbitrary memory ranges")

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.h |  49 +++++++++-----
 lib/alloc_page.c | 165 +++++++++++++++++++++++------------------------
 2 files changed, 116 insertions(+), 98 deletions(-)

diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index b6aace5..d8550c6 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -8,6 +8,7 @@
 #ifndef ALLOC_PAGE_H
 #define ALLOC_PAGE_H 1
 
+#include <stdbool.h>
 #include <asm/memory_areas.h>
 
 #define AREA_ANY -1
@@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
  * top_pfn is the physical frame number of the first page immediately after
  * the end of the area to initialize
  */
-void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn);
+void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn);
 
 /* Enables the page allocator. At least one area must have been initialized */
 void page_alloc_ops_enable(void);
@@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas, size_t alignment, size_t size);
 
 /*
  * Allocate aligned memory from any area.
- * Equivalent to memalign_pages_area(~0, alignment, size).
+ * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
  */
-void *memalign_pages(size_t alignment, size_t size);
+static inline void *memalign_pages(size_t alignment, size_t size)
+{
+	return memalign_pages_area(AREA_ANY, alignment, size);
+}
 
 /*
  * Allocate naturally aligned memory from the specified areas.
@@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t size);
 void *alloc_pages_area(unsigned int areas, unsigned int order);
 
 /*
- * Allocate one page from any area.
- * Equivalent to alloc_pages(0);
+ * Allocate naturally aligned memory from any area.
+ * Equivalent to alloc_pages_area(AREA_ANY, order);
  */
-void *alloc_page(void);
+static inline void *alloc_pages(unsigned int order)
+{
+	return alloc_pages_area(AREA_ANY, order);
+}
 
 /*
- * Allocate naturally aligned memory from any area.
- * Equivalent to alloc_pages_area(~0, order);
+ * Allocate one page from any area.
+ * Equivalent to alloc_pages(0);
  */
-void *alloc_pages(unsigned int order);
+static inline void *alloc_page(void)
+{
+	return alloc_pages(0);
+}
 
 /*
  * Frees a memory block allocated with any of the memalign_pages* or
@@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
  */
 void free_pages(void *mem);
 
-/* For backwards compatibility */
+/*
+ * Free one page.
+ * Equivalent to free_pages(mem).
+ */
 static inline void free_page(void *mem)
 {
 	return free_pages(mem);
 }
 
-/* For backwards compatibility */
+/*
+ * Free pages by order.
+ * Equivalent to free_pages(mem).
+ */
 static inline void free_pages_by_order(void *mem, unsigned int order)
 {
 	free_pages(mem);
 }
 
 /*
- * Allocates and reserves the specified memory range if possible.
- * Returns NULL in case of failure.
+ * Allocates and reserves the specified physical memory range if possible.
+ * If the specified range cannot be reserved in its entirety, no action is
+ * performed and false is returned.
+ *
+ * Returns true in case of success, false otherwise.
  */
-void *alloc_pages_special(uintptr_t addr, size_t npages);
+bool alloc_pages_special(phys_addr_t addr, size_t npages);
 
 /*
  * Frees a reserved memory range that had been reserved with
@@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t npages);
  * exactly, it can also be a subset, in which case only the specified
  * pages will be freed and unreserved.
  */
-void free_pages_special(uintptr_t addr, size_t npages);
+void free_pages_special(phys_addr_t addr, size_t npages);
 
 #endif
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index ed0ff02..8d2700d 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -17,25 +17,29 @@
 
 #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
 #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
-#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
 
 #define ORDER_MASK	0x3f
 #define ALLOC_MASK	0x40
 #define SPECIAL_MASK	0x80
 
+typedef phys_addr_t pfn_t;
+
 struct mem_area {
 	/* Physical frame number of the first usable frame in the area */
-	uintptr_t base;
+	pfn_t base;
 	/* Physical frame number of the first frame outside the area */
-	uintptr_t top;
-	/* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
+	pfn_t top;
+	/* Per page metadata, each entry is a combination *_MASK and order */
 	u8 *page_states;
 	/* One freelist for each possible block size, up to NLISTS */
 	struct linked_list freelists[NLISTS];
 };
 
+/* Descriptors for each possible area */
 static struct mem_area areas[MAX_AREAS];
+/* Mask of initialized areas */
 static unsigned int areas_mask;
+/* Protects areas and areas mask */
 static struct spinlock lock;
 
 bool page_alloc_initialized(void)
@@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
 	return areas_mask != 0;
 }
 
-static inline bool area_or_metadata_contains(struct mem_area *a, uintptr_t pfn)
+/*
+ * Each memory area contains an array of metadata entries at the very
+ * beginning. The usable memory follows immediately afterwards.
+ * This function returns true if the given pfn falls anywhere within the
+ * memory area, including the metadata area.
+ */
+static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
 {
-	return (pfn >= PFN(a->page_states)) && (pfn < a->top);
+	return (pfn >= virt_to_pfn(a->page_states)) && (pfn < a->top);
 }
 
-static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
+/*
+ * Each memory area contains an array of metadata entries at the very
+ * beginning. The usable memory follows immediately afterwards.
+ * This function returns true if the given pfn falls in the usable range of
+ * the given memory area.
+ */
+static inline bool usable_area_contains_pfn(struct mem_area *a, pfn_t pfn)
 {
 	return (pfn >= a->base) && (pfn < a->top);
 }
@@ -69,21 +85,19 @@ static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
  */
 static void split(struct mem_area *a, void *addr)
 {
-	uintptr_t pfn = PFN(addr);
-	struct linked_list *p;
-	uintptr_t i, idx;
+	pfn_t pfn = virt_to_pfn(addr);
+	pfn_t i, idx;
 	u8 order;
 
-	assert(a && area_contains(a, pfn));
+	assert(a && usable_area_contains_pfn(a, pfn));
 	idx = pfn - a->base;
 	order = a->page_states[idx];
 	assert(!(order & ~ORDER_MASK) && order && (order < NLISTS));
 	assert(IS_ALIGNED_ORDER(pfn, order));
-	assert(area_contains(a, pfn + BIT(order) - 1));
+	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
 
 	/* Remove the block from its free list */
-	p = list_remove(addr);
-	assert(p);
+	list_remove(addr);
 
 	/* update the block size for each page in the block */
 	for (i = 0; i < BIT(order); i++) {
@@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
 	}
 	order--;
 	/* add the first half block to the appropriate free list */
-	list_add(a->freelists + order, p);
+	list_add(a->freelists + order, addr);
 	/* add the second half block to the appropriate free list */
-	list_add(a->freelists + order, (void *)((pfn + BIT(order)) * PAGE_SIZE));
+	list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
 }
 
 /*
@@ -105,7 +119,7 @@ static void split(struct mem_area *a, void *addr)
  */
 static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
 {
-	struct linked_list *p, *res = NULL;
+	struct linked_list *p;
 	u8 order;
 
 	assert((al < NLISTS) && (sz < NLISTS));
@@ -130,17 +144,17 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
 	for (; order > sz; order--)
 		split(a, p);
 
-	res = list_remove(p);
-	memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK | order, BIT(order));
-	return res;
+	list_remove(p);
+	memset(a->page_states + (virt_to_pfn(p) - a->base), ALLOC_MASK | order, BIT(order));
+	return p;
 }
 
-static struct mem_area *get_area(uintptr_t pfn)
+static struct mem_area *get_area(pfn_t pfn)
 {
 	uintptr_t i;
 
 	for (i = 0; i < MAX_AREAS; i++)
-		if ((areas_mask & BIT(i)) && area_contains(areas + i, pfn))
+		if ((areas_mask & BIT(i)) && usable_area_contains_pfn(areas + i, pfn))
 			return areas + i;
 	return NULL;
 }
@@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t pfn)
  * - all of the pages of the two blocks must have the same block size
  * - the function is called with the lock held
  */
-static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2)
+static bool coalesce(struct mem_area *a, u8 order, pfn_t pfn, pfn_t pfn2)
 {
-	uintptr_t first, second, i;
-	struct linked_list *li;
+	pfn_t first, second, i;
 
 	assert(IS_ALIGNED_ORDER(pfn, order) && IS_ALIGNED_ORDER(pfn2, order));
 	assert(pfn2 == pfn + BIT(order));
 	assert(a);
 
 	/* attempting to coalesce two blocks that belong to different areas */
-	if (!area_contains(a, pfn) || !area_contains(a, pfn2 + BIT(order) - 1))
+	if (!usable_area_contains_pfn(a, pfn) || !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1))
 		return false;
 	first = pfn - a->base;
 	second = pfn2 - a->base;
@@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
 		return false;
 
 	/* we can coalesce, remove both blocks from their freelists */
-	li = list_remove((void *)(pfn2 << PAGE_SHIFT));
-	assert(li);
-	li = list_remove((void *)(pfn << PAGE_SHIFT));
-	assert(li);
+	list_remove(pfn_to_virt(pfn2));
+	list_remove(pfn_to_virt(pfn));
 	/* check the metadata entries and update with the new size */
 	for (i = 0; i < (2ull << order); i++) {
 		assert(a->page_states[first + i] == order);
 		a->page_states[first + i] = order + 1;
 	}
 	/* finally add the newly coalesced block to the appropriate freelist */
-	list_add(a->freelists + order + 1, li);
+	list_add(a->freelists + order + 1, pfn_to_virt(pfn));
 	return true;
 }
 
@@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
  */
 static void _free_pages(void *mem)
 {
-	uintptr_t pfn2, pfn = PFN(mem);
+	pfn_t pfn2, pfn = virt_to_pfn(mem);
 	struct mem_area *a = NULL;
 	uintptr_t i, p;
 	u8 order;
@@ -232,7 +243,7 @@ static void _free_pages(void *mem)
 	/* ensure that the block is aligned properly for its size */
 	assert(IS_ALIGNED_ORDER(pfn, order));
 	/* ensure that the area can contain the whole block */
-	assert(area_contains(a, pfn + BIT(order) - 1));
+	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
 
 	for (i = 0; i < BIT(order); i++) {
 		/* check that all pages of the block have consistent metadata */
@@ -268,63 +279,68 @@ void free_pages(void *mem)
 	spin_unlock(&lock);
 }
 
-static void *_alloc_page_special(uintptr_t addr)
+static bool _alloc_page_special(pfn_t pfn)
 {
 	struct mem_area *a;
-	uintptr_t mask, i;
+	pfn_t mask, i;
 
-	a = get_area(PFN(addr));
-	assert(a);
-	i = PFN(addr) - a->base;
+	a = get_area(pfn);
+	if (!a)
+		return false;
+	i = pfn - a->base;
 	if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
-		return NULL;
+		return false;
 	while (a->page_states[i]) {
-		mask = GENMASK_ULL(63, PAGE_SHIFT + a->page_states[i]);
-		split(a, (void *)(addr & mask));
+		mask = GENMASK_ULL(63, a->page_states[i]);
+		split(a, pfn_to_virt(pfn & mask));
 	}
 	a->page_states[i] = SPECIAL_MASK;
-	return (void *)addr;
+	return true;
 }
 
-static void _free_page_special(uintptr_t addr)
+static void _free_page_special(pfn_t pfn)
 {
 	struct mem_area *a;
-	uintptr_t i;
+	pfn_t i;
 
-	a = get_area(PFN(addr));
+	a = get_area(pfn);
 	assert(a);
-	i = PFN(addr) - a->base;
+	i = pfn - a->base;
 	assert(a->page_states[i] == SPECIAL_MASK);
 	a->page_states[i] = ALLOC_MASK;
-	_free_pages((void *)addr);
+	_free_pages(pfn_to_virt(pfn));
 }
 
-void *alloc_pages_special(uintptr_t addr, size_t n)
+bool alloc_pages_special(phys_addr_t addr, size_t n)
 {
-	uintptr_t i;
+	pfn_t pfn;
+	size_t i;
 
 	assert(IS_ALIGNED(addr, PAGE_SIZE));
+	pfn = addr >> PAGE_SHIFT;
 	spin_lock(&lock);
 	for (i = 0; i < n; i++)
-		if (!_alloc_page_special(addr + i * PAGE_SIZE))
+		if (!_alloc_page_special(pfn + i))
 			break;
 	if (i < n) {
 		for (n = 0 ; n < i; n++)
-			_free_page_special(addr + n * PAGE_SIZE);
-		addr = 0;
+			_free_page_special(pfn + n);
+		n = 0;
 	}
 	spin_unlock(&lock);
-	return (void *)addr;
+	return n;
 }
 
-void free_pages_special(uintptr_t addr, size_t n)
+void free_pages_special(phys_addr_t addr, size_t n)
 {
-	uintptr_t i;
+	pfn_t pfn;
+	size_t i;
 
 	assert(IS_ALIGNED(addr, PAGE_SIZE));
+	pfn = addr >> PAGE_SHIFT;
 	spin_lock(&lock);
 	for (i = 0; i < n; i++)
-		_free_page_special(addr + i * PAGE_SIZE);
+		_free_page_special(pfn + i);
 	spin_unlock(&lock);
 }
 
@@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area, unsigned int order)
 	return page_memalign_order_area(area, order, order);
 }
 
-void *alloc_pages(unsigned int order)
-{
-	return alloc_pages_area(AREA_ANY, order);
-}
-
 /*
  * Allocates (1 << order) physically contiguous aligned pages.
  * Returns NULL if the allocation was not possible.
@@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
 	return page_memalign_order_area(area, size, alignment);
 }
 
-void *memalign_pages(size_t alignment, size_t size)
-{
-	return memalign_pages_area(AREA_ANY, alignment, size);
-}
-
-/*
- * Allocates one page
- */
-void *alloc_page()
-{
-	return alloc_pages(0);
-}
 
 static struct alloc_ops page_alloc_ops = {
 	.memalign = memalign_pages,
@@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
  * - the memory area to add does not overlap with existing areas
  * - the memory area to add has at least 5 pages available
  */
-static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
+static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
 {
 	size_t table_size, npages, i;
 	struct mem_area *a;
@@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
 
 	/* fill in the values of the new area */
 	a = areas + n;
-	a->page_states = (void *)(start_pfn << PAGE_SHIFT);
+	a->page_states = pfn_to_virt(start_pfn);
 	a->base = start_pfn + table_size;
 	a->top = top_pfn;
 	npages = top_pfn - a->base;
@@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
 	for (i = 0; i < MAX_AREAS; i++) {
 		if (!(areas_mask & BIT(i)))
 			continue;
-		assert(!area_or_metadata_contains(areas + i, start_pfn));
-		assert(!area_or_metadata_contains(areas + i, top_pfn - 1));
-		assert(!area_or_metadata_contains(a, PFN(areas[i].page_states)));
-		assert(!area_or_metadata_contains(a, areas[i].top - 1));
+		assert(!area_contains_pfn(areas + i, start_pfn));
+		assert(!area_contains_pfn(areas + i, top_pfn - 1));
+		assert(!area_contains_pfn(a, virt_to_pfn(areas[i].page_states)));
+		assert(!area_contains_pfn(a, areas[i].top - 1));
 	}
 	/* initialize all freelists for the new area */
 	for (i = 0; i < NLISTS; i++)
-		a->freelists[i].next = a->freelists[i].prev = a->freelists + i;
+		a->freelists[i].prev = a->freelists[i].next = a->freelists + i;
 
 	/* initialize the metadata for the available memory */
 	for (i = a->base; i < a->top; i += 1ull << order) {
@@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
 		assert(order < NLISTS);
 		/* initialize the metadata and add to the freelist */
 		memset(a->page_states + (i - a->base), order, BIT(order));
-		list_add(a->freelists + order, (void *)(i << PAGE_SHIFT));
+		list_add(a->freelists + order, pfn_to_virt(i));
 	}
 	/* finally mark the area as present */
 	areas_mask |= BIT(n);
 }
 
-static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, uintptr_t *top_pfn)
+static void __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t *top_pfn)
 {
 	if (*top_pfn > cutoff) {
 		spin_lock(&lock);
@@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, u
  * Prerequisites:
  * see _page_alloc_init_area
  */
-void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
+void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn)
 {
 	if (n != AREA_ANY_NUMBER) {
 		__page_alloc_init_area(n, 0, base_pfn, &top_pfn);
-- 
2.26.2

[kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops

[Claudio Imbrenda]

Remove align_min from struct alloc_ops, since it is no longer used.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc.h      | 1 -
 lib/alloc_page.c | 1 -
 lib/alloc_phys.c | 9 +++++----
 lib/vmalloc.c    | 1 -
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/alloc.h b/lib/alloc.h
index 9b4b634..db90b01 100644
--- a/lib/alloc.h
+++ b/lib/alloc.h
@@ -25,7 +25,6 @@
 struct alloc_ops {
 	void *(*memalign)(size_t alignment, size_t size);
 	void (*free)(void *ptr);
-	size_t align_min;
 };
 
 extern struct alloc_ops *alloc_ops;
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 8d2700d..b1cdf21 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
 static struct alloc_ops page_alloc_ops = {
 	.memalign = memalign_pages,
 	.free = free_pages,
-	.align_min = PAGE_SIZE,
 };
 
 /*
diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
index 72e20f7..a4d2bf2 100644
--- a/lib/alloc_phys.c
+++ b/lib/alloc_phys.c
@@ -29,8 +29,8 @@ static phys_addr_t base, top;
 static void *early_memalign(size_t alignment, size_t size);
 static struct alloc_ops early_alloc_ops = {
 	.memalign = early_memalign,
-	.align_min = DEFAULT_MINIMUM_ALIGNMENT
 };
+static size_t align_min;
 
 struct alloc_ops *alloc_ops = &early_alloc_ops;
 
@@ -39,8 +39,7 @@ void phys_alloc_show(void)
 	int i;
 
 	spin_lock(&lock);
-	printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
-		(u64)early_alloc_ops.align_min);
+	printf("phys_alloc minimum alignment: %#" PRIx64 "\n", (u64)align_min);
 	for (i = 0; i < nr_regions; ++i)
 		printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
 			(u64)regions[i].base,
@@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t align)
 {
 	assert(align && !(align & (align - 1)));
 	spin_lock(&lock);
-	early_alloc_ops.align_min = align;
+	align_min = align;
 	spin_unlock(&lock);
 }
 
@@ -83,6 +82,8 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
 		top_safe = MIN(top_safe, 1ULL << 32);
 
 	assert(base < top_safe);
+	if (align < align_min)
+		align = align_min;
 
 	addr = ALIGN(base, align);
 	size += addr - base;
diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 7a49adf..e146162 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -190,7 +190,6 @@ static void vm_free(void *mem)
 static struct alloc_ops vmalloc_ops = {
 	.memalign = vm_memalign,
 	.free = vm_free,
-	.align_min = PAGE_SIZE,
 };
 
 void __attribute__((__weak__)) find_highmem(void)
-- 
2.26.2

[kvm-unit-tests PATCH v1 07/12] lib/alloc_page: Optimization to skip known empty freelists

[Claudio Imbrenda]

Keep track of the largest block order available in each area, and do not
search past it when looking for free memory.

This will avoid needlessly scanning the freelists for the largest block
orders, which will be empty in most cases.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index b1cdf21..6a76b45 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -31,6 +31,8 @@ struct mem_area {
 	pfn_t top;
 	/* Per page metadata, each entry is a combination *_MASK and order */
 	u8 *page_states;
+	/* Highest block order available in this area */
+	u8 max_order;
 	/* One freelist for each possible block size, up to NLISTS */
 	struct linked_list freelists[NLISTS];
 };
@@ -104,6 +106,8 @@ static void split(struct mem_area *a, void *addr)
 		assert(a->page_states[idx + i] == order);
 		a->page_states[idx + i] = order - 1;
 	}
+	if ((order == a->max_order) && (is_list_empty(a->freelists + order)))
+		a->max_order--;
 	order--;
 	/* add the first half block to the appropriate free list */
 	list_add(a->freelists + order, addr);
@@ -127,13 +131,13 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
 	order = sz > al ? sz : al;
 
 	/* search all free lists for some memory */
-	for ( ; order < NLISTS; order++) {
+	for ( ; order <= a->max_order; order++) {
 		p = a->freelists[order].next;
 		if (!is_list_empty(p))
 			break;
 	}
 	/* out of memory */
-	if (order >= NLISTS)
+	if (order > a->max_order)
 		return NULL;
 
 	/*
@@ -201,6 +205,8 @@ static bool coalesce(struct mem_area *a, u8 order, pfn_t pfn, pfn_t pfn2)
 	}
 	/* finally add the newly coalesced block to the appropriate freelist */
 	list_add(a->freelists + order + 1, pfn_to_virt(pfn));
+	if (order + 1 > a->max_order)
+		a->max_order = order + 1;
 	return true;
 }
 
@@ -438,6 +444,7 @@ static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
 	a->page_states = pfn_to_virt(start_pfn);
 	a->base = start_pfn + table_size;
 	a->top = top_pfn;
+	a->max_order = 0;
 	npages = top_pfn - a->base;
 	assert((a->base - start_pfn) * PAGE_SIZE >= npages);
 
@@ -472,6 +479,8 @@ static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
 		/* initialize the metadata and add to the freelist */
 		memset(a->page_states + (i - a->base), order, BIT(order));
 		list_add(a->freelists + order, pfn_to_virt(i));
+		if (order > a->max_order)
+			a->max_order = order;
 	}
 	/* finally mark the area as present */
 	areas_mask |= BIT(n);
-- 
2.26.2

[kvm-unit-tests PATCH v1 08/12] lib/alloc_page: rework metadata format

[Claudio Imbrenda]

This patch changes the format of the metadata so that the metadata is
now a 2-bit field instead of two separate flags.

This allows to have 4 different states for memory:

STATUS_FRESH: the memory is free and has not been touched at all since
              boot (not even read from!)
STATUS_FREE: the memory is free, but it is probably not fresh any more
STATUS_ALLOCATED: the memory has been allocated and is in use
STATUS_SPECIAL: the memory has been removed from the pool of allocated
                memory for some kind of special purpose according to
                the needs of the caller

Some macros are also introduced to test the status of a specific
metadata item.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.c | 49 +++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 6a76b45..dfa43d5 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -18,9 +18,20 @@
 #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
 #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
 
-#define ORDER_MASK	0x3f
-#define ALLOC_MASK	0x40
-#define SPECIAL_MASK	0x80
+#define ORDER_MASK		0x3f
+#define STATUS_MASK		0xc0
+
+#define STATUS_FRESH		0x00
+#define STATUS_FREE		0x40
+#define STATUS_ALLOCATED	0x80
+#define STATUS_SPECIAL		0xc0
+
+#define IS_FRESH(x)	(((x) & STATUS_MASK) == STATUS_FRESH)
+#define IS_FREE(x)	(((x) & STATUS_MASK) == STATUS_FREE)
+#define IS_ALLOCATED(x)	(((x) & STATUS_MASK) == STATUS_ALLOCATED)
+#define IS_SPECIAL(x)	(((x) & STATUS_MASK) == STATUS_SPECIAL)
+
+#define IS_USABLE(x)	(IS_FREE(x) || IS_FRESH(x))
 
 typedef phys_addr_t pfn_t;
 
@@ -87,14 +98,14 @@ static inline bool usable_area_contains_pfn(struct mem_area *a, pfn_t pfn)
  */
 static void split(struct mem_area *a, void *addr)
 {
-	pfn_t pfn = virt_to_pfn(addr);
-	pfn_t i, idx;
-	u8 order;
+	pfn_t i, idx, pfn = virt_to_pfn(addr);
+	u8 metadata, order;
 
 	assert(a && usable_area_contains_pfn(a, pfn));
 	idx = pfn - a->base;
-	order = a->page_states[idx];
-	assert(!(order & ~ORDER_MASK) && order && (order < NLISTS));
+	metadata = a->page_states[idx];
+	order = metadata & ORDER_MASK;
+	assert(IS_USABLE(metadata) && order && (order < NLISTS));
 	assert(IS_ALIGNED_ORDER(pfn, order));
 	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
 
@@ -103,8 +114,8 @@ static void split(struct mem_area *a, void *addr)
 
 	/* update the block size for each page in the block */
 	for (i = 0; i < BIT(order); i++) {
-		assert(a->page_states[idx + i] == order);
-		a->page_states[idx + i] = order - 1;
+		assert(a->page_states[idx + i] == metadata);
+		a->page_states[idx + i] = metadata - 1;
 	}
 	if ((order == a->max_order) && (is_list_empty(a->freelists + order)))
 		a->max_order--;
@@ -149,7 +160,7 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
 		split(a, p);
 
 	list_remove(p);
-	memset(a->page_states + (virt_to_pfn(p) - a->base), ALLOC_MASK | order, BIT(order));
+	memset(a->page_states + (virt_to_pfn(p) - a->base), STATUS_ALLOCATED | order, BIT(order));
 	return p;
 }
 
@@ -243,7 +254,7 @@ static void _free_pages(void *mem)
 	order = a->page_states[p] & ORDER_MASK;
 
 	/* ensure that the first page is allocated and not special */
-	assert(a->page_states[p] == (order | ALLOC_MASK));
+	assert(IS_ALLOCATED(a->page_states[p]));
 	/* ensure that the order has a sane value */
 	assert(order < NLISTS);
 	/* ensure that the block is aligned properly for its size */
@@ -253,9 +264,9 @@ static void _free_pages(void *mem)
 
 	for (i = 0; i < BIT(order); i++) {
 		/* check that all pages of the block have consistent metadata */
-		assert(a->page_states[p + i] == (ALLOC_MASK | order));
+		assert(a->page_states[p + i] == (STATUS_ALLOCATED | order));
 		/* set the page as free */
-		a->page_states[p + i] &= ~ALLOC_MASK;
+		a->page_states[p + i] = STATUS_FREE | order;
 	}
 	/* provisionally add the block to the appropriate free list */
 	list_add(a->freelists + order, mem);
@@ -294,13 +305,13 @@ static bool _alloc_page_special(pfn_t pfn)
 	if (!a)
 		return false;
 	i = pfn - a->base;
-	if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
+	if (!IS_USABLE(a->page_states[i]))
 		return false;
 	while (a->page_states[i]) {
 		mask = GENMASK_ULL(63, a->page_states[i]);
 		split(a, pfn_to_virt(pfn & mask));
 	}
-	a->page_states[i] = SPECIAL_MASK;
+	a->page_states[i] = STATUS_SPECIAL;
 	return true;
 }
 
@@ -312,8 +323,8 @@ static void _free_page_special(pfn_t pfn)
 	a = get_area(pfn);
 	assert(a);
 	i = pfn - a->base;
-	assert(a->page_states[i] == SPECIAL_MASK);
-	a->page_states[i] = ALLOC_MASK;
+	assert(a->page_states[i] == STATUS_SPECIAL);
+	a->page_states[i] = STATUS_ALLOCATED;
 	_free_pages(pfn_to_virt(pfn));
 }
 
@@ -477,7 +488,7 @@ static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
 			order++;
 		assert(order < NLISTS);
 		/* initialize the metadata and add to the freelist */
-		memset(a->page_states + (i - a->base), order, BIT(order));
+		memset(a->page_states + (i - a->base), STATUS_FRESH | order, BIT(order));
 		list_add(a->freelists + order, pfn_to_virt(i));
 		if (order > a->max_order)
 			a->max_order = order;
-- 
2.26.2

[kvm-unit-tests PATCH v1 09/12] lib/alloc: replace areas with more generic flags

[Claudio Imbrenda]

Replace the areas parameter with a more generic flags parameter.

This allows for up to 16 allocation areas and 16 allocation flags.

This patch introduces the flags and changes the names of the funcions,
subsequent patches will actually wire up the flags to do something.

The first two flags introduced are:

FLAG_ZERO to ask the allocated memory to be zeroed

FLAG_FRESH to indicate that the allocated memory should have not been
touched (READ or written to) in any way since boot.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.h | 21 +++++++++++++--------
 lib/alloc_page.c | 14 +++++++-------
 lib/s390x/smp.c  |  2 +-
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index d8550c6..1039814 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -11,8 +11,13 @@
 #include <stdbool.h>
 #include <asm/memory_areas.h>
 
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define AREA_ANY_NUMBER	0xff
+
+#define AREA_ANY	0x00000
+#define AREA_MASK	0x0ffff
+
+#define FLAG_ZERO	0x10000
+#define FLAG_FRESH	0x20000
 
 /* Returns true if the page allocator has been initialized */
 bool page_alloc_initialized(void);
@@ -34,22 +39,22 @@ void page_alloc_ops_enable(void);
  * areas is a bitmap of allowed areas
  * alignment must be a power of 2
  */
-void *memalign_pages_area(unsigned int areas, size_t alignment, size_t size);
+void *memalign_pages_flags(size_t alignment, size_t size, unsigned int flags);
 
 /*
  * Allocate aligned memory from any area.
- * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
+ * Equivalent to memalign_pages_flags(alignment, size, AREA_ANY).
  */
 static inline void *memalign_pages(size_t alignment, size_t size)
 {
-	return memalign_pages_area(AREA_ANY, alignment, size);
+	return memalign_pages_flags(alignment, size, AREA_ANY);
 }
 
 /*
  * Allocate naturally aligned memory from the specified areas.
- * Equivalent to memalign_pages_area(areas, 1ull << order, 1ull << order).
+ * Equivalent to memalign_pages_flags(1ull << order, 1ull << order, flags).
  */
-void *alloc_pages_area(unsigned int areas, unsigned int order);
+void *alloc_pages_flags(unsigned int order, unsigned int flags);
 
 /*
  * Allocate naturally aligned memory from any area.
@@ -57,7 +62,7 @@ void *alloc_pages_area(unsigned int areas, unsigned int order);
  */
 static inline void *alloc_pages(unsigned int order)
 {
-	return alloc_pages_area(AREA_ANY, order);
+	return alloc_pages_flags(order, AREA_ANY);
 }
 
 /*
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index dfa43d5..d850b6a 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -361,13 +361,13 @@ void free_pages_special(phys_addr_t addr, size_t n)
 	spin_unlock(&lock);
 }
 
-static void *page_memalign_order_area(unsigned area, u8 ord, u8 al)
+static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
 {
 	void *res = NULL;
-	int i;
+	int i, area;
 
 	spin_lock(&lock);
-	area &= areas_mask;
+	area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;
 	for (i = 0; !res && (i < MAX_AREAS); i++)
 		if (area & BIT(i))
 			res = page_memalign_order(areas + i, ord, al);
@@ -379,23 +379,23 @@ static void *page_memalign_order_area(unsigned area, u8 ord, u8 al)
  * Allocates (1 << order) physically contiguous and naturally aligned pages.
  * Returns NULL if the allocation was not possible.
  */
-void *alloc_pages_area(unsigned int area, unsigned int order)
+void *alloc_pages_flags(unsigned int order, unsigned int flags)
 {
-	return page_memalign_order_area(area, order, order);
+	return page_memalign_order_flags(order, order, flags);
 }
 
 /*
  * Allocates (1 << order) physically contiguous aligned pages.
  * Returns NULL if the allocation was not possible.
  */
-void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
+void *memalign_pages_flags(size_t alignment, size_t size, unsigned int flags)
 {
 	assert(is_power_of_2(alignment));
 	alignment = get_order(PAGE_ALIGN(alignment) >> PAGE_SHIFT);
 	size = get_order(PAGE_ALIGN(size) >> PAGE_SHIFT);
 	assert(alignment < NLISTS);
 	assert(size < NLISTS);
-	return page_memalign_order_area(area, size, alignment);
+	return page_memalign_order_flags(size, alignment, flags);
 }
 
 
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 77d80ca..44b2eb4 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -190,7 +190,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 
 	sigp_retry(cpu->addr, SIGP_INITIAL_CPU_RESET, 0, NULL);
 
-	lc = alloc_pages_area(AREA_DMA31, 1);
+	lc = alloc_pages_flags(1, AREA_DMA31);
 	cpu->lowcore = lc;
 	memset(lc, 0, PAGE_SIZE * 2);
 	sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
-- 
2.26.2

[kvm-unit-tests PATCH v1 10/12] lib/alloc_page: Wire up ZERO_FLAG

[Claudio Imbrenda]

Memory allocated with the ZERO_FLAG will now be zeroed before being
returned to the caller.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index d850b6a..8c79202 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -372,6 +372,8 @@ static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
 		if (area & BIT(i))
 			res = page_memalign_order(areas + i, ord, al);
 	spin_unlock(&lock);
+	if (res && (flags & FLAG_ZERO))
+		memset(res, 0, BIT(ord) * PAGE_SIZE);
 	return res;
 }
 
-- 
2.26.2

[kvm-unit-tests PATCH v1 11/12] lib/alloc_page: Properly handle requests for fresh blocks

[Claudio Imbrenda]

Upon initialization, all memory in an area is marked as fresh. Once
memory is used and freed, the freed memory is marked as free.

Free memory is always appended to the front of the freelist, meaning
that fresh memory stays on the tail.

When a block of fresh memory is split, the two blocks are put on the
tail of the appropriate freelist, so they can be found when needed.

When a fresh block is requested, a fresh block one order bigger is
taken, the first half is put back in the free pool (on the tail), and
the second half is returned. The reason behind this is that the first
page of every block always contains the pointers of the freelist.
Since the first page of a fresh block is actually not fresh, it cannot
be returned when a fresh allocation is requested.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.c | 51 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 8c79202..4d5722f 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -120,10 +120,17 @@ static void split(struct mem_area *a, void *addr)
 	if ((order == a->max_order) && (is_list_empty(a->freelists + order)))
 		a->max_order--;
 	order--;
-	/* add the first half block to the appropriate free list */
-	list_add(a->freelists + order, addr);
-	/* add the second half block to the appropriate free list */
-	list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
+
+	/* add the two half blocks to the appropriate free list */
+	if (IS_FRESH(metadata)) {
+		/* add to the tail if the blocks are fresh */
+		list_add_tail(a->freelists + order, addr);
+		list_add_tail(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
+	} else {
+		/* add to the front if the blocks are dirty */
+		list_add(a->freelists + order, addr);
+		list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
+	}
 }
 
 /*
@@ -132,21 +139,33 @@ static void split(struct mem_area *a, void *addr)
  *
  * Both parameters must be not larger than the largest allowed order
  */
-static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
+static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz, bool fresh)
 {
 	struct linked_list *p;
+	pfn_t idx;
 	u8 order;
 
 	assert((al < NLISTS) && (sz < NLISTS));
 	/* we need the bigger of the two as starting point */
 	order = sz > al ? sz : al;
+	/*
+	 * we need to go one order up if we want a completely fresh block,
+	 * since the first page contains the freelist pointers, and
+	 * therefore it is always dirty
+	 */
+	order += fresh;
 
 	/* search all free lists for some memory */
 	for ( ; order <= a->max_order; order++) {
-		p = a->freelists[order].next;
-		if (!is_list_empty(p))
-			break;
+		p = fresh ? a->freelists[order].prev : a->freelists[order].next;
+		if (is_list_empty(p))
+			continue;
+		idx = virt_to_pfn(p) - a->base;
+		if (fresh && !IS_FRESH(a->page_states[idx]))
+			continue;
+		break;
 	}
+
 	/* out of memory */
 	if (order > a->max_order)
 		return NULL;
@@ -160,7 +179,16 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
 		split(a, p);
 
 	list_remove(p);
-	memset(a->page_states + (virt_to_pfn(p) - a->base), STATUS_ALLOCATED | order, BIT(order));
+	/* We now have a block twice the size, but the first page is dirty. */
+	if (fresh) {
+		order--;
+		/* Put back the first (partially dirty) half of the block */
+		memset(a->page_states + idx, STATUS_FRESH | order, BIT(order));
+		list_add_tail(a->freelists + order, p);
+		idx += BIT(order);
+		p = pfn_to_virt(a->base + idx);
+	}
+	memset(a->page_states + idx, STATUS_ALLOCATED | order, BIT(order));
 	return p;
 }
 
@@ -364,13 +392,14 @@ void free_pages_special(phys_addr_t addr, size_t n)
 static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
 {
 	void *res = NULL;
-	int i, area;
+	int i, area, fresh;
 
+	fresh = !!(flags & FLAG_FRESH);
 	spin_lock(&lock);
 	area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;
 	for (i = 0; !res && (i < MAX_AREAS); i++)
 		if (area & BIT(i))
-			res = page_memalign_order(areas + i, ord, al);
+			res = page_memalign_order(areas + i, ord, al, fresh);
 	spin_unlock(&lock);
 	if (res && (flags & FLAG_ZERO))
 		memset(res, 0, BIT(ord) * PAGE_SIZE);
-- 
2.26.2

[kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default

[Claudio Imbrenda]

The new function page_alloc_set_default_flags can be used to set the
default flags for allocations. The passed value will be ORed with the
flags argument passed to the allocator at each allocation.

The default value for the default flags is FLAG_ZERO, which means that
by default all allocated memory is now zeroed, restoring the default
behaviour that had been accidentally removed by a previous commit.

If needed, a testcase can call page_alloc_set_default_flags(0) in order
to get non-zeroed pages from the allocator. For example, if the
testcase will need fresh memory, the zero flag should be removed from
the default.

Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
Reported-by: Nadav Amit <nadav.amit@gmail.com>

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.h | 3 +++
 lib/alloc_page.c | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index 1039814..8b53a58 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -22,6 +22,9 @@
 /* Returns true if the page allocator has been initialized */
 bool page_alloc_initialized(void);
 
+/* Sets the default flags for the page allocator, the default is FLAG_ZERO */
+void page_alloc_set_default_flags(unsigned int flags);
+
 /*
  * Initializes a memory area.
  * n is the number of the area to initialize
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 4d5722f..08e0d05 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -54,12 +54,19 @@ static struct mem_area areas[MAX_AREAS];
 static unsigned int areas_mask;
 /* Protects areas and areas mask */
 static struct spinlock lock;
+/* Default behaviour: zero allocated pages */
+static unsigned int default_flags = FLAG_ZERO;
 
 bool page_alloc_initialized(void)
 {
 	return areas_mask != 0;
 }
 
+void page_alloc_set_default_flags(unsigned int flags)
+{
+	default_flags = flags;
+}
+
 /*
  * Each memory area contains an array of metadata entries at the very
  * beginning. The usable memory follows immediately afterwards.
@@ -394,6 +401,7 @@ static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
 	void *res = NULL;
 	int i, area, fresh;
 
+	flags |= default_flags;
 	fresh = !!(flags & FLAG_FRESH);
 	spin_lock(&lock);
 	area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;
-- 
2.26.2

Re: [kvm-unit-tests PATCH v1 01/12] lib/x86: fix page.h to include the generic header

[Thomas Huth]

On 16/12/2020 21.11, Claudio Imbrenda wrote:
> Bring x86 in line with the other architectures and include the generic header
> at asm-generic/page.h .
> This provides the macros PAGE_SHIFT, PAGE_SIZE, PAGE_MASK, virt_to_pfn, and
> pfn_to_virt.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/x86/asm/page.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
> index 1359eb7..2cf8881 100644
> --- a/lib/x86/asm/page.h
> +++ b/lib/x86/asm/page.h
> @@ -13,9 +13,7 @@
>  typedef unsigned long pteval_t;
>  typedef unsigned long pgd_t;
>  
> -#define PAGE_SHIFT	12
> -#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
> -#define PAGE_MASK	(~(PAGE_SIZE-1))
> +#include <asm-generic/page.h>
>  
>  #ifndef __ASSEMBLY__
>  
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v1 02/12] lib/list.h: add list_add_tail

[Thomas Huth]

On 16/12/2020 21.11, Claudio Imbrenda wrote:
> Add a list_add_tail wrapper function to allow adding elements to the end
> of a list.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/list.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/list.h b/lib/list.h
> index 18d9516..7f9717e 100644
> --- a/lib/list.h
> +++ b/lib/list.h
> @@ -50,4 +50,13 @@ static inline void list_add(struct linked_list *head, struct linked_list *li)
>  	head->next = li;
>  }
>  
> +/*
> + * Add the given element before the given list head.
> + */
> +static inline void list_add_tail(struct linked_list *head, struct linked_list *li)
> +{
> +	assert(head);
> +	list_add(head->prev, li);
> +}
> +
>  #endif
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator

[Nadav Amit]

> On Dec 16, 2020, at 12:11 PM, Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
> 
> My previous patchseries was rushed and not polished enough. Furthermore it
> introduced some regressions.
> 
> This patchseries fixes hopefully all the issues reported, and introduces
> some new features.
> 
> It also simplifies the code and hopefully makes it more readable.
> 
> Fixed:
> * allocated memory is now zeroed by default

Thanks for doing that. Before I test it, did you also fix the issue of x86’s
setup_vm() [1]?

[1] https://www.spinics.net/lists/kvm/msg230470.html

Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator

[Claudio Imbrenda]

On Thu, 17 Dec 2020 11:41:05 -0800
Nadav Amit <nadav.amit@gmail.com> wrote:

> > On Dec 16, 2020, at 12:11 PM, Claudio Imbrenda
> > <imbrenda@linux.ibm.com> wrote:
> > 
> > My previous patchseries was rushed and not polished enough.
> > Furthermore it introduced some regressions.
> > 
> > This patchseries fixes hopefully all the issues reported, and
> > introduces some new features.
> > 
> > It also simplifies the code and hopefully makes it more readable.
> > 
> > Fixed:
> > * allocated memory is now zeroed by default  
> 
> Thanks for doing that. Before I test it, did you also fix the issue
> of x86’s setup_vm() [1]?
> 
> [1] https://www.spinics.net/lists/kvm/msg230470.html

unfortunately no, because I could not reproduce it.

In theory setup_vm should just work, since it is called twice, but on
different address ranges.

The only issue I can think of is if the second call overlaps multiple
areas.

can you send me the memory map of that machine you're running the tests
on? (/proc/iomem)


Claudio

Re: [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements

[Krish Sadhukhan]

On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> Add some asserts to make sure the state is consistent.
>
> Simplify and improve the readability of vm_free.
>
> Fixes: 3f6fee0d4da4 ("lib/vmalloc: vmalloc support for handling allocation metadata")
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
>   lib/vmalloc.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> index 986a34c..7a49adf 100644
> --- a/lib/vmalloc.c
> +++ b/lib/vmalloc.c
> @@ -162,13 +162,14 @@ static void *vm_memalign(size_t alignment, size_t size)
>   static void vm_free(void *mem)
>   {
>   	struct metadata *m;
> -	uintptr_t ptr, end;
> +	uintptr_t ptr, page, i;
>   
>   	/* the pointer is not page-aligned, it was a single-page allocation */


Do we need an assert() for 'mem' if it is NULL for some reason ?

>   	if (!IS_ALIGNED((uintptr_t)mem, PAGE_SIZE)) {
>   		assert(GET_MAGIC(mem) == VM_MAGIC);
> -		ptr = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
> -		free_page(phys_to_virt(ptr));
> +		page = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
> +		assert(page);
> +		free_page(phys_to_virt(page));
>   		return;
>   	}
>   
> @@ -176,13 +177,14 @@ static void vm_free(void *mem)
>   	m = GET_METADATA(mem);
>   	assert(m->magic == VM_MAGIC);
>   	assert(m->npages > 0);
> +	assert(m->npages < BIT_ULL(BITS_PER_LONG - PAGE_SHIFT));


NIT:  Combine the two assert()s for 'npages' perhaps ?

>   	/* free all the pages including the metadata page */
> -	ptr = (uintptr_t)mem - PAGE_SIZE;
> -	end = ptr + m->npages * PAGE_SIZE;
> -	for ( ; ptr < end; ptr += PAGE_SIZE)
> -		free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
> -	/* free the last one separately to avoid overflow issues */
> -	free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
> +	ptr = (uintptr_t)m & PAGE_MASK;
> +	for (i = 0 ; i < m->npages + 1; i++, ptr += PAGE_SIZE) {
> +		page = virt_to_pte_phys(page_root, (void *)ptr) & PAGE_MASK;
> +		assert(page);
> +		free_page(phys_to_virt(page));
> +	}
>   }
>   
>   static struct alloc_ops vmalloc_ops = {

Re: [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas

[Krish Sadhukhan]

On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> Fix the definitions of the memory areas.
>
> Bring the headers in line with the rest of the asm headers, by having the
> appropriate #ifdef _ASM$ARCH_ guarding the headers.


Should we mention MAX_AREAS that the patch adds in each arch-specific 
header ?

> Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
>   lib/asm-generic/memory_areas.h |  9 ++++-----
>   lib/arm/asm/memory_areas.h     | 11 +++--------
>   lib/arm64/asm/memory_areas.h   | 11 +++--------
>   lib/powerpc/asm/memory_areas.h | 11 +++--------
>   lib/ppc64/asm/memory_areas.h   | 11 +++--------
>   lib/s390x/asm/memory_areas.h   | 13 ++++++-------
>   lib/x86/asm/memory_areas.h     | 27 ++++++++++++++++-----------
>   lib/alloc_page.h               |  3 +++
>   lib/alloc_page.c               |  4 +---
>   9 files changed, 42 insertions(+), 58 deletions(-)
>
> diff --git a/lib/asm-generic/memory_areas.h b/lib/asm-generic/memory_areas.h
> index 927baa7..3074afe 100644
> --- a/lib/asm-generic/memory_areas.h
> +++ b/lib/asm-generic/memory_areas.h
> @@ -1,11 +1,10 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
> +#define __ASM_GENERIC_MEMORY_AREAS_H__
>   
>   #define AREA_NORMAL_PFN 0
>   #define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>   
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#define MAX_AREAS 1
>   
>   #endif
> diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
> index 927baa7..c723310 100644
> --- a/lib/arm/asm/memory_areas.h
> +++ b/lib/arm/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMARM_MEMORY_AREAS_H_
> +#define _ASMARM_MEMORY_AREAS_H_
>   
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>   
>   #endif
> diff --git a/lib/arm64/asm/memory_areas.h b/lib/arm64/asm/memory_areas.h
> index 927baa7..18e8ca8 100644
> --- a/lib/arm64/asm/memory_areas.h
> +++ b/lib/arm64/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMARM64_MEMORY_AREAS_H_
> +#define _ASMARM64_MEMORY_AREAS_H_
>   
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>   
>   #endif
> diff --git a/lib/powerpc/asm/memory_areas.h b/lib/powerpc/asm/memory_areas.h
> index 927baa7..76d1738 100644
> --- a/lib/powerpc/asm/memory_areas.h
> +++ b/lib/powerpc/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
> +#define _ASMPOWERPC_MEMORY_AREAS_H_
>   
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>   
>   #endif
> diff --git a/lib/ppc64/asm/memory_areas.h b/lib/ppc64/asm/memory_areas.h
> index 927baa7..b9fd46b 100644
> --- a/lib/ppc64/asm/memory_areas.h
> +++ b/lib/ppc64/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMPPC64_MEMORY_AREAS_H_
> +#define _ASMPPC64_MEMORY_AREAS_H_
>   
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>   
>   #endif
> diff --git a/lib/s390x/asm/memory_areas.h b/lib/s390x/asm/memory_areas.h
> index 4856a27..827bfb3 100644
> --- a/lib/s390x/asm/memory_areas.h
> +++ b/lib/s390x/asm/memory_areas.h
> @@ -1,16 +1,15 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMS390X_MEMORY_AREAS_H_
> +#define _ASMS390X_MEMORY_AREAS_H_
>   
> -#define AREA_NORMAL_PFN BIT(31-12)
> +#define AREA_NORMAL_PFN (1 << 19)
>   #define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>   
>   #define AREA_LOW_PFN 0
>   #define AREA_LOW_NUMBER 1
> -#define AREA_LOW 2
> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>   
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#define MAX_AREAS 2
>   
>   #define AREA_DMA31 AREA_LOW
>   
> diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
> index 952f5bd..e84016f 100644
> --- a/lib/x86/asm/memory_areas.h
> +++ b/lib/x86/asm/memory_areas.h
> @@ -1,21 +1,26 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASM_X86_MEMORY_AREAS_H_
> +#define _ASM_X86_MEMORY_AREAS_H_
>   
>   #define AREA_NORMAL_PFN BIT(36-12)
>   #define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>   
> -#define AREA_PAE_HIGH_PFN BIT(32-12)
> -#define AREA_PAE_HIGH_NUMBER 1
> -#define AREA_PAE_HIGH 2
> +#define AREA_HIGH_PFN BIT(32-12)
> +#define AREA_HIGH_NUMBER 1
> +#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
>   
> -#define AREA_LOW_PFN 0
> +#define AREA_LOW_PFN BIT(24-12)
>   #define AREA_LOW_NUMBER 2
> -#define AREA_LOW 4
> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>   
> -#define AREA_PAE (AREA_PAE | AREA_LOW)
> +#define AREA_LOWEST_PFN 0
> +#define AREA_LOWEST_NUMBER 3
> +#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
>   
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#define MAX_AREAS 4
> +
> +#define AREA_DMA24 AREA_LOWEST
> +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
> +#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
>   
>   #endif
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index 816ff5d..b6aace5 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -10,6 +10,9 @@
>   
>   #include <asm/memory_areas.h>
>   
> +#define AREA_ANY -1
> +#define AREA_ANY_NUMBER 0xff
> +
>   /* Returns true if the page allocator has been initialized */
>   bool page_alloc_initialized(void);
>   
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 685ab1e..ed0ff02 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -19,8 +19,6 @@
>   #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
>   #define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>   
> -#define MAX_AREAS	6
> -
>   #define ORDER_MASK	0x3f
>   #define ALLOC_MASK	0x40
>   #define SPECIAL_MASK	0x80
> @@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
>   		return;
>   	}
>   #ifdef AREA_HIGH_PFN
> -	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN), base_pfn, &top_pfn);
> +	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN, base_pfn, &top_pfn);


Surprising that the compiler didn't complain !

>   #endif
>   	__page_alloc_init_area(AREA_NORMAL_NUMBER, AREA_NORMAL_PFN, base_pfn, &top_pfn);
>   #ifdef AREA_LOW_PFN

Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator

[Krish Sadhukhan]

On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> This patch introduces some improvements to the code, mostly readability
> improvements, but also some semantic details, and improvements in the
> documentation.
>
> * introduce and use pfn_t to semantically tag parameters as PFNs
> * remove the PFN macro, use virt_to_pfn instead
> * rename area_or_metadata_contains and area_contains to area_contains_pfn
>    and usable_area_contains_pfn respectively
> * fix/improve comments in lib/alloc_page.h
> * move some wrapper functions to the header
>
> Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
> Fixes: 34c950651861 ("lib/alloc_page: allow reserving arbitrary memory ranges")
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
>   lib/alloc_page.h |  49 +++++++++-----
>   lib/alloc_page.c | 165 +++++++++++++++++++++++------------------------
>   2 files changed, 116 insertions(+), 98 deletions(-)
>
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index b6aace5..d8550c6 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -8,6 +8,7 @@
>   #ifndef ALLOC_PAGE_H
>   #define ALLOC_PAGE_H 1
>   
> +#include <stdbool.h>
>   #include <asm/memory_areas.h>
>   
>   #define AREA_ANY -1
> @@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
>    * top_pfn is the physical frame number of the first page immediately after
>    * the end of the area to initialize
>    */
> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn);
> +void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn);
>   
>   /* Enables the page allocator. At least one area must have been initialized */
>   void page_alloc_ops_enable(void);
> @@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas, size_t alignment, size_t size);
>   
>   /*
>    * Allocate aligned memory from any area.
> - * Equivalent to memalign_pages_area(~0, alignment, size).
> + * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
>    */
> -void *memalign_pages(size_t alignment, size_t size);
> +static inline void *memalign_pages(size_t alignment, size_t size)
> +{
> +	return memalign_pages_area(AREA_ANY, alignment, size);
> +}
>   
>   /*
>    * Allocate naturally aligned memory from the specified areas.
> @@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t size);
>   void *alloc_pages_area(unsigned int areas, unsigned int order);
>   
>   /*
> - * Allocate one page from any area.
> - * Equivalent to alloc_pages(0);
> + * Allocate naturally aligned memory from any area.


This one allocates page size memory and the comment should reflect that.

> + * Equivalent to alloc_pages_area(AREA_ANY, order);
>    */
> -void *alloc_page(void);
> +static inline void *alloc_pages(unsigned int order)
> +{
> +	return alloc_pages_area(AREA_ANY, order);
> +}
>   
>   /*
> - * Allocate naturally aligned memory from any area.
> - * Equivalent to alloc_pages_area(~0, order);
> + * Allocate one page from any area.
> + * Equivalent to alloc_pages(0);
>    */
> -void *alloc_pages(unsigned int order);
> +static inline void *alloc_page(void)
> +{
> +	return alloc_pages(0);
> +}
>   
>   /*
>    * Frees a memory block allocated with any of the memalign_pages* or
> @@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
>    */
>   void free_pages(void *mem);
>   
> -/* For backwards compatibility */
> +/*
> + * Free one page.
> + * Equivalent to free_pages(mem).
> + */
>   static inline void free_page(void *mem)
>   {
>   	return free_pages(mem);
>   }
>   
> -/* For backwards compatibility */
> +/*
> + * Free pages by order.
> + * Equivalent to free_pages(mem).
> + */
>   static inline void free_pages_by_order(void *mem, unsigned int order)
>   {
>   	free_pages(mem);
>   }
>   
>   /*
> - * Allocates and reserves the specified memory range if possible.
> - * Returns NULL in case of failure.
> + * Allocates and reserves the specified physical memory range if possible.
> + * If the specified range cannot be reserved in its entirety, no action is
> + * performed and false is returned.
> + *
> + * Returns true in case of success, false otherwise.
>    */
> -void *alloc_pages_special(uintptr_t addr, size_t npages);
> +bool alloc_pages_special(phys_addr_t addr, size_t npages);
>   
>   /*
>    * Frees a reserved memory range that had been reserved with
> @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t npages);
>    * exactly, it can also be a subset, in which case only the specified
>    * pages will be freed and unreserved.
>    */
> -void free_pages_special(uintptr_t addr, size_t npages);
> +void free_pages_special(phys_addr_t addr, size_t npages);
>   
>   #endif
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index ed0ff02..8d2700d 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -17,25 +17,29 @@
>   
>   #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
>   #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
> -#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>   
>   #define ORDER_MASK	0x3f
>   #define ALLOC_MASK	0x40
>   #define SPECIAL_MASK	0x80
>   
> +typedef phys_addr_t pfn_t;
> +
>   struct mem_area {
>   	/* Physical frame number of the first usable frame in the area */
> -	uintptr_t base;
> +	pfn_t base;
>   	/* Physical frame number of the first frame outside the area */
> -	uintptr_t top;
> -	/* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
> +	pfn_t top;
> +	/* Per page metadata, each entry is a combination *_MASK and order */
>   	u8 *page_states;
>   	/* One freelist for each possible block size, up to NLISTS */
>   	struct linked_list freelists[NLISTS];
>   };
>   
> +/* Descriptors for each possible area */
>   static struct mem_area areas[MAX_AREAS];
> +/* Mask of initialized areas */
>   static unsigned int areas_mask;
> +/* Protects areas and areas mask */
>   static struct spinlock lock;
>   
>   bool page_alloc_initialized(void)
> @@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
>   	return areas_mask != 0;
>   }
>   
> -static inline bool area_or_metadata_contains(struct mem_area *a, uintptr_t pfn)
> +/*
> + * Each memory area contains an array of metadata entries at the very
> + * beginning. The usable memory follows immediately afterwards.
> + * This function returns true if the given pfn falls anywhere within the
> + * memory area, including the metadata area.
> + */
> +static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
>   {
> -	return (pfn >= PFN(a->page_states)) && (pfn < a->top);
> +	return (pfn >= virt_to_pfn(a->page_states)) && (pfn < a->top);
>   }
>   
> -static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
> +/*
> + * Each memory area contains an array of metadata entries at the very
> + * beginning. The usable memory follows immediately afterwards.
> + * This function returns true if the given pfn falls in the usable range of
> + * the given memory area.
> + */
> +static inline bool usable_area_contains_pfn(struct mem_area *a, pfn_t pfn)
>   {
>   	return (pfn >= a->base) && (pfn < a->top);
>   }
> @@ -69,21 +85,19 @@ static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
>    */
>   static void split(struct mem_area *a, void *addr)
>   {
> -	uintptr_t pfn = PFN(addr);
> -	struct linked_list *p;
> -	uintptr_t i, idx;
> +	pfn_t pfn = virt_to_pfn(addr);
> +	pfn_t i, idx;
>   	u8 order;
>   
> -	assert(a && area_contains(a, pfn));
> +	assert(a && usable_area_contains_pfn(a, pfn));
>   	idx = pfn - a->base;
>   	order = a->page_states[idx];
>   	assert(!(order & ~ORDER_MASK) && order && (order < NLISTS));
>   	assert(IS_ALIGNED_ORDER(pfn, order));
> -	assert(area_contains(a, pfn + BIT(order) - 1));
> +	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>   
>   	/* Remove the block from its free list */
> -	p = list_remove(addr);
> -	assert(p);
> +	list_remove(addr);
>   
>   	/* update the block size for each page in the block */
>   	for (i = 0; i < BIT(order); i++) {
> @@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
>   	}
>   	order--;
>   	/* add the first half block to the appropriate free list */
> -	list_add(a->freelists + order, p);
> +	list_add(a->freelists + order, addr);
>   	/* add the second half block to the appropriate free list */
> -	list_add(a->freelists + order, (void *)((pfn + BIT(order)) * PAGE_SIZE));
> +	list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
>   }
>   
>   /*
> @@ -105,7 +119,7 @@ static void split(struct mem_area *a, void *addr)
>    */
>   static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
>   {
> -	struct linked_list *p, *res = NULL;
> +	struct linked_list *p;
>   	u8 order;
>   
>   	assert((al < NLISTS) && (sz < NLISTS));
> @@ -130,17 +144,17 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
>   	for (; order > sz; order--)
>   		split(a, p);
>   
> -	res = list_remove(p);
> -	memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK | order, BIT(order));
> -	return res;
> +	list_remove(p);
> +	memset(a->page_states + (virt_to_pfn(p) - a->base), ALLOC_MASK | order, BIT(order));
> +	return p;
>   }
>   
> -static struct mem_area *get_area(uintptr_t pfn)
> +static struct mem_area *get_area(pfn_t pfn)
>   {
>   	uintptr_t i;
>   
>   	for (i = 0; i < MAX_AREAS; i++)
> -		if ((areas_mask & BIT(i)) && area_contains(areas + i, pfn))
> +		if ((areas_mask & BIT(i)) && usable_area_contains_pfn(areas + i, pfn))
>   			return areas + i;
>   	return NULL;
>   }
> @@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t pfn)
>    * - all of the pages of the two blocks must have the same block size
>    * - the function is called with the lock held
>    */
> -static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2)
> +static bool coalesce(struct mem_area *a, u8 order, pfn_t pfn, pfn_t pfn2)
>   {
> -	uintptr_t first, second, i;
> -	struct linked_list *li;
> +	pfn_t first, second, i;
>   
>   	assert(IS_ALIGNED_ORDER(pfn, order) && IS_ALIGNED_ORDER(pfn2, order));
>   	assert(pfn2 == pfn + BIT(order));
>   	assert(a);
>   
>   	/* attempting to coalesce two blocks that belong to different areas */
> -	if (!area_contains(a, pfn) || !area_contains(a, pfn2 + BIT(order) - 1))
> +	if (!usable_area_contains_pfn(a, pfn) || !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1))
>   		return false;
>   	first = pfn - a->base;
>   	second = pfn2 - a->base;
> @@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
>   		return false;
>   
>   	/* we can coalesce, remove both blocks from their freelists */
> -	li = list_remove((void *)(pfn2 << PAGE_SHIFT));
> -	assert(li);
> -	li = list_remove((void *)(pfn << PAGE_SHIFT));
> -	assert(li);
> +	list_remove(pfn_to_virt(pfn2));
> +	list_remove(pfn_to_virt(pfn));
>   	/* check the metadata entries and update with the new size */
>   	for (i = 0; i < (2ull << order); i++) {
>   		assert(a->page_states[first + i] == order);
>   		a->page_states[first + i] = order + 1;
>   	}
>   	/* finally add the newly coalesced block to the appropriate freelist */
> -	list_add(a->freelists + order + 1, li);
> +	list_add(a->freelists + order + 1, pfn_to_virt(pfn));
>   	return true;
>   }
>   
> @@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
>    */
>   static void _free_pages(void *mem)
>   {
> -	uintptr_t pfn2, pfn = PFN(mem);
> +	pfn_t pfn2, pfn = virt_to_pfn(mem);
>   	struct mem_area *a = NULL;
>   	uintptr_t i, p;
>   	u8 order;
> @@ -232,7 +243,7 @@ static void _free_pages(void *mem)
>   	/* ensure that the block is aligned properly for its size */
>   	assert(IS_ALIGNED_ORDER(pfn, order));
>   	/* ensure that the area can contain the whole block */
> -	assert(area_contains(a, pfn + BIT(order) - 1));
> +	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>   
>   	for (i = 0; i < BIT(order); i++) {
>   		/* check that all pages of the block have consistent metadata */
> @@ -268,63 +279,68 @@ void free_pages(void *mem)
>   	spin_unlock(&lock);
>   }
>   
> -static void *_alloc_page_special(uintptr_t addr)
> +static bool _alloc_page_special(pfn_t pfn)
>   {
>   	struct mem_area *a;
> -	uintptr_t mask, i;
> +	pfn_t mask, i;
>   
> -	a = get_area(PFN(addr));
> -	assert(a);
> -	i = PFN(addr) - a->base;
> +	a = get_area(pfn);
> +	if (!a)
> +		return false;
> +	i = pfn - a->base;
>   	if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
> -		return NULL;
> +		return false;
>   	while (a->page_states[i]) {
> -		mask = GENMASK_ULL(63, PAGE_SHIFT + a->page_states[i]);
> -		split(a, (void *)(addr & mask));
> +		mask = GENMASK_ULL(63, a->page_states[i]);
> +		split(a, pfn_to_virt(pfn & mask));
>   	}
>   	a->page_states[i] = SPECIAL_MASK;
> -	return (void *)addr;
> +	return true;
>   }
>   
> -static void _free_page_special(uintptr_t addr)
> +static void _free_page_special(pfn_t pfn)
>   {
>   	struct mem_area *a;
> -	uintptr_t i;
> +	pfn_t i;
>   
> -	a = get_area(PFN(addr));
> +	a = get_area(pfn);
>   	assert(a);
> -	i = PFN(addr) - a->base;
> +	i = pfn - a->base;
>   	assert(a->page_states[i] == SPECIAL_MASK);
>   	a->page_states[i] = ALLOC_MASK;
> -	_free_pages((void *)addr);
> +	_free_pages(pfn_to_virt(pfn));
>   }
>   
> -void *alloc_pages_special(uintptr_t addr, size_t n)
> +bool alloc_pages_special(phys_addr_t addr, size_t n)


The convention for these alloc functions seems to be that of returning 
void *. For example, alloc_pages_area(), alloc_pages() etc.  Probably we 
should maintain the convention or change all of their return type.

>   {
> -	uintptr_t i;
> +	pfn_t pfn;
> +	size_t i;
>   
>   	assert(IS_ALIGNED(addr, PAGE_SIZE));
> +	pfn = addr >> PAGE_SHIFT;
>   	spin_lock(&lock);
>   	for (i = 0; i < n; i++)
> -		if (!_alloc_page_special(addr + i * PAGE_SIZE))
> +		if (!_alloc_page_special(pfn + i))


Can the PFN macro be used here instead of the 'pfn' variable ?

>   			break;
>   	if (i < n) {
>   		for (n = 0 ; n < i; n++)
> -			_free_page_special(addr + n * PAGE_SIZE);
> -		addr = 0;
> +			_free_page_special(pfn + n);
> +		n = 0;
>   	}
>   	spin_unlock(&lock);
> -	return (void *)addr;
> +	return n;
>   }
>   
> -void free_pages_special(uintptr_t addr, size_t n)
> +void free_pages_special(phys_addr_t addr, size_t n)
>   {
> -	uintptr_t i;
> +	pfn_t pfn;
> +	size_t i;
>   
>   	assert(IS_ALIGNED(addr, PAGE_SIZE));
> +	pfn = addr >> PAGE_SHIFT;
>   	spin_lock(&lock);
>   	for (i = 0; i < n; i++)
> -		_free_page_special(addr + i * PAGE_SIZE);
> +		_free_page_special(pfn + i);


Can the PFN macro be used here instead of the 'pfn' variable ?

>   	spin_unlock(&lock);
>   }
>   
> @@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area, unsigned int order)
>   	return page_memalign_order_area(area, order, order);
>   }
>   
> -void *alloc_pages(unsigned int order)
> -{
> -	return alloc_pages_area(AREA_ANY, order);
> -}
> -
>   /*
>    * Allocates (1 << order) physically contiguous aligned pages.
>    * Returns NULL if the allocation was not possible.
> @@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
>   	return page_memalign_order_area(area, size, alignment);
>   }
>   
> -void *memalign_pages(size_t alignment, size_t size)
> -{
> -	return memalign_pages_area(AREA_ANY, alignment, size);
> -}
> -
> -/*
> - * Allocates one page
> - */
> -void *alloc_page()
> -{
> -	return alloc_pages(0);
> -}
>   
>   static struct alloc_ops page_alloc_ops = {
>   	.memalign = memalign_pages,
> @@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
>    * - the memory area to add does not overlap with existing areas
>    * - the memory area to add has at least 5 pages available
>    */
> -static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
> +static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
>   {
>   	size_t table_size, npages, i;
>   	struct mem_area *a;
> @@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
>   
>   	/* fill in the values of the new area */
>   	a = areas + n;
> -	a->page_states = (void *)(start_pfn << PAGE_SHIFT);
> +	a->page_states = pfn_to_virt(start_pfn);
>   	a->base = start_pfn + table_size;
>   	a->top = top_pfn;
>   	npages = top_pfn - a->base;
> @@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
>   	for (i = 0; i < MAX_AREAS; i++) {
>   		if (!(areas_mask & BIT(i)))
>   			continue;
> -		assert(!area_or_metadata_contains(areas + i, start_pfn));
> -		assert(!area_or_metadata_contains(areas + i, top_pfn - 1));
> -		assert(!area_or_metadata_contains(a, PFN(areas[i].page_states)));
> -		assert(!area_or_metadata_contains(a, areas[i].top - 1));
> +		assert(!area_contains_pfn(areas + i, start_pfn));
> +		assert(!area_contains_pfn(areas + i, top_pfn - 1));
> +		assert(!area_contains_pfn(a, virt_to_pfn(areas[i].page_states)));
> +		assert(!area_contains_pfn(a, areas[i].top - 1));
>   	}
>   	/* initialize all freelists for the new area */
>   	for (i = 0; i < NLISTS; i++)
> -		a->freelists[i].next = a->freelists[i].prev = a->freelists + i;
> +		a->freelists[i].prev = a->freelists[i].next = a->freelists + i;
>   
>   	/* initialize the metadata for the available memory */
>   	for (i = a->base; i < a->top; i += 1ull << order) {
> @@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
>   		assert(order < NLISTS);
>   		/* initialize the metadata and add to the freelist */
>   		memset(a->page_states + (i - a->base), order, BIT(order));
> -		list_add(a->freelists + order, (void *)(i << PAGE_SHIFT));
> +		list_add(a->freelists + order, pfn_to_virt(i));
>   	}
>   	/* finally mark the area as present */
>   	areas_mask |= BIT(n);
>   }
>   
> -static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, uintptr_t *top_pfn)
> +static void __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t *top_pfn)
>   {
>   	if (*top_pfn > cutoff) {
>   		spin_lock(&lock);
> @@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, u
>    * Prerequisites:
>    * see _page_alloc_init_area
>    */
> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
> +void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn)
>   {
>   	if (n != AREA_ANY_NUMBER) {
>   		__page_alloc_init_area(n, 0, base_pfn, &top_pfn);

Re: [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops

[Krish Sadhukhan]

On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> Remove align_min from struct alloc_ops, since it is no longer used.
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
>   lib/alloc.h      | 1 -
>   lib/alloc_page.c | 1 -
>   lib/alloc_phys.c | 9 +++++----
>   lib/vmalloc.c    | 1 -
>   4 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/lib/alloc.h b/lib/alloc.h
> index 9b4b634..db90b01 100644
> --- a/lib/alloc.h
> +++ b/lib/alloc.h
> @@ -25,7 +25,6 @@
>   struct alloc_ops {
>   	void *(*memalign)(size_t alignment, size_t size);
>   	void (*free)(void *ptr);
> -	size_t align_min;
>   };
>   
>   extern struct alloc_ops *alloc_ops;
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 8d2700d..b1cdf21 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
>   static struct alloc_ops page_alloc_ops = {
>   	.memalign = memalign_pages,
>   	.free = free_pages,
> -	.align_min = PAGE_SIZE,
>   };
>   
>   /*
> diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
> index 72e20f7..a4d2bf2 100644
> --- a/lib/alloc_phys.c
> +++ b/lib/alloc_phys.c
> @@ -29,8 +29,8 @@ static phys_addr_t base, top;
>   static void *early_memalign(size_t alignment, size_t size);
>   static struct alloc_ops early_alloc_ops = {
>   	.memalign = early_memalign,
> -	.align_min = DEFAULT_MINIMUM_ALIGNMENT
>   };
> +static size_t align_min;


I don't see any caller of phys_alloc_set_minimum_alignment(). So when 
you are comparing against this variable in phys_alloc_aligned_safe() 
below, you are comparing against zero. Is that what you is intended or 
should 'align_min' be set some default ?

>   
>   struct alloc_ops *alloc_ops = &early_alloc_ops;
>   
> @@ -39,8 +39,7 @@ void phys_alloc_show(void)
>   	int i;
>   
>   	spin_lock(&lock);
> -	printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
> -		(u64)early_alloc_ops.align_min);
> +	printf("phys_alloc minimum alignment: %#" PRIx64 "\n", (u64)align_min);
>   	for (i = 0; i < nr_regions; ++i)
>   		printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
>   			(u64)regions[i].base,
> @@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t align)
>   {
>   	assert(align && !(align & (align - 1)));
>   	spin_lock(&lock);
> -	early_alloc_ops.align_min = align;
> +	align_min = align;
>   	spin_unlock(&lock);
>   }
>   
> @@ -83,6 +82,8 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
>   		top_safe = MIN(top_safe, 1ULL << 32);
>   
>   	assert(base < top_safe);
> +	if (align < align_min)
> +		align = align_min;
>   
>   	addr = ALIGN(base, align);
>   	size += addr - base;
> diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> index 7a49adf..e146162 100644
> --- a/lib/vmalloc.c
> +++ b/lib/vmalloc.c
> @@ -190,7 +190,6 @@ static void vm_free(void *mem)
>   static struct alloc_ops vmalloc_ops = {
>   	.memalign = vm_memalign,
>   	.free = vm_free,
> -	.align_min = PAGE_SIZE,
>   };
>   
>   void __attribute__((__weak__)) find_highmem(void)

Re: [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default

[Krish Sadhukhan]

On 12/16/20 12:12 PM, Claudio Imbrenda wrote:
> The new function page_alloc_set_default_flags can be used to set the
> default flags for allocations. The passed value will be ORed with the
> flags argument passed to the allocator at each allocation.
>
> The default value for the default flags is FLAG_ZERO, which means that
> by default all allocated memory is now zeroed, restoring the default
> behaviour that had been accidentally removed by a previous commit.
>
> If needed, a testcase can call page_alloc_set_default_flags(0) in order
> to get non-zeroed pages from the allocator. For example, if the
> testcase will need fresh memory, the zero flag should be removed from
> the default.
>
> Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
> Reported-by: Nadav Amit<nadav.amit@gmail.com>
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
>   lib/alloc_page.h | 3 +++
>   lib/alloc_page.c | 8 ++++++++
>   2 files changed, 11 insertions(+)
>
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index 1039814..8b53a58 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -22,6 +22,9 @@
>   /* Returns true if the page allocator has been initialized */
>   bool page_alloc_initialized(void);
>   
> +/* Sets the default flags for the page allocator, the default is FLAG_ZERO */
> +void page_alloc_set_default_flags(unsigned int flags);
> +
>   /*
>    * Initializes a memory area.
>    * n is the number of the area to initialize
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 4d5722f..08e0d05 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -54,12 +54,19 @@ static struct mem_area areas[MAX_AREAS];
>   static unsigned int areas_mask;
>   /* Protects areas and areas mask */
>   static struct spinlock lock;
> +/* Default behaviour: zero allocated pages */
> +static unsigned int default_flags = FLAG_ZERO;
>   
>   bool page_alloc_initialized(void)
>   {
>   	return areas_mask != 0;
>   }
>   
> +void page_alloc_set_default_flags(unsigned int flags)
> +{
> +	default_flags = flags;


Who calls this functions ?

Just wondering if default flag should be a static set of flag values 
which the caller can override based on needs rather than the caller 
setting the default flag.

> +}
> +
>   /*
>    * Each memory area contains an array of metadata entries at the very
>    * beginning. The usable memory follows immediately afterwards.
> @@ -394,6 +401,7 @@ static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
>   	void *res = NULL;
>   	int i, area, fresh;
>   
> +	flags |= default_flags;
>   	fresh = !!(flags & FLAG_FRESH);
>   	spin_lock(&lock);
>   	area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;

Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator

[Krish Sadhukhan]

On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> My previous patchseries was rushed and not polished enough. Furthermore it
> introduced some regressions.
>
> This patchseries fixes hopefully all the issues reported, and introduces
> some new features.
>
> It also simplifies the code and hopefully makes it more readable.
>
> Fixed:
> * allocated memory is now zeroed by default
>
> New features:
> * per-allocation flags to specify not just the area (like before) but also
>    other parameters
>    - zero flag: the allocation will be zeroed
>    - fresh flag: the returned memory has never been read or written to before
> * default flags: it's possible to specify which flags should be enabled
>    automatically at each allocation; by default the zero flag is set.
>
>
> I would appreciate if people could test these patches, especially on
> strange, unusual or exotic hardware (I tested only on s390x)
>
>
> GitLab:
>    https://urldefense.com/v3/__https://gitlab.com/imbrenda/kvm-unit-tests/-/tree/page_allocator_fixes__;!!GqivPVa7Brio!LehVoD4e6fUc92A7OE_Rxl2QwwkrW4aY0WHTmPkgKyxYviNfnTs3hEmYWHsMj3I9paC-$
> CI:
>    https://urldefense.com/v3/__https://gitlab.com/imbrenda/kvm-unit-tests/-/pipelines/229689192__;!!GqivPVa7Brio!LehVoD4e6fUc92A7OE_Rxl2QwwkrW4aY0WHTmPkgKyxYviNfnTs3hEmYWHsMj5GdDaIf$
>
>
> Claudio Imbrenda (12):
>    lib/x86: fix page.h to include the generic header
>    lib/list.h: add list_add_tail
>    lib/vmalloc: add some asserts and improvements
>    lib/asm: Fix definitions of memory areas
>    lib/alloc_page: fix and improve the page allocator
>    lib/alloc.h: remove align_min from struct alloc_ops
>    lib/alloc_page: Optimization to skip known empty freelists
>    lib/alloc_page: rework metadata format
>    lib/alloc: replace areas with more generic flags
>    lib/alloc_page: Wire up ZERO_FLAG
>    lib/alloc_page: Properly handle requests for fresh blocks
>    lib/alloc_page: default flags and zero pages by default
>
>   lib/asm-generic/memory_areas.h |   9 +-
>   lib/arm/asm/memory_areas.h     |  11 +-
>   lib/arm64/asm/memory_areas.h   |  11 +-
>   lib/powerpc/asm/memory_areas.h |  11 +-
>   lib/ppc64/asm/memory_areas.h   |  11 +-
>   lib/s390x/asm/memory_areas.h   |  13 +-
>   lib/x86/asm/memory_areas.h     |  27 +--
>   lib/x86/asm/page.h             |   4 +-
>   lib/alloc.h                    |   1 -
>   lib/alloc_page.h               |  66 ++++++--
>   lib/list.h                     |   9 +
>   lib/alloc_page.c               | 291 ++++++++++++++++++++-------------
>   lib/alloc_phys.c               |   9 +-
>   lib/s390x/smp.c                |   2 +-
>   lib/vmalloc.c                  |  21 +--
>   15 files changed, 286 insertions(+), 210 deletions(-)
>
For patch# 1, 2, 7, 8, 9, 10 and 11:

     Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator

[Nadav Amit]

> On Dec 18, 2020, at 6:19 AM, Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
> 
> On Thu, 17 Dec 2020 11:41:05 -0800
> Nadav Amit <nadav.amit@gmail.com> wrote:
> 
>>> On Dec 16, 2020, at 12:11 PM, Claudio Imbrenda
>>> <imbrenda@linux.ibm.com> wrote:
>>> 
>>> My previous patchseries was rushed and not polished enough.
>>> Furthermore it introduced some regressions.
>>> 
>>> This patchseries fixes hopefully all the issues reported, and
>>> introduces some new features.
>>> 
>>> It also simplifies the code and hopefully makes it more readable.
>>> 
>>> Fixed:
>>> * allocated memory is now zeroed by default  
>> 
>> Thanks for doing that. Before I test it, did you also fix the issue
>> of x86’s setup_vm() [1]?
>> 
>> [1] https://www.spinics.net/lists/kvm/msg230470.html
> 
> unfortunately no, because I could not reproduce it.
> 
> In theory setup_vm should just work, since it is called twice, but on
> different address ranges.
> 
> The only issue I can think of is if the second call overlaps multiple
> areas.
> 
> can you send me the memory map of that machine you're running the tests
> on? (/proc/iomem)

Sorry for the late response.

I see two calls to _page_alloc_init_area() with AREA_LOW_NUMBER, one with
(base_pfn=621, top_pfn=bfdd0) and one with (base_pfn=100000 top_pfn=240000).

As for /proc/iomem:

00000000-00000fff : Reserved
00001000-0009e7ff : System RAM
0009e800-0009ffff : Reserved
000a0000-000bffff : PCI Bus 0000:00
000c0000-000c7fff : Video ROM
000ca000-000cafff : Adapter ROM
000cb000-000ccfff : Adapter ROM
000d0000-000d3fff : PCI Bus 0000:00
000d4000-000d7fff : PCI Bus 0000:00
000d8000-000dbfff : PCI Bus 0000:00
000dc000-000fffff : Reserved
  000f0000-000fffff : System ROM
00100000-bfecffff : System RAM
  01000000-01c031d0 : Kernel code
  01c031d1-0266f3bf : Kernel data
  028ef000-02b97fff : Kernel bss
bfed0000-bfefefff : ACPI Tables
bfeff000-bfefffff : ACPI Non-volatile Storage
bff00000-bfffffff : System RAM
c0000000-febfffff : PCI Bus 0000:00
  c0008000-c000bfff : 0000:00:10.0
  e5b00000-e5bfffff : PCI Bus 0000:22
  e5c00000-e5cfffff : PCI Bus 0000:1a
  e5d00000-e5dfffff : PCI Bus 0000:12
  e5e00000-e5efffff : PCI Bus 0000:0a
  e5f00000-e5ffffff : PCI Bus 0000:21
  e6000000-e60fffff : PCI Bus 0000:19
  e6100000-e61fffff : PCI Bus 0000:11
  e6200000-e62fffff : PCI Bus 0000:09
  e6300000-e63fffff : PCI Bus 0000:20
  e6400000-e64fffff : PCI Bus 0000:18
  e6500000-e65fffff : PCI Bus 0000:10
  e6600000-e66fffff : PCI Bus 0000:08
  e6700000-e67fffff : PCI Bus 0000:1f
  e6800000-e68fffff : PCI Bus 0000:17
  e6900000-e69fffff : PCI Bus 0000:0f
  e6a00000-e6afffff : PCI Bus 0000:07
  e6b00000-e6bfffff : PCI Bus 0000:1e
  e6c00000-e6cfffff : PCI Bus 0000:16
  e6d00000-e6dfffff : PCI Bus 0000:0e
  e6e00000-e6efffff : PCI Bus 0000:06
  e6f00000-e6ffffff : PCI Bus 0000:1d
  e7000000-e70fffff : PCI Bus 0000:15
  e7100000-e71fffff : PCI Bus 0000:0d
  e7200000-e72fffff : PCI Bus 0000:05
  e7300000-e73fffff : PCI Bus 0000:1c
  e7400000-e74fffff : PCI Bus 0000:14
  e7500000-e75fffff : PCI Bus 0000:0c
  e7600000-e76fffff : PCI Bus 0000:04
  e7700000-e77fffff : PCI Bus 0000:1b
  e7800000-e78fffff : PCI Bus 0000:13
  e7900000-e79fffff : PCI Bus 0000:0b
  e7a00000-e7afffff : PCI Bus 0000:03
  e7b00000-e7ffffff : PCI Bus 0000:02
  e8000000-efffffff : 0000:00:0f.0
    e8000000-efffffff : vmwgfx probe
  f0000000-f7ffffff : PCI MMCONFIG 0000 [bus 00-7f]
    f0000000-f7ffffff : Reserved
      f0000000-f7ffffff : pnp 00:08
  fb500000-fb5fffff : PCI Bus 0000:22
  fb600000-fb6fffff : PCI Bus 0000:1a
  fb700000-fb7fffff : PCI Bus 0000:12
  fb800000-fb8fffff : PCI Bus 0000:0a
  fb900000-fb9fffff : PCI Bus 0000:21
  fba00000-fbafffff : PCI Bus 0000:19
  fbb00000-fbbfffff : PCI Bus 0000:11
  fbc00000-fbcfffff : PCI Bus 0000:09
  fbd00000-fbdfffff : PCI Bus 0000:20
  fbe00000-fbefffff : PCI Bus 0000:18
  fbf00000-fbffffff : PCI Bus 0000:10
  fc000000-fc0fffff : PCI Bus 0000:08
  fc100000-fc1fffff : PCI Bus 0000:1f
  fc200000-fc2fffff : PCI Bus 0000:17
  fc300000-fc3fffff : PCI Bus 0000:0f
  fc400000-fc4fffff : PCI Bus 0000:07
  fc500000-fc5fffff : PCI Bus 0000:1e
  fc600000-fc6fffff : PCI Bus 0000:16
  fc700000-fc7fffff : PCI Bus 0000:0e
  fc800000-fc8fffff : PCI Bus 0000:06
  fc900000-fc9fffff : PCI Bus 0000:1d
  fca00000-fcafffff : PCI Bus 0000:15
  fcb00000-fcbfffff : PCI Bus 0000:0d
  fcc00000-fccfffff : PCI Bus 0000:05
  fcd00000-fcdfffff : PCI Bus 0000:1c
  fce00000-fcefffff : PCI Bus 0000:14
  fcf00000-fcffffff : PCI Bus 0000:0c
  fd000000-fd0fffff : PCI Bus 0000:04
  fd100000-fd1fffff : PCI Bus 0000:1b
  fd200000-fd2fffff : PCI Bus 0000:13
  fd300000-fd3fffff : PCI Bus 0000:0b
  fd400000-fd4fffff : PCI Bus 0000:03
  fd500000-fdffffff : PCI Bus 0000:02
    fd500000-fd50ffff : 0000:02:01.0
    fd510000-fd51ffff : 0000:02:05.0
    fd5c0000-fd5dffff : 0000:02:01.0
      fd5c0000-fd5dffff : e1000
    fd5ee000-fd5eefff : 0000:02:05.0
      fd5ee000-fd5eefff : ahci
    fd5ef000-fd5effff : 0000:02:03.0
      fd5ef000-fd5effff : ehci_hcd
    fdff0000-fdffffff : 0000:02:01.0
      fdff0000-fdffffff : e1000
  fe000000-fe7fffff : 0000:00:0f.0
    fe000000-fe7fffff : vmwgfx probe
  fe800000-fe9fffff : pnp 00:08
  feba0000-febbffff : 0000:00:10.0
    feba0000-febbffff : mpt
  febc0000-febdffff : 0000:00:10.0
    febc0000-febdffff : mpt
  febfe000-febfffff : 0000:00:07.7
fec00000-fec0ffff : Reserved
  fec00000-fec003ff : IOAPIC 0
fec10000-fec10fff : dmar0
fed00000-fed003ff : HPET 0
  fed00000-fed003ff : pnp 00:04
fee00000-fee00fff : Local APIC
  fee00000-fee00fff : Reserved
fffe0000-ffffffff : Reserved
100000000-23fffffff : System RAM

Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator

[Sean Christopherson]

On Wed, Dec 16, 2020, Claudio Imbrenda wrote:
>  /*
> - * Allocates and reserves the specified memory range if possible.
> - * Returns NULL in case of failure.
> + * Allocates and reserves the specified physical memory range if possible.
> + * If the specified range cannot be reserved in its entirety, no action is
> + * performed and false is returned.
> + *
> + * Returns true in case of success, false otherwise.
>   */
> -void *alloc_pages_special(uintptr_t addr, size_t npages);
> +bool alloc_pages_special(phys_addr_t addr, size_t npages);

The boolean return is a bit awkward as kernel programmers will likely expect a
non-zero return to mean failure.  But, since there are no users, can we simply
drop the entire *_pages_special() API?  Allocating a specific PFN that isn't
MMIO seems doomed to fail anyways; I'm having a hard time envisioning a test
that would be able to use such an API without being horribly fragile.

>  
>  /*
>   * Frees a reserved memory range that had been reserved with
> @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t npages);
>   * exactly, it can also be a subset, in which case only the specified
>   * pages will be freed and unreserved.
>   */
> -void free_pages_special(uintptr_t addr, size_t npages);
> +void free_pages_special(phys_addr_t addr, size_t npages);

Re: [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops

[Claudio Imbrenda]

On Thu, 24 Dec 2020 10:17:20 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:

> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > Remove align_min from struct alloc_ops, since it is no longer used.
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> >   lib/alloc.h      | 1 -
> >   lib/alloc_page.c | 1 -
> >   lib/alloc_phys.c | 9 +++++----
> >   lib/vmalloc.c    | 1 -
> >   4 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/alloc.h b/lib/alloc.h
> > index 9b4b634..db90b01 100644
> > --- a/lib/alloc.h
> > +++ b/lib/alloc.h
> > @@ -25,7 +25,6 @@
> >   struct alloc_ops {
> >   	void *(*memalign)(size_t alignment, size_t size);
> >   	void (*free)(void *ptr);
> > -	size_t align_min;
> >   };
> >   
> >   extern struct alloc_ops *alloc_ops;
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index 8d2700d..b1cdf21 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area,
> > size_t alignment, size_t size) static struct alloc_ops
> > page_alloc_ops = { .memalign = memalign_pages,
> >   	.free = free_pages,
> > -	.align_min = PAGE_SIZE,
> >   };
> >   
> >   /*
> > diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
> > index 72e20f7..a4d2bf2 100644
> > --- a/lib/alloc_phys.c
> > +++ b/lib/alloc_phys.c
> > @@ -29,8 +29,8 @@ static phys_addr_t base, top;
> >   static void *early_memalign(size_t alignment, size_t size);
> >   static struct alloc_ops early_alloc_ops = {
> >   	.memalign = early_memalign,
> > -	.align_min = DEFAULT_MINIMUM_ALIGNMENT
> >   };
> > +static size_t align_min;  
> 
> 
> I don't see any caller of phys_alloc_set_minimum_alignment(). So when 

lib/arm/setup.c:150
lib/powerpc/setup.c:150

> you are comparing against this variable in phys_alloc_aligned_safe() 
> below, you are comparing against zero. Is that what you is intended
> or should 'align_min' be set some default ?

if the architecture specific code did not specify anything better, 0 is
ok.
 
> >   
> >   struct alloc_ops *alloc_ops = &early_alloc_ops;
> >   
> > @@ -39,8 +39,7 @@ void phys_alloc_show(void)
> >   	int i;
> >   
> >   	spin_lock(&lock);
> > -	printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
> > -		(u64)early_alloc_ops.align_min);
> > +	printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
> > (u64)align_min); for (i = 0; i < nr_regions; ++i)
> >   		printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
> >   			(u64)regions[i].base,
> > @@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t
> > align) {
> >   	assert(align && !(align & (align - 1)));
> >   	spin_lock(&lock);
> > -	early_alloc_ops.align_min = align;
> > +	align_min = align;
> >   	spin_unlock(&lock);
> >   }
> >   
> > @@ -83,6 +82,8 @@ static phys_addr_t
> > phys_alloc_aligned_safe(phys_addr_t size, top_safe = MIN(top_safe,
> > 1ULL << 32); 
> >   	assert(base < top_safe);
> > +	if (align < align_min)
> > +		align = align_min;
> >   
> >   	addr = ALIGN(base, align);
> >   	size += addr - base;
> > diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> > index 7a49adf..e146162 100644
> > --- a/lib/vmalloc.c
> > +++ b/lib/vmalloc.c
> > @@ -190,7 +190,6 @@ static void vm_free(void *mem)
> >   static struct alloc_ops vmalloc_ops = {
> >   	.memalign = vm_memalign,
> >   	.free = vm_free,
> > -	.align_min = PAGE_SIZE,
> >   };
> >   
> >   void __attribute__((__weak__)) find_highmem(void)  

Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator

[Claudio Imbrenda]

On Thu, 24 Dec 2020 10:17:06 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:

> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > This patch introduces some improvements to the code, mostly
> > readability improvements, but also some semantic details, and
> > improvements in the documentation.
> >
> > * introduce and use pfn_t to semantically tag parameters as PFNs
> > * remove the PFN macro, use virt_to_pfn instead
> > * rename area_or_metadata_contains and area_contains to
> > area_contains_pfn and usable_area_contains_pfn respectively
> > * fix/improve comments in lib/alloc_page.h
> > * move some wrapper functions to the header
> >
> > Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page
> > allocator") Fixes: 34c950651861 ("lib/alloc_page: allow reserving
> > arbitrary memory ranges")
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> >   lib/alloc_page.h |  49 +++++++++-----
> >   lib/alloc_page.c | 165
> > +++++++++++++++++++++++------------------------ 2 files changed,
> > 116 insertions(+), 98 deletions(-)
> >
> > diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> > index b6aace5..d8550c6 100644
> > --- a/lib/alloc_page.h
> > +++ b/lib/alloc_page.h
> > @@ -8,6 +8,7 @@
> >   #ifndef ALLOC_PAGE_H
> >   #define ALLOC_PAGE_H 1
> >   
> > +#include <stdbool.h>
> >   #include <asm/memory_areas.h>
> >   
> >   #define AREA_ANY -1
> > @@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
> >    * top_pfn is the physical frame number of the first page
> > immediately after
> >    * the end of the area to initialize
> >    */
> > -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
> > top_pfn); +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
> > phys_addr_t top_pfn); 
> >   /* Enables the page allocator. At least one area must have been
> > initialized */ void page_alloc_ops_enable(void);
> > @@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas,
> > size_t alignment, size_t size); 
> >   /*
> >    * Allocate aligned memory from any area.
> > - * Equivalent to memalign_pages_area(~0, alignment, size).
> > + * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
> >    */
> > -void *memalign_pages(size_t alignment, size_t size);
> > +static inline void *memalign_pages(size_t alignment, size_t size)
> > +{
> > +	return memalign_pages_area(AREA_ANY, alignment, size);
> > +}
> >   
> >   /*
> >    * Allocate naturally aligned memory from the specified areas.
> > @@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t
> > size); void *alloc_pages_area(unsigned int areas, unsigned int
> > order); 
> >   /*
> > - * Allocate one page from any area.
> > - * Equivalent to alloc_pages(0);
> > + * Allocate naturally aligned memory from any area.  
> 
> 
> This one allocates page size memory and the comment should reflect
> that.

I'll fix the comment
 
> > + * Equivalent to alloc_pages_area(AREA_ANY, order);
> >    */
> > -void *alloc_page(void);
> > +static inline void *alloc_pages(unsigned int order)
> > +{
> > +	return alloc_pages_area(AREA_ANY, order);
> > +}
> >   
> >   /*
> > - * Allocate naturally aligned memory from any area.
> > - * Equivalent to alloc_pages_area(~0, order);
> > + * Allocate one page from any area.
> > + * Equivalent to alloc_pages(0);
> >    */
> > -void *alloc_pages(unsigned int order);
> > +static inline void *alloc_page(void)
> > +{
> > +	return alloc_pages(0);
> > +}
> >   
> >   /*
> >    * Frees a memory block allocated with any of the memalign_pages*
> > or @@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
> >    */
> >   void free_pages(void *mem);
> >   
> > -/* For backwards compatibility */
> > +/*
> > + * Free one page.
> > + * Equivalent to free_pages(mem).
> > + */
> >   static inline void free_page(void *mem)
> >   {
> >   	return free_pages(mem);
> >   }
> >   
> > -/* For backwards compatibility */
> > +/*
> > + * Free pages by order.
> > + * Equivalent to free_pages(mem).
> > + */
> >   static inline void free_pages_by_order(void *mem, unsigned int
> > order) {
> >   	free_pages(mem);
> >   }
> >   
> >   /*
> > - * Allocates and reserves the specified memory range if possible.
> > - * Returns NULL in case of failure.
> > + * Allocates and reserves the specified physical memory range if
> > possible.
> > + * If the specified range cannot be reserved in its entirety, no
> > action is
> > + * performed and false is returned.
> > + *
> > + * Returns true in case of success, false otherwise.
> >    */
> > -void *alloc_pages_special(uintptr_t addr, size_t npages);
> > +bool alloc_pages_special(phys_addr_t addr, size_t npages);
> >   
> >   /*
> >    * Frees a reserved memory range that had been reserved with
> > @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t
> > npages);
> >    * exactly, it can also be a subset, in which case only the
> > specified
> >    * pages will be freed and unreserved.
> >    */
> > -void free_pages_special(uintptr_t addr, size_t npages);
> > +void free_pages_special(phys_addr_t addr, size_t npages);
> >   
> >   #endif
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index ed0ff02..8d2700d 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -17,25 +17,29 @@
> >   
> >   #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
> >   #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
> > -#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
> >   
> >   #define ORDER_MASK	0x3f
> >   #define ALLOC_MASK	0x40
> >   #define SPECIAL_MASK	0x80
> >   
> > +typedef phys_addr_t pfn_t;
> > +
> >   struct mem_area {
> >   	/* Physical frame number of the first usable frame in the
> > area */
> > -	uintptr_t base;
> > +	pfn_t base;
> >   	/* Physical frame number of the first frame outside the
> > area */
> > -	uintptr_t top;
> > -	/* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
> > +	pfn_t top;
> > +	/* Per page metadata, each entry is a combination *_MASK
> > and order */ u8 *page_states;
> >   	/* One freelist for each possible block size, up to
> > NLISTS */ struct linked_list freelists[NLISTS];
> >   };
> >   
> > +/* Descriptors for each possible area */
> >   static struct mem_area areas[MAX_AREAS];
> > +/* Mask of initialized areas */
> >   static unsigned int areas_mask;
> > +/* Protects areas and areas mask */
> >   static struct spinlock lock;
> >   
> >   bool page_alloc_initialized(void)
> > @@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
> >   	return areas_mask != 0;
> >   }
> >   
> > -static inline bool area_or_metadata_contains(struct mem_area *a,
> > uintptr_t pfn) +/*
> > + * Each memory area contains an array of metadata entries at the
> > very
> > + * beginning. The usable memory follows immediately afterwards.
> > + * This function returns true if the given pfn falls anywhere
> > within the
> > + * memory area, including the metadata area.
> > + */
> > +static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
> >   {
> > -	return (pfn >= PFN(a->page_states)) && (pfn < a->top);
> > +	return (pfn >= virt_to_pfn(a->page_states)) && (pfn <
> > a->top); }
> >   
> > -static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
> > +/*
> > + * Each memory area contains an array of metadata entries at the
> > very
> > + * beginning. The usable memory follows immediately afterwards.
> > + * This function returns true if the given pfn falls in the usable
> > range of
> > + * the given memory area.
> > + */
> > +static inline bool usable_area_contains_pfn(struct mem_area *a,
> > pfn_t pfn) {
> >   	return (pfn >= a->base) && (pfn < a->top);
> >   }
> > @@ -69,21 +85,19 @@ static inline bool area_contains(struct
> > mem_area *a, uintptr_t pfn) */
> >   static void split(struct mem_area *a, void *addr)
> >   {
> > -	uintptr_t pfn = PFN(addr);
> > -	struct linked_list *p;
> > -	uintptr_t i, idx;
> > +	pfn_t pfn = virt_to_pfn(addr);
> > +	pfn_t i, idx;
> >   	u8 order;
> >   
> > -	assert(a && area_contains(a, pfn));
> > +	assert(a && usable_area_contains_pfn(a, pfn));
> >   	idx = pfn - a->base;
> >   	order = a->page_states[idx];
> >   	assert(!(order & ~ORDER_MASK) && order && (order <
> > NLISTS)); assert(IS_ALIGNED_ORDER(pfn, order));
> > -	assert(area_contains(a, pfn + BIT(order) - 1));
> > +	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
> >   
> >   	/* Remove the block from its free list */
> > -	p = list_remove(addr);
> > -	assert(p);
> > +	list_remove(addr);
> >   
> >   	/* update the block size for each page in the block */
> >   	for (i = 0; i < BIT(order); i++) {
> > @@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
> >   	}
> >   	order--;
> >   	/* add the first half block to the appropriate free list
> > */
> > -	list_add(a->freelists + order, p);
> > +	list_add(a->freelists + order, addr);
> >   	/* add the second half block to the appropriate free list
> > */
> > -	list_add(a->freelists + order, (void *)((pfn + BIT(order))
> > * PAGE_SIZE));
> > +	list_add(a->freelists + order, pfn_to_virt(pfn +
> > BIT(order))); }
> >   
> >   /*
> > @@ -105,7 +119,7 @@ static void split(struct mem_area *a, void
> > *addr) */
> >   static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
> >   {
> > -	struct linked_list *p, *res = NULL;
> > +	struct linked_list *p;
> >   	u8 order;
> >   
> >   	assert((al < NLISTS) && (sz < NLISTS));
> > @@ -130,17 +144,17 @@ static void *page_memalign_order(struct
> > mem_area *a, u8 al, u8 sz) for (; order > sz; order--)
> >   		split(a, p);
> >   
> > -	res = list_remove(p);
> > -	memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK |
> > order, BIT(order));
> > -	return res;
> > +	list_remove(p);
> > +	memset(a->page_states + (virt_to_pfn(p) - a->base),
> > ALLOC_MASK | order, BIT(order));
> > +	return p;
> >   }
> >   
> > -static struct mem_area *get_area(uintptr_t pfn)
> > +static struct mem_area *get_area(pfn_t pfn)
> >   {
> >   	uintptr_t i;
> >   
> >   	for (i = 0; i < MAX_AREAS; i++)
> > -		if ((areas_mask & BIT(i)) && area_contains(areas +
> > i, pfn))
> > +		if ((areas_mask & BIT(i)) &&
> > usable_area_contains_pfn(areas + i, pfn)) return areas + i;
> >   	return NULL;
> >   }
> > @@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t
> > pfn)
> >    * - all of the pages of the two blocks must have the same block
> > size
> >    * - the function is called with the lock held
> >    */
> > -static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn,
> > uintptr_t pfn2) +static bool coalesce(struct mem_area *a, u8 order,
> > pfn_t pfn, pfn_t pfn2) {
> > -	uintptr_t first, second, i;
> > -	struct linked_list *li;
> > +	pfn_t first, second, i;
> >   
> >   	assert(IS_ALIGNED_ORDER(pfn, order) &&
> > IS_ALIGNED_ORDER(pfn2, order)); assert(pfn2 == pfn + BIT(order));
> >   	assert(a);
> >   
> >   	/* attempting to coalesce two blocks that belong to
> > different areas */
> > -	if (!area_contains(a, pfn) || !area_contains(a, pfn2 +
> > BIT(order) - 1))
> > +	if (!usable_area_contains_pfn(a, pfn) ||
> > !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1)) return false;
> >   	first = pfn - a->base;
> >   	second = pfn2 - a->base;
> > @@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8
> > order, uintptr_t pfn, uintptr_t pfn2 return false;
> >   
> >   	/* we can coalesce, remove both blocks from their
> > freelists */
> > -	li = list_remove((void *)(pfn2 << PAGE_SHIFT));
> > -	assert(li);
> > -	li = list_remove((void *)(pfn << PAGE_SHIFT));
> > -	assert(li);
> > +	list_remove(pfn_to_virt(pfn2));
> > +	list_remove(pfn_to_virt(pfn));
> >   	/* check the metadata entries and update with the new
> > size */ for (i = 0; i < (2ull << order); i++) {
> >   		assert(a->page_states[first + i] == order);
> >   		a->page_states[first + i] = order + 1;
> >   	}
> >   	/* finally add the newly coalesced block to the
> > appropriate freelist */
> > -	list_add(a->freelists + order + 1, li);
> > +	list_add(a->freelists + order + 1, pfn_to_virt(pfn));
> >   	return true;
> >   }
> >   
> > @@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8
> > order, uintptr_t pfn, uintptr_t pfn2 */
> >   static void _free_pages(void *mem)
> >   {
> > -	uintptr_t pfn2, pfn = PFN(mem);
> > +	pfn_t pfn2, pfn = virt_to_pfn(mem);
> >   	struct mem_area *a = NULL;
> >   	uintptr_t i, p;
> >   	u8 order;
> > @@ -232,7 +243,7 @@ static void _free_pages(void *mem)
> >   	/* ensure that the block is aligned properly for its size
> > */ assert(IS_ALIGNED_ORDER(pfn, order));
> >   	/* ensure that the area can contain the whole block */
> > -	assert(area_contains(a, pfn + BIT(order) - 1));
> > +	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
> >   
> >   	for (i = 0; i < BIT(order); i++) {
> >   		/* check that all pages of the block have
> > consistent metadata */ @@ -268,63 +279,68 @@ void free_pages(void
> > *mem) spin_unlock(&lock);
> >   }
> >   
> > -static void *_alloc_page_special(uintptr_t addr)
> > +static bool _alloc_page_special(pfn_t pfn)
> >   {
> >   	struct mem_area *a;
> > -	uintptr_t mask, i;
> > +	pfn_t mask, i;
> >   
> > -	a = get_area(PFN(addr));
> > -	assert(a);
> > -	i = PFN(addr) - a->base;
> > +	a = get_area(pfn);
> > +	if (!a)
> > +		return false;
> > +	i = pfn - a->base;
> >   	if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
> > -		return NULL;
> > +		return false;
> >   	while (a->page_states[i]) {
> > -		mask = GENMASK_ULL(63, PAGE_SHIFT +
> > a->page_states[i]);
> > -		split(a, (void *)(addr & mask));
> > +		mask = GENMASK_ULL(63, a->page_states[i]);
> > +		split(a, pfn_to_virt(pfn & mask));
> >   	}
> >   	a->page_states[i] = SPECIAL_MASK;
> > -	return (void *)addr;
> > +	return true;
> >   }
> >   
> > -static void _free_page_special(uintptr_t addr)
> > +static void _free_page_special(pfn_t pfn)
> >   {
> >   	struct mem_area *a;
> > -	uintptr_t i;
> > +	pfn_t i;
> >   
> > -	a = get_area(PFN(addr));
> > +	a = get_area(pfn);
> >   	assert(a);
> > -	i = PFN(addr) - a->base;
> > +	i = pfn - a->base;
> >   	assert(a->page_states[i] == SPECIAL_MASK);
> >   	a->page_states[i] = ALLOC_MASK;
> > -	_free_pages((void *)addr);
> > +	_free_pages(pfn_to_virt(pfn));
> >   }
> >   
> > -void *alloc_pages_special(uintptr_t addr, size_t n)
> > +bool alloc_pages_special(phys_addr_t addr, size_t n)  
> 
> 
> The convention for these alloc functions seems to be that of
> returning void *. For example, alloc_pages_area(), alloc_pages() etc.
>  Probably we should maintain the convention or change all of their
> return type.

what if you try to allocate memory that is not directly addressable?
(e.g. on x86_32)

you pass a phys_addr_t and it succeeds, but you can't get a pointer to
it, how should I indicate success/failure?

> >   {
> > -	uintptr_t i;
> > +	pfn_t pfn;
> > +	size_t i;
> >   
> >   	assert(IS_ALIGNED(addr, PAGE_SIZE));
> > +	pfn = addr >> PAGE_SHIFT;
> >   	spin_lock(&lock);
> >   	for (i = 0; i < n; i++)
> > -		if (!_alloc_page_special(addr + i * PAGE_SIZE))
> > +		if (!_alloc_page_special(pfn + i))  
> 
> 
> Can the PFN macro be used here instead of the 'pfn' variable ?

I remove the PFN macro in this patch, also addr is not a virtual
address.

> >   			break;
> >   	if (i < n) {
> >   		for (n = 0 ; n < i; n++)
> > -			_free_page_special(addr + n * PAGE_SIZE);
> > -		addr = 0;
> > +			_free_page_special(pfn + n);
> > +		n = 0;
> >   	}
> >   	spin_unlock(&lock);
> > -	return (void *)addr;
> > +	return n;
> >   }
> >   
> > -void free_pages_special(uintptr_t addr, size_t n)
> > +void free_pages_special(phys_addr_t addr, size_t n)
> >   {
> > -	uintptr_t i;
> > +	pfn_t pfn;
> > +	size_t i;
> >   
> >   	assert(IS_ALIGNED(addr, PAGE_SIZE));
> > +	pfn = addr >> PAGE_SHIFT;
> >   	spin_lock(&lock);
> >   	for (i = 0; i < n; i++)
> > -		_free_page_special(addr + i * PAGE_SIZE);
> > +		_free_page_special(pfn + i);  
> 
> 
> Can the PFN macro be used here instead of the 'pfn' variable ?

same as above

> >   	spin_unlock(&lock);
> >   }
> >   
> > @@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area,
> > unsigned int order) return page_memalign_order_area(area, order,
> > order); }
> >   
> > -void *alloc_pages(unsigned int order)
> > -{
> > -	return alloc_pages_area(AREA_ANY, order);
> > -}
> > -
> >   /*
> >    * Allocates (1 << order) physically contiguous aligned pages.
> >    * Returns NULL if the allocation was not possible.
> > @@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area,
> > size_t alignment, size_t size) return
> > page_memalign_order_area(area, size, alignment); }
> >   
> > -void *memalign_pages(size_t alignment, size_t size)
> > -{
> > -	return memalign_pages_area(AREA_ANY, alignment, size);
> > -}
> > -
> > -/*
> > - * Allocates one page
> > - */
> > -void *alloc_page()
> > -{
> > -	return alloc_pages(0);
> > -}
> >   
> >   static struct alloc_ops page_alloc_ops = {
> >   	.memalign = memalign_pages,
> > @@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
> >    * - the memory area to add does not overlap with existing areas
> >    * - the memory area to add has at least 5 pages available
> >    */
> > -static void _page_alloc_init_area(u8 n, uintptr_t start_pfn,
> > uintptr_t top_pfn) +static void _page_alloc_init_area(u8 n, pfn_t
> > start_pfn, pfn_t top_pfn) {
> >   	size_t table_size, npages, i;
> >   	struct mem_area *a;
> > @@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n,
> > uintptr_t start_pfn, uintptr_t top_pfn) 
> >   	/* fill in the values of the new area */
> >   	a = areas + n;
> > -	a->page_states = (void *)(start_pfn << PAGE_SHIFT);
> > +	a->page_states = pfn_to_virt(start_pfn);
> >   	a->base = start_pfn + table_size;
> >   	a->top = top_pfn;
> >   	npages = top_pfn - a->base;
> > @@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n,
> > uintptr_t start_pfn, uintptr_t top_pfn) for (i = 0; i < MAX_AREAS;
> > i++) { if (!(areas_mask & BIT(i)))
> >   			continue;
> > -		assert(!area_or_metadata_contains(areas + i,
> > start_pfn));
> > -		assert(!area_or_metadata_contains(areas + i,
> > top_pfn - 1));
> > -		assert(!area_or_metadata_contains(a,
> > PFN(areas[i].page_states)));
> > -		assert(!area_or_metadata_contains(a, areas[i].top
> > - 1));
> > +		assert(!area_contains_pfn(areas + i, start_pfn));
> > +		assert(!area_contains_pfn(areas + i, top_pfn - 1));
> > +		assert(!area_contains_pfn(a,
> > virt_to_pfn(areas[i].page_states)));
> > +		assert(!area_contains_pfn(a, areas[i].top - 1));
> >   	}
> >   	/* initialize all freelists for the new area */
> >   	for (i = 0; i < NLISTS; i++)
> > -		a->freelists[i].next = a->freelists[i].prev =
> > a->freelists + i;
> > +		a->freelists[i].prev = a->freelists[i].next =
> > a->freelists + i; 
> >   	/* initialize the metadata for the available memory */
> >   	for (i = a->base; i < a->top; i += 1ull << order) {
> > @@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n,
> > uintptr_t start_pfn, uintptr_t top_pfn) assert(order < NLISTS);
> >   		/* initialize the metadata and add to the
> > freelist */ memset(a->page_states + (i - a->base), order,
> > BIT(order));
> > -		list_add(a->freelists + order, (void *)(i <<
> > PAGE_SHIFT));
> > +		list_add(a->freelists + order, pfn_to_virt(i));
> >   	}
> >   	/* finally mark the area as present */
> >   	areas_mask |= BIT(n);
> >   }
> >   
> > -static void __page_alloc_init_area(u8 n, uintptr_t cutoff,
> > uintptr_t base_pfn, uintptr_t *top_pfn) +static void
> > __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t
> > *top_pfn) { if (*top_pfn > cutoff) {
> >   		spin_lock(&lock);
> > @@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n,
> > uintptr_t cutoff, uintptr_t base_pfn, u
> >    * Prerequisites:
> >    * see _page_alloc_init_area
> >    */
> > -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
> > top_pfn) +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
> > phys_addr_t top_pfn) {
> >   	if (n != AREA_ANY_NUMBER) {
> >   		__page_alloc_init_area(n, 0, base_pfn, &top_pfn);
> >  

Re: [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas

[Claudio Imbrenda]

On Thu, 24 Dec 2020 10:17:00 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:

> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > Fix the definitions of the memory areas.
> >
> > Bring the headers in line with the rest of the asm headers, by
> > having the appropriate #ifdef _ASM$ARCH_ guarding the headers.  
> 
> 
> Should we mention MAX_AREAS that the patch adds in each arch-specific 
> header ?

not sure, it's a minor detail. I mentioned above that I "Fix the
definitions of the memory areas". 

MAX_AREAS is not totally new, now it's per-arch instead of being in the
generic code.

> > Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> >   lib/asm-generic/memory_areas.h |  9 ++++-----
> >   lib/arm/asm/memory_areas.h     | 11 +++--------
> >   lib/arm64/asm/memory_areas.h   | 11 +++--------
> >   lib/powerpc/asm/memory_areas.h | 11 +++--------
> >   lib/ppc64/asm/memory_areas.h   | 11 +++--------
> >   lib/s390x/asm/memory_areas.h   | 13 ++++++-------
> >   lib/x86/asm/memory_areas.h     | 27 ++++++++++++++++-----------
> >   lib/alloc_page.h               |  3 +++
> >   lib/alloc_page.c               |  4 +---
> >   9 files changed, 42 insertions(+), 58 deletions(-)
> >
> > diff --git a/lib/asm-generic/memory_areas.h
> > b/lib/asm-generic/memory_areas.h index 927baa7..3074afe 100644
> > --- a/lib/asm-generic/memory_areas.h
> > +++ b/lib/asm-generic/memory_areas.h
> > @@ -1,11 +1,10 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
> > +#define __ASM_GENERIC_MEMORY_AREAS_H__
> >   
> >   #define AREA_NORMAL_PFN 0
> >   #define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
> >   
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#define MAX_AREAS 1
> >   
> >   #endif
> > diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
> > index 927baa7..c723310 100644
> > --- a/lib/arm/asm/memory_areas.h
> > +++ b/lib/arm/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMARM_MEMORY_AREAS_H_
> > +#define _ASMARM_MEMORY_AREAS_H_
> >   
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >   
> >   #endif
> > diff --git a/lib/arm64/asm/memory_areas.h
> > b/lib/arm64/asm/memory_areas.h index 927baa7..18e8ca8 100644
> > --- a/lib/arm64/asm/memory_areas.h
> > +++ b/lib/arm64/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMARM64_MEMORY_AREAS_H_
> > +#define _ASMARM64_MEMORY_AREAS_H_
> >   
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >   
> >   #endif
> > diff --git a/lib/powerpc/asm/memory_areas.h
> > b/lib/powerpc/asm/memory_areas.h index 927baa7..76d1738 100644
> > --- a/lib/powerpc/asm/memory_areas.h
> > +++ b/lib/powerpc/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
> > +#define _ASMPOWERPC_MEMORY_AREAS_H_
> >   
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >   
> >   #endif
> > diff --git a/lib/ppc64/asm/memory_areas.h
> > b/lib/ppc64/asm/memory_areas.h index 927baa7..b9fd46b 100644
> > --- a/lib/ppc64/asm/memory_areas.h
> > +++ b/lib/ppc64/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMPPC64_MEMORY_AREAS_H_
> > +#define _ASMPPC64_MEMORY_AREAS_H_
> >   
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >   
> >   #endif
> > diff --git a/lib/s390x/asm/memory_areas.h
> > b/lib/s390x/asm/memory_areas.h index 4856a27..827bfb3 100644
> > --- a/lib/s390x/asm/memory_areas.h
> > +++ b/lib/s390x/asm/memory_areas.h
> > @@ -1,16 +1,15 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMS390X_MEMORY_AREAS_H_
> > +#define _ASMS390X_MEMORY_AREAS_H_
> >   
> > -#define AREA_NORMAL_PFN BIT(31-12)
> > +#define AREA_NORMAL_PFN (1 << 19)
> >   #define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
> >   
> >   #define AREA_LOW_PFN 0
> >   #define AREA_LOW_NUMBER 1
> > -#define AREA_LOW 2
> > +#define AREA_LOW (1 << AREA_LOW_NUMBER)
> >   
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#define MAX_AREAS 2
> >   
> >   #define AREA_DMA31 AREA_LOW
> >   
> > diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
> > index 952f5bd..e84016f 100644
> > --- a/lib/x86/asm/memory_areas.h
> > +++ b/lib/x86/asm/memory_areas.h
> > @@ -1,21 +1,26 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASM_X86_MEMORY_AREAS_H_
> > +#define _ASM_X86_MEMORY_AREAS_H_
> >   
> >   #define AREA_NORMAL_PFN BIT(36-12)
> >   #define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
> >   
> > -#define AREA_PAE_HIGH_PFN BIT(32-12)
> > -#define AREA_PAE_HIGH_NUMBER 1
> > -#define AREA_PAE_HIGH 2
> > +#define AREA_HIGH_PFN BIT(32-12)
> > +#define AREA_HIGH_NUMBER 1
> > +#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
> >   
> > -#define AREA_LOW_PFN 0
> > +#define AREA_LOW_PFN BIT(24-12)
> >   #define AREA_LOW_NUMBER 2
> > -#define AREA_LOW 4
> > +#define AREA_LOW (1 << AREA_LOW_NUMBER)
> >   
> > -#define AREA_PAE (AREA_PAE | AREA_LOW)
> > +#define AREA_LOWEST_PFN 0
> > +#define AREA_LOWEST_NUMBER 3
> > +#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
> >   
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#define MAX_AREAS 4
> > +
> > +#define AREA_DMA24 AREA_LOWEST
> > +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
> > +#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
> >   
> >   #endif
> > diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> > index 816ff5d..b6aace5 100644
> > --- a/lib/alloc_page.h
> > +++ b/lib/alloc_page.h
> > @@ -10,6 +10,9 @@
> >   
> >   #include <asm/memory_areas.h>
> >   
> > +#define AREA_ANY -1
> > +#define AREA_ANY_NUMBER 0xff
> > +
> >   /* Returns true if the page allocator has been initialized */
> >   bool page_alloc_initialized(void);
> >   
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index 685ab1e..ed0ff02 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -19,8 +19,6 @@
> >   #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
> >   #define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
> >   
> > -#define MAX_AREAS	6
> > -
> >   #define ORDER_MASK	0x3f
> >   #define ALLOC_MASK	0x40
> >   #define SPECIAL_MASK	0x80
> > @@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t
> > base_pfn, uintptr_t top_pfn) return;
> >   	}
> >   #ifdef AREA_HIGH_PFN
> > -	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN),
> > base_pfn, &top_pfn);
> > +	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN,
> > base_pfn, &top_pfn);  
> 
> 
> Surprising that the compiler didn't complain !
> 
> >   #endif
> >   	__page_alloc_init_area(AREA_NORMAL_NUMBER,
> > AREA_NORMAL_PFN, base_pfn, &top_pfn); #ifdef AREA_LOW_PFN  

Re: [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements

[Claudio Imbrenda]

On Thu, 24 Dec 2020 10:16:45 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:

> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > Add some asserts to make sure the state is consistent.
> >
> > Simplify and improve the readability of vm_free.
> >
> > Fixes: 3f6fee0d4da4 ("lib/vmalloc: vmalloc support for handling
> > allocation metadata")
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> >   lib/vmalloc.c | 20 +++++++++++---------
> >   1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> > index 986a34c..7a49adf 100644
> > --- a/lib/vmalloc.c
> > +++ b/lib/vmalloc.c
> > @@ -162,13 +162,14 @@ static void *vm_memalign(size_t alignment,
> > size_t size) static void vm_free(void *mem)
> >   {
> >   	struct metadata *m;
> > -	uintptr_t ptr, end;
> > +	uintptr_t ptr, page, i;
> >   
> >   	/* the pointer is not page-aligned, it was a single-page
> > allocation */  
> 
> 
> Do we need an assert() for 'mem' if it is NULL for some reason ?

I thought the NULL case was handled in alloc.c, but I was mistaken.
In any case, the metadata will probably not match and trigger the other
asserts.

I will still add assert(mem), though, as it looks cleaner

> >   	if (!IS_ALIGNED((uintptr_t)mem, PAGE_SIZE)) {
> >   		assert(GET_MAGIC(mem) == VM_MAGIC);
> > -		ptr = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
> > -		free_page(phys_to_virt(ptr));
> > +		page = virt_to_pte_phys(page_root, mem) &
> > PAGE_MASK;
> > +		assert(page);
> > +		free_page(phys_to_virt(page));
> >   		return;
> >   	}
> >   
> > @@ -176,13 +177,14 @@ static void vm_free(void *mem)
> >   	m = GET_METADATA(mem);
> >   	assert(m->magic == VM_MAGIC);
> >   	assert(m->npages > 0);
> > +	assert(m->npages < BIT_ULL(BITS_PER_LONG - PAGE_SHIFT));  
> 
> 
> NIT:  Combine the two assert()s for 'npages' perhaps ?

no, when one assert is triggered, it will print a useful message; if
you combine the two asserts you don't know why exactly it failed.

it's functionally equivalent, but it has a better user experience.

> >   	/* free all the pages including the metadata page */
> > -	ptr = (uintptr_t)mem - PAGE_SIZE;
> > -	end = ptr + m->npages * PAGE_SIZE;
> > -	for ( ; ptr < end; ptr += PAGE_SIZE)
> > -		free_page(phys_to_virt(virt_to_pte_phys(page_root,
> > (void *)ptr)));
> > -	/* free the last one separately to avoid overflow issues */
> > -	free_page(phys_to_virt(virt_to_pte_phys(page_root, (void
> > *)ptr)));
> > +	ptr = (uintptr_t)m & PAGE_MASK;
> > +	for (i = 0 ; i < m->npages + 1; i++, ptr += PAGE_SIZE) {
> > +		page = virt_to_pte_phys(page_root, (void *)ptr) &
> > PAGE_MASK;
> > +		assert(page);
> > +		free_page(phys_to_virt(page));
> > +	}
> >   }
> >   
> >   static struct alloc_ops vmalloc_ops = {  

Re: [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default

[Claudio Imbrenda]

On Thu, 24 Dec 2020 10:17:30 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:

> On 12/16/20 12:12 PM, Claudio Imbrenda wrote:
> > The new function page_alloc_set_default_flags can be used to set the
> > default flags for allocations. The passed value will be ORed with
> > the flags argument passed to the allocator at each allocation.
> >
> > The default value for the default flags is FLAG_ZERO, which means
> > that by default all allocated memory is now zeroed, restoring the
> > default behaviour that had been accidentally removed by a previous
> > commit.
> >
> > If needed, a testcase can call page_alloc_set_default_flags(0) in
> > order to get non-zeroed pages from the allocator. For example, if
> > the testcase will need fresh memory, the zero flag should be
> > removed from the default.
> >
> > Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page
> > allocator") Reported-by: Nadav Amit<nadav.amit@gmail.com>
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> >   lib/alloc_page.h | 3 +++
> >   lib/alloc_page.c | 8 ++++++++
> >   2 files changed, 11 insertions(+)
> >
> > diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> > index 1039814..8b53a58 100644
> > --- a/lib/alloc_page.h
> > +++ b/lib/alloc_page.h
> > @@ -22,6 +22,9 @@
> >   /* Returns true if the page allocator has been initialized */
> >   bool page_alloc_initialized(void);
> >   
> > +/* Sets the default flags for the page allocator, the default is
> > FLAG_ZERO */ +void page_alloc_set_default_flags(unsigned int flags);
> > +
> >   /*
> >    * Initializes a memory area.
> >    * n is the number of the area to initialize
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index 4d5722f..08e0d05 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -54,12 +54,19 @@ static struct mem_area areas[MAX_AREAS];
> >   static unsigned int areas_mask;
> >   /* Protects areas and areas mask */
> >   static struct spinlock lock;
> > +/* Default behaviour: zero allocated pages */
> > +static unsigned int default_flags = FLAG_ZERO;
> >   
> >   bool page_alloc_initialized(void)
> >   {
> >   	return areas_mask != 0;
> >   }
> >   
> > +void page_alloc_set_default_flags(unsigned int flags)
> > +{
> > +	default_flags = flags;  
> 
> 
> Who calls this functions ?

nobody yet, since I have just introduced them.

The idea is that a testcase should call this early on to set the default

> Just wondering if default flag should be a static set of flag values 
> which the caller can override based on needs rather than the caller 
> setting the default flag.

hmmmmm

I would only need to reverse the semantics of FLAG_ZERO, but then I can
get rid of this patch

I think I'll do it
 
> > +}
> > +
> >   /*
> >    * Each memory area contains an array of metadata entries at the
> > very
> >    * beginning. The usable memory follows immediately afterwards.
> > @@ -394,6 +401,7 @@ static void *page_memalign_order_flags(u8 ord,
> > u8 al, u32 flags) void *res = NULL;
> >   	int i, area, fresh;
> >   
> > +	flags |= default_flags;
> >   	fresh = !!(flags & FLAG_FRESH);
> >   	spin_lock(&lock);
> >   	area = (flags & AREA_MASK) ? flags & areas_mask :
> > areas_mask;  

Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator

[Claudio Imbrenda]

On Mon, 28 Dec 2020 11:34:38 -0800
Sean Christopherson <seanjc@google.com> wrote:

> On Wed, Dec 16, 2020, Claudio Imbrenda wrote:
> >  /*
> > - * Allocates and reserves the specified memory range if possible.
> > - * Returns NULL in case of failure.
> > + * Allocates and reserves the specified physical memory range if
> > possible.
> > + * If the specified range cannot be reserved in its entirety, no
> > action is
> > + * performed and false is returned.
> > + *
> > + * Returns true in case of success, false otherwise.
> >   */
> > -void *alloc_pages_special(uintptr_t addr, size_t npages);
> > +bool alloc_pages_special(phys_addr_t addr, size_t npages);  
> 
> The boolean return is a bit awkward as kernel programmers will likely

do you prefer int, with 0 for success and -1 for failure?
that's surely not a problem

> expect a non-zero return to mean failure.  But, since there are no
> users, can we simply drop the entire *_pages_special() API?
> Allocating a specific PFN that isn't MMIO seems doomed to fail
> anyways; I'm having a hard time envisioning a test that would be able
> to use such an API without being horribly fragile.

I can. s390x can use this for some tests, where we need to allocate
memory at within or outside of specific areas, which might only be
known at run time (so we can't use the memory areas)

the alternative would be to allocate all the memory, take what is
needed, and then free the rest.... not very elegant

> >  
> >  /*
> >   * Frees a reserved memory range that had been reserved with
> > @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t
> > npages);
> >   * exactly, it can also be a subset, in which case only the
> > specified
> >   * pages will be freed and unreserved.
> >   */
> > -void free_pages_special(uintptr_t addr, size_t npages);
> > +void free_pages_special(phys_addr_t addr, size_t npages);  

Re: [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops

[Krish Sadhukhan]

On 1/4/21 5:05 AM, Claudio Imbrenda wrote:
> On Thu, 24 Dec 2020 10:17:20 -0800
> Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
>
>> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
>>> Remove align_min from struct alloc_ops, since it is no longer used.
>>>
>>> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
>>> ---
>>>    lib/alloc.h      | 1 -
>>>    lib/alloc_page.c | 1 -
>>>    lib/alloc_phys.c | 9 +++++----
>>>    lib/vmalloc.c    | 1 -
>>>    4 files changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/lib/alloc.h b/lib/alloc.h
>>> index 9b4b634..db90b01 100644
>>> --- a/lib/alloc.h
>>> +++ b/lib/alloc.h
>>> @@ -25,7 +25,6 @@
>>>    struct alloc_ops {
>>>    	void *(*memalign)(size_t alignment, size_t size);
>>>    	void (*free)(void *ptr);
>>> -	size_t align_min;
>>>    };
>>>    
>>>    extern struct alloc_ops *alloc_ops;
>>> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
>>> index 8d2700d..b1cdf21 100644
>>> --- a/lib/alloc_page.c
>>> +++ b/lib/alloc_page.c
>>> @@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area,
>>> size_t alignment, size_t size) static struct alloc_ops
>>> page_alloc_ops = { .memalign = memalign_pages,
>>>    	.free = free_pages,
>>> -	.align_min = PAGE_SIZE,
>>>    };
>>>    
>>>    /*
>>> diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
>>> index 72e20f7..a4d2bf2 100644
>>> --- a/lib/alloc_phys.c
>>> +++ b/lib/alloc_phys.c
>>> @@ -29,8 +29,8 @@ static phys_addr_t base, top;
>>>    static void *early_memalign(size_t alignment, size_t size);
>>>    static struct alloc_ops early_alloc_ops = {
>>>    	.memalign = early_memalign,
>>> -	.align_min = DEFAULT_MINIMUM_ALIGNMENT
>>>    };
>>> +static size_t align_min;
>>
>> I don't see any caller of phys_alloc_set_minimum_alignment(). So when
> lib/arm/setup.c:150
> lib/powerpc/setup.c:150
>
>> you are comparing against this variable in phys_alloc_aligned_safe()
>> below, you are comparing against zero. Is that what you is intended
>> or should 'align_min' be set some default ?
> if the architecture specific code did not specify anything better, 0 is
> ok.
>   
>>>    
>>>    struct alloc_ops *alloc_ops = &early_alloc_ops;
>>>    
>>> @@ -39,8 +39,7 @@ void phys_alloc_show(void)
>>>    	int i;
>>>    
>>>    	spin_lock(&lock);
>>> -	printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
>>> -		(u64)early_alloc_ops.align_min);
>>> +	printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
>>> (u64)align_min); for (i = 0; i < nr_regions; ++i)
>>>    		printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
>>>    			(u64)regions[i].base,
>>> @@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t
>>> align) {
>>>    	assert(align && !(align & (align - 1)));
>>>    	spin_lock(&lock);
>>> -	early_alloc_ops.align_min = align;
>>> +	align_min = align;
>>>    	spin_unlock(&lock);
>>>    }
>>>    
>>> @@ -83,6 +82,8 @@ static phys_addr_t
>>> phys_alloc_aligned_safe(phys_addr_t size, top_safe = MIN(top_safe,
>>> 1ULL << 32);
>>>    	assert(base < top_safe);
>>> +	if (align < align_min)
>>> +		align = align_min;
>>>    
>>>    	addr = ALIGN(base, align);
>>>    	size += addr - base;
>>> diff --git a/lib/vmalloc.c b/lib/vmalloc.c
>>> index 7a49adf..e146162 100644
>>> --- a/lib/vmalloc.c
>>> +++ b/lib/vmalloc.c
>>> @@ -190,7 +190,6 @@ static void vm_free(void *mem)
>>>    static struct alloc_ops vmalloc_ops = {
>>>    	.memalign = vm_memalign,
>>>    	.free = vm_free,
>>> -	.align_min = PAGE_SIZE,
>>>    };
>>>    
>>>    void __attribute__((__weak__)) find_highmem(void)
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator

[Krish Sadhukhan]

On 1/4/21 5:11 AM, Claudio Imbrenda wrote:
> On Thu, 24 Dec 2020 10:17:06 -0800
> Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
>
>> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
>>> This patch introduces some improvements to the code, mostly
>>> readability improvements, but also some semantic details, and
>>> improvements in the documentation.
>>>
>>> * introduce and use pfn_t to semantically tag parameters as PFNs
>>> * remove the PFN macro, use virt_to_pfn instead
>>> * rename area_or_metadata_contains and area_contains to
>>> area_contains_pfn and usable_area_contains_pfn respectively
>>> * fix/improve comments in lib/alloc_page.h
>>> * move some wrapper functions to the header
>>>
>>> Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page
>>> allocator") Fixes: 34c950651861 ("lib/alloc_page: allow reserving
>>> arbitrary memory ranges")
>>>
>>> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
>>> ---
>>>    lib/alloc_page.h |  49 +++++++++-----
>>>    lib/alloc_page.c | 165
>>> +++++++++++++++++++++++------------------------ 2 files changed,
>>> 116 insertions(+), 98 deletions(-)
>>>
>>> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
>>> index b6aace5..d8550c6 100644
>>> --- a/lib/alloc_page.h
>>> +++ b/lib/alloc_page.h
>>> @@ -8,6 +8,7 @@
>>>    #ifndef ALLOC_PAGE_H
>>>    #define ALLOC_PAGE_H 1
>>>    
>>> +#include <stdbool.h>
>>>    #include <asm/memory_areas.h>
>>>    
>>>    #define AREA_ANY -1
>>> @@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
>>>     * top_pfn is the physical frame number of the first page
>>> immediately after
>>>     * the end of the area to initialize
>>>     */
>>> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
>>> top_pfn); +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
>>> phys_addr_t top_pfn);
>>>    /* Enables the page allocator. At least one area must have been
>>> initialized */ void page_alloc_ops_enable(void);
>>> @@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas,
>>> size_t alignment, size_t size);
>>>    /*
>>>     * Allocate aligned memory from any area.
>>> - * Equivalent to memalign_pages_area(~0, alignment, size).
>>> + * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
>>>     */
>>> -void *memalign_pages(size_t alignment, size_t size);
>>> +static inline void *memalign_pages(size_t alignment, size_t size)
>>> +{
>>> +	return memalign_pages_area(AREA_ANY, alignment, size);
>>> +}
>>>    
>>>    /*
>>>     * Allocate naturally aligned memory from the specified areas.
>>> @@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t
>>> size); void *alloc_pages_area(unsigned int areas, unsigned int
>>> order);
>>>    /*
>>> - * Allocate one page from any area.
>>> - * Equivalent to alloc_pages(0);
>>> + * Allocate naturally aligned memory from any area.
>>
>> This one allocates page size memory and the comment should reflect
>> that.
> I'll fix the comment
>   
>>> + * Equivalent to alloc_pages_area(AREA_ANY, order);
>>>     */
>>> -void *alloc_page(void);
>>> +static inline void *alloc_pages(unsigned int order)
>>> +{
>>> +	return alloc_pages_area(AREA_ANY, order);
>>> +}
>>>    
>>>    /*
>>> - * Allocate naturally aligned memory from any area.
>>> - * Equivalent to alloc_pages_area(~0, order);
>>> + * Allocate one page from any area.
>>> + * Equivalent to alloc_pages(0);
>>>     */
>>> -void *alloc_pages(unsigned int order);
>>> +static inline void *alloc_page(void)
>>> +{
>>> +	return alloc_pages(0);
>>> +}
>>>    
>>>    /*
>>>     * Frees a memory block allocated with any of the memalign_pages*
>>> or @@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
>>>     */
>>>    void free_pages(void *mem);
>>>    
>>> -/* For backwards compatibility */
>>> +/*
>>> + * Free one page.
>>> + * Equivalent to free_pages(mem).
>>> + */
>>>    static inline void free_page(void *mem)
>>>    {
>>>    	return free_pages(mem);
>>>    }
>>>    
>>> -/* For backwards compatibility */
>>> +/*
>>> + * Free pages by order.
>>> + * Equivalent to free_pages(mem).
>>> + */
>>>    static inline void free_pages_by_order(void *mem, unsigned int
>>> order) {
>>>    	free_pages(mem);
>>>    }
>>>    
>>>    /*
>>> - * Allocates and reserves the specified memory range if possible.
>>> - * Returns NULL in case of failure.
>>> + * Allocates and reserves the specified physical memory range if
>>> possible.
>>> + * If the specified range cannot be reserved in its entirety, no
>>> action is
>>> + * performed and false is returned.
>>> + *
>>> + * Returns true in case of success, false otherwise.
>>>     */
>>> -void *alloc_pages_special(uintptr_t addr, size_t npages);
>>> +bool alloc_pages_special(phys_addr_t addr, size_t npages);
>>>    
>>>    /*
>>>     * Frees a reserved memory range that had been reserved with
>>> @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t
>>> npages);
>>>     * exactly, it can also be a subset, in which case only the
>>> specified
>>>     * pages will be freed and unreserved.
>>>     */
>>> -void free_pages_special(uintptr_t addr, size_t npages);
>>> +void free_pages_special(phys_addr_t addr, size_t npages);
>>>    
>>>    #endif
>>> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
>>> index ed0ff02..8d2700d 100644
>>> --- a/lib/alloc_page.c
>>> +++ b/lib/alloc_page.c
>>> @@ -17,25 +17,29 @@
>>>    
>>>    #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
>>>    #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
>>> -#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>>>    
>>>    #define ORDER_MASK	0x3f
>>>    #define ALLOC_MASK	0x40
>>>    #define SPECIAL_MASK	0x80
>>>    
>>> +typedef phys_addr_t pfn_t;
>>> +
>>>    struct mem_area {
>>>    	/* Physical frame number of the first usable frame in the
>>> area */
>>> -	uintptr_t base;
>>> +	pfn_t base;
>>>    	/* Physical frame number of the first frame outside the
>>> area */
>>> -	uintptr_t top;
>>> -	/* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
>>> +	pfn_t top;
>>> +	/* Per page metadata, each entry is a combination *_MASK
>>> and order */ u8 *page_states;
>>>    	/* One freelist for each possible block size, up to
>>> NLISTS */ struct linked_list freelists[NLISTS];
>>>    };
>>>    
>>> +/* Descriptors for each possible area */
>>>    static struct mem_area areas[MAX_AREAS];
>>> +/* Mask of initialized areas */
>>>    static unsigned int areas_mask;
>>> +/* Protects areas and areas mask */
>>>    static struct spinlock lock;
>>>    
>>>    bool page_alloc_initialized(void)
>>> @@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
>>>    	return areas_mask != 0;
>>>    }
>>>    
>>> -static inline bool area_or_metadata_contains(struct mem_area *a,
>>> uintptr_t pfn) +/*
>>> + * Each memory area contains an array of metadata entries at the
>>> very
>>> + * beginning. The usable memory follows immediately afterwards.
>>> + * This function returns true if the given pfn falls anywhere
>>> within the
>>> + * memory area, including the metadata area.
>>> + */
>>> +static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
>>>    {
>>> -	return (pfn >= PFN(a->page_states)) && (pfn < a->top);
>>> +	return (pfn >= virt_to_pfn(a->page_states)) && (pfn <
>>> a->top); }
>>>    
>>> -static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
>>> +/*
>>> + * Each memory area contains an array of metadata entries at the
>>> very
>>> + * beginning. The usable memory follows immediately afterwards.
>>> + * This function returns true if the given pfn falls in the usable
>>> range of
>>> + * the given memory area.
>>> + */
>>> +static inline bool usable_area_contains_pfn(struct mem_area *a,
>>> pfn_t pfn) {
>>>    	return (pfn >= a->base) && (pfn < a->top);
>>>    }
>>> @@ -69,21 +85,19 @@ static inline bool area_contains(struct
>>> mem_area *a, uintptr_t pfn) */
>>>    static void split(struct mem_area *a, void *addr)
>>>    {
>>> -	uintptr_t pfn = PFN(addr);
>>> -	struct linked_list *p;
>>> -	uintptr_t i, idx;
>>> +	pfn_t pfn = virt_to_pfn(addr);
>>> +	pfn_t i, idx;
>>>    	u8 order;
>>>    
>>> -	assert(a && area_contains(a, pfn));
>>> +	assert(a && usable_area_contains_pfn(a, pfn));
>>>    	idx = pfn - a->base;
>>>    	order = a->page_states[idx];
>>>    	assert(!(order & ~ORDER_MASK) && order && (order <
>>> NLISTS)); assert(IS_ALIGNED_ORDER(pfn, order));
>>> -	assert(area_contains(a, pfn + BIT(order) - 1));
>>> +	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>>>    
>>>    	/* Remove the block from its free list */
>>> -	p = list_remove(addr);
>>> -	assert(p);
>>> +	list_remove(addr);
>>>    
>>>    	/* update the block size for each page in the block */
>>>    	for (i = 0; i < BIT(order); i++) {
>>> @@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
>>>    	}
>>>    	order--;
>>>    	/* add the first half block to the appropriate free list
>>> */
>>> -	list_add(a->freelists + order, p);
>>> +	list_add(a->freelists + order, addr);
>>>    	/* add the second half block to the appropriate free list
>>> */
>>> -	list_add(a->freelists + order, (void *)((pfn + BIT(order))
>>> * PAGE_SIZE));
>>> +	list_add(a->freelists + order, pfn_to_virt(pfn +
>>> BIT(order))); }
>>>    
>>>    /*
>>> @@ -105,7 +119,7 @@ static void split(struct mem_area *a, void
>>> *addr) */
>>>    static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
>>>    {
>>> -	struct linked_list *p, *res = NULL;
>>> +	struct linked_list *p;
>>>    	u8 order;
>>>    
>>>    	assert((al < NLISTS) && (sz < NLISTS));
>>> @@ -130,17 +144,17 @@ static void *page_memalign_order(struct
>>> mem_area *a, u8 al, u8 sz) for (; order > sz; order--)
>>>    		split(a, p);
>>>    
>>> -	res = list_remove(p);
>>> -	memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK |
>>> order, BIT(order));
>>> -	return res;
>>> +	list_remove(p);
>>> +	memset(a->page_states + (virt_to_pfn(p) - a->base),
>>> ALLOC_MASK | order, BIT(order));
>>> +	return p;
>>>    }
>>>    
>>> -static struct mem_area *get_area(uintptr_t pfn)
>>> +static struct mem_area *get_area(pfn_t pfn)
>>>    {
>>>    	uintptr_t i;
>>>    
>>>    	for (i = 0; i < MAX_AREAS; i++)
>>> -		if ((areas_mask & BIT(i)) && area_contains(areas +
>>> i, pfn))
>>> +		if ((areas_mask & BIT(i)) &&
>>> usable_area_contains_pfn(areas + i, pfn)) return areas + i;
>>>    	return NULL;
>>>    }
>>> @@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t
>>> pfn)
>>>     * - all of the pages of the two blocks must have the same block
>>> size
>>>     * - the function is called with the lock held
>>>     */
>>> -static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn,
>>> uintptr_t pfn2) +static bool coalesce(struct mem_area *a, u8 order,
>>> pfn_t pfn, pfn_t pfn2) {
>>> -	uintptr_t first, second, i;
>>> -	struct linked_list *li;
>>> +	pfn_t first, second, i;
>>>    
>>>    	assert(IS_ALIGNED_ORDER(pfn, order) &&
>>> IS_ALIGNED_ORDER(pfn2, order)); assert(pfn2 == pfn + BIT(order));
>>>    	assert(a);
>>>    
>>>    	/* attempting to coalesce two blocks that belong to
>>> different areas */
>>> -	if (!area_contains(a, pfn) || !area_contains(a, pfn2 +
>>> BIT(order) - 1))
>>> +	if (!usable_area_contains_pfn(a, pfn) ||
>>> !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1)) return false;
>>>    	first = pfn - a->base;
>>>    	second = pfn2 - a->base;
>>> @@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8
>>> order, uintptr_t pfn, uintptr_t pfn2 return false;
>>>    
>>>    	/* we can coalesce, remove both blocks from their
>>> freelists */
>>> -	li = list_remove((void *)(pfn2 << PAGE_SHIFT));
>>> -	assert(li);
>>> -	li = list_remove((void *)(pfn << PAGE_SHIFT));
>>> -	assert(li);
>>> +	list_remove(pfn_to_virt(pfn2));
>>> +	list_remove(pfn_to_virt(pfn));
>>>    	/* check the metadata entries and update with the new
>>> size */ for (i = 0; i < (2ull << order); i++) {
>>>    		assert(a->page_states[first + i] == order);
>>>    		a->page_states[first + i] = order + 1;
>>>    	}
>>>    	/* finally add the newly coalesced block to the
>>> appropriate freelist */
>>> -	list_add(a->freelists + order + 1, li);
>>> +	list_add(a->freelists + order + 1, pfn_to_virt(pfn));
>>>    	return true;
>>>    }
>>>    
>>> @@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8
>>> order, uintptr_t pfn, uintptr_t pfn2 */
>>>    static void _free_pages(void *mem)
>>>    {
>>> -	uintptr_t pfn2, pfn = PFN(mem);
>>> +	pfn_t pfn2, pfn = virt_to_pfn(mem);
>>>    	struct mem_area *a = NULL;
>>>    	uintptr_t i, p;
>>>    	u8 order;
>>> @@ -232,7 +243,7 @@ static void _free_pages(void *mem)
>>>    	/* ensure that the block is aligned properly for its size
>>> */ assert(IS_ALIGNED_ORDER(pfn, order));
>>>    	/* ensure that the area can contain the whole block */
>>> -	assert(area_contains(a, pfn + BIT(order) - 1));
>>> +	assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>>>    
>>>    	for (i = 0; i < BIT(order); i++) {
>>>    		/* check that all pages of the block have
>>> consistent metadata */ @@ -268,63 +279,68 @@ void free_pages(void
>>> *mem) spin_unlock(&lock);
>>>    }
>>>    
>>> -static void *_alloc_page_special(uintptr_t addr)
>>> +static bool _alloc_page_special(pfn_t pfn)
>>>    {
>>>    	struct mem_area *a;
>>> -	uintptr_t mask, i;
>>> +	pfn_t mask, i;
>>>    
>>> -	a = get_area(PFN(addr));
>>> -	assert(a);
>>> -	i = PFN(addr) - a->base;
>>> +	a = get_area(pfn);
>>> +	if (!a)
>>> +		return false;
>>> +	i = pfn - a->base;
>>>    	if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
>>> -		return NULL;
>>> +		return false;
>>>    	while (a->page_states[i]) {
>>> -		mask = GENMASK_ULL(63, PAGE_SHIFT +
>>> a->page_states[i]);
>>> -		split(a, (void *)(addr & mask));
>>> +		mask = GENMASK_ULL(63, a->page_states[i]);
>>> +		split(a, pfn_to_virt(pfn & mask));
>>>    	}
>>>    	a->page_states[i] = SPECIAL_MASK;
>>> -	return (void *)addr;
>>> +	return true;
>>>    }
>>>    
>>> -static void _free_page_special(uintptr_t addr)
>>> +static void _free_page_special(pfn_t pfn)
>>>    {
>>>    	struct mem_area *a;
>>> -	uintptr_t i;
>>> +	pfn_t i;
>>>    
>>> -	a = get_area(PFN(addr));
>>> +	a = get_area(pfn);
>>>    	assert(a);
>>> -	i = PFN(addr) - a->base;
>>> +	i = pfn - a->base;
>>>    	assert(a->page_states[i] == SPECIAL_MASK);
>>>    	a->page_states[i] = ALLOC_MASK;
>>> -	_free_pages((void *)addr);
>>> +	_free_pages(pfn_to_virt(pfn));
>>>    }
>>>    
>>> -void *alloc_pages_special(uintptr_t addr, size_t n)
>>> +bool alloc_pages_special(phys_addr_t addr, size_t n)
>>
>> The convention for these alloc functions seems to be that of
>> returning void *. For example, alloc_pages_area(), alloc_pages() etc.
>>   Probably we should maintain the convention or change all of their
>> return type.
> what if you try to allocate memory that is not directly addressable?
> (e.g. on x86_32)
>
> you pass a phys_addr_t and it succeeds, but you can't get a pointer to
> it, how should I indicate success/failure?


The function can perhaps return an error code via a parameter to 
indicate why NULL was returned.

>
>>>    {
>>> -	uintptr_t i;
>>> +	pfn_t pfn;
>>> +	size_t i;
>>>    
>>>    	assert(IS_ALIGNED(addr, PAGE_SIZE));
>>> +	pfn = addr >> PAGE_SHIFT;
>>>    	spin_lock(&lock);
>>>    	for (i = 0; i < n; i++)
>>> -		if (!_alloc_page_special(addr + i * PAGE_SIZE))
>>> +		if (!_alloc_page_special(pfn + i))
>>
>> Can the PFN macro be used here instead of the 'pfn' variable ?
> I remove the PFN macro in this patch, also addr is not a virtual
> address.
>
>>>    			break;
>>>    	if (i < n) {
>>>    		for (n = 0 ; n < i; n++)
>>> -			_free_page_special(addr + n * PAGE_SIZE);
>>> -		addr = 0;
>>> +			_free_page_special(pfn + n);
>>> +		n = 0;
>>>    	}
>>>    	spin_unlock(&lock);
>>> -	return (void *)addr;
>>> +	return n;
>>>    }
>>>    
>>> -void free_pages_special(uintptr_t addr, size_t n)
>>> +void free_pages_special(phys_addr_t addr, size_t n)
>>>    {
>>> -	uintptr_t i;
>>> +	pfn_t pfn;
>>> +	size_t i;
>>>    
>>>    	assert(IS_ALIGNED(addr, PAGE_SIZE));
>>> +	pfn = addr >> PAGE_SHIFT;
>>>    	spin_lock(&lock);
>>>    	for (i = 0; i < n; i++)
>>> -		_free_page_special(addr + i * PAGE_SIZE);
>>> +		_free_page_special(pfn + i);
>>
>> Can the PFN macro be used here instead of the 'pfn' variable ?
> same as above
>
>>>    	spin_unlock(&lock);
>>>    }
>>>    
>>> @@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area,
>>> unsigned int order) return page_memalign_order_area(area, order,
>>> order); }
>>>    
>>> -void *alloc_pages(unsigned int order)
>>> -{
>>> -	return alloc_pages_area(AREA_ANY, order);
>>> -}
>>> -
>>>    /*
>>>     * Allocates (1 << order) physically contiguous aligned pages.
>>>     * Returns NULL if the allocation was not possible.
>>> @@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area,
>>> size_t alignment, size_t size) return
>>> page_memalign_order_area(area, size, alignment); }
>>>    
>>> -void *memalign_pages(size_t alignment, size_t size)
>>> -{
>>> -	return memalign_pages_area(AREA_ANY, alignment, size);
>>> -}
>>> -
>>> -/*
>>> - * Allocates one page
>>> - */
>>> -void *alloc_page()
>>> -{
>>> -	return alloc_pages(0);
>>> -}
>>>    
>>>    static struct alloc_ops page_alloc_ops = {
>>>    	.memalign = memalign_pages,
>>> @@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
>>>     * - the memory area to add does not overlap with existing areas
>>>     * - the memory area to add has at least 5 pages available
>>>     */
>>> -static void _page_alloc_init_area(u8 n, uintptr_t start_pfn,
>>> uintptr_t top_pfn) +static void _page_alloc_init_area(u8 n, pfn_t
>>> start_pfn, pfn_t top_pfn) {
>>>    	size_t table_size, npages, i;
>>>    	struct mem_area *a;
>>> @@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n,
>>> uintptr_t start_pfn, uintptr_t top_pfn)
>>>    	/* fill in the values of the new area */
>>>    	a = areas + n;
>>> -	a->page_states = (void *)(start_pfn << PAGE_SHIFT);
>>> +	a->page_states = pfn_to_virt(start_pfn);
>>>    	a->base = start_pfn + table_size;
>>>    	a->top = top_pfn;
>>>    	npages = top_pfn - a->base;
>>> @@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n,
>>> uintptr_t start_pfn, uintptr_t top_pfn) for (i = 0; i < MAX_AREAS;
>>> i++) { if (!(areas_mask & BIT(i)))
>>>    			continue;
>>> -		assert(!area_or_metadata_contains(areas + i,
>>> start_pfn));
>>> -		assert(!area_or_metadata_contains(areas + i,
>>> top_pfn - 1));
>>> -		assert(!area_or_metadata_contains(a,
>>> PFN(areas[i].page_states)));
>>> -		assert(!area_or_metadata_contains(a, areas[i].top
>>> - 1));
>>> +		assert(!area_contains_pfn(areas + i, start_pfn));
>>> +		assert(!area_contains_pfn(areas + i, top_pfn - 1));
>>> +		assert(!area_contains_pfn(a,
>>> virt_to_pfn(areas[i].page_states)));
>>> +		assert(!area_contains_pfn(a, areas[i].top - 1));
>>>    	}
>>>    	/* initialize all freelists for the new area */
>>>    	for (i = 0; i < NLISTS; i++)
>>> -		a->freelists[i].next = a->freelists[i].prev =
>>> a->freelists + i;
>>> +		a->freelists[i].prev = a->freelists[i].next =
>>> a->freelists + i;
>>>    	/* initialize the metadata for the available memory */
>>>    	for (i = a->base; i < a->top; i += 1ull << order) {
>>> @@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n,
>>> uintptr_t start_pfn, uintptr_t top_pfn) assert(order < NLISTS);
>>>    		/* initialize the metadata and add to the
>>> freelist */ memset(a->page_states + (i - a->base), order,
>>> BIT(order));
>>> -		list_add(a->freelists + order, (void *)(i <<
>>> PAGE_SHIFT));
>>> +		list_add(a->freelists + order, pfn_to_virt(i));
>>>    	}
>>>    	/* finally mark the area as present */
>>>    	areas_mask |= BIT(n);
>>>    }
>>>    
>>> -static void __page_alloc_init_area(u8 n, uintptr_t cutoff,
>>> uintptr_t base_pfn, uintptr_t *top_pfn) +static void
>>> __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t
>>> *top_pfn) { if (*top_pfn > cutoff) {
>>>    		spin_lock(&lock);
>>> @@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n,
>>> uintptr_t cutoff, uintptr_t base_pfn, u
>>>     * Prerequisites:
>>>     * see _page_alloc_init_area
>>>     */
>>> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
>>> top_pfn) +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
>>> phys_addr_t top_pfn) {
>>>    	if (n != AREA_ANY_NUMBER) {
>>>    		__page_alloc_init_area(n, 0, base_pfn, &top_pfn);
>>>   

Re: [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas

[Krish Sadhukhan]

On 1/4/21 5:19 AM, Claudio Imbrenda wrote:
> On Thu, 24 Dec 2020 10:17:00 -0800
> Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
>
>> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
>>> Fix the definitions of the memory areas.
>>>
>>> Bring the headers in line with the rest of the asm headers, by
>>> having the appropriate #ifdef _ASM$ARCH_ guarding the headers.
>>
>> Should we mention MAX_AREAS that the patch adds in each arch-specific
>> header ?
> not sure, it's a minor detail. I mentioned above that I "Fix the
> definitions of the memory areas".
>
> MAX_AREAS is not totally new, now it's per-arch instead of being in the
> generic code.
>
>>> Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")
>>>
>>> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
>>> ---
>>>    lib/asm-generic/memory_areas.h |  9 ++++-----
>>>    lib/arm/asm/memory_areas.h     | 11 +++--------
>>>    lib/arm64/asm/memory_areas.h   | 11 +++--------
>>>    lib/powerpc/asm/memory_areas.h | 11 +++--------
>>>    lib/ppc64/asm/memory_areas.h   | 11 +++--------
>>>    lib/s390x/asm/memory_areas.h   | 13 ++++++-------
>>>    lib/x86/asm/memory_areas.h     | 27 ++++++++++++++++-----------
>>>    lib/alloc_page.h               |  3 +++
>>>    lib/alloc_page.c               |  4 +---
>>>    9 files changed, 42 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/lib/asm-generic/memory_areas.h
>>> b/lib/asm-generic/memory_areas.h index 927baa7..3074afe 100644
>>> --- a/lib/asm-generic/memory_areas.h
>>> +++ b/lib/asm-generic/memory_areas.h
>>> @@ -1,11 +1,10 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
>>> +#define __ASM_GENERIC_MEMORY_AREAS_H__
>>>    
>>>    #define AREA_NORMAL_PFN 0
>>>    #define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>>>    
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#define MAX_AREAS 1
>>>    
>>>    #endif
>>> diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
>>> index 927baa7..c723310 100644
>>> --- a/lib/arm/asm/memory_areas.h
>>> +++ b/lib/arm/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMARM_MEMORY_AREAS_H_
>>> +#define _ASMARM_MEMORY_AREAS_H_
>>>    
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>    
>>>    #endif
>>> diff --git a/lib/arm64/asm/memory_areas.h
>>> b/lib/arm64/asm/memory_areas.h index 927baa7..18e8ca8 100644
>>> --- a/lib/arm64/asm/memory_areas.h
>>> +++ b/lib/arm64/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMARM64_MEMORY_AREAS_H_
>>> +#define _ASMARM64_MEMORY_AREAS_H_
>>>    
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>    
>>>    #endif
>>> diff --git a/lib/powerpc/asm/memory_areas.h
>>> b/lib/powerpc/asm/memory_areas.h index 927baa7..76d1738 100644
>>> --- a/lib/powerpc/asm/memory_areas.h
>>> +++ b/lib/powerpc/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
>>> +#define _ASMPOWERPC_MEMORY_AREAS_H_
>>>    
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>    
>>>    #endif
>>> diff --git a/lib/ppc64/asm/memory_areas.h
>>> b/lib/ppc64/asm/memory_areas.h index 927baa7..b9fd46b 100644
>>> --- a/lib/ppc64/asm/memory_areas.h
>>> +++ b/lib/ppc64/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMPPC64_MEMORY_AREAS_H_
>>> +#define _ASMPPC64_MEMORY_AREAS_H_
>>>    
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>    
>>>    #endif
>>> diff --git a/lib/s390x/asm/memory_areas.h
>>> b/lib/s390x/asm/memory_areas.h index 4856a27..827bfb3 100644
>>> --- a/lib/s390x/asm/memory_areas.h
>>> +++ b/lib/s390x/asm/memory_areas.h
>>> @@ -1,16 +1,15 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMS390X_MEMORY_AREAS_H_
>>> +#define _ASMS390X_MEMORY_AREAS_H_
>>>    
>>> -#define AREA_NORMAL_PFN BIT(31-12)
>>> +#define AREA_NORMAL_PFN (1 << 19)
>>>    #define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>>>    
>>>    #define AREA_LOW_PFN 0
>>>    #define AREA_LOW_NUMBER 1
>>> -#define AREA_LOW 2
>>> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>>>    
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#define MAX_AREAS 2
>>>    
>>>    #define AREA_DMA31 AREA_LOW
>>>    
>>> diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
>>> index 952f5bd..e84016f 100644
>>> --- a/lib/x86/asm/memory_areas.h
>>> +++ b/lib/x86/asm/memory_areas.h
>>> @@ -1,21 +1,26 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASM_X86_MEMORY_AREAS_H_
>>> +#define _ASM_X86_MEMORY_AREAS_H_
>>>    
>>>    #define AREA_NORMAL_PFN BIT(36-12)
>>>    #define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>>>    
>>> -#define AREA_PAE_HIGH_PFN BIT(32-12)
>>> -#define AREA_PAE_HIGH_NUMBER 1
>>> -#define AREA_PAE_HIGH 2
>>> +#define AREA_HIGH_PFN BIT(32-12)
>>> +#define AREA_HIGH_NUMBER 1
>>> +#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
>>>    
>>> -#define AREA_LOW_PFN 0
>>> +#define AREA_LOW_PFN BIT(24-12)
>>>    #define AREA_LOW_NUMBER 2
>>> -#define AREA_LOW 4
>>> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>>>    
>>> -#define AREA_PAE (AREA_PAE | AREA_LOW)
>>> +#define AREA_LOWEST_PFN 0
>>> +#define AREA_LOWEST_NUMBER 3
>>> +#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
>>>    
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#define MAX_AREAS 4
>>> +
>>> +#define AREA_DMA24 AREA_LOWEST
>>> +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
>>> +#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
>>>    
>>>    #endif
>>> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
>>> index 816ff5d..b6aace5 100644
>>> --- a/lib/alloc_page.h
>>> +++ b/lib/alloc_page.h
>>> @@ -10,6 +10,9 @@
>>>    
>>>    #include <asm/memory_areas.h>
>>>    
>>> +#define AREA_ANY -1
>>> +#define AREA_ANY_NUMBER 0xff
>>> +
>>>    /* Returns true if the page allocator has been initialized */
>>>    bool page_alloc_initialized(void);
>>>    
>>> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
>>> index 685ab1e..ed0ff02 100644
>>> --- a/lib/alloc_page.c
>>> +++ b/lib/alloc_page.c
>>> @@ -19,8 +19,6 @@
>>>    #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
>>>    #define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>>>    
>>> -#define MAX_AREAS	6
>>> -
>>>    #define ORDER_MASK	0x3f
>>>    #define ALLOC_MASK	0x40
>>>    #define SPECIAL_MASK	0x80
>>> @@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t
>>> base_pfn, uintptr_t top_pfn) return;
>>>    	}
>>>    #ifdef AREA_HIGH_PFN
>>> -	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN),
>>> base_pfn, &top_pfn);
>>> +	__page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN,
>>> base_pfn, &top_pfn);
>>
>> Surprising that the compiler didn't complain !
>>
>>>    #endif
>>>    	__page_alloc_init_area(AREA_NORMAL_NUMBER,
>>> AREA_NORMAL_PFN, base_pfn, &top_pfn); #ifdef AREA_LOW_PFN
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator

[Claudio Imbrenda]

On Sun, 27 Dec 2020 22:31:56 -0800
Nadav Amit <nadav.amit@gmail.com> wrote:

[...]

> >> Thanks for doing that. Before I test it, did you also fix the issue
> >> of x86’s setup_vm() [1]?
> >> 
> >> [1] https://www.spinics.net/lists/kvm/msg230470.html  
> > 
> > unfortunately no, because I could not reproduce it.
> > 
> > In theory setup_vm should just work, since it is called twice, but
> > on different address ranges.
> > 
> > The only issue I can think of is if the second call overlaps
> > multiple areas.
> > 
> > can you send me the memory map of that machine you're running the
> > tests on? (/proc/iomem)  
> 
> Sorry for the late response.
> 
> I see two calls to _page_alloc_init_area() with AREA_LOW_NUMBER, one
> with (base_pfn=621, top_pfn=bfdd0) and one with (base_pfn=100000
> top_pfn=240000).

ok, I could reproduce the issue now.

to put it simply, the old code is broken.

I could not reproduce the issue with the new code, so you should be
able to test it.