lowRISC / ibex

Ibex is a small 32 bit RISC-V CPU core, previously known as zero-riscy.
https://www.lowrisc.org
Apache License 2.0
1.37k stars 543 forks source link

Incorrect stalling behaviour for configurations with writeback stage but without branch target ALU results in an incorrect branch target #2169

Open KatCe opened 5 months ago

KatCe commented 5 months ago

Observed Behavior

If the top level data_gnt_i signal is either constantly high, or it is high at certain clock cycles (without an outstanding request), and a load is followed by a branch, the fetch address is set to the comparison result of the branch.

Expected Behavior

Ibex should not divert the control flow in data-dependent ways in case of unsolicited data grants.

Steps to reproduce the issue

I created a small sample setup that shows the bug in the latest code version (and also with the fix for #2144). https://github.com/KatCe/ibex/blob/bug_cf_hijack_load_branch/dv/bug_cf_hijack_load_branch/tb_top.sv

My Environment

EDA tool and version: Modelsim

Operating system: Ubuntu 20.04.6 LTS

Version of the Ibex source code: eea2bf0c1c62bbd676edf69cc60a56041d53b669 SecureIbex (configuration see testbench)

GregAC commented 5 months ago

Thank you for the report @KatCe

I think this boils down to an issue which occurs when Ibex is configured with the writeback stage (WritebackStage == 1) but without the branch target ALU (BranchTargetALU == 0). The logic around a multicycle branch (where the ALU first computes the branch result and then computes the branch target) does not correctly handle a stall caused by an outstanding load in the writeback stage. So the logic that handles the pc set believes the branch target to be computed and available where in fact the pipeline is still stalled in the first cycle of the instruction (which uses the ALU to calculate the branch result) so it gets the wrong result.

I believe the spurious data grant is a red herring here, it's just related to the stall induced by waiting for data_rvalid.

Please note that the configuration space of Ibex is large and we do not aim to support/verify it all. A configuration with the writeback stage but without the branch target ALU is not one we have verified so it is not hugely surprising to see issues like this. You can find a list of supported configurations in the file here: https://github.com/lowRISC/ibex/blob/master/ibex_configs.yaml

You've also got the branch predictor enabled, that's marked as an experimental feature and may well have various issues, thouygh I don't think it's effected things in this case.

I'm not sure we'd ever seek to support BranchTargetALU == 0 and WritebackStage == 1 in a supported configuration, certainly this won't be a priority to fix.

So for now I'll leave this issue open but rename it to make it clearer what the problem is.