Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

target/riscv: only update mstatus.*ie bits with set_maskisr steponly #1222

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from
Open

target/riscv: only update mstatus.*ie bits with set_maskisr steponly #1222

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/target/riscv/riscv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4170,12 +4170,12 @@ static int riscv_openocd_step_impl(struct target *target, int current,
}

bool success = true;
uint64_t current_mstatus;
riscv_reg_t current_mstatus;
riscv_reg_t irq_disabled_mask = MSTATUS_MIE | MSTATUS_HIE | MSTATUS_SIE | MSTATUS_UIE;
RISCV_INFO(info);

if (info->isrmask_mode == RISCV_ISRMASK_STEPONLY) {
/* Disable Interrupts before stepping. */
uint64_t irq_disabled_mask = MSTATUS_MIE | MSTATUS_HIE | MSTATUS_SIE | MSTATUS_UIE;
if (riscv_interrupts_disable(target, irq_disabled_mask,
&current_mstatus) != ERROR_OK) {
success = false;
Expand All @@ -4199,11 +4199,24 @@ static int riscv_openocd_step_impl(struct target *target, int current,

riscv_reg_cache_invalidate_all(target);

if (info->isrmask_mode == RISCV_ISRMASK_STEPONLY)
if (info->isrmask_mode == RISCV_ISRMASK_STEPONLY) {
riscv_reg_t new_mstatus;
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);
}
Comment on lines +4204 to +4213
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Move the read of mstatus and the masking-out of irrelevant bits into riscv_interrupts_restore().
  2. Replace riscv_interrupts_restore() with riscv_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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@JanMatCodasip JanMatCodasip Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().


if (riscv_interrupts_restore(target, current_mstatus) != ERROR_OK) {
success = false;
LOG_TARGET_ERROR(target, "Unable to restore interrupts.");
}
}

_exit:
if (enable_watchpoints(target, wps_to_enable) != ERROR_OK) {
Expand Down