diff mbox series

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

Message ID 20190211221345.31980-2-lukas.auer@aisec.fraunhofer.de
State Superseded
Delegated to: Andes
Headers show
Series SMP support for RISC-V | expand

Commit Message

Lukas Auer Feb. 11, 2019, 10:13 p.m. UTC
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@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

Comments

Anup Patel Feb. 12, 2019, 1:44 a.m. UTC | #1
> -----Original Message-----
> From: Lukas Auer [mailto:lukas.auer@aisec.fraunhofer.de]
> Sent: Tuesday, February 12, 2019 3:44 AM
> To: u-boot@lists.denx.de
> Cc: Atish Patra <Atish.Patra@wdc.com>; Anup Patel
> <Anup.Patel@wdc.com>; Bin Meng <bmeng.cn@gmail.com>; Andreas
> Schwab <schwab@suse.de>; Palmer Dabbelt <palmer@sifive.com>;
> Alexander Graf <agraf@suse.de>; Lukas Auer
> <lukas.auer@aisec.fraunhofer.de>; Anup Patel <anup@brainfault.org>; Rick
> Chen <rick@andestech.com>
> Subject: [PATCH 1/7] riscv: add infrastructure for calling functions on other
> harts
> 
> 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@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];
> +#endif
>  };
> 
>  #include <asm-generic/global_data.h>
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> new file mode 100644 index 0000000000..bc863fdbaf
> --- /dev/null
> +++ b/arch/riscv/include/asm/smp.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Fraunhofer AISEC,
> + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> +
> +#ifndef _ASM_RISCV_SMP_H
> +#define _ASM_RISCV_SMP_H
> +
> +/**
> + * struct ipi_data - Inter-processor interrupt (IPI) data structure
> + *
> + * IPIs are used for SMP support to communicate to other harts what
> +function to
> + * call. Functions are in the form
> + * void (*addr)(ulong hart, ulong arg0, ulong arg1).
> + *
> + * The function address and the two arguments, arg0 and arg1, are
> +stored in the
> + * IPI data structure. The hart ID is inserted by the hart handling the
> +IPI and
> + * calling the function.
> + *
> + * @addr: Address of function
> + * @arg0: First argument of function
> + * @arg1: Second argument of function
> + */
> +struct ipi_data {
> +	ulong addr;
> +	ulong arg0;
> +	ulong arg1;
> +};
> +
> +/**
> + * handle_ipi() - interrupt handler for software interrupts
> + *
> + * The IPI interrupt handler must be called to handle software
> +interrupts. It
> + * calls the function specified in the hart's IPI data structure.
> + *
> + * @hart: Hart ID of the current hart
> + */
> +void handle_ipi(ulong hart);
> +
> +/**
> + * smp_call_function() - Call a function on all other harts
> + *
> + * Send IPIs with the specified function call to all harts.
> + *
> + * @addr: Address of function
> + * @arg0: First argument of function
> + * @arg1: Second argument of function
> + * @return 0 if OK, -ve on error
> + */
> +int smp_call_function(ulong addr, ulong arg0, ulong arg1);
> +
> +#endif
> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index
> edfa61690c..19370f9749 100644
> --- a/arch/riscv/lib/Makefile
> +++ b/arch/riscv/lib/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
>  obj-y	+= interrupts.o
>  obj-y	+= reset.o
>  obj-y   += setjmp.o
> +obj-$(CONFIG_SMP) += smp.o
> 
>  # For building EFI apps
>  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c new file mode 100644
> index 0000000000..1266a2a0ef
> --- /dev/null
> +++ b/arch/riscv/lib/smp.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Fraunhofer AISEC,
> + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/barrier.h>
> +#include <asm/smp.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/**
> + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> + *
> + * Platform code must provide this function.
> + *
> + * @hart: Hart ID of receiving hart
> + * @return 0 if OK, -ve on error
> + */
> +extern int riscv_send_ipi(int hart);
> +
> +/**
> + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
> + *
> + * Platform code must provide this function.
> + *
> + * @hart: Hart ID of hart to be cleared
> + * @return 0 if OK, -ve on error
> + */
> +extern int riscv_clear_ipi(int hart);
> +
> +static int send_ipi_many(struct ipi_data *ipi) {
> +	ofnode node, cpus;
> +	u32 reg;
> +	int ret;
> +
> +	cpus = ofnode_path("/cpus");
> +	if (!ofnode_valid(cpus)) {
> +		pr_err("Can't find cpus node!\n");
> +		return -EINVAL;
> +	}
> +
> +	ofnode_for_each_subnode(node, cpus) {
> +		if (!ofnode_is_available(node))
> +			continue;

It is not correct to assume that whatever CPUs are marked
available will come online. It is possible that certain available
CPUs failed to come online due HW failure.

Better approach would be keep an atomic bitmask of HARTs
that have entered U-Boot. All HARTs that enter U-Boot will
update the atomic HART available bitmask. We send IPI only
to HARTs that are available as-per atomic HART bitmask.

> +
> +		/* read hart ID of CPU */
> +		ret = ofnode_read_u32(node, "reg", &reg);
> +		if (ret)
> +			continue;
> +
> +		/* skip if it is the hart we are running on */
> +		if (reg == gd->arch.boot_hart)
> +			continue;
> +
> +		if (reg >= CONFIG_NR_CPUS) {
> +			pr_err("Hart ID %d is out of range, increase
> CONFIG_NR_CPUS\n",
> +			       reg);
> +			continue;
> +		}
> +
> +		gd->arch.ipi[reg].addr = ipi->addr;
> +		gd->arch.ipi[reg].arg0 = ipi->arg0;
> +		gd->arch.ipi[reg].arg1 = ipi->arg1;
> +		mb();
> +
> +		ret = riscv_send_ipi(reg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void handle_ipi(ulong hart)
> +{
> +	int ret;
> +	void (*smp_function)(ulong hart, ulong arg0, ulong arg1);
> +
> +	if (hart >= CONFIG_NR_CPUS)
> +		return;
> +
> +	ret = riscv_clear_ipi(hart);
> +	if (ret) {
> +		pr_err("Cannot clear IPI\n");
> +		return;
> +	}
> +
> +	smp_function = (void (*)(ulong, ulong, ulong))gd-
> >arch.ipi[hart].addr;
> +	invalidate_icache_all();
> +
> +	smp_function(hart, gd->arch.ipi[hart].arg0, gd->arch.ipi[hart].arg1);
> +}
> +
> +int smp_call_function(ulong addr, ulong arg0, ulong arg1) {
> +	int ret = 0;
> +	struct ipi_data ipi;
> +
> +	ipi.addr = addr;
> +	ipi.arg0 = arg0;
> +	ipi.arg1 = arg1;
> +
> +	ret = send_ipi_many(&ipi);
> +
> +	return ret;
> +}
> --
> 2.20.1

Regards,
Anup
Bin Meng Feb. 12, 2019, 3:03 a.m. UTC | #2
Hi Lukas,

On Tue, Feb 12, 2019 at 6:14 AM Lukas Auer
<lukas.auer@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@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
>

Looks pretty clean to me. Thanks! Some nits below.

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

U-Boot will run on both single and multiprocessor machines?

> +
> +config NR_CPUS
> +       int "Maximum number of CPUs (2-32)"
> +       range 2 32
> +       depends on SMP
> +       default "8"

no quotation mark?

> +       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];
> +#endif
>  };
>
>  #include <asm-generic/global_data.h>
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> new file mode 100644
> index 0000000000..bc863fdbaf
> --- /dev/null
> +++ b/arch/riscv/include/asm/smp.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Fraunhofer AISEC,
> + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> + */
> +
> +#ifndef _ASM_RISCV_SMP_H
> +#define _ASM_RISCV_SMP_H
> +
> +/**
> + * struct ipi_data - Inter-processor interrupt (IPI) data structure
> + *
> + * IPIs are used for SMP support to communicate to other harts what function to
> + * call. Functions are in the form
> + * void (*addr)(ulong hart, ulong arg0, ulong arg1).
> + *
> + * The function address and the two arguments, arg0 and arg1, are stored in the
> + * IPI data structure. The hart ID is inserted by the hart handling the IPI and
> + * calling the function.
> + *
> + * @addr: Address of function
> + * @arg0: First argument of function
> + * @arg1: Second argument of function
> + */
> +struct ipi_data {
> +       ulong addr;
> +       ulong arg0;
> +       ulong arg1;
> +};
> +
> +/**
> + * handle_ipi() - interrupt handler for software interrupts
> + *
> + * The IPI interrupt handler must be called to handle software interrupts. It
> + * calls the function specified in the hart's IPI data structure.
> + *
> + * @hart: Hart ID of the current hart
> + */
> +void handle_ipi(ulong hart);
> +
> +/**
> + * smp_call_function() - Call a function on all other harts
> + *
> + * Send IPIs with the specified function call to all harts.
> + *
> + * @addr: Address of function
> + * @arg0: First argument of function
> + * @arg1: Second argument of function
> + * @return 0 if OK, -ve on error
> + */
> +int smp_call_function(ulong addr, ulong arg0, ulong arg1);
> +
> +#endif
> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> index edfa61690c..19370f9749 100644
> --- a/arch/riscv/lib/Makefile
> +++ b/arch/riscv/lib/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
>  obj-y  += interrupts.o
>  obj-y  += reset.o
>  obj-y   += setjmp.o
> +obj-$(CONFIG_SMP) += smp.o
>
>  # For building EFI apps
>  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c
> new file mode 100644
> index 0000000000..1266a2a0ef
> --- /dev/null
> +++ b/arch/riscv/lib/smp.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Fraunhofer AISEC,
> + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/barrier.h>
> +#include <asm/smp.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/**
> + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> + *
> + * Platform code must provide this function.
> + *
> + * @hart: Hart ID of receiving hart
> + * @return 0 if OK, -ve on error
> + */
> +extern int riscv_send_ipi(int hart);
> +
> +/**
> + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
> + *
> + * Platform code must provide this function.
> + *
> + * @hart: Hart ID of hart to be cleared
> + * @return 0 if OK, -ve on error
> + */
> +extern int riscv_clear_ipi(int hart);
> +
> +static int send_ipi_many(struct ipi_data *ipi)
> +{
> +       ofnode node, cpus;
> +       u32 reg;
> +       int ret;
> +
> +       cpus = ofnode_path("/cpus");
> +       if (!ofnode_valid(cpus)) {
> +               pr_err("Can't find cpus node!\n");
> +               return -EINVAL;
> +       }
> +
> +       ofnode_for_each_subnode(node, cpus) {
> +               if (!ofnode_is_available(node))
> +                       continue;
> +
> +               /* read hart ID of CPU */
> +               ret = ofnode_read_u32(node, "reg", &reg);
> +               if (ret)
> +                       continue;
> +
> +               /* skip if it is the hart we are running on */
> +               if (reg == gd->arch.boot_hart)
> +                       continue;
> +
> +               if (reg >= CONFIG_NR_CPUS) {
> +                       pr_err("Hart ID %d is out of range, increase CONFIG_NR_CPUS\n",
> +                              reg);
> +                       continue;
> +               }
> +
> +               gd->arch.ipi[reg].addr = ipi->addr;
> +               gd->arch.ipi[reg].arg0 = ipi->arg0;
> +               gd->arch.ipi[reg].arg1 = ipi->arg1;
> +               mb();
> +
> +               ret = riscv_send_ipi(reg);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +void handle_ipi(ulong hart)
> +{
> +       int ret;
> +       void (*smp_function)(ulong hart, ulong arg0, ulong arg1);
> +
> +       if (hart >= CONFIG_NR_CPUS)
> +               return;
> +
> +       ret = riscv_clear_ipi(hart);
> +       if (ret) {
> +               pr_err("Cannot clear IPI\n");
> +               return;
> +       }
> +
> +       smp_function = (void (*)(ulong, ulong, ulong))gd->arch.ipi[hart].addr;
> +       invalidate_icache_all();
> +
> +       smp_function(hart, gd->arch.ipi[hart].arg0, gd->arch.ipi[hart].arg1);
> +}
> +
> +int smp_call_function(ulong addr, ulong arg0, ulong arg1)
> +{
> +       int ret = 0;
> +       struct ipi_data ipi;
> +
> +       ipi.addr = addr;
> +       ipi.arg0 = arg0;
> +       ipi.arg1 = arg1;
> +
> +       ret = send_ipi_many(&ipi);
> +
> +       return ret;
> +}
> --

Regards,
Bin
Lukas Auer Feb. 17, 2019, 9:55 p.m. UTC | #3
On Tue, 2019-02-12 at 01:44 +0000, Anup Patel wrote:
> > -----Original Message-----
> > From: Lukas Auer [mailto:lukas.auer@aisec.fraunhofer.de]
> > Sent: Tuesday, February 12, 2019 3:44 AM
> > To: u-boot@lists.denx.de
> > Cc: Atish Patra <Atish.Patra@wdc.com>; Anup Patel
> > <Anup.Patel@wdc.com>; Bin Meng <bmeng.cn@gmail.com>; Andreas
> > Schwab <schwab@suse.de>; Palmer Dabbelt <palmer@sifive.com>;
> > Alexander Graf <agraf@suse.de>; Lukas Auer
> > <lukas.auer@aisec.fraunhofer.de>; Anup Patel <anup@brainfault.org>;
> > Rick
> > Chen <rick@andestech.com>
> > Subject: [PATCH 1/7] riscv: add infrastructure for calling
> > functions on other
> > harts
> > 
> > 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@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];
> > +#endif
> >  };
> > 
> >  #include <asm-generic/global_data.h>
> > diff --git a/arch/riscv/include/asm/smp.h
> > b/arch/riscv/include/asm/smp.h
> > new file mode 100644 index 0000000000..bc863fdbaf
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/smp.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2019 Fraunhofer AISEC,
> > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > +
> > +#ifndef _ASM_RISCV_SMP_H
> > +#define _ASM_RISCV_SMP_H
> > +
> > +/**
> > + * struct ipi_data - Inter-processor interrupt (IPI) data
> > structure
> > + *
> > + * IPIs are used for SMP support to communicate to other harts
> > what
> > +function to
> > + * call. Functions are in the form
> > + * void (*addr)(ulong hart, ulong arg0, ulong arg1).
> > + *
> > + * The function address and the two arguments, arg0 and arg1, are
> > +stored in the
> > + * IPI data structure. The hart ID is inserted by the hart
> > handling the
> > +IPI and
> > + * calling the function.
> > + *
> > + * @addr: Address of function
> > + * @arg0: First argument of function
> > + * @arg1: Second argument of function
> > + */
> > +struct ipi_data {
> > +	ulong addr;
> > +	ulong arg0;
> > +	ulong arg1;
> > +};
> > +
> > +/**
> > + * handle_ipi() - interrupt handler for software interrupts
> > + *
> > + * The IPI interrupt handler must be called to handle software
> > +interrupts. It
> > + * calls the function specified in the hart's IPI data structure.
> > + *
> > + * @hart: Hart ID of the current hart
> > + */
> > +void handle_ipi(ulong hart);
> > +
> > +/**
> > + * smp_call_function() - Call a function on all other harts
> > + *
> > + * Send IPIs with the specified function call to all harts.
> > + *
> > + * @addr: Address of function
> > + * @arg0: First argument of function
> > + * @arg1: Second argument of function
> > + * @return 0 if OK, -ve on error
> > + */
> > +int smp_call_function(ulong addr, ulong arg0, ulong arg1);
> > +
> > +#endif
> > diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> > index
> > edfa61690c..19370f9749 100644
> > --- a/arch/riscv/lib/Makefile
> > +++ b/arch/riscv/lib/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
> >  obj-y	+= interrupts.o
> >  obj-y	+= reset.o
> >  obj-y   += setjmp.o
> > +obj-$(CONFIG_SMP) += smp.o
> > 
> >  # For building EFI apps
> >  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> > diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c new file
> > mode 100644
> > index 0000000000..1266a2a0ef
> > --- /dev/null
> > +++ b/arch/riscv/lib/smp.c
> > @@ -0,0 +1,110 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2019 Fraunhofer AISEC,
> > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <asm/barrier.h>
> > +#include <asm/smp.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/**
> > + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> > + *
> > + * Platform code must provide this function.
> > + *
> > + * @hart: Hart ID of receiving hart
> > + * @return 0 if OK, -ve on error
> > + */
> > +extern int riscv_send_ipi(int hart);
> > +
> > +/**
> > + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
> > + *
> > + * Platform code must provide this function.
> > + *
> > + * @hart: Hart ID of hart to be cleared
> > + * @return 0 if OK, -ve on error
> > + */
> > +extern int riscv_clear_ipi(int hart);
> > +
> > +static int send_ipi_many(struct ipi_data *ipi) {
> > +	ofnode node, cpus;
> > +	u32 reg;
> > +	int ret;
> > +
> > +	cpus = ofnode_path("/cpus");
> > +	if (!ofnode_valid(cpus)) {
> > +		pr_err("Can't find cpus node!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ofnode_for_each_subnode(node, cpus) {
> > +		if (!ofnode_is_available(node))
> > +			continue;
> 
> It is not correct to assume that whatever CPUs are marked
> available will come online. It is possible that certain available
> CPUs failed to come online due HW failure.
> 

This was intended so that we don't send IPIs to harts, which have been
explicitly marked as disabled.

> Better approach would be keep an atomic bitmask of HARTs
> that have entered U-Boot. All HARTs that enter U-Boot will
> update the atomic HART available bitmask. We send IPI only
> to HARTs that are available as-per atomic HART bitmask.
> 

I'm not sure if this is required in U-Boot, since we are not relying on
all harts to boot for U-Boot to function. We only try to boot all harts
listed as available in the device tree.

Is there a situation, in which we would want to avoid sending an IPI to
an unavailable hart?

Thanks,
Lukas

> > +
> > +		/* read hart ID of CPU */
> > +		ret = ofnode_read_u32(node, "reg", &reg);
> > +		if (ret)
> > +			continue;
> > +
> > +		/* skip if it is the hart we are running on */
> > +		if (reg == gd->arch.boot_hart)
> > +			continue;
> > +
> > +		if (reg >= CONFIG_NR_CPUS) {
> > +			pr_err("Hart ID %d is out of range, increase
> > CONFIG_NR_CPUS\n",
> > +			       reg);
> > +			continue;
> > +		}
> > +
> > +		gd->arch.ipi[reg].addr = ipi->addr;
> > +		gd->arch.ipi[reg].arg0 = ipi->arg0;
> > +		gd->arch.ipi[reg].arg1 = ipi->arg1;
> > +		mb();
> > +
> > +		ret = riscv_send_ipi(reg);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void handle_ipi(ulong hart)
> > +{
> > +	int ret;
> > +	void (*smp_function)(ulong hart, ulong arg0, ulong arg1);
> > +
> > +	if (hart >= CONFIG_NR_CPUS)
> > +		return;
> > +
> > +	ret = riscv_clear_ipi(hart);
> > +	if (ret) {
> > +		pr_err("Cannot clear IPI\n");
> > +		return;
> > +	}
> > +
> > +	smp_function = (void (*)(ulong, ulong, ulong))gd-
> > > arch.ipi[hart].addr;
> > +	invalidate_icache_all();
> > +
> > +	smp_function(hart, gd->arch.ipi[hart].arg0, gd-
> > >arch.ipi[hart].arg1);
> > +}
> > +
> > +int smp_call_function(ulong addr, ulong arg0, ulong arg1) {
> > +	int ret = 0;
> > +	struct ipi_data ipi;
> > +
> > +	ipi.addr = addr;
> > +	ipi.arg0 = arg0;
> > +	ipi.arg1 = arg1;
> > +
> > +	ret = send_ipi_many(&ipi);
> > +
> > +	return ret;
> > +}
> > --
> > 2.20.1
> 
> Regards,
> Anup
Lukas Auer Feb. 17, 2019, 10 p.m. UTC | #4
Hi Bin,

On Tue, 2019-02-12 at 11:03 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Tue, Feb 12, 2019 at 6:14 AM Lukas Auer
> <lukas.auer@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@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
> > 
> 
> Looks pretty clean to me. Thanks! Some nits below.
> 

Thank you, and thanks for your review!

> > 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.
> 
> U-Boot will run on both single and multiprocessor machines?
> 

I simply adapted the help text used in the Linux kernel here. Should I
rephrase it?

> > +
> > +config NR_CPUS
> > +       int "Maximum number of CPUs (2-32)"
> > +       range 2 32
> > +       depends on SMP
> > +       default "8"
> 
> no quotation mark?
> 

Yes, I will remove them.

Thanks,
Lukas

> > +       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];
> > +#endif
> >  };
> > 
> >  #include <asm-generic/global_data.h>
> > diff --git a/arch/riscv/include/asm/smp.h
> > b/arch/riscv/include/asm/smp.h
> > new file mode 100644
> > index 0000000000..bc863fdbaf
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/smp.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2019 Fraunhofer AISEC,
> > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> > + */
> > +
> > +#ifndef _ASM_RISCV_SMP_H
> > +#define _ASM_RISCV_SMP_H
> > +
> > +/**
> > + * struct ipi_data - Inter-processor interrupt (IPI) data
> > structure
> > + *
> > + * IPIs are used for SMP support to communicate to other harts
> > what function to
> > + * call. Functions are in the form
> > + * void (*addr)(ulong hart, ulong arg0, ulong arg1).
> > + *
> > + * The function address and the two arguments, arg0 and arg1, are
> > stored in the
> > + * IPI data structure. The hart ID is inserted by the hart
> > handling the IPI and
> > + * calling the function.
> > + *
> > + * @addr: Address of function
> > + * @arg0: First argument of function
> > + * @arg1: Second argument of function
> > + */
> > +struct ipi_data {
> > +       ulong addr;
> > +       ulong arg0;
> > +       ulong arg1;
> > +};
> > +
> > +/**
> > + * handle_ipi() - interrupt handler for software interrupts
> > + *
> > + * The IPI interrupt handler must be called to handle software
> > interrupts. It
> > + * calls the function specified in the hart's IPI data structure.
> > + *
> > + * @hart: Hart ID of the current hart
> > + */
> > +void handle_ipi(ulong hart);
> > +
> > +/**
> > + * smp_call_function() - Call a function on all other harts
> > + *
> > + * Send IPIs with the specified function call to all harts.
> > + *
> > + * @addr: Address of function
> > + * @arg0: First argument of function
> > + * @arg1: Second argument of function
> > + * @return 0 if OK, -ve on error
> > + */
> > +int smp_call_function(ulong addr, ulong arg0, ulong arg1);
> > +
> > +#endif
> > diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> > index edfa61690c..19370f9749 100644
> > --- a/arch/riscv/lib/Makefile
> > +++ b/arch/riscv/lib/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
> >  obj-y  += interrupts.o
> >  obj-y  += reset.o
> >  obj-y   += setjmp.o
> > +obj-$(CONFIG_SMP) += smp.o
> > 
> >  # For building EFI apps
> >  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> > diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c
> > new file mode 100644
> > index 0000000000..1266a2a0ef
> > --- /dev/null
> > +++ b/arch/riscv/lib/smp.c
> > @@ -0,0 +1,110 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2019 Fraunhofer AISEC,
> > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <asm/barrier.h>
> > +#include <asm/smp.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/**
> > + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> > + *
> > + * Platform code must provide this function.
> > + *
> > + * @hart: Hart ID of receiving hart
> > + * @return 0 if OK, -ve on error
> > + */
> > +extern int riscv_send_ipi(int hart);
> > +
> > +/**
> > + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
> > + *
> > + * Platform code must provide this function.
> > + *
> > + * @hart: Hart ID of hart to be cleared
> > + * @return 0 if OK, -ve on error
> > + */
> > +extern int riscv_clear_ipi(int hart);
> > +
> > +static int send_ipi_many(struct ipi_data *ipi)
> > +{
> > +       ofnode node, cpus;
> > +       u32 reg;
> > +       int ret;
> > +
> > +       cpus = ofnode_path("/cpus");
> > +       if (!ofnode_valid(cpus)) {
> > +               pr_err("Can't find cpus node!\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ofnode_for_each_subnode(node, cpus) {
> > +               if (!ofnode_is_available(node))
> > +                       continue;
> > +
> > +               /* read hart ID of CPU */
> > +               ret = ofnode_read_u32(node, "reg", &reg);
> > +               if (ret)
> > +                       continue;
> > +
> > +               /* skip if it is the hart we are running on */
> > +               if (reg == gd->arch.boot_hart)
> > +                       continue;
> > +
> > +               if (reg >= CONFIG_NR_CPUS) {
> > +                       pr_err("Hart ID %d is out of range,
> > increase CONFIG_NR_CPUS\n",
> > +                              reg);
> > +                       continue;
> > +               }
> > +
> > +               gd->arch.ipi[reg].addr = ipi->addr;
> > +               gd->arch.ipi[reg].arg0 = ipi->arg0;
> > +               gd->arch.ipi[reg].arg1 = ipi->arg1;
> > +               mb();
> > +
> > +               ret = riscv_send_ipi(reg);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +void handle_ipi(ulong hart)
> > +{
> > +       int ret;
> > +       void (*smp_function)(ulong hart, ulong arg0, ulong arg1);
> > +
> > +       if (hart >= CONFIG_NR_CPUS)
> > +               return;
> > +
> > +       ret = riscv_clear_ipi(hart);
> > +       if (ret) {
> > +               pr_err("Cannot clear IPI\n");
> > +               return;
> > +       }
> > +
> > +       smp_function = (void (*)(ulong, ulong, ulong))gd-
> > >arch.ipi[hart].addr;
> > +       invalidate_icache_all();
> > +
> > +       smp_function(hart, gd->arch.ipi[hart].arg0, gd-
> > >arch.ipi[hart].arg1);
> > +}
> > +
> > +int smp_call_function(ulong addr, ulong arg0, ulong arg1)
> > +{
> > +       int ret = 0;
> > +       struct ipi_data ipi;
> > +
> > +       ipi.addr = addr;
> > +       ipi.arg0 = arg0;
> > +       ipi.arg1 = arg1;
> > +
> > +       ret = send_ipi_many(&ipi);
> > +
> > +       return ret;
> > +}
> > --
> 
> Regards,
> Bin
Rick Chen Feb. 18, 2019, 3:24 a.m. UTC | #5
<rick@andestech.com> 於 2019年2月18日 週一 上午11:00寫道:
>
>
>
> > -----Original Message-----
> > From: Auer, Lukas [mailto:lukas.auer@aisec.fraunhofer.de]
> > Sent: Monday, February 18, 2019 5:55 AM
> > To: u-boot@lists.denx.de; Anup.Patel@wdc.com
> > Cc: anup@brainfault.org; bmeng.cn@gmail.com; schwab@suse.de; Rick Jian-Zhi
> > Chen(陳建志); palmer@sifive.com; Atish.Patra@wdc.com; agraf@suse.de
> > Subject: Re: [PATCH 1/7] riscv: add infrastructure for calling functions on other
> > harts
> >
> > On Tue, 2019-02-12 at 01:44 +0000, Anup Patel wrote:
> > > > -----Original Message-----
> > > > From: Lukas Auer [mailto:lukas.auer@aisec.fraunhofer.de]
> > > > Sent: Tuesday, February 12, 2019 3:44 AM
> > > > To: u-boot@lists.denx.de
> > > > Cc: Atish Patra <Atish.Patra@wdc.com>; Anup Patel
> > > > <Anup.Patel@wdc.com>; Bin Meng <bmeng.cn@gmail.com>; Andreas
> > Schwab
> > > > <schwab@suse.de>; Palmer Dabbelt <palmer@sifive.com>; Alexander Graf
> > > > <agraf@suse.de>; Lukas Auer <lukas.auer@aisec.fraunhofer.de>; Anup
> > > > Patel <anup@brainfault.org>; Rick Chen <rick@andestech.com>
> > > > Subject: [PATCH 1/7] riscv: add infrastructure for calling functions
> > > > on other harts
> > > >
> > > > 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@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]; #endif
> > > >  };
> > > >
> > > >  #include <asm-generic/global_data.h>
> > > > diff --git a/arch/riscv/include/asm/smp.h
> > > > b/arch/riscv/include/asm/smp.h
> > > > new file mode 100644 index 0000000000..bc863fdbaf
> > > > --- /dev/null
> > > > +++ b/arch/riscv/include/asm/smp.h
> > > > @@ -0,0 +1,53 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Copyright (C) 2019 Fraunhofer AISEC,
> > > > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > > > +
> > > > +#ifndef _ASM_RISCV_SMP_H
> > > > +#define _ASM_RISCV_SMP_H
> > > > +
> > > > +/**
> > > > + * struct ipi_data - Inter-processor interrupt (IPI) data
> > > > structure
> > > > + *
> > > > + * IPIs are used for SMP support to communicate to other harts
> > > > what
> > > > +function to
> > > > + * call. Functions are in the form
> > > > + * void (*addr)(ulong hart, ulong arg0, ulong arg1).
> > > > + *
> > > > + * The function address and the two arguments, arg0 and arg1, are
> > > > +stored in the
> > > > + * IPI data structure. The hart ID is inserted by the hart
> > > > handling the
> > > > +IPI and
> > > > + * calling the function.
> > > > + *
> > > > + * @addr: Address of function
> > > > + * @arg0: First argument of function
> > > > + * @arg1: Second argument of function
> > > > + */
> > > > +struct ipi_data {
> > > > + ulong addr;
> > > > + ulong arg0;
> > > > + ulong arg1;
> > > > +};
> > > > +
> > > > +/**
> > > > + * handle_ipi() - interrupt handler for software interrupts
> > > > + *
> > > > + * The IPI interrupt handler must be called to handle software
> > > > +interrupts. It
> > > > + * calls the function specified in the hart's IPI data structure.
> > > > + *
> > > > + * @hart: Hart ID of the current hart
> > > > + */
> > > > +void handle_ipi(ulong hart);
> > > > +
> > > > +/**
> > > > + * smp_call_function() - Call a function on all other harts
> > > > + *
> > > > + * Send IPIs with the specified function call to all harts.
> > > > + *
> > > > + * @addr: Address of function
> > > > + * @arg0: First argument of function
> > > > + * @arg1: Second argument of function
> > > > + * @return 0 if OK, -ve on error
> > > > + */
> > > > +int smp_call_function(ulong addr, ulong arg0, ulong arg1);
> > > > +
> > > > +#endif
> > > > diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> > > > index
> > > > edfa61690c..19370f9749 100644
> > > > --- a/arch/riscv/lib/Makefile
> > > > +++ b/arch/riscv/lib/Makefile
> > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
> > > >  obj-y    += interrupts.o
> > > >  obj-y    += reset.o
> > > >  obj-y   += setjmp.o
> > > > +obj-$(CONFIG_SMP) += smp.o
> > > >
> > > >  # For building EFI apps
> > > >  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> > > > diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c new file
> > > > mode 100644
> > > > index 0000000000..1266a2a0ef
> > > > --- /dev/null
> > > > +++ b/arch/riscv/lib/smp.c
> > > > @@ -0,0 +1,110 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (C) 2019 Fraunhofer AISEC,
> > > > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > > > +
> > > > +#include <common.h>
> > > > +#include <dm.h>
> > > > +#include <asm/barrier.h>
> > > > +#include <asm/smp.h>
> > > > +
> > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > +
> > > > +/**
> > > > + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> > > > + *
> > > > + * Platform code must provide this function.
> > > > + *
> > > > + * @hart: Hart ID of receiving hart
> > > > + * @return 0 if OK, -ve on error
> > > > + */
> > > > +extern int riscv_send_ipi(int hart);
> > > > +
> > > > +/**
> > > > + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
> > > > + *
> > > > + * Platform code must provide this function.
> > > > + *
> > > > + * @hart: Hart ID of hart to be cleared
> > > > + * @return 0 if OK, -ve on error
> > > > + */
> > > > +extern int riscv_clear_ipi(int hart);
> > > > +
> > > > +static int send_ipi_many(struct ipi_data *ipi) {
> > > > + ofnode node, cpus;
> > > > + u32 reg;
> > > > + int ret;
> > > > +
> > > > + cpus = ofnode_path("/cpus");
> > > > + if (!ofnode_valid(cpus)) {
> > > > +         pr_err("Can't find cpus node!\n");
> > > > +         return -EINVAL;
> > > > + }
> > > > +
> > > > + ofnode_for_each_subnode(node, cpus) {
> > > > +         if (!ofnode_is_available(node))
> > > > +                 continue;
> > >
> > > It is not correct to assume that whatever CPUs are marked
> > > available will come online. It is possible that certain available
> > > CPUs failed to come online due HW failure.
> > >
> >
> > This was intended so that we don't send IPIs to harts, which have been
> > explicitly marked as disabled.
> >
> > > Better approach would be keep an atomic bitmask of HARTs
> > > that have entered U-Boot. All HARTs that enter U-Boot will
> > > update the atomic HART available bitmask. We send IPI only
> > > to HARTs that are available as-per atomic HART bitmask.
> > >
> >
> > I'm not sure if this is required in U-Boot, since we are not relying on
> > all harts to boot for U-Boot to function. We only try to boot all harts
> > listed as available in the device tree.
> >

It may also need to get information from each cpu nodes.
The cpu nodes is important, they must be match with the bitmask, or it
maybe go wrong.
So the bitmask way seem not be necessary.

BR
Rick

> > Is there a situation, in which we would want to avoid sending an IPI to
> > an unavailable hart?
> >
> > Thanks,
> > Lukas
> >
> > > > +
> > > > +         /* read hart ID of CPU */
> > > > +         ret = ofnode_read_u32(node, "reg", &reg);
> > > > +         if (ret)
> > > > +                 continue;
> > > > +
> > > > +         /* skip if it is the hart we are running on */
> > > > +         if (reg == gd->arch.boot_hart)
> > > > +                 continue;
> > > > +
> > > > +         if (reg >= CONFIG_NR_CPUS) {
> > > > +                 pr_err("Hart ID %d is out of range, increase
> > > > CONFIG_NR_CPUS\n",
> > > > +                        reg);
> > > > +                 continue;
> > > > +         }
> > > > +
> > > > +         gd->arch.ipi[reg].addr = ipi->addr;
> > > > +         gd->arch.ipi[reg].arg0 = ipi->arg0;
> > > > +         gd->arch.ipi[reg].arg1 = ipi->arg1;
> > > > +         mb();
> > > > +
> > > > +         ret = riscv_send_ipi(reg);
> > > > +         if (ret)
> > > > +                 return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void handle_ipi(ulong hart)
> > > > +{
> > > > + int ret;
> > > > + void (*smp_function)(ulong hart, ulong arg0, ulong arg1);
> > > > +
> > > > + if (hart >= CONFIG_NR_CPUS)
> > > > +         return;
> > > > +
> > > > + ret = riscv_clear_ipi(hart);
> > > > + if (ret) {
> > > > +         pr_err("Cannot clear IPI\n");
> > > > +         return;
> > > > + }
> > > > +
> > > > + smp_function = (void (*)(ulong, ulong, ulong))gd-
> > > > > arch.ipi[hart].addr;
> > > > + invalidate_icache_all();
> > > > +
> > > > + smp_function(hart, gd->arch.ipi[hart].arg0, gd-
> > > > >arch.ipi[hart].arg1);
> > > > +}
> > > > +
> > > > +int smp_call_function(ulong addr, ulong arg0, ulong arg1) {
> > > > + int ret = 0;
> > > > + struct ipi_data ipi;
> > > > +
> > > > + ipi.addr = addr;
> > > > + ipi.arg0 = arg0;
> > > > + ipi.arg1 = arg1;
> > > > +
> > > > + ret = send_ipi_many(&ipi);
> > > > +
> > > > + return ret;
> > > > +}
> > > > --
> > > > 2.20.1
> > >
> > > Regards,
> > > Anup
Anup Patel Feb. 18, 2019, 3:40 a.m. UTC | #6
On Mon, Feb 18, 2019 at 8:53 AM Rick Chen <rickchen36@gmail.com> wrote:
>
> <rick@andestech.com> 於 2019年2月18日 週一 上午11:00寫道:
> >
> >
> >
> > > -----Original Message-----
> > > From: Auer, Lukas [mailto:lukas.auer@aisec.fraunhofer.de]
> > > Sent: Monday, February 18, 2019 5:55 AM
> > > To: u-boot@lists.denx.de; Anup.Patel@wdc.com
> > > Cc: anup@brainfault.org; bmeng.cn@gmail.com; schwab@suse.de; Rick Jian-Zhi
> > > Chen(陳建志); palmer@sifive.com; Atish.Patra@wdc.com; agraf@suse.de
> > > Subject: Re: [PATCH 1/7] riscv: add infrastructure for calling functions on other
> > > harts
> > >
> > > On Tue, 2019-02-12 at 01:44 +0000, Anup Patel wrote:
> > > > > -----Original Message-----
> > > > > From: Lukas Auer [mailto:lukas.auer@aisec.fraunhofer.de]
> > > > > Sent: Tuesday, February 12, 2019 3:44 AM
> > > > > To: u-boot@lists.denx.de
> > > > > Cc: Atish Patra <Atish.Patra@wdc.com>; Anup Patel
> > > > > <Anup.Patel@wdc.com>; Bin Meng <bmeng.cn@gmail.com>; Andreas
> > > Schwab
> > > > > <schwab@suse.de>; Palmer Dabbelt <palmer@sifive.com>; Alexander Graf
> > > > > <agraf@suse.de>; Lukas Auer <lukas.auer@aisec.fraunhofer.de>; Anup
> > > > > Patel <anup@brainfault.org>; Rick Chen <rick@andestech.com>
> > > > > Subject: [PATCH 1/7] riscv: add infrastructure for calling functions
> > > > > on other harts
> > > > >
> > > > > 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@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]; #endif
> > > > >  };
> > > > >
> > > > >  #include <asm-generic/global_data.h>
> > > > > diff --git a/arch/riscv/include/asm/smp.h
> > > > > b/arch/riscv/include/asm/smp.h
> > > > > new file mode 100644 index 0000000000..bc863fdbaf
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/include/asm/smp.h
> > > > > @@ -0,0 +1,53 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +/*
> > > > > + * Copyright (C) 2019 Fraunhofer AISEC,
> > > > > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > > > > +
> > > > > +#ifndef _ASM_RISCV_SMP_H
> > > > > +#define _ASM_RISCV_SMP_H
> > > > > +
> > > > > +/**
> > > > > + * struct ipi_data - Inter-processor interrupt (IPI) data
> > > > > structure
> > > > > + *
> > > > > + * IPIs are used for SMP support to communicate to other harts
> > > > > what
> > > > > +function to
> > > > > + * call. Functions are in the form
> > > > > + * void (*addr)(ulong hart, ulong arg0, ulong arg1).
> > > > > + *
> > > > > + * The function address and the two arguments, arg0 and arg1, are
> > > > > +stored in the
> > > > > + * IPI data structure. The hart ID is inserted by the hart
> > > > > handling the
> > > > > +IPI and
> > > > > + * calling the function.
> > > > > + *
> > > > > + * @addr: Address of function
> > > > > + * @arg0: First argument of function
> > > > > + * @arg1: Second argument of function
> > > > > + */
> > > > > +struct ipi_data {
> > > > > + ulong addr;
> > > > > + ulong arg0;
> > > > > + ulong arg1;
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * handle_ipi() - interrupt handler for software interrupts
> > > > > + *
> > > > > + * The IPI interrupt handler must be called to handle software
> > > > > +interrupts. It
> > > > > + * calls the function specified in the hart's IPI data structure.
> > > > > + *
> > > > > + * @hart: Hart ID of the current hart
> > > > > + */
> > > > > +void handle_ipi(ulong hart);
> > > > > +
> > > > > +/**
> > > > > + * smp_call_function() - Call a function on all other harts
> > > > > + *
> > > > > + * Send IPIs with the specified function call to all harts.
> > > > > + *
> > > > > + * @addr: Address of function
> > > > > + * @arg0: First argument of function
> > > > > + * @arg1: Second argument of function
> > > > > + * @return 0 if OK, -ve on error
> > > > > + */
> > > > > +int smp_call_function(ulong addr, ulong arg0, ulong arg1);
> > > > > +
> > > > > +#endif
> > > > > diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> > > > > index
> > > > > edfa61690c..19370f9749 100644
> > > > > --- a/arch/riscv/lib/Makefile
> > > > > +++ b/arch/riscv/lib/Makefile
> > > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
> > > > >  obj-y    += interrupts.o
> > > > >  obj-y    += reset.o
> > > > >  obj-y   += setjmp.o
> > > > > +obj-$(CONFIG_SMP) += smp.o
> > > > >
> > > > >  # For building EFI apps
> > > > >  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> > > > > diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c new file
> > > > > mode 100644
> > > > > index 0000000000..1266a2a0ef
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/lib/smp.c
> > > > > @@ -0,0 +1,110 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + * Copyright (C) 2019 Fraunhofer AISEC,
> > > > > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <dm.h>
> > > > > +#include <asm/barrier.h>
> > > > > +#include <asm/smp.h>
> > > > > +
> > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > +
> > > > > +/**
> > > > > + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> > > > > + *
> > > > > + * Platform code must provide this function.
> > > > > + *
> > > > > + * @hart: Hart ID of receiving hart
> > > > > + * @return 0 if OK, -ve on error
> > > > > + */
> > > > > +extern int riscv_send_ipi(int hart);
> > > > > +
> > > > > +/**
> > > > > + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
> > > > > + *
> > > > > + * Platform code must provide this function.
> > > > > + *
> > > > > + * @hart: Hart ID of hart to be cleared
> > > > > + * @return 0 if OK, -ve on error
> > > > > + */
> > > > > +extern int riscv_clear_ipi(int hart);
> > > > > +
> > > > > +static int send_ipi_many(struct ipi_data *ipi) {
> > > > > + ofnode node, cpus;
> > > > > + u32 reg;
> > > > > + int ret;
> > > > > +
> > > > > + cpus = ofnode_path("/cpus");
> > > > > + if (!ofnode_valid(cpus)) {
> > > > > +         pr_err("Can't find cpus node!\n");
> > > > > +         return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + ofnode_for_each_subnode(node, cpus) {
> > > > > +         if (!ofnode_is_available(node))
> > > > > +                 continue;
> > > >
> > > > It is not correct to assume that whatever CPUs are marked
> > > > available will come online. It is possible that certain available
> > > > CPUs failed to come online due HW failure.
> > > >
> > >
> > > This was intended so that we don't send IPIs to harts, which have been
> > > explicitly marked as disabled.
> > >
> > > > Better approach would be keep an atomic bitmask of HARTs
> > > > that have entered U-Boot. All HARTs that enter U-Boot will
> > > > update the atomic HART available bitmask. We send IPI only
> > > > to HARTs that are available as-per atomic HART bitmask.
> > > >
> > >
> > > I'm not sure if this is required in U-Boot, since we are not relying on
> > > all harts to boot for U-Boot to function. We only try to boot all harts
> > > listed as available in the device tree.
> > >
>
> It may also need to get information from each cpu nodes.
> The cpu nodes is important, they must be match with the bitmask, or it
> maybe go wrong.
> So the bitmask way seem not be necessary.

Makes sense. How about considering both HART DT nodes availability
and HART online bitmask when sending IPI ?

My motivation behind HART online bitmask in U-Boot is because:

1. HART DT nodes marked available in DTB might actually be in unknown
state in case of HW failure. It is commonly seen that as silicon ages or
due to continuous heating of silicon new HW failures start showing up
over-time. This means even HARTs can fail to come online over-time.

2. We are moving towards SBI v0.2 and SBI PM extensions. This means
RISC-V SoCs that implement SBI PM extensions will only bring-up one
HART at boot-time. Rest of the HARTs, will have to powerd-up using
SBI calls. This means that even if HART DT node is marked available,
the HART might be still be offline until someone from S-mode does
SBI call to power-up HART.

Of course, we will be handling in above cases in OpenSBI too but it
is better to have "HART online bitmask" in U-Boot as well so that it
works fine if some buggy proprietary SBI runtime firmware is not
handling above cases.

Regards,
Anup
Anup Patel Feb. 18, 2019, 4:58 a.m. UTC | #7
On Tue, Feb 12, 2019 at 3:44 AM Lukas Auer
<lukas.auer@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@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).

Regards,
Anup
Lukas Auer Feb. 18, 2019, 9:53 a.m. UTC | #8
On Mon, 2019-02-18 at 09:10 +0530, Anup Patel wrote:
> On Mon, Feb 18, 2019 at 8:53 AM Rick Chen <rickchen36@gmail.com>
> wrote:
> > <rick@andestech.com> 於 2019年2月18日 週一 上午11:00寫道:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Auer, Lukas [mailto:lukas.auer@aisec.fraunhofer.de]
> > > > Sent: Monday, February 18, 2019 5:55 AM
> > > > To: u-boot@lists.denx.de; Anup.Patel@wdc.com
> > > > Cc: anup@brainfault.org; bmeng.cn@gmail.com; schwab@suse.de;
> > > > Rick Jian-Zhi
> > > > Chen(陳建志); palmer@sifive.com; Atish.Patra@wdc.com; 
> > > > agraf@suse.de
> > > > Subject: Re: [PATCH 1/7] riscv: add infrastructure for calling
> > > > functions on other
> > > > harts
> > > > 
> > > > On Tue, 2019-02-12 at 01:44 +0000, Anup Patel wrote:
> > > > > > -----Original Message-----
> > > > > > From: Lukas Auer [mailto:lukas.auer@aisec.fraunhofer.de]
> > > > > > Sent: Tuesday, February 12, 2019 3:44 AM
> > > > > > To: u-boot@lists.denx.de
> > > > > > Cc: Atish Patra <Atish.Patra@wdc.com>; Anup Patel
> > > > > > <Anup.Patel@wdc.com>; Bin Meng <bmeng.cn@gmail.com>;
> > > > > > Andreas
> > > > Schwab
> > > > > > <schwab@suse.de>; Palmer Dabbelt <palmer@sifive.com>;
> > > > > > Alexander Graf
> > > > > > <agraf@suse.de>; Lukas Auer <lukas.auer@aisec.fraunhofer.de
> > > > > > >; Anup
> > > > > > Patel <anup@brainfault.org>; Rick Chen <rick@andestech.com>
> > > > > > Subject: [PATCH 1/7] riscv: add infrastructure for calling
> > > > > > functions
> > > > > > on other harts
> > > > > > 
> > > > > > 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@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]; #endif
> > > > > >  };
> > > > > > 
> > > > > >  #include <asm-generic/global_data.h>
> > > > > > diff --git a/arch/riscv/include/asm/smp.h
> > > > > > b/arch/riscv/include/asm/smp.h
> > > > > > new file mode 100644 index 0000000000..bc863fdbaf
> > > > > > --- /dev/null
> > > > > > +++ b/arch/riscv/include/asm/smp.h
> > > > > > @@ -0,0 +1,53 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > +/*
> > > > > > + * Copyright (C) 2019 Fraunhofer AISEC,
> > > > > > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > > > > > +
> > > > > > +#ifndef _ASM_RISCV_SMP_H
> > > > > > +#define _ASM_RISCV_SMP_H
> > > > > > +
> > > > > > +/**
> > > > > > + * struct ipi_data - Inter-processor interrupt (IPI) data
> > > > > > structure
> > > > > > + *
> > > > > > + * IPIs are used for SMP support to communicate to other
> > > > > > harts
> > > > > > what
> > > > > > +function to
> > > > > > + * call. Functions are in the form
> > > > > > + * void (*addr)(ulong hart, ulong arg0, ulong arg1).
> > > > > > + *
> > > > > > + * The function address and the two arguments, arg0 and
> > > > > > arg1, are
> > > > > > +stored in the
> > > > > > + * IPI data structure. The hart ID is inserted by the hart
> > > > > > handling the
> > > > > > +IPI and
> > > > > > + * calling the function.
> > > > > > + *
> > > > > > + * @addr: Address of function
> > > > > > + * @arg0: First argument of function
> > > > > > + * @arg1: Second argument of function
> > > > > > + */
> > > > > > +struct ipi_data {
> > > > > > + ulong addr;
> > > > > > + ulong arg0;
> > > > > > + ulong arg1;
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * handle_ipi() - interrupt handler for software
> > > > > > interrupts
> > > > > > + *
> > > > > > + * The IPI interrupt handler must be called to handle
> > > > > > software
> > > > > > +interrupts. It
> > > > > > + * calls the function specified in the hart's IPI data
> > > > > > structure.
> > > > > > + *
> > > > > > + * @hart: Hart ID of the current hart
> > > > > > + */
> > > > > > +void handle_ipi(ulong hart);
> > > > > > +
> > > > > > +/**
> > > > > > + * smp_call_function() - Call a function on all other
> > > > > > harts
> > > > > > + *
> > > > > > + * Send IPIs with the specified function call to all
> > > > > > harts.
> > > > > > + *
> > > > > > + * @addr: Address of function
> > > > > > + * @arg0: First argument of function
> > > > > > + * @arg1: Second argument of function
> > > > > > + * @return 0 if OK, -ve on error
> > > > > > + */
> > > > > > +int smp_call_function(ulong addr, ulong arg0, ulong arg1);
> > > > > > +
> > > > > > +#endif
> > > > > > diff --git a/arch/riscv/lib/Makefile
> > > > > > b/arch/riscv/lib/Makefile
> > > > > > index
> > > > > > edfa61690c..19370f9749 100644
> > > > > > --- a/arch/riscv/lib/Makefile
> > > > > > +++ b/arch/riscv/lib/Makefile
> > > > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_SIFIVE_CLINT) +=
> > > > > > sifive_clint.o
> > > > > >  obj-y    += interrupts.o
> > > > > >  obj-y    += reset.o
> > > > > >  obj-y   += setjmp.o
> > > > > > +obj-$(CONFIG_SMP) += smp.o
> > > > > > 
> > > > > >  # For building EFI apps
> > > > > >  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> > > > > > diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c
> > > > > > new file
> > > > > > mode 100644
> > > > > > index 0000000000..1266a2a0ef
> > > > > > --- /dev/null
> > > > > > +++ b/arch/riscv/lib/smp.c
> > > > > > @@ -0,0 +1,110 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + * Copyright (C) 2019 Fraunhofer AISEC,
> > > > > > + * Lukas Auer <lukas.auer@aisec.fraunhofer.de>  */
> > > > > > +
> > > > > > +#include <common.h>
> > > > > > +#include <dm.h>
> > > > > > +#include <asm/barrier.h>
> > > > > > +#include <asm/smp.h>
> > > > > > +
> > > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > > +
> > > > > > +/**
> > > > > > + * riscv_send_ipi() - Send inter-processor interrupt (IPI)
> > > > > > + *
> > > > > > + * Platform code must provide this function.
> > > > > > + *
> > > > > > + * @hart: Hart ID of receiving hart
> > > > > > + * @return 0 if OK, -ve on error
> > > > > > + */
> > > > > > +extern int riscv_send_ipi(int hart);
> > > > > > +
> > > > > > +/**
> > > > > > + * riscv_clear_ipi() - Clear inter-processor interrupt
> > > > > > (IPI)
> > > > > > + *
> > > > > > + * Platform code must provide this function.
> > > > > > + *
> > > > > > + * @hart: Hart ID of hart to be cleared
> > > > > > + * @return 0 if OK, -ve on error
> > > > > > + */
> > > > > > +extern int riscv_clear_ipi(int hart);
> > > > > > +
> > > > > > +static int send_ipi_many(struct ipi_data *ipi) {
> > > > > > + ofnode node, cpus;
> > > > > > + u32 reg;
> > > > > > + int ret;
> > > > > > +
> > > > > > + cpus = ofnode_path("/cpus");
> > > > > > + if (!ofnode_valid(cpus)) {
> > > > > > +         pr_err("Can't find cpus node!\n");
> > > > > > +         return -EINVAL;
> > > > > > + }
> > > > > > +
> > > > > > + ofnode_for_each_subnode(node, cpus) {
> > > > > > +         if (!ofnode_is_available(node))
> > > > > > +                 continue;
> > > > > 
> > > > > It is not correct to assume that whatever CPUs are marked
> > > > > available will come online. It is possible that certain
> > > > > available
> > > > > CPUs failed to come online due HW failure.
> > > > > 
> > > > 
> > > > This was intended so that we don't send IPIs to harts, which
> > > > have been
> > > > explicitly marked as disabled.
> > > > 
> > > > > Better approach would be keep an atomic bitmask of HARTs
> > > > > that have entered U-Boot. All HARTs that enter U-Boot will
> > > > > update the atomic HART available bitmask. We send IPI only
> > > > > to HARTs that are available as-per atomic HART bitmask.
> > > > > 
> > > > 
> > > > I'm not sure if this is required in U-Boot, since we are not
> > > > relying on
> > > > all harts to boot for U-Boot to function. We only try to boot
> > > > all harts
> > > > listed as available in the device tree.
> > > > 
> > 
> > It may also need to get information from each cpu nodes.
> > The cpu nodes is important, they must be match with the bitmask, or
> > it
> > maybe go wrong.
> > So the bitmask way seem not be necessary.
> 
> Makes sense. How about considering both HART DT nodes availability
> and HART online bitmask when sending IPI ?
> 
> My motivation behind HART online bitmask in U-Boot is because:
> 
> 1. HART DT nodes marked available in DTB might actually be in unknown
> state in case of HW failure. It is commonly seen that as silicon ages
> or
> due to continuous heating of silicon new HW failures start showing up
> over-time. This means even HARTs can fail to come online over-time.
> 
> 2. We are moving towards SBI v0.2 and SBI PM extensions. This means
> RISC-V SoCs that implement SBI PM extensions will only bring-up one
> HART at boot-time. Rest of the HARTs, will have to powerd-up using
> SBI calls. This means that even if HART DT node is marked available,
> the HART might be still be offline until someone from S-mode does
> SBI call to power-up HART.
> 
> Of course, we will be handling in above cases in OpenSBI too but it
> is better to have "HART online bitmask" in U-Boot as well so that it
> works fine if some buggy proprietary SBI runtime firmware is not
> handling above cases.
> 

Thanks for explaining! I agree, especially for case 2 we will need to
handle offline harts. I'll integrate a hart online bitmask in the next
version.

Thanks,
Lukas
Lukas Auer Feb. 18, 2019, 10:01 a.m. UTC | #9
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@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@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!

Thanks,
Lukas
Lukas Auer Feb. 18, 2019, 11:41 a.m. UTC | #10
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@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@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.

918c1354b75c74b62f67c4e929551d643f035443 is the first bad commit
commit 918c1354b75c74b62f67c4e929551d643f035443
Author: Nick Kossifidis <mickflemm@gmail.com>
Date:   Sun Feb 17 09:00:20 2019 +0200

    lib: Improve delivery of SBI_IPI_EVENT_HALT
    
    When sbi_ipi_send_many gets called with the current hartid
    included on pmask (or when pmask is NULL), and we send
    a HALT event, since the for loop works sequentially over
    all hartids on the mask, we may send a HALT event to the
    current hart before the loop finishes. So we will halt
    the current hart before it can deliver a HALT IPI to the
    rest and some harts will remain active.
    
    Make sure we send an IPI to the current hart after we've
    finished with everybody else.
    
    Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>

I don't have time to look into it right now, but I will do so later.

Thanks,
Lukas
Anup Patel Feb. 18, 2019, 1:16 p.m. UTC | #11
On Mon, Feb 18, 2019 at 5:11 PM Auer, Lukas
<lukas.auer@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@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@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.
>
> 918c1354b75c74b62f67c4e929551d643f035443 is the first bad commit
> commit 918c1354b75c74b62f67c4e929551d643f035443
> Author: Nick Kossifidis <mickflemm@gmail.com>
> Date:   Sun Feb 17 09:00:20 2019 +0200
>
>     lib: Improve delivery of SBI_IPI_EVENT_HALT
>
>     When sbi_ipi_send_many gets called with the current hartid
>     included on pmask (or when pmask is NULL), and we send
>     a HALT event, since the for loop works sequentially over
>     all hartids on the mask, we may send a HALT event to the
>     current hart before the loop finishes. So we will halt
>     the current hart before it can deliver a HALT IPI to the
>     rest and some harts will remain active.
>
>     Make sure we send an IPI to the current hart after we've
>     finished with everybody else.
>
>     Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
>

No issue. Already fixed it with following commit:

lib: Fix mask shift in sbi_ipi_send_many()

The mask shift in for-loop of sbi_ipi_send_many() is
broken with commit 918c135
("lib: Improve delivery of SBI_IPI_EVENT_HALT")

This patch fix it.

Signed-off-by: Anup Patel <anup.patel@wdc.com>


Please pull latest OpenSBI repo.

Thanks,
Anup
Lukas Auer Feb. 18, 2019, 4:04 p.m. UTC | #12
On Mon, 2019-02-18 at 18:46 +0530, Anup Patel wrote:
> On Mon, Feb 18, 2019 at 5:11 PM Auer, Lukas
> <lukas.auer@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@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@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.
> > 
> > 918c1354b75c74b62f67c4e929551d643f035443 is the first bad commit
> > commit 918c1354b75c74b62f67c4e929551d643f035443
> > Author: Nick Kossifidis <mickflemm@gmail.com>
> > Date:   Sun Feb 17 09:00:20 2019 +0200
> > 
> >     lib: Improve delivery of SBI_IPI_EVENT_HALT
> > 
> >     When sbi_ipi_send_many gets called with the current hartid
> >     included on pmask (or when pmask is NULL), and we send
> >     a HALT event, since the for loop works sequentially over
> >     all hartids on the mask, we may send a HALT event to the
> >     current hart before the loop finishes. So we will halt
> >     the current hart before it can deliver a HALT IPI to the
> >     rest and some harts will remain active.
> > 
> >     Make sure we send an IPI to the current hart after we've
> >     finished with everybody else.
> > 
> >     Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
> > 
> 
> No issue. Already fixed it with following commit:
> 
> lib: Fix mask shift in sbi_ipi_send_many()
> 
> The mask shift in for-loop of sbi_ipi_send_many() is
> broken with commit 918c135
> ("lib: Improve delivery of SBI_IPI_EVENT_HALT")
> 
> This patch fix it.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> 
> 
> Please pull latest OpenSBI repo.
> 

Thanks, that fixed it.

Lukas
Anup Patel Feb. 19, 2019, 8:16 a.m. UTC | #13
On Mon, Feb 18, 2019 at 5:11 PM Auer, Lukas
<lukas.auer@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@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@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.

Regards,
Anup
Lukas Auer Feb. 25, 2019, 11:13 a.m. UTC | #14
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@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@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@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
diff mbox series

Patch

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];
+#endif
 };
 
 #include <asm-generic/global_data.h>
diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
new file mode 100644
index 0000000000..bc863fdbaf
--- /dev/null
+++ b/arch/riscv/include/asm/smp.h
@@ -0,0 +1,53 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 Fraunhofer AISEC,
+ * Lukas Auer <lukas.auer@aisec.fraunhofer.de>
+ */
+
+#ifndef _ASM_RISCV_SMP_H
+#define _ASM_RISCV_SMP_H
+
+/**
+ * struct ipi_data - Inter-processor interrupt (IPI) data structure
+ *
+ * IPIs are used for SMP support to communicate to other harts what function to
+ * call. Functions are in the form
+ * void (*addr)(ulong hart, ulong arg0, ulong arg1).
+ *
+ * The function address and the two arguments, arg0 and arg1, are stored in the
+ * IPI data structure. The hart ID is inserted by the hart handling the IPI and
+ * calling the function.
+ *
+ * @addr: Address of function
+ * @arg0: First argument of function
+ * @arg1: Second argument of function
+ */
+struct ipi_data {
+	ulong addr;
+	ulong arg0;
+	ulong arg1;
+};
+
+/**
+ * handle_ipi() - interrupt handler for software interrupts
+ *
+ * The IPI interrupt handler must be called to handle software interrupts. It
+ * calls the function specified in the hart's IPI data structure.
+ *
+ * @hart: Hart ID of the current hart
+ */
+void handle_ipi(ulong hart);
+
+/**
+ * smp_call_function() - Call a function on all other harts
+ *
+ * Send IPIs with the specified function call to all harts.
+ *
+ * @addr: Address of function
+ * @arg0: First argument of function
+ * @arg1: Second argument of function
+ * @return 0 if OK, -ve on error
+ */
+int smp_call_function(ulong addr, ulong arg0, ulong arg1);
+
+#endif
diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
index edfa61690c..19370f9749 100644
--- a/arch/riscv/lib/Makefile
+++ b/arch/riscv/lib/Makefile
@@ -14,6 +14,7 @@  obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
 obj-y	+= interrupts.o
 obj-y	+= reset.o
 obj-y   += setjmp.o
+obj-$(CONFIG_SMP) += smp.o
 
 # For building EFI apps
 CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c
new file mode 100644
index 0000000000..1266a2a0ef
--- /dev/null
+++ b/arch/riscv/lib/smp.c
@@ -0,0 +1,110 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Fraunhofer AISEC,
+ * Lukas Auer <lukas.auer@aisec.fraunhofer.de>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <asm/barrier.h>
+#include <asm/smp.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/**
+ * riscv_send_ipi() - Send inter-processor interrupt (IPI)
+ *
+ * Platform code must provide this function.
+ *
+ * @hart: Hart ID of receiving hart
+ * @return 0 if OK, -ve on error
+ */
+extern int riscv_send_ipi(int hart);
+
+/**
+ * riscv_clear_ipi() - Clear inter-processor interrupt (IPI)
+ *
+ * Platform code must provide this function.
+ *
+ * @hart: Hart ID of hart to be cleared
+ * @return 0 if OK, -ve on error
+ */
+extern int riscv_clear_ipi(int hart);
+
+static int send_ipi_many(struct ipi_data *ipi)
+{
+	ofnode node, cpus;
+	u32 reg;
+	int ret;
+
+	cpus = ofnode_path("/cpus");
+	if (!ofnode_valid(cpus)) {
+		pr_err("Can't find cpus node!\n");
+		return -EINVAL;
+	}
+
+	ofnode_for_each_subnode(node, cpus) {
+		if (!ofnode_is_available(node))
+			continue;
+
+		/* read hart ID of CPU */
+		ret = ofnode_read_u32(node, "reg", &reg);
+		if (ret)
+			continue;
+
+		/* skip if it is the hart we are running on */
+		if (reg == gd->arch.boot_hart)
+			continue;
+
+		if (reg >= CONFIG_NR_CPUS) {
+			pr_err("Hart ID %d is out of range, increase CONFIG_NR_CPUS\n",
+			       reg);
+			continue;
+		}
+
+		gd->arch.ipi[reg].addr = ipi->addr;
+		gd->arch.ipi[reg].arg0 = ipi->arg0;
+		gd->arch.ipi[reg].arg1 = ipi->arg1;
+		mb();
+
+		ret = riscv_send_ipi(reg);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+void handle_ipi(ulong hart)
+{
+	int ret;
+	void (*smp_function)(ulong hart, ulong arg0, ulong arg1);
+
+	if (hart >= CONFIG_NR_CPUS)
+		return;
+
+	ret = riscv_clear_ipi(hart);
+	if (ret) {
+		pr_err("Cannot clear IPI\n");
+		return;
+	}
+
+	smp_function = (void (*)(ulong, ulong, ulong))gd->arch.ipi[hart].addr;
+	invalidate_icache_all();
+
+	smp_function(hart, gd->arch.ipi[hart].arg0, gd->arch.ipi[hart].arg1);
+}
+
+int smp_call_function(ulong addr, ulong arg0, ulong arg1)
+{
+	int ret = 0;
+	struct ipi_data ipi;
+
+	ipi.addr = addr;
+	ipi.arg0 = arg0;
+	ipi.arg1 = arg1;
+
+	ret = send_ipi_many(&ipi);
+
+	return ret;
+}