|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 5/7] xen: arm: rewrite start of day page table and cpu bring up
Hi,
This looks pretty good - thanks for cleaning it all up! I've reviewed
the asm bits below; I'll look at the C code separately, just to break
things up.
At 02:40 +0100 on 17 Sep (1379385648), Ian Campbell wrote:
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -37,6 +37,25 @@
> #include EARLY_PRINTK_INC
> #endif
>
> +/*
> + * Common register usage in this file:
> + * r0 -
> + * r1 -
> + * r2 -
> + * r3 -
> + * r4 -
> + * r5 -
> + * r6 -
> + * r7 - CPUID
> + * r8 - DTB address (boot CPU only)
> + * r9 - paddr(start)
> + * r10 - phys offset
> + * r11 - UART address
> + * r12 - !!is_boot_cpu
s/!!/!/ throughout, right? In fact, why not call it is_secondary_cpu
for ease of thinking? :)
> +common_start:
> + mov r7, #0 /* r13 := CPU ID */
s/r13/r7/ in comment.
> + mrc CP32(r1, MPIDR)
> + tst r1, #(1<<31) /* Multiprocessor extension supported?
> */
#MPIDR_SMP? likewise #MPIDR_SMP and #(~MPIDR_HW_MASK) below.
OTOH it might be better idea to leave this almost-code-motion
alone and then, once the dust settles, sweep through head.S replacing
magic constants with their named equivalents.
> + beq 1f
> + tst r1, #(1<<30) /* Uniprocessor system? */
> + bne 1f
> + bic r7, r1, #(0xff << 24) /* Mask out flags to get CPU ID */
> +1:
[...]
> /* console fixmap */
> #if defined(EARLY_PRINTK)
> + /* Non-boot CPUs don't need to rebuild the fixmap */
> + teq r12, #0
> + bne 1f
> +
While we're shuffling things around, I think this console fixmap
fettling should be moved to the end of the PT build process for
consisitency. I think it belongs with the fixmap building that happens
after paging is enabled Also s/rebuild/build/ in the comment?
> + /* Write Xen's PT's paddr into the HTTBR */
> + ldr r4, =boot_pgtable
> + add r4, r4, r10 /* r4 := paddr (xen_pagetable) */
> + mov r5, #0 /* r4:r5 is paddr (xen_pagetable) */
s/xen_/boot_/ in comments. There's some more of that later on, too.
> + mcrr CP64(r4, r5, HTTBR)
> +
> + /* Setup boot_pgtable: */
> + ldr r1, =boot_second
> + add r1, r1, r10 /* r1 := paddr (boot_second) */
> mov r3, #0x0
> +
> + /* ... map boot_second in boot_pgtable[0] */
> orr r2, r1, #PT_UPPER(PT) /* r2:r3 := table map of xen_second */
> orr r2, r2, #PT_LOWER(PT) /* (+ rights for linear PT) */
> strd r2, r3, [r4, #0] /* Map it in slot 0 */
> - add r2, r2, #0x1000
> - strd r2, r3, [r4, #8] /* Map 2nd page in slot 1 */
> - add r2, r2, #0x1000
> - strd r2, r3, [r4, #16] /* Map 3rd page in slot 2 */
> - add r2, r2, #0x1000
> - strd r2, r3, [r4, #24] /* Map 4th page in slot 3 */
> -
> - /* Now set up the second-level entries */
> +
> + /* ... map of paddr(start) in boot_pgtable */
> + lsrs r1, r9, #30 /* Offset of base paddr in boot_pgtable
> */
> + beq 1f /* If it is in slot 0 then map in
> xen_second
> + * later on */
> + lsl r2, r1, #30 /* Base address for 1GB mapping */
> + orr r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
> + orr r2, r2, #PT_LOWER(MEM)
> + lsl r1, r1, #3 /* r1 := Slot offset */
> + strd r2, r3, [r4, r1] /* Mapping of paddr(start) */
> +
> +1: /* Setup boot_second: */
> + ldr r4, =boot_second
> + add r4, r4, r10 /* r1 := paddr (xen_second) */
> +
> + lsr r2, r9, #20 /* Base address for 2MB mapping */
> + lsl r2, r2, #20
> + orr r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
> + orr r2, r2, #PT_LOWER(MEM)
> +
> + /* ... map of vaddr(start) in boot_second */
> + ldr r1, =start
> + lsr r1, #18 /* Slot for vaddr(start) */
> + strd r2, r3, [r4, r1] /* Map vaddr(start) */
> +
> + /* ... map of paddr(start) in boot_second */
> orr r2, r9, #PT_UPPER(MEM)
> orr r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB normal map of Xen */
Isn't this the same as the entry you already had in r2:r3 (assuming
we're loaded at a 2MB-aligned address)? You can just store it again in
the other slot.
> - mov r4, r9, lsr #18 /* Slot for paddr(start) */
> - strd r2, r3, [r1, r4] /* Map Xen there */
> - ldr r4, =start
> - lsr r4, #18 /* Slot for vaddr(start) */
> - strd r2, r3, [r1, r4] /* Map Xen there too */
>
> - /* xen_fixmap pagetable */
> - ldr r2, =xen_fixmap
> - add r2, r2, r10 /* r2 := paddr (xen_fixmap) */
> - orr r2, r2, #PT_UPPER(PT)
> - orr r2, r2, #PT_LOWER(PT) /* r2:r3 := table map of xen_fixmap */
> - add r4, r4, #8
> - strd r2, r3, [r1, r4] /* Map it in the fixmap's slot */
> + lsrs r1, r9, #30 /* Base paddr */
> + bne 1f /* If paddr(start) is not in slot 0
> + * then the mapping was done in
> + * boot_pgtable above */
>
> - mov r3, #0x0
> - lsr r2, r8, #21
> - lsl r2, r2, #21 /* 2MB-aligned paddr of DTB */
> - orr r2, r2, #PT_UPPER(MEM)
> - orr r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
> - add r4, r4, #8
> - strd r2, r3, [r1, r4] /* Map it in the early boot slot */
> + mov r1, r9, lsr #18 /* Slot for paddr(start) */
> + strd r2, r3, [r4, r1] /* Map Xen there */
> +
> + /* Defer fixmap and dtb mapping until after paging enabled, to
> + * avoid them clashing with the 1:1 mapping.
> + */
Block comments elsewhere in this file don't have the extra '/*' and '*/'
lines. I guess it's Xen style to add them, but please either both or
neither. :)
> +
> +1: /* boot pagetable setup complete */
This label belongs above the 'Defer fixmap' comment, or else it needs a
name.
> -pt_ready:
> PRINT("- Turning on paging -\r\n")
>
> ldr r1, =paging /* Explicit vaddr, not RIP-relative */
> @@ -315,22 +332,47 @@ pt_ready:
> mov pc, r1 /* Get a proper vaddr into PC */
> paging:
>
> -
> -#ifdef EARLY_PRINTK
> + /* Now we can install the fixmap and dtb mappings, since we
> + * don't need the 1:1 map any more */
> + dsb sy
> + ldr r1, =boot_second
> +#if defined(EARLY_PRINTK)
> + /* xen_fixmap pagetable */
> + ldr r2, =xen_fixmap
> + add r2, r2, r10 /* r2 := paddr (xen_fixmap) */
> + orr r2, r2, #PT_UPPER(PT)
> + orr r2, r2, #PT_LOWER(PT) /* r2:r3 := table map of xen_fixmap */
> + ldr r4, =FIXMAP_ADDR(0)
> + mov r4, r4, lsr #18 /* r4 := Slot for FIXMAP(0) */
> + strd r2, r3, [r1, r4] /* Map it in the fixmap's slot */
So, I think this is where the code to populate FIXMAP_CONSOLE ought to end up.
> /* Use a virtual address to access the UART. */
> ldr r11, =FIXMAP_ADDR(FIXMAP_CONSOLE)
> #endif
> + /* Map the DTB in the boot misc slot */
> + teq r12, #0 /* Only on boot CPU */
> + bne 1f
> +
> + mov r3, #0x0
> + lsr r2, r8, #21
> + lsl r2, r2, #21 /* r2: 2MB-aligned paddr of DTB */
> + orr r2, r2, #PT_UPPER(MEM)
> + orr r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
> + ldr r4, =BOOT_MISC_VIRT_START
> + mov r4, r4, lsr #18 /* Slot for BOOT_MISC_VIRT_START */
> + strd r2, r3, [r1, r4] /* Map it in the early boot slot */
> + dsb sy
Do we need any TLB flushes here, since we're modifying the live
pagetables? I don't recall whether the ARM MMU is guaranteed not to
cache/prefetch negative results (like the x86 one is).
> - PRINT("- Ready -\r\n")
> +1: PRINT("- Ready -\r\n")
Again, I'd prefer the label to go above (logically connected to the end
of the block being skipped) to avoid the risk of someone adding new code
just before '- Ready -' and having it be accidentally skipped.
> /* The boot CPU should go straight into C now */
> teq r12, #0
> beq launch
>
> - /* Non-boot CPUs need to move on to the relocated pagetables */
> - mov r0, #0
> - ldr r4, =boot_ttbr /* VA of HTTBR value stashed by CPU 0 */
> - add r4, r4, r10 /* PA of it */
> + /* Non-boot CPUs need to move on to proper pagetables,
> + * temporarily use cpu0's table and switch to our own in
> + * mmu_init_secondary_cpu.
> + */
Another mixed-style block comment.
[...]
> +ENTRY(relocate_xen)
> + push {r4,r5,r6,r7,r8,r9,r10,r11}
> +
> + ldr r4, [sp, #8*4] /* Get 4th argument from
> stack */
Long line -- that comment can be reeled in a bit.
> +
> + /* Copy 16 bytes at a time using:
> + * r5: counter
> + * r6: data
> + * r7: data
> + * r8: data
> + * r9: data
> + * r10: source
> + * r11: destination
> + */
> + mov r5, r4
> + mov r10, r2
> + mov r11, r3
> +1: ldmia r10!, {r6, r7, r8, r9}
> + stmia r11!, {r6, r7, r8, r9}
> +
> + subs r5, r5, #16
Misalignment ^^^
> + bgt 1b
> +
> + /* Flush destination from dcache using:
> + * r5: counter
> + * r6: step
> + * r7: vaddr
> + */
> + dsb sy /* So the CPU issues all writes to the range */
> +
> + mov r5, r4
> + ldr r6, =cacheline_bytes /* r6 := step */
> + ldr r6, [r6]
> + mov r7, r3
> +
> +1: mcr CP32(r7, DCCMVAC)
> +
> + add r7, r7, r6
> + subs r5, r5, r6
> + bgt 1b
> +
> + dsb sy /* So we know the flushes happen before continuing */
> +
> + isb /* Ensure synchronization with previous changes to
> text */
> + mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */
> + mcr CP32(r0, ICIALLU) /* Flush I-cache */
> + mcr CP32(r0, BPIALL) /* Flush branch predictor */
> + dsb /* Ensure completion of TLB+BP flush
> */
Can you be consistent about 'dsb sy' vs 'dsb'? There were no 'dsb sy's
in the file to start with, so I'd be inclined to stick to plain 'dsb'
throughout (and maybe follow up adding 'sy' throughout if you like).
> + isb
> +
> + mcrr CP64(r0, r1, HTTBR)
> +
> + dsb sy /* ensure memory accesses do not cross over the TTBR0 write
> */
> +
> + isb /* Ensure synchronization with previous changes to
> text */
Weird alignment of comments here. I know it seems like bikeshedding,
but I really do find that keeping asm code tidy makes it easier to
understand. :)
> + mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */
> + mcr CP32(r0, ICIALLU) /* Flush I-cache */
> + mcr CP32(r0, BPIALL) /* Flush branch predictor */
> + dsb /* Ensure completion of TLB+BP flush */
> + isb
> +
> + pop {r4, r5,r6,r7,r8,r9,r10,r11}
> +
> + mov pc, lr
[...]
> --- a/xen/arch/arm/arm32/mode_switch.S
> +++ /dev/null
Yay!
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 21b7e4d..6406562 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
A lot of the things I commented on in the 32-bit version apply here too;
I won't repeat them here.
[...]
> - /* Are we the boot CPU? */
> - mov x22, #0 /* x22 := CPU ID */
> + mov x22, #0 /* x22 := !!is_boot_cpu */
> +
> + b common_start
> +
> +GLOBAL(init_secondary)
> + msr DAIFSet, 0xf /* Disable all interrupts */
> +
> + /* Find out where we are */
> + ldr x0, =start
> + adr x19, start /* x19 := paddr (start) */
> + sub x20, x19, x0 /* x20 := phys-offset */
> +
> + mov x22, #1 /* x22 := !!is_boot_cpu */
> +
> +common_start:
> + mov x24, #0 /* x24 := CPU ID */
No it's not. :) I guess it's already like that, but this seems like it
deserves a better comment.
[...]
> @@ -130,30 +165,18 @@ boot_cpu:
> bl putn
> PRINT(" -\r\n")
>
> - /* Are we in EL3 */
> - mrs x0, CurrentEL
The PRINT() above clobbers x0, so we need to either keep that re-read of
CurrentEL, or stash the value somewhere else while we print it.
[...]
> +1: /* Setup boot_second: */
> + ldr x4, =boot_second
> + add x4, x4, x20 /* x4 := paddr (boot_second) */
> +
> + lsr x2, x19, #20 /* Base address for 2MB mapping */
> + lsl x2, x2, #20
> + mov x3, #PT_MEM /* x2 := Section map */
> + orr x2, x2, x3
> +
> + /* ... map of vaddr(start) in boot_second */
> + ldr x1, =start
> + lsr x1, x1, #18 /* Slot for vaddr(start) */
> + str x2, [x4, x1] /* Map vaddr(start) */
> +
> + /* ... map of paddr(start) in boot_second */
> + mov x3, #PT_PT /* x2 := table map of boot_second */
> + orr x2, x1, x3 /* + rights for linear PT */
Wait, what? Don't you want to reuse the 2MB map of PA(start) that you
already have in x2? And x1 here is constant 0x8, not the address of
anything.
> +
> + lsr x1, x19, #30 /* Base paddr */
> + cbnz x1, 1f /* If paddr(start) is not in slot 0
> + * then the mapping was done in
> + * boot_pgtable or boot_first above */
> +
> + lsr x1, x19, #18 /* Slot for paddr(start) */
> + str x2, [x4, x1] /* Map Xen there */
[...]
> - /* Non-boot CPUs need to move on to the relocated pagetables */
> - ldr x4, =boot_ttbr /* VA of TTBR0_EL2 stashed by CPU 0 */
> - add x4, x4, x20 /* PA of it */
> + /* Non-boot CPUs need to move on to the relocated pagetables,
> + temporarily use cpu0's table and switch */
Missing * ^^^
[...]
> --- a/xen/arch/arm/arm64/mode_switch.S
> +++ /dev/null
Also yay!
Tim
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |