[PATCH 1/6] cmd: exception: arm64: fix undefined, add faults

[Heinrich Schuchardt]

On 1/10/22 00:19, Andre Przywara wrote:
> On Sun, 9 Jan 2022 23:47:32 +0100 (CET)
> Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> 
> Hi Mark,
> 
> (I knew I forgot to CC: one person ...)
> 
>>> Date: Sun, 9 Jan 2022 22:35:27 +0000
>>> From: Andre Przywara <andre.przywara at arm.com>
>>>
>>> Hi Heinrich,
>>>    
>>>> On 1/9/22 22:31, Andre Przywara wrote:
>>>>> On Sun, 9 Jan 2022 20:08:41 +0100
>>>>> Heinrich Schuchardt <heinrich.schuchardt at canonical.com> wrote:
>>>>>      
>>>>>> On 1/9/22 18:30, Andre Przywara wrote:
>>>>>>> The arm64 version of the exception command was just defining the
>>>>>>> undefined exception, but actually copied the AArch32 instruction.
>>>>>>>
>>>>>>> Replace that with an encoding that is guaranteed to be and stay
>>>>>>> undefined. Also add instructions to trigger unaligned access faults and
>>>>>>> a breakpoint.
>>>>>>> This brings ARM64 on par with ARM(32) for the exception command.
>>>>>>>
>>>>>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>>>>>>> ---
>>>>>>>     cmd/arm/exception64.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>>>>>>     1 file changed, 38 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/cmd/arm/exception64.c b/cmd/arm/exception64.c
>>>>>>> index d5de50a0803..1a9730e6aec 100644
>>>>>>> --- a/cmd/arm/exception64.c
>>>>>>> +++ b/cmd/arm/exception64.c
>>>>>>> @@ -12,14 +12,46 @@ static int do_undefined(struct cmd_tbl *cmdtp, int flag, int argc,
>>>>>>>     			char *const argv[])
>>>>>>>     {
>>>>>>>     	/*
>>>>>>> -	 * 0xe7f...f.	is undefined in ARM mode
>>>>>>> -	 * 0xde..	is undefined in Thumb mode
>>>>>>> +	 * Instructions starting with the upper 16 bits all 0 are permanently
>>>>>>> +	 * undefined. The lower 16 bits can be used for some kind of immediate.
>>>>>>> +	 * --- ARMv8 ARM (ARM DDI 0487G.a C6.2.339: "UDF")
>>>>>>>     	 */
>>>>>>> -	asm volatile (".word 0xe7f7defb\n");
>>>>>>> +	asm volatile (".word 0x00001234\n");
>>>>>>> +
>>>>>>> +	return CMD_RET_FAILURE;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int do_unaligned(struct cmd_tbl *cmdtp, int flag, int argc,
>>>>>>> +			char *const argv[])
>>>>>>> +{
>>>>>>> +	/*
>>>>>>> +	 * The load acquire instruction requires the data source to be
>>>>>>> +	 * naturally aligned, and will fault even if strict alignment fault
>>>>>>> +	 * checking is disabled.
>>>>>>> +	 * --- ARMv8 ARM (ARM DDI 0487G.a B2.5.2: "Alignment of data accesses")
>>>>>>
>>>>>> According to DI0487G_b_armv8_arm.pdf available at
>>>>>> https://developer.arm.com/documentation/ddi0487/latest the generation of
>>>>>> an alignment fault for ldar depends on FEAT_LSE2 (Large System
>>>>>> Extensions v2) which is mandatory for ARMv8.4. See p. B2-161.
>>>>>
>>>>> Well found, but I wonder if that matters for the SoCs running U-Boot.
>>>>> It looks like the Apple M1 is the only one so far and will probably
>>>>> stay for a while.
>>>>
>>>> Developers are using U-Boot on Apple M1 already.
>>>
>>> Yeah, sorry, I didn't realise that the M1 is fully v8.4 compliant. Most
>>> other SoCs I checked are v8.2/v8.3 + some 8.4 features, at most. The
>>> Cortex-A76/Neoverse-N1 for instance does not have LSE2.
>>>    
>>>>> But I can of course check ID_AA64MMFR2_EL1.AT before executing the LDAR,
>>>>> and will ask around for a better method to provoke unaligned accesses.
>>>>
>>>> It is sufficient if you update the comment for this function. Returning
>>>> CMD_RET_FAILURE as return value if unaligned access is supported is
>>>> fine. Cf. cmd/riscv/exception.c. (On RISC-V OpenSBI emulates unaligned
>>>> access.)
>>>
>>> Well, I now have the check and a message, always returning FAILURE in
>>> this case. Let me see if people in the office have a better idea...
>>>    
>>>> Maybe we should also add a comment in doc/usage/exception.rst.
>>>
>>> By the way: this was triggered by my need to check SError generation. I
>>> don't know of a nice architectural way to trigger an SError (yet), but
>>> some SoCs happily generate one by accessing unimplemented memory regions
>>> (beyond DRAM, for instance). So I could trigger it on my Juno board
>>> with a specific address, but not on an Allwinner board so far.
>>> Do you think it's worthwhile to have a platform specific address in
>>> Kconfig to implement the serror exception sub-command?


https://developer.arm.com/documentation/102412/0100/Exception-types
explains that an Serror will not occur if the MMU already detects that 
the memory address is invalid.

Maybe the SError location could be put into *u-boot.dtsi? Then you would 
not have to maintain it on board level.

My reason to implement the exception command was my work on the 
exception output. Serror is an asynchronous interrupt. So couldn't the 
output show some arbitrary instruction and not the instruction causing 
the problem?

>>
>> Well, on the M1 writing to the serial port output register with the
>> wrong width (8 bits instead of 32 bits) triggered an SError while
>> still producing output.  But once the OS booted, it did panic with an
>> SError.  Which indeed caused some head scratching like you described.
>>
>> Do you want me to test this series on the M1?
> 
> Oh yes, please, that would be much appreciated! Just booting would be a
> good test already, but bonus points for enabling CMD_EXCEPTION and
> testing all subcommands.
> 
> Cheers,
> Andre
> 
> P.S.: I am just about to finish up my large bitmap font patch, I
> think the M1 laptops would be a grateful customer for this ...

Why not use TrueType? I have patch for adding a usable monospaced font:
https://patchwork.ozlabs.org/project/uboot/list/?series=231521
[v2,1/1] video: add DejaVu Mono font

Best regards

Heinrich