target/riscv: only update mstatus.*ie bits with set_maskisr steponly #1222
Conversation
When value of mstatus CSR changes while stepping with "set_maskisr steponly", OpenOCD should not write back the old value to mstatus when reenabling interrupts. Signed-off-by: Samuel Obuch <[email protected]>
| if (riscv_reg_get(target, &new_mstatus, GDB_REGNO_MSTATUS) != ERROR_OK) { | ||
| success = false; | ||
| LOG_TARGET_ERROR(target, "Unable to read mstatus after step"); | ||
| goto _exit; | ||
| } | ||
| if (new_mstatus != (current_mstatus & ~irq_disabled_mask)) { | ||
| LOG_TARGET_DEBUG(target, "mstatus value changed while stepping, " | ||
| "only restoring interrupt enable bits."); | ||
| current_mstatus = new_mstatus | (current_mstatus & irq_disabled_mask); | ||
| } |
There was a problem hiding this comment.
TBH, I'm confused by the current implementation of riscv_interrupts_restore().
Looking at the name I would expect it only touches certain bits of mstatus, but it is actually equivalent to riscv_reg_set(target, GDB_REGNO_MSTATUS, current_mstatus).
My suggestion is to either:
- Move the read of
mstatusand the masking-out of irrelevant bits intoriscv_interrupts_restore(). - Replace
riscv_interrupts_restore()withriscv_reg_set().
I slightly prefer the first option since there is the riscv_interrupts_disable() function.
This will introduce a change in behavior since there is a use of riscv_interrupts_disable/restore in riscv_run_algorithm(), and if an algorithm clobbers mstatus current code restores it fully, but I don't think it is the intended behavior.
Note: if you decide to touch riscv_interrupts_disable/restore(), please make them static.
There was a problem hiding this comment.
Yeah, I didnt want to affect the code in riscv_run_algorithm, both to keep this minimal and because I dont have a deep enough understanding of it.
Could you please elaborate on what you think the behavior should be, when restoring registers after algorithm run? Why would we not want to restore mstatus fully?
I am quite curious about this, in the espressif fork we use a different implementation, and actually save/restore a lot more registers (all writable registers as far as I understand).
There was a problem hiding this comment.
I am leaning on the second option. As together with that, we could log a warning if any of the interrupt enable bits have changed.
There was a problem hiding this comment.
Similarly to what @en-sc says above, I would prefer to:
- Move the mstatus change detection into
riscv_interrupt_restore(). - If mstatus change is detected inside
riscv_interrupt_restore(), display a warning, as @MarekVCodasip prefers it. - Change the code in
riscv_interrupt_restore()to update only the IRQ bits, and keep the others intact.
I am not concerned about riscv_run_algorithm() since sensible algorithms (like CRC calculation) will only clobber DPC and general-purpose registers. Restoration of DPC and GPRs is already handled inside riscv_run_algorithm() as of now. It is IMO unlikely that someone would come up with an algorithm that would modify mstatus. And if that happened, the mstatus save & restore would need to be added into riscv_run_algorithm().
With "set_maskisr steponly" setting, OpenOCD can alter the program behaviour when stepping over instructions modifying mstatus. The original value of mstatus is restored after step, discarding any changes that were made. This patch limits the differences to the interrupt enable bits.
To eliminate any influence of OpenOCD on the program state, we would need to decode the stepped instruction and also detect exceptions. Therefore the patch for that is very extensive, but even this small improvement should help in many cases.