[U-Boot] [PATCH 1/7] riscv: add infrastructure for calling functions on other harts

Auer, Lukas lukas.auer at aisec.fraunhofer.de
Mon Feb 25 11:13:02 UTC 2019


On Tue, 2019-02-19 at 13:46 +0530, Anup Patel wrote:
> On Mon, Feb 18, 2019 at 5:11 PM Auer, Lukas
> <lukas.auer at aisec.fraunhofer.de> wrote:
> > On Mon, 2019-02-18 at 10:01 +0000, Auer, Lukas wrote:
> > > On Mon, 2019-02-18 at 10:28 +0530, Anup Patel wrote:
> > > > On Tue, Feb 12, 2019 at 3:44 AM Lukas Auer
> > > > <lukas.auer at aisec.fraunhofer.de> wrote:
> > > > > Harts on RISC-V boot independently and U-Boot is responsible
> > > > > for
> > > > > managing them. Functions are called on other harts with
> > > > > smp_call_function(), which sends inter-processor interrupts
> > > > > (IPIs)
> > > > > to
> > > > > all other harts. Functions are specified with their address
> > > > > and
> > > > > two
> > > > > function arguments (argument 2 and 3). The first function
> > > > > argument
> > > > > is
> > > > > always the hart ID of the hart calling the function. On the
> > > > > other
> > > > > harts,
> > > > > the IPI interrupt handler handle_ipi() must be called on
> > > > > software
> > > > > interrupts to handle the request and call the specified
> > > > > function.
> > > > > 
> > > > > Functions are stored in the ipi_data data structure. Every
> > > > > hart
> > > > > has
> > > > > its
> > > > > own data structure in global data. While this is not required
> > > > > at
> > > > > the
> > > > > moment (all harts are expected to boot Linux), this does
> > > > > allow
> > > > > future
> > > > > expansion, where other harts may be used for monitoring or
> > > > > other
> > > > > tasks.
> > > > > 
> > > > > Signed-off-by: Lukas Auer <lukas.auer at aisec.fraunhofer.de>
> > > > > ---
> > > > > 
> > > > >  arch/riscv/Kconfig                   |  19 +++++
> > > > >  arch/riscv/include/asm/global_data.h |   5 ++
> > > > >  arch/riscv/include/asm/smp.h         |  53 +++++++++++++
> > > > >  arch/riscv/lib/Makefile              |   1 +
> > > > >  arch/riscv/lib/smp.c                 | 110
> > > > > +++++++++++++++++++++++++++
> > > > >  5 files changed, 188 insertions(+)
> > > > >  create mode 100644 arch/riscv/include/asm/smp.h
> > > > >  create mode 100644 arch/riscv/lib/smp.c
> > > > > 
> > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > index c45e4d73a8..c0842178dd 100644
> > > > > --- a/arch/riscv/Kconfig
> > > > > +++ b/arch/riscv/Kconfig
> > > > > @@ -116,4 +116,23 @@ config RISCV_RDTIME
> > > > >  config SYS_MALLOC_F_LEN
> > > > >         default 0x1000
> > > > > 
> > > > > +config SMP
> > > > > +       bool "Symmetric Multi-Processing"
> > > > > +       help
> > > > > +         This enables support for systems with more than one
> > > > > CPU.
> > > > > If
> > > > > +         you say N here, U-Boot will run on single and
> > > > > multiprocessor
> > > > > +         machines, but will use only one CPU of a
> > > > > multiprocessor
> > > > > +         machine. If you say Y here, U-Boot will run on
> > > > > many,
> > > > > but
> > > > > not
> > > > > +         all, single processor machines.
> > > > > +
> > > > > +config NR_CPUS
> > > > > +       int "Maximum number of CPUs (2-32)"
> > > > > +       range 2 32
> > > > > +       depends on SMP
> > > > > +       default "8"
> > > > > +       help
> > > > > +         On multiprocessor machines, U-Boot sets up a stack
> > > > > for
> > > > > each CPU.
> > > > > +         Stack memory is pre-allocated. U-Boot must
> > > > > therefore
> > > > > know
> > > > > the
> > > > > +         maximum number of CPUs that may be present.
> > > > > +
> > > > >  endmenu
> > > > > diff --git a/arch/riscv/include/asm/global_data.h
> > > > > b/arch/riscv/include/asm/global_data.h
> > > > > index a3a342c6e1..23a5f35af5 100644
> > > > > --- a/arch/riscv/include/asm/global_data.h
> > > > > +++ b/arch/riscv/include/asm/global_data.h
> > > > > @@ -10,12 +10,17 @@
> > > > >  #ifndef        __ASM_GBL_DATA_H
> > > > >  #define __ASM_GBL_DATA_H
> > > > > 
> > > > > +#include <asm/smp.h>
> > > > > +
> > > > >  /* Architecture-specific global data */
> > > > >  struct arch_global_data {
> > > > >         long boot_hart;         /* boot hart id */
> > > > >  #ifdef CONFIG_SIFIVE_CLINT
> > > > >         void __iomem *clint;    /* clint base address */
> > > > >  #endif
> > > > > +#ifdef CONFIG_SMP
> > > > > +       struct ipi_data ipi[CONFIG_NR_CPUS];
> > > > 
> > > > The data passed by "main HART" via global data to other HARTs
> > > > is not visible reliably and randomly few HARTs fail to enter
> > > > Linux
> > > > kernel on my board. I am suspecting per-HART "ipi" data in
> > > > global
> > > > data not being cache-line aligned as the cause of behavior but
> > > > there
> > > > could be other issue too.
> > > > 
> > > > I have a hack which works reliable for me. As-per this hack, we
> > > > add
> > > > "mdelay(10)" just before calling riscv_send_ipi() in
> > > > send_ipi_many().
> > > > This hack helped me conclude that there is some sync issue in
> > > > per-
> > > > HART
> > > > "ipi" data.
> > > > 
> > > > The above issue is not seen on QEMU so we are fine there.
> > > > 
> > > > I would suggest to make per-HART "ipi" data cache-line aligned
> > > > (just like Linux kernel).
> > > > 
> > > 
> > > Interesting, this is definitely a memory coherency issue,
> > > probably
> > > inadequate fences. I am not sure if making the ipi data structure
> > > cache-line aligned will reliably fix it. I'll try to replicate
> > > the
> > > issue and get it fixed. Thanks for reporting it!
> > > 
> > 
> > Not sure if it is connected, but I have noticed a regression with
> > the
> > current OpenSBI. Testing U-Boot with the SMP patches applied on
> > QEMU
> > with 4 harts, hart 4 will not receive software interrupts. I have
> > bisected the issue to the following commit.
> > 
> 
> It's not related to OpenSBI IPI issue we fixed recently. I was seeing
> this issue before OpenSBI IPI breakage.
> 
> I tried again with latest OpenSBI and it worked for me 8 out-of 10
> times.
> 
> Strangely, Atish and you don't see this issue. Maybe it's just my
> board.
> 
> In any case, it will be good to have:
> 1. Few more fences (taking reference from Linux code)
> 2. Ensure per-HART "ipi" data is cache line aligned
> 
> Maybe with above things in-place, it will become unlikely for
> me to see this issue.
> 
> You can even address this issue as separate patches later
> since it's only me seeing it occasionally.
> 

I was unable to reproduce this issue. I did however see strange
behavior when using the reset button to reset the board [1]. Did you
use the reset button to reset the board?

In any case, I will move the memory barrier to the handle_ipi()
function. It should not be needed in send_ipi_many, since the IO access
function already include suitable memory barriers. We do not have those
on the handle_ipi side, so this could actually be the cause of the
issue you are seeing.

Thanks,
Lukas

[1]: https://github.com/riscv/opensbi/issues/65


More information about the U-Boot mailing list