Implement open_protocol, use it to fix flaky screenshot test (#318)

Author: nicholasbishop

src/table/boot.rs

@@ -123,8 +123,20 @@ pub struct BootServices {
  disconnect_controller: usize,
 
  // Protocol open / close services
- open_protocol: usize,
- close_protocol: usize,
+ open_protocol: extern "efiapi" fn(
+ handle: Handle,
+ protocol: &Guid,
+ interface: &mut *mut c_void,
+ agent_handle: Handle,
+ controller_handle: Option<Handle>,
+ attributes: u32,
+ ) -> Status,
+ close_protocol: extern "efiapi" fn(
+ handle: Handle,
+ protocol: &Guid,
+ agent_handle: Handle,
+ controller_handle: Option<Handle>,
+ ) -> Status,
  open_protocol_information: usize,
 
  // Library services
@@ -481,10 +493,16 @@ impl BootServices {
  /// This function attempts to get the protocol implementation of a handle,
  /// based on the protocol GUID.
  ///
+ /// It is recommended that all new drivers and applications use
+ /// [`open_protocol`] instead of `handle_protocol`.
+ ///
  /// UEFI protocols are neither thread-safe nor reentrant, but the firmware
  /// provides no mechanism to protect against concurrent usage. Such
  /// protections must be implemented by user-level code, for example via a
  /// global `HashSet`.
+ ///
+ /// [`open_protocol`]: BootServices::open_protocol
+ #[deprecated(note = "it is recommended to use `open_protocol` instead")]
  pub fn handle_protocol<P: Protocol>(&self, handle: Handle) -> Result<&UnsafeCell<P>> {
  let mut ptr = ptr::null_mut();
  (self.handle_protocol)(handle, &P::GUID, &mut ptr).into_with_val(|| {
@@ -672,6 +690,67 @@ impl BootServices {
  unsafe { (self.set_watchdog_timer)(timeout, watchdog_code, data_len, data) }.into()
  }
 
+ /// Open a protocol interface for a handle.
+ ///
+ /// This function attempts to get the protocol implementation of a
+ /// handle, based on the protocol GUID. It is an extended version of
+ /// [`handle_protocol`]. It is recommended that all
+ /// new drivers and applications use `open_protocol` instead of
+ /// `handle_protocol`.
+ ///
+ /// See [`OpenProtocolParams`] and [`OpenProtocolAttributes`] for
+ /// details of the input parameters.
+ ///
+ /// If successful, a [`ScopedProtocol`] is returned that will
+ /// automatically close the protocol interface when dropped.
+ ///
+ /// UEFI protocols are neither thread-safe nor reentrant, but the firmware
+ /// provides no mechanism to protect against concurrent usage. Such
+ /// protections must be implemented by user-level code, for example via a
+ /// global `HashSet`.
+ ///
+ /// [`handle_protocol`]: BootServices::handle_protocol
+ pub fn open_protocol<P: Protocol>(
+ &self,
+ params: OpenProtocolParams,
+ attributes: OpenProtocolAttributes,
+ ) -> Result<ScopedProtocol<P>> {
+ let mut interface = ptr::null_mut();
+ (self.open_protocol)(
+ params.handle,
+ &P::GUID,
+ &mut interface,
+ params.agent,
+ params.controller,
+ attributes as u32,
+ )
+ .into_with_val(|| {
+ let interface = interface as *mut P as *mut UnsafeCell<P>;
+ unsafe {
+ ScopedProtocol {
+ interface: &*interface,
+ open_params: params,
+ boot_services: self,
+ }
+ }
+ })
+ }
+
+ /// Test whether a handle supports a protocol.
+ pub fn test_protocol<P: Protocol>(&self, params: OpenProtocolParams) -> Result<()> {
+ const TEST_PROTOCOL: u32 = 0x04;
+ let mut interface = ptr::null_mut();
+ (self.open_protocol)(
+ params.handle,
+ &P::GUID,
+ &mut interface,
+ params.agent,
+ params.controller,
+ TEST_PROTOCOL,
+ )
+ .into_with_val(|| ())
+ }
+
  /// Get the list of protocol interface [`Guids`][Guid] that are installed
  /// on a [`Handle`].
  pub fn protocols_per_handle(&self, handle: Handle) -> Result<ProtocolsPerHandle> {
@@ -770,24 +849,45 @@ impl BootServices {
  pub fn get_image_file_system(
  &self,
  image_handle: Handle,
- ) -> Result<&UnsafeCell<SimpleFileSystem>> {
+ ) -> Result<ScopedProtocol<SimpleFileSystem>> {
  let loaded_image = self
- .handle_protocol::<LoadedImage>(image_handle)?
+ .open_protocol::<LoadedImage>(
+ OpenProtocolParams {
+ handle: image_handle,
+ agent: image_handle,
+ controller: None,
+ },
+ OpenProtocolAttributes::Exclusive,
+ )?
  .expect("Failed to retrieve `LoadedImage` protocol from handle");
- let loaded_image = unsafe { &*loaded_image.get() };
+ let loaded_image = unsafe { &*loaded_image.interface.get() };
 
  let device_handle = loaded_image.device();
 
  let device_path = self
- .handle_protocol::<DevicePath>(device_handle)?
+ .open_protocol::<DevicePath>(
+ OpenProtocolParams {
+ handle: device_handle,
+ agent: image_handle,
+ controller: None,
+ },
+ OpenProtocolAttributes::Exclusive,
+ )?
  .expect("Failed to retrieve `DevicePath` protocol from image's device handle");
- let mut device_path = unsafe { &*device_path.get() };
+ let mut device_path = unsafe { &*device_path.interface.get() };
 
  let device_handle = self
  .locate_device_path::<SimpleFileSystem>(&mut device_path)?
  .expect("Failed to locate `SimpleFileSystem` protocol on device path");
 
- self.handle_protocol::<SimpleFileSystem>(device_handle)
+ self.open_protocol::<SimpleFileSystem>(
+ OpenProtocolParams {
+ handle: device_handle,
+ agent: image_handle,
+ controller: None,
+ },
+ OpenProtocolAttributes::Exclusive,
+ )
  }
 }
 
@@ -962,6 +1062,97 @@ impl Drop for TplGuard<'_> {
  }
 }
 
+// OpenProtocolAttributes is safe to model as a regular enum because it
+// is only used as an input. The attributes are bitflags, but all valid
+// combinations are listed in the spec and only ByDriver and Exclusive
+// can actually be combined.
+//
+// Some values intentionally excluded:
+//
+// ByHandleProtocol (0x01) excluded because it is only intended to be
+// used in an implementation of `HandleProtocol`.
+//
+// TestProtocol (0x04) excluded because it doesn't actually open the
+// protocol, just tests if it's present on the handle. Since that
+// changes the interface significantly, that's exposed as a separate
+// method: `BootServices::test_protocol`.
+
+/// Attributes for [`BootServices::open_protocol`].
+#[repr(u32)]
+pub enum OpenProtocolAttributes {
+ /// Used by drivers to get a protocol interface for a handle. The
+ /// driver will not be informed if the interface is uninstalled or
+ /// reinstalled.
+ GetProtocol = 0x02,
+
+ /// Used by bus drivers to show that a protocol is being used by one
+ /// of the child controllers of the bus.
+ ByChildController = 0x08,
+
+ /// Used by a driver to gain access to a protocol interface. When
+ /// this mode is used, the driver's `Stop` function will be called
+ /// if the protocol interface is reinstalled or uninstalled. Once a
+ /// protocol interface is opened with this attribute, no other
+ /// drivers will be allowed to open the same protocol interface with
+ /// the `ByDriver` attribute.
+ ByDriver = 0x10,
+
+ /// Used by a driver to gain exclusive access to a protocol
+ /// interface. If any other drivers have the protocol interface
+ /// opened with an attribute of `ByDriver`, then an attempt will be
+ /// made to remove them with `DisconnectController`.
+ ByDriverExclusive = 0x30,
+
+ /// Used by applications to gain exclusive access to a protocol
+ /// interface. If any drivers have the protocol opened with an
+ /// attribute of `ByDriver`, then an attempt will be made to remove
+ /// them by calling the driver's `Stop` function.
+ Exclusive = 0x20,
+}
+
+/// Parameters passed to [`BootServices::open_protocol`].
+pub struct OpenProtocolParams {
+ /// The handle for the protocol to open.
+ pub handle: Handle,
+
+ /// The handle of the calling agent. For drivers, this is the handle
+ /// containing the `EFI_DRIVER_BINDING_PROTOCOL` instance. For
+ /// applications, this is the image handle.
+ pub agent: Handle,
+
+ /// For drivers, this is the controller handle that requires the
+ /// protocol interface. For applications this should be set to
+ /// `None`.
+ pub controller: Option<Handle>,
+}
+
+/// An open protocol interface. Automatically closes the protocol
+/// interface on drop.
+pub struct ScopedProtocol<'a, P: Protocol> {
+ /// The protocol interface.
+ pub interface: &'a UnsafeCell<P>,
+
+ open_params: OpenProtocolParams,
+ boot_services: &'a BootServices,
+}
+
+impl<'a, P: Protocol> Drop for ScopedProtocol<'a, P> {
+ fn drop(&mut self) {
+ let status = (self.boot_services.close_protocol)(
+ self.open_params.handle,
+ &P::GUID,
+ self.open_params.agent,
+ self.open_params.controller,
+ );
+ // All of the error cases for close_protocol boil down to
+ // calling it with a different set of parameters than what was
+ // passed to open_protocol. The public API prevents such errors,
+ // and the error can't be propagated out of drop anyway, so just
+ // assert success.
+ assert_eq!(status, Status::SUCCESS);
+ }
+}
+
 /// Type of allocation to perform.
 #[derive(Debug, Copy, Clone)]
 pub enum AllocateType {

uefi-test-runner/src/main.rs

@@ -12,7 +12,7 @@ use alloc::string::String;
 use core::mem;
 use uefi::prelude::*;
 use uefi::proto::console::serial::Serial;
-use uefi::table::boot::MemoryDescriptor;
+use uefi::table::boot::{MemoryDescriptor, OpenProtocolAttributes, OpenProtocolParams};
 
 mod boot;
 mod proto;
@@ -77,13 +77,25 @@ fn check_revision(rev: uefi::table::Revision) {
 /// This functionality is very specific to our QEMU-based test runner. Outside
 /// of it, we just pause the tests for a couple of seconds to allow visual
 /// inspection of the output.
-fn check_screenshot(bt: &BootServices, name: &str) {
+fn check_screenshot(image: Handle, bt: &BootServices, name: &str) {
  if cfg!(feature = "qemu") {
- // Access the serial port (in a QEMU environment, it should always be there)
+ let serial_handle = *bt
+ .find_handles::<Serial>()
+ .expect_success("Failed to get serial handles")
+ .first()
+ .expect("Could not find serial port");
+
  let serial = bt
- .locate_protocol::<Serial>()
- .expect_success("Could not find serial port");
- let serial = unsafe { &mut *serial.get() };
+ .open_protocol::<Serial>(
+ OpenProtocolParams {
+ handle: serial_handle,
+ agent: image,
+ controller: None,
+ },
+ OpenProtocolAttributes::Exclusive,
+ )
+ .expect_success("Could not open serial protocol");
+ let serial = unsafe { &mut *serial.interface.get() };
 
  // Set a large timeout to avoid problems with Travis
  let mut io_mode = *serial.io_mode();

uefi-test-runner/src/proto/console/gop.rs

@@ -2,7 +2,7 @@ use uefi::prelude::*;
 use uefi::proto::console::gop::{BltOp, BltPixel, FrameBuffer, GraphicsOutput, PixelFormat};
 use uefi::table::boot::BootServices;
 
-pub fn test(bt: &BootServices) {
+pub fn test(image: Handle, bt: &BootServices) {
  info!("Running graphics output protocol test");
  if let Ok(gop) = bt.locate_protocol::<GraphicsOutput>() {
  let gop = gop.expect("Warnings encountered while opening GOP");
@@ -12,7 +12,7 @@ pub fn test(bt: &BootServices) {
  fill_color(gop);
  draw_fb(gop);
 
- crate::check_screenshot(bt, "gop_test");
+ crate::check_screenshot(image, bt, "gop_test");
  } else {
  // No tests can be run.
  warn!("UEFI Graphics Output Protocol is not supported");

uefi-test-runner/src/proto/console/mod.rs

@@ -1,13 +1,13 @@
 use uefi::prelude::*;
 
-pub fn test(st: &mut SystemTable<Boot>) {
+pub fn test(image: Handle, st: &mut SystemTable<Boot>) {
  info!("Testing console protocols");
 
  stdout::test(st.stdout());
 
  let bt = st.boot_services();
  serial::test(bt);
- gop::test(bt);
+ gop::test(image, bt);
  pointer::test(bt);
 }
 

uefi-test-runner/src/proto/debug.rs

@@ -1,18 +1,25 @@
 use core::ffi::c_void;
 use uefi::proto::debug::{DebugSupport, ExceptionType, ProcessorArch, SystemContext};
-use uefi::table::boot::BootServices;
-use uefi::ResultExt;
+use uefi::table::boot::{BootServices, OpenProtocolAttributes, OpenProtocolParams};
+use uefi::{Handle, ResultExt};
 
-pub fn test(bt: &BootServices) {
+pub fn test(image: Handle, bt: &BootServices) {
  info!("Running UEFI debug connection protocol test");
  if let Ok(handles) = bt.find_handles::<DebugSupport>() {
  let handles = handles.expect("Problem encountered while querying handles for DebugSupport");
 
  for handle in handles {
- if let Ok(debug_support) = bt.handle_protocol::<DebugSupport>(handle) {
+ if let Ok(debug_support) = bt.open_protocol::<DebugSupport>(
+ OpenProtocolParams {
+ handle,
+ agent: image,
+ controller: None,
+ },
+ OpenProtocolAttributes::Exclusive,
+ ) {
  let debug_support = debug_support
  .expect("Warnings encountered while opening debug support protocol");
- let debug_support = unsafe { &mut *debug_support.get() };
+ let debug_support = unsafe { &mut *debug_support.interface.get() };
 
  // make sure that the max processor index is a sane value, i.e. it works
  let maximum_processor_index = debug_support.get_maximum_processor_index();