Move QEMU executables to a non-default member test-qemu, and add multi-hart tests#382
Conversation
test-qemu, and add multi-hart tests
romancardenas
left a comment
There was a problem hiding this comment.
Thanks for your PR! I'm currently on holidays, so I cannot do a proper review. You can address this first batch of comments in the meantime
| //! UART example for QEMU virt machine | ||
| //! | ||
| //! This example demonstrates direct UART output on QEMU's virt machine. | ||
| //! It writes to the NS16550-compatible UART at 0x1000_0000. | ||
|
|
There was a problem hiding this comment.
Please, put back the docs, I think they are valuable
| @@ -1,8 +1,3 @@ | |||
| //! Semihosting example for QEMU | |||
There was a problem hiding this comment.
Please, put back the docs, I think they are valuable
tests-qemu/examples/multi_hart.rs
Outdated
| while !HART0_READY.load(Ordering::Acquire) { | ||
| core::hint::spin_loop(); | ||
| } |
There was a problem hiding this comment.
I think this handshake is not necessary: HART 1 will not reach main until it receives an interrupt provoked by HART 0 at line 127.
tests-qemu/examples/multi_hart.rs
Outdated
| HART0_READY.store(true, Ordering::Release); | ||
|
|
||
| // Small delay to allow Hart 1 to reach wfi() in _mp_hook | ||
| // This is needed because we can't use atomics in assembly _mp_hook | ||
| for _ in 0..100000 { | ||
| core::hint::spin_loop(); | ||
| } |
There was a problem hiding this comment.
I think this is not necessary. Just initializing the UART and triggering an interrupt should be enough
tests-qemu/examples/multi_hart.rs
Outdated
| // Enable machine software interrupts | ||
| li t0, 8 // MIE.MSIE bit | ||
| csrs mie, t0 | ||
|
|
||
| 1: wfi // Wait for interrupt | ||
| // Check if software interrupt pending | ||
| csrr t0, mip | ||
| andi t0, t0, 8 // Check MSIP bit | ||
| beqz t0, 1b // If not set, keep waiting |
There was a problem hiding this comment.
Maybe, at this point, it is better to use polling with interrupts disabled, so interrupts are not triggered?
| let memory_x = match (s_mode, multi_hart) { | ||
| (true, _) => include_bytes!("memory-s-mode.x").as_slice(), | ||
| (false, true) => include_bytes!("memory-m-mode-multihart.x").as_slice(), | ||
| (false, false) => include_bytes!("memory-m-mode.x").as_slice(), | ||
| }; |
There was a problem hiding this comment.
Does S mode affect the linker file? I think only multi-hart is relevant. We only need two linker files: one for single hart and one for multi hart
There was a problem hiding this comment.
Also, as it is now, multi hart is only compatible with M mode right? we could add here a check
There was a problem hiding this comment.
Well @romancardenas the S mode linker file is indeed needed, I can't remove it otherwise the S-mode tests fail :(
At-least from what's visible in my local tests, therefore I have kept it.
There was a problem hiding this comment.
Yes, I know you need a linker file, but this linker file can be the exact same as in M mode. We only need two linker files: memory.x and memory-multihart.x
tests-qemu/Cargo.toml
Outdated
| [features] | ||
| default = ["m-mode", "single-hart"] | ||
| s-mode = ["riscv-rt/s-mode", "single-hart"] | ||
| m-mode = [] |
There was a problem hiding this comment.
We can remove this feature
|
DONE! @romancardenas I have tried to address all of your comments here... I apologize if I missed something. |
| let memory_x = match (s_mode, multi_hart) { | ||
| (true, _) => include_bytes!("memory-s-mode.x").as_slice(), | ||
| (false, true) => include_bytes!("memory-m-mode-multihart.x").as_slice(), | ||
| (false, false) => include_bytes!("memory-m-mode.x").as_slice(), | ||
| }; |
There was a problem hiding this comment.
Yes, I know you need a linker file, but this linker file can be the exact same as in M mode. We only need two linker files: memory.x and memory-multihart.x
| MEMORY | ||
| { | ||
| RAM : ORIGIN = 0x80200000, LENGTH = 16M | ||
| } | ||
| REGION_ALIAS("REGION_TEXT", RAM); | ||
| REGION_ALIAS("REGION_RODATA", RAM); | ||
| REGION_ALIAS("REGION_DATA", RAM); | ||
| REGION_ALIAS("REGION_BSS", RAM); | ||
| REGION_ALIAS("REGION_HEAP", RAM); | ||
| REGION_ALIAS("REGION_STACK", RAM); | ||
| INCLUDE link.x |
There was a problem hiding this comment.
this file is not required, use memory.x instead
There was a problem hiding this comment.
The memory-s-mode.x linker file cannot be replaced with memory.x because OpenSBI loads the payload at 0x80200000, not 0x80000000. I did initially delete it but I had to restore it since the tests just fail.
.github/workflows/qemu.yaml
Outdated
| - example-features: | ||
| features: "s-mode" | ||
| target-qemu: | ||
| target: riscv32i-unknown-none-elf | ||
| - example-features: | ||
| features: "s-mode" | ||
| target-qemu: | ||
| target: riscv32im-unknown-none-elf | ||
| - example-features: | ||
| features: "s-mode" | ||
| target-qemu: | ||
| target: riscv32imc-unknown-none-elf | ||
| - example-features: | ||
| features: "s-mode" | ||
| target-qemu: | ||
| target: riscv32imac-unknown-none-elf | ||
| - example-features: | ||
| features: "s-mode" | ||
| target-qemu: | ||
| target: riscv32imafc-unknown-none-elf |
There was a problem hiding this comment.
Can you try this instead?
| - example-features: | |
| features: "s-mode" | |
| target-qemu: | |
| target: riscv32i-unknown-none-elf | |
| - example-features: | |
| features: "s-mode" | |
| target-qemu: | |
| target: riscv32im-unknown-none-elf | |
| - example-features: | |
| features: "s-mode" | |
| target-qemu: | |
| target: riscv32imc-unknown-none-elf | |
| - example-features: | |
| features: "s-mode" | |
| target-qemu: | |
| target: riscv32imac-unknown-none-elf | |
| - example-features: | |
| features: "s-mode" | |
| target-qemu: | |
| target: riscv32imafc-unknown-none-elf | |
| - example-features: | |
| features: "s-mode" | |
| target-qemu: | |
| qemu: riscv32 |
xtask/src/main.rs
Outdated
| let smp_arg; | ||
| if multi_hart { | ||
| smp_arg = "2".to_string(); | ||
| qemu_args.push("-smp"); | ||
| qemu_args.push(&smp_arg); | ||
| } |
There was a problem hiding this comment.
| let smp_arg; | |
| if multi_hart { | |
| smp_arg = "2".to_string(); | |
| qemu_args.push("-smp"); | |
| qemu_args.push(&smp_arg); | |
| } | |
| if multi_hart { | |
| qemu_args.push("-smp"); | |
| qemu_args.push("2"); | |
| } |
|
Thanks! |
This PR tends to resolve #311.
As requested this PR moves the QEMU executables to a
test-qemunon-default member, with a smarter set of tests features that would allow us to execute tests in supervisor mode, machine mode, single hart, multi hart.