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

[Richard Henderson <richard.henderson@linaro.org>]

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?

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

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

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


r~