[otbn] Teach ISS to generate errors on call stack under/overflow This is a reasonably large patch because the ISS didn't do any handling of architectural errors before. We now define a special exception type (Alert) in the Python and raise it if something goes wrong. If that happens, it gets caught in OTBNSim.step(), which tells the state to throw away any changes that were in flight and then to come to a screeching halt. The commit also comes with two simple ISS tests, to check that we stop properly on underflow or overflow. Since the RTL doesn't support this yet, we'll get mismatches for code that triggers the error (but hopefully we don't actually have any such code in our testing at the moment). Finally, we add the new enum item to the DIF. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
diff --git a/hw/ip/otbn/dv/otbnsim/sim/alert.py b/hw/ip/otbn/dv/otbnsim/sim/alert.py new file mode 100644 index 0000000..39b2284 --- /dev/null +++ b/hw/ip/otbn/dv/otbnsim/sim/alert.py
@@ -0,0 +1,26 @@ +# Copyright lowRISC contributors. +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +from typing import Optional + +# A copy of the list of error codes. This also appears in otbn.hjson and +# otbn_pkg.sv: we should probably be generating them from the hjson every time. +ERR_CODE_NO_ERROR = 0 +ERR_CODE_BAD_DATA_ADDR = 1 +ERR_CODE_CALL_STACK = 2 + + +class Alert(Exception): + '''An exception raised to signal that the program did something wrong + + This maps onto alerts in the implementation. The err_code value is the + value that should be written to the ERR_CODE external register. + + ''' + # Subclasses should override this class field + err_code = None # type: Optional[int] + + def error_code(self) -> int: + assert self.err_code is not None + return self.err_code
diff --git a/hw/ip/otbn/dv/otbnsim/sim/dmem.py b/hw/ip/otbn/dv/otbnsim/sim/dmem.py index 14313ac..eac233d 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/dmem.py +++ b/hw/ip/otbn/dv/otbnsim/sim/dmem.py
@@ -203,3 +203,6 @@ for item in self.trace: self._commit_store(item) self.trace = [] + + def abort(self) -> None: + self.trace = []
diff --git a/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py b/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py index 7125616..c80fd02 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py +++ b/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py
@@ -94,6 +94,9 @@ def commit(self) -> None: self.value = self.next_value + def abort(self) -> None: + self.next_value = self.value + class RGReg: '''A wrapper around a register as parsed by reggen''' @@ -143,6 +146,10 @@ for field in self.fields: field.commit() + def abort(self) -> None: + for field in self.fields: + field.abort() + class OTBNExtRegs: def __init__(self) -> None: @@ -211,3 +218,8 @@ for reg in self.regs.values(): reg.commit() self.trace = [] + + def abort(self) -> None: + for reg in self.regs.values(): + reg.abort() + self.trace = []
diff --git a/hw/ip/otbn/dv/otbnsim/sim/flags.py b/hw/ip/otbn/dv/otbnsim/sim/flags.py index 2c083fa..e07dd33 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/flags.py +++ b/hw/ip/otbn/dv/otbnsim/sim/flags.py
@@ -52,6 +52,9 @@ setattr(self, n, getattr(self._new_val, n)) self._new_val = None + def abort(self) -> None: + self._new_val = None + def read_unsigned(self) -> int: '''Return a 4-bit number with the flags as ZLMC''' return ((int(self.Z) << 3) | @@ -112,6 +115,12 @@ self._groups[1].commit() self._dirty = False + def abort(self) -> None: + if self._dirty: + self._groups[0].abort() + self._groups[1].abort() + self._dirty = False + def read_unsigned(self) -> int: '''Return the flag groups as an unsigned value (as seen by CSRs)
diff --git a/hw/ip/otbn/dv/otbnsim/sim/gpr.py b/hw/ip/otbn/dv/otbnsim/sim/gpr.py index 6eaf004..61cb90b 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/gpr.py +++ b/hw/ip/otbn/dv/otbnsim/sim/gpr.py
@@ -4,68 +4,98 @@ from typing import List +from .alert import Alert, ERR_CODE_CALL_STACK from .reg import Reg, RegFile +class CallStackError(Alert): + '''Raised when under- or over-flowing the call stack''' + + err_code = ERR_CODE_CALL_STACK + + def __init__(self, is_overflow: bool): + self.is_overflow = is_overflow + + def __str__(self) -> str: + xflow = 'overflow' if self.is_overflow else 'underflow' + return 'Instruction caused {} of x1 call stack.'.format(xflow) + + +class CallStackReg(Reg): + '''A register used to represent x1''' + + # The depth of the x1 call stack + stack_depth = 8 + + def __init__(self, parent: RegFile): + super().__init__(parent, 1, 32, 0) + self.stack = [] # type: List[int] + self.saw_read = False + + # We overload read_unsigned here, to handle the read-sensitive behaviour + # without needing the base class to deal with it. + def read_unsigned(self, backdoor: bool = False) -> int: + if backdoor: + # If a backdoor access, don't take note of the read. If the stack + # turns out to be empty, return some "obviously bogus" value. + return self.stack[-1] if self.stack else 0xcafef00d + + if not self.stack: + raise CallStackError(False) + + # Mark that we've read something (so that we pop from the stack as part + # of commit) and return the top of the stack. + self.saw_read = True + return self.stack[-1] + + def write_unsigned(self, uval: int, backdoor: bool = False) -> None: + super().write_unsigned(uval, backdoor) + + def commit(self) -> None: + if self.saw_read: + assert self.stack + self.stack.pop() + self.saw_read = False + + if self._next_uval is not None: + if len(self.stack) == 8: + raise CallStackError(True) + self.stack.append(self._next_uval) + + super().commit() + + def abort(self) -> None: + self.saw_read = False + super().abort() + + class GPRs(RegFile): '''The narrow OTBN register file''' + def __init__(self) -> None: super().__init__('x', 32, 32) - - # The call stack for x1 and its pending updates. self.callstack is - # never empty and (after commit) the tail always matches - # self._registers[1].read_unsigned() (pushing from the right) - self._callstack = [0] # type: List[int] - self._callstack_pop = False - - def mark_read(self, idx: int) -> None: - super().mark_read(idx) - if idx == 1: - self._callstack_pop = True + self._x1 = CallStackReg(self) def get_reg(self, idx: int) -> Reg: - # If idx == 0, this is a zeros register that should ignore writes. - # Return a fresh Reg with no parent, so writes to it have no effect. if idx == 0: + # If idx == 0, this is a zeros register that should ignore writes. + # Return a fresh Reg with no parent, so writes to it have no effect. return Reg(None, 0, 32, 0) - return super().get_reg(idx) - - def post_insn(self) -> None: - '''Update call stack after instruction execution but before commit - - This is not idempotent: call it exactly once. - - ''' - callstack_push = 1 in self._pending_writes - - # If we read from the call stack, we have popped from it. (Even if the - # stack is empty, we treat this as a logical pop. We need to decide - # what happens here: see issue #3239). - if self._callstack_pop: - if len(self._callstack) > 1: - self._callstack.pop() - - # Write the new head of the call stack to the relevant register, - # unless x1 has a pending write (which means that we had an - # instruction like "addi x1, x1, 0", so the pop happened logically - # before the push) - if not callstack_push: - self._registers[1].write_unsigned(self._callstack[-1]) - - # If there is a pending write to x1 (which wasn't caused by the - # callstack pop code just above) then callstack_push will be true. In - # that case, we should push the new value onto the call stack. - if callstack_push: - new_x1 = self._registers[1].read_next() - assert new_x1 is not None - self._callstack.append(new_x1) + elif idx == 1: + # If idx == 1, we return self._x1: element 1 of the underlying + # register file is not actually used. + return self._x1 + else: + return super().get_reg(idx) def peek_call_stack(self) -> List[int]: - '''Get a list of values on the call stack''' - # _callstack is never empty and always contains 0 as the first element, - # so strip that off the returned value. - return self._callstack[1:] + '''Get the call stack, bottom-first.''' + return self._x1.stack def commit(self) -> None: - self._callstack_pop = False + self._x1.commit() super().commit() + + def abort(self) -> None: + self._x1.abort() + super().abort()
diff --git a/hw/ip/otbn/dv/otbnsim/sim/insn.py b/hw/ip/otbn/dv/otbnsim/sim/insn.py index 5f4ad37..c36791d 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/insn.py +++ b/hw/ip/otbn/dv/otbnsim/sim/insn.py
@@ -308,15 +308,8 @@ pass def execute(self, state: OTBNState) -> None: - # INTR_STATE is the interrupt state register. Bit 0 (which is being - # set) is the 'done' flag. - state.ext_regs.set_bits('INTR_STATE', 1 << 0) - # STATUS is a status register. Bit 0 (being cleared) is the 'busy' flag - state.ext_regs.clear_bits('STATUS', 1 << 0) - - # As well as the external register, clear an internal 'running' flag to - # tell the simulation to stop. - state.running = False + # Set INTR_STATE.done and STATUS, reflecting the fact we've stopped. + state.stop(None) class LOOP(OTBNInsn):
diff --git a/hw/ip/otbn/dv/otbnsim/sim/reg.py b/hw/ip/otbn/dv/otbnsim/sim/reg.py index d8b8ecb..3f42745 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/reg.py +++ b/hw/ip/otbn/dv/otbnsim/sim/reg.py
@@ -35,8 +35,6 @@ self._next_uval = None # type: Optional[int] def read_unsigned(self, backdoor: bool = False) -> int: - if not backdoor and self._parent is not None: - self._parent.mark_read(self._idx) return self._uval def write_unsigned(self, uval: int, backdoor: bool = False) -> None: @@ -66,6 +64,9 @@ self._uval = self._next_uval self._next_uval = None + def abort(self) -> None: + self._next_uval = None + class RegFile: '''A base class for register files (used for both GPRs and WDRs). @@ -86,12 +87,6 @@ self._registers = [Reg(self, i, width, 0) for i in range(depth)] self._pending_writes = set() # type: Set[int] - def mark_read(self, idx: int) -> None: - '''Mark a register as having been read''' - # The default implementation ignores this, but the GPRs subclass - # overrides it to deal with reads from x1. - pass - def mark_written(self, idx: int) -> None: '''Mark a register as having been written''' assert 0 <= idx < len(self._registers) @@ -105,7 +100,7 @@ ret = [] for idx in sorted(self._pending_writes): assert 0 <= idx < len(self._registers) - next_val = self._registers[idx].read_next() + next_val = self.get_reg(idx).read_next() assert next_val is not None ret.append(TraceRegister(self._name_pfx + str(idx), self._width, @@ -118,6 +113,12 @@ self._registers[idx].commit() self._pending_writes.clear() + def abort(self) -> None: + for idx in self._pending_writes: + assert 0 <= idx < len(self._registers) + self._registers[idx].abort() + self._pending_writes.clear() + def peek_unsigned_values(self) -> List[int]: '''Get a list of the (unsigned) values of the registers''' return [reg.read_unsigned(backdoor=True) for reg in self._registers]
diff --git a/hw/ip/otbn/dv/otbnsim/sim/sim.py b/hw/ip/otbn/dv/otbnsim/sim/sim.py index c7c2150..7772a8c 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/sim.py +++ b/hw/ip/otbn/dv/otbnsim/sim/sim.py
@@ -4,6 +4,7 @@ from typing import List, Optional, Tuple +from .alert import Alert from .isa import OTBNInsn from .state import OTBNState @@ -46,32 +47,46 @@ was_stalled = self.state.stalled pc_before = self.state.pc - if was_stalled: - insn = None - changes = [] - else: - word_pc = int(self.state.pc) >> 2 - if word_pc >= len(self.program): - raise RuntimeError('Trying to execute instruction at address ' - '{:#x}, but the program is only {:#x} ' - 'bytes ({} instructions) long. Since there ' - 'are no architectural contents of the ' - 'memory here, we have to stop.' - .format(int(self.state.pc), - 4 * len(self.program), - len(self.program))) - insn = self.program[word_pc] + try: + if was_stalled: + insn = None + changes = [] + else: + word_pc = int(self.state.pc) >> 2 + if word_pc >= len(self.program): + raise RuntimeError('Trying to execute instruction at address ' + '{:#x}, but the program is only {:#x} ' + 'bytes ({} instructions) long. Since there ' + 'are no architectural contents of the ' + 'memory here, we have to stop.' + .format(int(self.state.pc), + 4 * len(self.program), + len(self.program))) + insn = self.program[word_pc] - if insn.insn.cycles > 1: - self.state.add_stall_cycles(insn.insn.cycles - 1) + if insn.insn.cycles > 1: + self.state.add_stall_cycles(insn.insn.cycles - 1) - insn.execute(self.state) - self.state.post_insn() + insn.execute(self.state) + self.state.post_insn() + changes = self.state.changes() + + self.state.commit() + + except Alert as alert: + # Roll back any pending state changes: we ensure that a faulting + # instruction doesn't actually do anything. + self.state.abort() + + # We've rolled back any changes, but need to actually generate an + # "alert". To do that, we tell the state to set an appropriate + # error code in the external ERR_CODE register and clear the busy + # flag. These register changes get reflected in the returned list + # of trace items, that we propagate up. + self.state.stop(alert.error_code()) changes = self.state.changes() - self.state.commit() - if verbose: disasm = ('(stall)' if insn is None else insn.disassemble(pc_before))
diff --git a/hw/ip/otbn/dv/otbnsim/sim/state.py b/hw/ip/otbn/dv/otbnsim/sim/state.py index 3bed104..c49dddf 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/state.py +++ b/hw/ip/otbn/dv/otbnsim/sim/state.py
@@ -7,6 +7,7 @@ from riscvmodel.types import Trace, TracePC # type: ignore +from .alert import Alert from .csr import CSRFile from .dmem import Dmem from .ext_regs import OTBNExtRegs @@ -118,6 +119,9 @@ def commit(self) -> None: self.trace = [] + def abort(self) -> None: + self.trace = [] + class OTBNState: def __init__(self) -> None: @@ -144,7 +148,7 @@ self.running = False def add_stall_cycles(self, num_cycles: int) -> None: - '''Add a single stall cycle before the next insn completes''' + '''Add stall cycles before the next insn completes''' assert num_cycles >= 0 self._stalls += num_cycles @@ -196,6 +200,22 @@ self.csrs.flags.commit() self.wdrs.commit() + def abort(self) -> None: + '''Abort any pending state changes''' + # This should only be called when an instruction's execution goes + # wrong. If self._stalls is positive, the bad execution caused those + # stalls, so we should just zero them. + self._stalls = 0 + + self.gprs.abort() + self.pc_next = None + self.dmem.abort() + self.loop_stack.abort() + self.ext_regs.abort() + self.wsrs.abort() + self.csrs.flags.abort() + self.wdrs.abort() + def start(self) -> None: '''Set the running flag and the ext_reg busy flag''' self.ext_regs.set_bits('STATUS', 1 << 0) @@ -288,7 +308,6 @@ def post_insn(self) -> None: '''Update state after running an instruction but before commit''' self.loop_step() - self.gprs.post_insn() def read_csr(self, idx: int) -> int: '''Read the CSR with index idx as an unsigned 32-bit number''' @@ -297,3 +316,25 @@ def write_csr(self, idx: int, value: int) -> None: '''Write value (an unsigned 32-bit number) to the CSR with index idx''' self.csrs.write_unsigned(self.wsrs, idx, value) + + def peek_call_stack(self) -> List[int]: + '''Return the current call stack, bottom-first''' + return self.gprs.peek_call_stack() + + def stop(self, err_code: Optional[int]) -> None: + '''Set flags to stop the processor. + + If err_code is not None, it is the value to write to the ERR_CODE + register. + + ''' + # INTR_STATE is the interrupt state register. Bit 0 (which is being + # set) is the 'done' flag. + self.ext_regs.set_bits('INTR_STATE', 1 << 0) + # STATUS is a status register. Bit 0 (being cleared) is the 'busy' flag + self.ext_regs.clear_bits('STATUS', 1 << 0) + + if err_code is not None: + self.ext_regs.write('ERR_CODE', err_code, True) + + self.running = False
diff --git a/hw/ip/otbn/dv/otbnsim/sim/wsr.py b/hw/ip/otbn/dv/otbnsim/sim/wsr.py index 36495d1..22a12ec 100644 --- a/hw/ip/otbn/dv/otbnsim/sim/wsr.py +++ b/hw/ip/otbn/dv/otbnsim/sim/wsr.py
@@ -44,6 +44,10 @@ '''Commit pending changes''' return + def abort(self) -> None: + '''Abort pending changes''' + return + def changes(self) -> List[TraceWSR]: '''Return list of pending architectural changes''' return [] @@ -68,6 +72,9 @@ self._value = self._next_value self._next_value = None + def abort(self) -> None: + self._next_value = None + def changes(self) -> List[TraceWSR]: return ([TraceWSR(self.name, self._next_value)] if self._next_value is not None @@ -131,5 +138,10 @@ self.RND.commit() self.ACC.commit() + def abort(self) -> None: + self.MOD.abort() + self.RND.abort() + self.ACC.abort() + def changes(self) -> List[TraceWSR]: return self.MOD.changes() + self.RND.changes() + self.ACC.changes()
diff --git a/hw/ip/otbn/dv/otbnsim/stepped.py b/hw/ip/otbn/dv/otbnsim/stepped.py index 6a68142..60dd41e 100755 --- a/hw/ip/otbn/dv/otbnsim/stepped.py +++ b/hw/ip/otbn/dv/otbnsim/stepped.py
@@ -168,13 +168,13 @@ def on_print_call_stack(sim: OTBNSim, args: List[str]) -> None: - '''Print call stack to stdout first element is the bottom of the stack''' + '''Print call stack to stdout. First element is the bottom of the stack''' if len(args): raise ValueError('print_call_stack expects zero arguments. Got {}.' .format(args)) print('PRINT_CALL_STACK') - for value in sim.state.gprs.peek_call_stack(): + for value in sim.state.peek_call_stack(): print('0x{:08x}'.format(value))
diff --git a/hw/ip/otbn/dv/otbnsim/test/simple/x1-overflow/expected.txt b/hw/ip/otbn/dv/otbnsim/test/simple/x1-overflow/expected.txt new file mode 100644 index 0000000..7e55767 --- /dev/null +++ b/hw/ip/otbn/dv/otbnsim/test/simple/x1-overflow/expected.txt
@@ -0,0 +1,6 @@ +# Copyright lowRISC contributors. +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +x2 = 8 +x10 = 10
diff --git a/hw/ip/otbn/dv/otbnsim/test/simple/x1-overflow/overflow.s b/hw/ip/otbn/dv/otbnsim/test/simple/x1-overflow/overflow.s new file mode 100644 index 0000000..d829ca4 --- /dev/null +++ b/hw/ip/otbn/dv/otbnsim/test/simple/x1-overflow/overflow.s
@@ -0,0 +1,20 @@ +/* Copyright lowRISC contributors. */ +/* Licensed under the Apache License, Version 2.0, see LICENSE for details. */ +/* SPDX-License-Identifier: Apache-2.0 */ + +/* + An example of overflowing the x1 stack. Make sure that we stop execution and + properly raise an alert. To avoid lots of copy-paste code, we just loop 100 + times, incrementing a variable as we go. This variable will contain the call + stack depth when we stop. +*/ + addi x10, x0, 10 + addi x2, x0, 0 + + loopi 100, 2 + addi x1, x0, 0 + addi x2, x2, 1 + + /* We shouldn't get here */ + addi x10, x0, 0 + ecall
diff --git a/hw/ip/otbn/dv/otbnsim/test/simple/x1-underflow/expected.txt b/hw/ip/otbn/dv/otbnsim/test/simple/x1-underflow/expected.txt new file mode 100644 index 0000000..50e32ef --- /dev/null +++ b/hw/ip/otbn/dv/otbnsim/test/simple/x1-underflow/expected.txt
@@ -0,0 +1,7 @@ +# Copyright lowRISC contributors. +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +x2 = 123 +x3 = 246 +x10 = 10
diff --git a/hw/ip/otbn/dv/otbnsim/test/simple/x1-underflow/underflow.s b/hw/ip/otbn/dv/otbnsim/test/simple/x1-underflow/underflow.s new file mode 100644 index 0000000..d4cc2fb --- /dev/null +++ b/hw/ip/otbn/dv/otbnsim/test/simple/x1-underflow/underflow.s
@@ -0,0 +1,26 @@ +/* Copyright lowRISC contributors. */ +/* Licensed under the Apache License, Version 2.0, see LICENSE for details. */ +/* SPDX-License-Identifier: Apache-2.0 */ + +/* + An example of underflowing the x1 stack. Make sure that we stop execution and + properly raise an alert. +*/ + addi x10, x0, 10 + + /* Push once onto the stack */ + addi x1, x0, 123 + /* Pop once from the stack: this should be fine */ + addi x2, x1, 0 + + /* Push once onto the stack */ + addi x1, x0, 123 + /* Pop once from the stack (but with both input operands) */ + add x3, x1, x1 + + /* Pop again from the stack. ERROR! */ + addi x0, x1, 0 + + /* We shouldn't get here */ + addi x10, x0, 0 + ecall