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

Fix clearing fetch FIFO twice when CJALR fails #27

Closed marnovandermaas closed 5 months ago

marnovandermaas commented 6 months ago

When CJALR fails, for example on a permit execute violation, the fetch FIFO is cleared twice, first because branch_req_spec_raw is asserted in the cheri_ex module and second because cheri_wb_err_o is asserted the next cycle. This causes the first instruction of the trap handler to be cleared and causes the PC and instruction data to get out of sync.

The solution in this commit is to not assert branch_req_spec_raw when there is an instruction fault.

Closes: https://github.com/microsoft/cheriot-ibex/issues/28

marnovandermaas commented 6 months ago

For future reference, this is the waveform where I saw the issue: IllegelCjalrBug

kliuMsft commented 6 months ago

First, I don't mind removing branch_req_spec_o now, that's actually the purpose of he ISA definition change (delayed address bound checking). However I'd like to understand why a speculative fetch followed by an exception in WB stage would cause PC & instruction to get out of sync? I have a little trouble seeing that from your waveform and my expectation was that the exception would flush the fetch fifo and essentially realign things.

marnovandermaas commented 5 months ago

First, I don't mind removing branch_req_spec_o now, that's actually the purpose of he ISA definition change (delayed address bound checking). However I'd like to understand why a speculative fetch followed by an exception in WB stage would cause PC & instruction to get out of sync? I have a little trouble seeing that from your waveform and my expectation was that the exception would flush the fetch fifo and essentially realign things.

My initial attempt at removing branch_req_spec_o did not work, so this may need further investigation. I agree that the waveform I've posted is not very clear. Hopefully, I have clarified this by opening the following issue: https://github.com/microsoft/cheriot-ibex/issues/28

marnovandermaas commented 5 months ago

Converting this PR to draft. The commit message is no longer correct although removing the speculative branch request may still be a good thing to do. See this issue for more details: https://github.com/microsoft/cheriot-ibex/issues/28

marnovandermaas commented 5 months ago

Closing this PR because the proposed solution causes timing issues and with the recent ISA change, bounds and permissions checking will happen before PC is written so this solution won't apply anyways.