WIP: Add Semihosting Support #1276
Conversation
.gitignore
Outdated
| /build | ||
| # CMake build directories used by CLion. | ||
| /cmake-build-* | ||
| test/semihosting |
There was a problem hiding this comment.
This probably shouldn't be here? If you want to add it locally you cna use .git/info/exclude
There was a problem hiding this comment.
I've added a few test files and integrated them with both CMake and CI. Although I meet some issues:
- The riscv-gnu-toolchain seems have issue when compiling Picolibc. [RISCV] Compile failed with riscv-gnu-toolchain picolibc/picolibc#1086
- Picolibc doesn't seem to work well with Clang, so for now, the semihosting tests are only supported compiling with GCC. tracking: Towards full LLVM (no gnu toolchain) builds RIOT-OS/RIOT#19642
- I don't know if the riscv64-unknown-elf-gcc package is available on macOS via Brew, so I only enabled Ubuntu in the CI.
- The picolibc package from apt on Ubuntu 22.04 is missing crt0-semihost.so, so I have to build picolibc from source from the official release. (ubuntu2404 is ok, at least in my machine)
| // Suffix = 0x40705013, // srai x0, x0, 7 NOP encoding semihosting | ||
| bool is_semihosting() | ||
| { | ||
| return (mem::read<uint32_t>(zPC.bits - 4) == 0x01f01013) |
There was a problem hiding this comment.
This here is slightly fragile, if someone builds the sail emulator on a big-endian platform they will not detect this. Either ensure you convert from little endian or just compare the individual bytes?
E.g. char magic[12]; mem::read(zPC.bits - 4, magic, 12); memcmp(magic, ...) == 0)
There was a problem hiding this comment.
This will be addressed next. And I guess we should also push forward #751?
There was a problem hiding this comment.
I think this is unrelated since I am concerned about the host platform being big endian, not the simulation target. Instructions are always the same endianess so this pr should be independent of that
| @@ -0,0 +1,368 @@ | |||
| // See LICENSE for license details. | |||
There was a problem hiding this comment.
Unrelated but I think we should just add a SPDX-License-Identifier: line to all files?
Some of this looks copied from gem5, I think this is fine due to the license, but probably need to add some attribution?
|
Features are basically complete now, also added a few tests and integrated them with both CMake and CI Add two new command-line arguments :
Also added two new CMake parameters:
The C++ template syntax here is quite complex, but it's also the way to keep the SYS_XXX implementations concise. I haven't found a better solution. Although I believe this is already much simpler than the gem5 implementation. |
trdthg
force-pushed
the
semihosting
branch
3 times, most recently
from
7a6af4d to
e0ec83f
Compare
arichardson
left a comment
arichardson
left a comment
There was a problem hiding this comment.
A few more comments, will try to review the rest later
| uint64_t addr = (uint64_t)ptr; | ||
| for (size_t i = 0; i < len; i += 1) { | ||
| char c = read_mem((uint64_t)addr); | ||
| if (c) { |
There was a problem hiding this comment.
Why are we skipping nul bytes here? Wouldn't it make sense to also just stop and treat Len as a maximum length?
|
|
||
| bool mem::target_is_little_endian = true; | ||
|
|
||
| void mem::read(uint64_t addr, void *dst, size_t len) |
There was a problem hiding this comment.
I don't think these functions should be doing the byteswapping, that should be done by wrapper functions that read an integer type
| // start from addr, len = sizeof(T) | ||
| template <typename T> static T read(uint64_t addr) | ||
| { | ||
| uint8_t buffer[sizeof(T)] = {0}; |
There was a problem hiding this comment.
| uint8_t buffer[sizeof(T)] = {0}; | |
| alignas(T) uint8_t buffer[sizeof(T)] = {0}; |
Otherwise the reinterpret_cast is unsafe. Or you could use a union.
| std::string real_mode(mode); | ||
| if (real_mode[0] == 'w') | ||
| real_mode[0] = 'a'; | ||
| file = fopen(name.c_str(), real_mode.c_str()); |
There was a problem hiding this comment.
Should we support access to the host file system by default? Gem5 requires an extra flag to do that and I feel we should do here too.
|
|
||
| RiscvSemihosting::File::~File() | ||
| { | ||
| if (file) { |
There was a problem hiding this comment.
We should not be closing stdin/stdout/stderr here
| { | ||
| } | ||
|
|
||
| int64_t RiscvSemihosting::FileFeatures::read(uint8_t *buffer, uint64_t size) |
There was a problem hiding this comment.
I wonder if we can replace this whole class with a call to fmemopen() in read-only mode. That should hopefully be equivalent
2 participants
link to #1250
Currently I have only implemented and tested a subset of the semihosting operations, based on the gem5 implementation, and there are still a lot of work to be done, including integrating the tests into CMake.
So for now, this is just a show of how to integrate semihosting into the sail-model to ensure we‘re heading in the right direction.