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

[Julien Grall <julien.grall@linaro.org>]

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