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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2025/06/05 17:51, Alex Bennée wrote:
> 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.
> 

Global symbols share the symbol space with C so the naming convention of 
C can be applied to assembly too.

I also pointed out it ".error" was prefixed with a dot probably due to a 
failed attempt to make it local. There is no point of following a mistake.

I don't see a reason to differentiate the string labels from the others 
either.

Regards,
Akihiko Odaki