Merge pull request #2036 from rust-osdev/fix

uefi-raw: make MemoryDescriptor layout portable across x86 targets

uefi-raw/CHANGELOG.md

@@ -4,7 +4,7 @@
 
 ## Changed
 - **Breaking**: `MemoryDescriptor` now has a new member to ensure correct
-  padding on 32-bit targets.
+  layout on all non-UEFI 32-bit targets.
 
 ## Removed
 

uefi-raw/src/table/boot.rs

@@ -9,6 +9,7 @@ use crate::{
 };
 use bitflags::bitflags;
 use core::ffi::c_void;
+use core::mem::offset_of;
 use core::ops::RangeInclusive;
 
 newtype_enum! {
@@ -360,12 +361,15 @@ bitflags! {
 /// [version]: MemoryDescriptor::VERSION
 /// [0]: https://github.com/tianocore/edk2/blob/7142e648416ff5d3eac6c6d607874805f5de0ca8/MdeModulePkg/Core/PiSmmCore/Page.c#L1059
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[repr(C)]
+#[repr(C, align(8))]
 pub struct MemoryDescriptor {
     /// Type of memory occupying this range.
     pub ty: MemoryType,
-    /// Reserved field that must be set to 0.
+    /// Implicit padding that should be set to 0.
-    pub reserved: u32,
+    ///
+    /// This field is required for a correct layout on non-UEFI targets to
+    /// properly parse a memory map.
+    pub padding: u32,
     // Implicit 32-bit padding.
     /// Starting physical address.
     pub phys_start: PhysicalAddress,
@@ -377,6 +381,18 @@ pub struct MemoryDescriptor {
     pub att: MemoryAttribute,
 }
 
+// Ensure ABI guarantees for MemoryDescriptor.
+const _: () = {
+    assert!(size_of::<MemoryDescriptor>() == 40);
+    assert!(align_of::<MemoryDescriptor>() == 8);
+
+    assert!(offset_of!(MemoryDescriptor, ty) == 0);
+    assert!(offset_of!(MemoryDescriptor, phys_start) == 8);
+    assert!(offset_of!(MemoryDescriptor, virt_start) == 16);
+    assert!(offset_of!(MemoryDescriptor, page_count) == 24);
+    assert!(offset_of!(MemoryDescriptor, att) == 32);
+};
+
 impl MemoryDescriptor {
     /// Memory descriptor version number.
     pub const VERSION: u32 = 1;
@@ -386,7 +402,7 @@ impl Default for MemoryDescriptor {
     fn default() -> Self {
         Self {
             ty: MemoryType::RESERVED,
-            reserved: 0,
+            padding: 0,
             phys_start: 0,
             virt_start: 0,
             page_count: 0,

uefi/src/mem/memory_map/impl_.rs

@@ -471,7 +471,7 @@ mod tests {
     const BASE_MMAP_UNSORTED: [MemoryDescriptor; 3] = [
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
-            reserved: 0,
+            padding: 0,
             phys_start: 0x3000,
             virt_start: 0x3000,
             page_count: 1,
@@ -479,7 +479,7 @@ mod tests {
         },
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
-            reserved: 0,
+            padding: 0,
             phys_start: 0x2000,
             virt_start: 0x2000,
             page_count: 1,
@@ -487,7 +487,7 @@ mod tests {
         },
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
-            reserved: 0,
+            padding: 0,
             phys_start: 0x1000,
             virt_start: 0x1000,
             page_count: 1,

uefi/src/mem/memory_map/mod.rs

@@ -133,7 +133,7 @@ mod tests_mmap_artificial {
 
         const BASE: MemoryDescriptor = MemoryDescriptor {
             ty: TY,
-            reserved: 0,
+            padding: 0,
             phys_start: 0,
             virt_start: 0,
             page_count: 0,
@@ -171,7 +171,7 @@ mod tests_mmap_artificial {
         const TY: MemoryType = MemoryType::RESERVED;
 
         const BASE: MemoryDescriptor = MemoryDescriptor {
-            reserved: 0,
+            padding: 0,
             ty: TY,
             phys_start: 0,
             virt_start: 0,
@@ -276,7 +276,7 @@ mod tests_mmap_real {
         let expected = [
             MemoryDescriptor {
                 ty: MemoryType::BOOT_SERVICES_CODE,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x0,
                 virt_start: 0x0,
                 page_count: 0x1,
@@ -287,7 +287,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::CONVENTIONAL,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x1000,
                 virt_start: 0x0,
                 page_count: 0x86,
@@ -298,7 +298,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::BOOT_SERVICES_DATA,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x87000,
                 virt_start: 0x0,
                 page_count: 0x1,
@@ -309,7 +309,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::CONVENTIONAL,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x88000,
                 virt_start: 0x0,
                 page_count: 0x18,
@@ -320,7 +320,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::CONVENTIONAL,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x100000,
                 virt_start: 0x0,
                 page_count: 0x700,
@@ -331,7 +331,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::ACPI_NON_VOLATILE,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x800000,
                 virt_start: 0x0,
                 page_count: 0x8,
@@ -342,7 +342,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::CONVENTIONAL,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x808000,
                 virt_start: 0x0,
                 page_count: 0x3,
@@ -353,7 +353,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::ACPI_NON_VOLATILE,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x80b000,
                 virt_start: 0x0,
                 page_count: 0x1,
@@ -364,7 +364,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::CONVENTIONAL,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x80c000,
                 virt_start: 0x0,
                 page_count: 0x4,
@@ -375,7 +375,7 @@ mod tests_mmap_real {
             },
             MemoryDescriptor {
                 ty: MemoryType::ACPI_NON_VOLATILE,
-                reserved: 0,
+                padding: 0,
                 phys_start: 0x810000,
                 virt_start: 0x0,
                 page_count: 0xf0,

uefi/tests/memory_map.rs

@@ -10,7 +10,7 @@ fn parse_boot_information_efi_mmap() {
     let mut mmap_source = [
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
-            reserved: 0,
+            padding: 0,
             phys_start: 0x3000,
             virt_start: 0x3000,
             page_count: 1,
@@ -18,7 +18,7 @@ fn parse_boot_information_efi_mmap() {
         },
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
-            reserved: 0,
+            padding: 0,
             phys_start: 0x2000,
             virt_start: 0x2000,
             page_count: 1,
@@ -26,7 +26,7 @@ fn parse_boot_information_efi_mmap() {
         },
         MemoryDescriptor {
             ty: MemoryType::CONVENTIONAL,
-            reserved: 0,
+            padding: 0,
             phys_start: 0x1000,
             virt_start: 0x1000,
             page_count: 1,

xtask/src/check_raw.rs

@@ -323,7 +323,12 @@ fn check_fields(fields: &Punctuated<Field, Comma>, src: &Path) -> Result<(), Err
 }
 
 /// List with allowed combinations of representations (see [`Repr`]).
-const ALLOWED_REPRS: &[&[Repr]] = &[&[Repr::C], &[Repr::C, Repr::Packed], &[Repr::Transparent]];
+const ALLOWED_REPRS: &[&[Repr]] = &[
+    &[Repr::C],
+    &[Repr::C, Repr::Packed],
+    &[Repr::Align(8), Repr::C],
+    &[Repr::Transparent],
+];
 
 fn check_type_attrs(attrs: &[Attribute], spanned: &dyn Spanned, src: &Path) -> Result<(), Error> {
     let attrs = parse_attrs(attrs, src)?;
@@ -398,9 +403,17 @@ fn check_macro(item: &ItemMacro, src: &Path) -> Result<(), Error> {
     Ok(())
 }
 
+/// True if the item is an anonymous compile-time assertion.
+fn is_anonymous_unit_const(item: &ItemConst) -> bool {
+    item.ident == "_" && matches!(&*item.ty, Type::Tuple(tuple) if tuple.elems.is_empty())
+}
+
 /// Validate a top-level item.
 fn check_item(item: &Item, src: &Path) -> Result<(), Error> {
     match item {
+        Item::Const(item) if is_anonymous_unit_const(item) => {
+            // Allow compile-time assertions such as ABI layout checks.
+        }
         Item::Const(ItemConst { vis, ty, .. }) => {
             if !is_pub(vis) {
                 return Err(Error::new(ErrorKind::MissingPub, src, item));
@@ -513,6 +526,30 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_anonymous_unit_const() {
+        // Compile-time assertions do not form part of the public API.
+        assert!(
+            check_item(
+                &parse_quote! {
+                    const _: () = {
+                        assert!(true);
+                    };
+                },
+                src(),
+            )
+            .is_ok()
+        );
+
+        // Named constants must remain public.
+        check_item_err(
+            parse_quote! {
+                const PRIVATE: () = ();
+            },
+            ErrorKind::MissingPub,
+        );
+    }
+
     #[test]
     fn test_macro() {
         // bitflags `repr` must be transparent.
@@ -606,6 +643,20 @@ mod tests {
             .is_ok()
         );
 
+        // Valid `repr(C, align(8))` struct.
+        assert!(
+            check_struct(
+                &parse_quote! {
+                    #[repr(C, align(8))]
+                    pub struct S {
+                        pub f: u32,
+                    }
+                },
+                src(),
+            )
+            .is_ok()
+        );
+
         // Missing `pub` on struct.
         check_item_err(
             parse_quote! {