On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel <a...@brainfault.org>:
> >On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt
> ><xypron.g...@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.g...@gmx.de> wrote:
> >> >>
> >> >> On 14.08.20 19:52, Anup Patel wrote:
> >> >>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
> ><xypron.g...@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.
Sure, let us know if you find any other issue.
Regards,
Anup
