On Fri, 2020-02-28 at 16:05 -0500, Sean Anderson wrote: > 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 <sean...@gmail.com> > --- > > Changes in v5: > - New > > arch/riscv/cpu/cpu.c | 9 ++++ > arch/riscv/include/asm/global_data.h | 1 + > 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 | 68 ++++++++-------------------- > 7 files changed, 101 insertions(+), 92 deletions(-) > > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c > index e457f6acbf..a971ec8694 100644 > --- a/arch/riscv/cpu/cpu.c > +++ b/arch/riscv/cpu/cpu.c > @@ -96,6 +96,15 @@ int arch_cpu_init_dm(void) > csr_write(CSR_SATP, 0); > } > > +#ifdef CONFIG_SMP > + ret = riscv_init_ipi(); > + if (ret) > + return ret; > + > + /* Atomically set a flag enabling IPI handling */ > + WRITE_ONCE(gd->arch.ipi_ready, 1); > +#endif > + > return 0; > } > > diff --git a/arch/riscv/include/asm/global_data.h > b/arch/riscv/include/asm/global_data.h > index 7276d9763f..b24f8fd2a7 100644 > --- a/arch/riscv/include/asm/global_data.h > +++ b/arch/riscv/include/asm/global_data.h > @@ -28,6 +28,7 @@ struct arch_global_data { > #endif > #ifdef CONFIG_SMP > struct ipi_data ipi[CONFIG_NR_CPUS]; > + long ipi_ready; /* Set after riscv_init_ipi is called */ > #endif > #ifndef CONFIG_XIP > ulong available_harts; > 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..62c1b2b0ef 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; > +} > +
Please fix the indentation here. > 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..3b1e52e9b2 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; > @@ -110,37 +78,41 @@ void handle_ipi(ulong hart) > int ret; > void (*smp_function)(ulong hart, ulong arg0, ulong arg1); > > - if (hart >= CONFIG_NR_CPUS) > + if (hart >= CONFIG_NR_CPUS || !READ_ONCE(gd->arch.ipi_ready)) > return; > > - __smp_mb(); > - > - smp_function = (void (*)(ulong, ulong, ulong))gd->arch.ipi[hart].addr; > - invalidate_icache_all(); > - Don't move this. It is intended to be run before the IPI is cleared. > /* > * Clear the IPI to acknowledge the request before jumping to the > * requested function. > */ > 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; > } > > + __smp_mb(); > + > + smp_function = (void (*)(ulong, ulong, ulong))gd->arch.ipi[hart].addr; > + /* > + * There may be an IPI raised before u-boot begins execution, so check > + * to ensure we actually have a function to call. > + */ > + if (!smp_function) > + return; > + log_debug("hart = %lu func = %p\n", hart, smp_function); The log messages might be corrupted if multiple harts are calling the log function here. I have not looked into the details so this might not be an issue. In that case it is fine to keep, otherwise please remove it. Thanks, Lukas > + invalidate_icache_all(); > + > smp_function(hart, gd->arch.ipi[hart].arg0, gd->arch.ipi[hart].arg1); > } > > 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); > }