Re: [Xen-devel] [PATCH 2/7] xen/arm: setup the fixmap in head.S

[Tim Deegan <tim@xxxxxxx>]

At 16:03 +0100 on 24 Oct (1351094622), Stefano Stabellini wrote:
> Setup the fixmap mapping directly in head.S rather than having a
> temporary mapping only to re-do it later in C.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

I'm generally against doing anything in asm that could be in C, as
you know, but this actually looks OK.  Nits below.

> @@ -183,6 +184,16 @@ skip_bss:
>       teq   r12, #0
>       bne   pt_ready
>       
> +     /* console fixmap */
> +     ldr   r1, =xen_fixmap
> +     add   r1, r1, r10            /* r1 := paddr (xen_fixmap) */
> +     mov   r3, #0
> +     lsr   r2, r11, #12
> +     lsl   r2, r2, #12            /* 4K aligned paddr of UART */
> +     orr   r2, r2, #PT_UPPER(DEV_L3)
> +     orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
> +     strd  r2, r3, [r1, #0]       /* Map it in the first fixmap's slot */

Can you use #(FIXMAP_CONSOLE*8) here rather than hard-coding the 0?

> +     
>       /* Build the baseline idle pagetable's first-level entries */
>       ldr   r1, =xen_second
>       add   r1, r1, r10            /* r1 := paddr (xen_second) */
> @@ -205,12 +216,13 @@ skip_bss:
>       ldr   r4, =start
>       lsr   r4, #18                /* Slot for vaddr(start) */
>       strd  r2, r3, [r1, r4]       /* Map Xen there too */
> +
>  #ifdef EARLY_UART_ADDRESS
> -     ldr   r3, =(1<<(54-32))      /* NS for device mapping */
> -     lsr   r2, r11, #21
> -     lsl   r2, r2, #21            /* 2MB-aligned paddr of UART */
> -     orr   r2, r2, #PT_UPPER(DEV)
> -     orr   r2, r2, #PT_LOWER(DEV) /* r2:r3 := 2MB dev map including UART */
> +     /* xen_fixmap pagetable */
> +     ldr r2, =xen_fixmap
> +     add r2, r2, r10

Please keep the operands aligned with the code above and below, and maybe
add a comment that r2 := paddr (xen_fixmap).

> +     orr   r2, r2, #PT_UPPER(PT)
> +     orr   r2, r2, #PT_LOWER(PT) /* r2:r3 := paddr (xen_fixmap) */

r2:r3 is a pagetable mapping here, not a paddr.  Also the comment's not
aligned with the ones above and below.

>       add   r4, r4, #8
>       strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
>  #else
> @@ -236,13 +248,9 @@ pt_ready:
>       mov   pc, r1                 /* Get a proper vaddr into PC */
>  paging:
>  
> +     
>  #ifdef EARLY_UART_ADDRESS
> -     /* Recover the UART address in the new address space. */
> -     lsl   r11, #11
> -     lsr   r11, #11               /* UART base's offset from 2MB base */
> -     adr   r0, start
> -     add   r0, r0, #0x200000      /* vaddr of the fixmap's 2MB slot */
> -     add   r11, r11, r0           /* r11 := vaddr (UART base address) */
> +     ldr r11, =FIXMAP_ADDR(FIXMAP_CONSOLE)

Please leave a comment in to say what this is doing.  Also, the
alignment is wrong again.  Sorry to be picky about this but I want to
keep head.S readable.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel