Re: [Qemu-arm] [Qemu-devel] [PATCH v5 03/15] tests/tcg/aarch64: add system boot.S

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 4/30/19 9:52 AM, Alex Bennée wrote:
>> +.error:
>> +    .string "Terminated by exception.\n"
>
> Put it in .rodata just because we can?
>
>> +        /* Page table setup (identity mapping).  */
>> +    adrp    x0, ttb
>> +    add     x0, x0, :lo12:ttb
>
> You are in control of the layout of the executable,
> and adr has a 1MB range.  Why use adrp+add?

Well I have to now as I've aligned .data with:

    /* align r/w section to next 2mb */
    . = ALIGN(1 << 21);

>
>> +        /* Create some (big) pages */
>> +    adr     x1, .                           /* phys address */
>> +    bic     x1, x1, #(1 << 30) - 1          /* 1GB block alignment */
>
> Do you really want 1GB pages?  You'll pretty much only be able to test valid
> memory operations with that.  Which is also true until there's something other
> than an exit for the exception vector... but ya know what I mean.

Not using it for testing but I'm trying to set-up a 2 stage translation
so we get:

  1gb->1gb+2mb = .text/.rodata
  1gb+2mb->1gb+4mb = .data/.bss

>
>> +        /* Setup some stack space and enter the test code.
>> +         * Assume everthing except the return value is garbage when we
>> +     * return, we won't need it.
>> +         */
>> +    adrp    x0, stack
>> +    add     x0, x0, :lo12:stack
>> +        mov      sp, x0
>
> You need a pointer to the end of the stack, not the beginning.
> Again, I think this could be just
>
>       adr     sp, stack_end
>
> Also, there's tab/space confusion all through this file.
> IMO, this is assembly, so it *should* be tabs.

I'm adding an entry to editorconfig and fixing up the damage.

>
>> @@ -0,0 +1,22 @@
>> +ENTRY(__start)
>> +
>> +SECTIONS
>> +{
>> +    /* virt machine, RAM starts at 1gb */
>> +    . = (1 << 30);
>> +    .text : {
>> +        *(.text)
>> +    }
>> +    .data : {
>> +        *(.data)
>> +    }
>> +    .rodata : {
>> +        *(.rodata)
>> +    }
>
> If you ever wanted to make this read-only, swap .rodata before .data, so that
> it's next to .text.

done.

>
>
> r~


--
Alex Bennée