Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for a Zisk guest program by providing a RISC-V64 linker script, startup assembly, Zig runtime modules (lib, io, layout), and registering a new zisk target in the build system.
- Defined memory regions and sections in
zisk.ldfor ROM/RAM layout - Implemented
_startassembly stub to set upgpandsp, then callmain - Added Zig modules for I/O (
get_input,print_str), layout constants, and ahaltruntime - Registered the
zisktarget inbuild.zig
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkgs/state-transition-runtime/src/zisk/zisk.ld | Linker script for ROM/RAM memory regions and section layout |
| pkgs/state-transition-runtime/src/zisk/start.s | Assembly entrypoint initializing registers and tail-calling main |
| pkgs/state-transition-runtime/src/zisk/lib.zig | Runtime functions: halt and free_input |
| pkgs/state-transition-runtime/src/zisk/layout.zig | Memory-mapped I/O address constants |
| pkgs/state-transition-runtime/src/zisk/io.zig | get_input and print_str implementations for Zisk |
| build.zig | Adds the zisk target configuration to the build script |
Comments suppressed due to low confidence (1)
pkgs/state-transition-runtime/src/zisk/io.zig:6
- These new I/O functions (
get_input,print_str) don’t have tests yet; consider adding unit or integration tests to verify input-size parsing and UART write behavior.
pub fn get_input(_: std.mem.Allocator) []const u8 {
| pub fn halt(_: u32) noreturn { | ||
| const arch_id_zisk = asm volatile ("csrr %[ret], marchid" | ||
| : [ret] "=r" (-> usize), | ||
| ); | ||
| if (arch_id_zisk == 0xFFFEEE) { | ||
| // Zisk exit | ||
| asm volatile ( | ||
| \\ li a7, 93 | ||
| \\ ecall | ||
| ); | ||
| } else { | ||
| // QEMU exit | ||
| //"li t0, {_QEMU_EXIT_ADDR}", | ||
| //"li t1, {_QEMU_EXIT_CODE}", | ||
| asm volatile ( | ||
| \\ li t0, 0x100000 | ||
| \\ li t1, 0x5555 |
There was a problem hiding this comment.
The halt function ignores its code parameter and uses a hardcoded exit code. Consider passing the code argument into the ecall (in a0) or the QEMU exit store instead of always using 0x5555.
| pub fn halt(_: u32) noreturn { | |
| const arch_id_zisk = asm volatile ("csrr %[ret], marchid" | |
| : [ret] "=r" (-> usize), | |
| ); | |
| if (arch_id_zisk == 0xFFFEEE) { | |
| // Zisk exit | |
| asm volatile ( | |
| \\ li a7, 93 | |
| \\ ecall | |
| ); | |
| } else { | |
| // QEMU exit | |
| //"li t0, {_QEMU_EXIT_ADDR}", | |
| //"li t1, {_QEMU_EXIT_CODE}", | |
| asm volatile ( | |
| \\ li t0, 0x100000 | |
| \\ li t1, 0x5555 | |
| pub fn halt(code: u32) noreturn { | |
| const arch_id_zisk = asm volatile ("csrr %[ret], marchid" | |
| : [ret] "=r" (-> usize), | |
| ); | |
| if (arch_id_zisk == 0xFFFEEE) { | |
| // Zisk exit | |
| asm volatile ( | |
| \\ mv a0, {code} | |
| \\ li a7, 93 | |
| \\ ecall | |
| ); | |
| } else { | |
| // QEMU exit | |
| asm volatile ( | |
| \\ li t0, 0x100000 | |
| \\ mv t1, {code} |
| \\ li t0, 0x100000 | ||
| \\ li t1, 0x5555 |
There was a problem hiding this comment.
[nitpick] Extract the hardcoded exit-store address 0x100000 and exit code 0x5555 into named constants to clarify their intention in the QEMU path.
| \\ li t0, 0x100000 | |
| \\ li t1, 0x5555 | |
| \\ li t0, QEMU_EXIT_ADDR | |
| \\ li t1, QEMU_EXIT_CODE |
Signed-off-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
Signed-off-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
Signed-off-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.