[kvm-unit-tests PATCH v1 0/8] Minor fixes, improvements, and cleanup

[kvm-unit-tests PATCH v1 0/8] Minor fixes, improvements, and cleanup

[Claudio Imbrenda]

This patch series provides a bunch of small improvements, fixes and
cleanups.
Some of these fixes are needed for an upcoming series that will
significantly refactor and improve the memory allocators.

Claudio Imbrenda (8):
  x86/cstart.S: initialize stack before using it
  x86: add missing PAGE_ALIGN macro from page.h
  lib: use PAGE_ALIGN
  lib/alloc.c: add overflow check for calloc
  lib: Fix a typo and add documentation comments
  lib/vmalloc: fix potential race and non-standard pointer arithmetic
  lib/alloc_page: make get_order return unsigned int
  lib/vmalloc: add locking and a check for initialization

 lib/x86/asm/page.h |  2 ++
 lib/alloc_page.h   |  2 +-
 lib/alloc_phys.h   |  2 +-
 lib/vmalloc.h      |  8 ++++++++
 lib/alloc.c        | 36 +++++++++++++++++++++++++++++++++++-
 lib/alloc_page.c   |  2 +-
 lib/vmalloc.c      | 34 +++++++++++++++++++++++-----------
 x86/cstart.S       |  2 +-
 8 files changed, 72 insertions(+), 16 deletions(-)

-- 
2.26.2

[kvm-unit-tests PATCH v1 1/8] x86/cstart.S: initialize stack before using it

[Claudio Imbrenda]

It seems the 32-bit initialization code uses the stack before actually
initializing it.

Probably the boot loader leaves a reasonable value in the stack pointer so
this issue has not been noticed before.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 x86/cstart.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/cstart.S b/x86/cstart.S
index 38ac19b..fa62e09 100644
--- a/x86/cstart.S
+++ b/x86/cstart.S
@@ -96,13 +96,13 @@ MSR_GS_BASE = 0xc0000101
 
 .globl start
 start:
+        mov $stacktop, %esp
         push %ebx
         call setup_multiboot
         call setup_libcflat
         mov mb_cmdline(%ebx), %eax
         mov %eax, __args
         call __setup_args
-        mov $stacktop, %esp
         setup_percpu_area
         call prepare_32
         jmpl $8, $start32
-- 
2.26.2

[kvm-unit-tests PATCH v1 2/8] x86: add missing PAGE_ALIGN macro from page.h

[Claudio Imbrenda]

The PAGE_ALIGN macro is present in all other page.h headers, including
the generic one.

This patch adds the missing PAGE_ALIGN macro to ib/x86/asm/page.h

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

diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
index 073580a..7e2a3dd 100644
--- a/lib/x86/asm/page.h
+++ b/lib/x86/asm/page.h
@@ -19,6 +19,8 @@ typedef unsigned long pgd_t;
 
 #ifndef __ASSEMBLY__
 
+#define PAGE_ALIGN(addr)        ALIGN(addr, PAGE_SIZE)
+
 #ifdef __x86_64__
 #define LARGE_PAGE_SIZE	(512 * PAGE_SIZE)
 #else
-- 
2.26.2

[kvm-unit-tests PATCH v1 3/8] lib: use PAGE_ALIGN

[Claudio Imbrenda]

Since now PAGE_ALIGN is available in all architectures, start using it in
common code to improve readability.

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

diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 5022a31..74b785c 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -41,7 +41,7 @@ void *vmap(phys_addr_t phys, size_t size)
 	void *mem, *p;
 	unsigned pages;
 
-	size = (size + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1);
+	size = PAGE_ALIGN(size);
 	pages = size / PAGE_SIZE;
 	mem = p = alloc_vpages(pages);
 
@@ -60,7 +60,7 @@ static void *vm_memalign(size_t alignment, size_t size)
 	unsigned pages;
 
 	assert(alignment <= PAGE_SIZE);
-	size = (size + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1);
+	size = PAGE_ALIGN(size);
 	pages = size / PAGE_SIZE;
 	mem = p = alloc_vpages(pages);
 	while (pages--) {
@@ -104,7 +104,7 @@ void setup_vm()
 	 * so that it can be used to allocate page tables.
 	 */
 	if (!page_alloc_initialized()) {
-		base = (base + PAGE_SIZE - 1) & -PAGE_SIZE;
+		base = PAGE_ALIGN(base);
 		top = top & -PAGE_SIZE;
 		free_pages(phys_to_virt(base), top - base);
 	}
@@ -113,7 +113,7 @@ void setup_vm()
 	phys_alloc_get_unused(&base, &top);
 	page_root = setup_mmu(top);
 	if (base != top) {
-		base = (base + PAGE_SIZE - 1) & -PAGE_SIZE;
+		base = PAGE_ALIGN(base);
 		top = top & -PAGE_SIZE;
 		free_pages(phys_to_virt(base), top - base);
 	}
-- 
2.26.2

[kvm-unit-tests PATCH v1 4/8] lib/alloc.c: add overflow check for calloc

[Claudio Imbrenda]

Add an overflow check for calloc to prevent potential multiplication overflow.

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

diff --git a/lib/alloc.c b/lib/alloc.c
index ed8f5f9..f4aa87a 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -6,9 +6,43 @@ void *malloc(size_t size)
 	return memalign(sizeof(long), size);
 }
 
+static bool mult_overflow(size_t a, size_t b)
+{
+#if BITS_PER_LONG == 32
+	/* 32 bit system, easy case: just use u64 */
+	return (u64)a * (u64)b >= (1ULL << 32);
+#else
+#ifdef __SIZEOF_INT128__
+	/* if __int128 is available use it (like the u64 case above) */
+	unsigned __int128 res = a;
+	res *= b;
+	res >>= 64;
+	return res != 0;
+#else
+	u64 tmp;
+
+	if ((a >> 32) && (b >> 32))
+		return true;
+	if (!(a >> 32) && !(b >> 32))
+		return false;
+	tmp = (u32)a;
+	tmp *= (u32)b;
+	tmp >>= 32;
+	if (a < b)
+		tmp += a * (b >> 32);
+	else
+		tmp += b * (a >> 32);
+	return tmp >> 32;
+#endif /* __SIZEOF_INT128__ */
+#endif /* BITS_PER_LONG == 32 */
+}
+
 void *calloc(size_t nmemb, size_t size)
 {
-	void *ptr = malloc(nmemb * size);
+	void *ptr;
+
+	assert(!mult_overflow(nmemb, size));
+	ptr = malloc(nmemb * size);
 	if (ptr)
 		memset(ptr, 0, nmemb * size);
 	return ptr;
-- 
2.26.2

[kvm-unit-tests PATCH v1 5/8] lib: Fix a typo and add documentation comments

[Claudio Imbrenda]

Fix a typo in lib/alloc_phys.h and add documentation comments to all
functions in lib/vmalloc.h

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_phys.h | 2 +-
 lib/vmalloc.h    | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/alloc_phys.h b/lib/alloc_phys.h
index ea38f91..611aa70 100644
--- a/lib/alloc_phys.h
+++ b/lib/alloc_phys.h
@@ -39,7 +39,7 @@ extern void phys_alloc_show(void);
 /*
  * phys_alloc_get_unused allocates all remaining memory from the region
  * passed to phys_alloc_init, returning the newly allocated memory's base
- * and top addresses. phys_allo_get_unused will still return base and top
+ * and top addresses. phys_alloc_get_unused will still return base and top
  * when no free memory is remaining, but base will equal top.
  */
 extern void phys_alloc_get_unused(phys_addr_t *p_base, phys_addr_t *p_top);
diff --git a/lib/vmalloc.h b/lib/vmalloc.h
index 3658b80..2b563f4 100644
--- a/lib/vmalloc.h
+++ b/lib/vmalloc.h
@@ -3,15 +3,23 @@
 
 #include <asm/page.h>
 
+/* Allocate consecutive virtual pages (without backing) */
 extern void *alloc_vpages(ulong nr);
+/* Allocate one virtual page (without backing) */
 extern void *alloc_vpage(void);
+/* Set the top of the virtual address space */
 extern void init_alloc_vpage(void *top);
+/* Set up the virtual allocator; also sets up the page allocator if needed */
 extern void setup_vm(void);
 
+/* Set up paging */
 extern void *setup_mmu(phys_addr_t top);
+/* Walk the page table and resolve the virtual address to a physical address */
 extern phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *virt);
+/* Map the virtual address to the physical address for the given page tables */
 extern pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *virt);
 
+/* Map consecutive physical pages */
 void *vmap(phys_addr_t phys, size_t size);
 
 #endif
-- 
2.26.2

[kvm-unit-tests PATCH v1 6/8] lib/vmalloc: fix potential race and non-standard pointer arithmetic

[Claudio Imbrenda]

The pointer vfree_top should only be accessed with the lock held, so
make sure we return a local copy of the pointer taken safely inside the
lock.

Also avoid doing pointer arithmetic on void pointers. Gcc allows it but
it is an ugly hack. Use uintptr_t for doing maths on the pointer.

This will also come useful in upcoming patches.

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

diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 74b785c..83e34aa 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -20,10 +20,16 @@ static void *page_root;
 
 void *alloc_vpages(ulong nr)
 {
+	uintptr_t ptr;
+
 	spin_lock(&lock);
-	vfree_top -= PAGE_SIZE * nr;
+	ptr = (uintptr_t)vfree_top;
+	ptr -= PAGE_SIZE * nr;
+	vfree_top = (void *)ptr;
 	spin_unlock(&lock);
-	return vfree_top;
+
+	/* Cannot return vfree_top here, we are outside the lock! */
+	return (void *)ptr;
 }
 
 void *alloc_vpage(void)
-- 
2.26.2

[kvm-unit-tests PATCH v1 7/8] lib/alloc_page: make get_order return unsigned int

[Claudio Imbrenda]

Since get_order never returns a negative value, it makes sense to make
it return an unsigned int.

The returned value will be in practice always very small, a u8 would
probably also do the trick.

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

diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index e6a51d2..6181299 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -15,6 +15,6 @@ void *alloc_pages(unsigned long order);
 void free_page(void *page);
 void free_pages(void *mem, unsigned long size);
 void free_pages_by_order(void *mem, unsigned long order);
-int get_order(size_t size);
+unsigned int get_order(size_t size);
 
 #endif
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 7c8461a..8769c3f 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -176,7 +176,7 @@ void page_alloc_ops_enable(void)
 	alloc_ops = &page_alloc_ops;
 }
 
-int get_order(size_t size)
+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 v1 8/8] lib/vmalloc: add locking and a check for initialization

[Claudio Imbrenda]

Make sure init_alloc_vpage is never called when vmalloc is in use.

Get both init_alloc_vpage and setup_vm to use the lock.

For setup_vm we only check at the end because at least on some
architectures setup_mmu can call init_alloc_vpage, which would cause
a deadlock.

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

diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 83e34aa..10f15af 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -37,11 +37,6 @@ void *alloc_vpage(void)
 	return alloc_vpages(1);
 }
 
-void init_alloc_vpage(void *top)
-{
-	vfree_top = top;
-}
-
 void *vmap(phys_addr_t phys, size_t size)
 {
 	void *mem, *p;
@@ -96,6 +91,14 @@ void __attribute__((__weak__)) find_highmem(void)
 {
 }
 
+void init_alloc_vpage(void *top)
+{
+	spin_lock(&lock);
+	assert(alloc_ops != &vmalloc_ops);
+	vfree_top = top;
+	spin_unlock(&lock);
+}
+
 void setup_vm()
 {
 	phys_addr_t base, top;
@@ -124,5 +127,8 @@ void setup_vm()
 		free_pages(phys_to_virt(base), top - base);
 	}
 
+	spin_lock(&lock);
+	assert(alloc_ops != &vmalloc_ops);
 	alloc_ops = &vmalloc_ops;
+	spin_unlock(&lock);
 }
-- 
2.26.2

Re: [kvm-unit-tests PATCH v1 0/8] Minor fixes, improvements, and cleanup

[Paolo Bonzini]

On 22/06/20 18:21, Claudio Imbrenda wrote:
> This patch series provides a bunch of small improvements, fixes and
> cleanups.
> Some of these fixes are needed for an upcoming series that will
> significantly refactor and improve the memory allocators.
> 
> Claudio Imbrenda (8):
>   x86/cstart.S: initialize stack before using it
>   x86: add missing PAGE_ALIGN macro from page.h
>   lib: use PAGE_ALIGN
>   lib/alloc.c: add overflow check for calloc
>   lib: Fix a typo and add documentation comments
>   lib/vmalloc: fix potential race and non-standard pointer arithmetic
>   lib/alloc_page: make get_order return unsigned int
>   lib/vmalloc: add locking and a check for initialization
> 
>  lib/x86/asm/page.h |  2 ++
>  lib/alloc_page.h   |  2 +-
>  lib/alloc_phys.h   |  2 +-
>  lib/vmalloc.h      |  8 ++++++++
>  lib/alloc.c        | 36 +++++++++++++++++++++++++++++++++++-
>  lib/alloc_page.c   |  2 +-
>  lib/vmalloc.c      | 34 +++++++++++++++++++++++-----------
>  x86/cstart.S       |  2 +-
>  8 files changed, 72 insertions(+), 16 deletions(-)
> 

Queued, thanks.

Paolo

Re: [kvm-unit-tests PATCH v1 4/8] lib/alloc.c: add overflow check for calloc

[Thomas Huth]

On 22/06/2020 18.21, Claudio Imbrenda wrote:
> Add an overflow check for calloc to prevent potential multiplication overflow.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/alloc.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index ed8f5f9..f4aa87a 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -6,9 +6,43 @@ void *malloc(size_t size)
>  	return memalign(sizeof(long), size);
>  }
>  
> +static bool mult_overflow(size_t a, size_t b)
> +{
> +#if BITS_PER_LONG == 32
> +	/* 32 bit system, easy case: just use u64 */
> +	return (u64)a * (u64)b >= (1ULL << 32);
> +#else
> +#ifdef __SIZEOF_INT128__
> +	/* if __int128 is available use it (like the u64 case above) */
> +	unsigned __int128 res = a;
> +	res *= b;
> +	res >>= 64;
> +	return res != 0;
> +#else
> +	u64 tmp;
> +
> +	if ((a >> 32) && (b >> 32))
> +		return true;
> +	if (!(a >> 32) && !(b >> 32))
> +		return false;
> +	tmp = (u32)a;
> +	tmp *= (u32)b;
> +	tmp >>= 32;
> +	if (a < b)
> +		tmp += a * (b >> 32);
> +	else
> +		tmp += b * (a >> 32);
> +	return tmp >> 32;
> +#endif /* __SIZEOF_INT128__ */
> +#endif /* BITS_PER_LONG == 32 */
> +}

This caused a new regression in the CI:

 https://gitlab.com/huth/kvm-unit-tests/-/jobs/606930599#L1754

 https://travis-ci.com/github/huth/kvm-unit-tests/jobs/352515116#L546

lib/alloc.c: In function 'mult_overflow':
lib/alloc.c:24:9: error: right shift count >= width of type
[-Werror=shift-count-overflow]
   24 |  if ((a >> 32) && (b >> 32))
      |         ^~
lib/alloc.c:24:22: error: right shift count >= width of type
[-Werror=shift-count-overflow]
   24 |  if ((a >> 32) && (b >> 32))
      |                      ^~
lib/alloc.c:26:10: error: right shift count >= width of type
[-Werror=shift-count-overflow]
   26 |  if (!(a >> 32) && !(b >> 32))
      |          ^~
lib/alloc.c:26:24: error: right shift count >= width of type
[-Werror=shift-count-overflow]
   26 |  if (!(a >> 32) && !(b >> 32))
      |                        ^~
lib/alloc.c:32:17: error: right shift count >= width of type
[-Werror=shift-count-overflow]
   32 |   tmp += a * (b >> 32);
      |                 ^~
lib/alloc.c:34:17: error: right shift count >= width of type
[-Werror=shift-count-overflow]
   34 |   tmp += b * (a >> 32);
      |                 ^~
cc1: all warnings being treated as errors

Claudio, any ideas how to fix that?

Paolo, could you maybe push your staging branches to Githab or Gitlab
first, to see whether there are any regressions in Travis or Gitlab-CI
before you push the branch to the main repo? ... almost all of the
recent updates broke something, and analyzing the problems afterwards is
a little bit cumbersome...

 Thomas

Re: [kvm-unit-tests PATCH v1 4/8] lib/alloc.c: add overflow check for calloc

[Paolo Bonzini]

On 23/06/20 07:57, Thomas Huth wrote:
> cc1: all warnings being treated as errors
> 
> Claudio, any ideas how to fix that?

I guess it's just

diff --git a/lib/alloc.c b/lib/alloc.c
index f4aa87a..6c89f98 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -1,5 +1,6 @@
 #include "alloc.h"
 #include "asm/page.h"
+#include "bitops.h"

 void *malloc(size_t size)
 {

> Paolo, could you maybe push your staging branches to Githab or Gitlab
> first, to see whether there are any regressions in Travis or Gitlab-CI
> before you push the branch to the main repo? ... almost all of the
> recent updates broke something, and analyzing the problems afterwards is
> a little bit cumbersome...

Ok, I'll catch you on IRC to discuss this.

Paolo