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 seanga2@gmail.com

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

•   csrc    MODE_PREFIX(ip), t0
    csrs    MODE_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));
  
  

•   PLIC_BASE_GET();
  

•   ipi = (SEND_IPI_TO_HART(hart) << (8 * gd->arch.boot_hart));
    writel(ipi, (void __iomem *)PENDING_REG(gd->arch.plic,
                            gd->arch.boot_hart));
  
  

@@ -110,8 +104,6 @@ int riscv_clear_ipi(int hart) { u32 source_id;

•   PLIC_BASE_GET();
  

•   source_id = readl((void __iomem *)CLAIM_REG(gd->arch.plic, hart));
    writel(source_id, (void __iomem *)CLAIM_REG(gd->arch.plic, hart));
  
  

@@ -120,8 +112,6 @@ int riscv_clear_ipi(int hart)

int riscv_get_ipi(int hart, int *pending) {

•   PLIC_BASE_GET();
  

•   *pending = readl((void __iomem *)PENDING_REG(gd->arch.plic,
                                                 gd->arch.boot_hart));
    *pending = !!(*pending & SEND_IPI_TO_HART(hart));
  

diff --git a/arch/riscv/lib/sbi_ipi.c b/arch/riscv/lib/sbi_ipi.c index 9a698ce74e..310d1bd2a4 100644 --- a/arch/riscv/lib/sbi_ipi.c +++ b/arch/riscv/lib/sbi_ipi.c @@ -7,6 +7,11 @@ #include <common.h> #include <asm/sbi.h>

+int riscv_init_ipi(void) +{

•   return 0;
  

+}

int riscv_send_ipi(int hart) { ulong mask; diff --git a/arch/riscv/lib/sifive_clint.c b/arch/riscv/lib/sifive_clint.c index 5e0d25720b..78fc6c868d 100644 --- a/arch/riscv/lib/sifive_clint.c +++ b/arch/riscv/lib/sifive_clint.c @@ -24,22 +24,8 @@

DECLARE_GLOBAL_DATA_PTR;

-#define CLINT_BASE_GET(void) \

•   do {                                                            \
  

•           long *ret;                                              \
  

•                                                                   \
  

•           if (!gd->arch.clint) {                                  \
  

•                   ret = syscon_get_first_range(RISCV_SYSCON_CLINT); \
  

•                   if (IS_ERR(ret))                                \
  

•                           return PTR_ERR(ret);                    \
  

•                   gd->arch.clint = ret;                           \
  

•           }                                                       \
  

•   } while (0)
  

int riscv_get_time(u64 *time) {

•   CLINT_BASE_GET();
  

•   *time = readq((void __iomem *)MTIME_REG(gd->arch.clint));
  
    return 0;
  

@@ -47,17 +33,24 @@ int riscv_get_time(u64 *time)

int riscv_set_timecmp(int hart, u64 cmp) {

•   CLINT_BASE_GET();
  

•   writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint, hart));
  
    return 0;
  

}

+int riscv_init_ipi(void) +{

•   long *ret = syscon_get_first_range(RISCV_SYSCON_CLINT);
  

•   if (IS_ERR(ret))
  

•           return PTR_ERR(ret);
  

•   gd->arch.clint = ret;
  

•   return 0;
  

+}

int riscv_send_ipi(int hart) {

•   CLINT_BASE_GET();
  

•   writel(1, (void __iomem *)MSIP_REG(gd->arch.clint, hart));
  
    return 0;
  

@@ -65,8 +58,6 @@ int riscv_send_ipi(int hart)

int riscv_clear_ipi(int hart) {

•   CLINT_BASE_GET();
  

•   writel(0, (void __iomem *)MSIP_REG(gd->arch.clint, hart));
  
    return 0;
  

@@ -74,8 +65,6 @@ int riscv_clear_ipi(int hart)

int riscv_get_ipi(int hart, int *pending) {

•   CLINT_BASE_GET();
  

•   *pending = readl((void __iomem *)MSIP_REG(gd->arch.clint, hart));
  
    return 0;
  

diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c index 17adb35730..fe992eb00f 100644 --- a/arch/riscv/lib/smp.c +++ b/arch/riscv/lib/smp.c @@ -12,38 +12,6 @@

DECLARE_GLOBAL_DATA_PTR;

-/**

• • 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
• */

-extern 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
• */

-extern 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
• */

-extern int riscv_get_ipi(int hart, int *pending);

static int send_ipi_many(struct ipi_data *ipi, int wait) { ofnode node, cpus; @@ -114,7 +82,6 @@ void handle_ipi(ulong hart) return;

    __smp_mb();

•   smp_function = (void (*)(ulong, ulong, ulong))gd->arch.ipi[hart].addr;
    invalidate_icache_all();
  
  

@@ -124,7 +91,13 @@ void handle_ipi(ulong hart) */ ret = riscv_clear_ipi(hart); if (ret) {

•           pr_err("Cannot clear IPI of hart %ld\n", hart);
  

•           pr_err("Cannot clear IPI of hart %ld (error %d)\n", hart, ret);
  

•           return;
  

•   }
  

•   /* Sanity check */
  

•   if (!smp_function) {
  

•           pr_err("smp_function is NULL on hart %ld\n", hart);
            return;
    }
  
  

@@ -133,14 +106,11 @@ void handle_ipi(ulong hart)

int smp_call_function(ulong addr, ulong arg0, ulong arg1, int wait) {

•   int ret = 0;
  

•   struct ipi_data ipi;
  

•   struct ipi_data ipi = {
  

•           .addr = addr,
  

•           .arg0 = arg0,
  

•           .arg1 = arg1,
  

•   };
  
  

•   ipi.addr = addr;
  

•   ipi.arg0 = arg0;
  

•   ipi.arg1 = arg1;
  

•   ret = send_ipi_many(&ipi, wait);
  

•   return ret;
  

•   return send_ipi_many(&ipi, wait);
  

}

2.25.0