[PATCH 1/1] lib: sbi: relax scoldboot_lock spinlock
Heinrich Schuchardt
xypron.glpk at gmx.de
Fri Aug 14 07:13:22 EDT 2020
On 14.08.20 11:36, Anup Patel wrote:
> On Fri, Aug 14, 2020 at 2:50 PM Anup Patel <Anup.Patel at wdc.com> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> Sent: 14 August 2020 14:46
>>> To: Anup Patel <Anup.Patel at wdc.com>; Damien Le Moal
>>> <Damien.LeMoal at wdc.com>; atishp at atishpatra.org; seanga2 at gmail.com
>>> Cc: Atish Patra <Atish.Patra at wdc.com>; bmeng.cn at gmail.com;
>>> jrtc27 at jrtc27.com; opensbi at lists.infradead.org; merle at hardenedlinux.org
>>> Subject: Re: [PATCH 1/1] lib: sbi: relax scoldboot_lock spinlock
>>>
>>> On 14.08.20 09:03, Anup Patel wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Damien Le Moal <Damien.LeMoal at wdc.com>
>>>>> Sent: 14 August 2020 11:49
>>>>> To: Anup Patel <Anup.Patel at wdc.com>; atishp at atishpatra.org;
>>>>> xypron.glpk at gmx.de; seanga2 at gmail.com
>>>>> Cc: jrtc27 at jrtc27.com; Atish Patra <Atish.Patra at wdc.com>;
>>>>> bmeng.cn at gmail.com; opensbi at lists.infradead.org;
>>>>> merle at hardenedlinux.org
>>>>> Subject: Re: [PATCH 1/1] lib: sbi: relax scoldboot_lock spinlock
>>>>>
>>>>> On 2020/08/14 15:08, Anup Patel wrote:
>>>>>>> The following works for me. However, the sbi_ecall_console_puts()
>>>>>>> call in
>>>>>>> test_main() for the test FW_PAYLOAD does not work. Individual calls
>>>>>>> to
>>>>>>> sbi_ecall_console_putc() do work, but not the full string. It looks
>>>>>>> like the string is broken, so the data section may be broken...
>>>>>>> Digging into
>>>>> that.
>>>>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c index
>>>>>>> a7fb848..9692cd9 100644
>>>>>>> --- a/lib/sbi/sbi_init.c
>>>>>>> +++ b/lib/sbi/sbi_init.c
>>>>>>> @@ -84,69 +84,18 @@ static void sbi_boot_prints(struct sbi_scratch
>>>>>>> *scratch, u32 hartid)
>>>>>>> sbi_hart_pmp_dump(scratch); }
>>>>>>>
>>>>>>> -static spinlock_t coldboot_lock = SPIN_LOCK_INITIALIZER; -static
>>>>>>> unsigned long coldboot_done = 0; -static struct sbi_hartmask
>>>>>>> coldboot_wait_hmask = {
>>>>>>> 0 };
>>>>>>> +static atomic_t coldboot_done = ATOMIC_INITIALIZER(0);
>>>>>>>
>>>>>>> -static void wait_for_coldboot(struct sbi_scratch *scratch, u32
>>>>>>> hartid)
>>>>>>> +static inline void wait_for_coldboot(struct sbi_scratch *scratch,
>>>>>>> +u32
>>>>>>> hartid)
>>>>>>> {
>>>>>>> - unsigned long saved_mie, cmip;
>>>>>>> - const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>>>>>>> -
>>>>>>> - /* Save MIE CSR */
>>>>>>> - saved_mie = csr_read(CSR_MIE);
>>>>>>> -
>>>>>>> - /* Set MSIE bit to receive IPI */
>>>>>>> - csr_set(CSR_MIE, MIP_MSIP);
>>>>>>> -
>>>>>>> - /* Acquire coldboot lock */
>>>>>>> - spin_lock(&coldboot_lock);
>>>>>>> -
>>>>>>> - /* Mark current HART as waiting */
>>>>>>> - sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>>>>>> -
>>>>>>> - /* Wait for coldboot to finish using WFI */
>>>>>>> - while (!coldboot_done) {
>>>>>>> - spin_unlock(&coldboot_lock);
>>>>>>> - do {
>>>>>>> - wfi();
>>>>>>> - cmip = csr_read(CSR_MIP);
>>>>>>> - } while (!(cmip & MIP_MSIP));
>>>>>>> - spin_lock(&coldboot_lock);
>>>>>>> - };
>>>>>>> -
>>>>>>> - /* Unmark current HART as waiting */
>>>>>>> - sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>>>>>>> -
>>>>>>> - /* Release coldboot lock */
>>>>>>> - spin_unlock(&coldboot_lock);
>>>>>>> -
>>>>>>> - /* Restore MIE CSR */
>>>>>>> - csr_write(CSR_MIE, saved_mie);
>>>>>>> -
>>>>>>> - /* Clear current HART IPI */
>>>>>>> - sbi_platform_ipi_clear(plat, hartid);
>>>>>>> + /* Wait for coldboot to finish */
>>>>>>> + while (!atomic_read(&coldboot_done)) {};
>>>
>>>
>>> Atomicity of accesses to coldboot_done is not needed for the cold hart to
>>> signal that it has proceeded far enough to let the warm harts continue.
>>>
>>> As Jessica pointed out in the inter-thread communication we have to make
>>> sure that the compiler optimization does not re-sequence commands
>>> incorrectly. A good description can be found in
>>>
>>> https://www.kernel.org/doc/Documentation/memory-barriers.txt
>>>
>>> The Linux kernel defines
>>>
>>> #deine barrier() __asm__ __volatile__("": : :"memory")
>>>
>>> as a compiler barrier.
>>>
>>> I think we can completely remove the 'coldboot_lock' spin-lock and use
>>> barrier() instead.
>>
>> coldboot_lock protects the coldboot_wait_hmask. We should keep the
>> Coldboot_lock for this bitmask and make coldboot_done atomic.
>>
>> The atomic_read() and atomic_write() are regular read/write with
>> RISC-V barriers.
>>
>
> This is what I am suggesting ....
>
> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index a7fb848..6b58983 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -85,9 +85,10 @@ static void sbi_boot_prints(struct sbi_scratch
> *scratch, u32 hartid)
> }
>
> static spinlock_t coldboot_lock = SPIN_LOCK_INITIALIZER;
> -static unsigned long coldboot_done = 0;
> static struct sbi_hartmask coldboot_wait_hmask = { 0 };
>
> +static atomic_t coldboot_done = ATOMIC_INITIALIZER(0);
> +
> static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
> {
> unsigned long saved_mie, cmip;
> @@ -105,16 +106,20 @@ static void wait_for_coldboot(struct sbi_scratch
> *scratch, u32 hartid)
> /* Mark current HART as waiting */
> sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>
> + /* Release coldboot lock */
> + spin_unlock(&coldboot_lock);
> +
> /* Wait for coldboot to finish using WFI */
> - while (!coldboot_done) {
> - spin_unlock(&coldboot_lock);
> + while (!atomic_read(&coldboot_done)) {
> do {
> wfi();
> cmip = csr_read(CSR_MIP);
> } while (!(cmip & MIP_MSIP));
> - spin_lock(&coldboot_lock);
> };
>
> + /* Acquire coldboot lock */
> + spin_lock(&coldboot_lock);
> +
> /* Unmark current HART as waiting */
> sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>
> @@ -132,12 +137,12 @@ static void wake_coldboot_harts(struct
> sbi_scratch *scratch, u32 hartid)
> {
> const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>
> + /* Mark coldboot done */
> + atomic_write(&coldboot_done, 1);
> +
> /* Acquire coldboot lock */
> spin_lock(&coldboot_lock);
>
> - /* Mark coldboot done */
> - coldboot_done = 1;
> -
> /* Send an IPI to all HARTs waiting for coldboot */
> for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
> if ((i != hartid) &&
>
This works fine on the MaixDuino (Kendryte K210)
Tested-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>
> Regards,
> Anup
>
More information about the opensbi
mailing list