|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [PATCH v3 06/43] arm64: add the boot code
Hi, On 16/04/18 07:31, Huang Shijie wrote: I see you use the PRINT(...) before enabling the MMU. Per include/xen/arch-arm.h: "All memory which is shared with other entities in the system (including the hypervisor and other guests) must reside in memory which is mapped as Normal Inner Write-Back Outer Write-Back Inner-Shareable. This applies to: - hypercall arguments passed via a pointer to guest memory. - memory shared via the grant table mechanism (including PV I/O rings etc). - memory shared with the hypervisor (struct shared_info, struct vcpu_info, the grant table, etc)."This means that your PRINT may not always appear and provoking other funny problem as the cache may have an inconsistent state. So I think you want to avoid using those print before MMU is enabled. +97: .asciz _s; \ Don't you want to put the string into the text section? +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. + + .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. +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? For reminder, the documentation of that flag is: 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 */ + +/* + * Primary CPU general-purpose register settings This is fairly unclear to me what you mean by primary. Did you mean, "Initial state of the GPR when _start is called?" I am pretty sure you need to clean the cache for the page-table region here to avoid potential dirty lines being evicted and ... + /* Setup the initial page table. */ + bl _setup_initial_pgtable + mov x19, x0 + + /* Setup the identity mapping */ + bl _setup_idmap_pgtable Also here because you populated the page-tables with non-cacheable accesses (MMU disabled), so you want to remove speculatively loaded cache lines. + + /* Load TTBRx */ + msr ttbr1_el1, x19 + msr ttbr0_el1, x0 + isb + + /* Turning on MMU */ + tlbi vmalle1 Can you please document why the TLB here. In that case it remove stall TLB entries. + dsb nsh + isb The isb here is not necessary. You can rely on the one after the MMU is enabled below. + ldr x1, =(SCTLR_M | SCTLR_C | SCTLR_I) If you fully overwrite SCTLR_EL1 value, then you need to make sure all RES1 are set to 1 in ARMv8.0. Otherwise, you are going to have some surprise when those bits are defined differently (such as moving to ARMv8.2). You probably want to flush the TLB here. So you don't end up to use stall data. Again, nothing in this header look arm64 specific. @@ -0,0 +1,18 @@ +#ifndef __ASM_H__ +#define __ASM_H__ + +#define ALIGN .align 4 + +#define ENTRY(name) \ + .globl name; \ + ALIGN; \ + name: + +#define END(name) \ + .size name, .-name + +#define ENDPROC(name) \ + .type name, @function; \ + END(name) + +#endif /* __ASM_H__ */ diff --git a/include/arm/arm64/pagetable.h b/include/arm/arm64/pagetable.h new file mode 100644 index 0000000..8e2384f --- /dev/null +++ b/include/arm/arm64/pagetable.h @@ -0,0 +1,108 @@ +#ifndef __ARM64_PAGE_TABLE__ + +#define __ARM64_PAGE_TABLE__ + +/* TCR flags */ +#define TCR_TxSZ(x) ((((64) - (x)) << 16) | (((64) - (x)) << 0)) +#define TCR_IRGN_WBWA (((1) << 8) | ((1) << 24)) +#define TCR_ORGN_WBWA (((1) << 10) | ((1) << 26)) +#define TCR_SHARED (((3) << 12) | ((3) << 28)) +#define TCR_ASID16 ((1) << 36) +#define TCR_IPS_48BIT ((5) << 32) +#define TCR_TG1_4K ((2) << 30) Those flags are only going to work well in assembly. In C, you will have truncation problem as the shift is based on integer (aka 32-bit). Also can you remove parenthese around all plain values. I.e s/(2)/2/. This will make the code more readable. +#define TCR_EPD0 (1 << 7) + +/* Max virtual address */ +#define VM_MAX_ADDRESS (0xffffffffffffffff) + +/* Number of virtual address bits */ +#define VA_BITS 48 + +/* + * Memory types available. + */ +#define MEM_DEVICE_nGnRnE 0 +#define MEM_DEVICE_nGnRE 1 +#define MEM_DEVICE_GRE 2 +#define MEM_NORMAL_NC 3 +#define MEM_NORMAL 4 + +#define SET_MAIR(attr, mt) ((attr) << ((mt) * 8)) + +/* SCTLR_EL1 - System Control Register */ +#define SCTLR_M (1 << 0) +#define SCTLR_C (1 << 2) +#define SCTLR_I (1 << 12) + +/* Level 0 table, 512GiB per entry */ +#define L0_SHIFT 39 +#define L0_SIZE (1UL << L0_SHIFT) +#define L0_INVAL 0x0 /* An invalid address */ +#define L0_TABLE 0x3 /* A next-level table */ + +/* Level 1 table, 1GiB per entry */ +#define L1_SHIFT 30 +#define L1_SIZE (1 << L1_SHIFT) Please try to be consistent to use 1UL everywhere for *_SIZE. +#define L1_OFFSET (L1_SIZE - 1) +#define L1_INVAL L0_INVAL +#define L1_BLOCK 0x1 +#define L1_TABLE L0_TABLE +#define L1_MASK (~(L1_SIZE-1)) + +/* Level 2 table, 2MiB per entry */ +#define L2_SHIFT 21 +#define L2_SIZE (1 << L2_SHIFT) +#define L2_OFFSET (L2_SIZE - 1) +#define L2_INVAL L0_INVAL +#define L2_BLOCK L1_BLOCK +#define L2_TABLE L0_TABLE +#define L2_MASK (~(L2_SIZE-1)) + +/* Level 3 table, 4KiB per entry */ +#define L3_SHIFT 12 +#define L3_SIZE (1 << L3_SHIFT) +#define L3_OFFSET (L3_SIZE - 1) +#define L3_INVAL 0x0 +#define L3_PAGE 0x3 +#define L3_MASK (~(L3_SIZE-1)) + +#define Ln_ENTRIES (1 << 9) +#define Ln_ADDR_MASK (Ln_ENTRIES - 1) + +#define ATTR_MASK_L 0xfff + +#define l0_pgt_idx(va) (((va) >> L0_SHIFT) & Ln_ADDR_MASK) +#define l1_pgt_idx(va) (((va) >> L1_SHIFT) & Ln_ADDR_MASK) +#define l2_pgt_idx(va) (((va) >> L2_SHIFT) & Ln_ADDR_MASK) +#define l3_pgt_idx(va) (((va) >> L3_SHIFT) & Ln_ADDR_MASK) + +/* + * Lower attributes fields in Stage 1 VMSAv8-A Block and Page descriptor + */ +#define ATTR_nG (1 << 11) +#define ATTR_AF (1 << 10) +#define ATTR_SH(x) ((x) << 8) +#define ATTR_SH_MASK ATTR_SH(3) +#define ATTR_SH_NS 0 /* Non-shareable */ +#define ATTR_SH_OS 2 /* Outer-shareable */ +#define ATTR_SH_IS 3 /* Inner-shareable */ +#define ATTR_AP_RW_BIT (1 << 7) +#define ATTR_AP(x) ((x) << 6) +#define ATTR_AP_MASK ATTR_AP(3) +#define ATTR_AP_RW (0 << 1) +#define ATTR_AP_RO (1 << 1) +#define ATTR_AP_USER (1 << 0) +#define ATTR_NS (1 << 5) +#define ATTR_IDX(x) ((x) << 2) +#define ATTR_IDX_MASK (7 << 2) + +#define MEM_DEF_ATTR (ATTR_AF|ATTR_SH(ATTR_SH_IS)|ATTR_IDX(MEM_NORMAL)) +#define MEM_NC_ATTR (ATTR_AF|ATTR_SH(ATTR_SH_IS)|ATTR_IDX(MEM_NORMAL_NC)) +#define MEM_DEV_ATTR (ATTR_AF|ATTR_SH(ATTR_SH_IS)|ATTR_IDX(MEM_DEVICE_nGnRnE)) + +#define MEM_RO_ATTR (MEM_DEF_ATTR|ATTR_AP(ATTR_AP_RO)) + +#define PT_PT (L0_TABLE) +#define PT_MEM (MEM_DEF_ATTR | L1_BLOCK) The naming for those 2 macros don't make sense based on the value. For instance, PT_PT gives the impression to be attribute for table and does not necessarily need to be level-0. But I just notice that you define L1_TABLE as L0_TABLE. This is so confusing, you probably want to do some renaming some where. Same for L1_BLOCK. Cheers, -- Julien Grall _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |