[PATCH v3 0/7] riscv: Correctly handle IPIs already pending upon boot

[PATCH v3 0/7] riscv: Correctly handle IPIs already pending upon boot

[Sean Anderson]

On the K210, the prior stage bootloader does not clear IPIs. This presents
a problem, because U-Boot up until this point assumes (with one exception)
that IPIs are cleared when it starts. This series attempts to fix this in a
robust manner, and fix several concurrency bugs I noticed while fixing
these other areas. Heinrich previously submitted a patch addressing part of
this problem in [1].

[1] https://patchwork.ozlabs.org/project/uboot/patch/20200811035648.3284-1-xypron.glpk at gmx.de/

Changes in v3:
- Rename riscv_ipi_init_secondary_hart to dummy_pending_ipi_clear
- Only compile dummy_pending_ipi_clear when SMP is enabled
- Clarify XIP comment
- Updated and expanded most commit messages

Changes in v2:
- Use a valid bit instead of addr to validate IPIs
- Make riscv_ipi_init_secondary_hart static
- Remove fences after amoswaps
- Set gp early with XIP
- Clarify comments regarding tp

Sean Anderson (7):
  Revert "riscv: Clear pending interrupts before enabling IPIs"
  riscv: Match memory barriers between send_ipi_many and handle_ipi
  riscv: Use a valid bit to ignore already-pending IPIs
  riscv: Clear pending IPIs on initialization
  riscv: Consolidate fences into AMOs for available_harts_lock
  riscv: Ensure gp is NULL or points to valid data
  riscv: Add some comments to start.S

 arch/riscv/cpu/cpu.c         | 20 +++++++++++++
 arch/riscv/cpu/start.S       | 58 ++++++++++++++++++++++++++++--------
 arch/riscv/include/asm/smp.h |  7 +++++
 arch/riscv/lib/interrupts.c  |  3 +-
 arch/riscv/lib/smp.c         | 16 +++++++++-
 5 files changed, 89 insertions(+), 15 deletions(-)

-- 
2.28.0

[PATCH v3 1/7] Revert "riscv: Clear pending interrupts before enabling IPIs"

[Sean Anderson]

Clearing MIP.MSIP is not guaranteed to do anything by the spec. In
addition, most existing RISC-V hardware does nothing when this bit is set.

The following commits "riscv: Use a valid bit to ignore already-pending
IPIs" and "riscv: Clear pending IPIs on initialization" should implement
the original intent of the reverted commit in a more robust manner.

This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
---

(no changes since v1)

 arch/riscv/cpu/start.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index bf9fdf369b..e3222b1ea7 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -65,8 +65,6 @@ _start:
 #else
 	li	t0, SIE_SSIE
 #endif
-	/* Clear any pending IPIs */
-	csrc	MODE_PREFIX(ip), t0
 	csrs	MODE_PREFIX(ie), t0
 #endif
 
-- 
2.28.0

[PATCH v3 1/7] Revert "riscv: Clear pending interrupts before enabling IPIs"

[Rick Chen]

> Clearing MIP.MSIP is not guaranteed to do anything by the spec. In
> addition, most existing RISC-V hardware does nothing when this bit is set.
>
> The following commits "riscv: Use a valid bit to ignore already-pending
> IPIs" and "riscv: Clear pending IPIs on initialization" should implement
> the original intent of the reverted commit in a more robust manner.
>
> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> ---

Reviewed-by: Rick Chen <rick@andestech.com>

[PATCH v3 2/7] riscv: Match memory barriers between send_ipi_many and handle_ipi

[Sean Anderson]

Without a matching barrier on the write side, the barrier in handle_ipi
does nothing. It was entirely possible for the boot hart to write to addr,
arg0, and arg1 *after* sending the IPI, because there was no barrier on the
sending side.

Fixes: 90ae281437 ("riscv: add option to wait for ack from secondary harts in smp functions")
Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Rick Chen <rick@andestech.com>
Reviewed-by: Leo Liang <ycliang@andestech.com>
---

(no changes since v1)

 arch/riscv/lib/smp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c
index ac22136314..ab6d8bd7fa 100644
--- a/arch/riscv/lib/smp.c
+++ b/arch/riscv/lib/smp.c
@@ -54,6 +54,8 @@ static int send_ipi_many(struct ipi_data *ipi, int wait)
 		gd->arch.ipi[reg].arg0 = ipi->arg0;
 		gd->arch.ipi[reg].arg1 = ipi->arg1;
 
+		__smp_mb();
+
 		ret = riscv_send_ipi(reg);
 		if (ret) {
 			pr_err("Cannot send IPI to hart %d\n", reg);
-- 
2.28.0

[PATCH v3 3/7] riscv: Use a valid bit to ignore already-pending IPIs

[Sean Anderson]

Some IPIs may already be pending when U-Boot is started. This could be a
problem if a secondary hart tries to handle an IPI before the boot hart has
initialized the IPI device.

To be specific, the Kendryte K210 ROM-based bootloader does not clear IPIs
before passing control to U-Boot. Without this patch, the secondary hart
jumps to address 0x0 as soon as it enters secondary_hart_loop, and then
hangs in its trap handler.

This commit introduces a valid bit so secondary harts know when and IPI
originates from U-Boot, and it is safe to use the IPI API. The valid bit is
initialized to 0 by board_init_f_init_reserve. Before this, secondary harts
wait in wait_for_gd_init.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Rick Chen <rick@andestech.com>
Reviewed-by: Leo Liang <ycliang@andestech.com>
---

(no changes since v2)

Changes in v2:
- Use a valid bit instead of addr to validate IPIs

 arch/riscv/include/asm/smp.h |  7 +++++++
 arch/riscv/lib/smp.c         | 16 ++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 1b428856b2..2dae0800ce 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -18,14 +18,21 @@
  * IPI data structure. The hart ID is inserted by the hart handling the IPI and
  * calling the function.
  *
+ * @valid is used to determine whether a sent IPI originated from U-Boot. It is
+ * initialized to zero by board_init_f_alloc_reserve. When U-Boot sends its
+ * first IPI, it is set to 1. This prevents already-pending IPIs not sent by
+ * U-Boot from being taken.
+ *
  * @addr: Address of function
  * @arg0: First argument of function
  * @arg1: Second argument of function
+ * @valid: Whether this IPI is valid
  */
 struct ipi_data {
 	ulong addr;
 	ulong arg0;
 	ulong arg1;
+	unsigned int valid;
 };
 
 /**
diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c
index ab6d8bd7fa..8f33ce1fe3 100644
--- a/arch/riscv/lib/smp.c
+++ b/arch/riscv/lib/smp.c
@@ -54,7 +54,13 @@ static int send_ipi_many(struct ipi_data *ipi, int wait)
 		gd->arch.ipi[reg].arg0 = ipi->arg0;
 		gd->arch.ipi[reg].arg1 = ipi->arg1;
 
-		__smp_mb();
+		/*
+		 * Ensure valid only becomes set when the IPI parameters are
+		 * set. An IPI may already be pending on other harts, so we
+		 * need a way to signal that the IPI device has been
+		 * initialized, and that it is ok to call the function.
+		 */
+		__smp_store_release(&gd->arch.ipi[reg].valid, 1);
 
 		ret = riscv_send_ipi(reg);
 		if (ret) {
@@ -83,7 +89,13 @@ void handle_ipi(ulong hart)
 	if (hart >= CONFIG_NR_CPUS)
 		return;
 
-	__smp_mb();
+	/*
+	 * If valid is not set, then U-Boot has not requested the IPI. The
+	 * IPI device may not be initialized, so all we can do is wait for
+	 * U-Boot to initialize it and send an IPI
+	 */
+	if (!__smp_load_acquire(&gd->arch.ipi[hart].valid))
+		return;
 
 	smp_function = (void (*)(ulong, ulong, ulong))gd->arch.ipi[hart].addr;
 	invalidate_icache_all();
-- 
2.28.0

[PATCH v3 4/7] riscv: Clear pending IPIs on initialization

[Sean Anderson]

Even though we no longer call smp_function if an IPI was not sent by
U-Boot, we still need to clear any IPIs which were pending from the
execution environment. Otherwise, secondary harts will busy-wait in
secondary_hart_loop, instead of relaxing.

Along with the previous commit ("riscv: Use a valid bit to ignore
already-pending IPIs"), this fixes SMP booting on the Kendryte K210.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v3:
- Rename riscv_ipi_init_secondary_hart to dummy_pending_ipi_clear
- Only compile dummy_pending_ipi_clear when SMP is enabled

Changes in v2:
- Make riscv_ipi_init_secondary_hart static

 arch/riscv/cpu/cpu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index bfa2d4a426..85592f5bee 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -72,6 +72,17 @@ static int riscv_cpu_probe(void)
 	return 0;
 }
 
+/*
+ * This is called on secondary harts just after the IPI is init'd. Currently
+ * there's nothing to do, since we just need to clear any existing IPIs, and
+ * that is handled by the sending of an ipi itself.
+ */
+#if CONFIG_IS_ENABLED(SMP)
+static void dummy_pending_ipi_clear(ulong hart, ulong arg0, ulong arg1)
+{
+}
+#endif
+
 int arch_cpu_init_dm(void)
 {
 	int ret;
@@ -111,6 +122,15 @@ int arch_cpu_init_dm(void)
 	ret = riscv_init_ipi();
 	if (ret)
 		return ret;
+
+	/*
+	 * Clear all pending IPIs on secondary harts. We don't do anything on
+	 * the boot hart, since we never send an IPI to ourselves, and no
+	 * interrupts are enabled
+	 */
+	ret = smp_call_function((ulong)dummy_pending_ipi_clear, 0, 0, 0);
+	if (ret)
+		return ret;
 #endif
 
 	return 0;
-- 
2.28.0

[PATCH v3 4/7] riscv: Clear pending IPIs on initialization

[Rick Chen]

> Even though we no longer call smp_function if an IPI was not sent by
> U-Boot, we still need to clear any IPIs which were pending from the
> execution environment. Otherwise, secondary harts will busy-wait in
> secondary_hart_loop, instead of relaxing.
>
> Along with the previous commit ("riscv: Use a valid bit to ignore
> already-pending IPIs"), this fixes SMP booting on the Kendryte K210.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>
> ---
>
> Changes in v3:
> - Rename riscv_ipi_init_secondary_hart to dummy_pending_ipi_clear
> - Only compile dummy_pending_ipi_clear when SMP is enabled
>
> Changes in v2:
> - Make riscv_ipi_init_secondary_hart static
>
>  arch/riscv/cpu/cpu.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>

Reviewed-by: Rick Chen <rick@andestech.com>

[PATCH v3 5/7] riscv: Consolidate fences into AMOs for available_harts_lock

[Sean Anderson]

We can reduce the number of instructions needed to use available_harts_lock
by using the aq and rl suffixes for AMOs.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Rick Chen <rick@andestech.com>
---

(no changes since v2)

Changes in v2:
- Remove fences after amoswaps
- Reword commit message

 arch/riscv/cpu/start.S | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index e3222b1ea7..66ca1c7020 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -125,14 +125,12 @@ call_board_init_f_0:
 
 #ifndef CONFIG_XIP
 	la	t0, available_harts_lock
-	fence	rw, w
-	amoswap.w zero, zero, 0(t0)
+	amoswap.w.rl zero, zero, 0(t0)
 
 wait_for_gd_init:
 	la	t0, available_harts_lock
 	li	t1, 1
-1:	amoswap.w t1, t1, 0(t0)
-	fence	r, rw
+1:	amoswap.w.aq t1, t1, 0(t0)
 	bnez	t1, 1b
 
 	/* register available harts in the available_harts mask */
@@ -142,8 +140,7 @@ wait_for_gd_init:
 	or	t2, t2, t1
 	SREG	t2, GD_AVAILABLE_HARTS(gp)
 
-	fence	rw, w
-	amoswap.w zero, zero, 0(t0)
+	amoswap.w.rl zero, zero, 0(t0)
 
 	/*
 	 * Continue on hart lottery winner, others branch to
-- 
2.28.0

[PATCH v3 6/7] riscv: Ensure gp is NULL or points to valid data

[Sean Anderson]

This ensures constructs like `if (gd & gd->...) { ... }` work when
accessing the global data pointer. Without this change, it was possible for
a very early trap to cause _exit_trap to directly or indirectly (through
printf) to read arbitrary memory. This could cause a second trap,
preventing show_regs from being printed.

printf (and specifically puts) uses gd to determine what function to print
with. These functions in turn use gd to find the serial device, etc.
However, before accessing gd, puts first checks to see if it is non-NULL.
This indicates an existing (perhaps undocumented) assumption that either gd
is NULL or it is completely valid.

Before this patch, gd either points to unexpected data (because it retains
the value it did from the prior-stage) or points to uninitialized data
(because it has not yet been initialized by board_init_f_init_reserve)
until the hart has acquired available_harts_lock. This can cause two
problems, depending on the value of gd->flags. If GD_FLG_SERIAL_READY is
unset, then some garbage data will be printed to stdout, but there will not
be a second trap. However, if GD_FLG_SERIAL_READY is set, then puts will
try to print with serial_puts, which will likely cause a second trap.

After this patch, gd is zero up until either a hart has set it in
wait_for_gd_init, or until it is set by arch_init_gd. This prevents its
usage before its data is initialized because both handle_trap and puts
ensure that gd is nonzero before using it. After gd has been set, it is OK
to access it because its data has been cleared (and so flags is valid).

XIP cannot use locks because flash is not writable. This leaves it
vulnerable to the same class of bugs regarding already-pending IPIs as
before this series. Fixing that would require finding another method of
synchronization, which is outside the scope of this series.

Fixes: 7c6ca03eae ("riscv: additional crash information")
Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v3:
- Clarify XIP comment

Changes in v2:
- Set gp early with XIP

 arch/riscv/cpu/start.S      | 28 +++++++++++++++++++++++++---
 arch/riscv/lib/interrupts.c |  3 ++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 66ca1c7020..eb852538ca 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -47,6 +47,13 @@ _start:
 	mv	tp, a0
 	mv	s1, a1
 
+	/*
+	 * Set the global data pointer to a known value in case we get a very
+	 * early trap. The global data pointer will be set its actual value only
+	 * after it has been initialized.
+	 */
+	mv	gp, zero
+
 	la	t0, trap_entry
 	csrw	MODE_PREFIX(tvec), t0
 
@@ -85,10 +92,10 @@ call_board_init_f_0:
 	jal	board_init_f_alloc_reserve
 
 	/*
-	 * Set global data pointer here for all harts, uninitialized at this
-	 * point.
+	 * Save global data pointer for later. We don't set it here because it
+	 * is not initialized yet.
 	 */
-	mv	gp, a0
+	mv	s0, a0
 
 	/* setup stack */
 #if CONFIG_IS_ENABLED(SMP)
@@ -109,6 +116,14 @@ call_board_init_f_0:
 	amoswap.w s2, t1, 0(t0)
 	bnez	s2, wait_for_gd_init
 #else
+	/*
+	 * FIXME: gp is set before it is initialized. If an XIP U-Boot ever
+	 * encounters a pending IPI on boot it is liable to jump to whatever
+	 * memory happens to be in ipi_data.addr on boot. It may also run into
+	 * problems if it encounters an exception too early (because printf/puts
+	 * accesses gd).
+	 */
+	mv	gp, s0
 	bnez	tp, secondary_hart_loop
 #endif
 
@@ -133,6 +148,13 @@ wait_for_gd_init:
 1:	amoswap.w.aq t1, t1, 0(t0)
 	bnez	t1, 1b
 
+	/*
+	 * Set the global data pointer only when gd_t has been initialized.
+	 * This was already set by arch_setup_gd on the boot hart, but all other
+	 * harts' global data pointers gets set here.
+	 */
+	mv	gp, s0
+
 	/* register available harts in the available_harts mask */
 	li	t1, 1
 	sll	t1, t1, tp
diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
index cd47e64487..ad870e98d8 100644
--- a/arch/riscv/lib/interrupts.c
+++ b/arch/riscv/lib/interrupts.c
@@ -78,7 +78,8 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs)
 
 	printf("EPC: " REG_FMT " RA: " REG_FMT " TVAL: " REG_FMT "\n",
 	       epc, regs->ra, tval);
-	if (gd->flags & GD_FLG_RELOC)
+	/* Print relocation adjustments, but only if gd is initialized */
+	if (gd && gd->flags & GD_FLG_RELOC)
 		printf("EPC: " REG_FMT " RA: " REG_FMT " reloc adjusted\n\n",
 		       epc - gd->reloc_off, regs->ra - gd->reloc_off);
 
-- 
2.28.0

[PATCH v3 6/7] riscv: Ensure gp is NULL or points to valid data

[Rick Chen]

> This ensures constructs like `if (gd & gd->...) { ... }` work when
> accessing the global data pointer. Without this change, it was possible for
> a very early trap to cause _exit_trap to directly or indirectly (through
> printf) to read arbitrary memory. This could cause a second trap,
> preventing show_regs from being printed.
>
> printf (and specifically puts) uses gd to determine what function to print
> with. These functions in turn use gd to find the serial device, etc.
> However, before accessing gd, puts first checks to see if it is non-NULL.
> This indicates an existing (perhaps undocumented) assumption that either gd
> is NULL or it is completely valid.
>
> Before this patch, gd either points to unexpected data (because it retains
> the value it did from the prior-stage) or points to uninitialized data
> (because it has not yet been initialized by board_init_f_init_reserve)
> until the hart has acquired available_harts_lock. This can cause two
> problems, depending on the value of gd->flags. If GD_FLG_SERIAL_READY is
> unset, then some garbage data will be printed to stdout, but there will not
> be a second trap. However, if GD_FLG_SERIAL_READY is set, then puts will
> try to print with serial_puts, which will likely cause a second trap.
>
> After this patch, gd is zero up until either a hart has set it in
> wait_for_gd_init, or until it is set by arch_init_gd. This prevents its
> usage before its data is initialized because both handle_trap and puts
> ensure that gd is nonzero before using it. After gd has been set, it is OK
> to access it because its data has been cleared (and so flags is valid).
>
> XIP cannot use locks because flash is not writable. This leaves it
> vulnerable to the same class of bugs regarding already-pending IPIs as
> before this series. Fixing that would require finding another method of
> synchronization, which is outside the scope of this series.
>
> Fixes: 7c6ca03eae ("riscv: additional crash information")
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>
> ---
>
> Changes in v3:
> - Clarify XIP comment
>
> Changes in v2:
> - Set gp early with XIP
>
>  arch/riscv/cpu/start.S      | 28 +++++++++++++++++++++++++---
>  arch/riscv/lib/interrupts.c |  3 ++-
>  2 files changed, 27 insertions(+), 4 deletions(-)
>

Reviewed-by: Rick Chen <rick@andestech.com>

[PATCH v3 7/7] riscv: Add some comments to start.S

[Sean Anderson]

This adds comments regarding the ordering and purpose of certain
instructions as I understand them.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Rick Chen <rick@andestech.com>
Reviewed-by: Leo Liang <ycliang@andestech.com>
---

(no changes since v2)

Changes in v2:
- Clarify comments regarding tp

 arch/riscv/cpu/start.S | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index eb852538ca..bbc737ed9a 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -43,7 +43,10 @@ _start:
 	csrr	a0, CSR_MHARTID
 #endif
 
-	/* save hart id and dtb pointer */
+	/*
+	 * Save hart id and dtb pointer. The thread pointer register is not
+	 * modified by C code. It is used by secondary_hart_loop.
+	 */
 	mv	tp, a0
 	mv	s1, a1
 
@@ -54,10 +57,18 @@ _start:
 	 */
 	mv	gp, zero
 
+	/*
+	 * Set the trap handler. This must happen after initializing gp because
+	 * the handler may use it.
+	 */
 	la	t0, trap_entry
 	csrw	MODE_PREFIX(tvec), t0
 
-	/* mask all interrupts */
+	/*
+	 * Mask all interrupts. Interrupts are disabled globally (in m/sstatus)
+	 * for U-Boot, but we will need to read m/sip to determine if we get an
+	 * IPI
+	 */
 	csrw	MODE_PREFIX(ie), zero
 
 #if CONFIG_IS_ENABLED(SMP)
@@ -412,6 +423,10 @@ secondary_hart_relocate:
 	mv	gp, a2
 #endif
 
+/*
+ * Interrupts are disabled globally, but they can still be read from m/sip. The
+ * wfi function will wake us up if we get an IPI, even if we do not trap.
+ */
 secondary_hart_loop:
 	wfi
 
-- 
2.28.0

[PATCH v3 7/7] riscv: Add some comments to start.S

[Heinrich Schuchardt]

On 21.09.20 13:51, Sean Anderson wrote:
> This adds comments regarding the ordering and purpose of certain
> instructions as I understand them.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Rick Chen <rick@andestech.com>
> Reviewed-by: Leo Liang <ycliang@andestech.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Clarify comments regarding tp
>
>  arch/riscv/cpu/start.S | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index eb852538ca..bbc737ed9a 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -43,7 +43,10 @@ _start:
>  	csrr	a0, CSR_MHARTID
>  #endif
>
> -	/* save hart id and dtb pointer */
> +	/*
> +	 * Save hart id and dtb pointer. The thread pointer register is not
> +	 * modified by C code. It is used by secondary_hart_loop.
> +	 */
>  	mv	tp, a0
>  	mv	s1, a1

I would like to understand the implications for the UEFI sub-system.

The current design only takes care of the conservation of the global
data gd.

When U-Boot calls a UEFI payload it saves the value of gd (register gp,
x3) in a variable.

When the payload calls the UEFI API implemented in U-Boot we store the
payload's value of register gp and restore the U-Boot value (see macro
EFI_ENTRY).

Before returning from the UEFI call we restore the payload's value of gp
(see macro EFI_EXIT).

When a payload returns to the U-Boot shell we restore gp.

Up to now we do not take care of any other register. I am not aware of
any specification requiring register tp to be conserved by the UEFI payload.

Is there any other register that has to be conserved except gp?

Best regards

Heinrich

>
> @@ -54,10 +57,18 @@ _start:
>  	 */
>  	mv	gp, zero
>
> +	/*
> +	 * Set the trap handler. This must happen after initializing gp because
> +	 * the handler may use it.
> +	 */
>  	la	t0, trap_entry
>  	csrw	MODE_PREFIX(tvec), t0
>
> -	/* mask all interrupts */
> +	/*
> +	 * Mask all interrupts. Interrupts are disabled globally (in m/sstatus)
> +	 * for U-Boot, but we will need to read m/sip to determine if we get an
> +	 * IPI
> +	 */
>  	csrw	MODE_PREFIX(ie), zero
>
>  #if CONFIG_IS_ENABLED(SMP)
> @@ -412,6 +423,10 @@ secondary_hart_relocate:
>  	mv	gp, a2
>  #endif
>
> +/*
> + * Interrupts are disabled globally, but they can still be read from m/sip. The
> + * wfi function will wake us up if we get an IPI, even if we do not trap.
> + */
>  secondary_hart_loop:
>  	wfi
>
>

[PATCH v3 7/7] riscv: Add some comments to start.S

[Sean Anderson]

On 9/21/20 12:23 PM, Heinrich Schuchardt wrote:
> On 21.09.20 13:51, Sean Anderson wrote:
>> This adds comments regarding the ordering and purpose of certain
>> instructions as I understand them.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>> Reviewed-by: Rick Chen <rick@andestech.com>
>> Reviewed-by: Leo Liang <ycliang@andestech.com>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - Clarify comments regarding tp
>>
>>  arch/riscv/cpu/start.S | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>> index eb852538ca..bbc737ed9a 100644
>> --- a/arch/riscv/cpu/start.S
>> +++ b/arch/riscv/cpu/start.S
>> @@ -43,7 +43,10 @@ _start:
>>  	csrr	a0, CSR_MHARTID
>>  #endif
>>
>> -	/* save hart id and dtb pointer */
>> +	/*
>> +	 * Save hart id and dtb pointer. The thread pointer register is not
>> +	 * modified by C code. It is used by secondary_hart_loop.
>> +	 */
>>  	mv	tp, a0
>>  	mv	s1, a1
> 
> I would like to understand the implications for the UEFI sub-system.
> 
> The current design only takes care of the conservation of the global
> data gd.
> 
> When U-Boot calls a UEFI payload it saves the value of gd (register gp,
> x3) in a variable.
> 
> When the payload calls the UEFI API implemented in U-Boot we store the
> payload's value of register gp and restore the U-Boot value (see macro
> EFI_ENTRY).
> 
> Before returning from the UEFI call we restore the payload's value of gp
> (see macro EFI_EXIT).
> 
> When a payload returns to the U-Boot shell we restore gp.
> 
> Up to now we do not take care of any other register. I am not aware of
> any specification requiring register tp to be conserved by the UEFI payload.
> 
> Is there any other register that has to be conserved except gp?

So the core problem here is that there is no way to get the current hart
id from S-Mode, except by saving it from a0 on entry. I believe an SBI
call was proposed, but it was deemed unnecessary. Unfortunately, gp is
already in-use for gd. On the boot hart, the current hart id is stored
in gd->arch.boot_hart, so tp is not used. On secondary harts, the hart
id is stored in tp, and must be valid for secondary_hart_loop. AFAIK tp
is the only other register besides gp which needs to be preserved on
secondary harts.

When we spoke on IRC a few weeks ago, I you said that only the boot hart
enters UEFI payloads. So there should be no need to save tp for UEFI,
because it is unused on the boot hart. If a payload does not return to
U-Boot, there is also no need to save tp on secondary harts. The only
issue would be if a payload is started on secondary harts and then
returns to U-Boot.

--Sean