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.
>>
>> > - 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?
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.
Best regards
Heinrich
>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
>> >>>
