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

[Julien Grall <julien.grall@xxxxxxx>]

On 28/04/18 10:44, Huang Shijie wrote:
> 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.
It should be explained in the commit message.

> > > 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.
I still don't understand why. Would you mind to expand your thoughts?

> > > +    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.
Well, map_frame_virt is a caller of build_pagetable... And when I see 
how caller are using it then I do wonder why it has to be so difficult 
to use. Clearly map_frame_virt should not have to care how to allocate 
page-table.
> 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.
Hu? You move the function earlier, so surely you have everything in 
hand. So why has it been moved?
> > >   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.
I looked at the x86 code and don't see them to use MAX_MEM_SIZE. So mind 
explaining what's going on? I am not going to ack a patch I don't 
understand.
Cheers,

--
Julien Grall

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