Re: [Minios-devel] [UNIKRAFT PATCHv3 19/25] plat/kvm: arm64: Calculate pagetable size dynamically by image size

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Wei Chen,

Please find the comments inline:

Thanks & Regards
Sharan

On 12/13/18 10:15 AM, Wei Chen wrote:
> The previous L3 page table size is a fixed value based on the
> assumption that, image size will not exceed 2MB. But that's not
> true, some applications like node.js will be about 40MB after
> converting to Unikernel. So in this patch, we calculate the
> page table size by real image size. In order to mapping the
> image occupied memory to L3 page table, we round up the image
> size with 2MB. If we don't round up the image size with 2MB.
> Some memory that is not occupied by image but shared the same
> 2MB block with image tail will be unmapped in page table.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   plat/common/include/sections.h      | 31 +++++++++++++++++++++++++
>   plat/kvm/arm/entry64.S              | 36 +++++++++++++++++++++++++++--
>   plat/kvm/arm/pagetable64.S          |  4 +++-
>   plat/kvm/arm/setup.c                |  2 +-
>   plat/kvm/include/kvm-arm/arm64/mm.h | 14 ++++++-----
>   5 files changed, 77 insertions(+), 10 deletions(-)
>
> diff --git a/plat/common/include/sections.h b/plat/common/include/sections.h
> index b32389c..5342e42 100644
> --- a/plat/common/include/sections.h
> +++ b/plat/common/include/sections.h
> @@ -35,6 +35,7 @@
>   #ifndef __PLAT_CMN_SECTIONS_H__
>   #define __PLAT_CMN_SECTIONS_H__
>   
> +#ifndef __ASSEMBLY__
>   /*
>    * Following global variables are defined in image link scripts, and some
>    * variables are optional and may be unavailable on some architectures
> @@ -94,4 +95,34 @@ extern char _end[];
>   #define __BSS_START   __uk_image_symbol(__bss_start)
>   #define __END         __uk_image_symbol(_end)
>   
> +#endif /*__ASSEMBLY__ */
> +
> +/*
> + * Because the section is 4KB alignment, and we will assign different
> + * attributes for different sections. We roundup image size to 2MB to
> + * avoid making holes in L3 table
> + *
> + * L2 table
> + * |-----------|    L3 table
> + * |   2MB     |===>|-----------|
> + * |-----------|    |  4KB      | entry#0
> + *                  |-----------|
> + *                  |  ...      |
> + *                  |           |
> + *                  |-----------|
> + *                  |  4KB      | entry# for last page of real image
> + *                  |-----------|
> + *                  |  4KB      | entry# for round up memory
> + *                  |-----------|
> + *                  |  ...      |
> + *                  |-----------|
> + *                  |  4KB      | entry#511
> + *                  |-----------|
> + * If we don't roundup the image size to 2MB, some memory that is not
> + * occupied by image but shared the same 2MB block with image tail will
> + * not be mapped in page table.
> + */
> +#define        IMAGE_ROUNDUP_SHIFT     20
Shouldn't this be 21, if we want to round to 2MB?

> +#define IMAGE_ROUNDUP_SIZE     (0x1 << (IMAGE_ROUNDUP_SHIFT))
> +
>   #endif /* __PLAT_CMN_SECTIONS_H__ */
> diff --git a/plat/kvm/arm/entry64.S b/plat/kvm/arm/entry64.S
> index d08b49e..867772f 100644
> --- a/plat/kvm/arm/entry64.S
> +++ b/plat/kvm/arm/entry64.S
> @@ -35,6 +35,12 @@
>   #include <uk/asm.h>
>   #include <kvm-arm/mm.h>
>   #include <arm/cpu_defs.h>
> +#include <sections.h>
> +
> +.global page_table_size
> +.data
> +page_table_size:
> +    .dword 0x0
>   
>   /*
>    * The registers used by _libkvmplat_start:
> @@ -43,6 +49,29 @@
>   
>   .text
>   ENTRY(_libkvmplat_entry)
> +       /* Caculate the image size */
s/caculate/calculate


> +       ldr x25, =_dtb
> +       ldr x26, =_end
> +       mov x15, x25
> +       sub x15, x26, x15
> +
> +       /* Round up the size to 2MB */
> +       mov x17, #(IMAGE_ROUNDUP_SIZE - 1)
> +       add x15, x15, x17
> +       lsr x15, x15, #IMAGE_ROUNDUP_SHIFT
> +       lsl x15, x15, #IMAGE_ROUNDUP_SHIFT
> +
> +       /*
> +        * How many bytes would be used for L3_TABLE
> +        * ((x15 >> 21) << 12)
> +        */
> +       lsr x17, x15, #9
> +
> +       /* Total bytes for pagetable */
> +       add x17, x17, #L0_TABLE_SIZE
> +       add x17, x17, #L1_TABLE_SIZE
> +       add x17, x17, #L2_TABLE_SIZE
> +
>   
>   	/*
>          * Clean the boot stack and page table. As _end, PAGE_TABLE_SIZE and
> @@ -50,8 +79,7 @@ ENTRY(_libkvmplat_entry)
>          * 64-bytes alignment too. Execute 4 stp consecutively without boundary
>          * check would be safe here.
>          */
> -       ldr x26, =_end
> -       add x27, x26, #PAGE_TABLE_SIZE
> +       add x27, x26, x17
>         add x27, x27, #__STACK_SIZE
>   1:
>         stp xzr, xzr, [x26], #16
> @@ -79,6 +107,10 @@ ENTRY(_libkvmplat_entry)
>         /* Set the context id */
>         msr contextidr_el1, xzr
>   
> +	/* Save page table size for later usage */
> +       ldr x26, =page_table_size
> +       str x17, [x26]
> +
>         /* Create a pagetable to do PA == VA mapping */
>         bl create_pagetables
>   
> diff --git a/plat/kvm/arm/pagetable64.S b/plat/kvm/arm/pagetable64.S
> index a00b71c..bd473bf 100644
> --- a/plat/kvm/arm/pagetable64.S
> +++ b/plat/kvm/arm/pagetable64.S
> @@ -233,7 +233,9 @@ ENTRY(start_mmu)
>          */
>         ldr x0, =_data
>         ldr x1, =_end
> -       add x1, x1, #PAGE_TABLE_SIZE
> +       ldr x2, =page_table_size
> +       ldr x2, [x2]
> +       add x1, x2, x1
>         add x1, x1, #__STACK_SIZE
>         sub x1, x1, x0
>         bl clean_and_invalidate_dcache_range
> diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
> index 5ffb629..7bd75bd 100644
> --- a/plat/kvm/arm/setup.c
> +++ b/plat/kvm/arm/setup.c
> @@ -146,7 +146,7 @@ static void _init_dtb_mem(void)
>   
>   	max_addr = mem_base + mem_size;
>         _libkvmplat_pagetable =(void *) ALIGN_DOWN((size_t)__END, __PAGE_SIZE);
> -       _libkvmplat_heap_start = _libkvmplat_pagetable + PAGE_TABLE_SIZE;
> +       _libkvmplat_heap_start = _libkvmplat_pagetable + page_table_size;
>         _libkvmplat_mem_end = (void *) max_addr;
>   
>   	/* AArch64 require stack be 16-bytes alignment by default */
> diff --git a/plat/kvm/include/kvm-arm/arm64/mm.h 
> b/plat/kvm/include/kvm-arm/arm64/mm.h
> index 23bc658..9ec1273 100644
> --- a/plat/kvm/include/kvm-arm/arm64/mm.h
> +++ b/plat/kvm/include/kvm-arm/arm64/mm.h
> @@ -69,15 +69,17 @@
>   #define L2_TABLE_SIZE   __PAGE_SIZE
>   
>   /*
> - * As Unikraft image's size is very tiny, from tens to hundreds kilo
> - * bytes. So one page for L3_TABLE is enough for us to manage section
> - * attributes of image.
> + * We will use Unikraft image's size to caculate the L3_TABLE_SIZE.
> + * Because we allocate one page for L2 TABLE, fo the max image size
> + * would be 1GB. It would be enough for current stage.
>    */
>   #define L3_TABLE_OFFSET (L2_TABLE_OFFSET + L2_TABLE_SIZE)
> -#define L3_TABLE_SIZE   __PAGE_SIZE
> +
> +#ifndef __ASSEMBLY__
>   
>   /* Total memory size that will be used by pagetable */
> -#define PAGE_TABLE_SIZE (L0_TABLE_SIZE + L1_TABLE_SIZE + \
> -                               L2_TABLE_SIZE + L3_TABLE_SIZE)
> +extern uint64_t page_table_size;
> +
> +#endif /*__ASSEMBLY__ */
>   
>   #endif /* __KVM_ARM_64_MM_H__ */
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel