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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Thu, Nov 09, 2017 at 11:37:39AM +0000, Julien Grall wrote:
> 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.
I read the p2m_set_entry, I am think it is not clear to me. :(

Can we just keep the current code for page table, we have other things
to do which have high prioiries.

I prefer to keep the current code, and if we really need to change the
page table, we can do it the sync exception.

> 
> 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).
okay I will add TLB operation...

> 
> > 
> >   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.
I can add more comment in the __setup_initial_pgtable().
I have already make at least 2M space for the page table:

   ---------------------------------------------------------
    /* Add 2 MiB for rounding above */
    add     x2, x2, #1                 /* x2 := total number of entries */
   ---------------------------------------------------------

And I think it is a good way to allocate page to setup the page table
during the boot.


If you think it is not good, please give me a better one.


> 
> 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.
Please see the comment above.

Thanks
Huang Shijie

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