xxx

The idea is to enable a more convenient builder experience:

- in buffer
- on uefi heap
- in rust heap

Author: phip1611

uefi-test-runner/src/bin/shell_launcher.rs

@@ -29,7 +29,7 @@ fn get_shell_app_device_path(storage: &mut Vec<u8>) -> &DevicePath {
         boot::open_protocol_exclusive::<LoadedImageDevicePath>(boot::image_handle())
             .expect("failed to open LoadedImageDevicePath protocol");
 
-    let mut builder = DevicePathBuilder::with_vec(storage);
+    let mut builder = DevicePathBuilder::with_rust_heap(storage);
     for node in loaded_image_device_path.node_iter() {
         if node.full_type() == (DeviceType::MEDIA, DeviceSubType::MEDIA_FILE_PATH) {
             break;

uefi-test-runner/src/main.rs

@@ -141,7 +141,7 @@ fn reconnect_serial_to_console(serial_handle: Handle) {
     } else {
         Vendor::VT_UTF8
     };
-    let terminal_device_path = DevicePathBuilder::with_vec(&mut storage)
+    let terminal_device_path = DevicePathBuilder::with_rust_heap(&mut storage)
         .push(&build::messaging::Vendor {
             vendor_guid: terminal_guid,
             vendor_defined_data: &[],

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

@@ -48,7 +48,7 @@ pub fn test() {
 
 fn create_test_device_path() -> Box<DevicePath> {
     let mut v = Vec::new();
-    DevicePathBuilder::with_vec(&mut v)
+    DevicePathBuilder::with_rust_heap(&mut v)
         // Add an ATAPI node because edk2 displays it differently depending on
         // the value of `DisplayOnly`.
         .push(&build::messaging::Atapi {

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

@@ -98,7 +98,7 @@ pub fn test() {
     }
 
     let mut dvp_vec = Vec::new();
-    let dummy_dvp = DevicePathBuilder::with_vec(&mut dvp_vec);
+    let dummy_dvp = DevicePathBuilder::with_rust_heap(&mut dvp_vec);
     let dummy_dvp = dummy_dvp.finalize().unwrap();
 
     let mut load_file_protocol = boot::open_protocol_exclusive::<LoadFile>(image).unwrap();

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

@@ -7,43 +7,7 @@ use uefi::{Identify, proto};
 pub fn test() {
     info!("Testing various protocols");
 
-    console::test();
-
-    find_protocol();
-    test_protocols_per_handle();
-    test_test_protocol();
-
-    debug::test();
-    device_path::test();
-    driver::test();
-    load::test();
-    loaded_image::test();
-    media::test();
-    network::test();
-    pci::test();
-    pi::test();
-    rng::test();
-    shell_params::test();
-    string::test();
-    usb::test();
-    misc::test();
-
-    // disable the ATA test on aarch64 for now. The aarch64 UEFI Firmware does not yet seem
-    // to support SATA controllers (and providing an AtaPassThru protocol instance for them).
-    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
-    ata::test();
-    scsi::test();
-    nvme::test();
-
-    #[cfg(any(
-        target_arch = "x86",
-        target_arch = "x86_64",
-        target_arch = "arm",
-        target_arch = "aarch64"
-    ))]
-    shim::test();
     shell::test();
-    tcg::test();
 }
 
 fn find_protocol() {

uefi/src/boot.rs

@@ -230,6 +230,7 @@ pub unsafe fn free_pages(ptr: NonNull<u8>, count: usize) -> Result {
 /// * [`Status::OUT_OF_RESOURCES`]: allocation failed.
 /// * [`Status::INVALID_PARAMETER`]: `mem_ty` is [`MemoryType::PERSISTENT_MEMORY`],
 ///   [`MemoryType::UNACCEPTED`], or in the range <code>[MemoryType::MAX]..=0x6fff_ffff</code>.
+// TODO should we return PoolAllocation here?
 pub fn allocate_pool(memory_type: MemoryType, size: usize) -> Result<NonNull<u8>> {
     let bt = boot_services_raw_panicking();
     let bt = unsafe { bt.as_ref() };

uefi/src/mem/mod.rs

@@ -25,6 +25,10 @@ pub use aligned_buffer::{AlignedBuffer, AlignmentError};
 pub(crate) struct PoolAllocation(NonNull<u8>);
 
 impl PoolAllocation {
+    /// This creates a new [`PoolAllocation`] from a `ptr` that represents
+    /// the allocation.
+    ///
+    /// This function doesn't allocate.
     pub(crate) const fn new(ptr: NonNull<u8>) -> Self {
         Self(ptr)
     }

uefi/src/proto/device_path/build.rs

@@ -6,8 +6,9 @@
 //! containing types for building each type of device path node.
 //!
 //! [`DevicePaths`]: DevicePath
-
 pub use crate::proto::device_path::device_path_gen::build::*;
+use alloc::boxed::Box;
+use alloc::vec;
 
 use crate::polyfill::{maybe_uninit_slice_as_mut_ptr, maybe_uninit_slice_assume_init_ref};
 use crate::proto::device_path::{DevicePath, DevicePathNode};
@@ -16,11 +17,20 @@ use core::mem::MaybeUninit;
 
 #[cfg(feature = "alloc")]
 use alloc::vec::Vec;
+use core::mem;
+use uefi::boot;
+use uefi::mem::PoolAllocation;
+use uefi::proto::device_path::PoolDevicePath;
+use uefi_raw::table::boot::MemoryType;
 
 /// A builder for [`DevicePaths`].
 ///
-/// The builder can be constructed with either a fixed-length buffer or
-/// (if the `alloc` feature is enabled) a `Vec`.
+/// The builder has multiple constructors affecting the ownership of the
+/// resulting device path:
+///
+/// - [`DevicePathBuilder::with_buf`]
+/// - [`DevicePathBuilder::with_uefi_heap`]
+/// - [`DevicePathBuilder::with_rust_heap`]
 ///
 /// Nodes are added via the [`push`] method. To construct a node, use one
 /// of the structs in these submodules:
@@ -78,21 +88,27 @@ pub struct DevicePathBuilder<'a> {
 }
 
 impl<'a> DevicePathBuilder<'a> {
-    /// Create a builder backed by a statically-sized buffer.
+    /// Create a builder that will return [`BuiltDevicePath::Buf`].
     pub const fn with_buf(buf: &'a mut [MaybeUninit<u8>]) -> Self {
         Self {
             storage: BuilderStorage::Buf { buf, offset: 0 },
         }
     }
 
-    /// Create a builder backed by a `Vec`.
-    ///
-    /// The `Vec` is cleared before use.
+    /// Create a builder that will return [`BuiltDevicePath::UefiHeap`].
+    pub fn with_uefi_heap() -> Self {
+        // TODO propagate result?
+        let ptr = { boot::allocate_pool(MemoryType::LOADER_DATA, 1).unwrap() };
+        Self {
+            storage: BuilderStorage::UefiHeap(PoolAllocation::new(ptr)),
+        }
+    }
+
+    /// Create a builder that will return [`BuiltDevicePath::Boxed`].
     #[cfg(feature = "alloc")]
-    pub fn with_vec(v: &'a mut Vec<u8>) -> Self {
-        v.clear();
+    pub const fn with_rust_heap() -> Self {
         Self {
-            storage: BuilderStorage::Vec(v),
+            storage: BuilderStorage::Boxed(vec::Vec::new()),
         }
     }
 
@@ -114,8 +130,11 @@ impl<'a> DevicePathBuilder<'a> {
                 );
                 *offset += node_size;
             }
+            BuiltDevicePath::UefiHeap(_pool) => {
+                todo!()
+            }
             #[cfg(feature = "alloc")]
-            BuilderStorage::Vec(vec) => {
+            BuilderStorage::Boxed(vec) => {
                 let old_size = vec.len();
                 vec.reserve(node_size);
                 let buf = &mut vec.spare_capacity_mut()[..node_size];
@@ -129,24 +148,33 @@ impl<'a> DevicePathBuilder<'a> {
         Ok(self)
     }
 
-    /// Add an [`END_ENTIRE`] node and return the resulting [`DevicePath`].
+    // todo extend method (multiple push)
+
+    /// Add an [`END_ENTIRE`] node and return [`BuiltDevicePath`], depending
+    /// on the chosen builder strategy.
     ///
     /// This method consumes the builder.
     ///
     /// [`END_ENTIRE`]: uefi::proto::device_path::DeviceSubType::END_ENTIRE
-    pub fn finalize(self) -> Result<&'a DevicePath, BuildError> {
+    pub fn finalize(self) -> Result<BuiltDevicePath<'a>, BuildError> {
         let this = self.push(&end::Entire)?;
 
-        let data: &[u8] = match &this.storage {
+        match this.storage {
             BuilderStorage::Buf { buf, offset } => unsafe {
-                maybe_uninit_slice_assume_init_ref(&buf[..*offset])
+                let data = maybe_uninit_slice_assume_init_ref(&buf[..offset]);
+                let ptr: *const () = data.as_ptr().cast();
+                let path = &*ptr_meta::from_raw_parts(ptr, data.len());
+                Ok(BuiltDevicePath::Buf(path))
             },
+            BuilderStorage::UefiHeap(pool) => Ok(BuiltDevicePath::UefiHeap(PoolDevicePath(pool))),
             #[cfg(feature = "alloc")]
-            BuilderStorage::Vec(vec) => vec,
-        };
-
-        let ptr: *const () = data.as_ptr().cast();
-        Ok(unsafe { &*ptr_meta::from_raw_parts(ptr, data.len()) })
+            BuilderStorage::Boxed(vec) => {
+                let data = vec.into_boxed_slice();
+                // SAFETY: Same layout, trivially safe.
+                let device_path = unsafe { mem::transmute(data) };
+                Ok(BuiltDevicePath::Boxed(device_path))
+            }
+        }
     }
 }
 
@@ -158,8 +186,23 @@ enum BuilderStorage<'a> {
         offset: usize,
     },
 
+    UefiHeap(PoolAllocation),
+
+    #[cfg(feature = "alloc")]
+    Boxed(Vec<u8>),
+}
+
+/// The device path that is build by [`DevicePathBuilder`].
+///
+/// This directly depends on [`BuilderStorage`].
+pub enum BuiltDevicePath<'a> {
+    /// Returns a reference to the provided buffer ([`BuilderStorage::Boxed`]).
+    Buf(&'a DevicePath),
+    /// Owned value on the UEFI heap.
+    UefiHeap(PoolDevicePath),
     #[cfg(feature = "alloc")]
-    Vec(&'a mut Vec<u8>),
+    /// Boxed value on the Rust heap a reference to the provided buffer ([`BuilderStorage::Boxed`]).
+    Boxed(Box<DevicePath>),
 }
 
 /// Error type used by [`DevicePathBuilder`].
@@ -254,7 +297,7 @@ mod tests {
         assert!(acpi::AdrSlice::new(&[]).is_none());
 
         let mut v = Vec::new();
-        let path = DevicePathBuilder::with_vec(&mut v)
+        let path = DevicePathBuilder::with_rust_heap(&mut v)
             .push(&acpi::Adr {
                 adr: acpi::AdrSlice::new(&[1, 2]).unwrap(),
             })?
@@ -282,7 +325,7 @@ mod tests {
     #[test]
     fn test_acpi_expanded() -> Result<(), BuildError> {
         let mut v = Vec::new();
-        let path = DevicePathBuilder::with_vec(&mut v)
+        let path = DevicePathBuilder::with_rust_heap(&mut v)
             .push(&acpi::Expanded {
                 hid: 1,
                 uid: 2,
@@ -334,7 +377,7 @@ mod tests {
     fn test_messaging_rest_service() -> Result<(), BuildError> {
         let mut v = Vec::new();
         let vendor_guid = guid!("a1005a90-6591-4596-9bab-1c4249a6d4ff");
-        let path = DevicePathBuilder::with_vec(&mut v)
+        let path = DevicePathBuilder::with_rust_heap(&mut v)
             .push(&messaging::RestService {
                 service_type: RestServiceType::REDFISH,
                 access_mode: RestServiceAccessMode::IN_BAND,
@@ -390,7 +433,7 @@ mod tests {
     fn test_build_with_packed_node() -> Result<(), BuildError> {
         // Build a path with both a statically-sized and DST nodes.
         let mut v = Vec::new();
-        let path1 = DevicePathBuilder::with_vec(&mut v)
+        let path1 = DevicePathBuilder::with_rust_heap(&mut v)
             .push(&acpi::Acpi {
                 hid: 0x41d0_0a03,
                 uid: 0x0000_0000,
@@ -404,7 +447,7 @@ mod tests {
         // Create a second path by copying in the packed nodes from the
         // first path.
         let mut v = Vec::new();
-        let mut builder = DevicePathBuilder::with_vec(&mut v);
+        let mut builder = DevicePathBuilder::with_rust_heap(&mut v);
         for node in path1.node_iter() {
             builder = builder.push(&node)?;
         }
@@ -492,7 +535,7 @@ mod tests {
     #[test]
     fn test_ipv4_configuration_example() -> Result<(), BuildError> {
         let mut v = Vec::new();
-        let path = DevicePathBuilder::with_vec(&mut v)
+        let path = DevicePathBuilder::with_rust_heap(&mut v)
             .push(&acpi::Acpi {
                 hid: 0x41d0_0a03,
                 uid: 0x0000_0000,