microsoft / cheriot-ibex

cheriot-ibex is a RTL implementation of CHERIoT ISA based on LowRISC's Ibex core.
Apache License 2.0
73 stars 14 forks source link

EBREAK #33

Closed mndstrmr closed 1 month ago

mndstrmr commented 4 months ago

The Sail requires that on an EBREAK instruction MTVAL is set to the PC. This is not done at the moment.

Update the EBREAK handling code in the controller to the following fixes the issue:

ebrk_insn_prio: begin
  if (debug_mode_q | ebreak_into_debug) begin
    /* snip */
  end else begin
    /* snip */
    exc_cause_o      = EXC_CAUSE_BREAKPOINT;
    csr_mtval_o      = pc_id_i;
  end
end
kliuMsft commented 4 months ago

Interesting. This is inherited from the original ibex design I think. Although, per RISC-v privileged spec, I think hardware platform is allowed to set mtval as 0 in this case? Or this is another case where sail wants to use the same setting across all exceptions?

mndstrmr commented 4 months ago

If that is the case the Sail does not reflect that.

  1. execute EBREAK calls
  2. handle_mem_exception(PC, E_Breakpoint()) calls
  3. exception_handler(..., CTL_TRAP(t), PC) calls
  4. trap_handler(...)
  5. Which sets mtval = PC
marnovandermaas commented 4 months ago

I don't see mtval being mentioned in the spec. In case the spec does not mind I would prefer changing the Sail rather than Ibex.

image

rmn30 commented 4 months ago

In that case this seems like an issue with upstream sail-riscv. This looks interesting: https://github.com/riscv/sail-riscv/issues/70

marnovandermaas commented 4 months ago

In that case this seems like an issue with upstream sail-riscv. This looks interesting: riscv/sail-riscv#70

Looks like that is in a newer version of the spec. It's probably better to stay with a ratified version for now though right?

kliuMsft commented 3 months ago

@mndstrmr I'd say let's table this one for now. I think part of the problem with sail is that there is not a uniform way to allow different implementations. In this case mtval is basically a don't care if we follow the ratified spec.

kliuMsft commented 2 months ago

Actually I am making a cheriot-specific (only when cheri_pmode = 1) change for this to match sail (which is also required for simulation-based verification).

mndstrmr commented 1 month ago

This proves as well, thanks.