| Message ID | 20200814174515.14383-1-xypron.glpk@gmx.de |
|---|---|
| State | Rejected, archived |
| Delegated to: | Andes |
| Headers | show |
| Series | [1/1] riscv: riscv_get_time() implementation for SMODE | expand |
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 > - 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 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 >> - 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: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 > >> - 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 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: >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. > 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: >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() > 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: > >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. > > > > 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: > >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() > > > > 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: >>> 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. >>> >> >> 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: >>> 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() >>> >> >> 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: > >>> 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. > >>> > >> > >> 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: > >>> 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() > >>> > >> > >> 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
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
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(-) -- 2.28.0