[PATCH v3 3/3] lib: sbi: Clear IPIs before init_warm_startup in non-boot harts

[Evgenii Shatokhin]

On 09.03.2023 19:30, Anup Patel wrote:
> On Mon, Mar 6, 2023 at 2:54 AM Evgenii Shatokhin <e.shatokhin at yadro.com> wrote:
>>
>> Since commit 50d4fde1c5a4 ("lib: Remove redundant sbi_platform_ipi_clear()
>> calls"), the IPI sent from the boot hart in wake_coldboot_harts() is not
>> cleared in the secondary harts until they reach sbi_ipi_init(). However,
>> sbi_hsm_init() and sbi_hsm_hart_wait() are called earlier, so a secondary
>> hart might enter sbi_hsm_hart_wait() with an already pending IPI.
>>
>> sbi_hsm_hart_wait() makes sure the hart leaves the loop only when it is
>> actually ready, so a pending unrelated IPI should not cause safety issues.
>> However, it might be inefficient on certain hardware, because it prevents
>> "wfi" from stalling the hart even if the hardware supports this, making the
>> hart needlessly spin in a "busy-wait" loop.
>>
>> This behaviour can be observed, for example, in a QEMU VM (QEMU 7.2.0) with
>> "-machine virt" running a Linux guest. Inserting delays in
>> sbi_hsm_hart_start() allows reproducing the issue more reliably.
>>
>> The comment in wait_for_coldboot() suggests that the initial IPI is needed
>> in the warm resume path, so let us clear it before init_warm_startup()
>> only.
>>
>> To do this, sbi_ipi_raw_clear() was created similar to sbi_ipi_raw_send().
> 
> As noted before, the sbi_ipi_raw_clear() will not work since secondary
> HARTs might call sbi_ipi_raw_clear() even before platform code has a
> chance to register IPI device ops.

How so? If I understand it correctly, the platform code registers IPI 
device ops and sets the IPI device structures when the boot hart calls 
sbi_platform_ipi_init() with cold_boot=true from sbi_ipi_init().

So, by the time the secondary harts exit wait_for_coldboot(), IPI device 
and ops are already in place and can be used.

sbi_platform_ipi_init() called from those harts with cold_boot == false 
only clears pending IPIs and disables certain interrupts, it looks like.

> 
> I also recollected why we have this WFI and IPI based coldboot wait logic.
> The rationale of using WFI and IPI based coldboot wait logic was to reduce
> bus traffic from secondary HARTs while the primary HART completes the
> coldboot sequence (i.e. init_coldboot()). Clearly, this WFI and IPI based
> coldboot wait logic will do the intended thing if WFI is a NOP.
> 
> I think we can do the following:
> 1) Remove coldboot_lock and coldboot_wait_hmask
> 2) Simplify the loop in wait_for_coldboot() to use
>      cpu_relax_loop(100) (or more) to reduce bus traffic
>      from secondary HARTs
> 3) Simplify wake_coldboot_harts() to only do
>      "__smp_store_release(&coldboot_done, 1);"
> 
> Regards,
> Anup
> 
> diff --git a/include/sbi/riscv_barrier.h b/include/sbi/riscv_barrier.h
> index 1fba8b8..dd8aa79 100644
> --- a/include/sbi/riscv_barrier.h
> +++ b/include/sbi/riscv_barrier.h
> @@ -40,7 +40,19 @@
>   #define smp_wmb()        RISCV_FENCE(w,w)
> 
>   /* CPU relax for busy loop */
> -#define cpu_relax()        asm volatile ("" : : : "memory")
> +#define cpu_relax()        \
> +do {                \
> +    long dummy = 0x1;    \
> +    asm volatile ("div %0, %0, zero" : "=r" (dummy) : : "memory");\
> +} while (0)
> +
> +/* CPU relax loop */
> +#define cpu_relax_loop(__loop_count)    \
> +do {                    \
> +    long i;                \
> +    for (i = 0; i < (__loop_count); i++) \
> +        cpu_relax();        \
> +} while (0)
> 
>   /* clang-format on */
> 

Regards,
Evgenii