Re: [PATCH v6 06/17] Introduce alloc_ops

[Andrew Jones <drjones@redhat.com>]

On Fri, Jul 11, 2014 at 12:07:25PM +0200, Andrew Jones wrote:
> 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.
>

I decided to merge the two designs (memregions and your proposal). I
took your API (except for the phys_addr_t change I already mentioned), but
kept the explicit array for the allocation logging. Keeping the array (which
should be allocated in low memory) ensures we can always walk it. Using TOP
as you suggest may not work in all cases. I set the array size to 256,
which should be [way] more than enough, so we don't really need a
"limitless" log anyway.

drew