Skip to content

Add set_watchdog_timer implementation and test #27

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

Merged
merged 1 commit into from
Sep 22, 2018
Merged

Add set_watchdog_timer implementation and test #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion src/table/boot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub struct BootServices {
// Misc services
get_next_monotonic_count: usize,
stall: extern "C" fn(usize) -> Status,
set_watchdog_timer: usize,
set_watchdog_timer: extern "C" fn(timeout: usize, watchdog_code: u64, data_size: usize, watchdog_data: *mut u16) -> Status,

// Driver support services
connect_controller: usize,
Expand Down Expand Up @@ -236,6 +236,11 @@ impl BootServices {
assert_eq!((self.stall)(time), Status::Success);
}

/// Set the watchdog timer.
pub fn set_watchdog_timer(&self, timeout: usize, watchdog_code: u64, data_size: usize, watchdog_data: *mut u16) {
Copy link
Contributor

@HadrienG2 HadrienG2 Sep 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This method should probably be unsafe since 1/it takes an unchecked pointer as a parameter and 2/data_size is not related in any way to the watchdog_data pointer so safety-critical caller mistakes can happen here as well.

Copy link
Contributor

@HadrienG2 HadrienG2 Sep 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

A safe alternative would be to pass in a slice of u16s.

assert_eq!((self.set_watchdog_timer)(timeout, watchdog_code, data_size, watchdog_data), Status::Success);
Copy link
Contributor

@HadrienG2 HadrienG2 Sep 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This method can return status codes other than Success if the wrong parameters are passed in or the system does not have a watchdog timer, therefore a Result should be propagated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GabrielMajeri Already applied some changes on master :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotta go fast! 😄

}

/// Copies memory from source to destination. The buffers can overlap.
pub fn memmove(&self, dest: *mut u8, src: *const u8, size: usize) {
(self.copy_mem)(dest, src, size);
Expand Down
10 changes: 10 additions & 0 deletions uefi-test-runner/src/boot/misc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
use uefi::table::boot::BootServices;
use core::ptr;

pub fn test(bt: &BootServices) {
test_watchdog(bt);
}

fn test_watchdog(bt: &BootServices) {
bt.set_watchdog_timer(0, 0, 0, ptr::null_mut());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a more minor point, but I'm not sure if the UEFI spec allows you to use 0 as a watchdog code. It does say that codes from 0 to 0xffff are reserved by the implementation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firmware is required to set a 5-minute watchdog timer before running an image, setting the code 0 to 0 disables it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can find a more detailed explanation here: https://wiki.osdev.org/UEFI#My_UEFI_application_hangs.2Fresets_after_about_5_minutes

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the UEFI spec, it is the timeout parameter that must be set to zero to disable the watchdog timer. The WatchdogCode is something that will be logged on a timeout event. Now, since we're disabling the timeout, that will never happen, but an UEFI implementation that carefully validates its inputs could still complain about our use of a reserved code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The rather hypothetical nature of an UEFI implementation that checks its inputs so carefully is why I classified this as a less pressing issue 😄

Copy link
Contributor

@HadrienG2 HadrienG2 Sep 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I opened #28 to resolve this.

}
2 changes: 2 additions & 0 deletions uefi-test-runner/src/boot/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ use uefi::table::boot::BootServices;

pub fn test(bt: &BootServices) {
memory::test(bt);
misc::test(bt);
}

mod memory;
mod misc;