This patch series aims to add support for SBI specification version
v0.2. It doesn't break compatibility with any v0.1 implementation.
Internally, all the v0.1 calls are just renamed to legacy to be in
sync with specification [1].
The patches for v0.2 support in OpenSBI are available at
http://lists.infradead.org/pipermail/opensbi/2019-August/000422.html
[1] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
Atish Patra (2):
RISC-V: Mark existing SBI as legacy SBI.
RISC-V: Add basic support for SBI v0.2
arch/riscv/include/asm/sbi.h | 109 +++++++++++++++++++++++++----------
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/sbi.c | 50 ++++++++++++++++
arch/riscv/kernel/setup.c | 2 +
4 files changed, 131 insertions(+), 31 deletions(-)
create mode 100644 arch/riscv/kernel/sbi.c
--
2.21.0
The SBI v0.2 introduces a base extension which is backward compatible
with v0.1. Implement all helper functions and minimum required SBI
calls from v0.2 for now. All other base extension function will be
added later as per need.
Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/sbi.h | 68 +++++++++++++++++++++++++++++-------
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
arch/riscv/kernel/setup.c | 2 ++
4 files changed, 108 insertions(+), 13 deletions(-)
create mode 100644 arch/riscv/kernel/sbi.c
diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 7f5ecaaaa0d7..4a4476956693 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -8,7 +8,6 @@
#include <linux/types.h>
-
#define SBI_EXT_LEGACY_SET_TIMER 0x0
#define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
#define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
@@ -19,28 +18,61 @@
#define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
#define SBI_EXT_LEGACY_SHUTDOWN 0x8
-#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
+#define SBI_EXT_BASE 0x10
+
+enum sbi_ext_base_fid {
+ SBI_EXT_BASE_GET_SPEC_VERSION = 0,
+ SBI_EXT_BASE_GET_IMP_ID,
+ SBI_EXT_BASE_GET_IMP_VERSION,
+ SBI_EXT_BASE_PROBE_EXT,
+ SBI_EXT_BASE_GET_MVENDORID,
+ SBI_EXT_BASE_GET_MARCHID,
+ SBI_EXT_BASE_GET_MIMPID,
+};
+
+#define SBI_CALL_LEGACY(ext, fid, 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); \
register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
- register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
+ register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
+ register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
asm volatile ("ecall" \
- : "+r" (a0) \
- : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
+ : "+r" (a0), "+r" (a1) \
+ : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
: "memory"); \
a0; \
})
/* Lazy implementations until SBI is finalized */
-#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)
+#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0, 0, 0)
+#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0, arg0, 0, 0, 0)
+#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
+ SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
+#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
+ SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
+#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
+ SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
+
+extern unsigned long sbi_firmware_version;
+struct sbiret {
+ long error;
+ long value;
+};
+
+void riscv_sbi_init(void);
+struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
+ int arg2, int arg3);
+
+#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
+#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0)
+#define SBI_CALL_2(ext, fid, arg0, arg1) \
+ riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
+#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
+ riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
+#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
+ riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
+
static inline void sbi_console_putchar(int ch)
{
@@ -99,4 +131,14 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
start, size, asid);
}
+static inline unsigned long riscv_sbi_major_version(void)
+{
+ return (sbi_firmware_version >> 24) & 0x7f;
+}
+
+static inline unsigned long riscv_sbi_minor_version(void)
+{
+ return sbi_firmware_version & 0xffffff;
+}
+
#endif
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 2420d37d96de..faf862d26924 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -17,6 +17,7 @@ obj-y += irq.o
obj-y += process.o
obj-y += ptrace.o
obj-y += reset.o
+obj-y += sbi.o
obj-y += setup.o
obj-y += signal.o
obj-y += syscall_table.o
diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
new file mode 100644
index 000000000000..457b8cc0e9d9
--- /dev/null
+++ b/arch/riscv/kernel/sbi.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SBI initialilization and base extension implementation.
+ *
+ * Copyright (c) 2019 Western Digital Corporation or its affiliates.
+ */
+
+#include <asm/sbi.h>
+#include <linux/sched.h>
+
+unsigned long sbi_firmware_version;
+
+struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
+ int arg2, int arg3)
+{
+ struct sbiret ret;
+
+ 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);
+ register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
+ register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
+ register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
+ asm volatile ("ecall"
+ : "+r" (a0), "+r" (a1)
+ : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
+ : "memory");
+ ret.error = a0;
+ ret.value = a1;
+
+ return ret;
+}
+
+static struct sbiret sbi_get_spec_version(void)
+{
+ return SBI_CALL_0(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION);
+}
+
+void riscv_sbi_init(void)
+{
+ struct sbiret ret;
+
+ /* legacy SBI version*/
+ sbi_firmware_version = 0x1;
+ ret = sbi_get_spec_version();
+ if (!ret.error)
+ sbi_firmware_version = ret.value;
+ pr_info("SBI version implemented in firmware [%lu:%lu]\n",
+ riscv_sbi_major_version(), riscv_sbi_minor_version());
+}
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index a990a6cb184f..4c324fd398c8 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -21,6 +21,7 @@
#include <asm/sections.h>
#include <asm/pgtable.h>
#include <asm/smp.h>
+#include <asm/sbi.h>
#include <asm/tlbflush.h>
#include <asm/thread_info.h>
@@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
swiotlb_init(1);
#endif
+ riscv_sbi_init();
#ifdef CONFIG_SMP
setup_smp();
#endif
--
2.21.0
On Mon, Aug 26, 2019 at 04:32:56PM -0700, Atish Patra wrote:
> The SBI v0.2 introduces a base extension which is backward compatible
> with v0.1. Implement all helper functions and minimum required SBI
> calls from v0.2 for now. All other base extension function will be
> added later as per need.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/sbi.h | 68 +++++++++++++++++++++++++++++-------
> arch/riscv/kernel/Makefile | 1 +
> arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
> arch/riscv/kernel/setup.c | 2 ++
> 4 files changed, 108 insertions(+), 13 deletions(-)
> create mode 100644 arch/riscv/kernel/sbi.c
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 7f5ecaaaa0d7..4a4476956693 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -8,7 +8,6 @@
>
> #include <linux/types.h>
>
> -
> #define SBI_EXT_LEGACY_SET_TIMER 0x0
> #define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> #define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> @@ -19,28 +18,61 @@
> #define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> #define SBI_EXT_LEGACY_SHUTDOWN 0x8
>
> -#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> +#define SBI_EXT_BASE 0x10
> +
> +enum sbi_ext_base_fid {
> + SBI_EXT_BASE_GET_SPEC_VERSION = 0,
> + SBI_EXT_BASE_GET_IMP_ID,
> + SBI_EXT_BASE_GET_IMP_VERSION,
> + SBI_EXT_BASE_PROBE_EXT,
> + SBI_EXT_BASE_GET_MVENDORID,
> + SBI_EXT_BASE_GET_MARCHID,
> + SBI_EXT_BASE_GET_MIMPID,
> +};
> +
> +#define SBI_CALL_LEGACY(ext, fid, 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); \
> register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
> asm volatile ("ecall" \
> - : "+r" (a0) \
> - : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
> + : "+r" (a0), "+r" (a1) \
> + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
Maybe I'm missing something, but how is this supposed to work on systems
with SBI v0.1? Wouldn't this cause a mismatch in the registers?
> : "memory"); \
> a0; \
> })
>
> /* Lazy implementations until SBI is finalized */
> -#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)
> +#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0, arg0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
> + SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
> +#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
> + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
> +#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
> + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
> +
> +extern unsigned long sbi_firmware_version;
> +struct sbiret {
> + long error;
> + long value;
> +};
> +
> +void riscv_sbi_init(void);
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> + int arg2, int arg3);
> +
> +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
> +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0)
> +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
> +
>
> static inline void sbi_console_putchar(int ch)
> {
> @@ -99,4 +131,14 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> start, size, asid);
> }
>
> +static inline unsigned long riscv_sbi_major_version(void)
> +{
> + return (sbi_firmware_version >> 24) & 0x7f;
> +}
> +
> +static inline unsigned long riscv_sbi_minor_version(void)
> +{
> + return sbi_firmware_version & 0xffffff;
> +}
> +
> #endif
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index 2420d37d96de..faf862d26924 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -17,6 +17,7 @@ obj-y += irq.o
> obj-y += process.o
> obj-y += ptrace.o
> obj-y += reset.o
> +obj-y += sbi.o
> obj-y += setup.o
> obj-y += signal.o
> obj-y += syscall_table.o
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> new file mode 100644
> index 000000000000..457b8cc0e9d9
> --- /dev/null
> +++ b/arch/riscv/kernel/sbi.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SBI initialilization and base extension implementation.
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + */
> +
> +#include <asm/sbi.h>
> +#include <linux/sched.h>
> +
> +unsigned long sbi_firmware_version;
> +
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> + int arg2, int arg3)
> +{
> + struct sbiret ret;
> +
> + 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);
> + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> + asm volatile ("ecall"
> + : "+r" (a0), "+r" (a1)
> + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> + : "memory");
> + ret.error = a0;
> + ret.value = a1;
> +
> + return ret;
> +}
> +
> +static struct sbiret sbi_get_spec_version(void)
> +{
> + return SBI_CALL_0(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION);
> +}
> +
> +void riscv_sbi_init(void)
> +{
> + struct sbiret ret;
> +
> + /* legacy SBI version*/
> + sbi_firmware_version = 0x1;
> + ret = sbi_get_spec_version();
> + if (!ret.error)
> + sbi_firmware_version = ret.value;
> + pr_info("SBI version implemented in firmware [%lu:%lu]\n",
> + riscv_sbi_major_version(), riscv_sbi_minor_version());
> +}
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index a990a6cb184f..4c324fd398c8 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -21,6 +21,7 @@
> #include <asm/sections.h>
> #include <asm/pgtable.h>
> #include <asm/smp.h>
> +#include <asm/sbi.h>
> #include <asm/tlbflush.h>
> #include <asm/thread_info.h>
>
> @@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
> swiotlb_init(1);
> #endif
>
> + riscv_sbi_init();
> #ifdef CONFIG_SMP
> setup_smp();
> #endif
> --
> 2.21.0
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Sincerely yours,
Mike.
On Tue, Aug 27, 2019 at 1:28 PM Mike Rapoport <[email protected]> wrote:
>
> On Mon, Aug 26, 2019 at 04:32:56PM -0700, Atish Patra wrote:
> > The SBI v0.2 introduces a base extension which is backward compatible
> > with v0.1. Implement all helper functions and minimum required SBI
> > calls from v0.2 for now. All other base extension function will be
> > added later as per need.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/sbi.h | 68 +++++++++++++++++++++++++++++-------
> > arch/riscv/kernel/Makefile | 1 +
> > arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
> > arch/riscv/kernel/setup.c | 2 ++
> > 4 files changed, 108 insertions(+), 13 deletions(-)
> > create mode 100644 arch/riscv/kernel/sbi.c
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 7f5ecaaaa0d7..4a4476956693 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -8,7 +8,6 @@
> >
> > #include <linux/types.h>
> >
> > -
> > #define SBI_EXT_LEGACY_SET_TIMER 0x0
> > #define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > #define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > @@ -19,28 +18,61 @@
> > #define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > #define SBI_EXT_LEGACY_SHUTDOWN 0x8
> >
> > -#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> > +#define SBI_EXT_BASE 0x10
> > +
> > +enum sbi_ext_base_fid {
> > + SBI_EXT_BASE_GET_SPEC_VERSION = 0,
> > + SBI_EXT_BASE_GET_IMP_ID,
> > + SBI_EXT_BASE_GET_IMP_VERSION,
> > + SBI_EXT_BASE_PROBE_EXT,
> > + SBI_EXT_BASE_GET_MVENDORID,
> > + SBI_EXT_BASE_GET_MARCHID,
> > + SBI_EXT_BASE_GET_MIMPID,
> > +};
> > +
> > +#define SBI_CALL_LEGACY(ext, fid, 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); \
> > register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> > - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
> > asm volatile ("ecall" \
> > - : "+r" (a0) \
> > - : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
> > + : "+r" (a0), "+r" (a1) \
> > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
>
> Maybe I'm missing something, but how is this supposed to work on systems
> with SBI v0.1? Wouldn't this cause a mismatch in the registers?
The SBI v0.2 has two major changes:
1. New improved calling convention which is backward compatible
with SBI v0.1 so older kernels with SBI v0.1 will continue to work as-is.
2. Base set of mandatory SBI v0.2 calls which can be used to detect
SBI version, check supported SBI calls and extentions.
Old calling convention in SBI v0.1 was:
Parameters:
a0 -> arg0
a1 -> arg1
a2 -> arg2
a3 -> arg3
a7 -> function_id
Return:
a0 -> return value or error code
In SBI v0.2, we have extension and function. Each SBI extension
is a set of function. The new calling convention in SBI v0.2 is:
Parameters:
a0 -> arg0
a1 -> arg1
a2 -> arg2
a3 -> arg3
a6 -> function_id
a7 -> extension_id
Return:
a0 -> error code
a1 -> return value (optional)
All legacy SBI v0.1 functions can be thought of as separate
extensions. That's how SBI v0.2 will be backward compatible.
Regards,
Anup
>
> > : "memory"); \
> > a0; \
> > })
> >
> > /* Lazy implementations until SBI is finalized */
> > -#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)
> > +#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0, 0, 0)
> > +#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0, arg0, 0, 0, 0)
> > +#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
> > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
> > +#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
> > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
> > +#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
> > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
> > +
> > +extern unsigned long sbi_firmware_version;
> > +struct sbiret {
> > + long error;
> > + long value;
> > +};
> > +
> > +void riscv_sbi_init(void);
> > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> > + int arg2, int arg3);
> > +
> > +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
> > +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0)
> > +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> > + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> > +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> > +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
> > +
> >
> > static inline void sbi_console_putchar(int ch)
> > {
> > @@ -99,4 +131,14 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > start, size, asid);
> > }
> >
> > +static inline unsigned long riscv_sbi_major_version(void)
> > +{
> > + return (sbi_firmware_version >> 24) & 0x7f;
> > +}
> > +
> > +static inline unsigned long riscv_sbi_minor_version(void)
> > +{
> > + return sbi_firmware_version & 0xffffff;
> > +}
> > +
> > #endif
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index 2420d37d96de..faf862d26924 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -17,6 +17,7 @@ obj-y += irq.o
> > obj-y += process.o
> > obj-y += ptrace.o
> > obj-y += reset.o
> > +obj-y += sbi.o
> > obj-y += setup.o
> > obj-y += signal.o
> > obj-y += syscall_table.o
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > new file mode 100644
> > index 000000000000..457b8cc0e9d9
> > --- /dev/null
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -0,0 +1,50 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * SBI initialilization and base extension implementation.
> > + *
> > + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > + */
> > +
> > +#include <asm/sbi.h>
> > +#include <linux/sched.h>
> > +
> > +unsigned long sbi_firmware_version;
> > +
> > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> > + int arg2, int arg3)
> > +{
> > + struct sbiret ret;
> > +
> > + 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);
> > + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> > + asm volatile ("ecall"
> > + : "+r" (a0), "+r" (a1)
> > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> > + : "memory");
> > + ret.error = a0;
> > + ret.value = a1;
> > +
> > + return ret;
> > +}
> > +
> > +static struct sbiret sbi_get_spec_version(void)
> > +{
> > + return SBI_CALL_0(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION);
> > +}
> > +
> > +void riscv_sbi_init(void)
> > +{
> > + struct sbiret ret;
> > +
> > + /* legacy SBI version*/
> > + sbi_firmware_version = 0x1;
> > + ret = sbi_get_spec_version();
> > + if (!ret.error)
> > + sbi_firmware_version = ret.value;
> > + pr_info("SBI version implemented in firmware [%lu:%lu]\n",
> > + riscv_sbi_major_version(), riscv_sbi_minor_version());
> > +}
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index a990a6cb184f..4c324fd398c8 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -21,6 +21,7 @@
> > #include <asm/sections.h>
> > #include <asm/pgtable.h>
> > #include <asm/smp.h>
> > +#include <asm/sbi.h>
> > #include <asm/tlbflush.h>
> > #include <asm/thread_info.h>
> >
> > @@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
> > swiotlb_init(1);
> > #endif
> >
> > + riscv_sbi_init();
> > #ifdef CONFIG_SMP
> > setup_smp();
> > #endif
> > --
> > 2.21.0
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> --
> Sincerely yours,
> Mike.
>
On Tue, Aug 27, 2019 at 01:53:23PM +0530, Anup Patel wrote:
> On Tue, Aug 27, 2019 at 1:28 PM Mike Rapoport <[email protected]> wrote:
> >
> > On Mon, Aug 26, 2019 at 04:32:56PM -0700, Atish Patra wrote:
> > > The SBI v0.2 introduces a base extension which is backward compatible
> > > with v0.1. Implement all helper functions and minimum required SBI
> > > calls from v0.2 for now. All other base extension function will be
> > > added later as per need.
> > >
> > > Signed-off-by: Atish Patra <[email protected]>
> > > ---
> > > arch/riscv/include/asm/sbi.h | 68 +++++++++++++++++++++++++++++-------
> > > arch/riscv/kernel/Makefile | 1 +
> > > arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
> > > arch/riscv/kernel/setup.c | 2 ++
> > > 4 files changed, 108 insertions(+), 13 deletions(-)
> > > create mode 100644 arch/riscv/kernel/sbi.c
> > >
> > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > index 7f5ecaaaa0d7..4a4476956693 100644
> > > --- a/arch/riscv/include/asm/sbi.h
> > > +++ b/arch/riscv/include/asm/sbi.h
> > > @@ -8,7 +8,6 @@
> > >
> > > #include <linux/types.h>
> > >
> > > -
> > > #define SBI_EXT_LEGACY_SET_TIMER 0x0
> > > #define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > > #define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > > @@ -19,28 +18,61 @@
> > > #define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > > #define SBI_EXT_LEGACY_SHUTDOWN 0x8
> > >
> > > -#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> > > +#define SBI_EXT_BASE 0x10
> > > +
> > > +enum sbi_ext_base_fid {
> > > + SBI_EXT_BASE_GET_SPEC_VERSION = 0,
> > > + SBI_EXT_BASE_GET_IMP_ID,
> > > + SBI_EXT_BASE_GET_IMP_VERSION,
> > > + SBI_EXT_BASE_PROBE_EXT,
> > > + SBI_EXT_BASE_GET_MVENDORID,
> > > + SBI_EXT_BASE_GET_MARCHID,
> > > + SBI_EXT_BASE_GET_MIMPID,
> > > +};
> > > +
> > > +#define SBI_CALL_LEGACY(ext, fid, 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); \
> > > register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> > > - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> > > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> > > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
> > > asm volatile ("ecall" \
> > > - : "+r" (a0) \
> > > - : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
> > > + : "+r" (a0), "+r" (a1) \
> > > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
> >
> > Maybe I'm missing something, but how is this supposed to work on systems
> > with SBI v0.1? Wouldn't this cause a mismatch in the registers?
>
> The SBI v0.2 has two major changes:
> 1. New improved calling convention which is backward compatible
> with SBI v0.1 so older kernels with SBI v0.1 will continue to work as-is.
> 2. Base set of mandatory SBI v0.2 calls which can be used to detect
> SBI version, check supported SBI calls and extentions.
>
> Old calling convention in SBI v0.1 was:
> Parameters:
> a0 -> arg0
> a1 -> arg1
> a2 -> arg2
> a3 -> arg3
> a7 -> function_id
> Return:
> a0 -> return value or error code
>
> In SBI v0.2, we have extension and function. Each SBI extension
> is a set of function. The new calling convention in SBI v0.2 is:
> Parameters:
> a0 -> arg0
> a1 -> arg1
> a2 -> arg2
> a3 -> arg3
> a6 -> function_id
> a7 -> extension_id
> Return:
> a0 -> error code
> a1 -> return value (optional)
So with this patch SBI_CALL_LEGACY() uses SBI v0.2 convention, right?
Doesn't it mean that you cannot run a new kernel on a system with SBI v0.1?
> All legacy SBI v0.1 functions can be thought of as separate
> extensions. That's how SBI v0.2 will be backward compatible.
>
> Regards,
> Anup
>
> >
> > > : "memory"); \
> > > a0; \
> > > })
> > >
> > > /* Lazy implementations until SBI is finalized */
> > > -#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)
> > > +#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0, 0, 0)
> > > +#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0, arg0, 0, 0, 0)
> > > +#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
> > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
> > > +#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
> > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
> > > +#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
> > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
> > > +
> > > +extern unsigned long sbi_firmware_version;
> > > +struct sbiret {
> > > + long error;
> > > + long value;
> > > +};
> > > +
> > > +void riscv_sbi_init(void);
> > > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> > > + int arg2, int arg3);
> > > +
> > > +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
> > > +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0)
> > > +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> > > + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> > > +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> > > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> > > +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> > > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
> > > +
> > >
> > > static inline void sbi_console_putchar(int ch)
> > > {
> > > @@ -99,4 +131,14 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > > start, size, asid);
> > > }
> > >
> > > +static inline unsigned long riscv_sbi_major_version(void)
> > > +{
> > > + return (sbi_firmware_version >> 24) & 0x7f;
> > > +}
> > > +
> > > +static inline unsigned long riscv_sbi_minor_version(void)
> > > +{
> > > + return sbi_firmware_version & 0xffffff;
> > > +}
> > > +
> > > #endif
> > > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > > index 2420d37d96de..faf862d26924 100644
> > > --- a/arch/riscv/kernel/Makefile
> > > +++ b/arch/riscv/kernel/Makefile
> > > @@ -17,6 +17,7 @@ obj-y += irq.o
> > > obj-y += process.o
> > > obj-y += ptrace.o
> > > obj-y += reset.o
> > > +obj-y += sbi.o
> > > obj-y += setup.o
> > > obj-y += signal.o
> > > obj-y += syscall_table.o
> > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > new file mode 100644
> > > index 000000000000..457b8cc0e9d9
> > > --- /dev/null
> > > +++ b/arch/riscv/kernel/sbi.c
> > > @@ -0,0 +1,50 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * SBI initialilization and base extension implementation.
> > > + *
> > > + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > > + */
> > > +
> > > +#include <asm/sbi.h>
> > > +#include <linux/sched.h>
> > > +
> > > +unsigned long sbi_firmware_version;
> > > +
> > > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> > > + int arg2, int arg3)
> > > +{
> > > + struct sbiret ret;
> > > +
> > > + 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);
> > > + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> > > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> > > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> > > + asm volatile ("ecall"
> > > + : "+r" (a0), "+r" (a1)
> > > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> > > + : "memory");
> > > + ret.error = a0;
> > > + ret.value = a1;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static struct sbiret sbi_get_spec_version(void)
> > > +{
> > > + return SBI_CALL_0(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION);
> > > +}
> > > +
> > > +void riscv_sbi_init(void)
> > > +{
> > > + struct sbiret ret;
> > > +
> > > + /* legacy SBI version*/
> > > + sbi_firmware_version = 0x1;
> > > + ret = sbi_get_spec_version();
> > > + if (!ret.error)
> > > + sbi_firmware_version = ret.value;
> > > + pr_info("SBI version implemented in firmware [%lu:%lu]\n",
> > > + riscv_sbi_major_version(), riscv_sbi_minor_version());
> > > +}
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index a990a6cb184f..4c324fd398c8 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -21,6 +21,7 @@
> > > #include <asm/sections.h>
> > > #include <asm/pgtable.h>
> > > #include <asm/smp.h>
> > > +#include <asm/sbi.h>
> > > #include <asm/tlbflush.h>
> > > #include <asm/thread_info.h>
> > >
> > > @@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
> > > swiotlb_init(1);
> > > #endif
> > >
> > > + riscv_sbi_init();
> > > #ifdef CONFIG_SMP
> > > setup_smp();
> > > #endif
> > > --
> > > 2.21.0
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> > --
> > Sincerely yours,
> > Mike.
> >
--
Sincerely yours,
Mike.
On Tue, Aug 27, 2019 at 2:09 PM Mike Rapoport <[email protected]> wrote:
>
> On Tue, Aug 27, 2019 at 01:53:23PM +0530, Anup Patel wrote:
> > On Tue, Aug 27, 2019 at 1:28 PM Mike Rapoport <[email protected]> wrote:
> > >
> > > On Mon, Aug 26, 2019 at 04:32:56PM -0700, Atish Patra wrote:
> > > > The SBI v0.2 introduces a base extension which is backward compatible
> > > > with v0.1. Implement all helper functions and minimum required SBI
> > > > calls from v0.2 for now. All other base extension function will be
> > > > added later as per need.
> > > >
> > > > Signed-off-by: Atish Patra <[email protected]>
> > > > ---
> > > > arch/riscv/include/asm/sbi.h | 68 +++++++++++++++++++++++++++++-------
> > > > arch/riscv/kernel/Makefile | 1 +
> > > > arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
> > > > arch/riscv/kernel/setup.c | 2 ++
> > > > 4 files changed, 108 insertions(+), 13 deletions(-)
> > > > create mode 100644 arch/riscv/kernel/sbi.c
> > > >
> > > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > > index 7f5ecaaaa0d7..4a4476956693 100644
> > > > --- a/arch/riscv/include/asm/sbi.h
> > > > +++ b/arch/riscv/include/asm/sbi.h
> > > > @@ -8,7 +8,6 @@
> > > >
> > > > #include <linux/types.h>
> > > >
> > > > -
> > > > #define SBI_EXT_LEGACY_SET_TIMER 0x0
> > > > #define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > > > #define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > > > @@ -19,28 +18,61 @@
> > > > #define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > > > #define SBI_EXT_LEGACY_SHUTDOWN 0x8
> > > >
> > > > -#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> > > > +#define SBI_EXT_BASE 0x10
> > > > +
> > > > +enum sbi_ext_base_fid {
> > > > + SBI_EXT_BASE_GET_SPEC_VERSION = 0,
> > > > + SBI_EXT_BASE_GET_IMP_ID,
> > > > + SBI_EXT_BASE_GET_IMP_VERSION,
> > > > + SBI_EXT_BASE_PROBE_EXT,
> > > > + SBI_EXT_BASE_GET_MVENDORID,
> > > > + SBI_EXT_BASE_GET_MARCHID,
> > > > + SBI_EXT_BASE_GET_MIMPID,
> > > > +};
> > > > +
> > > > +#define SBI_CALL_LEGACY(ext, fid, 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); \
> > > > register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> > > > - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> > > > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> > > > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
> > > > asm volatile ("ecall" \
> > > > - : "+r" (a0) \
> > > > - : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
> > > > + : "+r" (a0), "+r" (a1) \
> > > > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
> > >
> > > Maybe I'm missing something, but how is this supposed to work on systems
> > > with SBI v0.1? Wouldn't this cause a mismatch in the registers?
> >
> > The SBI v0.2 has two major changes:
> > 1. New improved calling convention which is backward compatible
> > with SBI v0.1 so older kernels with SBI v0.1 will continue to work as-is.
> > 2. Base set of mandatory SBI v0.2 calls which can be used to detect
> > SBI version, check supported SBI calls and extentions.
> >
> > Old calling convention in SBI v0.1 was:
> > Parameters:
> > a0 -> arg0
> > a1 -> arg1
> > a2 -> arg2
> > a3 -> arg3
> > a7 -> function_id
> > Return:
> > a0 -> return value or error code
> >
> > In SBI v0.2, we have extension and function. Each SBI extension
> > is a set of function. The new calling convention in SBI v0.2 is:
> > Parameters:
> > a0 -> arg0
> > a1 -> arg1
> > a2 -> arg2
> > a3 -> arg3
> > a6 -> function_id
> > a7 -> extension_id
> > Return:
> > a0 -> error code
> > a1 -> return value (optional)
>
> So with this patch SBI_CALL_LEGACY() uses SBI v0.2 convention, right?
> Doesn't it mean that you cannot run a new kernel on a system with SBI v0.1?
This is certainly possible. It's just that this patch is using SBI v0.2
convention for older firmware as well.
Ideally, we should check sbi_version for each legacy SBI calls and
use different calling convention based sbi_version. The sbi_version
detection will work fine on both old and new firmwares because
on old firmware SBI version call will fail which means it is SBI v0.1
interface.
I think all legacy calls should be moved to kernel/sbi.c.
I have other comments too.
Let me post detailed review comments.
Regards,
Anup
>
> > All legacy SBI v0.1 functions can be thought of as separate
> > extensions. That's how SBI v0.2 will be backward compatible.
> >
> > Regards,
> > Anup
> >
> > >
> > > > : "memory"); \
> > > > a0; \
> > > > })
> > > >
> > > > /* Lazy implementations until SBI is finalized */
> > > > -#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)
> > > > +#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0, 0, 0)
> > > > +#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0, arg0, 0, 0, 0)
> > > > +#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
> > > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
> > > > +#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
> > > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
> > > > +#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
> > > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
> > > > +
> > > > +extern unsigned long sbi_firmware_version;
> > > > +struct sbiret {
> > > > + long error;
> > > > + long value;
> > > > +};
> > > > +
> > > > +void riscv_sbi_init(void);
> > > > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> > > > + int arg2, int arg3);
> > > > +
> > > > +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
> > > > +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0)
> > > > +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> > > > + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> > > > +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> > > > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> > > > +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> > > > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
> > > > +
> > > >
> > > > static inline void sbi_console_putchar(int ch)
> > > > {
> > > > @@ -99,4 +131,14 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > > > start, size, asid);
> > > > }
> > > >
> > > > +static inline unsigned long riscv_sbi_major_version(void)
> > > > +{
> > > > + return (sbi_firmware_version >> 24) & 0x7f;
> > > > +}
> > > > +
> > > > +static inline unsigned long riscv_sbi_minor_version(void)
> > > > +{
> > > > + return sbi_firmware_version & 0xffffff;
> > > > +}
> > > > +
> > > > #endif
> > > > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > > > index 2420d37d96de..faf862d26924 100644
> > > > --- a/arch/riscv/kernel/Makefile
> > > > +++ b/arch/riscv/kernel/Makefile
> > > > @@ -17,6 +17,7 @@ obj-y += irq.o
> > > > obj-y += process.o
> > > > obj-y += ptrace.o
> > > > obj-y += reset.o
> > > > +obj-y += sbi.o
> > > > obj-y += setup.o
> > > > obj-y += signal.o
> > > > obj-y += syscall_table.o
> > > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > > new file mode 100644
> > > > index 000000000000..457b8cc0e9d9
> > > > --- /dev/null
> > > > +++ b/arch/riscv/kernel/sbi.c
> > > > @@ -0,0 +1,50 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * SBI initialilization and base extension implementation.
> > > > + *
> > > > + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > > > + */
> > > > +
> > > > +#include <asm/sbi.h>
> > > > +#include <linux/sched.h>
> > > > +
> > > > +unsigned long sbi_firmware_version;
> > > > +
> > > > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> > > > + int arg2, int arg3)
> > > > +{
> > > > + struct sbiret ret;
> > > > +
> > > > + 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);
> > > > + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> > > > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> > > > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> > > > + asm volatile ("ecall"
> > > > + : "+r" (a0), "+r" (a1)
> > > > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> > > > + : "memory");
> > > > + ret.error = a0;
> > > > + ret.value = a1;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static struct sbiret sbi_get_spec_version(void)
> > > > +{
> > > > + return SBI_CALL_0(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION);
> > > > +}
> > > > +
> > > > +void riscv_sbi_init(void)
> > > > +{
> > > > + struct sbiret ret;
> > > > +
> > > > + /* legacy SBI version*/
> > > > + sbi_firmware_version = 0x1;
> > > > + ret = sbi_get_spec_version();
> > > > + if (!ret.error)
> > > > + sbi_firmware_version = ret.value;
> > > > + pr_info("SBI version implemented in firmware [%lu:%lu]\n",
> > > > + riscv_sbi_major_version(), riscv_sbi_minor_version());
> > > > +}
> > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > > index a990a6cb184f..4c324fd398c8 100644
> > > > --- a/arch/riscv/kernel/setup.c
> > > > +++ b/arch/riscv/kernel/setup.c
> > > > @@ -21,6 +21,7 @@
> > > > #include <asm/sections.h>
> > > > #include <asm/pgtable.h>
> > > > #include <asm/smp.h>
> > > > +#include <asm/sbi.h>
> > > > #include <asm/tlbflush.h>
> > > > #include <asm/thread_info.h>
> > > >
> > > > @@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
> > > > swiotlb_init(1);
> > > > #endif
> > > >
> > > > + riscv_sbi_init();
> > > > #ifdef CONFIG_SMP
> > > > setup_smp();
> > > > #endif
> > > > --
> > > > 2.21.0
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > [email protected]
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
> > > --
> > > Sincerely yours,
> > > Mike.
> > >
>
> --
> Sincerely yours,
> Mike.
>
On Tue, Aug 27, 2019 at 5:03 AM Atish Patra <[email protected]> wrote:
>
> The SBI v0.2 introduces a base extension which is backward compatible
> with v0.1. Implement all helper functions and minimum required SBI
> calls from v0.2 for now. All other base extension function will be
> added later as per need.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/sbi.h | 68 +++++++++++++++++++++++++++++-------
> arch/riscv/kernel/Makefile | 1 +
> arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
> arch/riscv/kernel/setup.c | 2 ++
> 4 files changed, 108 insertions(+), 13 deletions(-)
> create mode 100644 arch/riscv/kernel/sbi.c
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 7f5ecaaaa0d7..4a4476956693 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -8,7 +8,6 @@
>
> #include <linux/types.h>
>
> -
> #define SBI_EXT_LEGACY_SET_TIMER 0x0
> #define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> #define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> @@ -19,28 +18,61 @@
> #define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> #define SBI_EXT_LEGACY_SHUTDOWN 0x8
>
> -#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3) ({ \
> +#define SBI_EXT_BASE 0x10
> +
> +enum sbi_ext_base_fid {
> + SBI_EXT_BASE_GET_SPEC_VERSION = 0,
> + SBI_EXT_BASE_GET_IMP_ID,
> + SBI_EXT_BASE_GET_IMP_VERSION,
> + SBI_EXT_BASE_PROBE_EXT,
> + SBI_EXT_BASE_GET_MVENDORID,
> + SBI_EXT_BASE_GET_MARCHID,
> + SBI_EXT_BASE_GET_MIMPID,
> +};
> +
> +#define SBI_CALL_LEGACY(ext, fid, 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); \
> register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
> asm volatile ("ecall" \
> - : "+r" (a0) \
> - : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
> + : "+r" (a0), "+r" (a1) \
> + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
> : "memory"); \
> a0; \
> })
I think instead of removing old convention we should use
calling convention based on sbi_version detected at boot-time.
>
> /* Lazy implementations until SBI is finalized */
> -#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)
> +#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0, arg0, 0, 0, 0)
> +#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
> + SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
> +#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
> + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
> +#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
> + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
> +
> +extern unsigned long sbi_firmware_version;
> +struct sbiret {
> + long error;
> + long value;
> +};
> +
> +void riscv_sbi_init(void);
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> + int arg2, int arg3);
> +
> +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
> +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0)
> +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
> +
>
> static inline void sbi_console_putchar(int ch)
> {
> @@ -99,4 +131,14 @@ static inline void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> start, size, asid);
> }
To be sure that new kernels work fine on older kernel, we
can be conservative and move all legacy SBI calls to kernel/sbi.c.
All legacy SBI calls can check sbi_version and use appropriate
SBI calling convention.
This might be redundant if we can ensure that newer kernels
work fine with older SBI v0.1 firmwares.
>
> +static inline unsigned long riscv_sbi_major_version(void)
> +{
> + return (sbi_firmware_version >> 24) & 0x7f;
> +}
> +
> +static inline unsigned long riscv_sbi_minor_version(void)
> +{
> + return sbi_firmware_version & 0xffffff;
> +}
> +
> #endif
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index 2420d37d96de..faf862d26924 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -17,6 +17,7 @@ obj-y += irq.o
> obj-y += process.o
> obj-y += ptrace.o
> obj-y += reset.o
> +obj-y += sbi.o
> obj-y += setup.o
> obj-y += signal.o
> obj-y += syscall_table.o
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> new file mode 100644
> index 000000000000..457b8cc0e9d9
> --- /dev/null
> +++ b/arch/riscv/kernel/sbi.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SBI initialilization and base extension implementation.
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + */
> +
> +#include <asm/sbi.h>
> +#include <linux/sched.h>
> +
> +unsigned long sbi_firmware_version;
Rename this too sbi_version or sbi_spec_version.
The firmware version is different thing.
> +
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> + int arg2, int arg3)
> +{
> + struct sbiret ret;
> +
> + 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);
> + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> + asm volatile ("ecall"
> + : "+r" (a0), "+r" (a1)
> + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> + : "memory");
> + ret.error = a0;
> + ret.value = a1;
> +
> + return ret;
> +}
> +
> +static struct sbiret sbi_get_spec_version(void)
> +{
> + return SBI_CALL_0(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION);
> +}
> +
> +void riscv_sbi_init(void)
> +{
> + struct sbiret ret;
> +
> + /* legacy SBI version*/
> + sbi_firmware_version = 0x1;
> + ret = sbi_get_spec_version();
> + if (!ret.error)
> + sbi_firmware_version = ret.value;
> + pr_info("SBI version implemented in firmware [%lu:%lu]\n",
> + riscv_sbi_major_version(), riscv_sbi_minor_version());
Should we not print SBI implementation ID and SBI firmware version.
Regards,
Anup
> +}
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index a990a6cb184f..4c324fd398c8 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -21,6 +21,7 @@
> #include <asm/sections.h>
> #include <asm/pgtable.h>
> #include <asm/smp.h>
> +#include <asm/sbi.h>
> #include <asm/tlbflush.h>
> #include <asm/thread_info.h>
>
> @@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
> swiotlb_init(1);
> #endif
>
> + riscv_sbi_init();
> #ifdef CONFIG_SMP
> setup_smp();
> #endif
> --
> 2.21.0
>
> +#define SBI_EXT_BASE 0x10
I think you want an enum enumerating the extensions.
> +#define SBI_CALL_LEGACY(ext, fid, 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); \
> register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
This seems to break the calling convention. I also think we should go
back to the Unix platform working group and make the calling convention
backwards compatible. There is really no advantage or disadvantag
in swapping a6 and a7 in the calling convention itself, but doing so
means you can just push the ext field in always and it will be
ignored by the old sbi.
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> + int arg2, int arg3);
> +
> +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
> +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0)
> +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
Again, no point in having these wrappers.
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1,
> + int arg2, int arg3)
> +{
> + struct sbiret ret;
> +
> + 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);
> + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> + asm volatile ("ecall"
> + : "+r" (a0), "+r" (a1)
> + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> + : "memory");
> + ret.error = a0;
> + ret.value = a1;
> +
> + return ret;
Again much simpler done in pure asm..
> + /* legacy SBI version*/
> + sbi_firmware_version = 0x1;
> + ret = sbi_get_spec_version();
> + if (!ret.error)
> + sbi_firmware_version = ret.value;
Why not:
ret = sbi_get_spec_version();
if (ret.error)
sbi_firmware_version = 0x1; /* legacy SBI */
else
sbi_firmware_version = ret.value;
btw, I'd find a calling convention that returns the value as a pointer
much nicer than the return by a struct. Yes, the RISC-V ABI still
returns that in registers, but it is a pain in the b**t to use. Without
that we could simply pass the variable to fill by reference.
On Mon, Aug 26, 2019 at 04:32:54PM -0700, Atish Patra wrote:
> This patch series aims to add support for SBI specification version
> v0.2. It doesn't break compatibility with any v0.1 implementation.
> Internally, all the v0.1 calls are just renamed to legacy to be in
> sync with specification [1].
>
> The patches for v0.2 support in OpenSBI are available at
> http://lists.infradead.org/pipermail/opensbi/2019-August/000422.html
>
> [1] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
I really don't like the current design of that SBI 0.2 spec,
and don't think implementing it as-is is helpful.
For one the way how the extension id is placed creates a compatibilty
problem, not allowing your to implement a backwards compatible sbi,
which seems bad.
Second just blindly moving all the existing calls to a single legacy
extension doesn't seem useful. We need to differenciate the existing
calls:
(1) actually board specific and have not place in a cpu abstraction
layer: getchar/putchar, these should just never be advertised in a
non-legacy setup, and the drivers using them should not probe
on a sbi 0.2+ system
(2) useful for currently taped out cpus and in the long run for
virtualization to avoid mmio traps: ipis, timers, tlb shootdown.
These should stay backwards compatible, but for sbi 0.2 be
negotiated individually
(3) in theory useful, but given how much of a big hammer sfence.i
not useful in theory: SBI_REMOTE_FENCE_I we can decide if we want
to either not allow it for sbi 0.2+ or also negotiate it. I'd
personally favor not advertising it and just use ipis to implement
it. If we want useful acceleration of i-cache synchronization
we'll need actual instructions that are much more fine grained
in the future.
On Tue, 2019-08-27 at 11:39 +0300, Mike Rapoport wrote:
> On Tue, Aug 27, 2019 at 01:53:23PM +0530, Anup Patel wrote:
> > On Tue, Aug 27, 2019 at 1:28 PM Mike Rapoport <[email protected]>
> > wrote:
> > > On Mon, Aug 26, 2019 at 04:32:56PM -0700, Atish Patra wrote:
> > > > The SBI v0.2 introduces a base extension which is backward
> > > > compatible
> > > > with v0.1. Implement all helper functions and minimum required
> > > > SBI
> > > > calls from v0.2 for now. All other base extension function will
> > > > be
> > > > added later as per need.
> > > >
> > > > Signed-off-by: Atish Patra <[email protected]>
> > > > ---
> > > > arch/riscv/include/asm/sbi.h | 68
> > > > +++++++++++++++++++++++++++++-------
> > > > arch/riscv/kernel/Makefile | 1 +
> > > > arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
> > > > arch/riscv/kernel/setup.c | 2 ++
> > > > 4 files changed, 108 insertions(+), 13 deletions(-)
> > > > create mode 100644 arch/riscv/kernel/sbi.c
> > > >
> > > > diff --git a/arch/riscv/include/asm/sbi.h
> > > > b/arch/riscv/include/asm/sbi.h
> > > > index 7f5ecaaaa0d7..4a4476956693 100644
> > > > --- a/arch/riscv/include/asm/sbi.h
> > > > +++ b/arch/riscv/include/asm/sbi.h
> > > > @@ -8,7 +8,6 @@
> > > >
> > > > #include <linux/types.h>
> > > >
> > > > -
> > > > #define SBI_EXT_LEGACY_SET_TIMER 0x0
> > > > #define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > > > #define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > > > @@ -19,28 +18,61 @@
> > > > #define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > > > #define SBI_EXT_LEGACY_SHUTDOWN 0x8
> > > >
> > > > -#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
> > > > ({ \
> > > > +#define SBI_EXT_BASE 0x10
> > > > +
> > > > +enum sbi_ext_base_fid {
> > > > + SBI_EXT_BASE_GET_SPEC_VERSION = 0,
> > > > + SBI_EXT_BASE_GET_IMP_ID,
> > > > + SBI_EXT_BASE_GET_IMP_VERSION,
> > > > + SBI_EXT_BASE_PROBE_EXT,
> > > > + SBI_EXT_BASE_GET_MVENDORID,
> > > > + SBI_EXT_BASE_GET_MARCHID,
> > > > + SBI_EXT_BASE_GET_MIMPID,
> > > > +};
> > > > +
> > > > +#define SBI_CALL_LEGACY(ext, fid, 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); \
> > > > register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> > > > - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> > > > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> > > > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
> > > > asm volatile ("ecall" \
> > > > - : "+r" (a0) \
> > > > - : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
> > > > + : "+r" (a0), "+r" (a1) \
> > > > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
> > >
> > > Maybe I'm missing something, but how is this supposed to work on
> > > systems
> > > with SBI v0.1? Wouldn't this cause a mismatch in the registers?
> >
> > The SBI v0.2 has two major changes:
> > 1. New improved calling convention which is backward compatible
> > with SBI v0.1 so older kernels with SBI v0.1 will continue to work
> > as-is.
> > 2. Base set of mandatory SBI v0.2 calls which can be used to detect
> > SBI version, check supported SBI calls and extentions.
> >
> > Old calling convention in SBI v0.1 was:
> > Parameters:
> > a0 -> arg0
> > a1 -> arg1
> > a2 -> arg2
> > a3 -> arg3
> > a7 -> function_id
> > Return:
> > a0 -> return value or error code
> >
> > In SBI v0.2, we have extension and function. Each SBI extension
> > is a set of function. The new calling convention in SBI v0.2 is:
> > Parameters:
> > a0 -> arg0
> > a1 -> arg1
> > a2 -> arg2
> > a3 -> arg3
> > a6 -> function_id
> > a7 -> extension_id
> > Return:
> > a0 -> error code
> > a1 -> return value (optional)
>
> So with this patch SBI_CALL_LEGACY() uses SBI v0.2 convention, right?
> Doesn't it mean that you cannot run a new kernel on a system with SBI
> v0.1?
>
Thanks Anup for the earlier explaination.
I have used SBI v0.2 convention for legacy calls as well to keep both
in sync and more redable. I just set a6 to zero and do not use a1 as a
return value.
With this, both SBI implementation & kernel are backward compatible in
both ways. Any newer kernels using SBI calls defined in 0.2 or later
will use new convention(SBI_CALL_*) and older SBI calls will continue
to will continue to use SBI_CALL_LEGACY*.
I thought this is more readable but probably it is not. I will preserve
the legacy convention and use different calling conventions based on
the firmware version.
> > All legacy SBI v0.1 functions can be thought of as separate
> > extensions. That's how SBI v0.2 will be backward compatible.
> >
> > Regards,
> > Anup
> >
> > > > : "memory"); \
> > > > a0; \
> > > > })
> > > >
> > > > /* Lazy implementations until SBI is finalized */
> > > > -#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)
> > > > +#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0,
> > > > 0, 0)
> > > > +#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0,
> > > > arg0, 0, 0, 0)
> > > > +#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
> > > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
> > > > +#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
> > > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
> > > > +#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
> > > > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
> > > > +
> > > > +extern unsigned long sbi_firmware_version;
> > > > +struct sbiret {
> > > > + long error;
> > > > + long value;
> > > > +};
> > > > +
> > > > +void riscv_sbi_init(void);
> > > > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int
> > > > arg1,
> > > > + int arg2, int arg3);
> > > > +
> > > > +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0,
> > > > 0, 0)
> > > > +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid,
> > > > arg0, 0, 0, 0)
> > > > +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> > > > + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> > > > +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> > > > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> > > > +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> > > > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
> > > > +
> > > >
> > > > static inline void sbi_console_putchar(int ch)
> > > > {
> > > > @@ -99,4 +131,14 @@ static inline void
> > > > sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > > > start, size, asid);
> > > > }
> > > >
> > > > +static inline unsigned long riscv_sbi_major_version(void)
> > > > +{
> > > > + return (sbi_firmware_version >> 24) & 0x7f;
> > > > +}
> > > > +
> > > > +static inline unsigned long riscv_sbi_minor_version(void)
> > > > +{
> > > > + return sbi_firmware_version & 0xffffff;
> > > > +}
> > > > +
> > > > #endif
> > > > diff --git a/arch/riscv/kernel/Makefile
> > > > b/arch/riscv/kernel/Makefile
> > > > index 2420d37d96de..faf862d26924 100644
> > > > --- a/arch/riscv/kernel/Makefile
> > > > +++ b/arch/riscv/kernel/Makefile
> > > > @@ -17,6 +17,7 @@ obj-y += irq.o
> > > > obj-y += process.o
> > > > obj-y += ptrace.o
> > > > obj-y += reset.o
> > > > +obj-y += sbi.o
> > > > obj-y += setup.o
> > > > obj-y += signal.o
> > > > obj-y += syscall_table.o
> > > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > > new file mode 100644
> > > > index 000000000000..457b8cc0e9d9
> > > > --- /dev/null
> > > > +++ b/arch/riscv/kernel/sbi.c
> > > > @@ -0,0 +1,50 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * SBI initialilization and base extension implementation.
> > > > + *
> > > > + * Copyright (c) 2019 Western Digital Corporation or its
> > > > affiliates.
> > > > + */
> > > > +
> > > > +#include <asm/sbi.h>
> > > > +#include <linux/sched.h>
> > > > +
> > > > +unsigned long sbi_firmware_version;
> > > > +
> > > > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int
> > > > arg1,
> > > > + int arg2, int arg3)
> > > > +{
> > > > + struct sbiret ret;
> > > > +
> > > > + 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);
> > > > + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> > > > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> > > > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> > > > + asm volatile ("ecall"
> > > > + : "+r" (a0), "+r" (a1)
> > > > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> > > > + : "memory");
> > > > + ret.error = a0;
> > > > + ret.value = a1;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static struct sbiret sbi_get_spec_version(void)
> > > > +{
> > > > + return SBI_CALL_0(SBI_EXT_BASE,
> > > > SBI_EXT_BASE_GET_SPEC_VERSION);
> > > > +}
> > > > +
> > > > +void riscv_sbi_init(void)
> > > > +{
> > > > + struct sbiret ret;
> > > > +
> > > > + /* legacy SBI version*/
> > > > + sbi_firmware_version = 0x1;
> > > > + ret = sbi_get_spec_version();
> > > > + if (!ret.error)
> > > > + sbi_firmware_version = ret.value;
> > > > + pr_info("SBI version implemented in firmware
> > > > [%lu:%lu]\n",
> > > > + riscv_sbi_major_version(),
> > > > riscv_sbi_minor_version());
> > > > +}
> > > > diff --git a/arch/riscv/kernel/setup.c
> > > > b/arch/riscv/kernel/setup.c
> > > > index a990a6cb184f..4c324fd398c8 100644
> > > > --- a/arch/riscv/kernel/setup.c
> > > > +++ b/arch/riscv/kernel/setup.c
> > > > @@ -21,6 +21,7 @@
> > > > #include <asm/sections.h>
> > > > #include <asm/pgtable.h>
> > > > #include <asm/smp.h>
> > > > +#include <asm/sbi.h>
> > > > #include <asm/tlbflush.h>
> > > > #include <asm/thread_info.h>
> > > >
> > > > @@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
> > > > swiotlb_init(1);
> > > > #endif
> > > >
> > > > + riscv_sbi_init();
> > > > #ifdef CONFIG_SMP
> > > > setup_smp();
> > > > #endif
> > > > --
> > > > 2.21.0
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > [email protected]
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
> > > --
> > > Sincerely yours,
> > > Mike.
> > >
--
Regards,
Atish
On Tue, 2019-08-27 at 15:06 +0530, Anup Patel wrote:
> On Tue, Aug 27, 2019 at 5:03 AM Atish Patra <[email protected]>
> wrote:
> > The SBI v0.2 introduces a base extension which is backward
> > compatible
> > with v0.1. Implement all helper functions and minimum required SBI
> > calls from v0.2 for now. All other base extension function will be
> > added later as per need.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/sbi.h | 68 +++++++++++++++++++++++++++++---
> > ----
> > arch/riscv/kernel/Makefile | 1 +
> > arch/riscv/kernel/sbi.c | 50 ++++++++++++++++++++++++++
> > arch/riscv/kernel/setup.c | 2 ++
> > 4 files changed, 108 insertions(+), 13 deletions(-)
> > create mode 100644 arch/riscv/kernel/sbi.c
> >
> > diff --git a/arch/riscv/include/asm/sbi.h
> > b/arch/riscv/include/asm/sbi.h
> > index 7f5ecaaaa0d7..4a4476956693 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -8,7 +8,6 @@
> >
> > #include <linux/types.h>
> >
> > -
> > #define SBI_EXT_LEGACY_SET_TIMER 0x0
> > #define SBI_EXT_LEGACY_CONSOLE_PUTCHAR 0x1
> > #define SBI_EXT_LEGACY_CONSOLE_GETCHAR 0x2
> > @@ -19,28 +18,61 @@
> > #define SBI_EXT_LEGACY_REMOTE_SFENCE_VMA_ASID 0x7
> > #define SBI_EXT_LEGACY_SHUTDOWN 0x8
> >
> > -#define SBI_CALL_LEGACY(which, arg0, arg1, arg2, arg3)
> > ({ \
> > +#define SBI_EXT_BASE 0x10
> > +
> > +enum sbi_ext_base_fid {
> > + SBI_EXT_BASE_GET_SPEC_VERSION = 0,
> > + SBI_EXT_BASE_GET_IMP_ID,
> > + SBI_EXT_BASE_GET_IMP_VERSION,
> > + SBI_EXT_BASE_PROBE_EXT,
> > + SBI_EXT_BASE_GET_MVENDORID,
> > + SBI_EXT_BASE_GET_MARCHID,
> > + SBI_EXT_BASE_GET_MIMPID,
> > +};
> > +
> > +#define SBI_CALL_LEGACY(ext, fid, 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); \
> > register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \
> > - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \
> > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \
> > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
> > asm volatile ("ecall" \
> > - : "+r" (a0) \
> > - : "r" (a1), "r" (a2), "r" (a3), "r" (a7) \
> > + : "+r" (a0), "+r" (a1) \
> > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) \
> > : "memory"); \
> > a0; \
> > })
>
> I think instead of removing old convention we should use
> calling convention based on sbi_version detected at boot-time.
>
> > /* Lazy implementations until SBI is finalized */
> > -#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)
> > +#define SBI_CALL_LEGACY_0(ext) SBI_CALL_LEGACY(ext, 0, 0, 0, 0, 0)
> > +#define SBI_CALL_LEGACY_1(ext, arg0) SBI_CALL_LEGACY(ext, 0, arg0,
> > 0, 0, 0)
> > +#define SBI_CALL_LEGACY_2(ext, arg0, arg1) \
> > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, 0, 0)
> > +#define SBI_CALL_LEGACY_3(ext, arg0, arg1, arg2) \
> > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, 0)
> > +#define SBI_CALL_LEGACY_4(ext, arg0, arg1, arg2, arg3) \
> > + SBI_CALL_LEGACY(ext, 0, arg0, arg1, arg2, arg3)
> > +
> > +extern unsigned long sbi_firmware_version;
> > +struct sbiret {
> > + long error;
> > + long value;
> > +};
> > +
> > +void riscv_sbi_init(void);
> > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int
> > arg1,
> > + int arg2, int arg3);
> > +
> > +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0)
> > +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0,
> > 0, 0, 0)
> > +#define SBI_CALL_2(ext, fid, arg0, arg1) \
> > + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0)
> > +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \
> > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0)
> > +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \
> > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
> > +
> >
> > static inline void sbi_console_putchar(int ch)
> > {
> > @@ -99,4 +131,14 @@ static inline void
> > sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
> > start, size, asid);
> > }
>
> To be sure that new kernels work fine on older kernel, we
> can be conservative and move all legacy SBI calls to kernel/sbi.c.
> All legacy SBI calls can check sbi_version and use appropriate
> SBI calling convention.
>
> This might be redundant if we can ensure that newer kernels
> work fine with older SBI v0.1 firmwares.
Yes that's why I didnot want to do it first time. How about this ?
Use only 0.2 convention only and get rid of the 0.1 convention. As it
is anyways backward compatible with 0.1, we don't need a if else
clause.
The legacy calls will not use any value set in a1 and always set 0 in
function id (a6).
>
> > +static inline unsigned long riscv_sbi_major_version(void)
> > +{
> > + return (sbi_firmware_version >> 24) & 0x7f;
> > +}
> > +
> > +static inline unsigned long riscv_sbi_minor_version(void)
> > +{
> > + return sbi_firmware_version & 0xffffff;
> > +}
> > +
> > #endif
> > diff --git a/arch/riscv/kernel/Makefile
> > b/arch/riscv/kernel/Makefile
> > index 2420d37d96de..faf862d26924 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -17,6 +17,7 @@ obj-y += irq.o
> > obj-y += process.o
> > obj-y += ptrace.o
> > obj-y += reset.o
> > +obj-y += sbi.o
> > obj-y += setup.o
> > obj-y += signal.o
> > obj-y += syscall_table.o
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > new file mode 100644
> > index 000000000000..457b8cc0e9d9
> > --- /dev/null
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -0,0 +1,50 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * SBI initialilization and base extension implementation.
> > + *
> > + * Copyright (c) 2019 Western Digital Corporation or its
> > affiliates.
> > + */
> > +
> > +#include <asm/sbi.h>
> > +#include <linux/sched.h>
> > +
> > +unsigned long sbi_firmware_version;
>
> Rename this too sbi_version or sbi_spec_version.
> The firmware version is different thing.
>
ok.
> > +
> > +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int
> > arg1,
> > + int arg2, int arg3)
> > +{
> > + struct sbiret ret;
> > +
> > + 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);
> > + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> > + asm volatile ("ecall"
> > + : "+r" (a0), "+r" (a1)
> > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7)
> > + : "memory");
> > + ret.error = a0;
> > + ret.value = a1;
> > +
> > + return ret;
> > +}
> > +
> > +static struct sbiret sbi_get_spec_version(void)
> > +{
> > + return SBI_CALL_0(SBI_EXT_BASE,
> > SBI_EXT_BASE_GET_SPEC_VERSION);
> > +}
> > +
> > +void riscv_sbi_init(void)
> > +{
> > + struct sbiret ret;
> > +
> > + /* legacy SBI version*/
> > + sbi_firmware_version = 0x1;
> > + ret = sbi_get_spec_version();
> > + if (!ret.error)
> > + sbi_firmware_version = ret.value;
> > + pr_info("SBI version implemented in firmware [%lu:%lu]\n",
> > + riscv_sbi_major_version(),
> > riscv_sbi_minor_version());
>
> Should we not print SBI implementation ID and SBI firmware version.
>
Eventually yes. I wanted to have this series minimal implementation and
build upon it.
I will go ahead and add those.
> Regards,
> Anup
>
> > +}
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index a990a6cb184f..4c324fd398c8 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -21,6 +21,7 @@
> > #include <asm/sections.h>
> > #include <asm/pgtable.h>
> > #include <asm/smp.h>
> > +#include <asm/sbi.h>
> > #include <asm/tlbflush.h>
> > #include <asm/thread_info.h>
> >
> > @@ -70,6 +71,7 @@ void __init setup_arch(char **cmdline_p)
> > swiotlb_init(1);
> > #endif
> >
> > + riscv_sbi_init();
> > #ifdef CONFIG_SMP
> > setup_smp();
> > #endif
> > --
> > 2.21.0
> >
--
Regards,
Atish
On Tue, 2019-08-27 at 07:46 -0700, Christoph Hellwig wrote:
> On Mon, Aug 26, 2019 at 04:32:54PM -0700, Atish Patra wrote:
> > This patch series aims to add support for SBI specification version
> > v0.2. It doesn't break compatibility with any v0.1 implementation.
> > Internally, all the v0.1 calls are just renamed to legacy to be in
> > sync with specification [1].
> >
> > The patches for v0.2 support in OpenSBI are available at
> > http://lists.infradead.org/pipermail/opensbi/2019-August/000422.html
> >
> > [1]
> > https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
>
> I really don't like the current design of that SBI 0.2 spec,
> and don't think implementing it as-is is helpful.
>
> For one the way how the extension id is placed creates a compatibilty
> problem, not allowing your to implement a backwards compatible sbi,
> which seems bad.
>
I did not understand this part. All the legacy SBI calls are defined as
a separate extension ID not single extension. How did it break the
backward compatibility ?
Here are the few possible usecases
1. New kernel can use SBI v0.2 calling convention for older legacy
calls. It will just set a6 to zero (function id) & not use the return
value in a1. a0 is still report error value.
2. New kernel with older SBI implementation (i.e. BBL) will also work
as sbi_get_spec_version will return error and spec_version will be set
0.1. BBL never checks a6 or set a1 which works for the legacy calls.
3. Older kernel with new SBI implementation(i.e OpenSBI) will never use
v0.2 calling conventions. OpenSBI never use a6 or set a1 for legacy
calls anyways.
Did I miss any usecase ?
> Second just blindly moving all the existing calls to a single legacy
> extension doesn't seem useful. We need to differenciate the existing
> calls:
I think the confusion is because of legacy renaming. They are not
single legacy extension. They are all separate extensions. The spec
just called all those extensions as collectively as legacy. So I just
tried to make the patch sync with the spec.
If that's the source of confusion, I can rename it to sbi_0.1_x in
stead of legacy.
>
> (1) actually board specific and have not place in a cpu abstraction
> layer: getchar/putchar, these should just never be advertised in
> a
> non-legacy setup, and the drivers using them should not probe
> on a sbi 0.2+ system
In that case, we have to update the drivers(earlycon-riscv-sbi &
hvc_riscv_sbi) in kernel as well. Once these patches are merged, nobody
will be able to use earlycon=sbi feature in mainline kernel.
Personally, I am fine with it. But there were some interest during
RISC-V workshop in keeping these for now for easy debugging and early
bringup.
> (2) useful for currently taped out cpus and in the long run for
> virtualization to avoid mmio traps: ipis, timers, tlb
> shootdown.
> These should stay backwards compatible, but for sbi 0.2 be
> negotiated individually
We still can do that with existing scheme.
> (3) in theory useful, but given how much of a big hammer sfence.i
> not useful in theory: SBI_REMOTE_FENCE_I we can decide if we
> want
> to either not allow it for sbi 0.2+ or also negotiate it. I'd
> personally favor not advertising it and just use ipis to
> implement
> it.
Once we have native IPIs, sure. Otherwise, sfence.i using IPI via SBI
will be even slower compared today. Gary had done the same thing for
sfence.vma and it was not enocouraged.
https://patchwork.kernel.org/patch/10845959/#22576987
> If we want useful acceleration of i-cache synchronization
> we'll need actual instructions that are much more fine grained
> in the future.
--
Regards,
Atish
On Tue, Aug 27, 2019 at 10:19:42PM +0000, Atish Patra wrote:
> I did not understand this part. All the legacy SBI calls are defined as
> a separate extension ID not single extension. How did it break the
> backward compatibility ?
Yes, sorry I mistead this. The way is is defined is rather
non-intuitive, but actually backwards compatible.
> I think the confusion is because of legacy renaming. They are not
> single legacy extension. They are all separate extensions. The spec
> just called all those extensions as collectively as legacy. So I just
> tried to make the patch sync with the spec.
>
> If that's the source of confusion, I can rename it to sbi_0.1_x in
> stead of legacy.
I think we actually need to fix the spec instead, even if it just the
naming and not the mechanism.
> > (1) actually board specific and have not place in a cpu abstraction
> > layer: getchar/putchar, these should just never be advertised in
> > a
> > non-legacy setup, and the drivers using them should not probe
> > on a sbi 0.2+ system
>
> In that case, we have to update the drivers(earlycon-riscv-sbi &
> hvc_riscv_sbi) in kernel as well. Once these patches are merged, nobody
> will be able to use earlycon=sbi feature in mainline kernel.
>
> Personally, I am fine with it. But there were some interest during
> RISC-V workshop in keeping these for now for easy debugging and early
> bringup.
The getchar/putchar calls unfortunately are fundamentally flawed, as
they mean the sbi can still access the console after the host has taken
it over using its own drivers. Which will lead to bugs sooner or later.
And if you can bring up a console driver in opensbi it should be just
as trivial to bring up the kernel version.
On Thu, 2019-08-29 at 03:59 -0700, [email protected] wrote:
> On Tue, Aug 27, 2019 at 10:19:42PM +0000, Atish Patra wrote:
> > I did not understand this part. All the legacy SBI calls are
> > defined as
> > a separate extension ID not single extension. How did it break the
> > backward compatibility ?
>
> Yes, sorry I mistead this. The way is is defined is rather
> non-intuitive, but actually backwards compatible.
>
> > I think the confusion is because of legacy renaming. They are not
> > single legacy extension. They are all separate extensions. The spec
> > just called all those extensions as collectively as legacy. So I
> > just
> > tried to make the patch sync with the spec.
> >
> > If that's the source of confusion, I can rename it to sbi_0.1_x in
> > stead of legacy.
>
> I think we actually need to fix the spec instead, even if it just the
> naming and not the mechanism.
>
If I understood you clearly, you want to call it legacy in the spec and
just say v0.1 extensions.
The whole idea of marking them as legacy extensions to indicate that it
would be obsolete in the future.
But I am not too worried about the semantics here. So I am fine with
just changing the text to v0.1 if that avoids confusion.
> > > (1) actually board specific and have not place in a cpu
> > > abstraction
> > > layer: getchar/putchar, these should just never be
> > > advertised in
> > > a
> > > non-legacy setup, and the drivers using them should not
> > > probe
> > > on a sbi 0.2+ system
> >
> > In that case, we have to update the drivers(earlycon-riscv-sbi &
> > hvc_riscv_sbi) in kernel as well. Once these patches are merged,
> > nobody
> > will be able to use earlycon=sbi feature in mainline kernel.
> >
> > Personally, I am fine with it. But there were some interest during
> > RISC-V workshop in keeping these for now for easy debugging and
> > early
> > bringup.
>
> The getchar/putchar calls unfortunately are fundamentally flawed, as
> they mean the sbi can still access the console after the host has
> taken
> it over using its own drivers. Which will lead to bugs sooner or
> later.
>
> And if you can bring up a console driver in opensbi it should be just
> as trivial to bring up the kernel version.
--
Regards,
Atish
On Fri, Aug 30, 2019 at 11:13:25PM +0000, Atish Patra wrote:
> If I understood you clearly, you want to call it legacy in the spec and
> just say v0.1 extensions.
>
> The whole idea of marking them as legacy extensions to indicate that it
> would be obsolete in the future.
>
> But I am not too worried about the semantics here. So I am fine with
> just changing the text to v0.1 if that avoids confusion.
So my main problems is that we are lumping all the "legacy" extensions
together. While some of them are simply a bad idea and shouldn't
really be implemented for anything new ever, others like the sfence.vma
and ipi ones are needed until we have hardware support to avoid them
and possibly forever for virtualization.
So either we use different markers of legacy for them, or we at least
define new extensions that replace them at the same time. What I
want to avoid is the possibіly of an implementation using the really
legacy bits and new extensions at the same time.
On Fri, Sep 13, 2019 at 08:54:27AM -0700, Palmer Dabbelt wrote:
> On Tue, Sep 3, 2019 at 12:38 AM [email protected] <[email protected]> wrote:
>
> > On Fri, Aug 30, 2019 at 11:13:25PM +0000, Atish Patra wrote:
> > > If I understood you clearly, you want to call it legacy in the spec and
> > > just say v0.1 extensions.
> > >
> > > The whole idea of marking them as legacy extensions to indicate that it
> > > would be obsolete in the future.
> > >
> > > But I am not too worried about the semantics here. So I am fine with
> > > just changing the text to v0.1 if that avoids confusion.
> >
> > So my main problems is that we are lumping all the "legacy" extensions
> > together. While some of them are simply a bad idea and shouldn't
> > really be implemented for anything new ever, others like the sfence.vma
> > and ipi ones are needed until we have hardware support to avoid them
> > and possibly forever for virtualization.
> >
> > So either we use different markers of legacy for them, or we at least
> > define new extensions that replace them at the same time. What I
> > want to avoid is the possibіly of an implementation using the really
> > legacy bits and new extensions at the same time.
> >
>
> Nominally we've got to replace these as well because we didn't include
> the length of the hart mask.
Well, let's do that as part of definining the first real post-0.1
SBI then, and don't bother defining the old ones as legacy at all.
Just two different specs that don't interact except that we reserve
extension space in the new one for the old one so that one SBI spec
can implement both.
On Sun, 15 Sep 2019 23:54:46 PDT (-0700), Christoph Hellwig wrote:
> On Fri, Sep 13, 2019 at 08:54:27AM -0700, Palmer Dabbelt wrote:
>> On Tue, Sep 3, 2019 at 12:38 AM [email protected] <[email protected]> wrote:
>>
>> > On Fri, Aug 30, 2019 at 11:13:25PM +0000, Atish Patra wrote:
>> > > If I understood you clearly, you want to call it legacy in the spec and
>> > > just say v0.1 extensions.
>> > >
>> > > The whole idea of marking them as legacy extensions to indicate that it
>> > > would be obsolete in the future.
>> > >
>> > > But I am not too worried about the semantics here. So I am fine with
>> > > just changing the text to v0.1 if that avoids confusion.
>> >
>> > So my main problems is that we are lumping all the "legacy" extensions
>> > together. While some of them are simply a bad idea and shouldn't
>> > really be implemented for anything new ever, others like the sfence.vma
>> > and ipi ones are needed until we have hardware support to avoid them
>> > and possibly forever for virtualization.
>> >
>> > So either we use different markers of legacy for them, or we at least
>> > define new extensions that replace them at the same time. What I
>> > want to avoid is the possibіly of an implementation using the really
>> > legacy bits and new extensions at the same time.
>> >
>>
>> Nominally we've got to replace these as well because we didn't include
>> the length of the hart mask.
>
> Well, let's do that as part of definining the first real post-0.1
> SBI then, and don't bother defining the old ones as legacy at all.
>
> Just two different specs that don't interact except that we reserve
> extension space in the new one for the old one so that one SBI spec
> can implement both.
Makes sense. We're getting finish with this "just go write everything down"
exercise, so we can start actually doing things now :).