[PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
Anup Patel
anup at brainfault.org
Sun Aug 16 15:38:05 CEST 2020
On Sun, Aug 16, 2020 at 3:49 PM Heinrich Schuchardt <xypron.glpk at 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 at gmx.de> wrote:
> >>
> >> Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel <anup at brainfault.org>:
> >>> On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt
> >>> <xypron.glpk at 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 at 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 at 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
More information about the U-Boot
mailing list