|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [PATCH 03/40] arm64: add the boot code
Hi Shijie, On 06/11/17 09:10, Huang Shijie wrote: On Fri, Nov 03, 2017 at 04:42:05PM +0000, Julien Grall wrote:+ .macro mem_clear, addr, len + adr x0, \addr + add x1, x0, \len +1: stp xzr, xzr, [x0], #16 + stp xzr, xzr, [x0], #16 + stp xzr, xzr, [x0], #16 + stp xzr, xzr, [x0], #16 + cmp x0, x1 + b.lo 1b + .endm + + .dataIf you move all those information in bss you avoid to worry about the initialization later on.A good idea. I will do it in the next version.This is also assuming that clearing the memory is done much earlier than doing it in C. In any case, clearing BSS much earlier would also avoid potential screw up if a global is been used in the code.+ .globl _boot_stack + .globl boot_l1_pgtable, boot_l2_pgtable, boot_l2_pgtable1 + .globl idmap_pgtablePlease put the .globl before each symbols.okay.+ + .align 12 +boot_l1_pgtable: + .space PAGE_SIZE +boot_l2_pgtable: + .space PAGE_SIZE +boot_l2_pgtable1: + .space PAGE_SIZE +idmap_pgtable: + .space PAGE_SIZE + + .align 12 +_boot_stack: + .space (4 * PAGE_SIZE)I thought here was a define about the size of the stack?Yes. we can add a macro for the stack size. The macro already exists. See STACK_SIZE. +stack_end: + +/* + * Kernel startup entry point. + * + * Please refer to kernel file Documentation/arm64/booting.txtMini-OS is a kernel. So which kernel is it?Fix it in next version.+ * 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 flages: LE, 4K page size */s/flages/flags/okay.Also do we really care to have the kernel placed as close as possible to the base of the DRAM?I have no idea about this.... Here the documentation of this flag: Bit 3: Kernel physical placement0 - 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? + .quad 0 /* reserved */ + .quad 0 /* reserved */ + .quad 0 /* reserved */ + .byte 0x41 /* Magic number, "ARM\x64" */ + .byte 0x52 + .byte 0x4d + .byte 0x64 + .long 0 /* reserved */ + +/* + * Get the phys-offset, and save it in x22 + */ +ENTRY(_calc_offset)ENTRY will export the symbol. Can you explain why you need to do that?The ENTRY is not needed. I will change it.+ ldr x22, =_start /* x0 := vaddr(_start) */ + adr x21, _start /* x21 := paddr(_start) */ + sub x22, x22, x21 /* x22 := phys-offset (vaddr - paddr) */Now, I understand why you switch from + to - in patch #5. Arm32 is calculating the offset using paddr - vaddr. Please stay consistent. I believe the negative offset will badly screw arm32 so it might be worth to stick to paddr - vaddr.I will spend some time to think about this issue...+ ret +ENDPROC(_calc_offset) + +/* + * Setup the memory region attribute; + * Setup the TCR. + */ +ENTRY(_setup_cpu)Ditto for exporting the symbol.okay.+ /* + * Setup memory attribute type tables + * + * Memory region attributes for LPAE: + * + * n = AttrIndx[2:0] + * n MAIR + * DEVICE_nGnRnE 000 00000000 (0x00) + * DEVICE_nGnRE 001 00000100 (0x04) + * DEVICE_GRE 010 00001100 (0x0c) + * NORMAL_NC 011 01000100 (0x44) + * NORMAL 100 11111111 (0xff) + */ + ldr x0, =(SET_MAIR(0x00, MEM_DEVICE_nGnRnE) | \ + SET_MAIR(0x04, MEM_DEVICE_nGnRE) | \ + SET_MAIR(0x0c, MEM_DEVICE_GRE) | \ + SET_MAIR(0x44, MEM_NORMAL_NC) | \ + SET_MAIR(0xff, MEM_NORMAL)) + msr mair_el1, x0 + + /* + * Setup translation control register (TCR) + */ + ldr x0, =(TCR_TxSZ(VA_BITS) | TCR_ASID16 | TCR_TG1_4K | TCR_FLAGS )Please remove one space after TCR_TG1_4K and before ). Also, can you explain why you define some flags in TCR_FLAGS and other directly here?Emm, it really looks strange. I can remove it in next version. FWIW, I would prefer them to be open-coded here (i.e no TCR_FLAGS) with a proper comment explain why using those flags. + adr x4, boot_l1_pgtable /* x4 := paddr (boot_l1_pgtable) */ + adr x5, boot_l2_pgtable /* x5 := paddr (boot_l2_pgtable) */ + + /* 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 */I believe, this algo will result to sometimes map 2MB more than necessary. This would happen if the code is exactly 2MB.yes. but it is okay. Why is that okay? More that handling properly the size is not that difficult... [...] + orr x7, x7, x9, lsl #12 + + /* Store the L1 entry */ + str x7, [x4, x3, lsl #3] + + /* Start to map the Device-Tree */Per the kernel protocol: "The device tree blob (dtb) must be placed on an 8-byte boundary and must not exceed 2 megabytes in size. Since the dtb will be mapped cacheable using blocks of up to 2 megabytes in size, it must not be placed within any 2M region which must be mapped with any specific attributes. NOTE: versions prior to v4.2 also require that the DTB be placed within the 512 MB region starting at text_offset bytes below the kernel Image." So the DTB may span over two 2MB regions. But do you really need to map the DT from assembly code?Yes, we cannot allocate pages for the page tables in C language. What do you mean here? You can modify page-table in C language... So why would it be a problem for mapping the Device-Tree in C? [...] This is not going to work well if you use them in C code. As pretty much all the values within this header.These flags are not used in C code, only used in assembly code. [...] +#define PT_PT (L0_TABLE) +#define PT_MEM (BLOCK_DEF_ATTR | L1_BLOCK) + +#ifndef PAGE_SIZEThis looks quite wrong. How can it be possible to define multiple PAGE_SIZE? What if they are not the same size?okay, I will remove it. I would highly recommend you to find out why the #ifdef was added here. Maybe there are other headers defining PAGE_SIZE. And therefore you should either remove this one or remove the others. Cheers, -- Julien Grall _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |