[PATCH 1/1] riscv: riscv_get_time() implementation for SMODE

[Heinrich Schuchardt]

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.

>
>>
>> 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?

> -               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().

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
>>>