Merge pull request #2048 from rust-osdev/spec-compliance-fixes-4

Spec Fixes: various smaller fixes regarding protocols

uefi-test-runner/src/proto/pi/mp.rs

@@ -4,7 +4,7 @@ use core::ffi::c_void;
 use core::ptr;
 use core::sync::atomic::{AtomicUsize, Ordering};
 use core::time::Duration;
-use uefi::proto::pi::mp::MpServices;
+use uefi::proto::pi::mp::{CPU_V2_EXTENDED_TOPOLOGY, MpServices};
 use uefi::{Status, boot};
 
 /// Number of cores qemu is configured to have
@@ -26,6 +26,7 @@ pub fn test() {
 
     test_get_number_of_processors(mp_support);
     test_get_processor_info(mp_support);
+    test_get_processor_info_extended(mp_support);
     test_startup_all_aps(mp_support);
     test_startup_this_ap(mp_support);
     test_enable_disable_ap(mp_support);
@@ -51,6 +52,19 @@ fn test_get_processor_info(mps: &MpServices) {
     let cpu1 = mps.get_processor_info(1).unwrap();
     let cpu2 = mps.get_processor_info(2).unwrap();
 
+    for cpu in [&cpu0, &cpu1, &cpu2] {
+        info!(
+            "CPU {}: bsp={}, enabled={}, healthy={}, location=(package={}, core={}, thread={})",
+            cpu.processor_id,
+            cpu.is_bsp(),
+            cpu.is_enabled(),
+            cpu.is_healthy(),
+            cpu.location.package,
+            cpu.location.core,
+            cpu.location.thread
+        );
+    }
+
     // Check that processor_id fields are sane
     assert_eq!(cpu0.processor_id, 0);
     assert_eq!(cpu1.processor_id, 1);
@@ -70,6 +84,46 @@ fn test_get_processor_info(mps: &MpServices) {
     mps.enable_disable_ap(1, true, None).unwrap();
 }
 
+/// Requesting the extended topology makes the firmware fill the whole
+/// spec-defined struct, including `extended_information`. This mainly
+/// guards the struct layout: if the Rust struct is too small, the
+/// firmware write corrupts the stack; if a field is misplaced, the
+/// value checks below fail.
+fn test_get_processor_info_extended(mps: &MpServices) {
+    for i in 0..NUM_CPUS {
+        let info = mps.get_processor_info(i).unwrap();
+        let ext_info = mps
+            .get_processor_info(CPU_V2_EXTENDED_TOPOLOGY | i)
+            .unwrap();
+
+        // Bit 24 is a flag, not part of the processor index.
+        assert_eq!(ext_info.processor_id, info.processor_id);
+
+        // Both locations are derived from the same APIC ID, and qemu
+        // reports a flat topology without module/tile/die levels, so the
+        // shared fields must match. All CPUs except one have a non-zero
+        // package or core number, so these checks also prove that the
+        // firmware really filled the pre-zeroed extended fields.
+        let location2 = &ext_info.extended_information;
+        info!(
+            "CPU {}: extended location: package={}, module={}, tile={}, die={}, core={}, thread={}",
+            ext_info.processor_id,
+            location2.package,
+            location2.module,
+            location2.tile,
+            location2.die,
+            location2.core,
+            location2.thread
+        );
+        assert_eq!(location2.package, info.location.package);
+        assert_eq!(location2.module, 0);
+        assert_eq!(location2.tile, 0);
+        assert_eq!(location2.die, 0);
+        assert_eq!(location2.core, info.location.core);
+        assert_eq!(location2.thread, info.location.thread);
+    }
+}
+
 extern "efiapi" fn proc_increment_atomic(arg: *mut c_void) {
     let counter: &AtomicUsize = unsafe { &*(arg as *const _) };
     counter.fetch_add(1, Ordering::Relaxed);

uefi-test-runner/src/proto/string/unicode_collation.rs

@@ -92,5 +92,14 @@ pub fn test() {
             uc.str_to_fat(s, &mut buf).unwrap(),
             CStr8::from_bytes_with_nul(b"HELLOWORLD!\0").unwrap()
         );
+
+        // The protocol writes only the converted characters (the space is
+        // skipped) and no NUL terminator. Ensure stale buffer content
+        // neither ends up in the result nor breaks its termination.
+        let mut buf = [0xff; 13];
+        assert_eq!(
+            uc.str_to_fat(s, &mut buf).unwrap(),
+            CStr8::from_bytes_with_nul(b"HELLOWORLD!\0").unwrap()
+        );
     }
 }

uefi/CHANGELOG.md

@@ -1,6 +1,8 @@
 # uefi - [Unreleased]
 
 ## Added
+- Added `proto::pi::mp::{CpuPhysicalLocation2, CPU_V2_EXTENDED_TOPOLOGY}` for
+  the extended processor topology.
 
 ## Changed
 - Made memory map types `#[repr(C)]`
@@ -17,6 +19,20 @@
 - `proto::network::pxe::DiscoverInfo::new_in_buffer` now accounts for the
   alignment padding before the server list in its buffer size check.
   Previously, an exactly-sized buffer was written 2 bytes out of bounds.
+- **Breaking:** `proto::pi::mp::ProcessorInformation` now contains the
+  `extended_information` field mandated by the PI specification. Previously,
+  the struct was 24 bytes too small, which firmware could write past.
+- **Breaking:** The revision-gated media fields `lowest_aligned_lba`,
+  `logical_blocks_per_physical_block`, and
+  `optimal_transfer_length_granularity` moved from `BlockIOMedia` to
+  `BlockIO` and return `None` if the protocol revision does not include
+  them. Previously, they read past the media structure on old revisions.
+- `DevicePath::to_pool`, `append_path`, and `append_node` now locate the
+  `DevicePathUtilities` protocol by its own GUID instead of the
+  `DevicePathToText` GUID.
+- `UnicodeCollation::str_to_fat` now zeroes the output buffer before the
+  conversion. Previously, the result could contain garbage from the
+  uninitialized buffer, or reference one byte past its end.
 
 ## Removed
 

uefi/src/proto/device_path/mod.rs

@@ -991,7 +991,7 @@ impl core::error::Error for DevicePathUtilitiesError {
 #[cfg(feature = "alloc")]
 fn open_utility_protocol() -> Result<ScopedProtocol<DevicePathUtilities>, DevicePathUtilitiesError>
 {
-    let &handle = boot::locate_handle_buffer(SearchType::ByProtocol(&DevicePathToText::GUID))
+    let &handle = boot::locate_handle_buffer(SearchType::ByProtocol(&DevicePathUtilities::GUID))
         .map_err(DevicePathUtilitiesError::CantLocateHandleBuffer)?
         .first()
         .ok_or(DevicePathUtilitiesError::NoHandle)?;

uefi/src/proto/media/block.rs

@@ -26,6 +26,52 @@ impl BlockIO {
         unsafe { &*self.0.media.cast::<BlockIOMedia>() }
     }
 
+    /// Returns the revision of this protocol.
+    #[must_use]
+    pub const fn revision(&self) -> u64 {
+        self.0.revision
+    }
+
+    /// Returns the first LBA that is aligned to a physical block boundary.
+    ///
+    /// Returns `None` for protocol revisions below 2, where this media
+    /// field is not present.
+    #[must_use]
+    pub const fn lowest_aligned_lba(&self) -> Option<Lba> {
+        if self.0.revision >= BlockIoProtocol::REVISION_2 {
+            Some(self.media().0.lowest_aligned_lba)
+        } else {
+            None
+        }
+    }
+
+    /// Returns the number of logical blocks per physical block.
+    ///
+    /// Returns `None` for protocol revisions below 2, where this media
+    /// field is not present.
+    #[must_use]
+    pub const fn logical_blocks_per_physical_block(&self) -> Option<u32> {
+        if self.0.revision >= BlockIoProtocol::REVISION_2 {
+            Some(self.media().0.logical_blocks_per_physical_block)
+        } else {
+            None
+        }
+    }
+
+    /// Returns the optimal transfer length granularity as a number of
+    /// logical blocks.
+    ///
+    /// Returns `None` for protocol revisions below 3, where this media
+    /// field is not present.
+    #[must_use]
+    pub const fn optimal_transfer_length_granularity(&self) -> Option<u32> {
+        if self.0.revision >= BlockIoProtocol::REVISION_3 {
+            Some(self.media().0.optimal_transfer_length_granularity)
+        } else {
+            None
+        }
+    }
+
     /// Resets the block device hardware.
     ///
     /// # Arguments
@@ -175,24 +221,6 @@ impl BlockIOMedia {
     pub const fn last_block(&self) -> Lba {
         self.0.last_block
     }
-
-    /// Returns the first LBA that is aligned to a physical block boundary.
-    #[must_use]
-    pub const fn lowest_aligned_lba(&self) -> Lba {
-        self.0.lowest_aligned_lba
-    }
-
-    /// Returns the number of logical blocks per physical block.
-    #[must_use]
-    pub const fn logical_blocks_per_physical_block(&self) -> u32 {
-        self.0.logical_blocks_per_physical_block
-    }
-
-    /// Returns the optimal transfer length granularity as a number of logical blocks.
-    #[must_use]
-    pub const fn optimal_transfer_length_granularity(&self) -> u32 {
-        self.0.optimal_transfer_length_granularity
-    }
 }
 
 /// Asynchronous transaction token for Block I/O 2 operations.

uefi/src/proto/pi/mp.rs

@@ -49,6 +49,12 @@ pub struct ProcessorCount {
     pub enabled: usize,
 }
 
+/// Flag for [`MpServices::get_processor_info`] requesting the extended
+/// topology in [`ProcessorInformation::extended_information`].
+///
+/// Corresponds to `CPU_V2_EXTENDED_TOPOLOGY` in the PI specification.
+pub const CPU_V2_EXTENDED_TOPOLOGY: usize = 1 << 24;
+
 /// Information about processor on the platform.
 #[repr(C)]
 #[derive(Default, Debug)]
@@ -59,6 +65,13 @@ pub struct ProcessorInformation {
     status_flag: StatusFlag,
     /// Physical location of the processor.
     pub location: CpuPhysicalLocation,
+    /// Extended physical location of the processor.
+    ///
+    /// Only filled by the firmware if [`CPU_V2_EXTENDED_TOPOLOGY`] is set in
+    /// the processor number passed to [`MpServices::get_processor_info`].
+    // The PI spec wraps this in EXTENDED_PROCESSOR_INFORMATION, a union with
+    // Location2 as its only member; the wrapper is skipped here.
+    pub extended_information: CpuPhysicalLocation2,
 }
 
 impl ProcessorInformation {
@@ -95,6 +108,27 @@ pub struct CpuPhysicalLocation {
     pub thread: u32,
 }
 
+/// Information about the 6-level physical location of the processor.
+///
+/// Corresponds to `EFI_CPU_PHYSICAL_LOCATION2` in the PI specification.
+#[repr(C)]
+#[derive(Default, Debug)]
+pub struct CpuPhysicalLocation2 {
+    /// Zero-based physical package number that identifies
+    /// the cartridge of the processor.
+    pub package: u32,
+    /// Zero-based physical module number within package of the processor.
+    pub module: u32,
+    /// Zero-based physical tile number within module of the processor.
+    pub tile: u32,
+    /// Zero-based physical die number within tile of the processor.
+    pub die: u32,
+    /// Zero-based physical core number within die of the processor.
+    pub core: u32,
+    /// Zero-based logical thread number within core of the processor.
+    pub thread: u32,
+}
+
 /// MP Services [`Protocol`].
 ///
 /// Protocol that provides services needed for multi-processor management.
@@ -156,6 +190,9 @@ impl MpServices {
     }
 
     /// Gets detailed information on the requested processor at the instant this call is made.
+    ///
+    /// Set [`CPU_V2_EXTENDED_TOPOLOGY`] in `processor_number` to also
+    /// retrieve [`ProcessorInformation::extended_information`].
     pub fn get_processor_info(&self, processor_number: usize) -> Result<ProcessorInformation> {
         let mut pi: ProcessorInformation = Default::default();
         (self.get_processor_info)(self, processor_number, &mut pi).to_result_with_val(|| pi)

uefi/src/proto/string/unicode_collation.rs

@@ -132,6 +132,10 @@ impl UnicodeCollation {
         if s.as_slice_with_nul().len() > buf.len() {
             return Err(StrConversionError::BufferTooSmall);
         }
+        // The protocol only writes the converted characters and no NUL
+        // terminator; the remaining buffer keeps its previous content.
+        // Pre-zero the buffer so the result is NUL-terminated.
+        buf.fill(0);
         // SAFETY: The memory is valid.
         let failed = unsafe {
             (self.0.str_to_fat)(
@@ -144,21 +148,15 @@ impl UnicodeCollation {
         if bool::from(failed) {
             Err(StrConversionError::ConversionFailed)
         } else {
-            // After the conversion, there is a possibility that the converted string
-            // is smaller than the original `s` string.
-            // When the converted string is smaller, there will be a bunch of trailing
-            // nulls.
-            // To remove all those trailing nulls:
-            let mut last_null_index = buf.len() - 1;
-            for i in (0..buf.len()).rev() {
-                if buf[i] != 0 {
-                    last_null_index = i + 1;
-                    break;
-                }
-            }
+            // The conversion writes at most one byte per input character
+            // and never a NUL, so the first NUL terminates the result.
+            let end = buf
+                .iter()
+                .position(|&b| b == 0)
+                .expect("conversion should leave the pre-zeroed NUL of the input intact");
             // SAFETY: The pointer is valid for the requested slice length.
-            let buf = unsafe { core::slice::from_raw_parts(buf.as_ptr(), last_null_index + 1) };
-            // SAFETY: The input was validated to be NUL-terminated with no interior NULs.
+            let buf = unsafe { core::slice::from_raw_parts(buf.as_ptr(), end + 1) };
+            // SAFETY: The slice ends with the first NUL, so there are no interior NULs.
             Ok(unsafe { CStr8::from_bytes_with_nul_unchecked(buf) })
         }
     }