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