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: Ensure to handle all triggered a halt events #1171

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

target/riscv: Ensure to handle all triggered a halt events #1171

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
17 changes: 17 additions & 0 deletions src/target/riscv/riscv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3743,6 +3743,7 @@ int riscv_openocd_poll(struct target *target)
unsigned int should_resume = 0;
unsigned int halted = 0;
unsigned int running = 0;
unsigned int cause_groups = 0;
struct target_list *entry;
foreach_smp_target(entry, targets) {
struct target *t = entry->target;
Expand Down Expand Up @@ -3790,6 +3791,22 @@ int riscv_openocd_poll(struct target *target)
LOG_TARGET_DEBUG(target, "resume all");
riscv_resume(target, true, 0, 0, 0, false);
} else if (halted && running) {
foreach_smp_target(entry, targets)
{
struct target *t = entry->target;
if (t->state == TARGET_HALTED) {
riscv_reg_t dcsr;
if (riscv_reg_get(t, &dcsr, GDB_REGNO_DCSR) != ERROR_OK)
return ERROR_FAIL;
if (get_field(dcsr, CSR_DCSR_CAUSE) == CSR_DCSR_CAUSE_GROUP)
cause_groups++;
else
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This loop seems weird. What is it's purpose? DCSR is writable, so we can occasionally trick debugger into wrong conclusion.

halt groups are an optional feature and I'm quite confused that we don't check for it.

Could you please provide a test scenario to reproduce your issue? Is it possible to use spike to model it? Or do you need a specific HW ?

Copy link
Contributor Author

@lz-bro lz-bro Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This bug was discovered when I was testing Semihosting on our hardware and smp is enable. It fails when the following happens:
If all current halted states are due to a halt group, and other harts state was running. In fact, there was a hart halted, which caused the other harts to halt because of the hart group.

riscv-openocd/src/target/riscv/riscv.c

Lines 3792 to 3796 in f51900b

} else if (halted && running) {
LOG_TARGET_DEBUG(target, "halt all; halted=%d",
halted);
riscv_halt(target);
} else {

If there is such a halted hart，but the record status is running，it would not process riscv_semihosting.

riscv-openocd/src/target/riscv/riscv.c

Lines 3605 to 3623 in f51900b

if (halt_reason == RISCV_HALT_EBREAK) {
int retval;
/* Detect if this EBREAK is a semihosting request. If so, handle it. */
switch (riscv_semihosting(target, &retval)) {
case SEMIHOSTING_NONE:
break;
case SEMIHOSTING_WAITING:
/* This hart should remain halted. */
*next_action = RPH_REMAIN_HALTED;
break;
case SEMIHOSTING_HANDLED:
/* This hart should be resumed, along with any other
* harts that halted due to haltgroups. */
*next_action = RPH_RESUME;
return ERROR_OK;
case SEMIHOSTING_ERROR:
return retval;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aap-sc I think this is a bug, would you provide some suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lz-bro I'm still trying to understand your reasoning and what the issue is exactly (the situation is still not quite obvious to me). It will take a couple of days - I'll ask additional question if necessary.

if (halted == cause_groups)
return ERROR_OK;

LOG_TARGET_DEBUG(target, "halt all; halted=%d",
halted);
riscv_halt(target);
Expand Down