Hi Sean > On Mon, 2020-03-02 at 10:43 -0500, Sean Anderson wrote: > > > 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. > > > > We did not have a problem with pending IPIs on other platforms. It > should suffice to clear SSIP / MSIP before enabling the interrupts. >
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. Thanks, Rick > > 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. > > > > I agree, the patch makes this more clear and helps make the code more > robust. > > Thanks, > Lukas
