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.

Sign up for GitHub

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

Fix the bootloader hanging during uefi exit_boot_services #457

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix the bootloader hanging during uefi exit_boot_services #457

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b8e2b9b
Run test with 8 processors
Wasabi375 Sep 16, 2024
e41c6c1
Update uefi crate from 0.20 to 0.32
Wasabi375 Sep 16, 2024
0075368
Start qemu uefi using pflash instead of bios
Wasabi375 Sep 17, 2024
034ddb6
Remove forced timeout in tests
Wasabi375 Sep 17, 2024
1ef3241
Remove error handling dependencies from tests
Wasabi375 Sep 17, 2024
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
10 changes: 10 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tests/runner/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ uefi = ["bootloader/uefi", "dep:ovmf-prebuilt"]
bootloader = { path = "../..", default-features = false }
strip-ansi-escapes = "0.1.1"
ovmf-prebuilt = { version = "0.1.0-alpha.1", optional = true }
wait-timeout = "0.2.0"
16 changes: 13 additions & 3 deletions tests/runner/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,11 @@ where
use std::{
io::Read,
process::{Command, Stdio},
time::Duration,
};

use wait_timeout::ChildExt;

const QEMU_ARGS: &[&str] = &[
"-device",
"isa-debug-exit,iobase=0xf4,iosize=0x04",
Expand All @@ -105,6 +108,8 @@ where
"-display",
"none",
"--no-reboot",
"-smp",
"8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Blog os at least doesn't support more than 1 core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why that matters here. This is just for the tests in this repo and has no impact on any kernel build with this bootloader. Also correct me if I am wrong but Blog Os should work fine on a multicore system. It will use just 1, but the other cores don't do anything unless specifically started by the kernel.

];

const SEPARATOR: &str = "\n____________________________________\n";
Expand Down Expand Up @@ -138,10 +143,15 @@ where
)
});

let exit_status = child.wait().unwrap();
let Some(exit_status) = child.wait_timeout(Duration::new(120, 0)).unwrap() else {
child
.kill()
.expect("Qemu could not be killed after timeout");
panic!("Test timed out after 2 minutes");
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Rust's default test runner never times out. At worst it shows a warning after 60s. A timeout means that if you suspend the testing for longer than 120s using ctrl-z, you get an unnnecessary test failure. Also depending on how complex the tests of your OS are you may genuinely need more than 120s on low-end hardware. On local systems you can always kill the test manually and on CI there should be a timeout from the CI provider anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind removing this. I don't think the crtl-z case is common, but maybe that just comes down to preference. I never use that myself.
I mostly added this so I could easily run the tests locally in the background and get notified about failures.
This bug is random in nature so I ran the tests locally in a loop so I needed some kind of failure state.

As for CI the timeout is 30minutes and that seems excessive. Especially when like here the CPU is maxed out by the hanging test.

Also depending on how complex the tests of your OS are you may genuinely need more than 120s on low-end hardware.

That might actually explain the failing CI tests. So maybe I should remove this.

match exit_status.code() {
Some(33) => {} // success
Some(35) => panic!("Test failed"), // success
Some(33) => {} // success
Some(35) => panic!("Test failed"),
other => panic!("Test failed with unexpected exit code `{other:?}`"),
}

Expand Down