[PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
Heinrich Schuchardt
xypron.glpk at gmx.de
Sun Aug 16 12:14:10 CEST 2020
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.
Best regards
Heinrich
>
> Sure, let us know if you find any other issue.
>
> Regards,
> Anup
>
More information about the U-Boot
mailing list