[PATCH v6 13/19] riscv: Fix race conditions when initializing IPI

Rick Chen rickchen36 at gmail.com
Tue Mar 10 09:20:48 CET 2020


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


More information about the U-Boot mailing list