[PATCH v2 0/7] riscv: Correctly handle IPIs already pending upon boot
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-xypro...
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(-)
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
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
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);
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
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
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();
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
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
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
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;
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
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
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
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
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
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
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);
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
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
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
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
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.*/@@ -54,10 +57,18 @@ _start: */ mv gp, zero
- /*
* Set the trap handler. This must happen after initializing gp because* the handler may use it.*/
- /* 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*/#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
participants (4)
-
Bin Meng -
Leo Liang -
Rick Chen -
Sean Anderson