[PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in SMODE.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- arch/riscv/Kconfig | 16 +++++++++++----- arch/riscv/cpu/fu540/Kconfig | 3 ++- arch/riscv/cpu/generic/Kconfig | 3 ++- arch/riscv/include/asm/global_data.h | 2 +- arch/riscv/lib/Makefile | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 009a545fcf..96c386225b 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -153,12 +153,18 @@ config 64BIT bool
config SIFIVE_CLINT - bool - depends on RISCV_MMODE || SPL_RISCV_MMODE + bool "SiFive's Core Local Interruptor (CLINT) driver" select REGMAP select SYSCON - select SPL_REGMAP if SPL - select SPL_SYSCON if SPL + help + The SiFive CLINT block holds memory-mapped control and status registers + associated with software and timer interrupts. + +config SPL_SIFIVE_CLINT + bool "SiFive's Core Local Interruptor (CLINT) driver in SPL" + depends on SPL + select SPL_REGMAP + select SPL_SYSCON help The SiFive CLINT block holds memory-mapped control and status registers associated with software and timer interrupts. @@ -186,7 +192,7 @@ config ANDES_PLMT associated with timer tick.
config RISCV_RDTIME - bool + bool "Timer API via rdtime instruction" default y if RISCV_SMODE || SPL_RISCV_SMODE help The provides the riscv_get_time() API that is implemented using the diff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig index 2dcad8e27f..8b2c83039f 100644 --- a/arch/riscv/cpu/fu540/Kconfig +++ b/arch/riscv/cpu/fu540/Kconfig @@ -8,7 +8,8 @@ config SIFIVE_FU540 imply CPU imply CPU_RISCV imply RISCV_TIMER - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE) + imply SIFIVE_CLINT if RISCV_MMODE + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBI diff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig index b2cb155d6d..62fcadd710 100644 --- a/arch/riscv/cpu/generic/Kconfig +++ b/arch/riscv/cpu/generic/Kconfig @@ -8,7 +8,8 @@ config GENERIC_RISCV imply CPU imply CPU_RISCV imply RISCV_TIMER - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE) + imply SIFIVE_CLINT if RISCV_MMODE + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBI diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 2eb14815bc..b89b469d41 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -18,7 +18,7 @@ struct arch_global_data { long boot_hart; /* boot hart id */ phys_addr_t firmware_fdt_addr; -#ifdef CONFIG_SIFIVE_CLINT +#if CONFIG_IS_ENABLED(SIFIVE_CLINT) void __iomem *clint; /* clint base address */ #endif #ifdef CONFIG_ANDES_PLIC diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 6c503ff2b2..861d7b489f 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o obj-$(CONFIG_CMD_GO) += boot.o obj-y += cache.o +obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y) -obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o obj-$(CONFIG_ANDES_PLIC) += andes_plic.o obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o else -- 2.28.0
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction fault which OpenSBI will emulate.
Regards, Anup
Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in SMODE.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
arch/riscv/Kconfig | 16 +++++++++++----- arch/riscv/cpu/fu540/Kconfig | 3 ++- arch/riscv/cpu/generic/Kconfig | 3 ++- arch/riscv/include/asm/global_data.h | 2 +- arch/riscv/lib/Makefile | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 009a545fcf..96c386225b 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -153,12 +153,18 @@ config 64BIT bool
config SIFIVE_CLINT
booldepends on RISCV_MMODE || SPL_RISCV_MMODE
bool "SiFive's Core Local Interruptor (CLINT) driver" select REGMAP select SYSCON
select SPL_REGMAP if SPLselect SPL_SYSCON if SPL
helpThe SiFive CLINT block holds memory-mapped control and status registersassociated with software and timer interrupts.+config SPL_SIFIVE_CLINT
bool "SiFive's Core Local Interruptor (CLINT) driver in SPL"depends on SPLselect SPL_REGMAPselect SPL_SYSCON help The SiFive CLINT block holds memory-mapped control and status registers associated with software and timer interrupts.@@ -186,7 +192,7 @@ config ANDES_PLMT associated with timer tick.
config RISCV_RDTIME
bool
bool "Timer API via rdtime instruction" default y if RISCV_SMODE || SPL_RISCV_SMODE help The provides the riscv_get_time() API that is implemented using thediff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig index 2dcad8e27f..8b2c83039f 100644 --- a/arch/riscv/cpu/fu540/Kconfig +++ b/arch/riscv/cpu/fu540/Kconfig @@ -8,7 +8,8 @@ config SIFIVE_FU540 imply CPU imply CPU_RISCV imply RISCV_TIMER
imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
imply SIFIVE_CLINT if RISCV_MMODEimply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBIdiff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig index b2cb155d6d..62fcadd710 100644 --- a/arch/riscv/cpu/generic/Kconfig +++ b/arch/riscv/cpu/generic/Kconfig @@ -8,7 +8,8 @@ config GENERIC_RISCV imply CPU imply CPU_RISCV imply RISCV_TIMER
imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
imply SIFIVE_CLINT if RISCV_MMODEimply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBIdiff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 2eb14815bc..b89b469d41 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -18,7 +18,7 @@ struct arch_global_data { long boot_hart; /* boot hart id */ phys_addr_t firmware_fdt_addr; -#ifdef CONFIG_SIFIVE_CLINT +#if CONFIG_IS_ENABLED(SIFIVE_CLINT) void __iomem *clint; /* clint base address */ #endif #ifdef CONFIG_ANDES_PLIC diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 6c503ff2b2..861d7b489f 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o obj-$(CONFIG_CMD_GO) += boot.o obj-y += cache.o +obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y) -obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o obj-$(CONFIG_ANDES_PLIC) += andes_plic.o obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o else -- 2.28.0
On 14.08.20 19:52, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction fault which OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11 has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and not the instruction:
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: insn 0x4114121602, epc 0x8058c168.
The illegal instruction being c01027f3 rdtime a5
In the long run it may make sense to provide backwards compatibility in OpenSBI.
Without this patch I observe a crash in the loady command. So I cannot load a file over the UART.
Best regards
Heinrich
Regards, Anup
Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in SMODE.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
arch/riscv/Kconfig | 16 +++++++++++----- arch/riscv/cpu/fu540/Kconfig | 3 ++- arch/riscv/cpu/generic/Kconfig | 3 ++- arch/riscv/include/asm/global_data.h | 2 +- arch/riscv/lib/Makefile | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 009a545fcf..96c386225b 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -153,12 +153,18 @@ config 64BIT bool
config SIFIVE_CLINT
booldepends on RISCV_MMODE || SPL_RISCV_MMODE
bool "SiFive's Core Local Interruptor (CLINT) driver" select REGMAP select SYSCON
select SPL_REGMAP if SPLselect SPL_SYSCON if SPL
helpThe SiFive CLINT block holds memory-mapped control and status registersassociated with software and timer interrupts.+config SPL_SIFIVE_CLINT
bool "SiFive's Core Local Interruptor (CLINT) driver in SPL"depends on SPLselect SPL_REGMAPselect SPL_SYSCON help The SiFive CLINT block holds memory-mapped control and status registers associated with software and timer interrupts.@@ -186,7 +192,7 @@ config ANDES_PLMT associated with timer tick.
config RISCV_RDTIME
bool
bool "Timer API via rdtime instruction" default y if RISCV_SMODE || SPL_RISCV_SMODE help The provides the riscv_get_time() API that is implemented using thediff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig index 2dcad8e27f..8b2c83039f 100644 --- a/arch/riscv/cpu/fu540/Kconfig +++ b/arch/riscv/cpu/fu540/Kconfig @@ -8,7 +8,8 @@ config SIFIVE_FU540 imply CPU imply CPU_RISCV imply RISCV_TIMER
imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
imply SIFIVE_CLINT if RISCV_MMODEimply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBIdiff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig index b2cb155d6d..62fcadd710 100644 --- a/arch/riscv/cpu/generic/Kconfig +++ b/arch/riscv/cpu/generic/Kconfig @@ -8,7 +8,8 @@ config GENERIC_RISCV imply CPU imply CPU_RISCV imply RISCV_TIMER
imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
imply SIFIVE_CLINT if RISCV_MMODEimply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBIdiff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 2eb14815bc..b89b469d41 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -18,7 +18,7 @@ struct arch_global_data { long boot_hart; /* boot hart id */ phys_addr_t firmware_fdt_addr; -#ifdef CONFIG_SIFIVE_CLINT +#if CONFIG_IS_ENABLED(SIFIVE_CLINT) void __iomem *clint; /* clint base address */ #endif #ifdef CONFIG_ANDES_PLIC diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 6c503ff2b2..861d7b489f 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o obj-$(CONFIG_CMD_GO) += boot.o obj-y += cache.o +obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y) -obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o obj-$(CONFIG_ANDES_PLIC) += andes_plic.o obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o else -- 2.28.0
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 14.08.20 19:52, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction fault which OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11 has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and not the instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this CSR.
The S-mode software always has working rdtime instruction for all RISC-V systems. If HW does not implement TIME CSR then OpenSBI emulates it. Please don't break this convention for U-Boot S-mode because we do have RISC-V systems where TIME CSR is implemeted in HW so this will patch will break U-Boot S-mode system because on those system we are supposed to use TIME CSR from S-mode.
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: insn 0x4114121602, epc 0x8058c168.
The illegal instruction being c01027f3 rdtime a5
In the long run it may make sense to provide backwards compatibility in OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c index 0e5523f..c8f2e4a 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) { - if (insn == 0) { - insn = sbi_get_insn(regs->mepc, &uptrap); - if (uptrap.cause) { - uptrap.epc = regs->mepc; - return sbi_trap_redirect(regs, &uptrap); - } + insn = sbi_get_insn(regs->mepc, &uptrap); + if (uptrap.cause) { + uptrap.epc = regs->mepc; + return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);
Regards, Anup
Without this patch I observe a crash in the loady command. So I cannot load a file over the UART.
Best regards
Heinrich
Regards, Anup
Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in SMODE.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
arch/riscv/Kconfig | 16 +++++++++++----- arch/riscv/cpu/fu540/Kconfig | 3 ++- arch/riscv/cpu/generic/Kconfig | 3 ++- arch/riscv/include/asm/global_data.h | 2 +- arch/riscv/lib/Makefile | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 009a545fcf..96c386225b 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -153,12 +153,18 @@ config 64BIT bool
config SIFIVE_CLINT
booldepends on RISCV_MMODE || SPL_RISCV_MMODE
bool "SiFive's Core Local Interruptor (CLINT) driver" select REGMAP select SYSCON
select SPL_REGMAP if SPLselect SPL_SYSCON if SPL
helpThe SiFive CLINT block holds memory-mapped control and status registersassociated with software and timer interrupts.+config SPL_SIFIVE_CLINT
bool "SiFive's Core Local Interruptor (CLINT) driver in SPL"depends on SPLselect SPL_REGMAPselect SPL_SYSCON help The SiFive CLINT block holds memory-mapped control and status registers associated with software and timer interrupts.@@ -186,7 +192,7 @@ config ANDES_PLMT associated with timer tick.
config RISCV_RDTIME
bool
bool "Timer API via rdtime instruction" default y if RISCV_SMODE || SPL_RISCV_SMODE help The provides the riscv_get_time() API that is implemented using thediff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig index 2dcad8e27f..8b2c83039f 100644 --- a/arch/riscv/cpu/fu540/Kconfig +++ b/arch/riscv/cpu/fu540/Kconfig @@ -8,7 +8,8 @@ config SIFIVE_FU540 imply CPU imply CPU_RISCV imply RISCV_TIMER
imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
imply SIFIVE_CLINT if RISCV_MMODEimply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBIdiff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig index b2cb155d6d..62fcadd710 100644 --- a/arch/riscv/cpu/generic/Kconfig +++ b/arch/riscv/cpu/generic/Kconfig @@ -8,7 +8,8 @@ config GENERIC_RISCV imply CPU imply CPU_RISCV imply RISCV_TIMER
imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
imply SIFIVE_CLINT if RISCV_MMODEimply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE imply CMD_CPU imply SPL_CPU_SUPPORT imply SPL_OPENSBIdiff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 2eb14815bc..b89b469d41 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -18,7 +18,7 @@ struct arch_global_data { long boot_hart; /* boot hart id */ phys_addr_t firmware_fdt_addr; -#ifdef CONFIG_SIFIVE_CLINT +#if CONFIG_IS_ENABLED(SIFIVE_CLINT) void __iomem *clint; /* clint base address */ #endif #ifdef CONFIG_ANDES_PLIC diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 6c503ff2b2..861d7b489f 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o obj-$(CONFIG_CMD_GO) += boot.o obj-y += cache.o +obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y) -obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o obj-$(CONFIG_ANDES_PLIC) += andes_plic.o obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o else -- 2.28.0
On 8/14/20 8:38 PM, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 14.08.20 19:52, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction fault which OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11 has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and not the instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this CSR.
The S-mode software always has working rdtime instruction for all RISC-V systems. If HW does not implement TIME CSR then OpenSBI emulates it. Please don't break this convention for U-Boot S-mode because we do have RISC-V systems where TIME CSR is implemeted in HW so this will patch will break U-Boot S-mode system because on those system we are supposed to use TIME CSR from S-mode.
This patch does not change anything for existing systems. It only allows me to customize U-Boot for the K210 differently. I understand that fixing OpenSBI is a better approach.
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: insn 0x4114121602, epc 0x8058c168.
The illegal instruction being c01027f3 rdtime a5
In the long run it may make sense to provide backwards compatibility in OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c index 0e5523f..c8f2e4a 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) {
Why do put sbi_get_insn() behind this if and not before it?
if (insn == 0) {insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap);}
insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);
For this illegal instruction exception the fix works. But I think the change is in the wrong place.
We have to cover all exceptions, e.g. unaligned access. So we better should determine the instruction in sbi_trap_handler().
Best regards
Heinrich
Regards, Anup
Without this patch I observe a crash in the loady command. So I cannot load a file over the UART.
Best regards
Heinrich
Regards, Anup
On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 8/14/20 8:38 PM, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 14.08.20 19:52, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction fault which OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11 has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and not the instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this CSR.
The S-mode software always has working rdtime instruction for all RISC-V systems. If HW does not implement TIME CSR then OpenSBI emulates it. Please don't break this convention for U-Boot S-mode because we do have RISC-V systems where TIME CSR is implemeted in HW so this will patch will break U-Boot S-mode system because on those system we are supposed to use TIME CSR from S-mode.
This patch does not change anything for existing systems. It only allows me to customize U-Boot for the K210 differently. I understand that fixing OpenSBI is a better approach.
Currently, on most RISC-V systems the CLINT timer interrupts and IPI interrupts are hard-wired to M-mode timer and software interrupt lines. In other words, the CLINT is integrated as M-mode only device on most RISC-V systems.
Due to above reason, CLINT driver is M-mode only driver for Linux kernel
The Linux S-mode will use: 1. TIME CSR as free running counter 2. SBI calls for timer interrupts 3. SBI calls for injecting IPIs
For #1 above, the M-mode firmware will trap-n-emulate TIME CSR for S-mode if HW does not implement TIME CSR.
Based on above mentioned convention, the U-Boot CLINT driver should be M-mode only and U-Boot S-mode should use TIME CSR as a free running counter.
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: insn 0x4114121602, epc 0x8058c168.
The illegal instruction being c01027f3 rdtime a5
In the long run it may make sense to provide backwards compatibility in OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c index 0e5523f..c8f2e4a 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) {Why do put sbi_get_insn() behind this if and not before it?
Currently, OpenSBI only deals with 32bit (or longer) illegal instructions. If we see insn == 0 OR insn is 16bit then we double-check instruction encoding using unprivileged access.
The PC in RISC-V is always 2-byte aligned address so if MTVAL has fault instruction address instead of instruction encoding then "(insn & 3) != 3" will be TRUE and we will be forced to double-check. The "insn == 0" check below is causing problem RISC-V v1.9 spec (i.e. MTVAL having instruction address) and it is totally harmless to remove the "insn == 0" check for RISC-V v1.10 (or higher) hence my suggestion to remove the check.
if (insn == 0) {insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap);}
insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);For this illegal instruction exception the fix works. But I think the change is in the wrong place.
We have to cover all exceptions, e.g. unaligned access. So we better should determine the instruction in sbi_trap_handler().
That's incorrect.
For RISC-V v1.10 (or higher), the MTVAL contains: 1. Instruction encoding for illegal instruction trap 2. Fault address for unaligned traps, page faults, and access faults
For RISC-V v1.10 (or higher), the unaligned trap does not provide fault instruction encoding so we end-up doing unpriv access.
Base on above, it's best to work-around your issue in sbi_illegal_insn_handler() instead of sbi_trap_handler()
Regards, Anup
Best regards
Heinrich
Regards, Anup
Without this patch I observe a crash in the loady command. So I cannot load a file over the UART.
Best regards
Heinrich
Regards, Anup
Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel anup@brainfault.org:
On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 8/14/20 8:38 PM, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
On 14.08.20 19:52, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
On the Kendryte K210 OpenBSI cannot emulate the rdtime
instruction. So we
have to use the Sifive CLINT driver to provide riscv_get_time()
in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction
fault which
OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur
1.11
has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and
not the
instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this CSR.
The S-mode software always has working rdtime instruction for all RISC-V systems. If HW does not implement TIME CSR then OpenSBI emulates it. Please don't break this convention for U-Boot S-mode because we do have RISC-V systems where TIME CSR is implemeted in HW so this will patch will break U-Boot S-mode system because on those system we are supposed to use TIME CSR from S-mode.
This patch does not change anything for existing systems. It only
allows
me to customize U-Boot for the K210 differently. I understand that fixing OpenSBI is a better approach.
Currently, on most RISC-V systems the CLINT timer interrupts and IPI interrupts are hard-wired to M-mode timer and software interrupt lines. In other words, the CLINT is integrated as M-mode only device on most RISC-V systems.
Due to above reason, CLINT driver is M-mode only driver for Linux kernel
The Linux S-mode will use:
- TIME CSR as free running counter
- SBI calls for timer interrupts
- SBI calls for injecting IPIs
For #1 above, the M-mode firmware will trap-n-emulate TIME CSR for S-mode if HW does not implement TIME CSR.
Based on above mentioned convention, the U-Boot CLINT driver should be M-mode only and U-Boot S-mode should use TIME CSR as a free running counter.
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: insn 0x4114121602, epc 0x8058c168.
The illegal instruction being c01027f3 rdtime a5
In the long run it may make sense to provide backwards
compatibility in
OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c
b/lib/sbi/sbi_illegal_insn.c
index 0e5523f..c8f2e4a 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
struct
sbi_trap_regs *regs) struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) {Why do put sbi_get_insn() behind this if and not before it?
Currently, OpenSBI only deals with 32bit (or longer) illegal instructions. If we see insn == 0 OR insn is 16bit then we double-check instruction encoding using unprivileged access.
The PC in RISC-V is always 2-byte aligned address so if MTVAL has fault instruction address instead of instruction encoding then "(insn & 3) != 3" will be TRUE and we will be forced to double-check. The "insn == 0" check below is causing problem RISC-V v1.9 spec (i.e. MTVAL having instruction address) and it is totally harmless to remove the "insn == 0" check for RISC-V v1.10 (or higher) hence my suggestion to remove the check.
Thank you for your detailed explanation. Maybe you can add a comment to the code.
if (insn == 0) {insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs,&uptrap);
}
insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);For this illegal instruction exception the fix works. But I think the change is in the wrong place.
We have to cover all exceptions, e.g. unaligned access. So we better should determine the instruction in sbi_trap_handler().
That's incorrect.
For RISC-V v1.10 (or higher), the MTVAL contains:
- Instruction encoding for illegal instruction trap
- Fault address for unaligned traps, page faults, and access faults
For RISC-V v1.10 (or higher), the unaligned trap does not provide fault instruction encoding so we end-up doing unpriv access.
Base on above, it's best to work-around your issue in sbi_illegal_insn_handler() instead of sbi_trap_handler()
That clarifies it.
For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
Best regards
Heinrich
Regards, Anup
Best regards
Heinrich
Regards, Anup
Without this patch I observe a crash in the loady command. So I
cannot
load a file over the UART.
Best regards
Heinrich
Regards, Anup
On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel anup@brainfault.org:
On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 8/14/20 8:38 PM, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
On 14.08.20 19:52, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
> > On the Kendryte K210 OpenBSI cannot emulate the rdtime
instruction. So we
> have to use the Sifive CLINT driver to provide riscv_get_time()
in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction
fault which
OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur
1.11
has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and
not the
instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this CSR.
The S-mode software always has working rdtime instruction for all RISC-V systems. If HW does not implement TIME CSR then OpenSBI emulates it. Please don't break this convention for U-Boot S-mode because we do have RISC-V systems where TIME CSR is implemeted in HW so this will patch will break U-Boot S-mode system because on those system we are supposed to use TIME CSR from S-mode.
This patch does not change anything for existing systems. It only
allows
me to customize U-Boot for the K210 differently. I understand that fixing OpenSBI is a better approach.
Currently, on most RISC-V systems the CLINT timer interrupts and IPI interrupts are hard-wired to M-mode timer and software interrupt lines. In other words, the CLINT is integrated as M-mode only device on most RISC-V systems.
Due to above reason, CLINT driver is M-mode only driver for Linux kernel
The Linux S-mode will use:
- TIME CSR as free running counter
- SBI calls for timer interrupts
- SBI calls for injecting IPIs
For #1 above, the M-mode firmware will trap-n-emulate TIME CSR for S-mode if HW does not implement TIME CSR.
Based on above mentioned convention, the U-Boot CLINT driver should be M-mode only and U-Boot S-mode should use TIME CSR as a free running counter.
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: insn 0x4114121602, epc 0x8058c168.
The illegal instruction being c01027f3 rdtime a5
In the long run it may make sense to provide backwards
compatibility in
OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c
b/lib/sbi/sbi_illegal_insn.c
index 0e5523f..c8f2e4a 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
struct
sbi_trap_regs *regs) struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) {Why do put sbi_get_insn() behind this if and not before it?
Currently, OpenSBI only deals with 32bit (or longer) illegal instructions. If we see insn == 0 OR insn is 16bit then we double-check instruction encoding using unprivileged access.
The PC in RISC-V is always 2-byte aligned address so if MTVAL has fault instruction address instead of instruction encoding then "(insn & 3) != 3" will be TRUE and we will be forced to double-check. The "insn == 0" check below is causing problem RISC-V v1.9 spec (i.e. MTVAL having instruction address) and it is totally harmless to remove the "insn == 0" check for RISC-V v1.10 (or higher) hence my suggestion to remove the check.
Thank you for your detailed explanation. Maybe you can add a comment to the code.
Sure will do.
if (insn == 0) {insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs,&uptrap);
}
insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);For this illegal instruction exception the fix works. But I think the change is in the wrong place.
We have to cover all exceptions, e.g. unaligned access. So we better should determine the instruction in sbi_trap_handler().
That's incorrect.
For RISC-V v1.10 (or higher), the MTVAL contains:
- Instruction encoding for illegal instruction trap
- Fault address for unaligned traps, page faults, and access faults
For RISC-V v1.10 (or higher), the unaligned trap does not provide fault instruction encoding so we end-up doing unpriv access.
Base on above, it's best to work-around your issue in sbi_illegal_insn_handler() instead of sbi_trap_handler()
That clarifies it.
For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
Yes, already doing that.
Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
Sure, let us know if you find any other issue.
Regards, Anup
On 8/15/20 5:55 PM, Anup Patel wrote:
On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel anup@brainfault.org:
On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 8/14/20 8:38 PM, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
On 14.08.20 19:52, Anup Patel wrote: > On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
>> >> On the Kendryte K210 OpenBSI cannot emulate the rdtime
instruction. So we
>> have to use the Sifive CLINT driver to provide riscv_get_time()
in SMODE.
> > Can you elaborate why ? > > The rdtime instruction should generate an illegal instruction
fault which
> OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur
1.11
has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and
not the
instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this CSR.
The S-mode software always has working rdtime instruction for all RISC-V systems. If HW does not implement TIME CSR then OpenSBI emulates it. Please don't break this convention for U-Boot S-mode because we do have RISC-V systems where TIME CSR is implemeted in HW so this will patch will break U-Boot S-mode system because on those system we are supposed to use TIME CSR from S-mode.
This patch does not change anything for existing systems. It only
allows
me to customize U-Boot for the K210 differently. I understand that fixing OpenSBI is a better approach.
Currently, on most RISC-V systems the CLINT timer interrupts and IPI interrupts are hard-wired to M-mode timer and software interrupt lines. In other words, the CLINT is integrated as M-mode only device on most RISC-V systems.
Due to above reason, CLINT driver is M-mode only driver for Linux kernel
The Linux S-mode will use:
- TIME CSR as free running counter
- SBI calls for timer interrupts
- SBI calls for injecting IPIs
For #1 above, the M-mode firmware will trap-n-emulate TIME CSR for S-mode if HW does not implement TIME CSR.
Based on above mentioned convention, the U-Boot CLINT driver should be M-mode only and U-Boot S-mode should use TIME CSR as a free running counter.
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: insn 0x4114121602, epc 0x8058c168.
The illegal instruction being c01027f3 rdtime a5
In the long run it may make sense to provide backwards
compatibility in
OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c
b/lib/sbi/sbi_illegal_insn.c
index 0e5523f..c8f2e4a 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
struct
sbi_trap_regs *regs) struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) {Why do put sbi_get_insn() behind this if and not before it?
Currently, OpenSBI only deals with 32bit (or longer) illegal instructions. If we see insn == 0 OR insn is 16bit then we double-check instruction encoding using unprivileged access.
The PC in RISC-V is always 2-byte aligned address so if MTVAL has fault instruction address instead of instruction encoding then "(insn & 3) != 3" will be TRUE and we will be forced to double-check. The "insn == 0" check below is causing problem RISC-V v1.9 spec (i.e. MTVAL having instruction address) and it is totally harmless to remove the "insn == 0" check for RISC-V v1.10 (or higher) hence my suggestion to remove the check.
Thank you for your detailed explanation. Maybe you can add a comment to the code.
Sure will do.
if (insn == 0) {insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs,&uptrap);
}
insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);For this illegal instruction exception the fix works. But I think the change is in the wrong place.
We have to cover all exceptions, e.g. unaligned access. So we better should determine the instruction in sbi_trap_handler().
That's incorrect.
For RISC-V v1.10 (or higher), the MTVAL contains:
- Instruction encoding for illegal instruction trap
- Fault address for unaligned traps, page faults, and access faults
For RISC-V v1.10 (or higher), the unaligned trap does not provide fault instruction encoding so we end-up doing unpriv access.
Base on above, it's best to work-around your issue in sbi_illegal_insn_handler() instead of sbi_trap_handler()
That clarifies it.
For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
Yes, already doing that.
Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
grubriscv64.efi with too many modules loaded runs out of memory on the Maixduino board.
With
./grub-mkimage -O riscv64-efi -o grubriscv64.efi --prefix= -d \ grub-core lsefisystab minicmd normal reboot
I get a GRUB binary containing the minimum required for the U-Boot unit tests.
After applying the patches
lib: sbi_init: Avoid thundering hurd problem with coldbook_lock - http://lists.infradead.org/pipermail/opensbi/2020-August/000107.html
lib: sbi: Handle the case were MTVAL has illegal instruction address http://lists.infradead.org/pipermail/opensbi/2020-August/000108.html
to OpenSBI I can start GRUB and execute the commands used by the U-Boot unit tests successfully on the Maixduino board.
Best regards
Heinrich
Sure, let us know if you find any other issue.
Regards, Anup
On Sun, Aug 16, 2020 at 3:49 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 8/15/20 5:55 PM, Anup Patel wrote:
On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel anup@brainfault.org:
On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 8/14/20 8:38 PM, Anup Patel wrote:
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
> > On 14.08.20 19:52, Anup Patel wrote: >> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
xypron.glpk@gmx.de wrote:
>>> >>> On the Kendryte K210 OpenBSI cannot emulate the rdtime
instruction. So we
>>> have to use the Sifive CLINT driver to provide riscv_get_time()
in SMODE.
>> >> Can you elaborate why ? >> >> The rdtime instruction should generate an illegal instruction
fault which
>> OpenSBI will emulate. > > The RISC-V Instruction Set Manual Volume II Privileged architectur
1.11
> has incompatible changes relative to version 1.9.1 > > mtval on the Kendryte K210 delivers the bad virtual address and
not the
> instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this CSR.
The S-mode software always has working rdtime instruction for all RISC-V systems. If HW does not implement TIME CSR then OpenSBI emulates it. Please don't break this convention for U-Boot S-mode because we do have RISC-V systems where TIME CSR is implemeted in HW so this will patch will break U-Boot S-mode system because on those system we are supposed to use TIME CSR from S-mode.
This patch does not change anything for existing systems. It only
allows
me to customize U-Boot for the K210 differently. I understand that fixing OpenSBI is a better approach.
Currently, on most RISC-V systems the CLINT timer interrupts and IPI interrupts are hard-wired to M-mode timer and software interrupt lines. In other words, the CLINT is integrated as M-mode only device on most RISC-V systems.
Due to above reason, CLINT driver is M-mode only driver for Linux kernel
The Linux S-mode will use:
- TIME CSR as free running counter
- SBI calls for timer interrupts
- SBI calls for injecting IPIs
For #1 above, the M-mode firmware will trap-n-emulate TIME CSR for S-mode if HW does not implement TIME CSR.
Based on above mentioned convention, the U-Boot CLINT driver should be M-mode only and U-Boot S-mode should use TIME CSR as a free running counter.
> > lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler: > insn 0x4114121602, epc 0x8058c168. > > The illegal instruction being > c01027f3 rdtime a5 > > In the long run it may make sense to provide backwards
compatibility in
> OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c
b/lib/sbi/sbi_illegal_insn.c
index 0e5523f..c8f2e4a 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
struct
sbi_trap_regs *regs) struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) {Why do put sbi_get_insn() behind this if and not before it?
Currently, OpenSBI only deals with 32bit (or longer) illegal instructions. If we see insn == 0 OR insn is 16bit then we double-check instruction encoding using unprivileged access.
The PC in RISC-V is always 2-byte aligned address so if MTVAL has fault instruction address instead of instruction encoding then "(insn & 3) != 3" will be TRUE and we will be forced to double-check. The "insn == 0" check below is causing problem RISC-V v1.9 spec (i.e. MTVAL having instruction address) and it is totally harmless to remove the "insn == 0" check for RISC-V v1.10 (or higher) hence my suggestion to remove the check.
Thank you for your detailed explanation. Maybe you can add a comment to the code.
Sure will do.
if (insn == 0) {insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs,&uptrap);
}
insn = sbi_get_insn(regs->mepc, &uptrap);if (uptrap.cause) {uptrap.epc = regs->mepc;return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);For this illegal instruction exception the fix works. But I think the change is in the wrong place.
We have to cover all exceptions, e.g. unaligned access. So we better should determine the instruction in sbi_trap_handler().
That's incorrect.
For RISC-V v1.10 (or higher), the MTVAL contains:
- Instruction encoding for illegal instruction trap
- Fault address for unaligned traps, page faults, and access faults
For RISC-V v1.10 (or higher), the unaligned trap does not provide fault instruction encoding so we end-up doing unpriv access.
Base on above, it's best to work-around your issue in sbi_illegal_insn_handler() instead of sbi_trap_handler()
That clarifies it.
For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
Yes, already doing that.
Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
grubriscv64.efi with too many modules loaded runs out of memory on the Maixduino board.
With
./grub-mkimage -O riscv64-efi -o grubriscv64.efi --prefix= -d \ grub-core lsefisystab minicmd normal reboot
I get a GRUB binary containing the minimum required for the U-Boot unit tests.
After applying the patches
lib: sbi_init: Avoid thundering hurd problem with coldbook_lock - http://lists.infradead.org/pipermail/opensbi/2020-August/000107.html
lib: sbi: Handle the case were MTVAL has illegal instruction address http://lists.infradead.org/pipermail/opensbi/2020-August/000108.html
to OpenSBI I can start GRUB and execute the commands used by the U-Boot unit tests successfully on the Maixduino board.
If possible please add: 1. defconfig for U-Boot S-mode on Kendryte K210 2. documentation about how to run U-Boot S-mode on Kendryte K210
This will be very helpful to users who want to run some RTOS in S-mode using U-Boot S-mode.
Regards, Anup
participants (2)
-
Anup Patel -
Heinrich Schuchardt