Re: [PATCH v4 03/17] 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/06/03 20:01, 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>
>> ---
>> v4
>>    - drop post eret nops
>>    - proper error string for EL0 error case
>>    - clamp any invalid target EL value to 1
>> v3
>>    - create system stack so we _exit cleanly
>>    - normalise EL string before compares
>>    - catch when we start in a lower EL than we asked for
>>    - default to EL1 when arg unclear
>> v2
>>    - allow tests to control the final EL we end up at
>>    - use tabs consistently
>>    - validate command line arg is between 1 and 3
>> ---
>>   tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
>>   tests/tcg/aarch64/system/boot.S           | 172 +++++++++++++++++++++-
>>   2 files changed, 169 insertions(+), 6 deletions(-)
>> diff --git a/tests/tcg/aarch64/Makefile.softmmu-target
>> b/tests/tcg/aarch64/Makefile.softmmu-target
>> index 9c52475b7a..f7a7d2b800 100644
>> --- a/tests/tcg/aarch64/Makefile.softmmu-target
>> +++ b/tests/tcg/aarch64/Makefile.softmmu-target
>> @@ -68,7 +68,8 @@ run-plugin-semiconsole-with-%: semiconsole
>>     # vtimer test needs EL2
>>   QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
>> -run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_BASE_ARGS) -kernel
>> +QEMU_EL2_BASE_ARGS=-semihosting-config enable=on,target=native,chardev=output,arg="2"
>> +run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_EL2_BASE_ARGS) -kernel
>>     # Simple Record/Replay Test
>>   .PHONY: memory-record
>> diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
>> index a5df9c173d..8bfa4e4efc 100644
>> --- a/tests/tcg/aarch64/system/boot.S
>> +++ b/tests/tcg/aarch64/system/boot.S
>> @@ -16,6 +16,7 @@
>>   #define semihosting_call hlt 0xf000
>>   #define SYS_WRITEC	0x03	/* character to debug channel */
>>   #define SYS_WRITE0	0x04	/* string to debug channel */
>> +#define SYS_GET_CMDLINE 0x15	/* get command line */
>>   #define SYS_EXIT	0x18
>>     	.align	12
>> @@ -70,21 +71,172 @@ lower_a32_sync:
>>   lower_a32_irq:
>>   lower_a32_fiq:
>>   lower_a32_serror:
>> +	adr	x1, .unexp_excp
>> +exit_msg:
>>   	mov	x0, SYS_WRITE0
>> -	adr	x1, .error
>>   	semihosting_call
>>   	mov	x0, 1 /* EXIT_FAILURE */
>>   	bl 	_exit
>>   	/* never returns */
>>     	.section .rodata
>> -.error:
>> -	.string "Terminated by exception.\n"
>> +.unexp_excp:
>> +	.string "Unexpected exception.\n"
>> +.high_el_msg:
>> +	.string "Started in lower EL than requested.\n"
>> +.unexp_el0:
>> +	.string "Started in invalid EL.\n"
>> +
>> +	.align 8
>> +.get_cmd:
>
> Please do not send a new version without addressing all comments for
> the previous versions or at least noting there are unaddressed
> comments:
> https://lore.kernel.org/qemu-devel/7a76e746-9022-48cf-8216-775071e6d631@daynix.com
>
> Following the best practices in docs/devel/submitting-a-patch.rst will
> ensure a smoother patch review. It is fine for me if you submit a new
> version noting unaddressed comments, but some may disagree.

There is no style guide for assembler. I have made the strings
consistently use the . prefix.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro