Re: [PATCH v6 06/17] Introduce alloc_ops

[Andrew Jones <drjones@redhat.com>]

On Fri, Jul 11, 2014 at 11:41:34AM +0200, Paolo Bonzini wrote:
> Il 11/07/2014 10:55, Andrew Jones ha scritto:
> >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.
> 
> Ok, having actually read the code a bit more, it looks like (correct me if
> I'm wrong) you really need the early allocator only for the physical
> addresses of the vring.  You are using it for the virtqueue structs too,
> that is not strictly necessary but I won't complain if you leave it that
> way.
> 
> All you need is a pointer to the bottom and top of the free area, then you
> can do a very simple allocator like sbrk.
> 
> So the API can look like this:
> 
> 	void phys_alloc_init(uintptr_t base, uintptr_t top);
> 	uintptr_t phys_alloc(size_t size);
> 	uintptr_t phys_zalloc(size_t size);
> 	uintptr_t phys_alloc_aligned(size_t size, size_t align);
> 	uintptr_t phys_zalloc_aligned(size_t size, size_t align);
> 
> If you keep the code that allocates the virtqueue, you can just add a
> phys_to_virt() on the result.
> 
> In order to keep the debugging code, just have a
> 
> 	struct phys_alloc_region {
> 		void *ptr;
> 		size_t size;
> 	};
> 
> 	struct phys_alloc_region *first_region;
> 
> and place these structs at the end of the region passed to phys_alloc_init.
> That is, phys_alloc will allocate SIZE bytes from the BASE, and a single
> struct phys_alloc_region from the TOP.  The FIRST_REGION is initialized with
> the same value as TOP in phys_alloc_init, and remains fixed there; the last
> region is represented by TOP.  To dump all regions, just walk the array from
> first_region down to top.
> 
> When paging is enabled, it can itself allocate memory using phys_alloc for
> page tables and the like.
> 
> >If memregions look useful outside of arm, then I can certainly move
> >them and early_[m]alloc, etc. to common code.
> 
> Yeah, they can live in lib/.  Only the call to phys_alloc_init needs to be
> in lib/arm.
>

Yes, you are correct, and I like your design and API better, but with one
change. We should introduce phys_addr_t=u64 to common code as well, and then
use that instead of uintptr_t/size_t. I'll do this for v7.

Thanks,
drew