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

[Andre Przywara <andre.przywara@arm.com>]

On Sun, 9 Jan 2022 23:19:20 +0100
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:

Hi Heinrich,

> On 1/9/22 22:31, Andre Przywara wrote:
> > On Sun, 9 Jan 2022 20:08:41 +0100
> > Heinrich Schuchardt <heinrich.schuchardt@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@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?

Cheers,
Andre


> 
> Best regards
> 
> Heinrich
> 
> > 
> > Cheers,
> > Andre
> > 
> >   
> >>
> >> Best regards
> >>
> >> Heinrich
> >>  
> >>> +	 */
> >>> +	asm volatile (
> >>> +		"mov	x1, sp\n\t"
> >>> +		"orr	x1, x1, #3\n\t"
> >>> +		"ldar	x0, [x1]\n"
> >>> +		::: "x0", "x1" );
> >>> +
> >>> +	return CMD_RET_FAILURE;
> >>> +}
> >>> +
> >>> +static int do_breakpoint(struct cmd_tbl *cmdtp, int flag, int argc,
> >>> +			 char *const argv[])
> >>> +{
> >>> +	asm volatile ("brk	#123\n");
> >>> +
> >>>    	return CMD_RET_FAILURE;
> >>>    }
> >>>    
> >>>    static struct cmd_tbl cmd_sub[] = {
> >>> +	U_BOOT_CMD_MKENT(breakpoint, CONFIG_SYS_MAXARGS, 1, do_breakpoint,
> >>> +			 "", ""),
> >>> +	U_BOOT_CMD_MKENT(unaligned, CONFIG_SYS_MAXARGS, 1, do_unaligned,
> >>> +			 "", ""),
> >>>    	U_BOOT_CMD_MKENT(undefined, CONFIG_SYS_MAXARGS, 1, do_undefined,
> >>>    			 "", ""),
> >>>    };
> >>> @@ -27,7 +59,9 @@ static struct cmd_tbl cmd_sub[] = {
> >>>    static char exception_help_text[] =
> >>>    	"<ex>\n"
> >>>    	"  The following exceptions are available:\n"
> >>> -	"  undefined  - undefined instruction\n"
> >>> +	"  breakpoint - breakpoint instruction exception\n"
> >>> +	"  unaligned  - unaligned LDAR data abort\n"
> >>> +	"  undefined  - undefined instruction exception\n"
> >>>    	;
> >>>    
> >>>    #include <exception.h>  
> >