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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Thu, Apr 26, 2018 at 02:31:32PM +0100, Julien Grall wrote:
> 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?
The old map_frame_virt does not setup the page table, while the new one does.

> 
> >
> >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.
okay.
> 
> >  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?
First step, make it simple, just change the arm code. :)

> 
> >+
> >+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?
Let the page be zeroed.

> 
> >+    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.
The caller does not need to know how many level.

The build_pagetable() is used internally, it is not a API function..
> 
> >+    ASSERT(ret == 0);
> >+
> >+    return addr;
> >+}
> >+
> 
> Why not implementing the function were it was?
We need to setup the page table entry.
> 
> >  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?
This is to keep aligned with x86.
Please check the x86 code.

Thanks
Huang Shijie

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