Wrong MEPC saved with branch predictor configuration

[sam-shahrestani]

Observed Behavior

During long LSU stalls, predicted taken branches can cause the skid buffer to be loaded with a speculative address. Unfortunately, an interrupt taken during the stall may cause this value to be saved into mepc. When the ISR returns, incorrect behaviour can be observed.

To demonstrate, I have made a few changes to the simple system and written some basic software to demonstrate: sam-shahrestani@ab12b99

The relevant disassembly:

0010033e <memory_access_fail>:
#include "simple_system_common.h"

volatile int foo;

void memory_access_fail(void)
{
  10033e:       1141                    addi    sp,sp,-16
  puts("Test failed!\n");
  100340:       00000517                auipc   a0,0x0
  100344:       11450513                addi    a0,a0,276 # 100454 <main+0x52>
{
  100348:       c606                    sw      ra,12(sp)
  puts("Test failed!\n");
  10034a:       3399                    jal     100090 <puts>
  sim_halt();
}
  10034c:       40b2                    lw      ra,12(sp)
  10034e:       0141                    addi    sp,sp,16
  sim_halt();
  100350:       bba5                    j       1000c8 <sim_halt>

00100352 <memory_access_loop>:

/* Write foo once, then read it indefinitely, failing if the set value has changed */
void memory_access_loop(void)
{
  foo = 0xdeadbeef;
  100352:       deadc737                lui     a4,0xdeadc
  100356:       00000797                auipc   a5,0x0
  10035a:       12278793                addi    a5,a5,290 # 100478 <foo>
  10035e:       eef70713                addi    a4,a4,-273 # deadbeef <fromhost+0xde9a3edf>
  100362:       c398                    sw      a4,0(a5)
  asm volatile("" ::: "memory");

  while (1)
  {
    asm volatile (
  100364:       a011                    j       100368 <memory_access_loop+0x16>
  100366:       3fe1                    jal     10033e <memory_access_fail>
  100368:       deadc2b7                lui     t0,0xdeadc
  10036c:       eef28293                addi    t0,t0,-273 # deadbeef <fromhost+0xde9a3edf>
  100370:       0007a303                lw      t1,0(a5)
  100374:       fe6299e3                bne     t0,t1,100366 <memory_access_loop+0x14>
  while (1)
  100378:       b7f5                    j       100364 <memory_access_loop+0x12>

memory_access_loop runs while timer interrupts are constantly going off. There is a branch targeting a (call to a) simulation halting function at 100366, but the comparison is a constant to a memory value that never changes, so this branch will never be taken. It is a backwards branch, so it is always predicted taken.

At the time of the failing round, one can observe 100366 being saved into mepc because pc_if_o is set to a speculative address by instr_skid_valid_q. Instead, it should be set to the corrected fetch_addr after a mispredict is detected (i.e. 100378).

I will attach a trace file in a comment for more detail.

Expected Behavior

Do not save an incorrect MEPC.

Steps to reproduce the issue

• Build the ibex simple system using the given branch with the configuration fusesoc --cores-root=. run --target=sim --setup --build lowrisc:ibex:ibex_simple_system ./util/ibex_config.py experimental-branch-predictor fusesoc_opts``
• Build the software example with make -C examples/sw/simple_system/mepc_test
• Run the test with ./build/lowrisc_ibex_ibex_simple_system_0/sim-verilator/Vibex_simple_system -t --meminit=ram,./examples/sw/simple_system/mepc_test/mepc_test.elf
• Observe the test does not loop indefinitely, it crashes with the log message "Test failed!"
• Additionally, using a BP-less config (such as small) or applying the suggested RTL fix in ibex_if_stage.sv, the test program will loop indefinitely (which is correct behaviour)

My Environment

Version of the Ibex source code:

You can see this on master, i.e. 9e8b32a13802db36515a8ca042a5e9889cdf9218, but only with LSU stalls. The provided branch models this in a simple way using simple_system.

[sam-shahrestani]

The trace file of the observed behaviour sim.fst.zip

I can turn the example fix from this branch into a PR very quickly if you guys think its a good fix for master

[marnovandermaas]

What's your proposed RTL fix?

[sam-shahrestani]

My proposed fix is in the demo branch:

    // assign instr_skid_en = predict_branch_taken & ~pc_set_i & ~id_in_ready_i & ~instr_skid_valid_q;
    assign instr_skid_en = predict_branch_taken & ~pc_set_i & ~id_in_ready_i & ~nt_branch_mispredict_i
                                 & ~instr_skid_valid_q;

    // assign instr_skid_valid_d = (instr_skid_valid_q & ~id_in_ready_i & ~stall_dummy_instr) |
    //                            instr_skid_en;
    assign instr_skid_valid_d = (instr_skid_valid_q & ~id_in_ready_i & ~stall_dummy_instr
                                 & ~nt_branch_mispredict_i) | instr_skid_en;