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