riscv: simplify state management during examine #900
Conversation
aap-sc
force-pushed
the
aap-sc/simplify_state_managment
branch
from
f058b30
to
e2bdf0e
Compare
src/target/riscv/riscv-013.c
Outdated
| target->state = TARGET_HALTED; | ||
| target->debug_reason = DBG_REASON_DBGRQ; |
There was a problem hiding this comment.
Seeing as the target-> state is set here and also in riscv013_halt_go and debug_reason later. I would just delete these two lines.
There was a problem hiding this comment.
Agreed, this seems redundant. The change of the recorded target state will be covered by lines 1979 and 1980.
There was a problem hiding this comment.
Agreed. Remove these 2 lines.
There was a problem hiding this comment.
I'll take look. on the first glance, yes it looks like we should remove this.
src/target/riscv/riscv-013.c
Outdated
| target->state = TARGET_UNKNOWN; | ||
| target->debug_reason = DBG_REASON_UNDEFINED; |
There was a problem hiding this comment.
What does this achieve? I don't think it's accessed below, it's just unconditionally overwritten.
There was a problem hiding this comment.
What does this achieve? I don't think it's accessed below, it's just unconditionally overwritten.
This is for the case when examine fails. Imagine we could not examine/could not finish examination process. This allows us just to reset the state from whatever it was. My understanding is that examine (regardless of the current implementation) can actually be invoked several times (via arp_examine for example). See here:
There was a problem hiding this comment.
corrected the message. arp_init -> arp_examine
There was a problem hiding this comment.
OK. Can you add a short comment, like /* In case e.g. riscv013_halt_go() fails. */
Generally I would like to see these status updates happen where we know the status changed, but that's a much larger change.
There was a problem hiding this comment.
OK. Can you add a short comment, like /* In case e.g. riscv013_halt_go() fails. */
Addressed. Please do note that I moved these to lines way higher. I think this is more appropriate place.
src/target/riscv/riscv-013.c
Outdated
| target->state = TARGET_HALTED; | ||
| target->debug_reason = DBG_REASON_DBGRQ; |
There was a problem hiding this comment.
Agreed. Remove these 2 lines.
| target->state = TARGET_HALTED; | ||
| target->debug_reason = DBG_REASON_UNDEFINED; |
There was a problem hiding this comment.
Why is this necessary? We know that the hart is halted on line 2054, and the state should reflect that.
There was a problem hiding this comment.
Why is this necessary? We know that the hart is halted on line 2054, and the state should reflect that.
Yes, we know that the hart is halted. And we have target->state = TARGET_HALTED to indicate that. However, we don't know why hart was halted (since we've just connected) and thus we set the reason to UNDEFINED.
aap-sc
force-pushed
the
aap-sc/simplify_state_managment
branch
from
e2bdf0e
to
0fbdf8c
Compare
src/target/riscv/riscv-013.c
Outdated
| target->state = TARGET_UNKNOWN; | ||
| target->debug_reason = DBG_REASON_UNDEFINED; |
There was a problem hiding this comment.
OK. Can you add a short comment, like /* In case e.g. riscv013_halt_go() fails. */
Generally I would like to see these status updates happen where we know the status changed, but that's a much larger change.
| target->state = TARGET_HALTED; | ||
| target->debug_reason = DBG_REASON_UNDEFINED; |
This also fixes a bug when, after `examine` completion, the target still has `unknown` status. To reproduce this one spike, it is enough to do the following: --- // make sure spike harts are halted openocd ... -c init -c 'echo "[targets]"' --- this behavior is quite dangerous and leads to segfaults in some cases Change-Id: I13915f7038ad6d0251d56d2d519fbad9a2f13c18 Signed-off-by: Parshintsev Anatoly <[email protected]>
aap-sc
force-pushed
the
aap-sc/simplify_state_managment
branch
from
0fbdf8c
to
198edca
Compare
This also fixes a bug when, after
examinecompletion, the target stillhas
unknownstatus. To reproduce this one spike, it is enough to dothe following:
this behavior is quite dangerous and leads to segfaults in some cases
Change-Id: I13915f7038ad6d0251d56d2d519fbad9a2f13c18