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

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

On Wed, 2019-12-04 at 10:52 -0800, Palmer Dabbelt wrote:
> On Tue, 26 Nov 2019 11:05:02 PST (-0800), Atish Patra 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
> 
> I'd say "software is", not "software are".  There's at least one more
> instance
> of this.
> 

Sure. Wil fix this.

> > longer in use, this config option and all relevant code can be
> > easily
> > removed.
> > 
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Reviewed-by: Anup Patel <anup@brainfault.org>
> > ---
> >  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"
> 
> I don't think presenting this to users as "v0.1" is correct.  I'd
> expect that
> to disable probing and 

Probing is done in SBI generic code which is necessary to dynamically
figure out the SBI version (v0.1 ..v0.2 or anything else in future).
This doesn't have to do anything with this config option.

> assume the legacy extension always exists, while this
> just allows the legacy extension to be used in a v0.2 style.  For
> example: it's
> checking the SBI version, which doesn't exist in v0.1.
> 

I am bit confused here. May be we are not on the same page in terms of
terminology. Here is my understanding

v0.1   		  - All v0.1 calls with v0.1 calling convention.
legacy extensions - representing v0.1 calls as extensions in v0.2

replacement extensions - new extensions defined in v0.2 to replace of
only necessary v0.1 calls. The extension definition and purpose are
defined in

https://github.com/riscv/riscv-sbi-doc/pull/27/commits/3c87fbdacf301a158015b8e57a801c48e30d5738

That's why the unused v0.1 extensions (i.e. CLEAR_IPI, CONSOLE*) are
not defind in replacement extensions.

These replacement extensions must be optional because, we may have
hardware support for these extensions in future. E.g. some hardware may
have tlb flushing as an instruction or can send IPIs directly from S-
mode.


> There should really be two options here: one to allow the legacy
> extension set
> and one to be compatible with SBI v0.1.  We can deprecate the v0.1
> support much
> sooner than the legacy extension set.

Why do we need a separate config just to be compatible with SBI v0.1 ?
The calling convention defined in v0.2 is fully backward compatible
with v0.1.

The idea of having a separate config is to help deprecate all
v0.1(=legacy) extensions in future all at once.


> > +	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 a47e23c3a2e1..ee710bfe0b0e 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);
> > 
> > +static void (*__sbi_set_timer)(uint64_t stime);
> > +static int (*__sbi_send_ipi)(const unsigned long *hart_mask);
> > +static 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);
> > +
> >  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 void __sbi_set_timer_dummy_warn(uint64_t stime_value)
> > +{
> > +	pr_warn("Timer extension is not available in SBI v%lu.%lu\n",
> > +		sbi_major_version(), sbi_minor_version());
> > +}
> > +
> > +static int __sbi_send_ipi_dummy_warn(const unsigned long
> > *hart_mask)
> > +{
> > +	pr_warn("IPI extension is not available in SBI v%lu.%lu\n",
> > +		sbi_major_version(), sbi_minor_version());
> > +	return 0;
> > +}
> > +
> > +static 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)
> > +{
> > +	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);
> > 
> > @@ -253,8 +352,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()) {
> >  		pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
> >  			sbi_get_firmware_id(),
> > sbi_get_firmware_version());
> > +	}
> > +
> > +	__sbi_set_timer = __sbi_set_timer_v01;
> > +	__sbi_send_ipi	= __sbi_send_ipi_v01;
> > +	__sbi_rfence	= __sbi_rfence_v01;
> > +
> >  	return 0;
> >  }

-- 
Regards,
Atish