2019-08-26 23:35:20

by Atish Patra

[permalink] [raw]
Subject: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

As per the new SBI specification, current SBI implementation is
defined as legacy and will be removed/replaced in future.

Rename existing implementation to reflect that. This patch is just
a preparatory patch for SBI v0.2 and doesn't introduce any functional
changes.

Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/sbi.h | 61 +++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 21134b3ef404..7f5ecaaaa0d7 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -8,17 +8,18 @@

#include <linux/types.h>

-#define SBI_SET_TIMER 0
-#define SBI_CONSOLE_PUTCHAR 1
-#define SBI_CONSOLE_GETCHAR 2
-#define SBI_CLEAR_IPI 3
-#define SBI_SEND_IPI 4
-#define SBI_REMOTE_FENCE_I 5
-#define SBI_REMOTE_SFENCE_VMA 6
-#define SBI_REMOTE_SFENCE_VMA_ASID 7
-#define SBI_SHUTDOWN 8
-
-#define SBI_CALL(which, arg0, arg1, arg2, arg3) ({ \
+
+#define SBI_EXT_LEGACY_SET_TIMER 0x0
+#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
+#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
+#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
+#define SBI_EXT_LEGACY_SEND_IPI 0x4
+#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
+#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
+#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
+#define SBI_EXT_LEGACY_SHUTDOWN 0x8
+
+#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
@@ -32,58 +33,61 @@
})

/* Lazy implementations until SBI is finalized */
-#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
-#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
-#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1, 0, 0)
-#define SBI_CALL_3(which, arg0, arg1, arg2) \
- SBI_CALL(which, arg0, arg1, arg2, 0)
-#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
- SBI_CALL(which, arg0, arg1, arg2, arg3)
+#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0, 0)
+#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which, arg0, 0, 0, 0)
+#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
+ SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
+#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
+ SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
+#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
+ SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)

static inline void sbi_console_putchar(int ch)
{
- SBI_CALL_1(SBI_CONSOLE_PUTCHAR, ch);
+ SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_CONSOLE_PUTCHAR, ch);
}

static inline int sbi_console_getchar(void)
{
- return SBI_CALL_0(SBI_CONSOLE_GETCHAR);
+ return SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CONSOLE_GETCHAR);
}

static inline void sbi_set_timer(uint64_t stime_value)
{
#if __riscv_xlen == 32
- SBI_CALL_2(SBI_SET_TIMER, stime_value, stime_value >> 32);
+ SBI_CALL_LEGACY_2(SBI_EXT_LEGACY_SET_TIMER, stime_value,
+ stime_value >> 32);
#else
- SBI_CALL_1(SBI_SET_TIMER, stime_value);
+ SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SET_TIMER, stime_value);
#endif
}

static inline void sbi_shutdown(void)
{
- SBI_CALL_0(SBI_SHUTDOWN);
+ SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_SHUTDOWN);
}

static inline void sbi_clear_ipi(void)
{
- SBI_CALL_0(SBI_CLEAR_IPI);
+ SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CLEAR_IPI);
}

static inline void sbi_send_ipi(const unsigned long *hart_mask)
{
- SBI_CALL_1(SBI_SEND_IPI, hart_mask);
+ SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SEND_IPI, hart_mask);
}

static inline void sbi_remote_fence_i(const unsigned long *hart_mask)
{
- SBI_CALL_1(SBI_REMOTE_FENCE_I, hart_mask);
+ SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_REMOTE_FENCE_I, hart_mask);
}

static inline void sbi_remote_sfence_vma(const unsigned long *hart_mask,
unsigned long start,
unsigned long size)
{
- SBI_CALL_3(SBI_REMOTE_SFENCE_VMA, hart_mask, start, size);
+ SBI_CALL_LEGACY_3(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA, hart_mask,
+ start, size);
}

static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
@@ -91,7 +95,8 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
unsigned long size,
unsigned long asid)
{
- SBI_CALL_4(SBI_REMOTE_SFENCE_VMA_ASID, hart_mask, start, size, asid);
+ SBI_CALL_LEGACY_4(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID, hart_mask,
+ start, size, asid);
}

#endif
--
2.21.0


2019-08-27 08:24:43

by Mike Rapoport

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Mon, Aug 26, 2019 at 04:32:55PM -0700, Atish Patra wrote:
> As per the new SBI specification, current SBI implementation is
> defined as legacy and will be removed/replaced in future.
>
> Rename existing implementation to reflect that. This patch is just
> a preparatory patch for SBI v0.2 and doesn't introduce any functional
> changes.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/sbi.h | 61 +++++++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 21134b3ef404..7f5ecaaaa0d7 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -8,17 +8,18 @@
>
> #include <linux/types.h>
>
> -#define SBI_SET_TIMER 0
> -#define SBI_CONSOLE_PUTCHAR 1
> -#define SBI_CONSOLE_GETCHAR 2
> -#define SBI_CLEAR_IPI 3
> -#define SBI_SEND_IPI 4
> -#define SBI_REMOTE_FENCE_I 5
> -#define SBI_REMOTE_SFENCE_VMA 6
> -#define SBI_REMOTE_SFENCE_VMA_ASID 7
> -#define SBI_SHUTDOWN 8
> -
> -#define SBI_CALL(which, arg0, arg1, arg2, arg3) ({ \
> +
> +#define SBI_EXT_LEGACY_SET_TIMER 0x0
> +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
> +#define SBI_EXT_LEGACY_SEND_IPI 0x4
> +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
> +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
> +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> +#define SBI_EXT_LEGACY_SHUTDOWN 0x8

I can't say I'm closely following RISC-V development, but what will happen
when SBI v0.3 will come out and will render v0.2 legacy?
Won't we need another similar renaming then?

> +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
> register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
> register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
> @@ -32,58 +33,61 @@
> })
>
> /* Lazy implementations until SBI is finalized */
> -#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
> -#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
> -#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1, 0, 0)
> -#define SBI_CALL_3(which, arg0, arg1, arg2) \
> - SBI_CALL(which, arg0, arg1, arg2, 0)
> -#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
> - SBI_CALL(which, arg0, arg1, arg2, arg3)
> +#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which, arg0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
> + SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
> +#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
> + SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
> +#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
> + SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
>
> static inline void sbi_console_putchar(int ch)
> {
> - SBI_CALL_1(SBI_CONSOLE_PUTCHAR, ch);
> + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_CONSOLE_PUTCHAR, ch);
> }
>
> static inline int sbi_console_getchar(void)
> {
> - return SBI_CALL_0(SBI_CONSOLE_GETCHAR);
> + return SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CONSOLE_GETCHAR);
> }
>
> static inline void sbi_set_timer(uint64_t stime_value)
> {
> #if __riscv_xlen == 32
> - SBI_CALL_2(SBI_SET_TIMER, stime_value, stime_value >> 32);
> + SBI_CALL_LEGACY_2(SBI_EXT_LEGACY_SET_TIMER, stime_value,
> + stime_value >> 32);
> #else
> - SBI_CALL_1(SBI_SET_TIMER, stime_value);
> + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SET_TIMER, stime_value);
> #endif
> }
>
> static inline void sbi_shutdown(void)
> {
> - SBI_CALL_0(SBI_SHUTDOWN);
> + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_SHUTDOWN);
> }
>
> static inline void sbi_clear_ipi(void)
> {
> - SBI_CALL_0(SBI_CLEAR_IPI);
> + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CLEAR_IPI);
> }
>
> static inline void sbi_send_ipi(const unsigned long *hart_mask)
> {
> - SBI_CALL_1(SBI_SEND_IPI, hart_mask);
> + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SEND_IPI, hart_mask);
> }
>
> static inline void sbi_remote_fence_i(const unsigned long *hart_mask)
> {
> - SBI_CALL_1(SBI_REMOTE_FENCE_I, hart_mask);
> + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_REMOTE_FENCE_I, hart_mask);
> }
>
> static inline void sbi_remote_sfence_vma(const unsigned long *hart_mask,
> unsigned long start,
> unsigned long size)
> {
> - SBI_CALL_3(SBI_REMOTE_SFENCE_VMA, hart_mask, start, size);
> + SBI_CALL_LEGACY_3(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA, hart_mask,
> + start, size);
> }
>
> static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> @@ -91,7 +95,8 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> unsigned long size,
> unsigned long asid)
> {
> - SBI_CALL_4(SBI_REMOTE_SFENCE_VMA_ASID, hart_mask, start, size, asid);
> + SBI_CALL_LEGACY_4(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID, hart_mask,
> + start, size, asid);
> }
>
> #endif
> --
> 2.21.0
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

--
Sincerely yours,
Mike.

2019-08-27 08:30:07

by Anup Patel

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Tue, Aug 27, 2019 at 1:21 PM Mike Rapoport <[email protected]> wrote:
>
> On Mon, Aug 26, 2019 at 04:32:55PM -0700, Atish Patra wrote:
> > As per the new SBI specification, current SBI implementation is
> > defined as legacy and will be removed/replaced in future.
> >
> > Rename existing implementation to reflect that. This patch is just
> > a preparatory patch for SBI v0.2 and doesn't introduce any functional
> > changes.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/sbi.h | 61 +++++++++++++++++++-----------------
> > 1 file changed, 33 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 21134b3ef404..7f5ecaaaa0d7 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -8,17 +8,18 @@
> >
> > #include <linux/types.h>
> >
> > -#define SBI_SET_TIMER 0
> > -#define SBI_CONSOLE_PUTCHAR 1
> > -#define SBI_CONSOLE_GETCHAR 2
> > -#define SBI_CLEAR_IPI 3
> > -#define SBI_SEND_IPI 4
> > -#define SBI_REMOTE_FENCE_I 5
> > -#define SBI_REMOTE_SFENCE_VMA 6
> > -#define SBI_REMOTE_SFENCE_VMA_ASID 7
> > -#define SBI_SHUTDOWN 8
> > -
> > -#define SBI_CALL(which, arg0, arg1, arg2, arg3) ({ \
> > +
> > +#define SBI_EXT_LEGACY_SET_TIMER 0x0
> > +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
> > +#define SBI_EXT_LEGACY_SEND_IPI 0x4
> > +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > +#define SBI_EXT_LEGACY_SHUTDOWN 0x8
>
> I can't say I'm closely following RISC-V development, but what will happen
> when SBI v0.3 will come out and will render v0.2 legacy?
> Won't we need another similar renaming then?

Going forward with SBI v0.3 and higher, we won't see any calling
convention changes.

The SBI spec will be maintained and improved by RISC-V UNIX
platform spec working group.

My best guess is that, all future SBI releases (v0.3 or higher) will
include more optional SBI extensions (hart hotplug, power management, etc).

Regards,
Anup

>
> > +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> > register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
> > register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
> > register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
> > @@ -32,58 +33,61 @@
> > })
> >
> > /* Lazy implementations until SBI is finalized */
> > -#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
> > -#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
> > -#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1, 0, 0)
> > -#define SBI_CALL_3(which, arg0, arg1, arg2) \
> > - SBI_CALL(which, arg0, arg1, arg2, 0)
> > -#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
> > - SBI_CALL(which, arg0, arg1, arg2, arg3)
> > +#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0, 0)
> > +#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which, arg0, 0, 0, 0)
> > +#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
> > +#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
> > +#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
> >
> > static inline void sbi_console_putchar(int ch)
> > {
> > - SBI_CALL_1(SBI_CONSOLE_PUTCHAR, ch);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_CONSOLE_PUTCHAR, ch);
> > }
> >
> > static inline int sbi_console_getchar(void)
> > {
> > - return SBI_CALL_0(SBI_CONSOLE_GETCHAR);
> > + return SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CONSOLE_GETCHAR);
> > }
> >
> > static inline void sbi_set_timer(uint64_t stime_value)
> > {
> > #if __riscv_xlen == 32
> > - SBI_CALL_2(SBI_SET_TIMER, stime_value, stime_value >> 32);
> > + SBI_CALL_LEGACY_2(SBI_EXT_LEGACY_SET_TIMER, stime_value,
> > + stime_value >> 32);
> > #else
> > - SBI_CALL_1(SBI_SET_TIMER, stime_value);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SET_TIMER, stime_value);
> > #endif
> > }
> >
> > static inline void sbi_shutdown(void)
> > {
> > - SBI_CALL_0(SBI_SHUTDOWN);
> > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_SHUTDOWN);
> > }
> >
> > static inline void sbi_clear_ipi(void)
> > {
> > - SBI_CALL_0(SBI_CLEAR_IPI);
> > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CLEAR_IPI);
> > }
> >
> > static inline void sbi_send_ipi(const unsigned long *hart_mask)
> > {
> > - SBI_CALL_1(SBI_SEND_IPI, hart_mask);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SEND_IPI, hart_mask);
> > }
> >
> > static inline void sbi_remote_fence_i(const unsigned long *hart_mask)
> > {
> > - SBI_CALL_1(SBI_REMOTE_FENCE_I, hart_mask);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_REMOTE_FENCE_I, hart_mask);
> > }
> >
> > static inline void sbi_remote_sfence_vma(const unsigned long *hart_mask,
> > unsigned long start,
> > unsigned long size)
> > {
> > - SBI_CALL_3(SBI_REMOTE_SFENCE_VMA, hart_mask, start, size);
> > + SBI_CALL_LEGACY_3(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA, hart_mask,
> > + start, size);
> > }
> >
> > static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > @@ -91,7 +95,8 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > unsigned long size,
> > unsigned long asid)
> > {
> > - SBI_CALL_4(SBI_REMOTE_SFENCE_VMA_ASID, hart_mask, start, size, asid);
> > + SBI_CALL_LEGACY_4(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID, hart_mask,
> > + start, size, asid);
> > }
> >
> > #endif
> > --
> > 2.21.0
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
>
> --
> Sincerely yours,
> Mike.
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2019-08-27 08:40:11

by Mike Rapoport

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Tue, Aug 27, 2019 at 01:58:03PM +0530, Anup Patel wrote:
> On Tue, Aug 27, 2019 at 1:21 PM Mike Rapoport <[email protected]> wrote:
> >
> > On Mon, Aug 26, 2019 at 04:32:55PM -0700, Atish Patra wrote:
> > > As per the new SBI specification, current SBI implementation is
> > > defined as legacy and will be removed/replaced in future.
> > >
> > > Rename existing implementation to reflect that. This patch is just
> > > a preparatory patch for SBI v0.2 and doesn't introduce any functional
> > > changes.
> > >
> > > Signed-off-by: Atish Patra <[email protected]>
> > > ---
> > > arch/riscv/include/asm/sbi.h | 61 +++++++++++++++++++-----------------
> > > 1 file changed, 33 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > index 21134b3ef404..7f5ecaaaa0d7 100644
> > > --- a/arch/riscv/include/asm/sbi.h
> > > +++ b/arch/riscv/include/asm/sbi.h
> > > @@ -8,17 +8,18 @@
> > >
> > > #include <linux/types.h>
> > >
> > > -#define SBI_SET_TIMER 0
> > > -#define SBI_CONSOLE_PUTCHAR 1
> > > -#define SBI_CONSOLE_GETCHAR 2
> > > -#define SBI_CLEAR_IPI 3
> > > -#define SBI_SEND_IPI 4
> > > -#define SBI_REMOTE_FENCE_I 5
> > > -#define SBI_REMOTE_SFENCE_VMA 6
> > > -#define SBI_REMOTE_SFENCE_VMA_ASID 7
> > > -#define SBI_SHUTDOWN 8
> > > -
> > > -#define SBI_CALL(which, arg0, arg1, arg2, arg3) ({ \
> > > +
> > > +#define SBI_EXT_LEGACY_SET_TIMER 0x0
> > > +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > > +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > > +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
> > > +#define SBI_EXT_LEGACY_SEND_IPI 0x4
> > > +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
> > > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
> > > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > > +#define SBI_EXT_LEGACY_SHUTDOWN 0x8
> >
> > I can't say I'm closely following RISC-V development, but what will happen
> > when SBI v0.3 will come out and will render v0.2 legacy?
> > Won't we need another similar renaming then?
>
> Going forward with SBI v0.3 and higher, we won't see any calling
> convention changes.
>
> The SBI spec will be maintained and improved by RISC-V UNIX
> platform spec working group.
>
> My best guess is that, all future SBI releases (v0.3 or higher) will
> include more optional SBI extensions (hart hotplug, power management, etc).

Thanks for the clarification!

> Regards,
> Anup
>
> >
> > > +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> > > register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
> > > register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
> > > register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
> > > @@ -32,58 +33,61 @@
> > > })
> > >
> > > /* Lazy implementations until SBI is finalized */
> > > -#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
> > > -#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
> > > -#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1, 0, 0)
> > > -#define SBI_CALL_3(which, arg0, arg1, arg2) \
> > > - SBI_CALL(which, arg0, arg1, arg2, 0)
> > > -#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
> > > - SBI_CALL(which, arg0, arg1, arg2, arg3)
> > > +#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0, 0)
> > > +#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which, arg0, 0, 0, 0)
> > > +#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
> > > + SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
> > > +#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
> > > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
> > > +#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
> > > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
> > >
> > > static inline void sbi_console_putchar(int ch)
> > > {
> > > - SBI_CALL_1(SBI_CONSOLE_PUTCHAR, ch);
> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_CONSOLE_PUTCHAR, ch);
> > > }
> > >
> > > static inline int sbi_console_getchar(void)
> > > {
> > > - return SBI_CALL_0(SBI_CONSOLE_GETCHAR);
> > > + return SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CONSOLE_GETCHAR);
> > > }
> > >
> > > static inline void sbi_set_timer(uint64_t stime_value)
> > > {
> > > #if __riscv_xlen == 32
> > > - SBI_CALL_2(SBI_SET_TIMER, stime_value, stime_value >> 32);
> > > + SBI_CALL_LEGACY_2(SBI_EXT_LEGACY_SET_TIMER, stime_value,
> > > + stime_value >> 32);
> > > #else
> > > - SBI_CALL_1(SBI_SET_TIMER, stime_value);
> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SET_TIMER, stime_value);
> > > #endif
> > > }
> > >
> > > static inline void sbi_shutdown(void)
> > > {
> > > - SBI_CALL_0(SBI_SHUTDOWN);
> > > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_SHUTDOWN);
> > > }
> > >
> > > static inline void sbi_clear_ipi(void)
> > > {
> > > - SBI_CALL_0(SBI_CLEAR_IPI);
> > > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CLEAR_IPI);
> > > }
> > >
> > > static inline void sbi_send_ipi(const unsigned long *hart_mask)
> > > {
> > > - SBI_CALL_1(SBI_SEND_IPI, hart_mask);
> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SEND_IPI, hart_mask);
> > > }
> > >
> > > static inline void sbi_remote_fence_i(const unsigned long *hart_mask)
> > > {
> > > - SBI_CALL_1(SBI_REMOTE_FENCE_I, hart_mask);
> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_REMOTE_FENCE_I, hart_mask);
> > > }
> > >
> > > static inline void sbi_remote_sfence_vma(const unsigned long *hart_mask,
> > > unsigned long start,
> > > unsigned long size)
> > > {
> > > - SBI_CALL_3(SBI_REMOTE_SFENCE_VMA, hart_mask, start, size);
> > > + SBI_CALL_LEGACY_3(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA, hart_mask,
> > > + start, size);
> > > }
> > >
> > > static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > > @@ -91,7 +95,8 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > > unsigned long size,
> > > unsigned long asid)
> > > {
> > > - SBI_CALL_4(SBI_REMOTE_SFENCE_VMA_ASID, hart_mask, start, size, asid);
> > > + SBI_CALL_LEGACY_4(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID, hart_mask,
> > > + start, size, asid);
> > > }
> > >
> > > #endif
> > > --
> > > 2.21.0
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
> >
> > --
> > Sincerely yours,
> > Mike.
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Sincerely yours,
Mike.

2019-08-27 14:04:34

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

> +#define SBI_EXT_LEGACY_SET_TIMER 0x0
> +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
> +#define SBI_EXT_LEGACY_SEND_IPI 0x4
> +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
> +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
> +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> +#define SBI_EXT_LEGACY_SHUTDOWN 0x8

As Mike said legacy is a bit of a weird name. I think this should
be SBI01_* or so. And pleae align the numeric values and maybe use
an enum.

> +
> +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
> register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
> register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \

Instead of the weird inline assembly with forced register allocation,
why not move this to pure assembly? AFAICs this is the whole assembly
code we'd need:

ENTRY(sbi01_call)
ecall
END(sbi01_call)

> /* Lazy implementations until SBI is finalized */
> -#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
> -#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
> -#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1, 0, 0)
> -#define SBI_CALL_3(which, arg0, arg1, arg2) \
> - SBI_CALL(which, arg0, arg1, arg2, 0)
> -#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
> - SBI_CALL(which, arg0, arg1, arg2, arg3)
> +#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which, arg0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
> + SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
> +#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
> + SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
> +#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
> + SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)

When you touch this anyway I'd suggest you kill these rather
pointless wrappers as well as the comment above them.

2019-08-27 14:07:24

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Tue, Aug 27, 2019 at 07:03:04AM -0700, Christoph Hellwig wrote:
> > +#define SBI_EXT_LEGACY_SET_TIMER 0x0
> > +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
> > +#define SBI_EXT_LEGACY_SEND_IPI 0x4
> > +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > +#define SBI_EXT_LEGACY_SHUTDOWN 0x8
>
> As Mike said legacy is a bit of a weird name. I think this should
> be SBI01_* or so. And pleae align the numeric values and maybe use
> an enum.
>
> > +
> > +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> > register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
> > register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
> > register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
>
> Instead of the weird inline assembly with forced register allocation,
> why not move this to pure assembly? AFAICs this is the whole assembly
> code we'd need:
>
> ENTRY(sbi01_call)
> ecall
> END(sbi01_call)

Actually we'll need a mv to a7 for the function id, but the point
still stands.

2019-08-27 20:35:53

by Atish Patra

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Tue, 2019-08-27 at 10:51 +0300, Mike Rapoport wrote:
> On Mon, Aug 26, 2019 at 04:32:55PM -0700, Atish Patra wrote:
> > As per the new SBI specification, current SBI implementation is
> > defined as legacy and will be removed/replaced in future.
> >
> > Rename existing implementation to reflect that. This patch is just
> > a preparatory patch for SBI v0.2 and doesn't introduce any
> > functional
> > changes.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/sbi.h | 61 +++++++++++++++++++-------------
> > ----
> > 1 file changed, 33 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h
> > b/arch/riscv/include/asm/sbi.h
> > index 21134b3ef404..7f5ecaaaa0d7 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -8,17 +8,18 @@
> >
> > #include <linux/types.h>
> >
> > -#define SBI_SET_TIMER 0
> > -#define SBI_CONSOLE_PUTCHAR 1
> > -#define SBI_CONSOLE_GETCHAR 2
> > -#define SBI_CLEAR_IPI 3
> > -#define SBI_SEND_IPI 4
> > -#define SBI_REMOTE_FENCE_I 5
> > -#define SBI_REMOTE_SFENCE_VMA 6
> > -#define SBI_REMOTE_SFENCE_VMA_ASID 7
> > -#define SBI_SHUTDOWN 8
> > -
> > -#define SBI_CALL(which, arg0, arg1, arg2, arg3) ({ \
> > +
> > +#define SBI_EXT_LEGACY_SET_TIMER 0x0
> > +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
> > +#define SBI_EXT_LEGACY_SEND_IPI 0x4
> > +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > +#define SBI_EXT_LEGACY_SHUTDOWN 0x8
>
> I can't say I'm closely following RISC-V development, but what will
> happen
> when SBI v0.3 will come out and will render v0.2 legacy?
> Won't we need another similar renaming then?
>

Noope.The idea of renaming 0.1 calls as legacy comes from the fact that
these calls will be eventually be replaced or depcreated after a
certain time.

v0.2 defines base specification which will be mandatory for any newer
implementation. Any versions newer than 0.2 will only add new
extensions but not necessarily replace the ones in 0.2

> > +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
> > ({ \
> > register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
> > register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
> > register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
> > @@ -32,58 +33,61 @@
> > })
> >
> > /* Lazy implementations until SBI is finalized */
> > -#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
> > -#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
> > -#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1,
> > 0, 0)
> > -#define SBI_CALL_3(which, arg0, arg1, arg2) \
> > - SBI_CALL(which, arg0, arg1, arg2, 0)
> > -#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
> > - SBI_CALL(which, arg0, arg1, arg2, arg3)
> > +#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0,
> > 0)
> > +#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which,
> > arg0, 0, 0, 0)
> > +#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
> > +#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
> > +#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
> >
> > static inline void sbi_console_putchar(int ch)
> > {
> > - SBI_CALL_1(SBI_CONSOLE_PUTCHAR, ch);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_CONSOLE_PUTCHAR, ch);
> > }
> >
> > static inline int sbi_console_getchar(void)
> > {
> > - return SBI_CALL_0(SBI_CONSOLE_GETCHAR);
> > + return SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CONSOLE_GETCHAR);
> > }
> >
> > static inline void sbi_set_timer(uint64_t stime_value)
> > {
> > #if __riscv_xlen == 32
> > - SBI_CALL_2(SBI_SET_TIMER, stime_value, stime_value >> 32);
> > + SBI_CALL_LEGACY_2(SBI_EXT_LEGACY_SET_TIMER, stime_value,
> > + stime_value >> 32);
> > #else
> > - SBI_CALL_1(SBI_SET_TIMER, stime_value);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SET_TIMER, stime_value);
> > #endif
> > }
> >
> > static inline void sbi_shutdown(void)
> > {
> > - SBI_CALL_0(SBI_SHUTDOWN);
> > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_SHUTDOWN);
> > }
> >
> > static inline void sbi_clear_ipi(void)
> > {
> > - SBI_CALL_0(SBI_CLEAR_IPI);
> > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CLEAR_IPI);
> > }
> >
> > static inline void sbi_send_ipi(const unsigned long *hart_mask)
> > {
> > - SBI_CALL_1(SBI_SEND_IPI, hart_mask);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SEND_IPI, hart_mask);
> > }
> >
> > static inline void sbi_remote_fence_i(const unsigned long
> > *hart_mask)
> > {
> > - SBI_CALL_1(SBI_REMOTE_FENCE_I, hart_mask);
> > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_REMOTE_FENCE_I, hart_mask);
> > }
> >
> > static inline void sbi_remote_sfence_vma(const unsigned long
> > *hart_mask,
> > unsigned long start,
> > unsigned long size)
> > {
> > - SBI_CALL_3(SBI_REMOTE_SFENCE_VMA, hart_mask, start, size);
> > + SBI_CALL_LEGACY_3(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA, hart_mask,
> > + start, size);
> > }
> >
> > static inline void sbi_remote_sfsimilarence_vma_asid(const
> > unsigned long *hart_mask,
> > @@ -91,7 +95,8 @@ static inline void
> > sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > unsigned long size,
> > unsigned long asid)
> > {
> > - SBI_CALL_4(SBI_REMOTE_SFENCE_VMA_ASID, hart_mask, start, size,
> > asid);
> > + SBI_CALL_LEGACY_4(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID,
> > hart_mask,
> > + start, size, asid);
> > }
> >
> > #endif
> > --
> > 2.21.0
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >

--
Regards,
Atish

2019-08-27 20:40:37

by Atish Patra

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Tue, 2019-08-27 at 07:03 -0700, Christoph Hellwig wrote:
> > +#define SBI_EXT_LEGACY_SET_TIMER 0x0
> > +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
> > +#define SBI_EXT_LEGACY_SEND_IPI 0x4
> > +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
> > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > +#define SBI_EXT_LEGACY_SHUTDOWN 0x8
>
> As Mike said legacy is a bit of a weird name. I think this should
> be SBI01_* or so. And pleae align the numeric values and maybe use
> an enum.
>
Will do.

> > +
> > +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
> > ({ \
> > register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
> > register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
> > register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
>
> Instead of the weird inline assembly with forced register allocation,
> why not move this to pure assembly? AFAICs this is the whole
> assembly
> code we'd need:
>
> ENTRY(sbi01_call)
> ecall
> END(sbi01_call)
>

That would split the implementation between C file & assembly file for
no good reason.

How about moving everything in sbi.c and just write everything inline
assembly there.

> > /* Lazy implementations until SBI is finalized */
> > -#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
> > -#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
> > -#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1,
> > 0, 0)
> > -#define SBI_CALL_3(which, arg0, arg1, arg2) \
> > - SBI_CALL(which, arg0, arg1, arg2, 0)
> > -#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
> > - SBI_CALL(which, arg0, arg1, arg2, arg3)
> > +#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0,
> > 0)
> > +#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which,
> > arg0, 0, 0, 0)
> > +#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
> > +#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
> > +#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
> > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
>
> When you touch this anyway I'd suggest you kill these rather
> pointless wrappers as well as the comment above them.

Sure. I did not want make bigger chagnes in 1st RFC patches. But
looking at the comments, I feel it was not a good decission.

I will get rid of these wrappers and move necessary ones to sbi.c

--
Regards,
Atish

2019-08-28 21:38:57

by Palmer Dabbelt

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Tue, 27 Aug 2019 01:37:00 PDT (-0700), [email protected] wrote:
> On Tue, Aug 27, 2019 at 01:58:03PM +0530, Anup Patel wrote:
>> On Tue, Aug 27, 2019 at 1:21 PM Mike Rapoport <[email protected]> wrote:
>> >
>> > On Mon, Aug 26, 2019 at 04:32:55PM -0700, Atish Patra wrote:
>> > > As per the new SBI specification, current SBI implementation is
>> > > defined as legacy and will be removed/replaced in future.
>> > >
>> > > Rename existing implementation to reflect that. This patch is just
>> > > a preparatory patch for SBI v0.2 and doesn't introduce any functional
>> > > changes.
>> > >
>> > > Signed-off-by: Atish Patra <[email protected]>
>> > > ---
>> > > arch/riscv/include/asm/sbi.h | 61 +++++++++++++++++++-----------------
>> > > 1 file changed, 33 insertions(+), 28 deletions(-)
>> > >
>> > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>> > > index 21134b3ef404..7f5ecaaaa0d7 100644
>> > > --- a/arch/riscv/include/asm/sbi.h
>> > > +++ b/arch/riscv/include/asm/sbi.h
>> > > @@ -8,17 +8,18 @@
>> > >
>> > > #include <linux/types.h>
>> > >
>> > > -#define SBI_SET_TIMER 0
>> > > -#define SBI_CONSOLE_PUTCHAR 1
>> > > -#define SBI_CONSOLE_GETCHAR 2
>> > > -#define SBI_CLEAR_IPI 3
>> > > -#define SBI_SEND_IPI 4
>> > > -#define SBI_REMOTE_FENCE_I 5
>> > > -#define SBI_REMOTE_SFENCE_VMA 6
>> > > -#define SBI_REMOTE_SFENCE_VMA_ASID 7
>> > > -#define SBI_SHUTDOWN 8
>> > > -
>> > > -#define SBI_CALL(which, arg0, arg1, arg2, arg3) ({ \
>> > > +
>> > > +#define SBI_EXT_LEGACY_SET_TIMER 0x0
>> > > +#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
>> > > +#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
>> > > +#define SBI_EXT_LEGACY_CLEAR_IPI 0x3
>> > > +#define SBI_EXT_LEGACY_SEND_IPI 0x4
>> > > +#define SBI_EXT_LEGACY_REMOTE_FENCE_I 0x5
>> > > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA 0x6
>> > > +#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
>> > > +#define SBI_EXT_LEGACY_SHUTDOWN 0x8
>> >
>> > I can't say I'm closely following RISC-V development, but what will happen
>> > when SBI v0.3 will come out and will render v0.2 legacy?
>> > Won't we need another similar renaming then?
>>
>> Going forward with SBI v0.3 and higher, we won't see any calling
>> convention changes.
>>
>> The SBI spec will be maintained and improved by RISC-V UNIX
>> platform spec working group.
>>
>> My best guess is that, all future SBI releases (v0.3 or higher) will
>> include more optional SBI extensions (hart hotplug, power management, etc).
>
> Thanks for the clarification!

Specifically the issue is that 0.1 didn't include any mechanism for probing the
existence of a function, but since that has been added in 0.2 it's possible to
maintain compatible with future versions.

>
>> Regards,
>> Anup
>>
>> >
>> > > +#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
>> > > register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \
>> > > register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \
>> > > register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \
>> > > @@ -32,58 +33,61 @@
>> > > })
>> > >
>> > > /* Lazy implementations until SBI is finalized */
>> > > -#define SBI_CALL_0(which) SBI_CALL(which, 0, 0, 0, 0)
>> > > -#define SBI_CALL_1(which, arg0) SBI_CALL(which, arg0, 0, 0, 0)
>> > > -#define SBI_CALL_2(which, arg0, arg1) SBI_CALL(which, arg0, arg1, 0, 0)
>> > > -#define SBI_CALL_3(which, arg0, arg1, arg2) \
>> > > - SBI_CALL(which, arg0, arg1, arg2, 0)
>> > > -#define SBI_CALL_4(which, arg0, arg1, arg2, arg3) \
>> > > - SBI_CALL(which, arg0, arg1, arg2, arg3)
>> > > +#define SBI_CALL_LEGACY_0(which) SBI_CALL_LEGACY(which, 0, 0, 0, 0)
>> > > +#define SBI_CALL_LEGACY_1(which, arg0) SBI_CALL_LEGACY(which, arg0, 0, 0, 0)
>> > > +#define SBI_CALL_LEGACY_2(which, arg0, arg1) \
>> > > + SBI_CALL_LEGACY(which, arg0, arg1, 0, 0)
>> > > +#define SBI_CALL_LEGACY_3(which, arg0, arg1, arg2) \
>> > > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, 0)
>> > > +#define SBI_CALL_LEGACY_4(which, arg0, arg1, arg2, arg3) \
>> > > + SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
>> > >
>> > > static inline void sbi_console_putchar(int ch)
>> > > {
>> > > - SBI_CALL_1(SBI_CONSOLE_PUTCHAR, ch);
>> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_CONSOLE_PUTCHAR, ch);
>> > > }
>> > >
>> > > static inline int sbi_console_getchar(void)
>> > > {
>> > > - return SBI_CALL_0(SBI_CONSOLE_GETCHAR);
>> > > + return SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CONSOLE_GETCHAR);
>> > > }
>> > >
>> > > static inline void sbi_set_timer(uint64_t stime_value)
>> > > {
>> > > #if __riscv_xlen == 32
>> > > - SBI_CALL_2(SBI_SET_TIMER, stime_value, stime_value >> 32);
>> > > + SBI_CALL_LEGACY_2(SBI_EXT_LEGACY_SET_TIMER, stime_value,
>> > > + stime_value >> 32);
>> > > #else
>> > > - SBI_CALL_1(SBI_SET_TIMER, stime_value);
>> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SET_TIMER, stime_value);
>> > > #endif
>> > > }
>> > >
>> > > static inline void sbi_shutdown(void)
>> > > {
>> > > - SBI_CALL_0(SBI_SHUTDOWN);
>> > > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_SHUTDOWN);
>> > > }
>> > >
>> > > static inline void sbi_clear_ipi(void)
>> > > {
>> > > - SBI_CALL_0(SBI_CLEAR_IPI);
>> > > + SBI_CALL_LEGACY_0(SBI_EXT_LEGACY_CLEAR_IPI);
>> > > }
>> > >
>> > > static inline void sbi_send_ipi(const unsigned long *hart_mask)
>> > > {
>> > > - SBI_CALL_1(SBI_SEND_IPI, hart_mask);
>> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_SEND_IPI, hart_mask);
>> > > }
>> > >
>> > > static inline void sbi_remote_fence_i(const unsigned long *hart_mask)
>> > > {
>> > > - SBI_CALL_1(SBI_REMOTE_FENCE_I, hart_mask);
>> > > + SBI_CALL_LEGACY_1(SBI_EXT_LEGACY_REMOTE_FENCE_I, hart_mask);
>> > > }
>> > >
>> > > static inline void sbi_remote_sfence_vma(const unsigned long *hart_mask,
>> > > unsigned long start,
>> > > unsigned long size)
>> > > {
>> > > - SBI_CALL_3(SBI_REMOTE_SFENCE_VMA, hart_mask, start, size);
>> > > + SBI_CALL_LEGACY_3(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA, hart_mask,
>> > > + start, size);
>> > > }
>> > >
>> > > static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
>> > > @@ -91,7 +95,8 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
>> > > unsigned long size,
>> > > unsigned long asid)
>> > > {
>> > > - SBI_CALL_4(SBI_REMOTE_SFENCE_VMA_ASID, hart_mask, start, size, asid);
>> > > + SBI_CALL_LEGACY_4(SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID, hart_mask,
>> > > + start, size, asid);
>> > > }
>> > >
>> > > #endif
>> > > --
>> > > 2.21.0
>> > >
>> > >
>> > > _______________________________________________
>> > > linux-riscv mailing list
>> > > [email protected]
>> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
>> > >
>> >
>> > --
>> > Sincerely yours,
>> > Mike.
>> >
>> >
>> > _______________________________________________
>> > linux-riscv mailing list
>> > [email protected]
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

2019-08-29 10:58:44

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI.

On Tue, Aug 27, 2019 at 08:37:27PM +0000, Atish Patra wrote:
> That would split the implementation between C file & assembly file for
> no good reason.
>
> How about moving everything in sbi.c and just write everything inline
> assembly there.

Well, if we implement it in pure assembly that would be the entire
implementation, wouldn't it?