Re: [Minios-devel] [PATCH 29/40] arm64: init the memory system before console/xenbus/gic

[Huang Shijie <shijie.huang@xxxxxxx>]

On Mon, Nov 06, 2017 at 05:06:58PM +0000, Julien Grall wrote:
> Hi Shijie,
> 
> On 03/11/17 03:12, Huang Shijie wrote:
> > The mappings for console/xenbus/gic need several pages to setup
> > the page tables.
> > 
> > So we init the memory system before initiating the console/xenbus/gic,
> > and then we can allocate the pages which are needed by
> > the console/xenbus/gic mapping.
> > 
> > we use the 2M memory block for the memory.
> 
> Why 2MB and not 1GB mapping when possible? Also, what if the guest is using
2MB is a proper size for most of the memory. For example, we can use 2M
for 512MB, it is not proper to use 1GB mapping.
> less than 2MB of memory?
Please increase the guest memory, even the mini-os itself is nearly 2MB.

> 
> > 
> > Change-Id: I0d075478c107cf680d1a20989d57c0fcd727ab29
> > Jira: ENTOS-247
> > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > ---
> >   arch/arm/mm.c         | 98 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   arch/arm/setup.c      |  5 +++
> >   include/arm/arch_mm.h |  1 +
> >   mm.c                  |  5 ++-
> >   4 files changed, 108 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mm.c b/arch/arm/mm.c
> > index edb734f..fb7886c 100644
> > --- a/arch/arm/mm.c
> > +++ b/arch/arm/mm.c
> > @@ -24,6 +24,102 @@ unsigned long allocate_ondemand(unsigned long n, 
> > unsigned long alignment)
> >       BUG();
> >   }
> > +#if defined(__aarch64__)
> > +
> > +#include <arm64/pagetable.h>
> > +
> > +extern lpae_t boot_l1_pgtable[512];
> > +
> > +static inline void set_pgt_entry(lpae_t *ptr, lpae_t val)
> > +{
> > +    *ptr = val;
> > +    dsb(ishst);
> > +    isb();
> > +}
> > +
> > +static void build_pud(lpae_t *pgd, unsigned long vaddr, unsigned long vend,
> 
> Is vend included or excluded from the range mapped?
excluded.
> 
> > +                      paddr_t phys, long mem_type,
> 
> Why the mem_type is long?
I like the long type. :)
Why use other type?

> 
> > +                      paddr_t (*new_page)(void), int level)
> > +{
> > +    lpae_t *pud;
> > +
> > +    pud = (lpae_t *)to_virt((*pgd) & ~ATTR_MASK_L) + l2_pgt_idx(vaddr);
> > +    do {
> > +        if (level == 2)
> > +             set_pgt_entry(pud, (phys & L2_MASK) | mem_type | L2_BLOCK);
> > +        vaddr += L2_SIZE;
> > +        phys += L2_SIZE;
> > +        pud++;
> 
> I am not sure to understand why you handle the third level of page-table in
The #30 is for page-size mapping, this one is for block mapping.

The two patches are used for different purpose.

> a separate patch (see #30). This is making more difficult to find whether
> something is missing.
> 
> However, I am not fully convinced the way you implement the page-tables
> support is the right way to go. Indeed you limit yourself in using the same
> level for all the range of the mapping. This is preventing a range of
> optimization such as using 1GB mapping whenever is possible and switch to
> 2MB when it is not.
> 
> Furthermore, the LPAE format has been nicely design. There are no need to
> implement each level in a separate helper. A loop would be sufficient. I can
> live with that implementation thought.
> 
> Lastly, this code only works when adding entry. We will need to remove
> entries. See unmap_frames used by munmap.
> 
> > +    } while (vaddr < vend);
> > +}
> > +
> > +/* Setup the page table when the MMU is enabled */
> 
> I am not sure to understand this comment.
This function is called after the MMU is enabled.
> 
> > +void build_pagetable(unsigned long vaddr, unsigned long start_pfn,
> 
> Why is that exposed outside mm.c?
I will change it to static.

> 
> > +                     unsigned long max_pfn, long mem_type,
> > +                     paddr_t (*new_page)(void), int level)
> > +{
> > +    paddr_t p_start;
> > +    unsigned long v_end, next;
> > +    lpae_t *pgd;
> > +
> > +    v_end = vaddr + max_pfn * PAGE_SIZE;
> > +    p_start = PFN_PHYS(start_pfn);
> > +
> > +    /* The boot_l1_pgtable can span 512G address space */
> > +    pgd = &boot_l1_pgtable[l1_pgt_idx(vaddr)];
> > +
> > +    do {
> > +        next = (vaddr + L1_SIZE);
> > +        if (next > v_end)
> > +            next = v_end;
> > +
> > +        if ((*pgd) == L1_INVAL) {
> > +            set_pgt_entry(pgd, (new_page()) | PT_PT);
> 
> What if new_page() fails?
It will call the BUG() when it fails, please see alloc_new_page()..
> 
> > +        }
> > +
> > +        build_pud(pgd, vaddr, next, p_start, mem_type, new_page, level);
> > +        p_start += next - vaddr;
> > +        vaddr = next;
> > +        pgd++;
> > +    } while (vaddr != v_end);
> > +}
> 
> The code you implemented for page-table looks far too simple. For instance,
> I am surprised there are no TLB maintenance anywhere.
We can add the TLB maintenance in future in the places it needed.
> 
> > +
> > +static unsigned long first_free_pfn;
> 
> Likely this variable and get_new_page needs more explanation in the code.
okay, I stole this code from the x86 in mini-os.

> 
> > +static paddr_t get_new_page(void)
> 
> I think it would be better if you had early (or something similar) in the
> name. To make clear that is only used for early boot.
What's about get_new_page_early or get_new_page_boot?

> 
> > +{
> > +    paddr_t new_page;
> > +
> > +    memset(pfn_to_virt(first_free_pfn), 0, PAGE_SIZE);
> 
> This is quite confusing, get_new_page() allocates page-table that
> will be used to map the RAM. So how do you bootstrap it?
sorry, I do not quite understand your meaning..

Do you worry about the virtual address of first_free_pfn?

> 
> > +    dsb(ishst);
> > +
> > +    new_page = PFN_PHYS(first_free_pfn);
> > +    first_free_pfn++;
> 
> What if you exhaust in memory? AFAICT, the user will see a failure like a
It will never happen. Each page will used for 1G. If we have 4G memory,
we will only use 4 page. 
 
Please just think it again..    

> data abort. This will not be very obvious to track.

> 
> > +    return new_page;
> > +}
> > +
> > +/*
> > + * This function will setup the page table for the memory system.
> > + *
> > + * Note: We get the page for page table from the first free PFN,
> 
> s/get/allocate/
okay.

> 
> > + *       the get_new_page() will increase the @first_free_pfn.
> 
> But this comment would have been better on top of
> get_new_page/first_free_pfn.
okay..
> 
> > + */
> > +void init_pagetable(unsigned long *start_pfn, unsigned long base_pfn,
> > +                    unsigned long max_pfn)
> > +{
> > +    first_free_pfn = *start_pfn;
> > +
> > +    build_pagetable((unsigned long)pfn_to_virt(base_pfn), base_pfn,
> > +               max_pfn, BLOCK_DEF_ATTR, get_new_page, 2);
> > +    *start_pfn = first_free_pfn;
> > +}
> > +
> > +#else
> > +void init_pagetable(unsigned long *start_pfn, unsigned long base_pfn,
> > +                    unsigned long max_pfn)
> > +{
> > +}
> > +#endif
> > +
> >   void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)
> >   {
> >       int memory;
> > @@ -74,6 +170,8 @@ void arch_init_mm(unsigned long *start_pfn_p, unsigned 
> > long *max_pfn_p)
> >       heap_len = mem_size - (PFN_PHYS(*start_pfn_p) - mem_base);
> >       *max_pfn_p = *start_pfn_p + PFN_DOWN(heap_len);
> > +    init_pagetable(start_pfn_p, PHYS_PFN(mem_base), PHYS_PFN(mem_size));
> > +
> >       printk("Using pages %lu to %lu as free space for heap.\n", 
> > *start_pfn_p, *max_pfn_p);
> >       /* The device tree is probably in memory that we're about to hand 
> > over to the page
> > diff --git a/arch/arm/setup.c b/arch/arm/setup.c
> > index bde30c6..61daca3 100644
> > --- a/arch/arm/setup.c
> > +++ b/arch/arm/setup.c
> > @@ -41,6 +41,11 @@ void arch_init(void *dtb_pointer, paddr_t 
> > physical_offset)
> >       /* Map shared_info page */
> >       HYPERVISOR_shared_info = map_shared_info(NULL);
> > +#if defined(__aarch64__)
> 
> Carve out from arch_init_mm what is necessary but don't call init_mm() early
> just for aarch64. See what x86 did with arch_mm_preinit().
Thanks, I will check that.
> 
> > +    /* We need to init the memory, and then init the console/xenbus/gic */
> > +    init_mm();
> > +#endif

Thanks
Huang Shijie

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