diff mbox

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

Message ID 1386175377-23086-5-git-send-email-drjones@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Jones Dec. 4, 2013, 4:42 p.m. UTC 
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

Comments

Christoffer Dall Dec. 29, 2013, 6:30 a.m. UTC | #1 
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
Andrew Jones Jan. 2, 2014, 3:17 p.m. UTC | #2 
On Sat, Dec 28, 2013 at 10:30:24PM -0800, Christoffer Dall wrote:
> > +
> > +	if (s != (s & ~(pagesize - 1))) {
> 
> you're just testing 'if (s & (pagesize -1))' right?

yeah, I'll simplify that.

> 
> > +		s += pagesize;
> > +		s &= ~(pagesize - 1);
> > +		p = (void *)s;
> > +	}
> 
> a one-line comment on this block saying 'page-align start of heap would
> be nice.

added

> 
> > +
> > +	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.

added "link free pages"

> 
> 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?

I don't see how. As soon as size is less than pagesize we won't
[re]enter the loop, and thus it can never go negative (big positive).

drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoffer Dall Jan. 2, 2014, 5:17 p.m. UTC | #3 
On Thu, Jan 02, 2014 at 04:17:08PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:30:24PM -0800, Christoffer Dall wrote:
> > > +
> > > +	if (s != (s & ~(pagesize - 1))) {
> > 
> > you're just testing 'if (s & (pagesize -1))' right?
> 
> yeah, I'll simplify that.
> 
> > 
> > > +		s += pagesize;
> > > +		s &= ~(pagesize - 1);
> > > +		p = (void *)s;
> > > +	}
> > 
> > a one-line comment on this block saying 'page-align start of heap would
> > be nice.
> 
> added
> 
> > 
> > > +
> > > +	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.
> 
> added "link free pages"
> 
> > 
> > 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?
> 
> I don't see how. As soon as size is less than pagesize we won't
> [re]enter the loop, and thus it can never go negative (big positive).
> 
yeah you're right. duh.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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))) {
+		s += pagesize;
+		s &= ~(pagesize - 1);
+		p = (void *)s;
+	}
+
+	while (size >= pagesize) {
+		*(void **)p = free_head;
+		free_head = p;
+		p += pagesize;
+		size -= pagesize;
+	}
+}
+
+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);
 }