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