Apply review comments

Co-authored-by: Philipp Oppermann <dev@phil-opp.com>

Author: jasoncouture

common/src/lib.rs

@@ -292,15 +292,16 @@ where
         let ramdisk_address_start = mapping_addr(
             config.mappings.ramdisk_memory,
             system_info.ramdisk_len,
-            8,
+            Size4KiB::SIZE,
             &mut used_entries,
         );
         let physical_address = PhysAddr::new(ramdisk_address);
         let ramdisk_physical_start_page: PhysFrame<Size4KiB> =
             PhysFrame::containing_address(physical_address);
-        let ramdisk_page_count = (system_info.ramdisk_len - 1 / Size4KiB::SIZE) + 1;
+        let ramdisk_page_count = ((system_info.ramdisk_len - 1) / Size4KiB::SIZE) + 1;
         let ramdisk_physical_end_page = ramdisk_physical_start_page + ramdisk_page_count;
-        let start_page = Page::containing_address(ramdisk_address_start);
+        let start_page = Page::from_start_address(ramdisk_address_start)
+            .expect("the ramdisk start address must be page aligned");
 
         let flags = PageTableFlags::PRESENT | PageTableFlags::WRITABLE;
         for (i, frame) in

src/lib.rs

@@ -108,7 +108,7 @@ impl UefiBoot {
         self
     }
 
-    /// Create a bootable BIOS disk image at the given path.
+    /// Create a bootable UEFI disk image at the given path.
     pub fn create_disk_image(&self, out_path: &Path) -> anyhow::Result<()> {
         let fat_partition = self
             .create_fat_partition()
@@ -131,10 +131,7 @@ impl UefiBoot {
     /// bootloader won't be found.
     pub fn create_pxe_tftp_folder(&self, out_path: &Path) -> anyhow::Result<()> {
         let bootloader_path = Path::new(env!("UEFI_BOOTLOADER_PATH"));
-        let ramdisk_path = match self.ramdisk.as_ref() {
-            Some(rd) => Some(rd.as_path()),
-            None => None,
-        };
+        let ramdisk_path = self.ramdisk.as_deref();
         pxe::create_uefi_tftp_folder(
             bootloader_path,
             self.kernel.as_path(),

uefi/src/main.rs

@@ -119,27 +119,28 @@ fn main_inner(image: Handle, mut st: SystemTable<Boot>) -> Status {
     let mmap_storage = {
         let mut memory_map_size = st.boot_services().memory_map_size();
         let mut target_size = memory_map_size.map_size + (8 * memory_map_size.entry_size);
-        let mut storage: &mut [u8];
         loop {
             let ptr = st
                 .boot_services()
                 .allocate_pool(MemoryType::LOADER_DATA, target_size)
                 .expect("Failed to allocate memory for mmap storage");
-            storage = unsafe { slice::from_raw_parts_mut(ptr, target_size) };
-            if let Err(_) = st.boot_services().memory_map(storage) {
-                memory_map_size = st.boot_services().memory_map_size();
-                // By measuring the size here, we can find out exactly how much we need.
-                // We may hit this code twice, if the map allocation ends up spanning more pages.
-                let next_target_size = memory_map_size.map_size + (8 * memory_map_size.entry_size);
-                target_size = next_target_size;
-                st.boot_services()
-                    .free_pool(ptr)
-                    .expect("Failed to free temporary memory for memory map!");
-                continue;
+            let storage = unsafe { slice::from_raw_parts_mut(ptr, target_size) };
+            if st.boot_services().memory_map(storage).is_ok() {
+                break storage;
             }
-            break;
+            // allocated memory region was not big enough -> free it again
+            st.boot_services()
+                .free_pool(ptr)
+                .expect("Failed to free temporary memory for memory map!");
+
+            // By measuring the size here, we can find out exactly how much we need.
+            // We may hit this code twice, if the map allocation ends up spanning more pages.
+            memory_map_size = st.boot_services().memory_map_size();
+
+            let next_target_size = memory_map_size.map_size + (8 * memory_map_size.entry_size);
+            target_size = next_target_size;
+            
         }
-        storage
     };
 
     log::trace!("exiting boot services");
@@ -309,11 +310,10 @@ fn load_file_from_disk(
 
     let file_handle_result = root.open(filename, FileMode::Read, FileAttribute::empty());
 
-    if file_handle_result.is_err() {
-        return None;
-    }
-
-    let file_handle = file_handle_result.unwrap();
+    let file_handle = match file_handle_result {
+        Err(_) => return None,
+        Ok(handle) => handle,
+    };
 
     let mut file = match file_handle.into_type().unwrap() {
         uefi::proto::media::file::FileType::Regular(f) => f,
@@ -353,7 +353,6 @@ fn load_file_from_tftp_boot_server(
     assert!(mode.dhcp_ack_received);
     let dhcpv4: &DhcpV4Packet = mode.dhcp_ack.as_ref();
     let server_ip = IpAddress::new_v4(dhcpv4.bootp_si_addr);
-    let mut buf = [0u8; 256];
     assert!(name.len() < 256);
 
     let filename = CStr8::from_bytes_with_nul(name.as_bytes()).unwrap();