Re: [Minios-devel] [PATCH v3 29/43] arm64: set the mapping for console and xenbus

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 16/04/18 07:32, Huang Shijie wrote:
> This patch sets the mapping for console and xenbus.
> Just following what x86 does:
>
>     1.) Add VIRT_KERNEL_AREA/VIRT_DEMAND_AREA to limit
>         the memory ranges for alloc_virt_kernel().
>
>     2.) Change map_frame_virt() to setup the page table for
>         console and xenbus.
map_frame_virt was already implemented. So what's the different with today?

> Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> ---
>   arch/arm/mm.c         | 44 +++++++++++++++++++++++++++++++++++++++-----
>   include/arm/arch_mm.h |  2 ++
>   2 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mm.c b/arch/arm/mm.c
> index ed59159..e83ac70 100644
> --- a/arch/arm/mm.c
> +++ b/arch/arm/mm.c
> @@ -233,12 +233,15 @@ void init_pagetable(unsigned long *start_pfn, unsigned 
> long base,
>       init_pagetable_ok = 1;
>   }
>   
> +static unsigned long virt_kernel_area_end;
Newline here please.

>   void arch_mm_preinit(void *dtb_pointer)
>   {
>       paddr_t **dtb_p = dtb_pointer;
>       paddr_t *dtb = *dtb_p;
>       uintptr_t end = (uintptr_t) &_end;
>   
> +    virt_kernel_area_end = VIRT_KERNEL_AREA;
> +
>       dtb = to_virt(((paddr_t)dtb));
>       first_free_pfn = PFN_UP(to_phys(end));
>       min_mem_pfn = PFN_UP(to_phys(_text) + MIN_MEM_SIZE);
> @@ -254,6 +257,42 @@ void arch_mm_preinit(void *dtb_pointer)
>       *dtb_p = dtb;
>   }
>   
> +static unsigned long alloc_virt_kernel(unsigned n_pages)
> +{
> +    unsigned long addr;
> +
> +    addr = virt_kernel_area_end;
> +    virt_kernel_area_end += PAGE_SIZE * n_pages;
> +    ASSERT(virt_kernel_area_end <= VIRT_DEMAND_AREA);
> +
> +    return addr;
> +}
Couldn't we make the virt allocation common between arm and x86?

> +
> +static paddr_t alloc_new_page(void)
> +{
> +    unsigned long page;
> +
> +    page = alloc_page();
> +    if (!page)
> +        BUG();
> +    memset((void *)page, 0, PAGE_SIZE);
> +    dsb(ishst);
Why the dsb here?

> +    return to_phys(page);
> +}
> +
> +unsigned long map_frame_virt(unsigned long mfn)
> +{
> +    unsigned long addr;
> +    int ret;
> +
> +    addr = alloc_virt_kernel(1);
> +    ret = build_pagetable(addr, mfn, 1, MEM_DEF_ATTR,
> +                    init_pagetable_ok? alloc_new_page: early_alloc_page, 3);
This smell like you want to introduce helper. It does not make sense for 
the caller to care how the page-table will be allocated nor which level.
> +    ASSERT(ret == 0);
> +
> +    return addr;
> +}
> +
Why not implementing the function were it was?

>   void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)
>   {
>       int memory;
> @@ -394,8 +433,3 @@ grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
>   
>       return to_virt(gnttab_table);
>   }
> -
> -unsigned long map_frame_virt(unsigned long mfn)
> -{
> -    return mfn_to_virt(mfn);
> -}
> diff --git a/include/arm/arch_mm.h b/include/arm/arch_mm.h
> index db6e781..4f3fd8f 100644
> --- a/include/arm/arch_mm.h
> +++ b/include/arm/arch_mm.h
> @@ -5,6 +5,8 @@ typedef uint64_t paddr_t;
>   #define PRIpaddr "lx"
>   #define MIN_MEM_SIZE            (0x400000)
>   #define MAX_MEM_SIZE            (1UL << 39)
> +#define VIRT_KERNEL_AREA        ((unsigned long)to_virt(MAX_MEM_SIZE))
> +#define VIRT_DEMAND_AREA        (VIRT_KERNEL_AREA + MAX_MEM_SIZE)
I don't understand the purpose of the 2 variables nor the value you gave 
to them. How come you are using MAX_MEM_SIZE as physical address to find 
the virt address?
Cheers,

>   
>   typedef uint64_t lpae_t;
>   
--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel