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

[Andre Przywara]

On Mon, 10 Jan 2022 23:37:59 +0100 (CET)
Mark Kettenis <mark.kettenis at xs4all.nl> wrote:

> > Date: Sun, 9 Jan 2022 23:19:10 +0000
> > From: Andre Przywara <andre.przywara at arm.com>
> > 
> > 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?    
> > > 
> > > 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.  
> 
> Booting the M1 mini still works.  With CMD_EXCEPTION enabled:
> 
> => exception breakpoint  
> "Synchronous Abort" handler, esr 0xf200007b
> 
> => exception unaligned  
> <nothing happens>
> 
> => exception undefined  
> "Synchronous Abort" handler, esr 0x02000000

Great, exactly as expected!
I have a slight change to check for the FEAT_LSE ID bit and print a
warning for the unaligned check, but it's already OK as is, I guess.
Will CC: you on a v2.

> So:
> 
> Tested-by: Mark Kettenis <kettenis at openbsd.org>

Many thanks, much appreciated!

Cheers,
Andre