Re: [Minios-devel] [PATCH v3 06/43] arm64: add the boot code

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 18/04/18 11:00, Huang Shijie wrote:
> On Mon, Apr 16, 2018 at 06:13:31PM +0100, Julien Grall wrote:
> > On 16/04/18 07:31, Huang Shijie wrote:
> > > +98: ;                                          \
> > > +    .align  2;                                 \
> > > +99:                                            \
> > > +
> > > +    .data
> > > +    .globl _boot_stack
> > > +    .globl boot_l0_pgtable
> > > +    .globl boot_l1_pgtable
> > > +    .globl boot_l2_pgtable
> > > +    .globl idmap_l0_pgtable
> > > +    .globl idmap_l1_pgtable
> > Please put the .globl label before each declaration. But I am not sure why
> > you need to export most of them.
> The code setup the page table may need them, so I should export them.
I only see use of boot_l0_pgtable and not the others. You should never 
need idmap_* one. For the boot_l{1,2}_pgtable, your page-table code 
should walk from the root so there are, IHMO, not necessary.
> > > +
> > > +    .align 12
> > > +boot_l0_pgtable:
> > > +    .fill  PAGE_SIZE,1,0
> > > +boot_l1_pgtable:
> > > +    .fill  PAGE_SIZE,1,0
> > > +boot_l2_pgtable:
> > > +    .fill  PAGE_SIZE,1,0
> > > +idmap_l0_pgtable:
> > > +    .fill  PAGE_SIZE,1,0
> > > +idmap_l1_pgtable:
> > > +    .fill  PAGE_SIZE,1,0
> > > +
> > > +    .align 12
> > > +_boot_stack:
> > > +    .fill  __STACK_SIZE,1,0
> > Can we please use STACK_SIZE and not __STACK_SIZE.
> I will try...
> But I remember I met something wrong in compiling when I used STACK_SIZE.
Can you post the error if there is any? :)

> > > +stack_end:
> > > +
> > > +/*
> > > + * Kernel startup entry point.
> > > + *
> > > + * Please refer to linux kernel file Documentation/arm64/booting.txt
> > > + * for the header format.
> > > + */
> > > +    .text
> > > +
> > > +    b       _start                  /* branch to kernel start, magic */
> > > +    .long   0                       /* reserved */
> > > +    .quad   0x0                     /* Image load offset from start of RAM */
> > > +    .quad   _end - _start           /* Effective Image size */
> > > +    .quad   2                       /* kernel flags: LE, 4K page size */
> > Again, do we really care to have the kernel placed as close as possible to
> > the base address of the DRAM?
> Do you have a better place to place the kernel?
What I was asking is whether you strictly need to have the kernel at the 
beginning of the RAM. You could also let the toolstack decide where to 
put the kernel in the memory. While this is not supported today, this 
would give a bit more flexibility.
> > For reminder, the documentation of that flag is:
> >
> > Bit 3:        Kernel physical placement
> >                          0 - 2MB aligned base should be as close as possible
> > to the base of DRAM, since memory below it is not accessible via the linear
> > mapping
> >                          1 - 2MB aligned base may be anywhere in physical
> > memory
> >
> > How does mini-OS on Arm64 deal with memory below the loaded address of the
> > kernel?
> We do not use the memory below the loaded address of kernel.
> We put kernel at 0xffff000000000000 (48bit).
>
> And we setup the page table for address >= 0xffff000000000000,
> the address below 0xffff000000000000 is not accessed.
I am fairly confused. Is that virtual address or physical address?

> > I am pretty sure you need to clean the cache for the page-table region here
> > to avoid potential dirty lines being evicted and ...
> I am not clear about this.
> is it possible that there is cache for the paget able region?
In short yes. You cannot guarantee there are no dirty or stale cache 
lines for the areas of the memory written by the kernel during boot with 
the MMU off (access will be non-cacheable).
From the boot protocol the only guarantee is:

"The address range corresponding to the loaded kernel image must be
 cleaned to the PoC. In the presence of a system cache or other
 coherent masters with caches enabled, this will typically require
 cache maintenance by VA rather than set/way operations.
 System caches which respect the architected cache maintenance by VA
 operations must be configured and may be enabled.
 System caches which do not respect architected cache maintenance by VA
 operations (not recommended) must be configured and disabled."

> Sorry, I will read more document and reply your following comments...
Feel free to ask questions here if you need to.

Cheers,

--
Julien Grall

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