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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Shijie,

On 09/11/17 05:33, Huang Shijie wrote:
> On Wed, Nov 08, 2017 at 10:38:09AM +0000, Julien Grall wrote:
> > Hi Shijie,
> >
> > On 08/11/17 08:54, Huang Shijie wrote:
> > > 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.
> > And you can be clever and mixing 2MB/1GB mapping depending on what you need to 
> > do.
> Yes, we can. but the code looks a little ugly.
I am sorry but avoiding using switch between 1GB/2MB because the code 
might look ugly is not an excuse.
Xen is able to decide what is the best page size to use (see 
p2m_set_entry) and the code is readable and fairly clean.
So maybe you should think a bit more how to design it nicely.

[...]

> > > > > +                     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()..
> > I looked at the only caller in this patch and there are no BUG()...
> >
> > So your interface is already wrong (see below). But a BUG() in alloc_new_page()
> > seem pretty odd. You want to let the caller decides what to do if the allocation
> > fails? In fact even the x86 do return an error on allocation failure. So why not
> > Arm?
> okay, I will add the return check for it...
> > > > > +        }
> > > > > +
> > > > > +        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.
> > When is that future? As I said previously, page-table are going to be used
> > in multiple difference places. Implementing the page-table correctly from
> > the beginning does not hurt and will save time later on for all of us later
> > on.
>   1.) we flush the TLB in the _start.
>   2.) We setup the table after 1.)
>   3.) we will not change the table in future.
I still don't understand why you keep saying table will not change in 
the future when I gave you proof of the invert...
We are going to need to modify page-table in near future (for instance 
for PV drivers/libc).
>   Why do we need the TLB maintainable?
Well, you already have mapping in the page-tables and may end up to 
replace them. For instance, you already have a bit of RAM already mapped 
and likely going to rewrite the PTE for them.
Are they going to be exactly the same value? Will you shatter some mappings?

If TLB maintainance is not necessary. Then documentation in the code 
explaining why is always helpful for developer looking at the code later on.
> > > > > +
> > > > > +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.
> > Sorry but I can't find this code in x86... The variable is here but seem to
> > be used differently. Would you mind giving a pointer?
> in arch/x86/mm.c, search first_free_pfn.
Have you read what I wrote? I am perfectly happy to explain if you don't 
understand something, but it is annoying me to see you don't seen to 
bother considering what I say. It is already the second time in this 
e-mail...
I know where the first_free_pfn is used in x86 code... My point was that 
the usage is completely different from here. It makes sense over there, 
here it looks quite hackish.
In any case, what works for x86 does not mean this is the best way to do 
it...
> > > > > +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?
> > I would prefer early_alloc_page()
> okay.
> > > > > +{
> > > > > +    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?
> > Yes. Where does the mapping between first_free_pfn and its virtual address
> > is done?
> In the __setup_initial_pgtable.
Really? AFAICT, __setup_initial_pgtable will only map the kernel and DT. 
You can't assume there are will be free RAM mapped.
Cheers,

--
Julien Grall

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