[otbn] Simplify error tracking in the ISS
Rather than tracking proper errors with messages (which we didn't
actually have any way to look at anyway), set the ERR_BITS bits
explicitly. One advantage of doing this is that it lets us write
something more understandable in the ISA docs.
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
deleted file mode 100644
index d7b5287..0000000
--- a/hw/ip/otbn/dv/otbnsim/sim/alert.py
+++ /dev/null
@@ -1,88 +0,0 @@
-# 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 bits in the ERR_BITS register. This also appears in the
-# documentation and otbn_pkg.sv: we should probably be generating them from the
-# hjson every time.
-BAD_DATA_ADDR = 1 << 0
-BAD_INSN_ADDR = 1 << 1
-CALL_STACK = 1 << 2
-ILLEGAL_INSN = 1 << 3
-LOOP = 1 << 4
-FATAL_IMEM = 1 << 5
-FATAL_DMEM = 1 << 6
-FATAL_REG = 1 << 7
-
-
-class Alert:
- '''An object describing something the program did wrong
-
- This maps onto alerts in the implementation. The err_bit value is the
- value that should be OR'd into the ERR_BITS external register.
-
- '''
- # Subclasses should override this class field or the error_bit method
- err_bit = None # type: Optional[int]
-
- def error_bit(self) -> int:
- assert self.err_bit is not None
- return self.err_bit
-
-
-class BadAddrError(Alert):
- '''Generated when loading or storing or setting PC with a bad address'''
-
- def __init__(self, operation: str, addr: int, what: str):
- assert operation in ['pc',
- 'narrow load', 'narrow store',
- 'wide load', 'wide store']
- self.operation = operation
- self.addr = addr
- self.what = what
-
- def error_bit(self) -> int:
- return BAD_INSN_ADDR if self.operation == 'fetch' else BAD_DATA_ADDR
-
- def __str__(self) -> str:
- return ('Bad {} address of {:#08x}: {}.'
- .format(self.operation, self.addr, self.what))
-
-
-class LoopError(Alert):
- '''Generated when doing something wrong with a LOOP/LOOPI'''
-
- err_bit = LOOP
-
- def __init__(self, what: str):
- self.what = what
-
- def __str__(self) -> str:
- return 'Loop error: {}'.format(self.what)
-
-
-class IllegalInsnError(Alert):
- '''Generated on a bad instruction'''
- err_bit = ILLEGAL_INSN
-
- def __init__(self, word: int, msg: str):
- self.word = word
- self.msg = msg
-
- def __str__(self) -> str:
- return ('Illegal instruction {:#010x}: {}'.format(self.word, self.msg))
-
-
-class CallStackError(Alert):
- '''Raised when under- or over-flowing the call stack'''
-
- err_bit = 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)
diff --git a/hw/ip/otbn/dv/otbnsim/sim/decode.py b/hw/ip/otbn/dv/otbnsim/sim/decode.py
index 146ec46..04cadfc 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/decode.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/decode.py
@@ -7,7 +7,7 @@
import struct
from typing import List, Optional, Tuple, Type
-from .alert import IllegalInsnError
+from .err_bits import ILLEGAL_INSN
from .isa import DecodeError, OTBNInsn
from .insn import INSN_CLASSES
from .state import OTBNState
@@ -39,7 +39,7 @@
self._disasm = (pc, '?? 0x{:08x}'.format(raw))
def execute(self, state: OTBNState) -> None:
- state.on_error(IllegalInsnError(self.raw, self.msg))
+ state.stop_at_end_of_cycle(ILLEGAL_INSN)
MASK_TUPLES = None # type: Optional[List[_MaskTuple]]
diff --git a/hw/ip/otbn/dv/otbnsim/sim/dmem.py b/hw/ip/otbn/dv/otbnsim/sim/dmem.py
index f2becd2..e6157b8 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/dmem.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/dmem.py
@@ -7,7 +7,7 @@
from shared.mem_layout import get_memory_layout
-from .alert import BadAddrError
+from .err_bits import BAD_DATA_ADDR
from .trace import Trace
@@ -61,7 +61,7 @@
self.data = [uninit] * num_words
self.trace = [] # type: List[TraceDmemStore]
- self._errs = [] # type: List[BadAddrError]
+ self.err_flag = False
def _get_u32s(self, idx: int) -> List[int]:
'''Return the value at idx as 8 uint32's
@@ -145,15 +145,13 @@
assert addr >= 0
if addr & 31:
- self._errs.append(BadAddrError('wide load', addr,
- 'address is not 32-byte aligned'))
+ self.err_flag = True
return 0
word_addr = addr // 32
if word_addr >= len(self.data):
- self._errs.append(BadAddrError('wide load', addr,
- 'address is above the top of dmem'))
+ self.err_flag = True
return 0
return self.data[word_addr]
@@ -164,14 +162,12 @@
assert 0 <= value < (1 << 256)
if addr & 31:
- self._errs.append(BadAddrError('wide store', addr,
- 'address is not 32-byte aligned'))
+ self.err_flag = True
return
word_addr = addr // 32
if word_addr >= len(self.data):
- self._errs.append(BadAddrError('wide store', addr,
- 'address is above the top of dmem'))
+ self.err_flag = True
return
self.trace.append(TraceDmemStore(addr, value, True))
@@ -185,12 +181,11 @@
'''
assert addr >= 0
if addr & 3:
- self._errs.append(BadAddrError('narrow load', addr,
- 'address is not 4-byte aligned'))
+ self.err_flag = True
return 0
+
if (addr + 3) // 32 >= len(self.data):
- self._errs.append(BadAddrError('narrow load', addr,
- 'address is above the top of dmem'))
+ self.err_flag = True
return 0
idx32 = addr // 4
@@ -209,18 +204,17 @@
assert 0 <= value <= (1 << 32) - 1
if addr & 3:
- self._errs.append(BadAddrError('narrow load', addr,
- 'address is not 4-byte aligned'))
+ self.err_flag = True
return
+
if (addr + 3) // 32 >= len(self.data):
- self._errs.append(BadAddrError('narrow load', addr,
- 'address is above the top of dmem'))
+ self.err_flag = True
return
self.trace.append(TraceDmemStore(addr, value, False))
- def errors(self) -> List[BadAddrError]:
- return self._errs
+ def err_bits(self) -> int:
+ return BAD_DATA_ADDR if self.err_flag else 0
def changes(self) -> Sequence[Trace]:
return self.trace
@@ -247,11 +241,11 @@
self._set_u32s(idxW, u32s)
def commit(self) -> None:
- assert not self._errs
+ assert not self.err_flag
for item in self.trace:
self._commit_store(item)
self.trace = []
def abort(self) -> None:
self.trace = []
- self._errs = []
+ self.err_flag = False
diff --git a/hw/ip/otbn/dv/otbnsim/sim/err_bits.py b/hw/ip/otbn/dv/otbnsim/sim/err_bits.py
new file mode 100644
index 0000000..44b9020
--- /dev/null
+++ b/hw/ip/otbn/dv/otbnsim/sim/err_bits.py
@@ -0,0 +1,15 @@
+# Copyright lowRISC contributors.
+# Licensed under the Apache License, Version 2.0, see LICENSE for details.
+# SPDX-License-Identifier: Apache-2.0
+
+# A copy of the list of bits in the ERR_BITS register. This also appears in the
+# documentation and otbn_pkg.sv: we should probably be generating them from the
+# hjson every time.
+BAD_DATA_ADDR = 1 << 0
+BAD_INSN_ADDR = 1 << 1
+CALL_STACK = 1 << 2
+ILLEGAL_INSN = 1 << 3
+LOOP = 1 << 4
+FATAL_IMEM = 1 << 5
+FATAL_DMEM = 1 << 6
+FATAL_REG = 1 << 7
diff --git a/hw/ip/otbn/dv/otbnsim/sim/gpr.py b/hw/ip/otbn/dv/otbnsim/sim/gpr.py
index ea64eb8..9d7898d 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/gpr.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/gpr.py
@@ -4,7 +4,7 @@
from typing import List
-from .alert import CallStackError
+from .err_bits import CALL_STACK
from .reg import Reg, RegFile
@@ -29,7 +29,7 @@
return self.stack[-1] if self.stack else 0xcafef00d
if not self.stack:
- self.gpr_parent.errs.append(CallStackError(False))
+ self.gpr_parent.err_flag = True
return 0
# Mark that we've read something (so that we pop from the stack as part
@@ -40,7 +40,7 @@
def post_insn(self) -> None:
if self._next_uval is not None:
if not self.saw_read and len(self.stack) == 8:
- self.gpr_parent.errs.append(CallStackError(True))
+ self.gpr_parent.err_flag = True
def commit(self) -> None:
if self.saw_read:
@@ -67,7 +67,7 @@
def __init__(self) -> None:
super().__init__('x', 32, 32)
self._x1 = CallStackReg(self)
- self.errs = [] # type: List[CallStackError]
+ self.err_flag = False
def get_reg(self, idx: int) -> Reg:
if idx == 0:
@@ -88,15 +88,15 @@
def post_insn(self) -> None:
return self._x1.post_insn()
- def errors(self) -> List[CallStackError]:
- return self.errs
+ def err_bits(self) -> int:
+ return CALL_STACK if self.err_flag else 0
def commit(self) -> None:
super().commit()
- assert not self.errs
+ assert not self.err_flag
self._x1.commit()
def abort(self) -> None:
super().abort()
self._x1.abort()
- self.errs = []
+ self.err_flag = False
diff --git a/hw/ip/otbn/dv/otbnsim/sim/insn.py b/hw/ip/otbn/dv/otbnsim/sim/insn.py
index 3828a7a..c7b1566 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/insn.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/insn.py
@@ -4,7 +4,7 @@
from typing import Dict
-from .alert import LoopError
+from sim import err_bits
from .flags import FlagReg
from .isa import (DecodeError, OTBNInsn, RV32RegReg, RV32RegImm, RV32ImmShift,
insn_for_mnemonic, logical_byte_shift)
@@ -309,13 +309,9 @@
class ECALL(OTBNInsn):
insn = insn_for_mnemonic('ecall', 0)
- def __init__(self, raw: int, op_vals: Dict[str, int]):
- super().__init__(raw, op_vals)
- pass
-
def execute(self, state: OTBNState) -> None:
# Set INTR_STATE.done and STATUS, reflecting the fact we've stopped.
- state._stop(0)
+ state.stop_at_end_of_cycle(err_bits=0)
class LOOP(OTBNInsn):
@@ -330,8 +326,7 @@
def execute(self, state: OTBNState) -> None:
num_iters = state.gprs.get_reg(self.grs).read_unsigned()
if num_iters == 0:
- state.on_error(LoopError('loop count in x{} was zero'
- .format(self.grs)))
+ state.stop_at_end_of_cycle(err_bits.LOOP)
return
state.loop_start(num_iters, self.bodysize)
diff --git a/hw/ip/otbn/dv/otbnsim/sim/loop.py b/hw/ip/otbn/dv/otbnsim/sim/loop.py
index 4e8e564..d36d367 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/loop.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/loop.py
@@ -4,7 +4,7 @@
from typing import List, Optional
-from .alert import LoopError
+from .err_bits import LOOP
from .trace import Trace
@@ -59,7 +59,7 @@
def __init__(self) -> None:
self.stack = [] # type: List[LoopLevel]
self.trace = [] # type: List[Trace]
- self.errs = [] # type: List[LoopError]
+ self.err_flag = False
def start_loop(self,
next_addr: int,
@@ -78,7 +78,7 @@
assert 0 < loop_count
if len(self.stack) == LoopStack.stack_depth:
- self.errs.append(LoopError('loop stack overflow'))
+ self.err_flag = True
self.trace.append(TraceLoopStart(loop_count, insn_count))
self.stack.append(LoopLevel(next_addr, insn_count, loop_count - 1))
@@ -92,8 +92,7 @@
# Make sure that it isn't a jump, branch or another loop
# instruction.
if insn_affects_control:
- self.errs.append(LoopError('control instruction '
- 'at end of loop'))
+ self.err_flag = True
def step(self, next_pc: int) -> Optional[int]:
'''Update loop stack. If we should loop, return new PC'''
@@ -118,16 +117,16 @@
return None
- def errors(self) -> List[LoopError]:
- return self.errs
+ def err_bits(self) -> int:
+ return LOOP if self.err_flag else 0
def changes(self) -> List[Trace]:
return self.trace
def commit(self) -> None:
- assert not self.errs
+ assert not self.err_flag
self.trace = []
def abort(self) -> None:
self.trace = []
- self.errs = []
+ self.err_flag = False
diff --git a/hw/ip/otbn/dv/otbnsim/sim/sim.py b/hw/ip/otbn/dv/otbnsim/sim/sim.py
index 05df7f1..9153741 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/sim.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/sim.py
@@ -4,7 +4,6 @@
from typing import List, Optional, Tuple
-from .alert import Alert
from .isa import OTBNInsn
from .state import OTBNState
from .trace import Trace
@@ -72,13 +71,14 @@
insn.execute(self.state)
self.state.post_insn()
- errors = self.state.errors()
- if errors:
+ if self.state.pending_halt:
# Roll back any pending state changes, ensuring that a faulting
- # instruction doesn't actually do anything. Also generate a
- # change that sets an appropriate error bits in the external
- # ERR_CODE register and clears the busy flag.
- self.state.die(errors)
+ # instruction doesn't actually do anything (this also aborts an
+ # ECALL instruction, but that doesn't really matter because it
+ # doesn't have any side-effects). Also generate a change that
+ # sets an appropriate error bits in the external ERR_CODE
+ # register and clears the busy flag.
+ self.state.stop()
changes = self.state.changes()
else:
changes = self.state.changes()
diff --git a/hw/ip/otbn/dv/otbnsim/sim/state.py b/hw/ip/otbn/dv/otbnsim/sim/state.py
index 7d78084..d180a05 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/state.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/state.py
@@ -6,9 +6,9 @@
from shared.mem_layout import get_memory_layout
-from .alert import Alert, BadAddrError
from .csr import CSRFile
from .dmem import Dmem
+from .err_bits import BAD_INSN_ADDR
from .ext_regs import OTBNExtRegs
from .flags import FlagReg
from .gpr import GPRs
@@ -51,7 +51,8 @@
self.ext_regs = OTBNExtRegs()
self.running = False
- self.errs = [] # type: List[Alert]
+ self._err_bits = 0
+ self.pending_halt = False
def add_stall_cycles(self, num_cycles: int) -> None:
'''Add stall cycles before the next insn completes'''
@@ -67,14 +68,6 @@
if back_pc is not None:
self.pc_next = back_pc
- def errors(self) -> List[Alert]:
- c = [] # type: List[Alert]
- c += self.errs
- c += self.gprs.errors()
- c += self.dmem.errors()
- c += self.loop_stack.errors()
- return c
-
def changes(self) -> List[Trace]:
c = [] # type: List[Trace]
c += self.gprs.changes()
@@ -89,6 +82,11 @@
return c
def commit(self) -> None:
+ # If the pending_halt flag is set or there are error bits (which should
+ # imply pending_halt is set), we shouldn't get as far as commit.
+ assert not self.pending_halt
+ assert self._err_bits == 0
+
# Update self.stalled. If the instruction we just ran stalled us then
# self._stalls will be positive but self.stalled will be false.
assert self._stalls >= 0
@@ -145,30 +143,22 @@
self.running = True
self._start_stall = True
self.stalled = True
+ self.pending_halt = False
+ self._err_bits = 0
- def _stop(self, err_bits: int) -> None:
- '''Set flags to stop the processor.
+ def stop(self) -> None:
+ '''Set flags to stop the processor and abort the instruction'''
+ self._abort()
- err_bits is the value written to the ERR_BITS 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)
- self.ext_regs.write('ERR_BITS', err_bits, True)
+ self.ext_regs.write('ERR_BITS', self._err_bits, True)
self.running = False
- def die(self, alerts: List[Alert]) -> None:
- err_bits = 0
- for alert in alerts:
- err_bits |= alert.error_bit()
-
- self._abort()
- self._stop(err_bits)
-
def get_quarter_word_unsigned(self, idx: int, qwsel: int) -> int:
'''Select a 64-bit quarter of a wide register.
@@ -266,13 +256,11 @@
# Check the new PC is word-aligned
if self.pc_next & 3:
- self.errs.append(BadAddrError('pc', self.pc_next,
- 'address is not 4-byte aligned'))
+ self._err_bits |= BAD_INSN_ADDR
# Check the new PC lies in instruction memory
if self.pc_next >= self.imem_size:
- self.errs.append(BadAddrError('pc', self.pc_next,
- 'address lies above the top of imem'))
+ self._err_bits |= BAD_INSN_ADDR
def post_insn(self) -> None:
'''Update state after running an instruction but before commit'''
@@ -280,6 +268,12 @@
self.loop_step()
self.gprs.post_insn()
+ self._err_bits |= (self.gprs.err_bits() |
+ self.dmem.err_bits() |
+ self.loop_stack.err_bits())
+ if self._err_bits:
+ self.pending_halt = True
+
def read_csr(self, idx: int) -> int:
'''Read the CSR with index idx as an unsigned 32-bit number'''
return self.csrs.read_unsigned(self.wsrs, idx)
@@ -292,6 +286,12 @@
'''Return the current call stack, bottom-first'''
return self.gprs.peek_call_stack()
- def on_error(self, error: Alert) -> None:
- '''Add a pending error that will be reported at the end of the cycle'''
- self.errs.append(error)
+ def stop_at_end_of_cycle(self, err_bits: int) -> None:
+ '''Tell the simulation to stop at the end of the cycle
+
+ Any bits set in err_bits will be set in the ERR_BITS register when
+ we're done.
+
+ '''
+ self._err_bits |= err_bits
+ self.pending_halt = True
