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

[Anup Patel]

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.

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

Regards,
Anup

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