On 8/15/20 5:55 PM, Anup Patel wrote: > On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt <[email protected]> > wrote: >> >> Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel <[email protected]>: >>> On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt >>> <[email protected]> wrote: >>>> >>>> On 8/14/20 8:38 PM, Anup Patel wrote: >>>>> On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt >>> <[email protected]> wrote: >>>>>> >>>>>> On 14.08.20 19:52, Anup Patel wrote: >>>>>>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt >>> <[email protected]> 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 >
