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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Tue, Nov 14, 2017 at 01:40:58PM +0000, Julien Grall wrote:
> 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
When there is a page table issue(such as page fault), fix it by the sync 
exception, just as
the linux kernel does.

> way you implemented the page-tables is not going to work well. You don't
> support removing mapping for instance.
I have not implemented the unmapping yet..

> 
> 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?
Can we do it gradually?  First, make it runs;secondly, make it runs better;
the last, run perfect.


> 
> [...]
> 
> > > > > > > 
> > > > > > > > +
> > > > > > > > +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.
okay, I can change the code like this:
  -------------------------------------------------------
         /* Add 4 MiB for rounding above */
         add     x2, x2, #2                 /* x2 := total number of entries */
  -------------------------------------------------------

Add extra 2M memory for the page table.  

> 
> > 
> > 
> > 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
How many pages we should reserved in the BSS? :)


> will be used to bootstrap the allocator.

Thanks
Huang Shijie

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