Re: [PATCH v4 3/4] RISC-V: Introduce a new config for SBI v0.1

From: Anup Patel
Date: Mon Nov 25 2019 - 23:14:42 EST

On Tue, Nov 26, 2019 at 8:50 AM Atish Patra <atish.patra@xxxxxxx> wrote:
>
> We now have SBI v0.2 which is more scalable and extendable to handle
> future needs for RISC-V supervisor interfaces.
>
> Introduce a new config and move all SBI v0.1 code under that config.
> This allows to implement the new replacement SBI extensions cleanly
> and remove v0.1 extensions easily in future. Currently, the config
> is enabled by default. Once all M-mode software with v0.1 are no
> longer in use, this config option and all relevant code can be easily
> removed.
>
> Signed-off-by: Atish Patra <atish.patra@xxxxxxx>
> ---
> arch/riscv/Kconfig | 6 ++
> arch/riscv/include/asm/sbi.h | 2 +
> arch/riscv/kernel/sbi.c | 154 +++++++++++++++++++++++++++++------
> 3 files changed, 138 insertions(+), 24 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index ca3b5541ae93..15c020d6837b 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -304,6 +304,12 @@ config SECCOMP
> and the task is only allowed to execute a few safe syscalls
> defined by each seccomp mode.
>
> +config RISCV_SBI_V01
> + bool "SBI v0.1 support"
> + default y
> + help
> + This config allows kernel to use SBI v0.1 APIs. This will be
> + deprecated in future once legacy M-mode software are no longer in use.
> endmenu
>
> menu "Boot options"
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 906438322932..cc82ae63f8e0 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -10,6 +10,7 @@
>
> #ifdef CONFIG_RISCV_SBI
> enum sbi_ext_id {
> +#ifdef CONFIG_RISCV_SBI_V01
> SBI_EXT_0_1_SET_TIMER = 0x0,
> SBI_EXT_0_1_CONSOLE_PUTCHAR = 0x1,
> SBI_EXT_0_1_CONSOLE_GETCHAR = 0x2,
> @@ -19,6 +20,7 @@ enum sbi_ext_id {
> SBI_EXT_0_1_REMOTE_SFENCE_VMA = 0x6,
> SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID = 0x7,
> SBI_EXT_0_1_SHUTDOWN = 0x8,
> +#endif
> SBI_EXT_BASE = 0x10,
> };
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 8b36269fa515..8574de1074c4 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -8,6 +8,14 @@
> unsigned long sbi_spec_version = SBI_SPEC_VERSION_DEFAULT;
> EXPORT_SYMBOL(sbi_spec_version);
>
> +void (*__sbi_set_timer)(uint64_t stime);
> +int (*__sbi_send_ipi)(const unsigned long *hart_mask);
> +int (*__sbi_rfence)(unsigned long extid, unsigned long fid,
> + const unsigned long *hart_mask,
> + unsigned long hbase, unsigned long start,
> + unsigned long size, unsigned long arg4,
> + unsigned long arg5);
> +

Make these function pointers static.

> struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> unsigned long arg1, unsigned long arg2,
> unsigned long arg3, unsigned long arg4,
> @@ -52,6 +60,32 @@ static int sbi_err_map_linux_errno(int err)
> };
> }
>
> +static inline void __sbi_set_timer_dummy_warn(uint64_t stime_value)

Don't make this function inline because you are assigning it to function
pointer below.

> +{
> + pr_warn("Timer extension is not available in SBI v%lu.%lu\n",
> + sbi_major_version(), sbi_minor_version());
> +}
> +
> +static inline int __sbi_send_ipi_dummy_warn(const unsigned long *hart_mask)

Same as above.

> +{
> + pr_warn("IPI extension is not available in SBI v%lu.%lu\n",
> + sbi_major_version(), sbi_minor_version());
> + return 0;
> +}
> +
> +static inline int __sbi_rfence_dummy_warn(unsigned long extid,
> + unsigned long fid,
> + const unsigned long *hart_mask,
> + unsigned long hbase, unsigned long start,
> + unsigned long size, unsigned long arg4,
> + unsigned long arg5)

Same as above.

> +{
> + pr_warn("remote fence extension is not available in SBI v%lu.%lu\n",
> + sbi_major_version(), sbi_minor_version());
> + return 0;
> +}
> +
> +#ifdef CONFIG_RISCV_SBI_V01
> /**
> * sbi_console_putchar() - Writes given character to the console device.
> * @ch: The data to be written to the console.
> @@ -80,41 +114,106 @@ int sbi_console_getchar(void)
> EXPORT_SYMBOL(sbi_console_getchar);
>
> /**
> - * sbi_set_timer() - Program the timer for next timer event.
> - * @stime_value: The value after which next timer event should fire.
> + * sbi_shutdown() - Remove all the harts from executing supervisor code.
> *
> * Return: None
> */
> -void sbi_set_timer(uint64_t stime_value)
> +void sbi_shutdown(void)
> {
> -#if __riscv_xlen == 32
> - sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
> - stime_value >> 32, 0, 0, 0, 0);
> -#else
> - sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value, 0, 0, 0, 0, 0);
> -#endif
> + sbi_ecall(SBI_EXT_0_1_SHUTDOWN, 0, 0, 0, 0, 0, 0, 0);
> }
> EXPORT_SYMBOL(sbi_set_timer);
>
> /**
> - * sbi_shutdown() - Remove all the harts from executing supervisor code.
> + * sbi_clear_ipi() - Clear any pending IPIs for the calling hart.
> *
> * Return: None
> */
> -void sbi_shutdown(void)
> +void sbi_clear_ipi(void)
> {
> - sbi_ecall(SBI_EXT_0_1_SHUTDOWN, 0, 0, 0, 0, 0, 0, 0);
> + sbi_ecall(SBI_EXT_0_1_CLEAR_IPI, 0, 0, 0, 0, 0, 0, 0);
> }
> EXPORT_SYMBOL(sbi_shutdown);
>
> /**
> - * sbi_clear_ipi() - Clear any pending IPIs for the calling hart.
> + * sbi_set_timer_v01() - Program the timer for next timer event.
> + * @stime_value: The value after which next timer event should fire.
> *
> * Return: None
> */
> -void sbi_clear_ipi(void)
> +static void __sbi_set_timer_v01(uint64_t stime_value)
> {
> - sbi_ecall(SBI_EXT_0_1_CLEAR_IPI, 0, 0, 0, 0, 0, 0, 0);
> +#if __riscv_xlen == 32
> + sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
> + stime_value >> 32, 0, 0, 0, 0);
> +#else
> + sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value, 0, 0, 0, 0, 0);
> +#endif
> +}
> +
> +static int __sbi_send_ipi_v01(const unsigned long *hart_mask)
> +{
> + sbi_ecall(SBI_EXT_0_1_SEND_IPI, 0, (unsigned long)hart_mask,
> + 0, 0, 0, 0, 0);
> + return 0;
> +}
> +
> +static int __sbi_rfence_v01(unsigned long ext, unsigned long fid,
> + const unsigned long *hart_mask,
> + unsigned long hbase, unsigned long start,
> + unsigned long size, unsigned long arg4,
> + unsigned long arg5)
> +{
> + switch (ext) {
> + case SBI_EXT_0_1_REMOTE_FENCE_I:
> + sbi_ecall(SBI_EXT_0_1_REMOTE_FENCE_I, 0,
> + (unsigned long)hart_mask, 0, 0, 0, 0, 0);
> + break;
> + case SBI_EXT_0_1_REMOTE_SFENCE_VMA:
> + sbi_ecall(SBI_EXT_0_1_REMOTE_SFENCE_VMA, 0,
> + (unsigned long)hart_mask, start, size,
> + 0, 0, 0);
> + break;
> + case SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID:
> + sbi_ecall(SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID, 0,
> + (unsigned long)hart_mask, start, size,
> + arg4, 0, 0);
> + break;
> + default:
> + pr_err("extid [%lu]not supported in SBI v0.1\n", ext);
> + }
> +
> + return 0;
> +}
> +#else
> +static void __sbi_set_timer_v01(uint64_t stime_value)
> +{
> + __sbi_set_timer_dummy_warn(0);
> +}
> +static int __sbi_send_ipi_v01(const unsigned long *hart_mask)
> +{
> + return __sbi_send_ipi_dummy_warn(NULL);
> +}
> +static int __sbi_rfence_v01(unsigned long ext, unsigned long fid,
> + const unsigned long *hart_mask,
> + unsigned long hbase, unsigned long start,
> + unsigned long size, unsigned long arg4,
> + unsigned long arg5)
> +{
> + return __sbi_rfence_dummy_warn(0, 0, 0, 0, 0, 0, 0, 0);
> +
> +}
> +#endif /* CONFIG_RISCV_SBI_V01 */
> +
> +/**
> + * sbi_set_timer() - Program the timer for next timer event.
> + * @stime_value: The value after which next timer event should fire.
> + *
> + * Return: None
> + */
> +void sbi_set_timer(uint64_t stime_value)
> +{
> + __sbi_set_timer(stime_value);
> }
>
> /**
> @@ -125,11 +224,11 @@ void sbi_clear_ipi(void)
> */
> void sbi_send_ipi(const unsigned long *hart_mask)
> {
> - sbi_ecall(SBI_EXT_0_1_SEND_IPI, 0, (unsigned long)hart_mask,
> - 0, 0, 0, 0, 0);
> + __sbi_send_ipi(hart_mask);
> }
> EXPORT_SYMBOL(sbi_send_ipi);
>
> +
> /**
> * sbi_remote_fence_i() - Execute FENCE.I instruction on given remote harts.
> * @hart_mask: A cpu mask containing all the target harts.
> @@ -138,8 +237,8 @@ EXPORT_SYMBOL(sbi_send_ipi);
> */
> void sbi_remote_fence_i(const unsigned long *hart_mask)
> {
> - sbi_ecall(SBI_EXT_0_1_REMOTE_FENCE_I, 0, (unsigned long)hart_mask,
> - 0, 0, 0, 0, 0);
> + __sbi_rfence(SBI_EXT_0_1_REMOTE_FENCE_I, 0,
> + hart_mask, 0, 0, 0, 0, 0);
> }
> EXPORT_SYMBOL(sbi_remote_fence_i);
>
> @@ -156,8 +255,8 @@ void sbi_remote_sfence_vma(const unsigned long *hart_mask,
> unsigned long start,
> unsigned long size)
> {
> - sbi_ecall(SBI_EXT_0_1_REMOTE_SFENCE_VMA, 0,
> - (unsigned long)hart_mask, start, size, 0, 0, 0);
> + __sbi_rfence(SBI_EXT_0_1_REMOTE_SFENCE_VMA, 0,
> + hart_mask, 0, start, size, 0, 0);
> }
> EXPORT_SYMBOL(sbi_remote_sfence_vma);
>
> @@ -177,8 +276,8 @@ void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> unsigned long size,
> unsigned long asid)
> {
> - sbi_ecall(SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID, 0,
> - (unsigned long)hart_mask, start, size, asid, 0, 0);
> + __sbi_rfence(SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID, 0,
> + hart_mask, 0, start, size, asid, 0);
> }
> EXPORT_SYMBOL(sbi_remote_sfence_vma_asid);
>
> @@ -254,8 +353,15 @@ int __init sbi_init(void)
>
> pr_info("SBI specification v%lu.%lu detected\n",
> sbi_major_version(), sbi_minor_version());
> - if (!sbi_spec_is_0_1())
> +
> + if (sbi_spec_is_0_1()) {
> + __sbi_set_timer = __sbi_set_timer_v01;
> + __sbi_send_ipi = __sbi_send_ipi_v01;
> + __sbi_rfence = __sbi_rfence_v01;
> + } else {
> pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
> sbi_get_firmware_id(), sbi_get_firmware_version());
> + }
> +
> return 0;
> }
> --
> 2.23.0
>

Minor comments above otherwise looks good.

Reviewed-by: Anup Patel <anup@xxxxxxxxxxxxxx>

Regards,
Anup