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

Store instructions do not always raise RV32E exceptions #2178

Open
mndstrmr opened this issue Jun 26, 2024 · 3 comments
Open

Store instructions do not always raise RV32E exceptions #2178

mndstrmr opened this issue Jun 26, 2024 · 3 comments
Assignees
Labels

Comments

@mndstrmr
Copy link

Store instructions will be decoded with alu_op_b_mux_sel_o = OP_B_IMM:

ibex/rtl/ibex_decoder.sv

Lines 778 to 788 in 1449ed5

OPCODE_STORE: begin
alu_op_a_mux_sel_o = OP_A_REG_A;
alu_op_b_mux_sel_o = OP_B_REG_B;
alu_operator_o = ALU_ADD;
if (!instr_alu[14]) begin
// offset from immediate
imm_b_mux_sel_o = IMM_B_S;
alu_op_b_mux_sel_o = OP_B_IMM;
end
end

When RV32E = 1 an illegal instruction exception should be raised if any of the registers used by a given instruction are outside of the 0-15 range. In the case of the store instruction this should apply to both the storage address and the stored value.

The existing logic in ibex will not enforce this since alu_op_b_mux_sel_o = OP_B_IMM:

ibex/rtl/ibex_decoder.sv

Lines 179 to 183 in 1449ed5

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)) |
(rf_waddr_o[4] & rf_we));
end else begin : gen_rv32e_reg_check_inactive

Therefore in RV32E mode a store instruction may attempt to store a value from registers 16-31, which is not well defined since the register file will not have those entries.

B-TYPE instructions, which have the same format, are likely unaffected since in their first cycle they do set alu_op_{a,b}_mux_sel_o = OP_{A,B}_REG_{A,B}.

@rswarbrick
Copy link
Contributor

Can you give me an example of an instruction that triggers this? I think the code that you quoted in the second snippet is trying to say "Don't read from a high register on A or B side, and don't write to a high register".

Is the problem that the RV32E spec doesn't allow immediate stores when the value is big? If so, I didn't know that was part of the spec. Can you give a reference?

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jun 27, 2024

The issue here is that for store instruction, both rs1 and rs2 are used. So if, say, we have sd x31, 0(x10), then it should be illegal instruction.

But since decoder sets alu_op_b_mux_sel_o to OP_B_IMM, the illegal_reg_rv32e check will pass because it doesn't check if rs2 is used, merely if the ALU uses rs2.

@rswarbrick
Copy link
Contributor

Oh, I see. Thanks for the explanation.

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

No branches or pull requests

4 participants