Implement missing Event-related functions

[timrobertsdev]

This is currently a draft; there are a few things I'm still unsure about or want to refine, but comments are more than welcome.

I've implemented the missing close_event(), signal_event(), and create_event_ex() functions. I've also adjusted the create_event() implementation to allow passing of the notify_ctx parameter, which is an opaque pointer to a struct that the notify_fn needs, commonly some kind of protocol interface. There is a new test, test_callback_with_ctx() that demonstrates this.

You'll see a few instances where I've used an Option<NonNull<T>> instead of Option<*mut T>, this is so we ensure that the None variant is equivalent to null, thus FFI-safe.

There is an issue I'm having with the safety around close_event(). Once an event is closed, the specification says that it and any reference to it are invalid. I'm not sure about other fw implementation, but EDK2/OVMF actually free the memory for an Event during this call. I've marked the function as unsafe and have noted this in the safety docs, but is there a way to leverage the type system here instead of trusting the caller to never use that Event again? I had thought about removing Copy from Event so that it has move semantics, but that would require quite a bit of refactoring and I'm not entirely confident it would actually work.

Thank you for looking at this! I've been having a lot of fun hacking on this project! :)

[nicholasbishop]

I had thought about removing Copy from Event so that it has move semantics, but that would require quite a bit of refactoring and I'm not entirely confident it would actually work.

I think that sounds like the right approach. Here's a quick sketch of what I think that could look like:

diff --git a/src/data_types/mod.rs b/src/data_types/mod.rs
index 4a84700..f3642f6 100644
--- a/src/data_types/mod.rs
+++ b/src/data_types/mod.rs
@@ -16,10 +16,16 @@ impl Handle {
 }
 
 /// Handle to an event structure
-#[derive(Clone, Copy)]
 #[repr(transparent)]
 pub struct Event(*mut c_void);
 
+impl Event {
+    /// TODO comment
+    pub unsafe fn unsafe_clone(&self) -> Event {
+        Event(self.0)
+    }
+}
+
 /// Trait for querying the alignment of a struct
 ///
 /// Needed for dynamic-sized types because `mem::align_of` has a `Sized` bound (due to `dyn Trait`)
diff --git a/src/proto/console/pointer/mod.rs b/src/proto/console/pointer/mod.rs
index 9c08881..1781af8 100644
--- a/src/proto/console/pointer/mod.rs
+++ b/src/proto/console/pointer/mod.rs
@@ -47,8 +47,8 @@ impl<'boot> Pointer<'boot> {
 
     /// Event to be used with `BootServices::wait_for_event()` in order to wait
     /// for input from the pointer device
-    pub fn wait_for_input_event(&self) -> Event {
-        self.wait_for_input
+    pub fn wait_for_input_event(&self) -> &Event {
+        &self.wait_for_input
     }
 
     /// Returns a reference to the pointer device information.
diff --git a/src/proto/console/text/input.rs b/src/proto/console/text/input.rs
index e2d28f5..c0cdbeb 100644
--- a/src/proto/console/text/input.rs
+++ b/src/proto/console/text/input.rs
@@ -44,8 +44,8 @@ impl Input {
 
     /// Event to be used with `BootServices::wait_for_event()` in order to wait
     /// for a key to be available
-    pub fn wait_for_key_event(&self) -> Event {
-        self.wait_for_key
+    pub fn wait_for_key_event(&self) -> &Event {
+        &self.wait_for_key
     }
 }
 
diff --git a/src/table/boot.rs b/src/table/boot.rs
index bcf9cc3..46e271c 100644
--- a/src/table/boot.rs
+++ b/src/table/boot.rs
@@ -387,13 +387,13 @@ impl BootServices {
     }
 
     /// Sets the trigger for `EventType::TIMER` event.
-    pub fn set_timer(&self, event: Event, trigger_time: TimerTrigger) -> Result {
+    pub fn set_timer(&self, event: &Event, trigger_time: TimerTrigger) -> Result {
         let (ty, time) = match trigger_time {
             TimerTrigger::Cancel => (0, 0),
             TimerTrigger::Periodic(hundreds_ns) => (1, hundreds_ns),
             TimerTrigger::Relative(hundreds_ns) => (2, hundreds_ns),
         };
-        unsafe { (self.set_timer)(event, ty, time) }.into()
+        unsafe { (self.set_timer)(event.unsafe_clone(), ty, time) }.into()
     }
 
     /// Stops execution until an event is signaled.
@@ -461,12 +461,8 @@ impl BootServices {
     ///
     /// Note: The UEFI Specification v2.9 states that this may only return `EFI_SUCCESS`, but,
     /// at least for application based on EDK2 (such as OVMF), it may also return `EFI_INVALID_PARAMETER`.
-    ///
-    /// # Safety
-    /// Once the event is closed, it is no longer valid and may not be used again. The firmware
-    /// implementation will have `free`'d the event's memory.
-    pub unsafe fn close_event(&self, event: Event) -> Result {
-        (self.close_event)(event).into()
+    pub fn close_event(&self, event: Event) -> Result {
+        unsafe { (self.close_event)(event).into() }
     }
 
     /// Checks to see if an event is signaled, without blocking execution to wait for it.
diff --git a/uefi-test-runner/src/boot/misc.rs b/uefi-test-runner/src/boot/misc.rs
index 33aa382..f429e7a 100644
--- a/uefi-test-runner/src/boot/misc.rs
+++ b/uefi-test-runner/src/boot/misc.rs
@@ -18,9 +18,9 @@ pub fn test(bt: &BootServices) {
 fn test_timer(bt: &BootServices) {
     let timer_event = unsafe { bt.create_event(EventType::TIMER, Tpl::APPLICATION, None, None) }
         .expect_success("Failed to create TIMER event");
-    let mut events = [timer_event];
-    bt.set_timer(timer_event, TimerTrigger::Relative(5_0 /*00 ns */))
+    bt.set_timer(&timer_event, TimerTrigger::Relative(5_0 /*00 ns */))
         .expect_success("Failed to set timer");
+    let mut events = [timer_event];
     bt.wait_for_event(&mut events)
         .expect_success("Wait for event failed");
 }

If that solution turns out to be insufficient for some reason though, I think it would be OK to leave close_event as unsafe for now.

[nicholasbishop]

I'm curious about the reason for changing from a regular fn here and removing the trampoline. It seems less convenient for callers, is it necessary for safety or something?

If the change is necessary, could simplify a bit here and a couple other places with the EventNotifyFn type.

[timrobertsdev]

In the original function, I couldn't see a way to pass a notification context pointer, as noted in issue #292 (I probably should've copied the text here after I closed the issue). After modifying the function to take a notification context so UEFI will pass it to the notification function, I didn't see the need for the trampoline any longer.

This might come down to policy for uefi-rs? Should we make it as convenient as possible for callers by letting them pass a regular rust fn and then trampoline it to an extern "efiapi" fn() ourselves? I can see the arguments for it, if that's what the policy is I'll need to go in and change some things in the DebugSupport protocol, but there's no issue there.

You're right that I can clean up the signature by using EventNotifyFn, there's a few places I need to do that still.

Text of issue #292:

Hey, I'm not quite able to wrap my head around the current implementation of BootServices::create_event(). How would we properly pass context into the notification function, such as in this example?

Based on that example, the notification context is intended to be an opaque pointer to whatever the function requires, which the notification function will cast back to the concrete type. In this case, the SimpleTextInputEx protocol. I currently don't see a way to pass in any context when creating an event.

Example from EDK2 docs that passes context: https://edk2-docs.gitbook.io/edk-ii-uefi-driver-writer-s-guide/5_uefi_services/51_services_that_uefi_drivers_commonly_use/515_event_services#example-47-create-and-close-a-wait-event

[timrobertsdev]

I was also considering taking a Box<Protocol> (or similar) for notify_ctx and converting that to a Some(NonNull<c_void>) instead of making the caller do it, but I had two reservations:

1. The UEFI spec isn't exactly specific on what notify_ctx can be. From their examples and source code, it appears to always be a Protocol, but it's not explicit in the docs. Theoretically, I guess it could be anything with a lifetime matching or exceeding that of the notify_fn.
2. Is it safe to convert a trait object to a single pointer in Rust? I've seen a few examples involving Box<Box<trait T>> and Box::from_raw(), but I feel that I need to dive into it a bit more to be sure.

[timrobertsdev]

Actually, I don't believe it's possible to keep the trampoline, as it needs to capture the notify_fn parameter and we can't have an extern "efiapi" closure.

Removing Copy and Clone from Event went swimmingly with your suggestions, thank you!

I have one more question about the create_event_ex function if you have time. The UEFI specification has a number of validations that the firmware implementation runs on the parameters, should we validate those as well, before calling the UEFI function? Or would it be better to let UEFI do it's thing and return any errors that it spits out? If we do validation ourselves we could return a better error message as a payload alongside Status::INVALID_PARAMETER and the overhead seems reasonable.

[nicholasbishop]

Actually, I don't believe it's possible to keep the trampoline, as it needs to capture the notify_fn parameter and we can't have an extern "efiapi" closure.

I see what you mean, that makes sense.

I have one more question about the create_event_ex function if you have time. The UEFI specification has a number of validations that the firmware implementation runs on the parameters, should we validate those as well, before calling the UEFI function? Or would it be better to let UEFI do it's thing and return any errors that it spits out? If we do validation ourselves we could return a better error message as a payload alongside Status::INVALID_PARAMETER and the overhead seems reasonable.

I don't feel strongly either way, but I think it's probably fine to not do extra validation unless we find that real-world firmware isn't checking something properly.

[timrobertsdev]

My last commit failed one of the ui tests on the CI, due to this warning being outputted before the error output:
WARN rustc_metadata::locator no metadata found: failed to mmap rmeta metadata: '$WORKSPACE/target/tests/target/x86_64-unknown-linux-gnu/debug/deps/libuefi_macros_tests-b483c565bfc18a64.rmeta'

I don't believe this is related to my changes and may be a transient error. I can't reproduce it on my machine.

EDIT: Reproduceable with a fresh copy of master. Opening an issue.

[nicholasbishop]

Looks good to me, thanks for the PR!