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

[Anup Patel]

On Thu, Mar 9, 2023 at 11:23 PM Evgenii Shatokhin <e.shatokhin at yadro.com> wrote:
>
> 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().

sbi_ipi_raw_clear() added by this patch is called from secondary HART
whereas platform code registers IPI device ops 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.

The sbi_platform_ipi_init() is called via init_warm_startup() only after
cold boot sequence is completed by primary hart but over here you
are calling sbi_ipi_raw_clear() before init_warm_startup().

Regards,
Anup

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