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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Shijie,

On 13/11/17 06:05, Huang Shijie wrote:
> 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.
I don't get the "we can do it the sync exception"... Anyway, as I said, 
the way you implemented the page-tables is not going to work well. You 
don't support removing mapping for instance.
I don't much care whether you follow the idea in p2m_set_entry, but I 
would like to see page-table correctly handled from the beginning. 
Otherwise, how are you going to use Mini-OS correctly?
[...]

> > > > > > > +
> > > > > > > +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.
I don't think your statement: "I have already make at least 2M space for 
the page table" is true.
If you get the whole calculation:

/* Find the size of the kernel */
ldr     x0, =_text                 /* x0 := vaddr(_text)            */
ldr     x1, =_end                  /* x1 := vaddr(_end)             */
sub     x2, x1, x0
/* Get the number of l2 pages to allocate, rounded down */
lsr     x2, x2, #L2_SHIFT
/* Add 2 MiB for rounding above */
add     x2, x2, #1                 /* x2 := total number of entries */

You first get the number of 2MB pages to allocate, rounded down, then add 1.

So if your kernel is less than 2MB, you will allocate only one 2MB page. 
And hence have less than 2MB space for the page table.
As you can see this is really fragile, because you depend on the size of 
the kernel which may change depending on the application linked with.
> If you think it is not good, please give me a better one.
A better solution is to statically allocate in BSS another page-table. 
It will be used to bootstrap the allocator.
Cheers,

--
Julien Grall

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