[1/1] lib: sbi: relax scoldboot_lock spinlock

Message ID	20200813162415.124469-1-xypron.glpk@gmx.de
State	Superseded
Headers	show Return-Path: <opensbi-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org> From: Heinrich Schuchardt <xypron.glpk@gmx.de> To: Anup Patel <anup.patel@wdc.com>, Atish Patra <atish.patra@wdc.com>, Bin Meng <bmeng.cn@gmail.com>, Xiang Wang <merle@hardenedlinux.org> Subject: [PATCH 1/1] lib: sbi: relax scoldboot_lock spinlock Date: Thu, 13 Aug 2020 18:24:15 +0200 Message-Id: <20200813162415.124469-1-xypron.glpk@gmx.de> MIME-Version: 1.0 summary: Content analysis details: (-0.1 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [212.227.15.18 listed in wl.mailspike.net] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider [xypron.glpk[at]gmx.de] -0.0 SPF_PASS SPF: sender matches SPF record 2.5 SORTED_RECIPS Recipient list is sorted by address -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at https://www.dnswl.org/, low trust [212.227.15.18 listed in list.dnswl.org] Precedence: list Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>, opensbi@lists.infradead.org, Sean Anderson <seanga2@gmail.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "opensbi" <opensbi-bounces@lists.infradead.org> Errors-To: opensbi-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org
Series	[1/1] lib: sbi: relax scoldboot_lock spinlock \| expand [1/1] lib: sbi: relax scoldboot_lock spinlock

Heinrich Schuchardt Aug. 13, 2020, 4:24 p.m. UTC

OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
when trying to boot U-Boot as FW_PAYLOAD.

Even if reading and writing coldboot_done is not atomic it is safe to check
it without using a spinlock.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/sbi/sbi_init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
2.28.0

Jessica Clarke Aug. 13, 2020, 4:36 p.m. UTC | #1

On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
> when trying to boot U-Boot as FW_PAYLOAD.

This smells of there being a more fundamental issue that's not being
addressed which this manages to work around, either reliably or by
chance. What is the actual deadlock condition you observe?

> Even if reading and writing coldboot_done is not atomic it is safe to check
> it without using a spinlock.

Maybe microarchitecturally on the specific hardware in question (though
I wouldn't even be so certain about that), but definitely not in C. You
now have a data race in reading coldboot_done, which isn't even
volatile^, so a sufficiently smart compiler would be completely within
its right to do whatever it wants to the code, including things like
delete the loop entirely (or, slightly less dramatic, turn it into an
if). This patch introduces undefined behaviour.

Jess

^ Please do not make it volatile as a workaround, that is not the right
  way to "fix" such issues. 

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> lib/sbi/sbi_init.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index a7fb848..a0e4f11 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
> 	sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
> 
> 	/* Wait for coldboot to finish using WFI */
> +	spin_unlock(&coldboot_lock);
> 	while (!coldboot_done) {
> -		spin_unlock(&coldboot_lock);
> 		do {
> 			wfi();
> 			cmip = csr_read(CSR_MIP);
> 		 } while (!(cmip & MIP_MSIP));
> -		spin_lock(&coldboot_lock);
> 	};
> +	spin_lock(&coldboot_lock);
> 
> 	/* Unmark current HART as waiting */
> 	sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
> --
> 2.28.0
> 
> 
> -- 
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Heinrich Schuchardt Aug. 13, 2020, 4:51 p.m. UTC | #2

On 13.08.20 18:36, Jessica Clarke wrote:
> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>> when trying to boot U-Boot as FW_PAYLOAD.
>
> This smells of there being a more fundamental issue that's not being
> addressed which this manages to work around, either reliably or by
> chance. What is the actual deadlock condition you observe?

There are only two function using coldboot_lock spinlock:

* wake_coldboot_harts()
* wait_for_coldboot()

wake_coldboot_harts() was entered but never left without patching one of
the functions.

The Kendryte K210 has two harts. One goes the cold boot path while the
other is sent to the warm boot path.

>
>> Even if reading and writing coldboot_done is not atomic it is safe to check
>> it without using a spinlock.
>
> Maybe microarchitecturally on the specific hardware in question (though
> I wouldn't even be so certain about that), but definitely not in C. You
> now have a data race in reading coldboot_done, which isn't even
> volatile^, so a sufficiently smart compiler would be completely within
> its right to do whatever it wants to the code, including things like
> delete the loop entirely (or, slightly less dramatic, turn it into an
> if). This patch introduces undefined behaviour.
>
> Jess
>
> ^ Please do not make it volatile as a workaround, that is not the right
>   way to "fix" such issues.

What do you suggest Jessica?

Best regards

Heinrich

>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> lib/sbi/sbi_init.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index a7fb848..a0e4f11 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>> 	sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>
>> 	/* Wait for coldboot to finish using WFI */
>> +	spin_unlock(&coldboot_lock);
>> 	while (!coldboot_done) {
>> -		spin_unlock(&coldboot_lock);
>> 		do {
>> 			wfi();
>> 			cmip = csr_read(CSR_MIP);
>> 		 } while (!(cmip & MIP_MSIP));
>> -		spin_lock(&coldboot_lock);
>> 	};
>> +	spin_lock(&coldboot_lock);
>>
>> 	/* Unmark current HART as waiting */
>> 	sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>> --
>> 2.28.0
>>
>>
>> --
>> opensbi mailing list
>> opensbi@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi

Jessica Clarke Aug. 13, 2020, 5:03 p.m. UTC | #3

On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 13.08.20 18:36, Jessica Clarke wrote:
>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> 
>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>> when trying to boot U-Boot as FW_PAYLOAD.
>> 
>> This smells of there being a more fundamental issue that's not being
>> addressed which this manages to work around, either reliably or by
>> chance. What is the actual deadlock condition you observe?
> 
> There are only two function using coldboot_lock spinlock:
> 
> * wake_coldboot_harts()
> * wait_for_coldboot()
> 
> wake_coldboot_harts() was entered but never left without patching one of
> the functions.
> 
> The Kendryte K210 has two harts. One goes the cold boot path while the
> other is sent to the warm boot path.

The K210's platform definition uses clint_ipi_send, which as far as I
can tell is entirely non-blocking. The only thing wake_coldboot_harts
does with the lock held is set coldboot_done to 1 and send an IPI to
all the other harts. Thus wake_coldboot_harts should never be able to
block holding the spin lock, meaning wait_for_coldboot should
eventually wake up and be able to acquire the lock (on platforms that
implement WFI fully it shouldn't have to wait long for the lock, though
on platforms that make it a NOP there might be a bit of .

Please further investigate (or explain if you already know) exactly how
you end up in a deadlock situation between wake_coldboot_harts and
wait_for_coldboot, as I cannot currently see how it could ever be
possible.

>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>> it without using a spinlock.
>> 
>> Maybe microarchitecturally on the specific hardware in question (though
>> I wouldn't even be so certain about that), but definitely not in C. You
>> now have a data race in reading coldboot_done, which isn't even
>> volatile^, so a sufficiently smart compiler would be completely within
>> its right to do whatever it wants to the code, including things like
>> delete the loop entirely (or, slightly less dramatic, turn it into an
>> if). This patch introduces undefined behaviour.
>> 
>> Jess
>> 
>> ^ Please do not make it volatile as a workaround, that is not the right
>>  way to "fix" such issues.
> 
> What do you suggest Jessica?

My primary suggestion is above, since this does not look like a
necessary fix.

There is the separate question of whether this code should be using
spin locks anyway. One could optimise it with suitable use of proper
atomics to access coldboot_done, which would mask whatever issue you
are seeing, but that should not be necessary, and in early boot code
like this that's not on a hot code path it's better to keep things
simple like it is currently rather than risk writing unsafe code
through incorrect use of atomics. I would feel much more comfortable
leaving the code using spin locks.

Jess

>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>> lib/sbi/sbi_init.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>> index a7fb848..a0e4f11 100644
>>> --- a/lib/sbi/sbi_init.c
>>> +++ b/lib/sbi/sbi_init.c
>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>>> 	sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>> 
>>> 	/* Wait for coldboot to finish using WFI */
>>> +	spin_unlock(&coldboot_lock);
>>> 	while (!coldboot_done) {
>>> -		spin_unlock(&coldboot_lock);
>>> 		do {
>>> 			wfi();
>>> 			cmip = csr_read(CSR_MIP);
>>> 		 } while (!(cmip & MIP_MSIP));
>>> -		spin_lock(&coldboot_lock);
>>> 	};
>>> +	spin_lock(&coldboot_lock);
>>> 
>>> 	/* Unmark current HART as waiting */
>>> 	sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>>> --
>>> 2.28.0
>>> 
>>> 
>>> --
>>> opensbi mailing list
>>> opensbi@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/opensbi

Atish Patra Aug. 13, 2020, 5:45 p.m. UTC | #4

On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > On 13.08.20 18:36, Jessica Clarke wrote:
> >> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>
> >>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
> >>> when trying to boot U-Boot as FW_PAYLOAD.
> >>
> >> This smells of there being a more fundamental issue that's not being
> >> addressed which this manages to work around, either reliably or by
> >> chance. What is the actual deadlock condition you observe?
> >
> > There are only two function using coldboot_lock spinlock:
> >
> > * wake_coldboot_harts()
> > * wait_for_coldboot()
> >
> > wake_coldboot_harts() was entered but never left without patching one of
> > the functions.
> >
> > The Kendryte K210 has two harts. One goes the cold boot path while the
> > other is sent to the warm boot path.
>
> The K210's platform definition uses clint_ipi_send, which as far as I
> can tell is entirely non-blocking. The only thing wake_coldboot_harts
> does with the lock held is set coldboot_done to 1 and send an IPI to
> all the other harts. Thus wake_coldboot_harts should never be able to
> block holding the spin lock, meaning wait_for_coldboot should
> eventually wake up and be able to acquire the lock (on platforms that
> implement WFI fully it shouldn't have to wait long for the lock, though
> on platforms that make it a NOP there might be a bit of .
>

Yes. If wake_coldboot_harts is never able to acquire the spinlock,
that indicates spinlock
is always acquired by wait_for_coldboot. That can happen if

1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
is set which
shouldn't happen unless there is an IPI sent.

Can you try clearing the MIP just before acquiring the lock ? In qemu
& unleashed, modifying the MSIP bit in
MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.

> Please further investigate (or explain if you already know) exactly how
> you end up in a deadlock situation between wake_coldboot_harts and
> wait_for_coldboot, as I cannot currently see how it could ever be
> possible.
>
> >>> Even if reading and writing coldboot_done is not atomic it is safe to check
> >>> it without using a spinlock.
> >>
> >> Maybe microarchitecturally on the specific hardware in question (though
> >> I wouldn't even be so certain about that), but definitely not in C. You
> >> now have a data race in reading coldboot_done, which isn't even
> >> volatile^, so a sufficiently smart compiler would be completely within
> >> its right to do whatever it wants to the code, including things like
> >> delete the loop entirely (or, slightly less dramatic, turn it into an
> >> if). This patch introduces undefined behaviour.
> >>
> >> Jess
> >>
> >> ^ Please do not make it volatile as a workaround, that is not the right
> >>  way to "fix" such issues.
> >
> > What do you suggest Jessica?
>
> My primary suggestion is above, since this does not look like a
> necessary fix.
>
> There is the separate question of whether this code should be using
> spin locks anyway. One could optimise it with suitable use of proper
> atomics to access coldboot_done, which would mask whatever issue you
> are seeing, but that should not be necessary, and in early boot code
> like this that's not on a hot code path it's better to keep things
> simple like it is currently rather than risk writing unsafe code
> through incorrect use of atomics. I would feel much more comfortable
> leaving the code using spin locks.
>
> Jess
>
> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> ---
> >>> lib/sbi/sbi_init.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> >>> index a7fb848..a0e4f11 100644
> >>> --- a/lib/sbi/sbi_init.c
> >>> +++ b/lib/sbi/sbi_init.c
> >>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
> >>>     sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
> >>>
> >>>     /* Wait for coldboot to finish using WFI */
> >>> +   spin_unlock(&coldboot_lock);
> >>>     while (!coldboot_done) {
> >>> -           spin_unlock(&coldboot_lock);
> >>>             do {
> >>>                     wfi();
> >>>                     cmip = csr_read(CSR_MIP);
> >>>              } while (!(cmip & MIP_MSIP));
> >>> -           spin_lock(&coldboot_lock);
> >>>     };
> >>> +   spin_lock(&coldboot_lock);
> >>>
> >>>     /* Unmark current HART as waiting */
> >>>     sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
> >>> --
> >>> 2.28.0
> >>>
> >>>
> >>> --
> >>> opensbi mailing list
> >>> opensbi@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/opensbi
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi



--
Regards,
Atish

Sean Anderson Aug. 13, 2020, 5:56 p.m. UTC | #5

On 8/13/20 1:45 PM, Atish Patra wrote:
> On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>
>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>
>>>> This smells of there being a more fundamental issue that's not being
>>>> addressed which this manages to work around, either reliably or by
>>>> chance. What is the actual deadlock condition you observe?
>>>
>>> There are only two function using coldboot_lock spinlock:
>>>
>>> * wake_coldboot_harts()
>>> * wait_for_coldboot()
>>>
>>> wake_coldboot_harts() was entered but never left without patching one of
>>> the functions.
>>>
>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>> other is sent to the warm boot path.
>>
>> The K210's platform definition uses clint_ipi_send, which as far as I
>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>> does with the lock held is set coldboot_done to 1 and send an IPI to
>> all the other harts. Thus wake_coldboot_harts should never be able to
>> block holding the spin lock, meaning wait_for_coldboot should
>> eventually wake up and be able to acquire the lock (on platforms that
>> implement WFI fully it shouldn't have to wait long for the lock, though
>> on platforms that make it a NOP there might be a bit of .
>>
> 
> Yes. If wake_coldboot_harts is never able to acquire the spinlock,
> that indicates spinlock
> is always acquired by wait_for_coldboot. That can happen if
> 
> 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
> is set which
> shouldn't happen unless there is an IPI sent.
>
> Can you try clearing the MIP just before acquiring the lock ? In qemu
> & unleashed, modifying the MSIP bit in
> MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.

I second this suggestion. I had a similar problem when porting U-Boot.

https://gitlab.denx.de/u-boot/u-boot/-/commit/9472630337e7c4ac442066b5a752aaa8c3b4d4a6

--Sean

Heinrich Schuchardt Aug. 13, 2020, 6:03 p.m. UTC | #6

On 13.08.20 19:03, Jessica Clarke wrote:
> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 13.08.20 18:36, Jessica Clarke wrote:
>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>
>>> This smells of there being a more fundamental issue that's not being
>>> addressed which this manages to work around, either reliably or by
>>> chance. What is the actual deadlock condition you observe?
>>
>> There are only two function using coldboot_lock spinlock:
>>
>> * wake_coldboot_harts()
>> * wait_for_coldboot()
>>
>> wake_coldboot_harts() was entered but never left without patching one of
>> the functions.
>>
>> The Kendryte K210 has two harts. One goes the cold boot path while the
>> other is sent to the warm boot path.
>
> The K210's platform definition uses clint_ipi_send, which as far as I
> can tell is entirely non-blocking. The only thing wake_coldboot_harts
> does with the lock held is set coldboot_done to 1 and send an IPI to
> all the other harts. Thus wake_coldboot_harts should never be able to
> block holding the spin lock, meaning wait_for_coldboot should
> eventually wake up and be able to acquire the lock (on platforms that
> implement WFI fully it shouldn't have to wait long for the lock, though
> on platforms that make it a NOP there might be a bit of .
>
> Please further investigate (or explain if you already know) exactly how
> you end up in a deadlock situation between wake_coldboot_harts and
> wait_for_coldboot, as I cannot currently see how it could ever be
> possible.
>
>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>> it without using a spinlock.
>>>
>>> Maybe microarchitecturally on the specific hardware in question (though
>>> I wouldn't even be so certain about that), but definitely not in C. You
>>> now have a data race in reading coldboot_done, which isn't even
>>> volatile^, so a sufficiently smart compiler would be completely within
>>> its right to do whatever it wants to the code, including things like
>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>> if). This patch introduces undefined behaviour.

The cold boot path prints out over the serial console so it is the slow
path. When it arrives at wake_coldboot_harts() the spinlock is already
held by wait_for_coldboot().

You have two loops running at the same time:

A loop in wait_for_coldboot() which switches the lock on and off
repeatedly and a second loop in wake_coldboot_harts' spin_lock().

The warm hart releases the spin lock. This is detected by the cold hart
in spin_lock_check(). In the meantime the warm hart locks the spinlock
again and the cold hart fails in amoswap.w. This can go forever.

It is only by different relative speed of the loops if other processor
got out of this situation.

A loop as in wait_for_coldboot() reaquiring a lock immediately without
giving sufficient time for other threads to acquire the lock is simply
incorrect coding.

The following change also resolves the problem:

diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index a7fb848..68be58f 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -132,12 +132,12 @@ static void wake_coldboot_harts(struct sbi_scratch
*scratch, u32 hartid)
 {
        const struct sbi_platform *plat = sbi_platform_ptr(scratch);

-       /* Acquire coldboot lock */
-       spin_lock(&coldboot_lock);
-
        /* Mark coldboot done */
        coldboot_done = 1;

+       /* Acquire coldboot lock */
+       spin_lock(&coldboot_lock);
+
        /* Send an IPI to all HARTs waiting for coldboot */
        for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
                if ((i != hartid) &&

Best regards

Heinrich

>>>
>>> Jess
>>>
>>> ^ Please do not make it volatile as a workaround, that is not the right
>>>  way to "fix" such issues.
>>
>> What do you suggest Jessica?
>
> My primary suggestion is above, since this does not look like a
> necessary fix.
>
> There is the separate question of whether this code should be using
> spin locks anyway. One could optimise it with suitable use of proper
> atomics to access coldboot_done, which would mask whatever issue you
> are seeing, but that should not be necessary, and in early boot code
> like this that's not on a hot code path it's better to keep things
> simple like it is currently rather than risk writing unsafe code
> through incorrect use of atomics. I would feel much more comfortable
> leaving the code using spin locks.
>
> Jess
>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>> lib/sbi/sbi_init.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>> index a7fb848..a0e4f11 100644
>>>> --- a/lib/sbi/sbi_init.c
>>>> +++ b/lib/sbi/sbi_init.c
>>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>>>> 	sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>>>
>>>> 	/* Wait for coldboot to finish using WFI */
>>>> +	spin_unlock(&coldboot_lock);
>>>> 	while (!coldboot_done) {
>>>> -		spin_unlock(&coldboot_lock);
>>>> 		do {
>>>> 			wfi();
>>>> 			cmip = csr_read(CSR_MIP);
>>>> 		 } while (!(cmip & MIP_MSIP));
>>>> -		spin_lock(&coldboot_lock);
>>>> 	};
>>>> +	spin_lock(&coldboot_lock);
>>>>
>>>> 	/* Unmark current HART as waiting */
>>>> 	sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>>>> --
>>>> 2.28.0
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>

Jessica Clarke Aug. 13, 2020, 6:11 p.m. UTC | #7

On 13 Aug 2020, at 19:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 13.08.20 19:03, Jessica Clarke wrote:
>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> 
>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>> 
>>>> This smells of there being a more fundamental issue that's not being
>>>> addressed which this manages to work around, either reliably or by
>>>> chance. What is the actual deadlock condition you observe?
>>> 
>>> There are only two function using coldboot_lock spinlock:
>>> 
>>> * wake_coldboot_harts()
>>> * wait_for_coldboot()
>>> 
>>> wake_coldboot_harts() was entered but never left without patching one of
>>> the functions.
>>> 
>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>> other is sent to the warm boot path.
>> 
>> The K210's platform definition uses clint_ipi_send, which as far as I
>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>> does with the lock held is set coldboot_done to 1 and send an IPI to
>> all the other harts. Thus wake_coldboot_harts should never be able to
>> block holding the spin lock, meaning wait_for_coldboot should
>> eventually wake up and be able to acquire the lock (on platforms that
>> implement WFI fully it shouldn't have to wait long for the lock, though
>> on platforms that make it a NOP there might be a bit of .
>> 
>> Please further investigate (or explain if you already know) exactly how
>> you end up in a deadlock situation between wake_coldboot_harts and
>> wait_for_coldboot, as I cannot currently see how it could ever be
>> possible.
>> 
>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>> it without using a spinlock.
>>>> 
>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>> now have a data race in reading coldboot_done, which isn't even
>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>> its right to do whatever it wants to the code, including things like
>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>> if). This patch introduces undefined behaviour.
> 
> The cold boot path prints out over the serial console so it is the slow
> path. When it arrives at wake_coldboot_harts() the spinlock is already
> held by wait_for_coldboot().
> 
> You have two loops running at the same time:
> 
> A loop in wait_for_coldboot() which switches the lock on and off
> repeatedly and a second loop in wake_coldboot_harts' spin_lock().
> 
> The warm hart releases the spin lock. This is detected by the cold hart
> in spin_lock_check(). In the meantime the warm hart locks the spinlock
> again and the cold hart fails in amoswap.w. This can go forever.
> 
> It is only by different relative speed of the loops if other processor
> got out of this situation.
> 
> A loop as in wait_for_coldboot() reaquiring a lock immediately without
> giving sufficient time for other threads to acquire the lock is simply
> incorrect coding.
> 
> The following change also resolves the problem:
> 
> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index a7fb848..68be58f 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -132,12 +132,12 @@ static void wake_coldboot_harts(struct sbi_scratch
> *scratch, u32 hartid)
> {
>        const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> 
> -       /* Acquire coldboot lock */
> -       spin_lock(&coldboot_lock);
> -
>        /* Mark coldboot done */
>        coldboot_done = 1;
> 
> +       /* Acquire coldboot lock */
> +       spin_lock(&coldboot_lock);
> +
>        /* Send an IPI to all HARTs waiting for coldboot */
>        for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
>                if ((i != hartid) &&

You don't want that; that causes the same kinds of data races for
coldboot_done as before. What could be done is to instead release
coldboot_lock earlier, provided it doesn't matter that the IPIs haven't
been sent yet (I don't think it does, because they're already
inherently asynchronous, so there's currently no guarantee that the
IPIs have actually occurred when coldboot_lock is unlocked in its
original location), but that won't fix anything for you if the issue is
acquiring the lock in the first place in wake_coldboot_harts.

So, ok, your problem is not deadlock but livelock. As Atish has
suggested, please try clearing MIP at some point during boot before
wait_for_coldboot, as it really should not be repeatedly trying to
acquire the lock.

Jess

Anup Patel Aug. 13, 2020, 6:17 p.m. UTC | #8

> -----Original Message-----
> From: Jessica Clarke <jrtc27@jrtc27.com>
> Sent: 13 August 2020 23:42
> To: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Anup Patel <Anup.Patel@wdc.com>; Atish Patra
> <Atish.Patra@wdc.com>; Bin Meng <bmeng.cn@gmail.com>; Xiang Wang
> <merle@hardenedlinux.org>; opensbi@lists.infradead.org; Sean Anderson
> <seanga2@gmail.com>
> Subject: Re: [PATCH 1/1] lib: sbi: relax scoldboot_lock spinlock
> 
> On 13 Aug 2020, at 19:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > On 13.08.20 19:03, Jessica Clarke wrote:
> >> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de>
> wrote:
> >>> On 13.08.20 18:36, Jessica Clarke wrote:
> >>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de>
> wrote:
> >>>>>
> >>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the
> >>>>> MaixDuino board when trying to boot U-Boot as FW_PAYLOAD.
> >>>>
> >>>> This smells of there being a more fundamental issue that's not
> >>>> being addressed which this manages to work around, either reliably
> >>>> or by chance. What is the actual deadlock condition you observe?
> >>>
> >>> There are only two function using coldboot_lock spinlock:
> >>>
> >>> * wake_coldboot_harts()
> >>> * wait_for_coldboot()
> >>>
> >>> wake_coldboot_harts() was entered but never left without patching
> >>> one of the functions.
> >>>
> >>> The Kendryte K210 has two harts. One goes the cold boot path while
> >>> the other is sent to the warm boot path.
> >>
> >> The K210's platform definition uses clint_ipi_send, which as far as I
> >> can tell is entirely non-blocking. The only thing wake_coldboot_harts
> >> does with the lock held is set coldboot_done to 1 and send an IPI to
> >> all the other harts. Thus wake_coldboot_harts should never be able to
> >> block holding the spin lock, meaning wait_for_coldboot should
> >> eventually wake up and be able to acquire the lock (on platforms that
> >> implement WFI fully it shouldn't have to wait long for the lock,
> >> though on platforms that make it a NOP there might be a bit of .
> >>
> >> Please further investigate (or explain if you already know) exactly
> >> how you end up in a deadlock situation between wake_coldboot_harts
> >> and wait_for_coldboot, as I cannot currently see how it could ever be
> >> possible.
> >>
> >>>>> Even if reading and writing coldboot_done is not atomic it is safe
> >>>>> to check it without using a spinlock.
> >>>>
> >>>> Maybe microarchitecturally on the specific hardware in question
> >>>> (though I wouldn't even be so certain about that), but definitely
> >>>> not in C. You now have a data race in reading coldboot_done, which
> >>>> isn't even volatile^, so a sufficiently smart compiler would be
> >>>> completely within its right to do whatever it wants to the code,
> >>>> including things like delete the loop entirely (or, slightly less
> >>>> dramatic, turn it into an if). This patch introduces undefined behaviour.
> >
> > The cold boot path prints out over the serial console so it is the
> > slow path. When it arrives at wake_coldboot_harts() the spinlock is
> > already held by wait_for_coldboot().
> >
> > You have two loops running at the same time:
> >
> > A loop in wait_for_coldboot() which switches the lock on and off
> > repeatedly and a second loop in wake_coldboot_harts' spin_lock().
> >
> > The warm hart releases the spin lock. This is detected by the cold
> > hart in spin_lock_check(). In the meantime the warm hart locks the
> > spinlock again and the cold hart fails in amoswap.w. This can go forever.
> >
> > It is only by different relative speed of the loops if other processor
> > got out of this situation.
> >
> > A loop as in wait_for_coldboot() reaquiring a lock immediately without
> > giving sufficient time for other threads to acquire the lock is simply
> > incorrect coding.
> >
> > The following change also resolves the problem:
> >
> > diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c index
> > a7fb848..68be58f 100644
> > --- a/lib/sbi/sbi_init.c
> > +++ b/lib/sbi/sbi_init.c
> > @@ -132,12 +132,12 @@ static void wake_coldboot_harts(struct
> > sbi_scratch *scratch, u32 hartid) {
> >        const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> >
> > -       /* Acquire coldboot lock */
> > -       spin_lock(&coldboot_lock);
> > -
> >        /* Mark coldboot done */
> >        coldboot_done = 1;
> >
> > +       /* Acquire coldboot lock */
> > +       spin_lock(&coldboot_lock);
> > +
> >        /* Send an IPI to all HARTs waiting for coldboot */
> >        for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
> >                if ((i != hartid) &&
> 
> You don't want that; that causes the same kinds of data races for
> coldboot_done as before. What could be done is to instead release
> coldboot_lock earlier, provided it doesn't matter that the IPIs haven't been
> sent yet (I don't think it does, because they're already inherently
> asynchronous, so there's currently no guarantee that the IPIs have actually
> occurred when coldboot_lock is unlocked in its original location), but that
> won't fix anything for you if the issue is acquiring the lock in the first place in
> wake_coldboot_harts.
> 
> So, ok, your problem is not deadlock but livelock. As Atish has suggested,
> please try clearing MIP at some point during boot before wait_for_coldboot,
> as it really should not be repeatedly trying to acquire the lock.

I suggest we:
1. Convert coldboot_done into atomic variable don't use coldboot_lock for it
2. Use coldboot_lock only to protect coldboot_wait_hmask

This way we will not have cold boot path starve for coldboot_lock.

Another suggestion is to ticket based spinlocks in OpenSBI.

Regards,
Anup

Heinrich Schuchardt Aug. 13, 2020, 6:31 p.m. UTC | #9

On 13.08.20 20:11, Jessica Clarke wrote:
> On 13 Aug 2020, at 19:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 13.08.20 19:03, Jessica Clarke wrote:
>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>>
>>>>> This smells of there being a more fundamental issue that's not being
>>>>> addressed which this manages to work around, either reliably or by
>>>>> chance. What is the actual deadlock condition you observe?
>>>>
>>>> There are only two function using coldboot_lock spinlock:
>>>>
>>>> * wake_coldboot_harts()
>>>> * wait_for_coldboot()
>>>>
>>>> wake_coldboot_harts() was entered but never left without patching one of
>>>> the functions.
>>>>
>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>>> other is sent to the warm boot path.
>>>
>>> The K210's platform definition uses clint_ipi_send, which as far as I
>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>>> does with the lock held is set coldboot_done to 1 and send an IPI to
>>> all the other harts. Thus wake_coldboot_harts should never be able to
>>> block holding the spin lock, meaning wait_for_coldboot should
>>> eventually wake up and be able to acquire the lock (on platforms that
>>> implement WFI fully it shouldn't have to wait long for the lock, though
>>> on platforms that make it a NOP there might be a bit of .
>>>
>>> Please further investigate (or explain if you already know) exactly how
>>> you end up in a deadlock situation between wake_coldboot_harts and
>>> wait_for_coldboot, as I cannot currently see how it could ever be
>>> possible.
>>>
>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>>> it without using a spinlock.
>>>>>
>>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>>> now have a data race in reading coldboot_done, which isn't even
>>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>>> its right to do whatever it wants to the code, including things like
>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>>> if). This patch introduces undefined behaviour.
>>
>> The cold boot path prints out over the serial console so it is the slow
>> path. When it arrives at wake_coldboot_harts() the spinlock is already
>> held by wait_for_coldboot().
>>
>> You have two loops running at the same time:
>>
>> A loop in wait_for_coldboot() which switches the lock on and off
>> repeatedly and a second loop in wake_coldboot_harts' spin_lock().
>>
>> The warm hart releases the spin lock. This is detected by the cold hart
>> in spin_lock_check(). In the meantime the warm hart locks the spinlock
>> again and the cold hart fails in amoswap.w. This can go forever.
>>
>> It is only by different relative speed of the loops if other processor
>> got out of this situation.
>>
>> A loop as in wait_for_coldboot() reaquiring a lock immediately without
>> giving sufficient time for other threads to acquire the lock is simply
>> incorrect coding.
>>
>> The following change also resolves the problem:
>>
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index a7fb848..68be58f 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -132,12 +132,12 @@ static void wake_coldboot_harts(struct sbi_scratch
>> *scratch, u32 hartid)
>> {
>>        const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>>
>> -       /* Acquire coldboot lock */
>> -       spin_lock(&coldboot_lock);
>> -
>>        /* Mark coldboot done */
>>        coldboot_done = 1;
>>
>> +       /* Acquire coldboot lock */
>> +       spin_lock(&coldboot_lock);
>> +
>>        /* Send an IPI to all HARTs waiting for coldboot */
>>        for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
>>                if ((i != hartid) &&
>
> You don't want that; that causes the same kinds of data races for
> coldboot_done as before. What could be done is to instead release
> coldboot_lock earlier, provided it doesn't matter that the IPIs haven't
> been sent yet (I don't think it does, because they're already
> inherently asynchronous, so there's currently no guarantee that the
> IPIs have actually occurred when coldboot_lock is unlocked in its
> original location), but that won't fix anything for you if the issue is
> acquiring the lock in the first place in wake_coldboot_harts.
>
> So, ok, your problem is not deadlock but livelock. As Atish has
> suggested, please try clearing MIP at some point during boot before
> wait_for_coldboot, as it really should not be repeatedly trying to
> acquire the lock.

A long as we have a loop in wait_for coldboot with spin_lock() inside
the loop we will always be acquiring that lock repeatedly. This is
independent of any other status.

Why do we need that field coldboot_done at all? There is anyway only one
thread entering the cold boot path. So we can start with the lock closed
and simply unlock it in wake_coldboot_harts():

diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index a7fb848..5645b18 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -84,8 +84,7 @@ 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 spinlock_t coldboot_lock = { .lock = ~__RISCV_SPIN_UNLOCKED };
 static struct sbi_hartmask coldboot_wait_hmask = { 0 };

 static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
@@ -106,14 +105,10 @@ static void wait_for_coldboot(struct sbi_scratch
*scratch, u32 hartid)
        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);
-       };
+       do {
+               wfi();
+               cmip = csr_read(CSR_MIP);
+       } while (!(cmip & MIP_MSIP));

        /* Unmark current HART as waiting */
        sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
@@ -132,12 +127,6 @@ static void wake_coldboot_harts(struct sbi_scratch
*scratch, u32 hartid)
 {
        const struct sbi_platform *plat = sbi_platform_ptr(scratch);

-       /* 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) &&

Best regards

Heinrich

Jessica Clarke Aug. 13, 2020, 6:37 p.m. UTC | #10

On 13 Aug 2020, at 19:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> On 13.08.20 20:11, Jessica Clarke wrote:
>> On 13 Aug 2020, at 19:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 13.08.20 19:03, Jessica Clarke wrote:
>>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> 
>>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>>> 
>>>>>> This smells of there being a more fundamental issue that's not being
>>>>>> addressed which this manages to work around, either reliably or by
>>>>>> chance. What is the actual deadlock condition you observe?
>>>>> 
>>>>> There are only two function using coldboot_lock spinlock:
>>>>> 
>>>>> * wake_coldboot_harts()
>>>>> * wait_for_coldboot()
>>>>> 
>>>>> wake_coldboot_harts() was entered but never left without patching one of
>>>>> the functions.
>>>>> 
>>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>>>> other is sent to the warm boot path.
>>>> 
>>>> The K210's platform definition uses clint_ipi_send, which as far as I
>>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>>>> does with the lock held is set coldboot_done to 1 and send an IPI to
>>>> all the other harts. Thus wake_coldboot_harts should never be able to
>>>> block holding the spin lock, meaning wait_for_coldboot should
>>>> eventually wake up and be able to acquire the lock (on platforms that
>>>> implement WFI fully it shouldn't have to wait long for the lock, though
>>>> on platforms that make it a NOP there might be a bit of .
>>>> 
>>>> Please further investigate (or explain if you already know) exactly how
>>>> you end up in a deadlock situation between wake_coldboot_harts and
>>>> wait_for_coldboot, as I cannot currently see how it could ever be
>>>> possible.
>>>> 
>>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>>>> it without using a spinlock.
>>>>>> 
>>>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>>>> now have a data race in reading coldboot_done, which isn't even
>>>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>>>> its right to do whatever it wants to the code, including things like
>>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>>>> if). This patch introduces undefined behaviour.
>>> 
>>> The cold boot path prints out over the serial console so it is the slow
>>> path. When it arrives at wake_coldboot_harts() the spinlock is already
>>> held by wait_for_coldboot().
>>> 
>>> You have two loops running at the same time:
>>> 
>>> A loop in wait_for_coldboot() which switches the lock on and off
>>> repeatedly and a second loop in wake_coldboot_harts' spin_lock().
>>> 
>>> The warm hart releases the spin lock. This is detected by the cold hart
>>> in spin_lock_check(). In the meantime the warm hart locks the spinlock
>>> again and the cold hart fails in amoswap.w. This can go forever.
>>> 
>>> It is only by different relative speed of the loops if other processor
>>> got out of this situation.
>>> 
>>> A loop as in wait_for_coldboot() reaquiring a lock immediately without
>>> giving sufficient time for other threads to acquire the lock is simply
>>> incorrect coding.
>>> 
>>> The following change also resolves the problem:
>>> 
>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>> index a7fb848..68be58f 100644
>>> --- a/lib/sbi/sbi_init.c
>>> +++ b/lib/sbi/sbi_init.c
>>> @@ -132,12 +132,12 @@ static void wake_coldboot_harts(struct sbi_scratch
>>> *scratch, u32 hartid)
>>> {
>>>       const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>>> 
>>> -       /* Acquire coldboot lock */
>>> -       spin_lock(&coldboot_lock);
>>> -
>>>       /* Mark coldboot done */
>>>       coldboot_done = 1;
>>> 
>>> +       /* Acquire coldboot lock */
>>> +       spin_lock(&coldboot_lock);
>>> +
>>>       /* Send an IPI to all HARTs waiting for coldboot */
>>>       for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
>>>               if ((i != hartid) &&
>> 
>> You don't want that; that causes the same kinds of data races for
>> coldboot_done as before. What could be done is to instead release
>> coldboot_lock earlier, provided it doesn't matter that the IPIs haven't
>> been sent yet (I don't think it does, because they're already
>> inherently asynchronous, so there's currently no guarantee that the
>> IPIs have actually occurred when coldboot_lock is unlocked in its
>> original location), but that won't fix anything for you if the issue is
>> acquiring the lock in the first place in wake_coldboot_harts.
>> 
>> So, ok, your problem is not deadlock but livelock. As Atish has
>> suggested, please try clearing MIP at some point during boot before
>> wait_for_coldboot, as it really should not be repeatedly trying to
>> acquire the lock.
> 
> A long as we have a loop in wait_for coldboot with spin_lock() inside
> the loop we will always be acquiring that lock repeatedly. This is
> independent of any other status.

That's why the !(cmip & MIP_MSIP) check is there; if no IPI has been
sent yet, it just goes round the *inner* loop without even thinking
about acquiring the spin lock. That's why Atish and others have
mentioned clearing MIP, so that you never break out of that outer loop
until an IPI has actually been sent. If you had followed their
suggestions then we believe you would no longer see the starvation.

> Why do we need that field coldboot_done at all? There is anyway only one
> thread entering the cold boot path. So we can start with the lock closed
> and simply unlock it in wake_coldboot_harts():

Because there's global state to set up. You can't have all the other
harts rampaging through memory reading and writing whatever they like
until they can guarantee that the primary hart has performed all the
necessary initialisation for them to be able to roam free. I understand
you are trying to help and offer solutions, but it is getting rather
tedious to deal with what is now the third fundamentally broken patch
based on a lack of understanding, so please try to listen to us and
work with us to fix the problem you are facing rather than continuing
to play doctor and concoct your own broken "fixes".

Jess

> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index a7fb848..5645b18 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -84,8 +84,7 @@ 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 spinlock_t coldboot_lock = { .lock = ~__RISCV_SPIN_UNLOCKED };
> static struct sbi_hartmask coldboot_wait_hmask = { 0 };
> 
> static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
> @@ -106,14 +105,10 @@ static void wait_for_coldboot(struct sbi_scratch
> *scratch, u32 hartid)
>        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);
> -       };
> +       do {
> +               wfi();
> +               cmip = csr_read(CSR_MIP);
> +       } while (!(cmip & MIP_MSIP));
> 
>        /* Unmark current HART as waiting */
>        sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
> @@ -132,12 +127,6 @@ static void wake_coldboot_harts(struct sbi_scratch
> *scratch, u32 hartid)
> {
>        const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> 
> -       /* 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) &&
> 
> Best regards
> 
> Heinrich

Heinrich Schuchardt Aug. 13, 2020, 6:42 p.m. UTC | #11

On 13.08.20 20:37, Jessica Clarke wrote:
> On 13 Aug 2020, at 19:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 13.08.20 20:11, Jessica Clarke wrote:
>>> On 13 Aug 2020, at 19:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 13.08.20 19:03, Jessica Clarke wrote:
>>>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>
>>>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>>>>
>>>>>>> This smells of there being a more fundamental issue that's not being
>>>>>>> addressed which this manages to work around, either reliably or by
>>>>>>> chance. What is the actual deadlock condition you observe?
>>>>>>
>>>>>> There are only two function using coldboot_lock spinlock:
>>>>>>
>>>>>> * wake_coldboot_harts()
>>>>>> * wait_for_coldboot()
>>>>>>
>>>>>> wake_coldboot_harts() was entered but never left without patching one of
>>>>>> the functions.
>>>>>>
>>>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>>>>> other is sent to the warm boot path.
>>>>>
>>>>> The K210's platform definition uses clint_ipi_send, which as far as I
>>>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>>>>> does with the lock held is set coldboot_done to 1 and send an IPI to
>>>>> all the other harts. Thus wake_coldboot_harts should never be able to
>>>>> block holding the spin lock, meaning wait_for_coldboot should
>>>>> eventually wake up and be able to acquire the lock (on platforms that
>>>>> implement WFI fully it shouldn't have to wait long for the lock, though
>>>>> on platforms that make it a NOP there might be a bit of .
>>>>>
>>>>> Please further investigate (or explain if you already know) exactly how
>>>>> you end up in a deadlock situation between wake_coldboot_harts and
>>>>> wait_for_coldboot, as I cannot currently see how it could ever be
>>>>> possible.
>>>>>
>>>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>>>>> it without using a spinlock.
>>>>>>>
>>>>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>>>>> now have a data race in reading coldboot_done, which isn't even
>>>>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>>>>> its right to do whatever it wants to the code, including things like
>>>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>>>>> if). This patch introduces undefined behaviour.
>>>>
>>>> The cold boot path prints out over the serial console so it is the slow
>>>> path. When it arrives at wake_coldboot_harts() the spinlock is already
>>>> held by wait_for_coldboot().
>>>>
>>>> You have two loops running at the same time:
>>>>
>>>> A loop in wait_for_coldboot() which switches the lock on and off
>>>> repeatedly and a second loop in wake_coldboot_harts' spin_lock().
>>>>
>>>> The warm hart releases the spin lock. This is detected by the cold hart
>>>> in spin_lock_check(). In the meantime the warm hart locks the spinlock
>>>> again and the cold hart fails in amoswap.w. This can go forever.
>>>>
>>>> It is only by different relative speed of the loops if other processor
>>>> got out of this situation.
>>>>
>>>> A loop as in wait_for_coldboot() reaquiring a lock immediately without
>>>> giving sufficient time for other threads to acquire the lock is simply
>>>> incorrect coding.
>>>>
>>>> The following change also resolves the problem:
>>>>
>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>> index a7fb848..68be58f 100644
>>>> --- a/lib/sbi/sbi_init.c
>>>> +++ b/lib/sbi/sbi_init.c
>>>> @@ -132,12 +132,12 @@ static void wake_coldboot_harts(struct sbi_scratch
>>>> *scratch, u32 hartid)
>>>> {
>>>>       const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>>>>
>>>> -       /* Acquire coldboot lock */
>>>> -       spin_lock(&coldboot_lock);
>>>> -
>>>>       /* Mark coldboot done */
>>>>       coldboot_done = 1;
>>>>
>>>> +       /* Acquire coldboot lock */
>>>> +       spin_lock(&coldboot_lock);
>>>> +
>>>>       /* Send an IPI to all HARTs waiting for coldboot */
>>>>       for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
>>>>               if ((i != hartid) &&
>>>
>>> You don't want that; that causes the same kinds of data races for
>>> coldboot_done as before. What could be done is to instead release
>>> coldboot_lock earlier, provided it doesn't matter that the IPIs haven't
>>> been sent yet (I don't think it does, because they're already
>>> inherently asynchronous, so there's currently no guarantee that the
>>> IPIs have actually occurred when coldboot_lock is unlocked in its
>>> original location), but that won't fix anything for you if the issue is
>>> acquiring the lock in the first place in wake_coldboot_harts.
>>>
>>> So, ok, your problem is not deadlock but livelock. As Atish has
>>> suggested, please try clearing MIP at some point during boot before
>>> wait_for_coldboot, as it really should not be repeatedly trying to
>>> acquire the lock.
>>
>> A long as we have a loop in wait_for coldboot with spin_lock() inside
>> the loop we will always be acquiring that lock repeatedly. This is
>> independent of any other status.
>
> That's why the !(cmip & MIP_MSIP) check is there; if no IPI has been
> sent yet, it just goes round the *inner* loop without even thinking
> about acquiring the spin lock. That's why Atish and others have
> mentioned clearing MIP, so that you never break out of that outer loop
> until an IPI has actually been sent. If you had followed their
> suggestions then we believe you would no longer see the starvation.
>
>> Why do we need that field coldboot_done at all? There is anyway only one
>> thread entering the cold boot path. So we can start with the lock closed
>> and simply unlock it in wake_coldboot_harts():
>
> Because there's global state to set up. You can't have all the other
> harts rampaging through memory reading and writing whatever they like
> until they can guarantee that the primary hart has performed all the
> necessary initialisation for them to be able to roam free. I understand
> you are trying to help and offer solutions, but it is getting rather
> tedious to deal with what is now the third fundamentally broken patch
> based on a lack of understanding, so please try to listen to us and
> work with us to fix the problem you are facing rather than continuing
> to play doctor and concoct your own broken "fixes".

Dear Jessica,

I will wait for your patch and then test it.

Best regards

Heinrich

>
> Jess
>
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index a7fb848..5645b18 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -84,8 +84,7 @@ 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 spinlock_t coldboot_lock = { .lock = ~__RISCV_SPIN_UNLOCKED };
>> static struct sbi_hartmask coldboot_wait_hmask = { 0 };
>>
>> static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>> @@ -106,14 +105,10 @@ static void wait_for_coldboot(struct sbi_scratch
>> *scratch, u32 hartid)
>>        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);
>> -       };
>> +       do {
>> +               wfi();
>> +               cmip = csr_read(CSR_MIP);
>> +       } while (!(cmip & MIP_MSIP));
>>
>>        /* Unmark current HART as waiting */
>>        sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>> @@ -132,12 +127,6 @@ static void wake_coldboot_harts(struct sbi_scratch
>> *scratch, u32 hartid)
>> {
>>        const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>>
>> -       /* 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) &&
>>
>> Best regards
>>
>> Heinrich
>

Jessica Clarke Aug. 13, 2020, 6:44 p.m. UTC | #12

On 13 Aug 2020, at 19:37, Jessica Clarke <jrtc27@jrtc27.com> wrote:
> On 13 Aug 2020, at 19:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> 
>> On 13.08.20 20:11, Jessica Clarke wrote:
>>> On 13 Aug 2020, at 19:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 13.08.20 19:03, Jessica Clarke wrote:
>>>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>> 
>>>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>>>> 
>>>>>>> This smells of there being a more fundamental issue that's not being
>>>>>>> addressed which this manages to work around, either reliably or by
>>>>>>> chance. What is the actual deadlock condition you observe?
>>>>>> 
>>>>>> There are only two function using coldboot_lock spinlock:
>>>>>> 
>>>>>> * wake_coldboot_harts()
>>>>>> * wait_for_coldboot()
>>>>>> 
>>>>>> wake_coldboot_harts() was entered but never left without patching one of
>>>>>> the functions.
>>>>>> 
>>>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>>>>> other is sent to the warm boot path.
>>>>> 
>>>>> The K210's platform definition uses clint_ipi_send, which as far as I
>>>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>>>>> does with the lock held is set coldboot_done to 1 and send an IPI to
>>>>> all the other harts. Thus wake_coldboot_harts should never be able to
>>>>> block holding the spin lock, meaning wait_for_coldboot should
>>>>> eventually wake up and be able to acquire the lock (on platforms that
>>>>> implement WFI fully it shouldn't have to wait long for the lock, though
>>>>> on platforms that make it a NOP there might be a bit of .
>>>>> 
>>>>> Please further investigate (or explain if you already know) exactly how
>>>>> you end up in a deadlock situation between wake_coldboot_harts and
>>>>> wait_for_coldboot, as I cannot currently see how it could ever be
>>>>> possible.
>>>>> 
>>>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>>>>> it without using a spinlock.
>>>>>>> 
>>>>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>>>>> now have a data race in reading coldboot_done, which isn't even
>>>>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>>>>> its right to do whatever it wants to the code, including things like
>>>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>>>>> if). This patch introduces undefined behaviour.
>>>> 
>>>> The cold boot path prints out over the serial console so it is the slow
>>>> path. When it arrives at wake_coldboot_harts() the spinlock is already
>>>> held by wait_for_coldboot().
>>>> 
>>>> You have two loops running at the same time:
>>>> 
>>>> A loop in wait_for_coldboot() which switches the lock on and off
>>>> repeatedly and a second loop in wake_coldboot_harts' spin_lock().
>>>> 
>>>> The warm hart releases the spin lock. This is detected by the cold hart
>>>> in spin_lock_check(). In the meantime the warm hart locks the spinlock
>>>> again and the cold hart fails in amoswap.w. This can go forever.
>>>> 
>>>> It is only by different relative speed of the loops if other processor
>>>> got out of this situation.
>>>> 
>>>> A loop as in wait_for_coldboot() reaquiring a lock immediately without
>>>> giving sufficient time for other threads to acquire the lock is simply
>>>> incorrect coding.
>>>> 
>>>> The following change also resolves the problem:
>>>> 
>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>> index a7fb848..68be58f 100644
>>>> --- a/lib/sbi/sbi_init.c
>>>> +++ b/lib/sbi/sbi_init.c
>>>> @@ -132,12 +132,12 @@ static void wake_coldboot_harts(struct sbi_scratch
>>>> *scratch, u32 hartid)
>>>> {
>>>>      const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>>>> 
>>>> -       /* Acquire coldboot lock */
>>>> -       spin_lock(&coldboot_lock);
>>>> -
>>>>      /* Mark coldboot done */
>>>>      coldboot_done = 1;
>>>> 
>>>> +       /* Acquire coldboot lock */
>>>> +       spin_lock(&coldboot_lock);
>>>> +
>>>>      /* Send an IPI to all HARTs waiting for coldboot */
>>>>      for (int i = 0; i <= sbi_scratch_last_hartid(); i++) {
>>>>              if ((i != hartid) &&
>>> 
>>> You don't want that; that causes the same kinds of data races for
>>> coldboot_done as before. What could be done is to instead release
>>> coldboot_lock earlier, provided it doesn't matter that the IPIs haven't
>>> been sent yet (I don't think it does, because they're already
>>> inherently asynchronous, so there's currently no guarantee that the
>>> IPIs have actually occurred when coldboot_lock is unlocked in its
>>> original location), but that won't fix anything for you if the issue is
>>> acquiring the lock in the first place in wake_coldboot_harts.
>>> 
>>> So, ok, your problem is not deadlock but livelock. As Atish has
>>> suggested, please try clearing MIP at some point during boot before
>>> wait_for_coldboot, as it really should not be repeatedly trying to
>>> acquire the lock.
>> 
>> A long as we have a loop in wait_for coldboot with spin_lock() inside
>> the loop we will always be acquiring that lock repeatedly. This is
>> independent of any other status.
> 
> That's why the !(cmip & MIP_MSIP) check is there; if no IPI has been
> sent yet, it just goes round the *inner* loop without even thinking
> about acquiring the spin lock. That's why Atish and others have
> mentioned clearing MIP, so that you never break out of that outer loop
> until an IPI has actually been sent. If you had followed their
> suggestions then we believe you would no longer see the starvation.
> 
>> Why do we need that field coldboot_done at all? There is anyway only one
>> thread entering the cold boot path. So we can start with the lock closed
>> and simply unlock it in wake_coldboot_harts():
> 
> Because there's global state to set up. You can't have all the other
> harts rampaging through memory reading and writing whatever they like
> until they can guarantee that the primary hart has performed all the
> necessary initialisation for them to be able to roam free. I understand
> you are trying to help and offer solutions, but it is getting rather
> tedious to deal with what is now the third fundamentally broken patch
> based on a lack of understanding, so please try to listen to us and
> work with us to fix the problem you are facing rather than continuing
> to play doctor and concoct your own broken "fixes".

Apologies, on closer inspection this does "fix" the problem as I had
neglected the change to coldboot_lock's initialisation state. Yeah, it
works, but, no, initialising a lock to be locked on boot is not good.
All you've done is replace coldboot_done with !coldboot_lock (and made
the latter play double-duty). This is hard to follow and just asking
for mistakes to happen because it's not a normal thing to do.

Also, by doing this, you've now made all the harts busy-wait the entire
time rather than be able to execute WFI until the primary hart is done,
as they will be spinning for ages on the lock. This also renders the
WFI you still have in there completely redundant, as to reach that
point the lock must already have been acquired and thus is guaranteed
to have been sent (though not necessarily received). So, no again.

Jess

>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index a7fb848..5645b18 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -84,8 +84,7 @@ 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 spinlock_t coldboot_lock = { .lock = ~__RISCV_SPIN_UNLOCKED };
>> static struct sbi_hartmask coldboot_wait_hmask = { 0 };
>> 
>> static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>> @@ -106,14 +105,10 @@ static void wait_for_coldboot(struct sbi_scratch
>> *scratch, u32 hartid)
>>       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);
>> -       };
>> +       do {
>> +               wfi();
>> +               cmip = csr_read(CSR_MIP);
>> +       } while (!(cmip & MIP_MSIP));
>> 
>>       /* Unmark current HART as waiting */
>>       sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>> @@ -132,12 +127,6 @@ static void wake_coldboot_harts(struct sbi_scratch
>> *scratch, u32 hartid)
>> {
>>       const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>> 
>> -       /* 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) &&
>> 
>> Best regards
>> 
>> Heinrich

Sean Anderson Aug. 13, 2020, 6:53 p.m. UTC | #13

On 8/13/20 2:44 PM, Jessica Clarke wrote:
> On 13 Aug 2020, at 19:37, Jessica Clarke <jrtc27@jrtc27.com> wrote:
>> On 13 Aug 2020, at 19:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> Because there's global state to set up. You can't have all the other
>> harts rampaging through memory reading and writing whatever they like
>> until they can guarantee that the primary hart has performed all the
>> necessary initialisation for them to be able to roam free. I understand
>> you are trying to help and offer solutions, but it is getting rather
>> tedious to deal with what is now the third fundamentally broken patch
>> based on a lack of understanding, so please try to listen to us and
>> work with us to fix the problem you are facing rather than continuing
>> to play doctor and concoct your own broken "fixes".
> 
> Apologies, on closer inspection this does "fix" the problem as I had
> neglected the change to coldboot_lock's initialisation state. Yeah, it
> works, but, no, initialising a lock to be locked on boot is not good.
> All you've done is replace coldboot_done with !coldboot_lock (and made
> the latter play double-duty). This is hard to follow and just asking
> for mistakes to happen because it's not a normal thing to do.
> 
> Also, by doing this, you've now made all the harts busy-wait the entire
> time rather than be able to execute WFI until the primary hart is done,
> as they will be spinning for ages on the lock.

Why does this matter? This code will run for far less than a second,
once per boot. Any energy savings in OpenSBI will be trivial,
especially when compared to later boot stages.

--Sean

Heinrich Schuchardt Aug. 13, 2020, 6:58 p.m. UTC | #14

On 13.08.20 19:45, Atish Patra wrote:
> On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>
>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>
>>>> This smells of there being a more fundamental issue that's not being
>>>> addressed which this manages to work around, either reliably or by
>>>> chance. What is the actual deadlock condition you observe?
>>>
>>> There are only two function using coldboot_lock spinlock:
>>>
>>> * wake_coldboot_harts()
>>> * wait_for_coldboot()
>>>
>>> wake_coldboot_harts() was entered but never left without patching one of
>>> the functions.
>>>
>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>> other is sent to the warm boot path.
>>
>> The K210's platform definition uses clint_ipi_send, which as far as I
>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>> does with the lock held is set coldboot_done to 1 and send an IPI to
>> all the other harts. Thus wake_coldboot_harts should never be able to
>> block holding the spin lock, meaning wait_for_coldboot should
>> eventually wake up and be able to acquire the lock (on platforms that
>> implement WFI fully it shouldn't have to wait long for the lock, though
>> on platforms that make it a NOP there might be a bit of .
>>
>
> Yes. If wake_coldboot_harts is never able to acquire the spinlock,
> that indicates spinlock
> is always acquired by wait_for_coldboot. That can happen if
>
> 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
> is set which
> shouldn't happen unless there is an IPI sent.
>
> Can you try clearing the MIP just before acquiring the lock ? In qemu
> & unleashed, modifying the MSIP bit in
> MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.

The following does *not* solve the problem:

diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index a7fb848..a3111d3 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -105,6 +105,8 @@ static void wait_for_coldboot(struct sbi_scratch
*scratch, u32 hartid)
        /* Mark current HART as waiting */
        sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);

+       sbi_platform_ipi_clear(plat, hartid);
+
        /* Wait for coldboot to finish using WFI */
        while (!coldboot_done) {
                spin_unlock(&coldboot_lock);

Anyway according to the RISC-V spec wfi may be implemented as a simple nop:

"The purpose of the WFI instruction is to provide a hint to the
implementation, and so a legal implementation is to simply implement WFI
as a NOP."

Best regards

Heinrich

>
>> Please further investigate (or explain if you already know) exactly how
>> you end up in a deadlock situation between wake_coldboot_harts and
>> wait_for_coldboot, as I cannot currently see how it could ever be
>> possible.
>>
>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>> it without using a spinlock.
>>>>
>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>> now have a data race in reading coldboot_done, which isn't even
>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>> its right to do whatever it wants to the code, including things like
>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>> if). This patch introduces undefined behaviour.
>>>>
>>>> Jess
>>>>
>>>> ^ Please do not make it volatile as a workaround, that is not the right
>>>>  way to "fix" such issues.
>>>
>>> What do you suggest Jessica?
>>
>> My primary suggestion is above, since this does not look like a
>> necessary fix.
>>
>> There is the separate question of whether this code should be using
>> spin locks anyway. One could optimise it with suitable use of proper
>> atomics to access coldboot_done, which would mask whatever issue you
>> are seeing, but that should not be necessary, and in early boot code
>> like this that's not on a hot code path it's better to keep things
>> simple like it is currently rather than risk writing unsafe code
>> through incorrect use of atomics. I would feel much more comfortable
>> leaving the code using spin locks.
>>
>> Jess
>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>> lib/sbi/sbi_init.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>>> index a7fb848..a0e4f11 100644
>>>>> --- a/lib/sbi/sbi_init.c
>>>>> +++ b/lib/sbi/sbi_init.c
>>>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>>>>>     sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>>>>
>>>>>     /* Wait for coldboot to finish using WFI */
>>>>> +   spin_unlock(&coldboot_lock);
>>>>>     while (!coldboot_done) {
>>>>> -           spin_unlock(&coldboot_lock);
>>>>>             do {
>>>>>                     wfi();
>>>>>                     cmip = csr_read(CSR_MIP);
>>>>>              } while (!(cmip & MIP_MSIP));
>>>>> -           spin_lock(&coldboot_lock);
>>>>>     };
>>>>> +   spin_lock(&coldboot_lock);
>>>>>
>>>>>     /* Unmark current HART as waiting */
>>>>>     sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>>>>> --
>>>>> 2.28.0
>>>>>
>>>>>
>>>>> --
>>>>> opensbi mailing list
>>>>> opensbi@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>
>>
>> --
>> opensbi mailing list
>> opensbi@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>
>
>
> --
> Regards,
> Atish
>

Jessica Clarke Aug. 13, 2020, 6:59 p.m. UTC | #15

On 13 Aug 2020, at 19:53, Sean Anderson <seanga2@gmail.com> wrote:
> On 8/13/20 2:44 PM, Jessica Clarke wrote:
>> On 13 Aug 2020, at 19:37, Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>> On 13 Aug 2020, at 19:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> Because there's global state to set up. You can't have all the other
>>> harts rampaging through memory reading and writing whatever they like
>>> until they can guarantee that the primary hart has performed all the
>>> necessary initialisation for them to be able to roam free. I understand
>>> you are trying to help and offer solutions, but it is getting rather
>>> tedious to deal with what is now the third fundamentally broken patch
>>> based on a lack of understanding, so please try to listen to us and
>>> work with us to fix the problem you are facing rather than continuing
>>> to play doctor and concoct your own broken "fixes".
>> 
>> Apologies, on closer inspection this does "fix" the problem as I had
>> neglected the change to coldboot_lock's initialisation state. Yeah, it
>> works, but, no, initialising a lock to be locked on boot is not good.
>> All you've done is replace coldboot_done with !coldboot_lock (and made
>> the latter play double-duty). This is hard to follow and just asking
>> for mistakes to happen because it's not a normal thing to do.
>> 
>> Also, by doing this, you've now made all the harts busy-wait the entire
>> time rather than be able to execute WFI until the primary hart is done,
>> as they will be spinning for ages on the lock.
> 
> Why does this matter? This code will run for far less than a second,
> once per boot. Any energy savings in OpenSBI will be trivial,
> especially when compared to later boot stages.

These things are ultimately subjective, and everyone on this list
probably has slightly different opinions. But in general, why use
inefficient code when you can just as easily write more efficient code,
especially when the inefficient code is harder to reason about and more
error-prone?

Jess

Jessica Clarke Aug. 13, 2020, 7:16 p.m. UTC | #16

On 13 Aug 2020, at 19:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 13.08.20 19:45, Atish Patra wrote:
>> On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>> 
>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>> 
>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>> 
>>>>> This smells of there being a more fundamental issue that's not being
>>>>> addressed which this manages to work around, either reliably or by
>>>>> chance. What is the actual deadlock condition you observe?
>>>> 
>>>> There are only two function using coldboot_lock spinlock:
>>>> 
>>>> * wake_coldboot_harts()
>>>> * wait_for_coldboot()
>>>> 
>>>> wake_coldboot_harts() was entered but never left without patching one of
>>>> the functions.
>>>> 
>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>>> other is sent to the warm boot path.
>>> 
>>> The K210's platform definition uses clint_ipi_send, which as far as I
>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>>> does with the lock held is set coldboot_done to 1 and send an IPI to
>>> all the other harts. Thus wake_coldboot_harts should never be able to
>>> block holding the spin lock, meaning wait_for_coldboot should
>>> eventually wake up and be able to acquire the lock (on platforms that
>>> implement WFI fully it shouldn't have to wait long for the lock, though
>>> on platforms that make it a NOP there might be a bit of .
>>> 
>> 
>> Yes. If wake_coldboot_harts is never able to acquire the spinlock,
>> that indicates spinlock
>> is always acquired by wait_for_coldboot. That can happen if
>> 
>> 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
>> is set which
>> shouldn't happen unless there is an IPI sent.
>> 
>> Can you try clearing the MIP just before acquiring the lock ? In qemu
>> & unleashed, modifying the MSIP bit in
>> MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.
> 
> The following does *not* solve the problem:

What if you do it before enabling MSIP in MIE? If that doesn't work it
sounds like something is very broken with that hardware.

> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index a7fb848..a3111d3 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -105,6 +105,8 @@ static void wait_for_coldboot(struct sbi_scratch
> *scratch, u32 hartid)
>        /* Mark current HART as waiting */
>        sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
> 
> +       sbi_platform_ipi_clear(plat, hartid);
> +
>        /* Wait for coldboot to finish using WFI */
>        while (!coldboot_done) {
>                spin_unlock(&coldboot_lock);
> 
> Anyway according to the RISC-V spec wfi may be implemented as a simple nop:
> 
> "The purpose of the WFI instruction is to provide a hint to the
> implementation, and so a legal implementation is to simply implement WFI
> as a NOP."

Yes, we know, that's not the problem here. The problem is that your
hardware _actually thinks there's a pending software interrupt_, since
the MSIP bit is set in MIP. An implementation that makes WFI a NOP is
perfectly fine because, as I've said at least once already, the _inner_
loop still checks MIP.MSIP before attempting to acquire the lock again.

Jess

>>> Please further investigate (or explain if you already know) exactly how
>>> you end up in a deadlock situation between wake_coldboot_harts and
>>> wait_for_coldboot, as I cannot currently see how it could ever be
>>> possible.
>>> 
>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>>> it without using a spinlock.
>>>>> 
>>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>>> now have a data race in reading coldboot_done, which isn't even
>>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>>> its right to do whatever it wants to the code, including things like
>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>>> if). This patch introduces undefined behaviour.
>>>>> 
>>>>> Jess
>>>>> 
>>>>> ^ Please do not make it volatile as a workaround, that is not the right
>>>>> way to "fix" such issues.
>>>> 
>>>> What do you suggest Jessica?
>>> 
>>> My primary suggestion is above, since this does not look like a
>>> necessary fix.
>>> 
>>> There is the separate question of whether this code should be using
>>> spin locks anyway. One could optimise it with suitable use of proper
>>> atomics to access coldboot_done, which would mask whatever issue you
>>> are seeing, but that should not be necessary, and in early boot code
>>> like this that's not on a hot code path it's better to keep things
>>> simple like it is currently rather than risk writing unsafe code
>>> through incorrect use of atomics. I would feel much more comfortable
>>> leaving the code using spin locks.
>>> 
>>> Jess
>>> 
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>> ---
>>>>>> lib/sbi/sbi_init.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>>>> index a7fb848..a0e4f11 100644
>>>>>> --- a/lib/sbi/sbi_init.c
>>>>>> +++ b/lib/sbi/sbi_init.c
>>>>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>>>>>>    sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>>>>> 
>>>>>>    /* Wait for coldboot to finish using WFI */
>>>>>> +   spin_unlock(&coldboot_lock);
>>>>>>    while (!coldboot_done) {
>>>>>> -           spin_unlock(&coldboot_lock);
>>>>>>            do {
>>>>>>                    wfi();
>>>>>>                    cmip = csr_read(CSR_MIP);
>>>>>>             } while (!(cmip & MIP_MSIP));
>>>>>> -           spin_lock(&coldboot_lock);
>>>>>>    };
>>>>>> +   spin_lock(&coldboot_lock);
>>>>>> 
>>>>>>    /* Unmark current HART as waiting */
>>>>>>    sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>>>>>> --
>>>>>> 2.28.0
>>>>>> 
>>>>>> 
>>>>>> --
>>>>>> opensbi mailing list
>>>>>> opensbi@lists.infradead.org
>>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>> 
>>> 
>>> --
>>> opensbi mailing list
>>> opensbi@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/opensbi
>> 
>> 
>> 
>> --
>> Regards,
>> Atish

Heinrich Schuchardt Aug. 13, 2020, 8:04 p.m. UTC | #17

On 13.08.20 21:16, Jessica Clarke wrote:
> On 13 Aug 2020, at 19:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 13.08.20 19:45, Atish Patra wrote:
>>> On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>>>
>>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>
>>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>>>
>>>>>> This smells of there being a more fundamental issue that's not being
>>>>>> addressed which this manages to work around, either reliably or by
>>>>>> chance. What is the actual deadlock condition you observe?
>>>>>
>>>>> There are only two function using coldboot_lock spinlock:
>>>>>
>>>>> * wake_coldboot_harts()
>>>>> * wait_for_coldboot()
>>>>>
>>>>> wake_coldboot_harts() was entered but never left without patching one of
>>>>> the functions.
>>>>>
>>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>>>> other is sent to the warm boot path.
>>>>
>>>> The K210's platform definition uses clint_ipi_send, which as far as I
>>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>>>> does with the lock held is set coldboot_done to 1 and send an IPI to
>>>> all the other harts. Thus wake_coldboot_harts should never be able to
>>>> block holding the spin lock, meaning wait_for_coldboot should
>>>> eventually wake up and be able to acquire the lock (on platforms that
>>>> implement WFI fully it shouldn't have to wait long for the lock, though
>>>> on platforms that make it a NOP there might be a bit of .
>>>>
>>>
>>> Yes. If wake_coldboot_harts is never able to acquire the spinlock,
>>> that indicates spinlock
>>> is always acquired by wait_for_coldboot. That can happen if
>>>
>>> 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
>>> is set which
>>> shouldn't happen unless there is an IPI sent.
>>>
>>> Can you try clearing the MIP just before acquiring the lock ? In qemu
>>> & unleashed, modifying the MSIP bit in
>>> MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.
>>
>> The following does *not* solve the problem:
>
> What if you do it before enabling MSIP in MIE? If that doesn't work it
> sounds like something is very broken with that hardware.

This *fails* too:

diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index a7fb848..a29de51 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -96,6 +96,8 @@ static void wait_for_coldboot(struct sbi_scratch
*scratch, u32 hartid)
        /* Save MIE CSR */
        saved_mie = csr_read(CSR_MIE);

+       sbi_platform_ipi_clear(plat, hartid);
+
        /* Set MSIE bit to receive IPI */
        csr_set(CSR_MIE, MIP_MSIP);

Sean mentioned that the Kendryte K210 implements RISC-V Privileged
Architectures Specification 1.9.1. But I don't see anything in the
change log of the spec that would be relevant here.

To my knowledge the Kendryte K210 systems are the only really affordable
64bit RISC-V boards up to now. So it would be great to get them properly
running in OpenSBI even if they deviate from the most current RISC-V specs.

Best regards

Heinrich

>
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index a7fb848..a3111d3 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -105,6 +105,8 @@ static void wait_for_coldboot(struct sbi_scratch
>> *scratch, u32 hartid)
>>        /* Mark current HART as waiting */
>>        sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>
>> +       sbi_platform_ipi_clear(plat, hartid);
>> +
>>        /* Wait for coldboot to finish using WFI */
>>        while (!coldboot_done) {
>>                spin_unlock(&coldboot_lock);
>>
>> Anyway according to the RISC-V spec wfi may be implemented as a simple nop:
>>
>> "The purpose of the WFI instruction is to provide a hint to the
>> implementation, and so a legal implementation is to simply implement WFI
>> as a NOP."
>
> Yes, we know, that's not the problem here. The problem is that your
> hardware _actually thinks there's a pending software interrupt_, since
> the MSIP bit is set in MIP. An implementation that makes WFI a NOP is
> perfectly fine because, as I've said at least once already, the _inner_
> loop still checks MIP.MSIP before attempting to acquire the lock again.
>
> Jess
>
>>>> Please further investigate (or explain if you already know) exactly how
>>>> you end up in a deadlock situation between wake_coldboot_harts and
>>>> wait_for_coldboot, as I cannot currently see how it could ever be
>>>> possible.
>>>>
>>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>>>> it without using a spinlock.
>>>>>>
>>>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>>>> now have a data race in reading coldboot_done, which isn't even
>>>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>>>> its right to do whatever it wants to the code, including things like
>>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>>>> if). This patch introduces undefined behaviour.
>>>>>>
>>>>>> Jess
>>>>>>
>>>>>> ^ Please do not make it volatile as a workaround, that is not the right
>>>>>> way to "fix" such issues.
>>>>>
>>>>> What do you suggest Jessica?
>>>>
>>>> My primary suggestion is above, since this does not look like a
>>>> necessary fix.
>>>>
>>>> There is the separate question of whether this code should be using
>>>> spin locks anyway. One could optimise it with suitable use of proper
>>>> atomics to access coldboot_done, which would mask whatever issue you
>>>> are seeing, but that should not be necessary, and in early boot code
>>>> like this that's not on a hot code path it's better to keep things
>>>> simple like it is currently rather than risk writing unsafe code
>>>> through incorrect use of atomics. I would feel much more comfortable
>>>> leaving the code using spin locks.
>>>>
>>>> Jess
>>>>
>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>> ---
>>>>>>> lib/sbi/sbi_init.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>>>>> index a7fb848..a0e4f11 100644
>>>>>>> --- a/lib/sbi/sbi_init.c
>>>>>>> +++ b/lib/sbi/sbi_init.c
>>>>>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>>>>>>>    sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>>>>>>
>>>>>>>    /* Wait for coldboot to finish using WFI */
>>>>>>> +   spin_unlock(&coldboot_lock);
>>>>>>>    while (!coldboot_done) {
>>>>>>> -           spin_unlock(&coldboot_lock);
>>>>>>>            do {
>>>>>>>                    wfi();
>>>>>>>                    cmip = csr_read(CSR_MIP);
>>>>>>>             } while (!(cmip & MIP_MSIP));
>>>>>>> -           spin_lock(&coldboot_lock);
>>>>>>>    };
>>>>>>> +   spin_lock(&coldboot_lock);
>>>>>>>
>>>>>>>    /* Unmark current HART as waiting */
>>>>>>>    sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>>>>>>> --
>>>>>>> 2.28.0
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> opensbi mailing list
>>>>>>> opensbi@lists.infradead.org
>>>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>>
>>>
>>>
>>> --
>>> Regards,
>>> Atish
>
>

Atish Patra Aug. 13, 2020, 9:14 p.m. UTC | #18

On Thu, Aug 13, 2020 at 1:04 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 13.08.20 21:16, Jessica Clarke wrote:
> > On 13 Aug 2020, at 19:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >> On 13.08.20 19:45, Atish Patra wrote:
> >>> On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>>>
> >>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>> On 13.08.20 18:36, Jessica Clarke wrote:
> >>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>
> >>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
> >>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
> >>>>>>
> >>>>>> This smells of there being a more fundamental issue that's not being
> >>>>>> addressed which this manages to work around, either reliably or by
> >>>>>> chance. What is the actual deadlock condition you observe?
> >>>>>
> >>>>> There are only two function using coldboot_lock spinlock:
> >>>>>
> >>>>> * wake_coldboot_harts()
> >>>>> * wait_for_coldboot()
> >>>>>
> >>>>> wake_coldboot_harts() was entered but never left without patching one of
> >>>>> the functions.
> >>>>>
> >>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
> >>>>> other is sent to the warm boot path.
> >>>>
> >>>> The K210's platform definition uses clint_ipi_send, which as far as I
> >>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
> >>>> does with the lock held is set coldboot_done to 1 and send an IPI to
> >>>> all the other harts. Thus wake_coldboot_harts should never be able to
> >>>> block holding the spin lock, meaning wait_for_coldboot should
> >>>> eventually wake up and be able to acquire the lock (on platforms that
> >>>> implement WFI fully it shouldn't have to wait long for the lock, though
> >>>> on platforms that make it a NOP there might be a bit of .
> >>>>
> >>>
> >>> Yes. If wake_coldboot_harts is never able to acquire the spinlock,
> >>> that indicates spinlock
> >>> is always acquired by wait_for_coldboot. That can happen if
> >>>
> >>> 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
> >>> is set which
> >>> shouldn't happen unless there is an IPI sent.
> >>>
> >>> Can you try clearing the MIP just before acquiring the lock ? In qemu
> >>> & unleashed, modifying the MSIP bit in
> >>> MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.
> >>
> >> The following does *not* solve the problem:
> >
> > What if you do it before enabling MSIP in MIE? If that doesn't work it
> > sounds like something is very broken with that hardware.
>
> This *fails* too:
>

Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
even before sbi_ipi_init is being called from the hart running coldboot.

Can we confirm the MSIP being set theory first by adding a long
outside the inner while loop
before acquiring the lock ? I don't see any other reason why the inner
loop will keep acquiring the spinlock
unless we are hitting some nasty spinlock bug with Kendryte.

You may need to directly write MIP value to the uart address
(0x38000000) if sbi_console_init is not invoked yet.

> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index a7fb848..a29de51 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -96,6 +96,8 @@ static void wait_for_coldboot(struct sbi_scratch
> *scratch, u32 hartid)
>         /* Save MIE CSR */
>         saved_mie = csr_read(CSR_MIE);
>
> +       sbi_platform_ipi_clear(plat, hartid);
> +
>         /* Set MSIE bit to receive IPI */
>         csr_set(CSR_MIE, MIP_MSIP);
>
> Sean mentioned that the Kendryte K210 implements RISC-V Privileged
> Architectures Specification 1.9.1. But I don't see anything in the
> change log of the spec that would be relevant here.
>

Yeah. I checked 1.9.1 as well. I couldn't find anything weird that can
cause this issue.

> To my knowledge the Kendryte K210 systems are the only really affordable
> 64bit RISC-V boards up to now. So it would be great to get them properly
> running in OpenSBI even if they deviate from the most current RISC-V specs.
>
> Best regards
>
> Heinrich
>
> >
> >> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> >> index a7fb848..a3111d3 100644
> >> --- a/lib/sbi/sbi_init.c
> >> +++ b/lib/sbi/sbi_init.c
> >> @@ -105,6 +105,8 @@ static void wait_for_coldboot(struct sbi_scratch
> >> *scratch, u32 hartid)
> >>        /* Mark current HART as waiting */
> >>        sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
> >>
> >> +       sbi_platform_ipi_clear(plat, hartid);
> >> +
> >>        /* Wait for coldboot to finish using WFI */
> >>        while (!coldboot_done) {
> >>                spin_unlock(&coldboot_lock);
> >>
> >> Anyway according to the RISC-V spec wfi may be implemented as a simple nop:
> >>
> >> "The purpose of the WFI instruction is to provide a hint to the
> >> implementation, and so a legal implementation is to simply implement WFI
> >> as a NOP."
> >
> > Yes, we know, that's not the problem here. The problem is that your
> > hardware _actually thinks there's a pending software interrupt_, since
> > the MSIP bit is set in MIP. An implementation that makes WFI a NOP is
> > perfectly fine because, as I've said at least once already, the _inner_
> > loop still checks MIP.MSIP before attempting to acquire the lock again.
> >
> > Jess
> >
> >>>> Please further investigate (or explain if you already know) exactly how
> >>>> you end up in a deadlock situation between wake_coldboot_harts and
> >>>> wait_for_coldboot, as I cannot currently see how it could ever be
> >>>> possible.
> >>>>
> >>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
> >>>>>>> it without using a spinlock.
> >>>>>>
> >>>>>> Maybe microarchitecturally on the specific hardware in question (though
> >>>>>> I wouldn't even be so certain about that), but definitely not in C. You
> >>>>>> now have a data race in reading coldboot_done, which isn't even
> >>>>>> volatile^, so a sufficiently smart compiler would be completely within
> >>>>>> its right to do whatever it wants to the code, including things like
> >>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
> >>>>>> if). This patch introduces undefined behaviour.
> >>>>>>
> >>>>>> Jess
> >>>>>>
> >>>>>> ^ Please do not make it volatile as a workaround, that is not the right
> >>>>>> way to "fix" such issues.
> >>>>>
> >>>>> What do you suggest Jessica?
> >>>>
> >>>> My primary suggestion is above, since this does not look like a
> >>>> necessary fix.
> >>>>
> >>>> There is the separate question of whether this code should be using
> >>>> spin locks anyway. One could optimise it with suitable use of proper
> >>>> atomics to access coldboot_done, which would mask whatever issue you
> >>>> are seeing, but that should not be necessary, and in early boot code
> >>>> like this that's not on a hot code path it's better to keep things
> >>>> simple like it is currently rather than risk writing unsafe code
> >>>> through incorrect use of atomics. I would feel much more comfortable
> >>>> leaving the code using spin locks.
> >>>>
> >>>> Jess
> >>>>
> >>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>> ---
> >>>>>>> lib/sbi/sbi_init.c | 4 ++--
> >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> >>>>>>> index a7fb848..a0e4f11 100644
> >>>>>>> --- a/lib/sbi/sbi_init.c
> >>>>>>> +++ b/lib/sbi/sbi_init.c
> >>>>>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
> >>>>>>>    sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
> >>>>>>>
> >>>>>>>    /* Wait for coldboot to finish using WFI */
> >>>>>>> +   spin_unlock(&coldboot_lock);
> >>>>>>>    while (!coldboot_done) {
> >>>>>>> -           spin_unlock(&coldboot_lock);
> >>>>>>>            do {
> >>>>>>>                    wfi();
> >>>>>>>                    cmip = csr_read(CSR_MIP);
> >>>>>>>             } while (!(cmip & MIP_MSIP));
> >>>>>>> -           spin_lock(&coldboot_lock);
> >>>>>>>    };
> >>>>>>> +   spin_lock(&coldboot_lock);
> >>>>>>>
> >>>>>>>    /* Unmark current HART as waiting */
> >>>>>>>    sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
> >>>>>>> --
> >>>>>>> 2.28.0
> >>>>>>>
> >>>>>>>
> >>>>>>> --
> >>>>>>> opensbi mailing list
> >>>>>>> opensbi@lists.infradead.org
> >>>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
> >>>>
> >>>>
> >>>> --
> >>>> opensbi mailing list
> >>>> opensbi@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/opensbi
> >>>
> >>>
> >>>
> >>> --
> >>> Regards,
> >>> Atish
> >
> >
>

Heinrich Schuchardt Aug. 13, 2020, 9:28 p.m. UTC | #19

On 8/13/20 11:14 PM, Atish Patra wrote:
> On Thu, Aug 13, 2020 at 1:04 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 13.08.20 21:16, Jessica Clarke wrote:
>>> On 13 Aug 2020, at 19:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 13.08.20 19:45, Atish Patra wrote:
>>>>> On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>>>>>
>>>>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> On 13.08.20 18:36, Jessica Clarke wrote:
>>>>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>>
>>>>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
>>>>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
>>>>>>>>
>>>>>>>> This smells of there being a more fundamental issue that's not being
>>>>>>>> addressed which this manages to work around, either reliably or by
>>>>>>>> chance. What is the actual deadlock condition you observe?
>>>>>>>
>>>>>>> There are only two function using coldboot_lock spinlock:
>>>>>>>
>>>>>>> * wake_coldboot_harts()
>>>>>>> * wait_for_coldboot()
>>>>>>>
>>>>>>> wake_coldboot_harts() was entered but never left without patching one of
>>>>>>> the functions.
>>>>>>>
>>>>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
>>>>>>> other is sent to the warm boot path.
>>>>>>
>>>>>> The K210's platform definition uses clint_ipi_send, which as far as I
>>>>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
>>>>>> does with the lock held is set coldboot_done to 1 and send an IPI to
>>>>>> all the other harts. Thus wake_coldboot_harts should never be able to
>>>>>> block holding the spin lock, meaning wait_for_coldboot should
>>>>>> eventually wake up and be able to acquire the lock (on platforms that
>>>>>> implement WFI fully it shouldn't have to wait long for the lock, though
>>>>>> on platforms that make it a NOP there might be a bit of .
>>>>>>
>>>>>
>>>>> Yes. If wake_coldboot_harts is never able to acquire the spinlock,
>>>>> that indicates spinlock
>>>>> is always acquired by wait_for_coldboot. That can happen if
>>>>>
>>>>> 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
>>>>> is set which
>>>>> shouldn't happen unless there is an IPI sent.
>>>>>
>>>>> Can you try clearing the MIP just before acquiring the lock ? In qemu
>>>>> & unleashed, modifying the MSIP bit in
>>>>> MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.
>>>>
>>>> The following does *not* solve the problem:
>>>
>>> What if you do it before enabling MSIP in MIE? If that doesn't work it
>>> sounds like something is very broken with that hardware.
>>
>> This *fails* too:
>>
>
> Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
> even before sbi_ipi_init is being called from the hart running coldboot.

There are two calls to sbi_ipi_init(). Is only the one in coldboot relevant?

Do you mean something like:

diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index a7fb848..b062ce2 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -33,6 +33,8 @@
 	"        | |\n"                                     \
 	"        |_|\n\n"

+static char first = 0x20;
+
 static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
 {
 	int xlen;
@@ -80,6 +82,8 @@ static void sbi_boot_prints(struct sbi_scratch
*scratch, u32 hartid)
 		   sbi_ecall_version_major(), sbi_ecall_version_minor());
 	sbi_printf("\n");

+	sbi_printf("First %c\n", first);
+
 	sbi_hart_delegation_dump(scratch);
 	sbi_hart_pmp_dump(scratch);
 }
@@ -93,6 +97,9 @@ static void wait_for_coldboot(struct sbi_scratch
*scratch, u32 hartid)
 	unsigned long saved_mie, cmip;
 	const struct sbi_platform *plat = sbi_platform_ptr(scratch);

+	if (!first)
+		first = 'W';
+
 	/* Save MIE CSR */
 	saved_mie = csr_read(CSR_MIE);

@@ -187,6 +194,8 @@ static void __noreturn init_coldboot(struct
sbi_scratch *scratch, u32 hartid)
 	if (rc)
 		sbi_hart_hang();

+	if (!first)
+		first = 'C';
 	rc = sbi_ipi_init(scratch, TRUE);
 	if (rc)
 		sbi_hart_hang();
@@ -247,6 +256,8 @@ static void __noreturn init_warmboot(struct
sbi_scratch *scratch, u32 hartid)
 	if (rc)
 		sbi_hart_hang();

+	if (!first)
+		first = 'w';
 	rc = sbi_ipi_init(scratch, FALSE);
 	if (rc)
 		sbi_hart_hang();

Best regards

Heinrich

>
> Can we confirm the MSIP being set theory first by adding a long
> outside the inner while loop
> before acquiring the lock ? I don't see any other reason why the inner
> loop will keep acquiring the spinlock
> unless we are hitting some nasty spinlock bug with Kendryte.
>
> You may need to directly write MIP value to the uart address
> (0x38000000) if sbi_console_init is not invoked yet.
>
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index a7fb848..a29de51 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -96,6 +96,8 @@ static void wait_for_coldboot(struct sbi_scratch
>> *scratch, u32 hartid)
>>         /* Save MIE CSR */
>>         saved_mie = csr_read(CSR_MIE);
>>
>> +       sbi_platform_ipi_clear(plat, hartid);
>> +
>>         /* Set MSIE bit to receive IPI */
>>         csr_set(CSR_MIE, MIP_MSIP);
>>
>> Sean mentioned that the Kendryte K210 implements RISC-V Privileged
>> Architectures Specification 1.9.1. But I don't see anything in the
>> change log of the spec that would be relevant here.
>>
>
> Yeah. I checked 1.9.1 as well. I couldn't find anything weird that can
> cause this issue.
>
>> To my knowledge the Kendryte K210 systems are the only really affordable
>> 64bit RISC-V boards up to now. So it would be great to get them properly
>> running in OpenSBI even if they deviate from the most current RISC-V specs.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>> index a7fb848..a3111d3 100644
>>>> --- a/lib/sbi/sbi_init.c
>>>> +++ b/lib/sbi/sbi_init.c
>>>> @@ -105,6 +105,8 @@ static void wait_for_coldboot(struct sbi_scratch
>>>> *scratch, u32 hartid)
>>>>        /* Mark current HART as waiting */
>>>>        sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>>>
>>>> +       sbi_platform_ipi_clear(plat, hartid);
>>>> +
>>>>        /* Wait for coldboot to finish using WFI */
>>>>        while (!coldboot_done) {
>>>>                spin_unlock(&coldboot_lock);
>>>>
>>>> Anyway according to the RISC-V spec wfi may be implemented as a simple nop:
>>>>
>>>> "The purpose of the WFI instruction is to provide a hint to the
>>>> implementation, and so a legal implementation is to simply implement WFI
>>>> as a NOP."
>>>
>>> Yes, we know, that's not the problem here. The problem is that your
>>> hardware _actually thinks there's a pending software interrupt_, since
>>> the MSIP bit is set in MIP. An implementation that makes WFI a NOP is
>>> perfectly fine because, as I've said at least once already, the _inner_
>>> loop still checks MIP.MSIP before attempting to acquire the lock again.
>>>
>>> Jess
>>>
>>>>>> Please further investigate (or explain if you already know) exactly how
>>>>>> you end up in a deadlock situation between wake_coldboot_harts and
>>>>>> wait_for_coldboot, as I cannot currently see how it could ever be
>>>>>> possible.
>>>>>>
>>>>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
>>>>>>>>> it without using a spinlock.
>>>>>>>>
>>>>>>>> Maybe microarchitecturally on the specific hardware in question (though
>>>>>>>> I wouldn't even be so certain about that), but definitely not in C. You
>>>>>>>> now have a data race in reading coldboot_done, which isn't even
>>>>>>>> volatile^, so a sufficiently smart compiler would be completely within
>>>>>>>> its right to do whatever it wants to the code, including things like
>>>>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
>>>>>>>> if). This patch introduces undefined behaviour.
>>>>>>>>
>>>>>>>> Jess
>>>>>>>>
>>>>>>>> ^ Please do not make it volatile as a workaround, that is not the right
>>>>>>>> way to "fix" such issues.
>>>>>>>
>>>>>>> What do you suggest Jessica?
>>>>>>
>>>>>> My primary suggestion is above, since this does not look like a
>>>>>> necessary fix.
>>>>>>
>>>>>> There is the separate question of whether this code should be using
>>>>>> spin locks anyway. One could optimise it with suitable use of proper
>>>>>> atomics to access coldboot_done, which would mask whatever issue you
>>>>>> are seeing, but that should not be necessary, and in early boot code
>>>>>> like this that's not on a hot code path it's better to keep things
>>>>>> simple like it is currently rather than risk writing unsafe code
>>>>>> through incorrect use of atomics. I would feel much more comfortable
>>>>>> leaving the code using spin locks.
>>>>>>
>>>>>> Jess
>>>>>>
>>>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>> ---
>>>>>>>>> lib/sbi/sbi_init.c | 4 ++--
>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>>>>>>>>> index a7fb848..a0e4f11 100644
>>>>>>>>> --- a/lib/sbi/sbi_init.c
>>>>>>>>> +++ b/lib/sbi/sbi_init.c
>>>>>>>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>>>>>>>>>    sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>>>>>>>>>
>>>>>>>>>    /* Wait for coldboot to finish using WFI */
>>>>>>>>> +   spin_unlock(&coldboot_lock);
>>>>>>>>>    while (!coldboot_done) {
>>>>>>>>> -           spin_unlock(&coldboot_lock);
>>>>>>>>>            do {
>>>>>>>>>                    wfi();
>>>>>>>>>                    cmip = csr_read(CSR_MIP);
>>>>>>>>>             } while (!(cmip & MIP_MSIP));
>>>>>>>>> -           spin_lock(&coldboot_lock);
>>>>>>>>>    };
>>>>>>>>> +   spin_lock(&coldboot_lock);
>>>>>>>>>
>>>>>>>>>    /* Unmark current HART as waiting */
>>>>>>>>>    sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>>>>>>>>> --
>>>>>>>>> 2.28.0
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> opensbi mailing list
>>>>>>>>> opensbi@lists.infradead.org
>>>>>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>>>>>
>>>>>>
>>>>>> --
>>>>>> opensbi mailing list
>>>>>> opensbi@lists.infradead.org
>>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Atish
>>>
>>>
>>
>
>

Atish Patra Aug. 13, 2020, 10:25 p.m. UTC | #20

On Thu, Aug 13, 2020 at 2:34 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/13/20 11:14 PM, Atish Patra wrote:
> > On Thu, Aug 13, 2020 at 1:04 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 13.08.20 21:16, Jessica Clarke wrote:
> >>> On 13 Aug 2020, at 19:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>> On 13.08.20 19:45, Atish Patra wrote:
> >>>>> On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>>>>>
> >>>>>> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>> On 13.08.20 18:36, Jessica Clarke wrote:
> >>>>>>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>>>
> >>>>>>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
> >>>>>>>>> when trying to boot U-Boot as FW_PAYLOAD.
> >>>>>>>>
> >>>>>>>> This smells of there being a more fundamental issue that's not being
> >>>>>>>> addressed which this manages to work around, either reliably or by
> >>>>>>>> chance. What is the actual deadlock condition you observe?
> >>>>>>>
> >>>>>>> There are only two function using coldboot_lock spinlock:
> >>>>>>>
> >>>>>>> * wake_coldboot_harts()
> >>>>>>> * wait_for_coldboot()
> >>>>>>>
> >>>>>>> wake_coldboot_harts() was entered but never left without patching one of
> >>>>>>> the functions.
> >>>>>>>
> >>>>>>> The Kendryte K210 has two harts. One goes the cold boot path while the
> >>>>>>> other is sent to the warm boot path.
> >>>>>>
> >>>>>> The K210's platform definition uses clint_ipi_send, which as far as I
> >>>>>> can tell is entirely non-blocking. The only thing wake_coldboot_harts
> >>>>>> does with the lock held is set coldboot_done to 1 and send an IPI to
> >>>>>> all the other harts. Thus wake_coldboot_harts should never be able to
> >>>>>> block holding the spin lock, meaning wait_for_coldboot should
> >>>>>> eventually wake up and be able to acquire the lock (on platforms that
> >>>>>> implement WFI fully it shouldn't have to wait long for the lock, though
> >>>>>> on platforms that make it a NOP there might be a bit of .
> >>>>>>
> >>>>>
> >>>>> Yes. If wake_coldboot_harts is never able to acquire the spinlock,
> >>>>> that indicates spinlock
> >>>>> is always acquired by wait_for_coldboot. That can happen if
> >>>>>
> >>>>> 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
> >>>>> is set which
> >>>>> shouldn't happen unless there is an IPI sent.
> >>>>>
> >>>>> Can you try clearing the MIP just before acquiring the lock ? In qemu
> >>>>> & unleashed, modifying the MSIP bit in
> >>>>> MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.
> >>>>
> >>>> The following does *not* solve the problem:
> >>>
> >>> What if you do it before enabling MSIP in MIE? If that doesn't work it
> >>> sounds like something is very broken with that hardware.
> >>
> >> This *fails* too:
> >>
> >
> > Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
> > even before sbi_ipi_init is being called from the hart running coldboot.
>
> There are two calls to sbi_ipi_init(). Is only the one in coldboot relevant?
>

Yes. That actually setups the clint base address.

> Do you mean something like:
>

No. I meant like this.

+#define UART_BASE 0x38000000ULL
+static void sbi_print_early(char ch) {
+    int32_t r;
+    do {
+      __asm__ __volatile__ (
+        "amoor.w %0, %2, %1\n"
+        : "=r" (r), "+A" (*(uint32_t*)(UART_BASE))
+        : "r" (ch));
+    } while (r < 0);
+}
+
 static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
 {
        int xlen;
@@ -112,6 +123,8 @@ static void wait_for_coldboot(struct sbi_scratch
*scratch, u32 hartid)
                        wfi();
                        cmip = csr_read(CSR_MIP);
                 } while (!(cmip & MIP_MSIP));
+               sbi_print_early('T');
+               sbi_printf("mip value %lx\n", cmip);
                spin_lock(&coldboot_lock);
        };


Note: sbi_printf may not work if sbi_console_init from the cold boot
path is not invoked.
That's why I added a "sbi_print_early".  If sbi_printf doesn't work,
please remove it.

Thanks for your time in figuring out the issue with Kendryte.
Unfortunately, I can't test these changes as I don't have
a kendryte board at home.


> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index a7fb848..b062ce2 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -33,6 +33,8 @@
>         "        | |\n"                                     \
>         "        |_|\n\n"
>
> +static char first = 0x20;
> +
>  static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
>  {
>         int xlen;
> @@ -80,6 +82,8 @@ static void sbi_boot_prints(struct sbi_scratch
> *scratch, u32 hartid)
>                    sbi_ecall_version_major(), sbi_ecall_version_minor());
>         sbi_printf("\n");
>
> +       sbi_printf("First %c\n", first);
> +
>         sbi_hart_delegation_dump(scratch);
>         sbi_hart_pmp_dump(scratch);
>  }
> @@ -93,6 +97,9 @@ static void wait_for_coldboot(struct sbi_scratch
> *scratch, u32 hartid)
>         unsigned long saved_mie, cmip;
>         const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>
> +       if (!first)
> +               first = 'W';
> +
>         /* Save MIE CSR */
>         saved_mie = csr_read(CSR_MIE);
>
> @@ -187,6 +194,8 @@ static void __noreturn init_coldboot(struct
> sbi_scratch *scratch, u32 hartid)
>         if (rc)
>                 sbi_hart_hang();
>
> +       if (!first)
> +               first = 'C';
>         rc = sbi_ipi_init(scratch, TRUE);
>         if (rc)
>                 sbi_hart_hang();
> @@ -247,6 +256,8 @@ static void __noreturn init_warmboot(struct
> sbi_scratch *scratch, u32 hartid)
>         if (rc)
>                 sbi_hart_hang();
>
> +       if (!first)
> +               first = 'w';
>         rc = sbi_ipi_init(scratch, FALSE);
>         if (rc)
>                 sbi_hart_hang();
>
> Best regards
>
> Heinrich
>
> >
> > Can we confirm the MSIP being set theory first by adding a long
> > outside the inner while loop
> > before acquiring the lock ? I don't see any other reason why the inner
> > loop will keep acquiring the spinlock
> > unless we are hitting some nasty spinlock bug with Kendryte.
> >
> > You may need to directly write MIP value to the uart address
> > (0x38000000) if sbi_console_init is not invoked yet.
> >
> >> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> >> index a7fb848..a29de51 100644
> >> --- a/lib/sbi/sbi_init.c
> >> +++ b/lib/sbi/sbi_init.c
> >> @@ -96,6 +96,8 @@ static void wait_for_coldboot(struct sbi_scratch
> >> *scratch, u32 hartid)
> >>         /* Save MIE CSR */
> >>         saved_mie = csr_read(CSR_MIE);
> >>
> >> +       sbi_platform_ipi_clear(plat, hartid);
> >> +
> >>         /* Set MSIE bit to receive IPI */
> >>         csr_set(CSR_MIE, MIP_MSIP);
> >>
> >> Sean mentioned that the Kendryte K210 implements RISC-V Privileged
> >> Architectures Specification 1.9.1. But I don't see anything in the
> >> change log of the spec that would be relevant here.
> >>
> >
> > Yeah. I checked 1.9.1 as well. I couldn't find anything weird that can
> > cause this issue.
> >
> >> To my knowledge the Kendryte K210 systems are the only really affordable
> >> 64bit RISC-V boards up to now. So it would be great to get them properly
> >> running in OpenSBI even if they deviate from the most current RISC-V specs.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> >>>> index a7fb848..a3111d3 100644
> >>>> --- a/lib/sbi/sbi_init.c
> >>>> +++ b/lib/sbi/sbi_init.c
> >>>> @@ -105,6 +105,8 @@ static void wait_for_coldboot(struct sbi_scratch
> >>>> *scratch, u32 hartid)
> >>>>        /* Mark current HART as waiting */
> >>>>        sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
> >>>>
> >>>> +       sbi_platform_ipi_clear(plat, hartid);
> >>>> +
> >>>>        /* Wait for coldboot to finish using WFI */
> >>>>        while (!coldboot_done) {
> >>>>                spin_unlock(&coldboot_lock);
> >>>>
> >>>> Anyway according to the RISC-V spec wfi may be implemented as a simple nop:
> >>>>
> >>>> "The purpose of the WFI instruction is to provide a hint to the
> >>>> implementation, and so a legal implementation is to simply implement WFI
> >>>> as a NOP."
> >>>
> >>> Yes, we know, that's not the problem here. The problem is that your
> >>> hardware _actually thinks there's a pending software interrupt_, since
> >>> the MSIP bit is set in MIP. An implementation that makes WFI a NOP is
> >>> perfectly fine because, as I've said at least once already, the _inner_
> >>> loop still checks MIP.MSIP before attempting to acquire the lock again.
> >>>
> >>> Jess
> >>>
> >>>>>> Please further investigate (or explain if you already know) exactly how
> >>>>>> you end up in a deadlock situation between wake_coldboot_harts and
> >>>>>> wait_for_coldboot, as I cannot currently see how it could ever be
> >>>>>> possible.
> >>>>>>
> >>>>>>>>> Even if reading and writing coldboot_done is not atomic it is safe to check
> >>>>>>>>> it without using a spinlock.
> >>>>>>>>
> >>>>>>>> Maybe microarchitecturally on the specific hardware in question (though
> >>>>>>>> I wouldn't even be so certain about that), but definitely not in C. You
> >>>>>>>> now have a data race in reading coldboot_done, which isn't even
> >>>>>>>> volatile^, so a sufficiently smart compiler would be completely within
> >>>>>>>> its right to do whatever it wants to the code, including things like
> >>>>>>>> delete the loop entirely (or, slightly less dramatic, turn it into an
> >>>>>>>> if). This patch introduces undefined behaviour.
> >>>>>>>>
> >>>>>>>> Jess
> >>>>>>>>
> >>>>>>>> ^ Please do not make it volatile as a workaround, that is not the right
> >>>>>>>> way to "fix" such issues.
> >>>>>>>
> >>>>>>> What do you suggest Jessica?
> >>>>>>
> >>>>>> My primary suggestion is above, since this does not look like a
> >>>>>> necessary fix.
> >>>>>>
> >>>>>> There is the separate question of whether this code should be using
> >>>>>> spin locks anyway. One could optimise it with suitable use of proper
> >>>>>> atomics to access coldboot_done, which would mask whatever issue you
> >>>>>> are seeing, but that should not be necessary, and in early boot code
> >>>>>> like this that's not on a hot code path it's better to keep things
> >>>>>> simple like it is currently rather than risk writing unsafe code
> >>>>>> through incorrect use of atomics. I would feel much more comfortable
> >>>>>> leaving the code using spin locks.
> >>>>>>
> >>>>>> Jess
> >>>>>>
> >>>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>> ---
> >>>>>>>>> lib/sbi/sbi_init.c | 4 ++--
> >>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> >>>>>>>>> index a7fb848..a0e4f11 100644
> >>>>>>>>> --- a/lib/sbi/sbi_init.c
> >>>>>>>>> +++ b/lib/sbi/sbi_init.c
> >>>>>>>>> @@ -106,14 +106,14 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
> >>>>>>>>>    sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
> >>>>>>>>>
> >>>>>>>>>    /* Wait for coldboot to finish using WFI */
> >>>>>>>>> +   spin_unlock(&coldboot_lock);
> >>>>>>>>>    while (!coldboot_done) {
> >>>>>>>>> -           spin_unlock(&coldboot_lock);
> >>>>>>>>>            do {
> >>>>>>>>>                    wfi();
> >>>>>>>>>                    cmip = csr_read(CSR_MIP);
> >>>>>>>>>             } while (!(cmip & MIP_MSIP));
> >>>>>>>>> -           spin_lock(&coldboot_lock);
> >>>>>>>>>    };
> >>>>>>>>> +   spin_lock(&coldboot_lock);
> >>>>>>>>>
> >>>>>>>>>    /* Unmark current HART as waiting */
> >>>>>>>>>    sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
> >>>>>>>>> --
> >>>>>>>>> 2.28.0
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> opensbi mailing list
> >>>>>>>>> opensbi@lists.infradead.org
> >>>>>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
> >>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> opensbi mailing list
> >>>>>> opensbi@lists.infradead.org
> >>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Regards,
> >>>>> Atish
> >>>
> >>>
> >>
> >
> >
>


--
Regards,
Atish

Heinrich Schuchardt Aug. 13, 2020, 10:52 p.m. UTC | #21

On 14.08.20 00:25, Atish Patra wrote:
>>> Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
>>> even before sbi_ipi_init is being called from the hart running coldboot.
>> There are two calls to sbi_ipi_init(). Is only the one in coldboot relevant?
>>
> Yes. That actually setups the clint base address.
>
>> Do you mean something like:
>>
> No. I meant like this.
>
> +#define UART_BASE 0x38000000ULL
> +static void sbi_print_early(char ch) {
> +    int32_t r;
> +    do {
> +      __asm__ __volatile__ (
> +        "amoor.w %0, %2, %1\n"
> +        : "=r" (r), "+A" (*(uint32_t*)(UART_BASE))
> +        : "r" (ch));
> +    } while (r < 0);
> +}
> +
>  static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
>  {
>         int xlen;
> @@ -112,6 +123,8 @@ static void wait_for_coldboot(struct sbi_scratch
> *scratch, u32 hartid)
>                         wfi();
>                         cmip = csr_read(CSR_MIP);
>                  } while (!(cmip & MIP_MSIP));
> +               sbi_print_early('T');
> +               sbi_printf("mip value %lx\n", cmip);
>                 spin_lock(&coldboot_lock);
>         };
>
>
> Note: sbi_printf may not work if sbi_console_init from the cold boot
> path is not invoked.
> That's why I added a "sbi_print_early".  If sbi_printf doesn't work,
> please remove it.
>
> Thanks for your time in figuring out the issue with Kendryte.
> Unfortunately, I can't test these changes as I don't have
> a kendryte board at home.
>
>

This now booted to U-Boot. The extra time for printing was enough to
break the livelock:


TTTTTTTTTT
          OpenSBI v0.8-11-g749e10e
mip value 8
T  ____                    _____ ____ _____
  / __ \                  / ____|  _ \_   _|
 | |  | |_ __   ___ _ __ | (___ | |_) || |
 | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
 | |__| | |_) |  __/ | | |____) | |_) || |_
  \____/| .__/ \___|_| |_|_____/|____/_____|
        | |
        |_|

mip value 8
Tlatform Name       : Kendryte K210
mip value 8
Tlatform Features   : timer
mip value 8
Tlatform HART Count : 2
Boot HART ID        : 0
mip value 8
Toot HART ISA       : rv64imafdcsu
mip value 8
TOOT HART Features  : none
mip value 8
TOOT HART PMP Count : 0
mip value 8
Tirmware Base       : 0x80000000
mip value 8
Tirmware Size       : 72 KB
mip value 8
TTuntime SBI Version : 0.2
mip value 8
T
 mip value 8
TIDELEG : 0x0000000000000222
mip value 8
TEDELEG : 0x0000000000000109
mip value 8


U-Boot 2020.10-rc2-00127-g795d62af44-dirty (Aug 13 2020 - 22:00:56 +0200)

DRAM:  8 MiB
In:    serial@38000000
Out:   serial@38000000
Err:   serial@38000000
=>

Best regards

Heinrich

Atish Patra Aug. 14, 2020, 12:32 a.m. UTC | #22

On Thu, Aug 13, 2020 at 3:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 14.08.20 00:25, Atish Patra wrote:
> >>> Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
> >>> even before sbi_ipi_init is being called from the hart running coldboot.
> >> There are two calls to sbi_ipi_init(). Is only the one in coldboot relevant?
> >>
> > Yes. That actually setups the clint base address.
> >
> >> Do you mean something like:
> >>
> > No. I meant like this.
> >
> > +#define UART_BASE 0x38000000ULL
> > +static void sbi_print_early(char ch) {
> > +    int32_t r;
> > +    do {
> > +      __asm__ __volatile__ (
> > +        "amoor.w %0, %2, %1\n"
> > +        : "=r" (r), "+A" (*(uint32_t*)(UART_BASE))
> > +        : "r" (ch));
> > +    } while (r < 0);
> > +}
> > +
> >  static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
> >  {
> >         int xlen;
> > @@ -112,6 +123,8 @@ static void wait_for_coldboot(struct sbi_scratch
> > *scratch, u32 hartid)
> >                         wfi();
> >                         cmip = csr_read(CSR_MIP);
> >                  } while (!(cmip & MIP_MSIP));
> > +               sbi_print_early('T');
> > +               sbi_printf("mip value %lx\n", cmip);
> >                 spin_lock(&coldboot_lock);
> >         };
> >
> >
> > Note: sbi_printf may not work if sbi_console_init from the cold boot
> > path is not invoked.
> > That's why I added a "sbi_print_early".  If sbi_printf doesn't work,
> > please remove it.
> >
> > Thanks for your time in figuring out the issue with Kendryte.
> > Unfortunately, I can't test these changes as I don't have
> > a kendryte board at home.
> >
> >
>
> This now booted to U-Boot. The extra time for printing was enough to
> break the livelock:
>
Thanks again for verification.

>
> TTTTTTTTTT

This confirms that MSIP bit is set even if there was no IPI sent and MIP was
cleared in fw_base.S [1]. I guess there is a hardware bug with Kendryte where
MSIP is not getting reset properly.

[1] https://github.com/riscv/opensbi/blob/master/firmware/fw_base.S#L370

If that's the case, we can convert coldboot_done to an atomic variable
as suggested by
Anup & Jessica.

>           OpenSBI v0.8-11-g749e10e
> mip value 8
> T  ____                    _____ ____ _____
>   / __ \                  / ____|  _ \_   _|
>  | |  | |_ __   ___ _ __ | (___ | |_) || |
>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>  | |__| | |_) |  __/ | | |____) | |_) || |_
>   \____/| .__/ \___|_| |_|_____/|____/_____|
>         | |
>         |_|
>
> mip value 8
> Tlatform Name       : Kendryte K210
> mip value 8
> Tlatform Features   : timer
> mip value 8
> Tlatform HART Count : 2
> Boot HART ID        : 0
> mip value 8
> Toot HART ISA       : rv64imafdcsu
> mip value 8
> TOOT HART Features  : none
> mip value 8
> TOOT HART PMP Count : 0
> mip value 8
> Tirmware Base       : 0x80000000
> mip value 8
> Tirmware Size       : 72 KB
> mip value 8
> TTuntime SBI Version : 0.2
> mip value 8
> T
>  mip value 8
> TIDELEG : 0x0000000000000222
> mip value 8
> TEDELEG : 0x0000000000000109
> mip value 8
>
>
> U-Boot 2020.10-rc2-00127-g795d62af44-dirty (Aug 13 2020 - 22:00:56 +0200)
>
> DRAM:  8 MiB
> In:    serial@38000000
> Out:   serial@38000000
> Err:   serial@38000000
> =>
>
> Best regards
>
> Heinrich

Sean Anderson Aug. 14, 2020, 12:39 a.m. UTC | #23

On 8/13/20 8:32 PM, Atish Patra wrote:
> On Thu, Aug 13, 2020 at 3:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 14.08.20 00:25, Atish Patra wrote:
>>>>> Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
>>>>> even before sbi_ipi_init is being called from the hart running coldboot.
>>>> There are two calls to sbi_ipi_init(). Is only the one in coldboot relevant?
>>>>
>>> Yes. That actually setups the clint base address.
>>>
>>>> Do you mean something like:
>>>>
>>> No. I meant like this.
>>>
>>> +#define UART_BASE 0x38000000ULL
>>> +static void sbi_print_early(char ch) {
>>> +    int32_t r;
>>> +    do {
>>> +      __asm__ __volatile__ (
>>> +        "amoor.w %0, %2, %1\n"
>>> +        : "=r" (r), "+A" (*(uint32_t*)(UART_BASE))
>>> +        : "r" (ch));
>>> +    } while (r < 0);
>>> +}
>>> +
>>>  static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
>>>  {
>>>         int xlen;
>>> @@ -112,6 +123,8 @@ static void wait_for_coldboot(struct sbi_scratch
>>> *scratch, u32 hartid)
>>>                         wfi();
>>>                         cmip = csr_read(CSR_MIP);
>>>                  } while (!(cmip & MIP_MSIP));
>>> +               sbi_print_early('T');
>>> +               sbi_printf("mip value %lx\n", cmip);
>>>                 spin_lock(&coldboot_lock);
>>>         };
>>>
>>>
>>> Note: sbi_printf may not work if sbi_console_init from the cold boot
>>> path is not invoked.
>>> That's why I added a "sbi_print_early".  If sbi_printf doesn't work,
>>> please remove it.
>>>
>>> Thanks for your time in figuring out the issue with Kendryte.
>>> Unfortunately, I can't test these changes as I don't have
>>> a kendryte board at home.
>>>
>>>
>>
>> This now booted to U-Boot. The extra time for printing was enough to
>> break the livelock:
>>
> Thanks again for verification.
> 
>>
>> TTTTTTTTTT
> 
> This confirms that MSIP bit is set even if there was no IPI sent and MIP was
> cleared in fw_base.S [1]. I guess there is a hardware bug with Kendryte where
> MSIP is not getting reset properly.

Hm, perhaps MIP is level-based, so the CLINT itself needs to be
acknowledged before MIP will be disabled.

> 
> [1] https://github.com/riscv/opensbi/blob/master/firmware/fw_base.S#L370
> 
> If that's the case, we can convert coldboot_done to an atomic variable
> as suggested by

I think this is the best course of action in general. All the harts are
just waiting around, so there's no need for an interrupt here.

--Sean

Damien Le Moal Aug. 14, 2020, 3:11 a.m. UTC | #24

On Thu, 2020-08-13 at 20:39 -0400, Sean Anderson wrote:
> On 8/13/20 8:32 PM, Atish Patra wrote:
> > On Thu, Aug 13, 2020 at 3:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > On 14.08.20 00:25, Atish Patra wrote:
> > > > > > Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
> > > > > > even before sbi_ipi_init is being called from the hart running coldboot.
> > > > > There are two calls to sbi_ipi_init(). Is only the one in coldboot relevant?
> > > > > 
> > > > Yes. That actually setups the clint base address.
> > > > 
> > > > > Do you mean something like:
> > > > > 
> > > > No. I meant like this.
> > > > 
> > > > +#define UART_BASE 0x38000000ULL
> > > > +static void sbi_print_early(char ch) {
> > > > +    int32_t r;
> > > > +    do {
> > > > +      __asm__ __volatile__ (
> > > > +        "amoor.w %0, %2, %1\n"
> > > > +        : "=r" (r), "+A" (*(uint32_t*)(UART_BASE))
> > > > +        : "r" (ch));
> > > > +    } while (r < 0);
> > > > +}
> > > > +
> > > >  static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
> > > >  {
> > > >         int xlen;
> > > > @@ -112,6 +123,8 @@ static void wait_for_coldboot(struct sbi_scratch
> > > > *scratch, u32 hartid)
> > > >                         wfi();
> > > >                         cmip = csr_read(CSR_MIP);
> > > >                  } while (!(cmip & MIP_MSIP));
> > > > +               sbi_print_early('T');
> > > > +               sbi_printf("mip value %lx\n", cmip);
> > > >                 spin_lock(&coldboot_lock);
> > > >         };
> > > > 
> > > > 
> > > > Note: sbi_printf may not work if sbi_console_init from the cold boot
> > > > path is not invoked.
> > > > That's why I added a "sbi_print_early".  If sbi_printf doesn't work,
> > > > please remove it.
> > > > 
> > > > Thanks for your time in figuring out the issue with Kendryte.
> > > > Unfortunately, I can't test these changes as I don't have
> > > > a kendryte board at home.
> > > > 
> > > > 
> > > 
> > > This now booted to U-Boot. The extra time for printing was enough to
> > > break the livelock:
> > > 
> > Thanks again for verification.
> > 
> > > TTTTTTTTTT
> > 
> > This confirms that MSIP bit is set even if there was no IPI sent and MIP was
> > cleared in fw_base.S [1]. I guess there is a hardware bug with Kendryte where
> > MSIP is not getting reset properly.
> 
> Hm, perhaps MIP is level-based, so the CLINT itself needs to be
> acknowledged before MIP will be disabled.
> 
> > [1] https://github.com/riscv/opensbi/blob/master/firmware/fw_base.S#L370
> > 
> > If that's the case, we can convert coldboot_done to an atomic variable
> > as suggested by
> 
> I think this is the best course of action in general. All the harts are
> just waiting around, so there's no need for an interrupt here.

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)) {};
 }
-static void wake_coldboot_harts(struct sbi_scratch *scratch, u32
hartid)
+static inline void wake_coldboot_harts(struct sbi_scratch *scratch,
u32 hartid)
 {
-       const struct sbi_platform *plat = sbi_platform_ptr(scratch);
-
-       /* 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) &&
-                   sbi_hartmask_test_hart(i, &coldboot_wait_hmask))
-                       sbi_platform_ipi_send(plat, i);
-       }
-
-       /* Release coldboot lock */
-       spin_unlock(&coldboot_lock);
+       atomic_write(&coldboot_done, 1);
 }
 
 static unsigned long init_count_offset;


> 
--Sean

Anup Patel Aug. 14, 2020, 6:08 a.m. UTC | #25

> -----Original Message-----
> From: Damien Le Moal <Damien.LeMoal@wdc.com>
> Sent: 14 August 2020 08:42
> To: atishp@atishpatra.org; xypron.glpk@gmx.de; seanga2@gmail.com
> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
> bmeng.cn@gmail.com; Anup Patel <Anup.Patel@wdc.com>;
> opensbi@lists.infradead.org; merle@hardenedlinux.org
> Subject: Re: [PATCH 1/1] lib: sbi: relax scoldboot_lock spinlock
> 
> On Thu, 2020-08-13 at 20:39 -0400, Sean Anderson wrote:
> > On 8/13/20 8:32 PM, Atish Patra wrote:
> > > On Thu, Aug 13, 2020 at 3:52 PM Heinrich Schuchardt
> <xypron.glpk@gmx.de> wrote:
> > > > On 14.08.20 00:25, Atish Patra wrote:
> > > > > > > Probably, the hart executing warmboot invokes
> > > > > > > sbi_platform_ipi_clear even before sbi_ipi_init is being called
> from the hart running coldboot.
> > > > > > There are two calls to sbi_ipi_init(). Is only the one in coldboot
> relevant?
> > > > > >
> > > > > Yes. That actually setups the clint base address.
> > > > >
> > > > > > Do you mean something like:
> > > > > >
> > > > > No. I meant like this.
> > > > >
> > > > > +#define UART_BASE 0x38000000ULL static void
> > > > > +sbi_print_early(char ch) {
> > > > > +    int32_t r;
> > > > > +    do {
> > > > > +      __asm__ __volatile__ (
> > > > > +        "amoor.w %0, %2, %1\n"
> > > > > +        : "=r" (r), "+A" (*(uint32_t*)(UART_BASE))
> > > > > +        : "r" (ch));
> > > > > +    } while (r < 0);
> > > > > +}
> > > > > +
> > > > >  static void sbi_boot_prints(struct sbi_scratch *scratch, u32
> > > > > hartid)  {
> > > > >         int xlen;
> > > > > @@ -112,6 +123,8 @@ static void wait_for_coldboot(struct
> > > > > sbi_scratch *scratch, u32 hartid)
> > > > >                         wfi();
> > > > >                         cmip = csr_read(CSR_MIP);
> > > > >                  } while (!(cmip & MIP_MSIP));
> > > > > +               sbi_print_early('T');
> > > > > +               sbi_printf("mip value %lx\n", cmip);
> > > > >                 spin_lock(&coldboot_lock);
> > > > >         };
> > > > >
> > > > >
> > > > > Note: sbi_printf may not work if sbi_console_init from the cold
> > > > > boot path is not invoked.
> > > > > That's why I added a "sbi_print_early".  If sbi_printf doesn't
> > > > > work, please remove it.
> > > > >
> > > > > Thanks for your time in figuring out the issue with Kendryte.
> > > > > Unfortunately, I can't test these changes as I don't have a
> > > > > kendryte board at home.
> > > > >
> > > > >
> > > >
> > > > This now booted to U-Boot. The extra time for printing was enough
> > > > to break the livelock:
> > > >
> > > Thanks again for verification.
> > >
> > > > TTTTTTTTTT
> > >
> > > This confirms that MSIP bit is set even if there was no IPI sent and
> > > MIP was cleared in fw_base.S [1]. I guess there is a hardware bug
> > > with Kendryte where MSIP is not getting reset properly.
> >
> > Hm, perhaps MIP is level-based, so the CLINT itself needs to be
> > acknowledged before MIP will be disabled.
> >
> > > [1]
> > >
> https://github.com/riscv/opensbi/blob/master/firmware/fw_base.S#L370
> > >
> > > If that's the case, we can convert coldboot_done to an atomic
> > > variable as suggested by
> >
> > I think this is the best course of action in general. All the harts
> > are just waiting around, so there's no need for an interrupt here.
> 
> 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)) {};

This is over-simplified and will slow down the booting in cold boot path.

Imagine, boot CPU in cold boot path and all secondary CPUs bombarding
the interconnect with atomic operations.

It has to be a combination of atomic coldboot_done and WFI to
avoid unnecessary traffic in interconnect. 

We just need to make coldboot_done atomic and don't use coldboot_lock
for coldboot_done. Rest of the things should stay as-is.

Regards,
Anup

>  }
> -static void wake_coldboot_harts(struct sbi_scratch *scratch, u32
> hartid)
> +static inline void wake_coldboot_harts(struct sbi_scratch *scratch,
> u32 hartid)
>  {
> -       const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> -
> -       /* 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) &&
> -                   sbi_hartmask_test_hart(i, &coldboot_wait_hmask))
> -                       sbi_platform_ipi_send(plat, i);
> -       }
> -
> -       /* Release coldboot lock */
> -       spin_unlock(&coldboot_lock);
> +       atomic_write(&coldboot_done, 1);
>  }
> 
>  static unsigned long init_count_offset;
> 
> 
> >
> --Sean
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Damien Le Moal Aug. 14, 2020, 6:19 a.m. UTC | #26

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)) {};
> 
> This is over-simplified and will slow down the booting in cold boot path.
> 
> Imagine, boot CPU in cold boot path and all secondary CPUs bombarding
> the interconnect with atomic operations.

That was just a quick test. With this, the kendryte goes to S mode and start
running the fw payload, which does not execute correctly still. So there is
another problem somewhere with kendryte platform. Not sure what.

> 
> It has to be a combination of atomic coldboot_done and WFI to
> avoid unnecessary traffic in interconnect. 
> 
> We just need to make coldboot_done atomic and don't use coldboot_lock
> for coldboot_done. Rest of the things should stay as-is.

Without the spinlock, there will be problems with coldboot_wait_hmask if it is
used as is. But I do not think that one is necessary. The boot hart can just
send an IPI to all waiting harts. The waiting harts do not need to clear that mask.

Anup Patel Aug. 14, 2020, 7:03 a.m. UTC | #27

> -----Original Message-----
> From: Damien Le Moal <Damien.LeMoal@wdc.com>
> Sent: 14 August 2020 11:49
> To: Anup Patel <Anup.Patel@wdc.com>; atishp@atishpatra.org;
> xypron.glpk@gmx.de; seanga2@gmail.com
> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
> bmeng.cn@gmail.com; opensbi@lists.infradead.org;
> merle@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)) {};
> >
> > This is over-simplified and will slow down the booting in cold boot path.
> >
> > Imagine, boot CPU in cold boot path and all secondary CPUs bombarding
> > the interconnect with atomic operations.
> 
> That was just a quick test. With this, the kendryte goes to S mode and start
> running the fw payload, which does not execute correctly still. So there is
> another problem somewhere with kendryte platform. Not sure what.
> 
> >
> > It has to be a combination of atomic coldboot_done and WFI to avoid
> > unnecessary traffic in interconnect.
> >
> > We just need to make coldboot_done atomic and don't use coldboot_lock
> > for coldboot_done. Rest of the things should stay as-is.
> 
> Without the spinlock, there will be problems with coldboot_wait_hmask if it
> is used as is. But I do not think that one is necessary. The boot hart can just
> send an IPI to all waiting harts. The waiting harts do not need to clear that
> mask.

There is a contradiction in your statement here. How does boot hart know
which all HARTs are waiting for IPI ? In real world systems, we can have
faulty parts where certain CPUs never come-up so we can't assume that
all CPUs are waiting for WFI.

The coldboot_wait_hmask is certainly required and if we use spinlock
just for coldboot_wait_hmask then it is fine because it won't be used in
a loop.

Regards,
Anup

> 
> 
> --
> Damien Le Moal
> Western Digital Research

Heinrich Schuchardt Aug. 14, 2020, 9:15 a.m. UTC | #28

On 14.08.20 09:03, Anup Patel wrote:
>
>
>> -----Original Message-----
>> From: Damien Le Moal <Damien.LeMoal@wdc.com>
>> Sent: 14 August 2020 11:49
>> To: Anup Patel <Anup.Patel@wdc.com>; atishp@atishpatra.org;
>> xypron.glpk@gmx.de; seanga2@gmail.com
>> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
>> bmeng.cn@gmail.com; opensbi@lists.infradead.org;
>> merle@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

    #define barrier() __asm__ __volatile__("": : :"memory")

as a compiler barrier.

I think we can completely remove the 'coldboot_lock' spin-lock and use
barrier() instead.

Best regards

Heinrich

>>>
>>> This is over-simplified and will slow down the booting in cold boot path.
>>>
>>> Imagine, boot CPU in cold boot path and all secondary CPUs bombarding
>>> the interconnect with atomic operations.
>>
>> That was just a quick test. With this, the kendryte goes to S mode and start
>> running the fw payload, which does not execute correctly still. So there is
>> another problem somewhere with kendryte platform. Not sure what.
>>
>>>
>>> It has to be a combination of atomic coldboot_done and WFI to avoid
>>> unnecessary traffic in interconnect.
>>>
>>> We just need to make coldboot_done atomic and don't use coldboot_lock
>>> for coldboot_done. Rest of the things should stay as-is.
>>
>> Without the spinlock, there will be problems with coldboot_wait_hmask if it
>> is used as is. But I do not think that one is necessary. The boot hart can just
>> send an IPI to all waiting harts. The waiting harts do not need to clear that
>> mask.
>
> There is a contradiction in your statement here. How does boot hart know
> which all HARTs are waiting for IPI ? In real world systems, we can have
> faulty parts where certain CPUs never come-up so we can't assume that
> all CPUs are waiting for WFI.
>
> The coldboot_wait_hmask is certainly required and if we use spinlock
> just for coldboot_wait_hmask then it is fine because it won't be used in
> a loop.
>
> Regards,
> Anup
>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>

Anup Patel Aug. 14, 2020, 9:20 a.m. UTC | #29

> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Sent: 14 August 2020 14:46
> To: Anup Patel <Anup.Patel@wdc.com>; Damien Le Moal
> <Damien.LeMoal@wdc.com>; atishp@atishpatra.org; seanga2@gmail.com
> Cc: Atish Patra <Atish.Patra@wdc.com>; bmeng.cn@gmail.com;
> jrtc27@jrtc27.com; opensbi@lists.infradead.org; merle@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@wdc.com>
> >> Sent: 14 August 2020 11:49
> >> To: Anup Patel <Anup.Patel@wdc.com>; atishp@atishpatra.org;
> >> xypron.glpk@gmx.de; seanga2@gmail.com
> >> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
> >> bmeng.cn@gmail.com; opensbi@lists.infradead.org;
> >> merle@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
> 
>     #define 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.

Regards,
Anup

> 
> Best regards
> 
> Heinrich
> 
> >>>
> >>> This is over-simplified and will slow down the booting in cold boot path.
> >>>
> >>> Imagine, boot CPU in cold boot path and all secondary CPUs
> >>> bombarding the interconnect with atomic operations.
> >>
> >> That was just a quick test. With this, the kendryte goes to S mode
> >> and start running the fw payload, which does not execute correctly
> >> still. So there is another problem somewhere with kendryte platform. Not
> sure what.
> >>
> >>>
> >>> It has to be a combination of atomic coldboot_done and WFI to avoid
> >>> unnecessary traffic in interconnect.
> >>>
> >>> We just need to make coldboot_done atomic and don't use
> >>> coldboot_lock for coldboot_done. Rest of the things should stay as-is.
> >>
> >> Without the spinlock, there will be problems with coldboot_wait_hmask
> >> if it is used as is. But I do not think that one is necessary. The
> >> boot hart can just send an IPI to all waiting harts. The waiting
> >> harts do not need to clear that mask.
> >
> > There is a contradiction in your statement here. How does boot hart
> > know which all HARTs are waiting for IPI ? In real world systems, we
> > can have faulty parts where certain CPUs never come-up so we can't
> > assume that all CPUs are waiting for WFI.
> >
> > The coldboot_wait_hmask is certainly required and if we use spinlock
> > just for coldboot_wait_hmask then it is fine because it won't be used
> > in a loop.
> >
> > Regards,
> > Anup
> >
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
> >

Anup Patel Aug. 14, 2020, 9:36 a.m. UTC | #30

On Fri, Aug 14, 2020 at 2:50 PM Anup Patel <Anup.Patel@wdc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Sent: 14 August 2020 14:46
> > To: Anup Patel <Anup.Patel@wdc.com>; Damien Le Moal
> > <Damien.LeMoal@wdc.com>; atishp@atishpatra.org; seanga2@gmail.com
> > Cc: Atish Patra <Atish.Patra@wdc.com>; bmeng.cn@gmail.com;
> > jrtc27@jrtc27.com; opensbi@lists.infradead.org; merle@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@wdc.com>
> > >> Sent: 14 August 2020 11:49
> > >> To: Anup Patel <Anup.Patel@wdc.com>; atishp@atishpatra.org;
> > >> xypron.glpk@gmx.de; seanga2@gmail.com
> > >> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
> > >> bmeng.cn@gmail.com; opensbi@lists.infradead.org;
> > >> merle@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) &&


Regards,
Anup

Heinrich Schuchardt Aug. 14, 2020, 9:36 a.m. UTC | #31

On 14.08.20 09:03, Anup Patel wrote:
>
>
>> -----Original Message-----
>> From: Damien Le Moal <Damien.LeMoal@wdc.com>
>> Sent: 14 August 2020 11:49
>> To: Anup Patel <Anup.Patel@wdc.com>; atishp@atishpatra.org;
>> xypron.glpk@gmx.de; seanga2@gmail.com
>> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
>> bmeng.cn@gmail.com; opensbi@lists.infradead.org;
>> merle@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)) {};
>>>
>>> This is over-simplified and will slow down the booting in cold boot path.
>>>
>>> Imagine, boot CPU in cold boot path and all secondary CPUs bombarding
>>> the interconnect with atomic operations.
>>
>> That was just a quick test. With this, the kendryte goes to S mode and start
>> running the fw payload, which does not execute correctly still. So there is
>> another problem somewhere with kendryte platform. Not sure what.
>>
>>>
>>> It has to be a combination of atomic coldboot_done and WFI to avoid
>>> unnecessary traffic in interconnect.
>>>
>>> We just need to make coldboot_done atomic and don't use coldboot_lock
>>> for coldboot_done. Rest of the things should stay as-is.
>>
>> Without the spinlock, there will be problems with coldboot_wait_hmask if it
>> is used as is. But I do not think that one is necessary. The boot hart can just
>> send an IPI to all waiting harts. The waiting harts do not need to clear that
>> mask.

The reason why we need a spin-lock for coldboot_wait_hmask is that it is
defined as bitmask. We could use a char array instead. The current
bitmask is at least sizeof(long). So only on systems with more than 4 or
8 cores we will use extra memory for the mask while saving on code size.

Best regards

Heinrich

>
> There is a contradiction in your statement here. How does boot hart know
> which all HARTs are waiting for IPI ? In real world systems, we can have
> faulty parts where certain CPUs never come-up so we can't assume that
> all CPUs are waiting for WFI.
>
> The coldboot_wait_hmask is certainly required and if we use spinlock
> just for coldboot_wait_hmask then it is fine because it won't be used in
> a loop.
>
> Regards,
> Anup
>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>

Anup Patel Aug. 14, 2020, 9:50 a.m. UTC | #32

> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Sent: 14 August 2020 15:07
> To: Anup Patel <Anup.Patel@wdc.com>; Damien Le Moal
> <Damien.LeMoal@wdc.com>; atishp@atishpatra.org; seanga2@gmail.com
> Cc: Atish Patra <Atish.Patra@wdc.com>; bmeng.cn@gmail.com;
> jrtc27@jrtc27.com; opensbi@lists.infradead.org; merle@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@wdc.com>
> >> Sent: 14 August 2020 11:49
> >> To: Anup Patel <Anup.Patel@wdc.com>; atishp@atishpatra.org;
> >> xypron.glpk@gmx.de; seanga2@gmail.com
> >> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
> >> bmeng.cn@gmail.com; opensbi@lists.infradead.org;
> >> merle@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)) {};
> >>>
> >>> This is over-simplified and will slow down the booting in cold boot path.
> >>>
> >>> Imagine, boot CPU in cold boot path and all secondary CPUs
> >>> bombarding the interconnect with atomic operations.
> >>
> >> That was just a quick test. With this, the kendryte goes to S mode
> >> and start running the fw payload, which does not execute correctly
> >> still. So there is another problem somewhere with kendryte platform. Not
> sure what.
> >>
> >>>
> >>> It has to be a combination of atomic coldboot_done and WFI to avoid
> >>> unnecessary traffic in interconnect.
> >>>
> >>> We just need to make coldboot_done atomic and don't use
> >>> coldboot_lock for coldboot_done. Rest of the things should stay as-is.
> >>
> >> Without the spinlock, there will be problems with coldboot_wait_hmask
> >> if it is used as is. But I do not think that one is necessary. The
> >> boot hart can just send an IPI to all waiting harts. The waiting
> >> harts do not need to clear that mask.
> 
> The reason why we need a spin-lock for coldboot_wait_hmask is that it is
> defined as bitmask. We could use a char array instead. The current bitmask is
> at least sizeof(long). So only on systems with more than 4 or
> 8 cores we will use extra memory for the mask while saving on code size.

OpenSBI supports discontiguous HART ids.

It seems that you are assuming HART ids are contiguous which is not true
for all RISC-V systems. We already have one RISC-V system with 8 CPUs
where HART ids are not contiguous.

Our current limit is max 128 HART ids but this can be easily increasing
by changing SBI_HARTMASK_MAX_BITS define in sbi_hartmask.h

Regards,
Anup

> 
> Best regards
> 
> Heinrich
> 
> >
> > There is a contradiction in your statement here. How does boot hart
> > know which all HARTs are waiting for IPI ? In real world systems, we
> > can have faulty parts where certain CPUs never come-up so we can't
> > assume that all CPUs are waiting for WFI.
> >
> > The coldboot_wait_hmask is certainly required and if we use spinlock
> > just for coldboot_wait_hmask then it is fine because it won't be used
> > in a loop.
> >
> > Regards,
> > Anup
> >
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
> >

Heinrich Schuchardt Aug. 14, 2020, 11:13 a.m. UTC | #33

On 14.08.20 11:36, Anup Patel wrote:
> On Fri, Aug 14, 2020 at 2:50 PM Anup Patel <Anup.Patel@wdc.com> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Sent: 14 August 2020 14:46
>>> To: Anup Patel <Anup.Patel@wdc.com>; Damien Le Moal
>>> <Damien.LeMoal@wdc.com>; atishp@atishpatra.org; seanga2@gmail.com
>>> Cc: Atish Patra <Atish.Patra@wdc.com>; bmeng.cn@gmail.com;
>>> jrtc27@jrtc27.com; opensbi@lists.infradead.org; merle@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@wdc.com>
>>>>> Sent: 14 August 2020 11:49
>>>>> To: Anup Patel <Anup.Patel@wdc.com>; atishp@atishpatra.org;
>>>>> xypron.glpk@gmx.de; seanga2@gmail.com
>>>>> Cc: jrtc27@jrtc27.com; Atish Patra <Atish.Patra@wdc.com>;
>>>>> bmeng.cn@gmail.com; opensbi@lists.infradead.org;
>>>>> merle@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@gmx.de>

>
> Regards,
> Anup
>

Bin Meng Aug. 16, 2020, 6:30 a.m. UTC | #34

On Fri, Aug 14, 2020 at 1:57 AM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 8/13/20 1:45 PM, Atish Patra wrote:
> > On Thu, Aug 13, 2020 at 10:03 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>
> >> On 13 Aug 2020, at 17:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>> On 13.08.20 18:36, Jessica Clarke wrote:
> >>>> On 13 Aug 2020, at 17:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>
> >>>>> OpenSBI hangs in wake_coldboot_harts() indefinitely on the MaixDuino board
> >>>>> when trying to boot U-Boot as FW_PAYLOAD.
> >>>>
> >>>> This smells of there being a more fundamental issue that's not being
> >>>> addressed which this manages to work around, either reliably or by
> >>>> chance. What is the actual deadlock condition you observe?
> >>>
> >>> There are only two function using coldboot_lock spinlock:
> >>>
> >>> * wake_coldboot_harts()
> >>> * wait_for_coldboot()
> >>>
> >>> wake_coldboot_harts() was entered but never left without patching one of
> >>> the functions.
> >>>
> >>> The Kendryte K210 has two harts. One goes the cold boot path while the
> >>> other is sent to the warm boot path.
> >>
> >> The K210's platform definition uses clint_ipi_send, which as far as I
> >> can tell is entirely non-blocking. The only thing wake_coldboot_harts
> >> does with the lock held is set coldboot_done to 1 and send an IPI to
> >> all the other harts. Thus wake_coldboot_harts should never be able to
> >> block holding the spin lock, meaning wait_for_coldboot should
> >> eventually wake up and be able to acquire the lock (on platforms that
> >> implement WFI fully it shouldn't have to wait long for the lock, though
> >> on platforms that make it a NOP there might be a bit of .
> >>
> >
> > Yes. If wake_coldboot_harts is never able to acquire the spinlock,
> > that indicates spinlock
> > is always acquired by wait_for_coldboot. That can happen if
> >
> > 1. WFI is implemented as NOP as suggested by Jessica and MSIP in MIP
> > is set which
> > shouldn't happen unless there is an IPI sent.
> >
> > Can you try clearing the MIP just before acquiring the lock ? In qemu
> > & unleashed, modifying the MSIP bit in
> > MIP doesn't actually do anything. So we do sbi_platform_ipi_clear  to reset it.
>
> I second this suggestion. I had a similar problem when porting U-Boot.
>
> https://gitlab.denx.de/u-boot/u-boot/-/commit/9472630337e7c4ac442066b5a752aaa8c3b4d4a6

I am a bit confused. Atish said: "modifying the MSIP bit in MIP
doesn't actually do anything", but the U-Boot commit did solve the
problem? How?

Also I am lost why Heinrich is seeing the problem? I thought the
U-Boot upstream work is working on Kendyte K210, no?

Regards,
Bin

[1/1] lib: sbi: relax scoldboot_lock spinlock

Commit Message

Comments

Patch