Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(testing): in-qemu tests #59

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .cargo/config
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ runner = "bootimage runner"
dev-env = "install cargo-xbuild bootimage"
run-x64 = "xrun --target=x86_64-mycelium.json"
debug-x64 = "xrun --target=x86_64-mycelium.json -- -gdb tcp::1234 -S"
test-x64 = "xtest --target=x86_64-mycelium.json"
21 changes: 19 additions & 2 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,27 @@ jobs:
with:
command: bootimage
args: --target=x86_64-mycelium.json
- name: Test

test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
- name: install rust toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: nightly
components: rust-src, llvm-tools-preview
- name: install dev env
uses: actions-rs/[email protected]
with:
command: dev-env
- name: install qemu
run: sudo apt-get install qemu
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this would still fail if we changed it to

Suggested change
run: sudo apt-get install qemu
run: sudo apt-get update && sudo apt-get install qemu

- name: run tests
uses: actions-rs/[email protected]
with:
command: test
command: test-x64
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like this only runs the in-qemu kernel tests, so if we add regular Rust unit tests in any of the lib crates, they're no longer being run on CI? IMHO we should probably have separate CI jobs for the in-qemu kernel tests and for regular rust unit tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they were running on CI before anyway, as we were only running cargo test on the root crate. We should definitely run tests for non-root crates though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they were running on CI before anyway, as we were only running cargo test

oh whoops, lol.


clippy:
runs-on: ubuntu-latest
Expand Down
10 changes: 7 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
members = [
"alloc",
"util",
"util-macros",
"hal-core",
"hal-x86_64",
]
Expand All @@ -12,12 +13,10 @@ version = "0.0.1"
authors = ["Eliza Weisman <[email protected]>"]
edition = "2018"

[lib]
name = "mycelium_kernel"

[dependencies]
hal-core = { path = "hal-core" }
mycelium-alloc = { path = "alloc" }
mycelium-util = { path = "util" }
tracing = { version = "0.1", default_features = false }

[target.'cfg(target_arch = "x86_64")'.dependencies]
Expand All @@ -35,6 +34,11 @@ wat = "1.0"

[package.metadata.bootimage]
default-target = "x86_64-mycelium.json"
test-success-exit-code = 33 # (0x10 << 1) | 1
test-args = [
"-device", "isa-debug-exit,iobase=0xf4,iosize=0x04",
"-serial", "stdio", "-display", "none"
]

[package.metadata.target.'cfg(target_arch = "x86_64")'.cargo-xbuild]
memcpy = true
Expand Down
22 changes: 20 additions & 2 deletions src/arch/x86_64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,9 @@ impl BootInfo for RustbootBootInfo {
}

#[no_mangle]
#[cfg(target_os = "none")]
pub extern "C" fn _start(info: &'static bootinfo::BootInfo) -> ! {
let bootinfo = RustbootBootInfo { inner: info };
mycelium_kernel::kernel_main(&bootinfo);
crate::kernel_main(&bootinfo);
}

#[cold]
Expand Down Expand Up @@ -122,3 +121,22 @@ pub(crate) fn oops(cause: &dyn core::fmt::Display) -> ! {
}
}
}

#[cfg(test)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[repr(u32)]
pub(crate) enum QemuExitCode {
Success = 0x10,
Failed = 0x11,
}

/// Exit using `isa-debug-exit`, for use in tests
///
/// NOTE: This is a temporary mechanism until we get proper shutdown implemented
#[cfg(test)]
pub(crate) fn qemu_exit(exit_code: QemuExitCode) {
let code = exit_code as u32;
unsafe {
asm!("out 0xf4, eax" :: "{eax}"(code) :: "intel","volatile");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, take it or leave it: i think that everywhere else, we use AT&T syntax for inline assembly. i'm fine with either, but i think we might want to agree on a project convention for consistency...since the operand ordering is reversed it can be kind of confusing IMO

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(oh god does this mean we need to have a style guide)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can use the AT&T syntax, I'm just more used to intel syntax so tend to use it by default.
I don't think we need a style guide because of inline asm, but it's good to try to be generally consistent.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i’m fine with either syntax (i actually have a slight preference for intel as well, for extremely silly reasons: [foo + bar] is far more obvious to me than...however you say that in at&t, which i actually don’t even remember).

i think existing inline asm is at&t mostly just because most of it is about one instruction that doesn’t have any memory operands and i was too lazy to type :::: “intel”. i would be absolutely fine with changing everything to intel. i’m sure @iximeow has at least one opinion about this...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh another note is that since @iximeow is working on adding disassembly to kernel oopses using yaxpeax, and i believe yax’s formatter is intel-ish, i have a slight preference for using the same syntax in the kernel source and in oopses

}
}
106 changes: 0 additions & 106 deletions src/lib.rs

This file was deleted.

137 changes: 136 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,121 @@
#![cfg_attr(target_os = "none", feature(alloc_error_handler))]
#![cfg_attr(target_os = "none", feature(asm))]
#![cfg_attr(target_os = "none", feature(panic_info_message, track_caller))]
#![cfg_attr(target_os = "none", feature(custom_test_frameworks))]
#![cfg_attr(target_os = "none", test_runner(test_runner))]
#![cfg_attr(target_os = "none", reexport_test_harness_main = "test_main")]

pub mod arch;
extern crate alloc;

use core::fmt::Write;
use hal_core::{boot::BootInfo, mem, Architecture};
use alloc::vec::Vec;

mod wasm;
mod arch;

const HELLOWORLD_WASM: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/helloworld.wasm"));

pub fn kernel_main<A>(bootinfo: &impl BootInfo<Arch = A>) -> !
where
A: Architecture,
{
let mut writer = bootinfo.writer();
writeln!(
&mut writer,
"hello from mycelium {} (on {})",
env!("CARGO_PKG_VERSION"),
A::NAME
)
.unwrap();
writeln!(&mut writer, "booting via {}", bootinfo.bootloader_name()).unwrap();

if let Some(subscriber) = bootinfo.subscriber() {
tracing::dispatcher::set_global_default(subscriber).unwrap();
}

let mut regions = 0;
let mut free_regions = 0;
let mut free_bytes = 0;

{
let span = tracing::info_span!("memory map");
let _enter = span.enter();
for region in bootinfo.memory_map() {
let kind = region.kind();
let size = region.size();
tracing::info!(
" {:>10?} {:>15?} {:>15?} B",
region.base_addr(),
kind,
size,
);
regions += 1;
if region.kind() == mem::RegionKind::FREE {
free_regions += 1;
free_bytes += size;
}
}

tracing::info!(
"found {} memory regions, {} free regions ({} bytes)",
regions,
free_regions,
free_bytes,
);
}

A::init_interrupts(bootinfo);

#[cfg(test)]
test_main();

// if this function returns we would boot loop. Hang, instead, so the debug
// output can be read.
//
// eventually we'll call into a kernel main loop here...
#[allow(clippy::empty_loop)]
loop {}
}

#[cfg(test)]
fn test_runner(tests: &[&mycelium_util::testing::TestCase]) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take it or leave it: I feel like a large chunk of this could go in the mycelium_util::testing module and then we could just implement the qemu exit stuff here? not a blocker though.

let span = tracing::info_span!("==== running tests ====", count = tests.len());
let _enter = span.enter();

let mut fails = 0;
for test in tests {
let span = tracing::info_span!("running test", test.name);
let _enter = span.enter();
Comment on lines +90 to +91
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, TIOLI: it seems like this is something the test-case macro could generate?

match (test.func)() {
Ok(_) => tracing::info!(test.name, "TEST OK"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/TIOLI: since test.name is part of the parent span here, I am not convinced we need to include it in these messages as well. up to you.

Err(_) => {
fails += 1;
tracing::error!(test.name, "TEST FAIL");
}
}
}

let exit_code;
if fails == 0 {
tracing::info!("Tests OK");
exit_code = arch::QemuExitCode::Success;
} else {
tracing::error!(fails, "Some Tests FAILED");
exit_code = arch::QemuExitCode::Failed;
}

arch::qemu_exit(exit_code);
panic!("failed to exit qemu");
}

#[global_allocator]
pub static GLOBAL: mycelium_alloc::Alloc = mycelium_alloc::Alloc;

#[alloc_error_handler]
fn alloc_error(layout: core::alloc::Layout) -> ! {
panic!("alloc error: {:?}", layout);
}

#[cfg(target_os = "none")]
#[panic_handler]
Expand Down Expand Up @@ -43,6 +156,28 @@ fn panic(panic: &core::panic::PanicInfo) -> ! {
arch::oops(&pp)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should add a line to arch::oops like

    #[cfg(test)]
    qemu_exit(QemuExitCode::Failed);

since the current oops handler will display the oops screen and then hang so the user can read the oops screen. this means that if a test fails by panicking or by triggering a CPU fault, rather than exiting and reporting that the test failed, we will hang forever (which we especially don't want in CI). if we are going to fail tests using the Rust assert macro, we definitely need to exit QEMU in the panic handler.

alternatively, we could put #[cfg(not(test))] on the call to arch::oops here, and have the panic handler exit qemu when running tests, but that means that CPU exceptions would still not result in normal test failures...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wanted to do this, but didn't in the first version, as your oops handler hadn't been written yet, and I didn't want to make my changes conflict :-)

}

#[mycelium_util::test]
fn test_alloc() {
// Let's allocate something, for funsies
let mut v = Vec::new();
tracing::info!(vec = ?v, vec.addr = ?v.as_ptr());
v.push(5u64);
tracing::info!(vec = ?v, vec.addr = ?v.as_ptr());
v.push(10u64);
tracing::info!(vec = ?v, vec.addr = ?v.as_ptr());
assert_eq!(v.pop(), Some(10));
assert_eq!(v.pop(), Some(5));
}


#[mycelium_util::test]
fn test_wasm() {
match wasm::run_wasm(HELLOWORLD_WASM) {
Ok(()) => tracing::info!("wasm test Ok!"),
Err(err) => tracing::error!(?err, "wasm test Err"),
}
Comment on lines +175 to +178
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since we already log the output of the test case in the test harness wrapper, can't this just be

#[mycelium_util::test]
fn test_wasm() {
    wasm::run_wasm(HELLOWORLD_WASM)
}

}
Comment on lines +159 to +179
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit/TIOLI: shouldn't these go in a test module?


fn main() {
unsafe {
core::hint::unreachable_unchecked();
Expand Down
14 changes: 14 additions & 0 deletions util-macros/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[package]
name = "mycelium-util-macros"
version = "0.1.0"
authors = ["Nika Layzell <[email protected]>"]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[lib]
proc-macro = true

[dependencies]
syn = { version = "1.0", features = ["full"] }
proc-macro2 = "1.0"
quote = "1.0"
Loading