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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 24 Oct 2012, Tim Deegan wrote:
> 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.

Thanks for the quick review!


> > @@ -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?

Yep


> > +   
> >     /* 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).

good point


> > +   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.

I'll fix


> >     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.

You have my complete support on code style issues and comments in
assembly code!

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