Re: [PATCH v6 06/17] Introduce alloc_ops

[Andrew Jones <drjones@redhat.com>]

On Fri, Jul 11, 2014 at 10:40:42AM +0200, Paolo Bonzini wrote:
> Il 11/07/2014 10:19, Andrew Jones ha scritto:
> >alloc_ops provide interfaces for alloc(), free() and friends, allowing
> >unit tests and common code to use dynamic memory allocation.
> >arch-specific code must provide the implementations.
> >
> >Signed-off-by: Andrew Jones <drjones@redhat.com>
> >---
> > lib/alloc.c |  2 ++
> > lib/alloc.h | 31 +++++++++++++++++++++++++++++++
> > 2 files changed, 33 insertions(+)
> > create mode 100644 lib/alloc.c
> > create mode 100644 lib/alloc.h
> >
> >diff --git a/lib/alloc.c b/lib/alloc.c
> >new file mode 100644
> >index 0000000000000..868664b4dcaa3
> >--- /dev/null
> >+++ b/lib/alloc.c
> >@@ -0,0 +1,2 @@
> >+#include "alloc.h"
> >+struct alloc_ops alloc_ops;
> >diff --git a/lib/alloc.h b/lib/alloc.h
> >new file mode 100644
> >index 0000000000000..c8cd61b387a9a
> >--- /dev/null
> >+++ b/lib/alloc.h
> >@@ -0,0 +1,31 @@
> >+#ifndef _ALLOC_H_
> >+#define _ALLOC_H_
> >+#include "libcflat.h"
> >+
> >+struct alloc_ops {
> >+	void *(*alloc)(size_t size);
> >+	void *(*alloc_aligned)(size_t size, size_t align);
> >+	void (*free)(const void *addr);
> >+};
> >+
> >+extern struct alloc_ops alloc_ops;
> >+
> >+static inline void *alloc(size_t size)
> >+{
> >+	assert(alloc_ops.alloc);
> >+	return alloc_ops.alloc(size);
> >+}
> >+
> >+static inline void *alloc_aligned(size_t size, size_t align)
> >+{
> >+	assert(alloc_ops.alloc_aligned);
> >+	return alloc_ops.alloc_aligned(size, align);
> >+}
> >+
> >+static inline void free(const void *addr)
> >+{
> >+	assert(alloc_ops.free);
> >+	alloc_ops.free(addr);
> >+}
> >+
> >+#endif
> >
> 
> Why do you need the wrappers?

A unit test may want to change the allocator after setting up paging.

> Could you just have lib/malloc.c define the
> three functions, with interface in lib/stdlib.h and lib/memregion.h?

If memregions look useful outside of arm, then I can certainly move
them and early_[m]alloc, etc. to common code.

> 
> Also, please call them with the "right" C names:
> 
>        void *malloc(size_t size);
>        void free(void *ptr);
>        void *calloc(size_t nmemb, size_t size);
>        void *memalign(size_t alignment, size_t size);
> 
> Only calloc should do the memset.

Sure, but I still think they should just be wrappers. Initial
implementations based on memregions would of course be named
something else. I guess just "early_*" or "memregion_*".

> 
> Later, x86 could also add a memregion_init call and be able to use
> malloc/free.

sounds good.

drew