On 3/2/20 4:08 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 <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); > > I think it shall not have race condition here. > Can you explain more detail why there will occur race condition ? > > Hi Lukas > > Do you have any comments ? > > Thanks > Rick
On the K210, there may already be an IPI pending when U-Boot starts. (Perhaps the prior stage sends an IPI but does not clear it). As soon as interrupts are enabled, the hart then tries to call riscv_clear_ipi(). Because the clint/plic has not yet been enabled, the clear_ipi function will try and bind/probe the device. This can have really nasty effects, since the boot hart is *also* trying to bind/probe devices. In addition, a hart could end up trying to probe the clint/plic because it could receive the IPI before (from its perspective) gd->arch.clint (or plic) gets initialized. Aside from the above, 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. --Sean
