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

[PATCH v2 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 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         | 18 ++++++++++++
 arch/riscv/cpu/start.S       | 56 +++++++++++++++++++++++++++---------
 arch/riscv/include/asm/smp.h |  7 +++++
 arch/riscv/lib/interrupts.c  |  3 +-
 arch/riscv/lib/smp.c         | 16 ++++++++++-
 5 files changed, 85 insertions(+), 15 deletions(-)

-- 
2.28.0

[PATCH v2 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>
---
I know there is still some discussion in the last series on whether to include
this commit, but I'd like to put out another revision and get feedback.

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

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

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

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

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

---

Changes in v2:
- Make riscv_ipi_init_secondary_hart static

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

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index bfa2d4a426..ab08af5a33 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -72,6 +72,15 @@ 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.
+ */
+static void riscv_ipi_init_secondary_hart(ulong hart, ulong arg0, ulong arg1)
+{
+}
+
 int arch_cpu_init_dm(void)
 {
 	int ret;
@@ -111,6 +120,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)riscv_ipi_init_secondary_hart, 0, 0, 0);
+	if (ret)
+		return ret;
 #endif
 
 	return 0;
-- 
2.28.0

[PATCH v2 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>
---

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 v2 6/7] riscv: Ensure gp is NULL or points to valid data

[Sean Anderson]

This allows code to use a construct like `if (gd & gd->...) { ... }` when
accessing the global data pointer. Without this change, it was possible for
a very early trap to cause _exit_trap to read arbitrary memory. This could
cause a second trap, preventing show_regs from being printed.

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

Changes in v2:
- Set gp early with XIP

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

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 66ca1c7020..a16af79fbe 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,12 @@ 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.
+	 */
+	mv	gp, s0
 	bnez	tp, secondary_hart_loop
 #endif
 
@@ -133,6 +146,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 v2 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>
---

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 a16af79fbe..cb1347559c 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)
@@ -410,6 +421,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 v2 1/7] Revert "riscv: Clear pending interrupts before enabling IPIs"

[Bin Meng]

On Mon, Sep 14, 2020 at 10:23 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> 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>
> ---
> I know there is still some discussion in the last series on whether to include
> this commit, but I'd like to put out another revision and get feedback.
>
> (no changes since v1)
>
>  arch/riscv/cpu/start.S | 2 --
>  1 file changed, 2 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

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

[Bin Meng]

On Mon, Sep 14, 2020 at 10:23 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> 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.
>
> 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

nits: U-Boot

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

Reviewed-by: Bin Meng <bin.meng@windriver.com>

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

[Bin Meng]

On Mon, Sep 14, 2020 at 10:23 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> 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>
> ---
>
> Changes in v2:
> - Remove fences after amoswaps
> - Reword commit message
>
>  arch/riscv/cpu/start.S | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

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

[Bin Meng]

On Mon, Sep 14, 2020 at 10:23 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> This allows code to use a construct like `if (gd & gd->...) { ... }` when
> accessing the global data pointer. Without this change, it was possible for
> a very early trap to cause _exit_trap to read arbitrary memory. This could
> cause a second trap, preventing show_regs from being printed.
>
> 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>
> ---
>
> Changes in v2:
> - Set gp early with XIP
>
>  arch/riscv/cpu/start.S      | 26 +++++++++++++++++++++++---
>  arch/riscv/lib/interrupts.c |  3 ++-
>  2 files changed, 25 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

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

[Bin Meng]

On Mon, Sep 14, 2020 at 10:23 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> This adds comments regarding the ordering and purpose of certain
> instructions as I understand them.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v2:
> - Clarify comments regarding tp
>
>  arch/riscv/cpu/start.S | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

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

[Rick Chen]

> 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>
> ---
>
> (no changes since v1)
>
>  arch/riscv/lib/smp.c | 2 ++
>  1 file changed, 2 insertions(+)
>

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

> 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 v2 3/7] riscv: Use a valid bit to ignore already-pending IPIs

[Rick Chen]

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

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

[PATCH v2 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.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>
> ---
>
> Changes in v2:
> - Make riscv_ipi_init_secondary_hart static
>
>  arch/riscv/cpu/cpu.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> index bfa2d4a426..ab08af5a33 100644
> --- a/arch/riscv/cpu/cpu.c
> +++ b/arch/riscv/cpu/cpu.c
> @@ -72,6 +72,15 @@ 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.

Can we call this function as dummy_pending_ipi_clear() or
prior_stage_pending_ipi_clear(), since the it is the IPIs which does
not be clear
from prior stage bootloader and it is not a general case. It is
special case to deal with.

Also move the descriptions "On the K210, the prior stage bootloader
does not clear IPIs. This presents a problem,..."
in [PATCH v2 0/7] riscv: Correctly handle IPIs already pending upon
boot here or commit of patch [1/7].
Because it will not show in git log history any more.

With the history description, it will help to understand and readable
about the SMP enhance flow for the later joiner.

Thanks,
Rick

> + */
> +static void riscv_ipi_init_secondary_hart(ulong hart, ulong arg0, ulong arg1)
> +{
> +}
> +
>  int arch_cpu_init_dm(void)
>  {
>         int ret;
> @@ -111,6 +120,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)riscv_ipi_init_secondary_hart, 0, 0, 0);
> +       if (ret)
> +               return ret;
>  #endif
>
>         return 0;
> --
> 2.28.0
>

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

[Sean Anderson]

On 9/15/20 5:15 AM, Rick Chen wrote:
>> 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.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>>
>> ---
>>
>> Changes in v2:
>> - Make riscv_ipi_init_secondary_hart static
>>
>>  arch/riscv/cpu/cpu.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
>> index bfa2d4a426..ab08af5a33 100644
>> --- a/arch/riscv/cpu/cpu.c
>> +++ b/arch/riscv/cpu/cpu.c
>> @@ -72,6 +72,15 @@ 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.
> 
> Can we call this function as dummy_pending_ipi_clear() or

Sure that sounds good.

> prior_stage_pending_ipi_clear(), since the it is the IPIs which does
> not be clear
> from prior stage bootloader and it is not a general case. It is
> special case to deal with.
> 
> Also move the descriptions "On the K210, the prior stage bootloader
> does not clear IPIs. This presents a problem,..."
> in [PATCH v2 0/7] riscv: Correctly handle IPIs already pending upon
> boot here or commit of patch [1/7].
> Because it will not show in git log history any more.
> 
> With the history description, it will help to understand and readable
> about the SMP enhance flow for the later joiner.

Ok, I will expand the description of those patches.

> 
> Thanks,
> Rick
> 
>> + */
>> +static void riscv_ipi_init_secondary_hart(ulong hart, ulong arg0, ulong arg1)
>> +{
>> +}
>> +
>>  int arch_cpu_init_dm(void)
>>  {
>>         int ret;
>> @@ -111,6 +120,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)riscv_ipi_init_secondary_hart, 0, 0, 0);
>> +       if (ret)
>> +               return ret;
>>  #endif
>>
>>         return 0;
>> --
>> 2.28.0
>>

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

[Rick Chen]

> On 9/15/20 5:15 AM, Rick Chen wrote:
> >> 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.
> >>
> >> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> >>
> >> ---
> >>
> >> Changes in v2:
> >> - Make riscv_ipi_init_secondary_hart static
> >>
> >>  arch/riscv/cpu/cpu.c | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>
> >> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> >> index bfa2d4a426..ab08af5a33 100644
> >> --- a/arch/riscv/cpu/cpu.c
> >> +++ b/arch/riscv/cpu/cpu.c
> >> @@ -72,6 +72,15 @@ 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.
> >
> > Can we call this function as dummy_pending_ipi_clear() or
>
> Sure that sounds good.

BTW,  riscv_ipi_init_secondary_hart() shall be enclosed by #if
CONFIG_IS_ENABLED(SMP),
or there will occur a warning when some other configs compiling:

arch/riscv/cpu/cpu.c:80:13: warning: 'riscv_ipi_init_secondary_hart'
defined but not used [-Wunused-function]
 static void riscv_ipi_init_secondary_hart(ulong hart, ulong arg0, ulong arg1)
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Thanks,
Rick

>
> > prior_stage_pending_ipi_clear(), since the it is the IPIs which does
> > not be clear
> > from prior stage bootloader and it is not a general case. It is
> > special case to deal with.
> >
> > Also move the descriptions "On the K210, the prior stage bootloader
> > does not clear IPIs. This presents a problem,..."
> > in [PATCH v2 0/7] riscv: Correctly handle IPIs already pending upon
> > boot here or commit of patch [1/7].
> > Because it will not show in git log history any more.
> >
> > With the history description, it will help to understand and readable
> > about the SMP enhance flow for the later joiner.
>
> Ok, I will expand the description of those patches.
>
> >
> > Thanks,
> > Rick
> >
> >> + */
> >> +static void riscv_ipi_init_secondary_hart(ulong hart, ulong arg0, ulong arg1)
> >> +{
> >> +}
> >> +
> >>  int arch_cpu_init_dm(void)
> >>  {
> >>         int ret;
> >> @@ -111,6 +120,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)riscv_ipi_init_secondary_hart, 0, 0, 0);
> >> +       if (ret)
> >> +               return ret;
> >>  #endif
> >>
> >>         return 0;
> >> --
> >> 2.28.0
> >>
>

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

[Rick Chen]

> 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>
> ---
>
> Changes in v2:
> - Remove fences after amoswaps
> - Reword commit message
>
>  arch/riscv/cpu/start.S | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>

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

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

[Rick Chen]

Hi Sean

> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Sean Anderson
> Sent: Monday, September 14, 2020 10:23 PM
> To: u-boot at lists.denx.de
> Cc: Alan Quey-Liang Kao(???); Leo Yu-Chi Liang(???); Heinrich Schuchardt; Bin Meng; Rick Chen; Sean Anderson
> Subject: [PATCH v2 6/7] riscv: Ensure gp is NULL or points to valid data
>
> This allows code to use a construct like `if (gd & gd->...) { ... }` when
> accessing the global data pointer. Without this change, it was possible for
> a very early trap to cause _exit_trap to read arbitrary memory. This could
> cause a second trap, preventing show_regs from being printed.
>
> 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>
> ---
>
> Changes in v2:
> - Set gp early with XIP
>
>  arch/riscv/cpu/start.S      | 26 +++++++++++++++++++++++---
>  arch/riscv/lib/interrupts.c |  3 ++-
>  2 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 66ca1c7020..a16af79fbe 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,12 @@ 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.
> +        */
> +       mv      gp, s0
>         bnez    tp, secondary_hart_loop
>  #endif
>
> @@ -133,6 +146,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
> +

This method seem a little speculative and complicated to read.
Otherwise if the board_init_f_init_reserve() be modified in the future
and the gp will not synchronize any more.
board_init_f_init_reserve is belong generic flow, it can be touch by
anyone and anytime.

May I ask some questions ?
This patch is trying to prevent printing garbage messages form early
trap or early trap itself ?
When early trap occurs, how is the status of mtvec ?
How the early trap be handled in handle_trap ?

Thanks,
Rick



>         /* 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 v2 7/7] riscv: Add some comments to start.S

[Rick Chen]

> This adds comments regarding the ordering and purpose of certain
> instructions as I understand them.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v2:
> - Clarify comments regarding tp
>
>  arch/riscv/cpu/start.S | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>

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

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

[Sean Anderson]

On 9/15/20 10:23 PM, Rick Chen wrote:
> Hi Sean
> 
>> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Sean Anderson
>> Sent: Monday, September 14, 2020 10:23 PM
>> To: u-boot at lists.denx.de
>> Cc: Alan Quey-Liang Kao(???); Leo Yu-Chi Liang(???); Heinrich Schuchardt; Bin Meng; Rick Chen; Sean Anderson
>> Subject: [PATCH v2 6/7] riscv: Ensure gp is NULL or points to valid data
>>
>> This allows code to use a construct like `if (gd & gd->...) { ... }` when
>> accessing the global data pointer. Without this change, it was possible for
>> a very early trap to cause _exit_trap to read arbitrary memory. This could
>> cause a second trap, preventing show_regs from being printed.
>>
>> 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>
>> ---
>>
>> Changes in v2:
>> - Set gp early with XIP
>>
>>  arch/riscv/cpu/start.S      | 26 +++++++++++++++++++++++---
>>  arch/riscv/lib/interrupts.c |  3 ++-
>>  2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>> index 66ca1c7020..a16af79fbe 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,12 @@ 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.
>> +        */
>> +       mv      gp, s0
>>         bnez    tp, secondary_hart_loop
>>  #endif
>>
>> @@ -133,6 +146,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
>> +
>
> May I ask some questions ?
> This patch is trying to prevent printing garbage messages form early
> trap or early trap itself ?

It is trying to prevent garbase messages and secondary early traps, but
not the initial early trap. 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.

> When early trap occurs, how is the status of mtvec ?

mtvec will always point to handle_trap after the first few instructions
executed by U-Boot. Before that, the prior stage's mtvec will be used.
However, because all instructions before setting mtvec are
regicter-register moves, I don't think it is likely for there to be a
trap (unless there is a bug such as enabling RISCV_ISA_C on a processor
which doesn't have that extension).

A more interesting question is "what is the status of gd during an early
trap"? I will compare the situation before and after this patch.

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

> How the early trap be handled in handle_trap ?

The same as a late trap, except _debug_uart_init may not have been
called. Fortunately for us, if the prior stage correctly initialized the
sifive uart, then it will work without initialization.

> This method seem a little speculative and complicated to read.
> Otherwise if the board_init_f_init_reserve() be modified in the future
> and the gp will not synchronize any more.
> board_init_f_init_reserve is belong generic flow, it can be touch by
> anyone and anytime.

And then it is their problem for breaking boot on other boards :)

This commit relies on two assumptions from the rest of U-boot. First,
that a check is made that gd is non-NULL before accessing it in code
which can be called very early. Second, that arch_init_gd is called
after gd is cleared. I think the first is likely to be enforced, given
its existance in several functions (in addition to puts, watchdog_reset
also does a check). The second is also likely to be enforced, because gd
needs to be cleared anyway, and the ordering of 3 lines is unlikely to
warrant significant change.

--Sean

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

[Leo Liang]

On Mon, Sep 14, 2020 at 10:22:58AM -0400, Sean Anderson wrote:
> 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>
> ---
> 
> (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);

Reviewed-by: Leo Liang <ycliang@andestech.com>

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

[Leo Liang]

On Mon, Sep 14, 2020 at 10:22:59AM -0400, Sean Anderson wrote:
> 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.
> 
> 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>
> ---
> 
> 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();

Reviewed-by: Leo Liang <ycliang@andestech.com>

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

[Leo Liang]

On Mon, Sep 14, 2020 at 10:23:03AM -0400, 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>
> ---
> 
> 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 a16af79fbe..cb1347559c 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)
> @@ -410,6 +421,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
>  

Reviewed-by: Leo Liang <ycliang@andestech.com>