Re: [Minios-devel] [UNIKRAFT PATCHv4 32/43] plat/kvm: Parse memory info from device tree for Arm64

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年7月16日 21:09
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 32/43] plat/kvm: Parse memory
> info from device tree for Arm64
> 
> Hi Wei,
> 
> On 06/07/18 10:03, Wei Chen wrote:
> > QEMU/KVM will store the memory informations like memory
> > region, memory base address and memory size to device
> > tree. We parse these informations for memory allocater and
> 
> s/allocater/allocator/
> 
> Also, this code does not seem very KVM specific. Might be worth thinking
> to move it in the common code at some point.
> 

Yes.

> > new stack setting.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > ---
> >   plat/kvm/arm/setup.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 59 insertions(+)
> >
> > diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
> > index a881152..685308c 100644
> > --- a/plat/kvm/arm/setup.c
> > +++ b/plat/kvm/arm/setup.c
> > @@ -35,10 +35,15 @@
> >   #include <uk/plat/console.h>
> >   #include <uk/assert.h>
> >   #include <uk/essentials.h>
> > +#include <arm/cpu_defs.h>
> >
> >   #define MAX_CMDLINE_SIZE 1024
> >   static char cmdline[MAX_CMDLINE_SIZE];
> >
> > +void *_libkvmplat_pagetable;
> > +void *_libkvmplat_heap_start;
> > +void *_libkvmplat_stack_top;
> > +void *_libkvmplat_mem_end;
> >   void *_libkvmplat_dtb;
> >
> >   static void _init_dtb(void *dtb_pointer)
> > @@ -78,6 +83,53 @@ enocmdl:
> >     strcpy(cmdline, CONFIG_UK_NAME);
> >   }
> >
> > +static void _init_dtb_mem(void)
> 
> Why do you put _ in front of the function name? AFAIK, the name prefixed
> with _ will be reserved for the compiler/libc.
> 

I copied the setup.c framework from xen/arm32/setup.c : )
Currently, Unikraft still lacks of detailed standards, I think we should need
a further work in the future @Simon Kuenzer (simon.kuenzer@xxxxxxxxx)

> > +{
> > +   extern char _text[];
> > +   extern char _end[];
> > +   int memory, prop_len = 0;
> > +   const uint64_t *regs;
> > +   uint64_t mem_base, mem_size, max_addr;
> > +
> > +   /* search for assigned VM memory in DTB */
> > +   if (fdt_num_mem_rsv(_libkvmplat_dtb) != 0)
> > +           uk_printd(DLVL_WARN, "Reserved memory is not supported\n");
> > +
> > +   memory = fdt_node_offset_by_prop_value(_libkvmplat_dtb, -1,
> > +                                              "device_type",
> > +                                              "memory", sizeof("memory"));
> > +   if (memory < 0) {
> > +           uk_printd(DLVL_WARN, "No memory found in DTB\n");
> > +           return;
> > +   }
> > +
> > +   /*
> > +    * QEMU will always provide us at least one bank of memory.
> > +    * unikraft will use the first bank for the time-being.
> > +    */
> > +   regs = fdt_getprop(_libkvmplat_dtb, memory, "reg", &prop_len);
> > +
> > +   /*
> > +    * The property must contain at least the start address
> > +    * and size, each of which is 8-bytes.
> 
> address-cells and size-cells may not be 2 for your platform. As for the
> PL011, it feels like you want to provide wrapper for reading range in
> the DT.
> 

Yes, we have a similar discussion in previous patch.

> > +    */
> > +   if (regs == NULL && prop_len < 16)
> > +           UK_CRASH("Bad 'reg' property: %p %d\n", regs, prop_len);
> > +
> > +   mem_base = fdt64_to_cpu(regs[0]);
> > +   mem_size = fdt64_to_cpu(regs[1]);
> 
> So you are only supported one bank here. It would be nice to at least
> write that assumption in the code and commit message. A warning would
> also be a nice addition if the user specifies more than 1 bank.
> 

That makes sense. I will add some code comments and commit message.

> > +   if (mem_base > (uint64_t)&_text)
> > +           UK_CRASH("Fatal: Image outside of RAM\n");
> > +
> > +   max_addr = mem_base + mem_size;
> > +   _libkvmplat_pagetable =(void *) ALIGN_UP((size_t)&_end, __PAGE_SIZE);
> > +   _libkvmplat_heap_start = _libkvmplat_pagetable + PAGE_TABLE_SIZE;
> > +   _libkvmplat_mem_end = (void *) max_addr;
> > +
> > +   /* AArch64 require stack be 16-bytes alignment by default */
> > +   _libkvmplat_stack_top = (void *) ALIGN_UP(max_addr, __STACK_ALIGN_SIZE);
> > +}
> > +
> >   static void _init_cpufeatures(void)
> >   {
> >     /* TODO */
> > @@ -93,4 +145,11 @@ void _libkvmplat_start(void *dtb_pointer)
> >
> >     /* Get command line from DTB */
> >     _dtb_get_cmdline(cmdline, sizeof(cmdline));
> > +
> > +   /* Initialize memory from DTB */
> > +   _init_dtb_mem();
> > +
> > +   uk_printd(DLVL_INFO, "pagetable start: %p\n", _libkvmplat_pagetable);
> > +   uk_printd(DLVL_INFO, "     heap start: %p\n", _libkvmplat_heap_start);
> > +   uk_printd(DLVL_INFO, "      stack top: %p\n", _libkvmplat_stack_top);
> >   }
> >
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel