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

[Thomas Leonard <talex5@xxxxxxxxx>]

On 16 July 2014 22:49, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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.

Yes, good idea.

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

Yes. Fixed.

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

Here, the leading "0" is EQ, so it's only executed if IRQ_handler is
unset ("undEQ").

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

OK, will add a note here.


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

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