On Sun, Aug 16, 2020 at 3:49 PM Heinrich Schuchardt <[email protected]> wrote: > > 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.
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
