[PATCH 0/6] armv8: fixes and cleanups

[PATCH 0/6] armv8: fixes and cleanups

[Andre Przywara]

Hi,

I was looking into the arm64 boot code lately and stumbled upon some
issues. Also Nishanth brought back memories of a lengthy debug session,
which was caused due to U-Boot keeping SErrors masked. As the resulting
patches are all somewhat related, I gathered this series here to address
those problems.

Patches 1 to 3 address exception handling issues, with the SError
enablement being the most prominent fix here.
Patch 4 cleans up asm/io.h. This was on the list before[1], but was
somehow lost when it was intercepted by a shorter version of itself.
Patches 5 and 6 clean up some unnecessarily complicated AArch64 assembly
code.

I did only some light testing, as some code paths do not apply to boards
I have (ARMV8_MULTIENTRY).

Please have a look and test!

Cheers,
Andre

[1] https://lists.denx.de/pipermail/u-boot/2019-January/354296.html

Andre Przywara (6):
  cmd: exception: arm64: fix undefined, add faults
  armv8: Always unmask SErrors
  armv8: Force SP_ELx stack pointer usage
  arm: Clean up asm/io.h
  armv8: Simplify switch_el macro
  armv8: Fix and simplify branch_if_master/branch_if_slave

 arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S | 11 +--
 arch/arm/cpu/armv8/start.S                   | 10 +-
 arch/arm/include/asm/io.h                    | 98 +-------------------
 arch/arm/include/asm/macro.h                 | 37 +++-----
 arch/arm/include/asm/system.h                |  1 +
 arch/arm/mach-rmobile/lowlevel_init_gen3.S   |  2 +-
 arch/arm/mach-socfpga/lowlevel_init_soc64.S  |  2 +-
 board/cortina/presidio-asic/lowlevel_init.S  |  2 +-
 cmd/arm/exception64.c                        | 42 ++++++++-
 9 files changed, 63 insertions(+), 142 deletions(-)

-- 
2.17.6

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

[Andre Przywara]

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")
+	 */
+	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>
-- 
2.17.6

[PATCH 2/6] armv8: Always unmask SErrors

[Andre Przywara]

The ARMv8 architecture describes the "SError interrupt" as the fourth
kind of exception, next to synchronous exceptions, IRQs, and FIQs.
Those SErrors signal exceptional conditions from which the system might
not easily recover, and are normally generated by the interconnect as a
response to some bus error. A typical situation is access to a
non-existing memory address or device, but it might be deliberately
triggered by a device as well.
The SError interrupt replaces the Armv7 asynchronous abort.

Trusted Firmware enters U-Boot (BL33) typically with SErrors masked,
and we never enable them. However any SError condition still triggers
the SError interrupt, and this condition stays pending, it just won't be
handled. If now later on the Linux kernel unmasks the "A" bit in PState,
it will immediately take the exception, leading to a kernel crash.
This leaves many people scratching their head about the reason for
this, and leads to long debug sessions, possibly looking at the wrong
places (the kernel, but not U-Boot).

To avoid the situation, just unmask SErrors early in the ARMv8 boot
process, so that the U-Boot exception handlers reports them in a timely
manner. As SErrors are typically asynchronous, the register dump does
not need to point at the actual culprit, but it should happen very
shortly after the condition.

For those exceptions to be taken, we also need to route them to EL2,
if U-Boot is running in this exception level.

This removes the respective code snippet from the Freescale lowlevel
routine, as this is now handled in generic ARMv8 code.

Reported-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S | 9 ---------
 arch/arm/cpu/armv8/start.S                   | 3 +++
 arch/arm/include/asm/system.h                | 1 +
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S b/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S
index 3aa1a9c3e5c..0929c58d43f 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S
+++ b/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S
@@ -74,15 +74,6 @@ ENDPROC(smp_kick_all_cpus)
 ENTRY(lowlevel_init)
 	mov	x29, lr			/* Save LR */
 
-	/* unmask SError and abort */
-	msr daifclr, #4
-
-	/* Set HCR_EL2[AMO] so SError @EL2 is taken */
-	mrs	x0, hcr_el2
-	orr	x0, x0, #0x20			/* AMO */
-	msr	hcr_el2, x0
-	isb
-
 	switch_el x1, 1f, 100f, 100f	/* skip if not in EL3 */
 1:
 
diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index b3eef705a53..a26c18329cb 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -130,6 +130,8 @@ pie_fixup_done:
 	b	0f
 2:	mrs	x1, hcr_el2
 	tbnz	x1, #34, 1f			/* HCR_EL2.E2H */
+	orr	x1, x1, #HCR_EL2_AMO_EL2	/* Route SErrors to EL2 */
+	msr	hcr_el2, x1
 	set_vbar vbar_el2, x0
 	mov	x0, #0x33ff
 	msr	cptr_el2, x0			/* Enable FP/SIMD */
@@ -138,6 +140,7 @@ pie_fixup_done:
 	mov	x0, #3 << 20
 	msr	cpacr_el1, x0			/* Enable FP/SIMD */
 0:
+	msr	daifclr, #0x4			/* Unmask SError interrupts */
 
 #ifdef COUNTER_FREQUENCY
 	branch_if_not_highest_el x0, 4f
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index f75eea16b36..87d1c77e8b1 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -82,6 +82,7 @@
 #define HCR_EL2_RW_AARCH64	(1 << 31) /* EL1 is AArch64                   */
 #define HCR_EL2_RW_AARCH32	(0 << 31) /* Lower levels are AArch32         */
 #define HCR_EL2_HCD_DIS		(1 << 29) /* Hypervisor Call disabled         */
+#define HCR_EL2_AMO_EL2		(1 <<  5) /* Route SErrors to EL2             */
 
 /*
  * ID_AA64ISAR1_EL1 bits definitions
-- 
2.17.6

[PATCH 3/6] armv8: Force SP_ELx stack pointer usage

[Andre Przywara]

In ARMv8 we have the choice between two stack pointers to use: SP_EL0 or
SP_ELx, which is banked per exception level. This choice is stored in
the SP field of PState, and can be read and set via the SPSel special
register. When the CPU takes an exception, it automatically switches to
the SP_ELx stack pointer.

Trusted Firmware enters U-Boot typically with SPSel set to 1, so we use
SP_ELx all along as our sole stack pointer, both for normal operation and
for exceptions.

But if we now for some reason enter U-Boot with SPSel cleared, we will
setup and use SP_EL0, which is fine, but leaves SP_ELx uninitialised.
When we now take an exception, we try to save the GPRs to some undefined
location, which will usually end badly.

To make sure we always have SP_ELx pointing to some memory, set SPSel
to 1 in the early boot code, to ensure safe operation at all times.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/cpu/armv8/start.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index a26c18329cb..47a612883c5 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -196,6 +196,7 @@ slave_cpu:
 	br	x0			/* branch to the given address */
 #endif /* CONFIG_ARMV8_MULTIENTRY */
 master_cpu:
+	msr	SPSel, #1		/* make sure we use SP_ELx */
 	bl	_main
 
 #ifdef CONFIG_SYS_RESET_SCTRL
-- 
2.17.6

[PATCH 4/6] arm: Clean up asm/io.h

[Andre Przywara]

asm/io.h is the header file containing the central MMIO accessor macros.
Judging by the header and the comments, it was apparently once copied
from the Linux kernel, but has deviated since then *heavily*. There is
absolutely no point in staying close to the original Linux code anymore,
so just remove the old cruft, by:
- removing pointless Linux history
- removing commented code
- removing outdated comments
- removing unused definitions (for mem_isa)

This massively improves the readability of the file.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/include/asm/io.h | 98 +--------------------------------------
 1 file changed, 2 insertions(+), 96 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 36b840378a9..89b1015bc4d 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -1,45 +1,26 @@
 /*
- *  linux/include/asm-arm/io.h
+ * I/O device access primitives. Based on early versions from the Linux kernel.
  *
  *  Copyright (C) 1996-2000 Russell King
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
- *
- * Modifications:
- *  16-Sep-1996	RMK	Inlined the inx/outx functions & optimised for both
- *			constant addresses and variable addresses.
- *  04-Dec-1997	RMK	Moved a lot of this stuff to the new architecture
- *			specific IO header files.
- *  27-Mar-1999	PJB	Second parameter of memcpy_toio is const..
- *  04-Apr-1999	PJB	Added check_signature.
- *  12-Dec-1999	RMK	More cleanups
- *  18-Jun-2000 RMK	Removed virt_to_* and friends definitions
  */
 #ifndef __ASM_ARM_IO_H
 #define __ASM_ARM_IO_H
 
-#ifdef __KERNEL__
-
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <asm/byteorder.h>
 #include <asm/memory.h>
 #include <asm/barriers.h>
-#if 0	/* XXX###XXX */
-#include <asm/arch/hardware.h>
-#endif	/* XXX###XXX */
 
 static inline void sync(void)
 {
 }
 
-/*
- * Generic virtual read/write.  Note that we don't support half-word
- * read/writes.  We define __arch_*[bl] here, and leave __arch_*w
- * to the architecture specific code.
- */
+/* Generic virtual read/write. */
 #define __arch_getb(a)			(*(volatile unsigned char *)(a))
 #define __arch_getw(a)			(*(volatile unsigned short *)(a))
 #define __arch_getl(a)			(*(volatile unsigned int *)(a))
@@ -247,13 +228,6 @@ static inline void __raw_readsl(unsigned long addr, void *data, int longlen)
 #define setbits_64(addr, set) setbits(64, addr, set)
 #define clrsetbits_64(addr, clear, set) clrsetbits(64, addr, clear, set)
 
-/*
- * Now, pick up the machine-defined IO definitions
- */
-#if 0	/* XXX###XXX */
-#include <asm/arch/io.h>
-#endif	/* XXX###XXX */
-
 /*
  *  IO port access primitives
  *  -------------------------
@@ -317,16 +291,6 @@ static inline void __raw_readsl(unsigned long addr, void *data, int longlen)
 #define writesb(a, d, s)	__raw_writesb((unsigned long)a, d, s)
 #define readsb(a, d, s)		__raw_readsb((unsigned long)a, d, s)
 
-/*
- * DMA-consistent mapping functions.  These allocate/free a region of
- * uncached, unwrite-buffered mapped memory space for use with DMA
- * devices.  This is the "generic" version.  The PCI specific version
- * is in pci.h
- */
-extern void *consistent_alloc(int gfp, size_t size, dma_addr_t *handle);
-extern void consistent_free(void *vaddr, size_t size, dma_addr_t handle);
-extern void consistent_sync(void *vaddr, size_t size, int rw);
-
 /*
  * String version of IO memory access ops:
  */
@@ -334,8 +298,6 @@ extern void _memcpy_fromio(void *, unsigned long, size_t);
 extern void _memcpy_toio(unsigned long, const void *, size_t);
 extern void _memset_io(unsigned long, int, size_t);
 
-extern void __readwrite_bug(const char *fn);
-
 /* Optimized copy functions to read from/write to IO sapce */
 #ifdef CONFIG_ARM64
 #include <cpu_func.h>
@@ -441,62 +403,6 @@ void __memset_io(volatile void __iomem *dst, int c, size_t count)
 #define memcpy_toio(a, b, c)		memcpy((void *)(a), (b), (c))
 #endif
 
-/*
- * If this architecture has ISA IO, then define the isa_read/isa_write
- * macros.
- */
-#ifdef __mem_isa
-
-#define isa_readb(addr)			__raw_readb(__mem_isa(addr))
-#define isa_readw(addr)			__raw_readw(__mem_isa(addr))
-#define isa_readl(addr)			__raw_readl(__mem_isa(addr))
-#define isa_writeb(val,addr)		__raw_writeb(val,__mem_isa(addr))
-#define isa_writew(val,addr)		__raw_writew(val,__mem_isa(addr))
-#define isa_writel(val,addr)		__raw_writel(val,__mem_isa(addr))
-#define isa_memset_io(a,b,c)		_memset_io(__mem_isa(a),(b),(c))
-#define isa_memcpy_fromio(a,b,c)	_memcpy_fromio((a),__mem_isa(b),(c))
-#define isa_memcpy_toio(a,b,c)		_memcpy_toio(__mem_isa((a)),(b),(c))
-
-#define isa_eth_io_copy_and_sum(a,b,c,d) \
-				eth_copy_and_sum((a),__mem_isa(b),(c),(d))
-
-static inline int
-isa_check_signature(unsigned long io_addr, const unsigned char *signature,
-		    int length)
-{
-	int retval = 0;
-	do {
-		if (isa_readb(io_addr) != *signature)
-			goto out;
-		io_addr++;
-		signature++;
-		length--;
-	} while (length);
-	retval = 1;
-out:
-	return retval;
-}
-
-#else	/* __mem_isa */
-
-#define isa_readb(addr)			(__readwrite_bug("isa_readb"),0)
-#define isa_readw(addr)			(__readwrite_bug("isa_readw"),0)
-#define isa_readl(addr)			(__readwrite_bug("isa_readl"),0)
-#define isa_writeb(val,addr)		__readwrite_bug("isa_writeb")
-#define isa_writew(val,addr)		__readwrite_bug("isa_writew")
-#define isa_writel(val,addr)		__readwrite_bug("isa_writel")
-#define isa_memset_io(a,b,c)		__readwrite_bug("isa_memset_io")
-#define isa_memcpy_fromio(a,b,c)	__readwrite_bug("isa_memcpy_fromio")
-#define isa_memcpy_toio(a,b,c)		__readwrite_bug("isa_memcpy_toio")
-
-#define isa_eth_io_copy_and_sum(a,b,c,d) \
-				__readwrite_bug("isa_eth_io_copy_and_sum")
-
-#define isa_check_signature(io,sig,len)	(0)
-
-#endif	/* __mem_isa */
-#endif	/* __KERNEL__ */
-
 #include <asm-generic/io.h>
 #include <iotrace.h>
 
-- 
2.17.6

[PATCH 5/6] armv8: Simplify switch_el macro

[Andre Przywara]

The switch_el macro is a neat contraption to handle cases where we need
different code depending on the current exception level, but its
implementation was longer than needed.

Simplify it by doing just one comparison, then using the different
condition codes to branch to the desired target. PState.CurrentEL just
holds two bits, and since we don't care about EL0, we can use >, =, < to
select EL3, EL2 and EL1, respectively.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/include/asm/macro.h | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/macro.h b/arch/arm/include/asm/macro.h
index ec0171e0e6c..acd519020d7 100644
--- a/arch/arm/include/asm/macro.h
+++ b/arch/arm/include/asm/macro.h
@@ -69,12 +69,10 @@ lr	.req	x30
  */
 .macro	switch_el, xreg, el3_label, el2_label, el1_label
 	mrs	\xreg, CurrentEL
-	cmp	\xreg, 0xc
-	b.eq	\el3_label
-	cmp	\xreg, 0x8
+	cmp	\xreg, #0x8
+	b.gt	\el3_label
 	b.eq	\el2_label
-	cmp	\xreg, 0x4
-	b.eq	\el1_label
+	b.lt	\el1_label
 .endm
 
 /*
-- 
2.17.6

[PATCH 6/6] armv8: Fix and simplify branch_if_master/branch_if_slave

[Andre Przywara]

The branch_if_master macro jumps to a label if the CPU is the "master"
core, which we define as having all affinity levels set to 0. To check
for this condition, we need to mask off some bits from the MPIDR
register, then compare the remaining register value against zero.

The implementation of this was slighly broken (it preserved the upper
RES0 bits), overly complicated and hard to understand, especially since
it lacked comments. The same was true for the very similar
branch_if_slave macro.

Use a much shorter assembly sequence for those checks, use the same
masking for both macros (just negate the final branch), and put some
comments on them, to make it clear what the code does.
This allows to drop the second temporary register for branch_if_master,
so we adjust all call sites as well.

Also use the opportunity to remove a misleading comment: the macro
works fine on SoCs with multiple clusters. Judging by the commit
message, the original problem with the Juno SoC stems from the fact that
the master CPU *can* be configured to be from cluster 1, so the
assumption that the master CPU has all affinity values set to 0 does not
hold there. But this is already mentioned above in a comment, so remove
the extra comment.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S |  2 +-
 arch/arm/cpu/armv8/start.S                   |  6 ++--
 arch/arm/include/asm/macro.h                 | 29 ++++++--------------
 arch/arm/mach-rmobile/lowlevel_init_gen3.S   |  2 +-
 arch/arm/mach-socfpga/lowlevel_init_soc64.S  |  2 +-
 board/cortina/presidio-asic/lowlevel_init.S  |  2 +-
 6 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S b/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S
index 0929c58d43f..2fb4e404a24 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S
+++ b/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S
@@ -200,7 +200,7 @@ ENTRY(lowlevel_init)
 #endif
 
 100:
-	branch_if_master x0, x1, 2f
+	branch_if_master x0, 2f
 
 #if defined(CONFIG_MP) && defined(CONFIG_ARMV8_MULTIENTRY)
 	/*
diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index 47a612883c5..b02fb76d627 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -179,11 +179,11 @@ pie_fixup_done:
 	bl	lowlevel_init
 
 #if defined(CONFIG_ARMV8_SPIN_TABLE) && !defined(CONFIG_SPL_BUILD)
-	branch_if_master x0, x1, master_cpu
+	branch_if_master x0, master_cpu
 	b	spin_table_secondary_jump
 	/* never return */
 #elif defined(CONFIG_ARMV8_MULTIENTRY)
-	branch_if_master x0, x1, master_cpu
+	branch_if_master x0, master_cpu
 
 	/*
 	 * Slave CPUs
@@ -342,7 +342,7 @@ WEAK(lowlevel_init)
 #endif
 
 #ifdef CONFIG_ARMV8_MULTIENTRY
-	branch_if_master x0, x1, 2f
+	branch_if_master x0, 2f
 
 	/*
 	 * Slave should wait for master clearing spin table.
diff --git a/arch/arm/include/asm/macro.h b/arch/arm/include/asm/macro.h
index acd519020d7..1a1edc98703 100644
--- a/arch/arm/include/asm/macro.h
+++ b/arch/arm/include/asm/macro.h
@@ -121,19 +121,10 @@ lr	.req	x30
  */
 .macro	branch_if_slave, xreg, slave_label
 #ifdef CONFIG_ARMV8_MULTIENTRY
-	/* NOTE: MPIDR handling will be erroneous on multi-cluster machines */
 	mrs	\xreg, mpidr_el1
-	tst	\xreg, #0xff		/* Test Affinity 0 */
-	b.ne	\slave_label
-	lsr	\xreg, \xreg, #8
-	tst	\xreg, #0xff		/* Test Affinity 1 */
-	b.ne	\slave_label
-	lsr	\xreg, \xreg, #8
-	tst	\xreg, #0xff		/* Test Affinity 2 */
-	b.ne	\slave_label
-	lsr	\xreg, \xreg, #16
-	tst	\xreg, #0xff		/* Test Affinity 3 */
-	b.ne	\slave_label
+	and	\xreg, \xreg,  0xffffffffff	/* clear bits [63:40] */
+	and	\xreg, \xreg, ~0x00ff000000	/* also clear bits [31:24] */
+	cbnz	\xreg, \slave_label
 #endif
 .endm
 
@@ -141,16 +132,12 @@ lr	.req	x30
  * Branch if current processor is a master,
  * choose processor with all zero affinity value as the master.
  */
-.macro	branch_if_master, xreg1, xreg2, master_label
+.macro	branch_if_master, xreg, master_label
 #ifdef CONFIG_ARMV8_MULTIENTRY
-	/* NOTE: MPIDR handling will be erroneous on multi-cluster machines */
-	mrs	\xreg1, mpidr_el1
-	lsr	\xreg2, \xreg1, #32
-	lsl	\xreg2, \xreg2, #32
-	lsl	\xreg1, \xreg1, #40
-	lsr	\xreg1, \xreg1, #40
-	orr	\xreg1, \xreg1, \xreg2
-	cbz	\xreg1, \master_label
+	mrs	\xreg, mpidr_el1
+	and	\xreg, \xreg,  0xffffffffff	/* clear bits [63:40] */
+	and	\xreg, \xreg, ~0x00ff000000	/* also clear bits [31:24] */
+	cbz	\xreg, \master_label
 #else
 	b	\master_label
 #endif
diff --git a/arch/arm/mach-rmobile/lowlevel_init_gen3.S b/arch/arm/mach-rmobile/lowlevel_init_gen3.S
index 1df2c403453..0d7780031ac 100644
--- a/arch/arm/mach-rmobile/lowlevel_init_gen3.S
+++ b/arch/arm/mach-rmobile/lowlevel_init_gen3.S
@@ -64,7 +64,7 @@ ENTRY(lowlevel_init)
 #endif
 #endif
 
-	branch_if_master x0, x1, 2f
+	branch_if_master x0, 2f
 
 	/*
 	 * Slave should wait for master clearing spin table.
diff --git a/arch/arm/mach-socfpga/lowlevel_init_soc64.S b/arch/arm/mach-socfpga/lowlevel_init_soc64.S
index 612ea8a0371..875927cc4d9 100644
--- a/arch/arm/mach-socfpga/lowlevel_init_soc64.S
+++ b/arch/arm/mach-socfpga/lowlevel_init_soc64.S
@@ -38,7 +38,7 @@ slave_wait_atf:
 #endif
 
 #ifdef CONFIG_ARMV8_MULTIENTRY
-	branch_if_master x0, x1, 2f
+	branch_if_master x0, 2f
 
 	/*
 	 * Slave should wait for master clearing spin table.
diff --git a/board/cortina/presidio-asic/lowlevel_init.S b/board/cortina/presidio-asic/lowlevel_init.S
index 4450a5df79f..cbf8134346d 100644
--- a/board/cortina/presidio-asic/lowlevel_init.S
+++ b/board/cortina/presidio-asic/lowlevel_init.S
@@ -50,7 +50,7 @@ skip_smp_setup:
 #endif
 
 #ifdef CONFIG_ARMV8_MULTIENTRY
-	branch_if_master x0, x1, 2f
+	branch_if_master x0, 2f
 
 	/*
 	 * Slave should wait for master clearing spin table.
-- 
2.17.6

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

[Heinrich Schuchardt]

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>

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

[Heinrich Schuchardt]

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>

Re: [PATCH 0/6] armv8: fixes and cleanups

[Michael Walle]

Am 2022-01-09 18:30, schrieb Andre Przywara:
> Hi,
> 
> I was looking into the arm64 boot code lately and stumbled upon some
> issues. Also Nishanth brought back memories of a lengthy debug session,
> which was caused due to U-Boot keeping SErrors masked. As the resulting
> patches are all somewhat related, I gathered this series here to 
> address
> those problems.
> 
> Patches 1 to 3 address exception handling issues, with the SError
> enablement being the most prominent fix here.
> Patch 4 cleans up asm/io.h. This was on the list before[1], but was
> somehow lost when it was intercepted by a shorter version of itself.
> Patches 5 and 6 clean up some unnecessarily complicated AArch64 
> assembly
> code.
> 
> I did only some light testing, as some code paths do not apply to 
> boards
> I have (ARMV8_MULTIENTRY).
> 
> Please have a look and test!

FWIW, tested on kontron sl28 board with and without TF-A. U-Boot is 
either
running in EL3 or EL2.

Tested-by: Michael Walle <michael@walle.cc>

-michael

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

[Andre Przywara]

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>  

Re: [PATCH 4/6] arm: Clean up asm/io.h

[Andre Przywara]

On Sun,  9 Jan 2022 17:30:07 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

Hi Rick, Leo:

> asm/io.h is the header file containing the central MMIO accessor macros.
> Judging by the header and the comments, it was apparently once copied
> from the Linux kernel, but has deviated since then *heavily*. There is
> absolutely no point in staying close to the original Linux code anymore,
> so just remove the old cruft, by:
> - removing pointless Linux history
> - removing commented code
> - removing outdated comments
> - removing unused definitions (for mem_isa)
> 
> This massively improves the readability of the file.

it looks like arch/risc-v/include/asm/io.h is a copy of the ARM
version, and includes the same pointless legacy. You might want to
remove those parts there as well.

Cheers,
Andre

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/include/asm/io.h | 98 +--------------------------------------
>  1 file changed, 2 insertions(+), 96 deletions(-)
> 
> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> index 36b840378a9..89b1015bc4d 100644
> --- a/arch/arm/include/asm/io.h
> +++ b/arch/arm/include/asm/io.h
> @@ -1,45 +1,26 @@
>  /*
> - *  linux/include/asm-arm/io.h
> + * I/O device access primitives. Based on early versions from the Linux kernel.
>   *
>   *  Copyright (C) 1996-2000 Russell King
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> - *
> - * Modifications:
> - *  16-Sep-1996	RMK	Inlined the inx/outx functions & optimised for both
> - *			constant addresses and variable addresses.
> - *  04-Dec-1997	RMK	Moved a lot of this stuff to the new architecture
> - *			specific IO header files.
> - *  27-Mar-1999	PJB	Second parameter of memcpy_toio is const..
> - *  04-Apr-1999	PJB	Added check_signature.
> - *  12-Dec-1999	RMK	More cleanups
> - *  18-Jun-2000 RMK	Removed virt_to_* and friends definitions
>   */
>  #ifndef __ASM_ARM_IO_H
>  #define __ASM_ARM_IO_H
>  
> -#ifdef __KERNEL__
> -
>  #include <linux/types.h>
>  #include <linux/kernel.h>
>  #include <asm/byteorder.h>
>  #include <asm/memory.h>
>  #include <asm/barriers.h>
> -#if 0	/* XXX###XXX */
> -#include <asm/arch/hardware.h>
> -#endif	/* XXX###XXX */
>  
>  static inline void sync(void)
>  {
>  }
>  
> -/*
> - * Generic virtual read/write.  Note that we don't support half-word
> - * read/writes.  We define __arch_*[bl] here, and leave __arch_*w
> - * to the architecture specific code.
> - */
> +/* Generic virtual read/write. */
>  #define __arch_getb(a)			(*(volatile unsigned char *)(a))
>  #define __arch_getw(a)			(*(volatile unsigned short *)(a))
>  #define __arch_getl(a)			(*(volatile unsigned int *)(a))
> @@ -247,13 +228,6 @@ static inline void __raw_readsl(unsigned long addr, void *data, int longlen)
>  #define setbits_64(addr, set) setbits(64, addr, set)
>  #define clrsetbits_64(addr, clear, set) clrsetbits(64, addr, clear, set)
>  
> -/*
> - * Now, pick up the machine-defined IO definitions
> - */
> -#if 0	/* XXX###XXX */
> -#include <asm/arch/io.h>
> -#endif	/* XXX###XXX */
> -
>  /*
>   *  IO port access primitives
>   *  -------------------------
> @@ -317,16 +291,6 @@ static inline void __raw_readsl(unsigned long addr, void *data, int longlen)
>  #define writesb(a, d, s)	__raw_writesb((unsigned long)a, d, s)
>  #define readsb(a, d, s)		__raw_readsb((unsigned long)a, d, s)
>  
> -/*
> - * DMA-consistent mapping functions.  These allocate/free a region of
> - * uncached, unwrite-buffered mapped memory space for use with DMA
> - * devices.  This is the "generic" version.  The PCI specific version
> - * is in pci.h
> - */
> -extern void *consistent_alloc(int gfp, size_t size, dma_addr_t *handle);
> -extern void consistent_free(void *vaddr, size_t size, dma_addr_t handle);
> -extern void consistent_sync(void *vaddr, size_t size, int rw);
> -
>  /*
>   * String version of IO memory access ops:
>   */
> @@ -334,8 +298,6 @@ extern void _memcpy_fromio(void *, unsigned long, size_t);
>  extern void _memcpy_toio(unsigned long, const void *, size_t);
>  extern void _memset_io(unsigned long, int, size_t);
>  
> -extern void __readwrite_bug(const char *fn);
> -
>  /* Optimized copy functions to read from/write to IO sapce */
>  #ifdef CONFIG_ARM64
>  #include <cpu_func.h>
> @@ -441,62 +403,6 @@ void __memset_io(volatile void __iomem *dst, int c, size_t count)
>  #define memcpy_toio(a, b, c)		memcpy((void *)(a), (b), (c))
>  #endif
>  
> -/*
> - * If this architecture has ISA IO, then define the isa_read/isa_write
> - * macros.
> - */
> -#ifdef __mem_isa
> -
> -#define isa_readb(addr)			__raw_readb(__mem_isa(addr))
> -#define isa_readw(addr)			__raw_readw(__mem_isa(addr))
> -#define isa_readl(addr)			__raw_readl(__mem_isa(addr))
> -#define isa_writeb(val,addr)		__raw_writeb(val,__mem_isa(addr))
> -#define isa_writew(val,addr)		__raw_writew(val,__mem_isa(addr))
> -#define isa_writel(val,addr)		__raw_writel(val,__mem_isa(addr))
> -#define isa_memset_io(a,b,c)		_memset_io(__mem_isa(a),(b),(c))
> -#define isa_memcpy_fromio(a,b,c)	_memcpy_fromio((a),__mem_isa(b),(c))
> -#define isa_memcpy_toio(a,b,c)		_memcpy_toio(__mem_isa((a)),(b),(c))
> -
> -#define isa_eth_io_copy_and_sum(a,b,c,d) \
> -				eth_copy_and_sum((a),__mem_isa(b),(c),(d))
> -
> -static inline int
> -isa_check_signature(unsigned long io_addr, const unsigned char *signature,
> -		    int length)
> -{
> -	int retval = 0;
> -	do {
> -		if (isa_readb(io_addr) != *signature)
> -			goto out;
> -		io_addr++;
> -		signature++;
> -		length--;
> -	} while (length);
> -	retval = 1;
> -out:
> -	return retval;
> -}
> -
> -#else	/* __mem_isa */
> -
> -#define isa_readb(addr)			(__readwrite_bug("isa_readb"),0)
> -#define isa_readw(addr)			(__readwrite_bug("isa_readw"),0)
> -#define isa_readl(addr)			(__readwrite_bug("isa_readl"),0)
> -#define isa_writeb(val,addr)		__readwrite_bug("isa_writeb")
> -#define isa_writew(val,addr)		__readwrite_bug("isa_writew")
> -#define isa_writel(val,addr)		__readwrite_bug("isa_writel")
> -#define isa_memset_io(a,b,c)		__readwrite_bug("isa_memset_io")
> -#define isa_memcpy_fromio(a,b,c)	__readwrite_bug("isa_memcpy_fromio")
> -#define isa_memcpy_toio(a,b,c)		__readwrite_bug("isa_memcpy_toio")
> -
> -#define isa_eth_io_copy_and_sum(a,b,c,d) \
> -				__readwrite_bug("isa_eth_io_copy_and_sum")
> -
> -#define isa_check_signature(io,sig,len)	(0)
> -
> -#endif	/* __mem_isa */
> -#endif	/* __KERNEL__ */
> -
>  #include <asm-generic/io.h>
>  #include <iotrace.h>
>  

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

[Heinrich Schuchardt]

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

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

[Andre Przywara]

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

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

[Mark Kettenis]

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

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

[Andre Przywara]

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

Re: [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@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

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

[Andre Przywara]

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

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

[Mark Kettenis]

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

Re: [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@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

Re: [PATCH 4/6] arm: Clean up asm/io.h

[Leo Liang]

On Sun, Jan 09, 2022 at 09:39:03PM +0000, Andre Przywara wrote:
> On Sun,  9 Jan 2022 17:30:07 +0000
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
> Hi Rick, Leo:
> 
> > asm/io.h is the header file containing the central MMIO accessor macros.
> > Judging by the header and the comments, it was apparently once copied
> > from the Linux kernel, but has deviated since then *heavily*. There is
> > absolutely no point in staying close to the original Linux code anymore,
> > so just remove the old cruft, by:
> > - removing pointless Linux history
> > - removing commented code
> > - removing outdated comments
> > - removing unused definitions (for mem_isa)
> > 
> > This massively improves the readability of the file.
> 
> it looks like arch/risc-v/include/asm/io.h is a copy of the ARM
> version, and includes the same pointless legacy. You might want to
> remove those parts there as well.
> 
> Cheers,
> Andre
> 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/include/asm/io.h | 98 +--------------------------------------
> >  1 file changed, 2 insertions(+), 96 deletions(-)

Hi Andre,

Thanks for the reminder!
Will do ASAP!

Best regards,
Leo