Re: [PATCH v2 03/14] tests/tcg: make aarch64 boot.S handle different starting modes

["Alex Bennée" <alex.bennee@linaro.org>]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/05/06 21:57, Alex Bennée wrote:
>> Currently the boot.S code assumes everything starts at EL1. This will
>> break things like the memory test which will barf on unaligned memory
>> access when run at a higher level.
>> Adapt the boot code to do some basic verification of the starting
>> mode
>> and the minimal configuration to move to the lower exception levels.
>> With this we can run the memory test with:
>>    -M virt,secure=on
>>    -M virt,secure=on,virtualization=on
>>    -M virt,virtualisation=on
>> If a test needs to be at a particular EL it can use the semihosting
>> command line to indicate the level we should execute in.
>> Cc: Julian Armistead <julian.armistead@linaro.org>
>> Cc: Jim MacArthur <jim.macarthur@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
<snip>
>> +
>> +	/* sanity check, clamp to 1 if invalid */
>> +	cmp w11, #'0'
>> +	b.lt 1f
>> +	cmp w11, #'4'
>> +	b.ge 1f
>> +	sub	w11, w11, #'0'
>> +	b 2f
>> +1:	mov w11, #1
>> +
>> +2:
>
> This "sanity check" made me wonder what it is for. This code is simply
> unnecessary if any unknown values are to be ignored and is a bit
> misleading to have this code as it looks like it adds an additional
> behavior. "sub w11, w11, #'0'" is also unnecessary; we can compare w11
> with '2' and '3' later instead.
>
> The patch message says this sanity check was added with v2 so I looked
> for a previous review and found this:
> https://lore.kernel.org/qemu-devel/svmkvs.2mf5q4qhsfz83@linaro.org/
>
>> cmp w11, #'0'
>> b.lt curr_sp0_sync
>> cmp w11, #'4'
>> b.ge curr_sp0_sync
>
> Now I get the original intent; it was to raise an error instead of
> simply ignoring unknown values. However I also see a few problems with
> this original code:
> - It still ignores unknown strings that are longer than one character.

I'm not intending to handle longer strings in assembly - I think we can
live with accepting "1junk" to "3foo".

> - curr_sp0_sync leads to the code that writes a message saying
>   "Terminated by exception.", which is incorrect.
> - "cmp w11, #'0'" is unnecessary if you check the value after "sub
>   w11, w11, #'0'".

I'll clean that up.

>
>> +	/* Determine current Exception Level */
>> +	mrs	x0, CurrentEL
>> +	lsr	x0, x0, #2	  /* CurrentEL[3:2] contains the current EL */
>> +
>> +	/* Branch based on current EL */
>> +	cmp	x0, #3
>> +	b.eq	setup_el3
>> +	cmp	x0, #2
>> +	b.eq	setup_el2
>> +	cmp	x0, #1
>> +	b.eq	at_testel	     /* Already at EL1, skip transition */
>> +	/* Should not be at EL0 - error out */
>> +	b	curr_sp0_sync
>> +
>> +setup_el3:
>> +	/* Ensure we trap if we get anything wrong */
>> +	adr	x0, vector_table
>> +	msr	vbar_el3, x0
>> +
>> +	/* Does the test want to be at EL3? */
>> +	cmp	w11, #3
>> +	beq	at_testel
>> +
>> +	/* Configure EL3 to for lower states (EL2 or EL1) */
>> +	mrs	x0, scr_el3
>> +	orr	x0, x0, #(1 << 10)    /* RW = 1: EL2/EL1 execution state is AArch64 */
>> +	orr	x0, x0, #(1 << 0)     /* NS = 1: Non-secure state */
>> +	msr	scr_el3, x0
>> +
>> +	/*
>> +	 * We need to check if EL2 is actually enabled via ID_AA64PFR0_EL1,
>> +	 * otherwise we should just jump straight to EL1.
>> +	 */
>> +	mrs	x0, id_aa64pfr0_el1
>> +	ubfx	x0, x0, #8, #4	      /* Extract EL2 field (bits 11:8) */
>> +	cbz	x0, el2_not_present   /* If field is 0 no EL2 */
>> +
>> +
>> +	/* Prepare SPSR for exception return to EL2 */
>> +	mov	x0, #0x3c9	      /* DAIF bits and EL2h mode (9) */
>> +	msr	spsr_el3, x0
>> +
>> +	/* Set EL2 entry point */
>> +	adr	x0, setup_el2
>> +	msr	elr_el3, x0
>> +
>> +	/* Return to EL2 */
>> +	eret
>> +	nop
>
> Why is a nop placed here?

Left over debug. I'll drop it.
>
<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro