RISC-V Semihosting 2 of 3: Refactor magic sequence detection #1199
Conversation
JanMatCodasip
changed the title
|
Note: This MR is built on top of #1198. |
src/target/riscv/riscv_semihosting.c
Outdated
| return SEMIHOSTING_NONE; | ||
| } | ||
| if (!sequence_found) { | ||
| LOG_TARGET_DEBUG(target, " -> NONE (no magic)"); |
There was a problem hiding this comment.
maybe move this printout inside riscv_semihosting_detect_magic_sequence?
if (result != ERROR_OK) {
*sequence_found = false;
return result;
}
Having context-dependent printout like this is somewhat confusing.
There was a problem hiding this comment.
Addressed, but differently.
My understanding is that the prints like -> NONE (no magic) were supposed to print the final outcome of riscv_semihosting() function into the debug log for easier troubleshooting.
I agree that the prints looked confusing.
For consistency and clarity, I changed all of them to this form Semihosting outcome: SOMETHING (extra explanation). Please, let me know if this form looks OK to you.
JanMatCodasip
force-pushed
the
jm-codasip/semihosting-2-refactor-magic-seq-detection
branch
from
4af47de to
eeedccd
Compare
Refactor (clean up) the code in riscv_semihosting.c by moving the magic sequence detection to its own function. Change-Id: I3a3ce991336ceeeff023d459d0e28558059554e0 Signed-off-by: Jan Matyas <[email protected]>
Cosmetic, non-functional improvements in `riscv_semihosting_detect_magic_sequence()`. Change-Id: I935d3847749a02e4f579900a0971c30a5cc826b7 Signed-off-by: Jan Matyas <[email protected]>
Cleanup the debug prints denoting the semihosting outcome so that they are easier to understand when reading the OpenOCD's verbose (debug) log. Change-Id: Ibdef1b4474e47dd0a135d4696b3e53600c544eb8 Signed-off-by: Jan Matyas <[email protected]>
JanMatCodasip
force-pushed
the
jm-codasip/semihosting-2-refactor-magic-seq-detection
branch
from
eeedccd to
d0165b4
Compare
|
|
||
| const uint32_t magic[] = { | ||
| 0x01f01013, /* slli zero,zero,0x1f */ | ||
| 0x00100073, /* ebreak */ |
There was a problem hiding this comment.
Is there a need to read the memory at the PC? The fact that the hart was halted due to ebreak execution is known from reading dcsr.cause.
Please note, this is just a question and I don't think it should be addressed in this commit.
| /* Read three uncompressed instructions: | ||
| * The previous, the current one (pointed to by PC) and the next one. */ | ||
| const target_addr_t sequence_start_address = pc - 4; | ||
| for (int i = 0; i < 3; i++) { |
There was a problem hiding this comment.
Wouldn't it be easier and faster to read 12 bytes at once using target_read_buffer() and compare it with the magic sequence from a buffer filled by buf_set_u32()?
Please note, this is just a question and I don't think it should be addressed in this commit.
There was a problem hiding this comment.
In general, I believe riscv_read_by_any_size() is redundant. See #1205
| return result; | ||
| } | ||
|
|
||
| const uint32_t value = target_buffer_get_u32(target, buf); |
There was a problem hiding this comment.
target_buffer_get_u32() converts the endianness, therefore it is not correct to use it here, since:
- Instruction encoding does not depend on endianness.
riscv_read_by_any_size()fills thebufby reads of some size. I hope the endianness of the elements is converted to little before storing to the buffer. Alternatively it is a bug intarget_buffer_get_u32().
Refactor (clean up) the code in riscv_semihosting.c by moving
the magic sequence detection to its own function.