| Message ID | 20220109173009.25522-2-andre.przywara@arm.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | armv8: fixes and cleanups | expand |
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") What is the title and URL of the document? I could not find it. It would be good to add this information in the comments. Best regards Heinrich > */ > - 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") > + */ > + 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>
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. 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>
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. 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. 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>
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. > 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.) Maybe we should also add a comment in doc/usage/exception.rst. 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> >
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> > >
> 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? 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?
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? > > 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 ...
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? >> >> 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
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
> Date: Sun, 9 Jan 2022 23:19:10 +0000 > From: Andre Przywara <andre.przywara@arm.com> > > 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? > > > > 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 So: Tested-by: Mark Kettenis <kettenis@openbsd.org> FWIF.
On Mon, 10 Jan 2022 23:37:59 +0100 (CET) Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > Date: Sun, 9 Jan 2022 23:19:10 +0000 > > From: Andre Przywara <andre.przywara@arm.com> > > > > 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? > > > > > > 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@openbsd.org> Many thanks, much appreciated! Cheers, Andre
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") + */ + 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>
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(-)