Re: [PATCH 4/9] move x86's simple heap management to common code

[Christoffer Dall <christoffer.dall@linaro.org>]

On Wed, Dec 04, 2013 at 05:42:52PM +0100, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  config/config-x86-common.mak |  1 +
>  lib/heap.c                   | 43 +++++++++++++++++++++++++++++++++++++++++++
>  lib/heap.h                   |  8 ++++++++
>  lib/x86/vm.c                 | 33 ++-------------------------------
>  4 files changed, 54 insertions(+), 31 deletions(-)
>  create mode 100644 lib/heap.c
>  create mode 100644 lib/heap.h
> 
> diff --git a/config/config-x86-common.mak b/config/config-x86-common.mak
> index 917cbbf801a65..8a78f7a6c8849 100644
> --- a/config/config-x86-common.mak
> +++ b/config/config-x86-common.mak
> @@ -2,6 +2,7 @@
>  
>  all: test_cases
>  
> +cflatobjs += lib/heap.o
>  cflatobjs += lib/x86/io.o
>  cflatobjs += lib/x86/smp.o
>  cflatobjs += lib/x86/vm.o
> diff --git a/lib/heap.c b/lib/heap.c
> new file mode 100644
> index 0000000000000..e52ee0edba163
> --- /dev/null
> +++ b/lib/heap.c
> @@ -0,0 +1,43 @@
> +#include "libcflat.h"
> +
> +static size_t pagesize;
> +static void *free_head;
> +
> +void heap_init(void *start, size_t size, size_t page_size)
> +{
> +	unsigned long s = (unsigned long)start;
> +	void *p = start;
> +
> +	pagesize = page_size;
> +
> +	if (s != (s & ~(pagesize - 1))) {

you're just testing 'if (s & (pagesize -1))' right?

> +		s += pagesize;
> +		s &= ~(pagesize - 1);
> +		p = (void *)s;
> +	}

a one-line comment on this block saying 'page-align start of heap would
be nice.

> +
> +	while (size >= pagesize) {
> +		*(void **)p = free_head;
> +		free_head = p;
> +		p += pagesize;
> +		size -= pagesize;
> +	}

you could also be nice and comment this block of code, saying something
like "set up linked list of free pages using the pages themselves as the
data structure" if you should feel so inclined.

why are you not trusting start to be page aligned but you are trusting
size to be?  If size is smaller than pagesize then this loop will go
nuts won't it?

> +}
> +
> +void *alloc_page(void)
> +{
> +	void *p;
> +
> +	if (!free_head)
> +		return NULL;
> +
> +	p = free_head;
> +	free_head = *(void **)free_head;
> +	return p;
> +}
> +
> +void free_page(void *page)
> +{
> +	*(void **)page = free_head;
> +	free_head = page;
> +}
> diff --git a/lib/heap.h b/lib/heap.h
> new file mode 100644
> index 0000000000000..78b893021ae92
> --- /dev/null
> +++ b/lib/heap.h
> @@ -0,0 +1,8 @@
> +#ifndef _HEAP_H_
> +#define _HEAP_H_
> +#include "libcflat.h"
> +
> +extern void heap_init(void *start, size_t size, size_t page_size);
> +extern void *alloc_page(void);
> +extern void free_page(void *page);
> +#endif
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 188bf57587aa7..725cfe33259c2 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -1,6 +1,7 @@
>  #include "fwcfg.h"
>  #include "vm.h"
>  #include "libcflat.h"
> +#include "heap.h"
>  
>  #define PAGE_SIZE 4096ul
>  #ifdef __x86_64__
> @@ -9,38 +10,8 @@
>  #define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
>  #endif
>  
> -static void *free = 0;
>  static void *vfree_top = 0;
>  
> -static void free_memory(void *mem, unsigned long size)
> -{
> -    while (size >= PAGE_SIZE) {
> -	*(void **)mem = free;
> -	free = mem;
> -	mem += PAGE_SIZE;
> -	size -= PAGE_SIZE;
> -    }
> -}
> -
> -void *alloc_page()
> -{
> -    void *p;
> -
> -    if (!free)
> -	return 0;
> -
> -    p = free;
> -    free = *(void **)free;
> -
> -    return p;
> -}
> -
> -void free_page(void *page)
> -{
> -    *(void **)page = free;
> -    free = page;
> -}
> -
>  extern char edata;
>  static unsigned long end_of_memory;
>  
> @@ -185,7 +156,7 @@ static void setup_mmu(unsigned long len)
>  void setup_vm()
>  {
>      end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
> -    free_memory(&edata, end_of_memory - (unsigned long)&edata);
> +    heap_init(&edata, end_of_memory - (unsigned long)&edata, PAGE_SIZE);
>      setup_mmu(end_of_memory);
>  }
>  
> -- 
> 1.8.1.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

-- 
Christoffer