[PATCH 2/2] lib: sbi: Handle the case were MTVAL has illegal instruction address

[Jessica Clarke]

On 16 Aug 2020, at 15:15, Anup Patel <anup at brainfault.org> wrote:
> 
> On Sun, Aug 16, 2020 at 1:21 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>> 
>> On 15 Aug 2020, at 18:00, Anup Patel <Anup.Patel at wdc.com> wrote:
>>> 
>>> The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction
>>> address (instead of instruction encoding) on illegal instruction trap.
>>> 
>>> To handle above case, we fix sbi_illegal_insn_handler() without any
>>> impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting
>>> the fact that program counter (and instruction address) is always 2-byte
>>> aligned in RISC-V world.
>>> 
>>> Signed-off-by: Anup Patel <anup.patel at wdc.com>
>>> ---
>>> lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------
>>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
>>> index 0e5523f..779a727 100644
>>> --- a/lib/sbi/sbi_illegal_insn.c
>>> +++ b/lib/sbi/sbi_illegal_insn.c
>>> @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
>>> {
>>>      struct sbi_trap_info uptrap;
>>> 
>>> +     /*
>>> +      * We only deal with 32bit (or longer) illegal instructions. If we
>>> +      * see instruction is zero OR instruction is 16bit then we fetch and
>>> +      * check the instruction encoding using unprivilege access.
>>> +      *
>>> +      * The program counter (PC) in RISC-V world is always 2-byte aligned
>>> +      * so handling only 32bit (or longer) illegal instructions also help
>>> +      * the case where MTVAL CSR contains instruction address for illegal
>>> +      * instruction trap.
>>> +      */
>>> +
>>>      if (unlikely((insn & 3) != 3)) {
>> 
>> Isn't `(insn & 3) == 1` also unambiguous? It's only ambiguous when the
>> LSB is 0.
> 
> I did not understand your question.
> 
> The "(insn & 3) != 3" means either:
> 1. insn[1:0] == 0 hence it is Quadrant0 16bit instruction OR entire insn == 0
> 2. insn[1:0] == 1 hence it is Quadrant1 16bit instruction
> 3. insn[1:0] == 2 hence it is Quadrant2 16bit instruction

My point was, for case 2, the LSB is 1, therefore MTVAL must be an
instruction and not a PC, so there is no need to fetch the instruction.

>> I do also wonder if this would be better as a single boot-time quirk
>> test (ie deliberately fault and see what the hart reports), so as to
>> not overly penalise 1.10-implementing harts.
> 
> We are not penalising 1.10-implemetation because we are not
> emulating any 16bit illegal instructions. In fact, we are removing
> in "insn == 0" check for 1.10-implementation so we have removed
> couple of instructions for 1.10-implementation.

Right, I had forgotten that the only compressed instruction emulation
is for unaligned accesses, at least for the current C specification.
Though this does still add overhead on 1.10 implementations for "truly
illegal" (I don't know if OpenSBI reuses that term from BBL)
instructions. For ones that are just going to cause the program in
question to be killed with SIGILL that's probably not so important (and
hopefully rare), but floating point instructions (like C.FLW) are
important to think about here. OSes like to lazily context-switch
floating-point state, and do so on RISC-V by setting MSTATUS.FS back to
0 so that the first time the switched-to process tries to perform a
floating-point operation it will trap, the OS can restore the right
floating-point state and then retry the instruction. Maybe the overhead
of doing all that already means that the increased inefficiency in
OpenSBI forwarding it to S-mode isn't significant, but it is a little
sad to be doing this unnecessary work.

Jess

>>> -             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);
>>> --
>>> 2.25.1
>>> 
>>> 
>>> --
>>> opensbi mailing list
>>> opensbi at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/opensbi
> 
> Regards,
> Anup