lowRISC / lowRISC/ibex

Wrong MEPC saved with branch predictor configuration

Open
#2,319 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Type:Bug
Dominant language
SystemVerilog
Stars
2.1k
Forks
810
Avg merge
5d 23h
Merged PRs (30d)
9

Description

## 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: https://github.com/sam-shahrestani/ibex/commit/ab12b99b38c917eb950433c2a581c74c4660d2d5

The relevant disassembly:
```assembly
0010033e :
#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
{
100348: c606 sw ra,12(sp)
puts("Test failed!\n");
10034a: 3399 jal 100090
sim_halt();
}
10034c: 40b2 lw ra,12(sp)
10034e: 0141 addi sp,sp,16
sim_halt();
100350: bba5 j 1000c8

00100352 :

/* 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
10035e: eef70713 addi a4,a4,-273 # deadbeef
100362: c398 sw a4,0(a5)
asm volatile("" ::: "memory");

while (1)
{
asm volatile (
100364: a011 j 100368
100366: 3fe1 jal 10033e
100368: deadc2b7 lui t0,0xdeadc
10036c: eef28293 addi t0,t0,-273 # deadbeef
100370: 0007a303 lw t1,0(a5)
100374: fe6299e3 bne t0,t1,100366
while (1)
100378: b7f5 j 100364
```

`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`).

Image

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](https://github.com/sam-shahrestani/ibex/commit/ab12b99b38c917eb950433c2a581c74c4660d2d5) models this in a simple way using `simple_system`.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the failure with the experimental-branch-predictor simple-system build and the examples/sw/simple_system/mepc_test program. Then inspect ibex_if_stage.sv around instr_skid_valid_q, pc_if_o, and the corrected fetch_addr; done means the test loops indefinitely and does not save an incorrect MEPC.

Written by the indexing model from the issue text.

Assessment

Domain
embedded-iot
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.