Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with multiple bit manipulation instructions #51

Open
itsomaia opened this issue Oct 2, 2024 · 6 comments
Open

Problem with multiple bit manipulation instructions #51

itsomaia opened this issue Oct 2, 2024 · 6 comments

Comments

@itsomaia
Copy link

itsomaia commented Oct 2, 2024

Observed Behavior

In the case of multiple bit manipulation instructions, such as BINVI, the result of the operation is computed incorrectly due to an incorrect operand. The operand value passed from the decode stage and the input of the ALU, differ from the value used in the operation. This issue arises because the core incorrectly assumes it is not in the first cycle for BINVI, even though it is, due to an illegal CHERI exception that occurred a few cycles earlier.

Please see the waveform for more details.

BINVI_NEW_ILL

Expected Behavior

Getting correct operands for the instruction.

Steps to reproduce the issue

Illegal_reg_cheri exception before the bit manipulation instructions.

My Environment

Running formalISA v 3.0 app with Cadence JasperGold 2023.09

EDA tool and version:

Running formalISA v 3.0 app with Cadence JasperGold 2023.09

Operating system:

Running formalISA v 3.0 app with Cadence JasperGold 2023.09

Version of the Ibex source code:

@GregAC
Copy link
Contributor

GregAC commented Oct 8, 2024

@itsomaia I'm a little confused as to how this occurred.

Going by what's on your waveform the issue is illegal_reg_cheri being raised the cycle after the FSL begins executing. This signal indicates the instruction is trying to use a register outside of the accessible 16 registers for RV32E. However this is a static property of the instruction and the instruction in the ID/EX stage shouldn't have changed between the FIRST_CYCLE -> MULTI_CYCLE transition.

The RTL that generates the illegal_reg_cheri signal is here:

if (CheriLimit16Regs) begin : gen_cheri_reg_check_active
assign illegal_reg_cheri = cheri_pmode_i &
((raddr_a[4] & rf_ren_a_o) |
(raddr_b[4] & rf_ren_b_o) |
(instr_rd[4] & rf_we_or_load ));
end else begin : gen_cheri_reg_check_inactive
assign illegal_reg_cheri = 1'b0;
end

Notably it involves the top level cheri_pmode_i signal. Were it to change during instruction execution the problem you identify would occur. However I think this should be seen as a static signal, or at least certainly can't be allowed to change whenever.

Have you constrained cheri_pmode_i at all? Could you explain how it's been constrained? I would suggest it's reasonable to constrain it to always be a fixed value (i.e. either you run your formal flow with CHERI enabled or disabled, you don't continuously flip between the two as the behaviour there is not specified).

If it is constrained can you confirm cheri_pmode_i remains a constant 1 in this run? In this case there's something happening with the instruction fetch and very first part of the ID/EX stage causing it alter an instruction when it should be stalled. A full wave dump rather than a screenshot with select signals would greatly aid in debugging this.

@itsomaia
Copy link
Author

Hello @GregAC,

Thank you for your response. I have already constrained cheri_pmode_i to 1. You can see in the attached .vcd file and the new waveform.

The raddr_a signal, which causes illegal_reg_cheri to go high, can change, as shown in the code and reflected in the waveform, depending on whether the instruction is in its first cycle or not.

logic [4:0] raddr_a, raddr_b;
assign raddr_a = cheri_auicgp_en ? 5'h3 : ((use_rs3_q & ~instr_first_cycle_i) ? instr_rs3 : instr_rs1); // rs3 / rs1

New Waveform:
Image

.vcd for this case:
BINVI_exception_new.vcd.zip

If you need more information, please let me know.

@kliuMsft
Copy link
Contributor

@itsomaia , this seems to have something to do with RV32B support configured (what parameter value are you using?). Basically there is a multi-cycle RV32B instruction involves rs3 in this case.

@kliuMsft
Copy link
Contributor

Looking further into the issue, the culprit seems to be that the id_fsm_d logic can't handle exception being issued in the 2nd half of a multi-cycle instruction. Specifically, the illegal_reg_cheri results in an EX stage exception but instr_kill is only raised in the 2nd half of a bit manipulation instruction (when rs3 is accessed). In this case multicycle_done is never issued and thus id_fsm_q will not updated properly.

@GregAC do you plan to keep supporting the bit instructions with rs3? if so I can try fix the behavior in cheriot-ibex. You may want to take a look at the upstream ibex implementation as well.

@GregAC
Copy link
Contributor

GregAC commented Oct 15, 2024

@kliuMsft the 3rd register operand is used by the non-ratified Zbt extension which we do support in Ibex but isn't part of the ratified v1.0 bitmanip spec (it comes from the earlier v0.93 spec). I think this issue will only occur in RV32E configurations (as in relies on an exception caused by trying to access a register index of 16 or above) so it wouldn't be a high priority fix for us but something we should sort out.

My preferred fix here would be to alter the way the register index out of range error is calculated. Currently it's based on the register addresses being output:

if (RV32E) begin : gen_rv32e_reg_check_active
//assign illegal_reg_rv32e = ((rf_raddr_a_o[4] & (alu_op_a_mux_sel_o == OP_A_REG_A)) |
// (rf_raddr_b_o[4] & (alu_op_b_mux_sel_o == OP_B_REG_B)) |
assign illegal_reg_rv32e = ((rf_raddr_a_o[4] & rf_ren_a_o) |
(rf_raddr_b_o[4] & rf_ren_b_o) |
(rf_waddr_o[4] & rf_we_or_load));
end else begin : gen_rv32e_reg_check_inactive
assign illegal_reg_rv32e = 1'b0;
end
if (CheriLimit16Regs) begin : gen_cheri_reg_check_active
assign illegal_reg_cheri = cheri_pmode_i &
((raddr_a[4] & rf_ren_a_o) |
(raddr_b[4] & rf_ren_b_o) |
(instr_rd[4] & rf_we_or_load ));
end else begin : gen_cheri_reg_check_inactive
assign illegal_reg_cheri = 1'b0;
end

I think that could be simply refactored to look at insn_rs1/insn_rs2/insn_rs3/insn_rd instead. Given this error is a static property of the instruction only raising it after 1 cycle seems overly complex.

This also means no refactoring of the state machine to deal with an instruction under going a multi-cycle execution taking an exception (ignoring this problem it should be the case that either an instruction takes an exception before beginning multi-cycle execution or once that has begun we're guaranteed that execution can complete without an exception, though its result may get discarded if the instruction ahead in the WB stage takes an exception).

@kliuMsft
Copy link
Contributor

Commit d7d4818 should fix the issue.
https://github.com/microsoft/cheriot-ibex/blob/d7d4818db943d22c9bcf568a510f6e2bfa8f6a27/rtl/ibex_decoder.sv#L238C3-L257C6

@GregAC please review.
@itsomaia please verify whether this indeed fix the issue in your setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants