Re: [Xen-devel] [PATCH ARM v6 07/14] mini-os: arm: boot code

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Thomas,

On 16/07/14 12:07, Thomas Leonard wrote:
> +
> +       @ Tell the system where our page table is located.
> +       @ This is the 16 KB top-level translation table, in which
> +       @ each word maps one 1MB virtual section to a physical section.
> +       mcr     p15, 0, r1, c2, c0, 0   @ set ttbr0 = r1
I don't see any modification of TTBCR (so the initial value is 0). I 
guess you are using short-descriptor translation table format, right? If 
so, I would write somewhere.
> +       @ Set access permission for domains.
> +       @ Domains are deprecated, but we have to configure them anyway.
> +       @ We mark every page as being domain 0 and set domain 0 to "client mode"
> +       @ (client mode = use access flags in page table).
> +       mov     r0, #1                  @ 1 = client
> +       mcr     p15, 0, r0, c3, c0, 0   @ DACR
> +
> +       @ Template (flags) for a 1 MB page-table entry.
> +       @ TEX[2:0] C B = 001 1 1 (outer and inner write-back, write-allocate)
> +       ldr     r8, =(0x2 +             /* Section entry */ \
> +                     0xc +             /* C B */ \
> +                     (3 << 10) +         /* Read/write */ \
> +                     (1 << 12) + /* TEX */ \
> +                     (1 << 16) + /* Sharable */ \
> +                     (1<<19))            /* Non-secure */
> +       @ r8 = template page table entry
> +
> +       @ Add an entry for the current physical section, at the old and new
> +       @ addresses. It's OK if they're the same.
> +       mov     r0, pc, lsr#20
> +       mov     r0, r0, lsl#20          @ r0 = physical address of this code's 
> section start
> +       orr     r3, r0, r8              @ r3 = table entry for this section
> +       ldr     r4, =_start             @ r4 = desired virtual address of this 
> section
> +       str     r3, [r1, r4, lsr#18]    @ map desired virtual section to this 
> code
> +       str     r3, [r1, r0, lsr#18]    @ map current section to this code too
> +
> +       @ Invalidate TLB
> +       mcr     p15, 0, r1, c8, c7, 0   @ TLBIALL
> +
> +       @ Enable MMU / SCTLR
> +       @ We leave caches off for now because we're going to be changing the
> +       @ TLB a lot and this avoids having to flush the caches each time.
Did you intend to mean page table rather than TLB?

[..]

> +irq_handler:
> +       ldr     sp, =irqstack_end
> +       push    {r0 - r12, r14}
> +
> +       ldr     r0, IRQ_handler
> +       cmp     r0, #0
> +       .word   0x07f000f0      @ undeq - panic if no handler
Hrmmm, I didn't spot this earlier. How can this work? You 
unconditionally called an undefined encoding.
[..]

> diff --git a/extras/mini-os/arch/arm/minios-arm32.lds 
> b/extras/mini-os/arch/arm/minios-arm32.lds
> new file mode 100755
> index 0000000..21c3893
> --- /dev/null
> +++ b/extras/mini-os/arch/arm/minios-arm32.lds
> @@ -0,0 +1,79 @@
> +OUTPUT_ARCH(arm)
> +ENTRY(_start)
> +SECTIONS
> +{
> +  _boot_stack   = 0x400000;    /* 16 KB boot stack */
> +  _boot_stack_end = 0x404000;
> +  _page_dir      = 0x404000;   /* 16 KB translation table */
It might be worse to explain that you are relying on there is a hole 
before the kernel address.
Regards,

--
Julien Grall

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