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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> 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.
> +{
> +       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.
> +        */
> +       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.
> +       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