Re: [PATCH v6 13/19] riscv: Fix race conditions when initializing IPI
On 3/10/20 9:33 PM, Rick Chen wrote: > Hi Sean > >> On 3/10/20 4:20 AM, Rick Chen wrote: >>> Hi Sean >>> The IPI code could have race conditions in several places. * Several harts could race on the value of gd->arch->clint/plic * Non-boot harts could race with the main hart on the DM subsystem In addition, if an IPI was pending when U-Boot started, it would cause the IPI handler to jump to address 0. To address these problems, a new function riscv_init_ipi is introduced. It is called once during arch_cpu_init_dm. Before this point, no riscv_*_ipi functions may be called. Access is synchronized by gd->arch->ipi_ready. Signed-off-by: Sean Anderson --- >>> Can you try to clear mip/sip in startup flow before secondary_hart_loop: Maybe it can help to overcome the problem of calling riscv_clear_ipi() before riscv_init_ipi() that you added. >>> >>> How is the verified result about trying to clear mip/sip in startup flow ? >>> I have asked you twice in v5, but you still have no response about it. >>> >>> [PATCH v5 27/33] riscv: Fix race conditions when initializing IPI >>> https://patchwork.ozlabs.org/patch/1246864/#2377119 >>> >>> Thanks >>> Rick >> >> I managed to get it working, and this patch incorporates that change. >> However, I forgot to update the commit message. The original issue I had >> was related to an accidental change to my config, and not to the >> clearing of MIP. > > So it is not a race condition issue, right ? It is sort of a race condition? If the IP CSR is not cleared, then we can have a race condition. > > Maybe you shall split this into two patchs as below > > patch 1 > riscv: Clear pending ipi in start code > Describe that it can how and what it help to fix the problem you > encounter more detail > e.g. > (Perhaps the prior stage sends an IPI but does not clear it) ... > > patch 2 > riscv: refine ipi initialize code flow > Describe that your motivation and intention more detail > e.g. > I think the macro approach is a bit confusing, > since it's unclear at first glance what function > will be initializing the clint/plic. > Given U-Boot's otherwise completely SMP-unsafe design, > I think it's better to be explicit and conservative in these areas. > > It can help us to roll back and debug in the future. > > Thanks, > Rick This will be split in the next revision. >> >> --Sean
Re: [PATCH v6 13/19] riscv: Fix race conditions when initializing IPI
Hi Sean > On 3/10/20 4:20 AM, Rick Chen wrote: > > Hi Sean > > > >> The IPI code could have race conditions in several places. > >> * Several harts could race on the value of gd->arch->clint/plic > >> * Non-boot harts could race with the main hart on the DM subsystem In > >> addition, if an IPI was pending when U-Boot started, it would cause the > >> IPI handler to jump to address 0. > >> > >> To address these problems, a new function riscv_init_ipi is introduced. It > >> is called once during arch_cpu_init_dm. Before this point, no riscv_*_ipi > >> functions may be called. Access is synchronized by gd->arch->ipi_ready. > >> > >> Signed-off-by: Sean Anderson > >> --- > >> > > > >> Can you try to clear mip/sip in startup flow before secondary_hart_loop: > >> Maybe it can help to overcome the problem of calling riscv_clear_ipi() > >> before riscv_init_ipi() that you added. > > > > How is the verified result about trying to clear mip/sip in startup flow ? > > I have asked you twice in v5, but you still have no response about it. > > > > [PATCH v5 27/33] riscv: Fix race conditions when initializing IPI > > https://patchwork.ozlabs.org/patch/1246864/#2377119 > > > > Thanks > > Rick > > I managed to get it working, and this patch incorporates that change. > However, I forgot to update the commit message. The original issue I had > was related to an accidental change to my config, and not to the > clearing of MIP. So it is not a race condition issue, right ? Maybe you shall split this into two patchs as below patch 1 riscv: Clear pending ipi in start code Describe that it can how and what it help to fix the problem you encounter more detail e.g. (Perhaps the prior stage sends an IPI but does not clear it) ... patch 2 riscv: refine ipi initialize code flow Describe that your motivation and intention more detail e.g. I think the macro approach is a bit confusing, since it's unclear at first glance what function will be initializing the clint/plic. Given U-Boot's otherwise completely SMP-unsafe design, I think it's better to be explicit and conservative in these areas. It can help us to roll back and debug in the future. Thanks, Rick > > --Sean
Re: [PATCH v6 13/19] riscv: Fix race conditions when initializing IPI
On 3/10/20 4:20 AM, Rick Chen wrote: > Hi Sean > >> The IPI code could have race conditions in several places. >> * Several harts could race on the value of gd->arch->clint/plic >> * Non-boot harts could race with the main hart on the DM subsystem In >> addition, if an IPI was pending when U-Boot started, it would cause the >> IPI handler to jump to address 0. >> >> To address these problems, a new function riscv_init_ipi is introduced. It >> is called once during arch_cpu_init_dm. Before this point, no riscv_*_ipi >> functions may be called. Access is synchronized by gd->arch->ipi_ready. >> >> Signed-off-by: Sean Anderson >> --- >> > >> Can you try to clear mip/sip in startup flow before secondary_hart_loop: >> Maybe it can help to overcome the problem of calling riscv_clear_ipi() >> before riscv_init_ipi() that you added. > > How is the verified result about trying to clear mip/sip in startup flow ? > I have asked you twice in v5, but you still have no response about it. > > [PATCH v5 27/33] riscv: Fix race conditions when initializing IPI > https://patchwork.ozlabs.org/patch/1246864/#2377119 > > Thanks > Rick I managed to get it working, and this patch incorporates that change. However, I forgot to update the commit message. The original issue I had was related to an accidental change to my config, and not to the clearing of MIP. --Sean
Re: [PATCH v6 13/19] riscv: Fix race conditions when initializing IPI
Hi Sean
> The IPI code could have race conditions in several places.
> * Several harts could race on the value of gd->arch->clint/plic
> * Non-boot harts could race with the main hart on the DM subsystem In
> addition, if an IPI was pending when U-Boot started, it would cause the
> IPI handler to jump to address 0.
>
> To address these problems, a new function riscv_init_ipi is introduced. It
> is called once during arch_cpu_init_dm. Before this point, no riscv_*_ipi
> functions may be called. Access is synchronized by gd->arch->ipi_ready.
>
> Signed-off-by: Sean Anderson
> ---
>
> Can you try to clear mip/sip in startup flow before secondary_hart_loop:
> Maybe it can help to overcome the problem of calling riscv_clear_ipi()
> before riscv_init_ipi() that you added.
How is the verified result about trying to clear mip/sip in startup flow ?
I have asked you twice in v5, but you still have no response about it.
[PATCH v5 27/33] riscv: Fix race conditions when initializing IPI
https://patchwork.ozlabs.org/patch/1246864/#2377119
Thanks
Rick
> Changes in v6:
> - Fix some formatting
> - Clear IPIs before enabling interrupts instead of using a ipi_ready flag
> - Only print messages on error in smp code
>
> Changes in v5:
> - New
>
> arch/riscv/cpu/cpu.c | 6
> arch/riscv/cpu/start.S| 2 ++
> arch/riscv/include/asm/smp.h | 43 +++
> arch/riscv/lib/andes_plic.c | 34 -
> arch/riscv/lib/sbi_ipi.c | 5
> arch/riscv/lib/sifive_clint.c | 33 +++--
> arch/riscv/lib/smp.c | 56 ---
> 7 files changed, 92 insertions(+), 87 deletions(-)
>
> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> index e457f6acbf..f851374255 100644
> --- a/arch/riscv/cpu/cpu.c
> +++ b/arch/riscv/cpu/cpu.c
> @@ -96,6 +96,12 @@ int arch_cpu_init_dm(void)
> csr_write(CSR_SATP, 0);
> }
>
> +#ifdef CONFIG_SMP
> + ret = riscv_init_ipi();
> + if (ret)
> + return ret;
> +#endif
> +
> return 0;
> }
>
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 6b3ff99c38..e8740c8568 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -67,6 +67,8 @@ _start:
> #else
> li t0, SIE_SSIE
> #endif
> + /* Clear any pending IPIs */
> + csrcMODE_PREFIX(ip), t0
> csrsMODE_PREFIX(ie), t0
> #endif
>
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index 74de92ed13..1b428856b2 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -51,4 +51,47 @@ void handle_ipi(ulong hart);
> */
> int smp_call_function(ulong addr, ulong arg0, ulong arg1, int wait);
>
> +/**
> + * riscv_init_ipi() - Initialize inter-process interrupt (IPI) driver
> + *
> + * Platform code must provide this function. This function is called once
> after
> + * the cpu driver is initialized. No other riscv_*_ipi() calls will be made
> + * before this function is called.
> + *
> + * @return 0 if OK, -ve on error
> + */
> +int riscv_init_ipi(void);
> +
> +/**
> + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> + *
> + * Platform code must provide this function.
> + *
> + * @hart: Hart ID of receiving hart
> + * @return 0 if OK, -ve on error
> + */
> +int riscv_send_ipi(int hart);
> +
> +/**
> + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
> + *
> + * Platform code must provide this function.
> + *
> + * @hart: Hart ID of hart to be cleared
> + * @return 0 if OK, -ve on error
> + */
> +int riscv_clear_ipi(int hart);
> +
> +/**
> + * riscv_get_ipi() - Get status of inter-processor interrupt (IPI)
> + *
> + * Platform code must provide this function.
> + *
> + * @hart: Hart ID of hart to be checked
> + * @pending: Pointer to variable with result of the check,
> + * 1 if IPI is pending, 0 otherwise
> + * @return 0 if OK, -ve on error
> + */
> +int riscv_get_ipi(int hart, int *pending);
> +
> #endif
> diff --git a/arch/riscv/lib/andes_plic.c b/arch/riscv/lib/andes_plic.c
> index 20529ab3eb..8484f76386 100644
> --- a/arch/riscv/lib/andes_plic.c
> +++ b/arch/riscv/lib/andes_plic.c
> @@ -30,20 +30,6 @@
> #define SEND_IPI_TO_HART(hart) (0x80 >> (hart))
>
> DECLARE_GLOBAL_DATA_PTR;
> -static int init_plic(void);
> -
> -#define PLIC_BASE_GET(void)\
> - do {\
> - long *ret; \
> - \
> - if (!gd->arch.plic) { \
> - ret = syscon_get_first_range(RISCV_SYSCON_PLIC); \
> - if (IS_ERR(ret))\
> - return PTR_ERR(ret);\
>
[PATCH v6 13/19] riscv: Fix race conditions when initializing IPI
The IPI code could have race conditions in several places.
* Several harts could race on the value of gd->arch->clint/plic
* Non-boot harts could race with the main hart on the DM subsystem In
addition, if an IPI was pending when U-Boot started, it would cause the
IPI handler to jump to address 0.
To address these problems, a new function riscv_init_ipi is introduced. It
is called once during arch_cpu_init_dm. Before this point, no riscv_*_ipi
functions may be called. Access is synchronized by gd->arch->ipi_ready.
Signed-off-by: Sean Anderson
---
Changes in v6:
- Fix some formatting
- Clear IPIs before enabling interrupts instead of using a ipi_ready flag
- Only print messages on error in smp code
Changes in v5:
- New
arch/riscv/cpu/cpu.c | 6
arch/riscv/cpu/start.S| 2 ++
arch/riscv/include/asm/smp.h | 43 +++
arch/riscv/lib/andes_plic.c | 34 -
arch/riscv/lib/sbi_ipi.c | 5
arch/riscv/lib/sifive_clint.c | 33 +++--
arch/riscv/lib/smp.c | 56 ---
7 files changed, 92 insertions(+), 87 deletions(-)
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index e457f6acbf..f851374255 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -96,6 +96,12 @@ int arch_cpu_init_dm(void)
csr_write(CSR_SATP, 0);
}
+#ifdef CONFIG_SMP
+ ret = riscv_init_ipi();
+ if (ret)
+ return ret;
+#endif
+
return 0;
}
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 6b3ff99c38..e8740c8568 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -67,6 +67,8 @@ _start:
#else
li t0, SIE_SSIE
#endif
+ /* Clear any pending IPIs */
+ csrcMODE_PREFIX(ip), t0
csrsMODE_PREFIX(ie), t0
#endif
diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 74de92ed13..1b428856b2 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -51,4 +51,47 @@ void handle_ipi(ulong hart);
*/
int smp_call_function(ulong addr, ulong arg0, ulong arg1, int wait);
+/**
+ * riscv_init_ipi() - Initialize inter-process interrupt (IPI) driver
+ *
+ * Platform code must provide this function. This function is called once after
+ * the cpu driver is initialized. No other riscv_*_ipi() calls will be made
+ * before this function is called.
+ *
+ * @return 0 if OK, -ve on error
+ */
+int riscv_init_ipi(void);
+
+/**
+ * riscv_send_ipi() - Send inter-processor interrupt (IPI)
+ *
+ * Platform code must provide this function.
+ *
+ * @hart: Hart ID of receiving hart
+ * @return 0 if OK, -ve on error
+ */
+int riscv_send_ipi(int hart);
+
+/**
+ * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
+ *
+ * Platform code must provide this function.
+ *
+ * @hart: Hart ID of hart to be cleared
+ * @return 0 if OK, -ve on error
+ */
+int riscv_clear_ipi(int hart);
+
+/**
+ * riscv_get_ipi() - Get status of inter-processor interrupt (IPI)
+ *
+ * Platform code must provide this function.
+ *
+ * @hart: Hart ID of hart to be checked
+ * @pending: Pointer to variable with result of the check,
+ * 1 if IPI is pending, 0 otherwise
+ * @return 0 if OK, -ve on error
+ */
+int riscv_get_ipi(int hart, int *pending);
+
#endif
diff --git a/arch/riscv/lib/andes_plic.c b/arch/riscv/lib/andes_plic.c
index 20529ab3eb..8484f76386 100644
--- a/arch/riscv/lib/andes_plic.c
+++ b/arch/riscv/lib/andes_plic.c
@@ -30,20 +30,6 @@
#define SEND_IPI_TO_HART(hart) (0x80 >> (hart))
DECLARE_GLOBAL_DATA_PTR;
-static int init_plic(void);
-
-#define PLIC_BASE_GET(void)\
- do {\
- long *ret; \
- \
- if (!gd->arch.plic) { \
- ret = syscon_get_first_range(RISCV_SYSCON_PLIC); \
- if (IS_ERR(ret))\
- return PTR_ERR(ret);\
- gd->arch.plic = ret;\
- init_plic();\
- } \
- } while (0)
static int enable_ipi(int hart)
{
@@ -93,13 +79,21 @@ static int init_plic(void)
return -ENODEV;
}
+int riscv_init_ipi(void)
+{
+ int ret = syscon_get_first_range(RISCV_SYSCON_PLIC);
+
+ if (IS_ERR(ret))
+ return PTR_ERR(ret);
+ gd->arch.plic = ret;
+
+ return init_plic();
+}
+
int riscv_send_ipi(int hart)
{
- unsigned int ipi;
+ unsigned int ipi = (SEND_IPI_TO_HART(hart) << (8 * gd->arch.boot_hart));
-
