Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191126032033.14825-1-atish.patra@wdc.com> <20191126032033.14825-4-atish.patra@wdc.com>
In-Reply-To: <20191126032033.14825-4-atish.patra@wdc.com>
From:   Anup Patel <anup@brainfault.org>
Date:   Tue, 26 Nov 2019 09:44:25 +0530
Message-ID: <CAAhSdy2icvYPfa5Dm+1qF9sr=22ErHVYr=ZmXYqu6dHZing81A@mail.gmail.com>
Subject: Re: [PATCH v4 3/4] RISC-V: Introduce a new config for SBI v0.1
To:     Atish Patra <atish.patra@wdc.com>
Cc:     "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Mao Han <han_mao@c-sky.com>,
        Mike Rapoport <rppt@linux.ibm.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 26, 2019 at 8:50 AM Atish Patra <atish.patra@wdc.com> 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@wdc.com>
> ---
>  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@brainfault.org>

Regards,
Anup