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

[Atish Patra <Atish.Patra@wdc.com>]

On Tue, 2019-11-26 at 09:44 +0530, Anup Patel wrote:
> 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.
> 

Sure.

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

Thanks for pointing that out. I wil fix all the instances.

> > +{
> > +       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

-- 
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv