[kvm-unit-tests PATCH v2 0/4] More lib/alloc cleanup and a minor improvement

[kvm-unit-tests PATCH v2 0/4] More lib/alloc cleanup and a minor improvement

[Claudio Imbrenda]

Some more cleanup of lib/alloc in light of upcoming changes

The first real feature: allow aligned virtual allocations with
alignment greater than one page.

Also export a function for allocating aligned non-backed virtual pages.

v1->v2
* rename helper function to alloc_vpages_aligned, call it directly
* alloc_vpages_aligned now expects a page order as alignment

Claudio Imbrenda (4):
  lib/vmalloc: fix pages count local variable to be size_t
  lib/alloc_page: change some parameter types
  lib/alloc_page: move get_order and is_power_of_2 to a bitops.h
  lib/vmalloc: allow vm_memalign with alignment > PAGE_SIZE

 lib/alloc_page.h |  7 +++----
 lib/bitops.h     | 10 ++++++++++
 lib/libcflat.h   |  5 -----
 lib/vmalloc.h    |  3 +++
 lib/alloc.c      |  1 +
 lib/alloc_page.c | 13 ++++---------
 lib/vmalloc.c    | 37 ++++++++++++++++++++++++++++---------
 7 files changed, 49 insertions(+), 27 deletions(-)

-- 
2.26.2

[kvm-unit-tests PATCH v2 1/4] lib/vmalloc: fix pages count local variable to be size_t

[Claudio Imbrenda]

Since size is of type size_t, size >> PAGE_SHIFT might still be too big
for a normal unsigned int.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 lib/vmalloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 10f15af..9237a0f 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -40,7 +40,7 @@ void *alloc_vpage(void)
 void *vmap(phys_addr_t phys, size_t size)
 {
 	void *mem, *p;
-	unsigned pages;
+	size_t pages;
 
 	size = PAGE_ALIGN(size);
 	pages = size / PAGE_SIZE;
@@ -58,7 +58,7 @@ void *vmap(phys_addr_t phys, size_t size)
 static void *vm_memalign(size_t alignment, size_t size)
 {
 	void *mem, *p;
-	unsigned pages;
+	size_t pages;
 
 	assert(alignment <= PAGE_SIZE);
 	size = PAGE_ALIGN(size);
-- 
2.26.2

Re: [kvm-unit-tests PATCH v2 1/4] lib/vmalloc: fix pages count local variable to be size_t

[Jim Mattson]

On Mon, Jul 6, 2020 at 9:43 AM Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
>
> Since size is of type size_t, size >> PAGE_SHIFT might still be too big
> for a normal unsigned int.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

[kvm-unit-tests PATCH v2 2/4] lib/alloc_page: change some parameter types

[Claudio Imbrenda]

For size parameters, size_t is probably semantically more appropriate
than unsigned long (although they map to the same value).

For order, unsigned long is just too big. Also, get_order returns an
unsigned int anyway.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 lib/alloc_page.h | 6 +++---
 lib/alloc_page.c | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index 6181299..d9aceb7 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -11,10 +11,10 @@
 bool page_alloc_initialized(void);
 void page_alloc_ops_enable(void);
 void *alloc_page(void);
-void *alloc_pages(unsigned long order);
+void *alloc_pages(unsigned int order);
 void free_page(void *page);
-void free_pages(void *mem, unsigned long size);
-void free_pages_by_order(void *mem, unsigned long order);
+void free_pages(void *mem, size_t size);
+void free_pages_by_order(void *mem, unsigned int order);
 unsigned int get_order(size_t size);
 
 #endif
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 8769c3f..f16eaad 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -21,7 +21,7 @@ bool page_alloc_initialized(void)
 	return freelist != 0;
 }
 
-void free_pages(void *mem, unsigned long size)
+void free_pages(void *mem, size_t size)
 {
 	void *old_freelist;
 	void *end;
@@ -53,7 +53,7 @@ void free_pages(void *mem, unsigned long size)
 	spin_unlock(&lock);
 }
 
-void free_pages_by_order(void *mem, unsigned long order)
+void free_pages_by_order(void *mem, unsigned int order)
 {
 	free_pages(mem, 1ul << (order + PAGE_SHIFT));
 }
@@ -79,7 +79,7 @@ void *alloc_page()
  * Allocates (1 << order) physically contiguous and naturally aligned pages.
  * Returns NULL if there's no memory left.
  */
-void *alloc_pages(unsigned long order)
+void *alloc_pages(unsigned int order)
 {
 	/* Generic list traversal. */
 	void *prev;
@@ -150,7 +150,7 @@ void free_page(void *page)
 static void *page_memalign(size_t alignment, size_t size)
 {
 	unsigned long n = ALIGN(size, PAGE_SIZE) >> PAGE_SHIFT;
-	unsigned long order;
+	unsigned int order;
 
 	if (!size)
 		return NULL;
-- 
2.26.2

Re: [kvm-unit-tests PATCH v2 2/4] lib/alloc_page: change some parameter types

[Jim Mattson]

On Mon, Jul 6, 2020 at 9:43 AM Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
>
> For size parameters, size_t is probably semantically more appropriate
> than unsigned long (although they map to the same value).
>
> For order, unsigned long is just too big. Also, get_order returns an
> unsigned int anyway.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [kvm-unit-tests PATCH v2 2/4] lib/alloc_page: change some parameter types

[Thomas Huth]

On 06/07/2020 18.43, Claudio Imbrenda wrote:
> For size parameters, size_t is probably semantically more appropriate
> than unsigned long (although they map to the same value).
> 
> For order, unsigned long is just too big. Also, get_order returns an
> unsigned int anyway.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/alloc_page.h | 6 +++---
>  lib/alloc_page.c | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index 6181299..d9aceb7 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -11,10 +11,10 @@
>  bool page_alloc_initialized(void);
>  void page_alloc_ops_enable(void);
>  void *alloc_page(void);
> -void *alloc_pages(unsigned long order);
> +void *alloc_pages(unsigned int order);
>  void free_page(void *page);
> -void free_pages(void *mem, unsigned long size);
> -void free_pages_by_order(void *mem, unsigned long order);
> +void free_pages(void *mem, size_t size);
> +void free_pages_by_order(void *mem, unsigned int order);
>  unsigned int get_order(size_t size);
>  
>  #endif
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 8769c3f..f16eaad 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -21,7 +21,7 @@ bool page_alloc_initialized(void)
>  	return freelist != 0;
>  }
>  
> -void free_pages(void *mem, unsigned long size)
> +void free_pages(void *mem, size_t size)

 Hi Claudio,

this patch broke 32-bit x86 and arm builds:

 https://travis-ci.com/github/huth/kvm-unit-tests/jobs/360418977#L693
 https://travis-ci.com/github/huth/kvm-unit-tests/jobs/360418980#L545

I think you either need to adjust the format string, or cast the argument.

 Thomas

[kvm-unit-tests PATCH v2 3/4] lib/alloc_page: move get_order and is_power_of_2 to a bitops.h

[Claudio Imbrenda]

The functions get_order and is_power_of_2 are simple and should
probably be in a header, like similar simple functions in bitops.h

Since they concern bit manipulation, the logical place for them is in
bitops.h

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 lib/alloc_page.h |  1 -
 lib/bitops.h     | 10 ++++++++++
 lib/libcflat.h   |  5 -----
 lib/alloc.c      |  1 +
 lib/alloc_page.c |  5 -----
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index d9aceb7..88540d1 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -15,6 +15,5 @@ void *alloc_pages(unsigned int order);
 void free_page(void *page);
 void free_pages(void *mem, size_t size);
 void free_pages_by_order(void *mem, unsigned int order);
-unsigned int get_order(size_t size);
 
 #endif
diff --git a/lib/bitops.h b/lib/bitops.h
index b310a22..308aa86 100644
--- a/lib/bitops.h
+++ b/lib/bitops.h
@@ -74,4 +74,14 @@ static inline unsigned long fls(unsigned long word)
 }
 #endif
 
+static inline bool is_power_of_2(unsigned long n)
+{
+	return n && !(n & (n - 1));
+}
+
+static inline unsigned int get_order(size_t size)
+{
+	return size ? fls(size) + !is_power_of_2(size) : 0;
+}
+
 #endif
diff --git a/lib/libcflat.h b/lib/libcflat.h
index 7092af2..ec0f58b 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -147,11 +147,6 @@ do {									\
 	}								\
 } while (0)
 
-static inline bool is_power_of_2(unsigned long n)
-{
-	return n && !(n & (n - 1));
-}
-
 /*
  * One byte per bit, a ' between each group of 4 bits, and a null terminator.
  */
diff --git a/lib/alloc.c b/lib/alloc.c
index 6c89f98..9d89d24 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -1,4 +1,5 @@
 #include "alloc.h"
+#include "bitops.h"
 #include "asm/page.h"
 #include "bitops.h"
 
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index f16eaad..fa3c527 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -175,8 +175,3 @@ void page_alloc_ops_enable(void)
 {
 	alloc_ops = &page_alloc_ops;
 }
-
-unsigned int get_order(size_t size)
-{
-	return is_power_of_2(size) ? fls(size) : fls(size) + 1;
-}
-- 
2.26.2

[kvm-unit-tests PATCH v2 4/4] lib/vmalloc: allow vm_memalign with alignment > PAGE_SIZE

[Claudio Imbrenda]

Allow allocating aligned virtual memory with alignment larger than only
one page.

Add a check that the backing pages were actually allocated.

Export the alloc_vpages_aligned function to allow users to allocate
non-backed aligned virtual addresses.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 lib/vmalloc.h |  3 +++
 lib/vmalloc.c | 35 +++++++++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/lib/vmalloc.h b/lib/vmalloc.h
index 2b563f4..8b158f5 100644
--- a/lib/vmalloc.h
+++ b/lib/vmalloc.h
@@ -5,6 +5,9 @@
 
 /* Allocate consecutive virtual pages (without backing) */
 extern void *alloc_vpages(ulong nr);
+/* Allocate consecutive and aligned virtual pages (without backing) */
+extern void *alloc_vpages_aligned(ulong nr, unsigned int alignment_order);
+
 /* Allocate one virtual page (without backing) */
 extern void *alloc_vpage(void);
 /* Set the top of the virtual address space */
diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 9237a0f..e0c7b6b 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -12,19 +12,28 @@
 #include "alloc.h"
 #include "alloc_phys.h"
 #include "alloc_page.h"
+#include <bitops.h>
 #include "vmalloc.h"
 
 static struct spinlock lock;
 static void *vfree_top = 0;
 static void *page_root;
 
-void *alloc_vpages(ulong nr)
+/*
+ * Allocate a certain number of pages from the virtual address space (without
+ * physical backing).
+ *
+ * nr is the number of pages to allocate
+ * alignment_pages is the alignment of the allocation *in pages*
+ */
+void *alloc_vpages_aligned(ulong nr, unsigned int align_order)
 {
 	uintptr_t ptr;
 
 	spin_lock(&lock);
 	ptr = (uintptr_t)vfree_top;
 	ptr -= PAGE_SIZE * nr;
+	ptr &= GENMASK_ULL(63, PAGE_SHIFT + align_order);
 	vfree_top = (void *)ptr;
 	spin_unlock(&lock);
 
@@ -32,6 +41,11 @@ void *alloc_vpages(ulong nr)
 	return (void *)ptr;
 }
 
+void *alloc_vpages(ulong nr)
+{
+	return alloc_vpages_aligned(nr, 0);
+}
+
 void *alloc_vpage(void)
 {
 	return alloc_vpages(1);
@@ -55,17 +69,22 @@ void *vmap(phys_addr_t phys, size_t size)
 	return mem;
 }
 
+/*
+ * Allocate virtual memory, with the specified minimum alignment.
+ */
 static void *vm_memalign(size_t alignment, size_t size)
 {
+	phys_addr_t pa;
 	void *mem, *p;
-	size_t pages;
 
-	assert(alignment <= PAGE_SIZE);
-	size = PAGE_ALIGN(size);
-	pages = size / PAGE_SIZE;
-	mem = p = alloc_vpages(pages);
-	while (pages--) {
-		phys_addr_t pa = virt_to_phys(alloc_page());
+	assert(is_power_of_2(alignment));
+
+	size = PAGE_ALIGN(size) / PAGE_SIZE;
+	alignment = get_order(PAGE_ALIGN(alignment) / PAGE_SIZE);
+	mem = p = alloc_vpages_aligned(size, alignment);
+	while (size--) {
+		pa = virt_to_phys(alloc_page());
+		assert(pa);
 		install_page(page_root, pa, p);
 		p += PAGE_SIZE;
 	}
-- 
2.26.2

Re: [kvm-unit-tests PATCH v2 0/4] More lib/alloc cleanup and a minor improvement

[Paolo Bonzini]

On 06/07/20 18:43, Claudio Imbrenda wrote:
> Some more cleanup of lib/alloc in light of upcoming changes
> 
> The first real feature: allow aligned virtual allocations with
> alignment greater than one page.
> 
> Also export a function for allocating aligned non-backed virtual pages.
> 
> v1->v2
> * rename helper function to alloc_vpages_aligned, call it directly
> * alloc_vpages_aligned now expects a page order as alignment
> 
> Claudio Imbrenda (4):
>   lib/vmalloc: fix pages count local variable to be size_t
>   lib/alloc_page: change some parameter types
>   lib/alloc_page: move get_order and is_power_of_2 to a bitops.h
>   lib/vmalloc: allow vm_memalign with alignment > PAGE_SIZE
> 
>  lib/alloc_page.h |  7 +++----
>  lib/bitops.h     | 10 ++++++++++
>  lib/libcflat.h   |  5 -----
>  lib/vmalloc.h    |  3 +++
>  lib/alloc.c      |  1 +
>  lib/alloc_page.c | 13 ++++---------
>  lib/vmalloc.c    | 37 ++++++++++++++++++++++++++++---------
>  7 files changed, 49 insertions(+), 27 deletions(-)
> 

Queued, thanks.

Paolo