| Message ID | 20200813162415.124469-1-xypron.glpk@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] lib: sbi: relax scoldboot_lock spinlock | expand |
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
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
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
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
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
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 >
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
> -----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
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
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
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 >
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
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
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 >
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
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
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 > >
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 > > > > >
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 >>> >>> >> > >
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
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
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
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
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
> -----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
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.
> -----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
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 >
> -----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 > >
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
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 >
> -----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 > >
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 >
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
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);
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