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

[Damien Le Moal]

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 at 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


-- 
Damien Le Moal
Western Digital Research