[PATCH v5 27/33] riscv: Fix race conditions when initializing IPI

Sean Anderson
2020-03-02T15:43:39Z

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 <seanga2@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