[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.