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

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

On Mon, 10 Jan 2022 00:23:17 +0100
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:

Hi,

> On 1/10/22 00:19, Andre Przywara wrote:
> > On Sun, 9 Jan 2022 23:47:32 +0100 (CET)
> > Mark Kettenis <mark.kettenis@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@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@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?  
> 
> 
> 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?

Yes, it would, but there is not much we can do about it. The reason for
having this subcommand would be more to verify that SErrors are
actually reported (and are not masked or disabled, for whatever reason).

Speaking of exception output: I think the stack pointer is missing?

> >>
> >> 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

I tried that a while back, and ran into some issues (don't remember
exactly). Eventually I gave up, and made a quite simple patch to support
bitmap fonts with glyphs wider than 8 pixels. I will post this, we can
have a discussion on the list then.

Cheers,
Andre