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

[Evgenii Shatokhin]

On 28.02.2023 07:41, Anup Patel wrote:
> On Wed, Feb 22, 2023 at 2:39 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().
>>
>> Signed-off-by: Evgenii Shatokhin <e.shatokhin at yadro.com>
>> ---
>>   include/sbi/sbi_ipi.h | 2 ++
>>   lib/sbi/sbi_init.c    | 6 ++++--
>>   lib/sbi/sbi_ipi.c     | 6 ++++++
>>   3 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sbi/sbi_ipi.h b/include/sbi/sbi_ipi.h
>> index f6ac807..f384e74 100644
>> --- a/include/sbi/sbi_ipi.h
>> +++ b/include/sbi/sbi_ipi.h
>> @@ -77,6 +77,8 @@ void sbi_ipi_process(void);
>>
>>   int sbi_ipi_raw_send(u32 target_hart);
>>
>> +void sbi_ipi_raw_clear(u32 target_hart);
>> +
>>   const struct sbi_ipi_device *sbi_ipi_get_device(void);
>>
>>   void sbi_ipi_set_device(const struct sbi_ipi_device *dev);
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index e353c33..a9a4084 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -438,10 +438,12 @@ static void __noreturn init_warmboot(struct sbi_scratch *scratch, u32 hartid)
>>          if (hstate < 0)
>>                  sbi_hart_hang();
>>
>> -       if (hstate == SBI_HSM_STATE_SUSPENDED)
>> +       if (hstate == SBI_HSM_STATE_SUSPENDED) {
>>                  init_warm_resume(scratch);
>> -       else
>> +       } else {
>> +               sbi_ipi_raw_clear(hartid);
> 
> This effectively does nothing in many cases because for a lot of
> platforms all HARTs enter OpenSBI at the same time and the HARTs
> entering warm boot path will call sbi_ipi_raw_clear() even before
> ipi_dev is provided by platform code.

Right, it would be a no-op then.

However, this is not the case when Linux 6.x boots, e.g. in QEMU with 
"-machine virt" or with other platforms which provide "cold" IPI init 
callbacks.

Here, the boot hart initializes ipi_dev early. For example, in QEMU with 
"-machine virt", it does the following:
init_coldboot() => sbi_ipi_init() => ... => aclint_mswi_cold_init().

Other harts are waiting in sbi_hsm_init() at the moment and will only 
call sbi_ipi_init() after they leave sbi_hsm_hart_wait(), etc.

While they are waiting, their previous IPI remains pending. It can be 
easily demonstrated if one checks the relevant bits of CSR_MIP in 
sbi_hsm_hart_wait() right before the wfi loop. A delay in 
sbi_hsm_hart_start(), same as for the first issue, also helps make it 
visible.

csr_read(CSR_MIP) & (MIP_MSIP | MIP_MEIP) == 0x8 in my case at that 
point. That is, MIP_MSIP bit is set even before the boot hart has send 
an IPI to the target hart in sbi_hsm_hart_start(). Not good.

So, sbi_ipi_raw_clear(hartid) really helps in such cases, even if it is 
a no-op in other situations.

> 
>>                  init_warm_startup(scratch, hartid);
>> +       }
>>
>>          sbi_hart_switch_mode(hartid, scratch->next_arg1,
>>                               scratch->next_addr,
>> diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c
>> index 1bcc2e4..b9f6205 100644
>> --- a/lib/sbi/sbi_ipi.c
>> +++ b/lib/sbi/sbi_ipi.c
>> @@ -217,6 +217,12 @@ int sbi_ipi_raw_send(u32 target_hart)
>>          return 0;
>>   }
>>
>> +void sbi_ipi_raw_clear(u32 target_hart)
>> +{
>> +       if (ipi_dev && ipi_dev->ipi_clear)
>> +               ipi_dev->ipi_clear(target_hart);
>> +}
>> +
>>   const struct sbi_ipi_device *sbi_ipi_get_device(void)
>>   {
>>          return ipi_dev;
>> --
>> 2.34.1
>>
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
> 
> Regards,
> Anup
> 

Regards,
Evgenii