From 6d7e5dd10741acda2fe7b8ab16cb48a4d4fe97fa Mon Sep 17 00:00:00 2001 From: Andreas Kurth Date: Tue, 30 Aug 2022 11:14:50 +0000 Subject: [PATCH] [otbn,rtl] Fix `BAD_DATA_ADDR` unknown on pop from empty call stack Prior to this commit, when a load or store used the call stack as address and the call stack was empty, that may or may not result in a `BAD_DATA_ADDR` error depending on the residual value in the empty call stack. However, a value popped from an empty stack is by definition undefined and can thus not be decided as bad data address. Furthermore, this was problematic in most RTL simulators, where an empty call stack that has never been pushed contains an unknown (X) value, so the `BAD_DATA_ADDR` error bit became unknown. This is described in more detail in #13641. These problems can be reproduced with the `bnlid-1.s` test program, which is run as part of the `otbn_multi_err` test. This commit fixes these problems by explicitly *not* setting the `BAD_DATA_ADDR` error bit when a load or store uses the call stack as address and the call stack is empty. It therefore fixes #13641. Signed-off-by: Andreas Kurth --- hw/ip/otbn/rtl/otbn_controller.sv | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/hw/ip/otbn/rtl/otbn_controller.sv b/hw/ip/otbn/rtl/otbn_controller.sv index 046bd0ebe7f15..9e11597362355 100644 --- a/hw/ip/otbn/rtl/otbn_controller.sv +++ b/hw/ip/otbn/rtl/otbn_controller.sv @@ -202,6 +202,7 @@ module otbn_controller logic jump_or_branch; logic branch_taken; logic insn_executing; + logic ld_insn_with_addr_from_call_stack, st_insn_with_addr_from_call_stack; logic [ImemAddrWidth-1:0] branch_target; logic branch_target_overflow; logic [ImemAddrWidth:0] next_insn_addr_wide; @@ -555,9 +556,31 @@ module otbn_controller assign reg_intg_violation_err = rf_bignum_intg_err | ispr_rdata_intg_err; assign key_invalid_err = ispr_rd_bignum_insn & insn_valid_i & key_invalid; assign illegal_insn_err = illegal_insn_static | rf_indirect_err; - assign bad_data_addr_err = dmem_addr_err; assign call_stack_sw_err = rf_base_call_stack_sw_err_i; + // Flag a bad data address error if the data memory address is invalid and it does not come from + // an empty call stack. The second case cannot be decided as bad data address because the address + // on top of the empty call stack may or may not be valid. (Also, in most RTL simulators an empty + // call stack that has never been pushed contains an unknown value, so this error bit would become + // unknown.) Thus, a data memory address coming from an empty call stack raises a call stack + // error but never a bad data address error. + assign bad_data_addr_err = dmem_addr_err & + ~(call_stack_sw_err & + (ld_insn_with_addr_from_call_stack | + st_insn_with_addr_from_call_stack)); + + // Identify load instructions that take the memory address from the call stack. + assign ld_insn_with_addr_from_call_stack = insn_valid_i & + insn_dec_shared_i.ld_insn & + insn_dec_base_i.rf_ren_a & + (insn_dec_base_i.a == 5'd1); + + // Identify store instructions that take the memory address from the call stack. + assign st_insn_with_addr_from_call_stack = insn_valid_i & + insn_dec_shared_i.st_insn & + insn_dec_base_i.rf_ren_a & + (insn_dec_base_i.a == 5'd1); + // All software errors that aren't bad_insn_addr. Factored into bad_insn_addr so it is only raised // if other software errors haven't ocurred. As bad_insn_addr relates to the next instruction // begin fetched it cannot occur if the current instruction has seen an error and failed to