Resolve simple issues on the review of kaist-cp#45 (cont'd)

hfo2/src/api.rs

@@ -230,7 +230,7 @@ pub unsafe extern "C" fn api_vcpu_get_count(
         return 0;
     }
 
-    let vm = ok_or_return!(vm_manager().get(vm_id).ok_or(()), 0);
+    let vm = unwrap_or!(vm_manager().get(vm_id), return 0);
 
     vm.vcpus.len() as spci_vcpu_count_t
 }

hfo2/src/fdt.rs

@@ -20,7 +20,6 @@ use core::ptr;
 use core::slice;
 use core::str;
 
-use crate::std::*;
 use crate::utils::*;
 
 use scopeguard::guard;
@@ -312,10 +311,7 @@ impl FdtNode {
 impl FdtHeader {
     pub unsafe fn dump(&self) {
         unsafe fn asciz_to_utf8(ptr: *const u8) -> &'static str {
-            let mut len = 0;
-            while *ptr.add(len) != 0u8 {
-                len += 1;
-            }
+            let len = (0..).find(|i| *ptr.add(*i) != 0).unwrap();
             let bytes = slice::from_raw_parts(ptr, len);
             str::from_utf8_unchecked(bytes)
         }
@@ -370,25 +366,24 @@ impl FdtHeader {
             "fdt: off_mem_rsvmap={}\n",
             u32::from_be(self.off_mem_rsvmap)
         );
-        {
-            let e = self as *const _ as usize + u32::from_be(self.off_mem_rsvmap) as usize;
-            let mut entry = e as *const FdtReserveEntry;
 
-            while (*entry).address != 0 || (*entry).size != 0 {
-                dlog!(
-                    "Entry: {:p} (0x{:x} bytes)\n",
-                    u64::from_be((*entry).address) as *const u8,
-                    u64::from_be((*entry).size)
-                );
-                entry = entry.add(1);
-            }
+        let mut entry = (self as *const _ as usize + u32::from_be(self.off_mem_rsvmap) as usize)
+            as *const FdtReserveEntry;
+
+        while (*entry).address != 0 || (*entry).size != 0 {
+            dlog!(
+                "Entry: {:p} (0x{:x} bytes)\n",
+                u64::from_be((*entry).address) as *const u8,
+                u64::from_be((*entry).size)
+            );
+            entry = entry.add(1);
         }
     }
 
     pub unsafe fn add_mem_reservation(&mut self, addr: u64, len: u64) {
         // TODO: Clean this up.
-        let begin = (self as *const _ as usize as *const u8)
-            .add(u32::from_be(self.off_mem_rsvmap) as usize);
+        let begin =
+            (self as *const _ as usize as *mut u8).add(u32::from_be(self.off_mem_rsvmap) as usize);
         let e = begin as *mut FdtReserveEntry;
         let old_size = u32::from_be(self.totalsize) - u32::from_be(self.off_mem_rsvmap);
 
@@ -399,12 +394,12 @@ impl FdtHeader {
         self.off_dt_strings =
             (u32::from_be(self.off_dt_strings) + mem::size_of::<FdtReserveEntry>() as u32).to_be();
 
-        memmove_s(
-            begin.add(mem::size_of::<FdtReserveEntry>()) as usize as *mut _,
-            old_size as usize,
-            begin as usize as *const _,
+        ptr::copy_nonoverlapping(
+            begin,
+            begin.add(mem::size_of::<FdtReserveEntry>()),
             old_size as usize,
         );
+
         (*e).address = addr.to_be();
         (*e).size = len.to_be();
     }
@@ -420,7 +415,7 @@ pub extern "C" fn fdt_header_size() -> usize {
 }
 
 #[no_mangle]
-pub unsafe extern "C" fn fdt_total_size(hdr: *mut FdtHeader) -> u32 {
+pub unsafe extern "C" fn fdt_total_size(hdr: *const FdtHeader) -> u32 {
     (*hdr).total_size()
 }
 
@@ -444,10 +439,9 @@ pub unsafe extern "C" fn fdt_find_child(node: *mut FdtNode, child: *const u8) ->
 
 #[no_mangle]
 pub unsafe extern "C" fn fdt_first_child(node: *mut FdtNode, child_name: *mut *const u8) -> bool {
-    (*node).first_child().map_or(false, |name| {
-        ptr::write(child_name, name);
-        true
-    })
+    let name = unwrap_or!((*node).first_child(), return false);
+    ptr::write(child_name, name);
+    true
 }
 
 #[no_mangle]
@@ -468,14 +462,10 @@ pub unsafe extern "C" fn fdt_read_property(
     buf: *mut *const u8,
     size: *mut u32,
 ) -> bool {
-    match (*node).read_property(name) {
-        Ok((prop_buf, prop_size)) => {
-            *buf = prop_buf;
-            *size = prop_size;
-            true
-        }
-        Err(_) => false,
-    }
+    let (prop_buf, prop_size) = ok_or_return!((*node).read_property(name), false);
+    *buf = prop_buf;
+    *size = prop_size;
+    true
 }
 
 #[no_mangle]

hfo2/src/init.rs

@@ -66,7 +66,7 @@ unsafe fn one_time_init() {
     let mut hypervisor_ptable = memory_manager().hypervisor_ptable.lock();
 
     let params = boot_params::get(&mut hypervisor_ptable, &mut ppool)
-        .unwrap_or_else(|| panic!("unable to retrieve boot params"));
+        .expect("unable to retrieve boot params");
 
     let cpu_manager = cpu_module_init(&params.cpu_ids[..params.cpu_count]);
     cpu_manager_init(cpu_manager);
@@ -86,22 +86,15 @@ unsafe fn one_time_init() {
     );
 
     // Map initrd in, and initialise cpio parser.
-    if hypervisor_ptable
+    hypervisor_ptable
         .identity_map(params.initrd_begin, params.initrd_end, Mode::R, &ppool)
-        .is_err()
-    {
-        panic!("unable to map initrd in");
-    }
+        .expect("unable to map initrd in");
 
     let initrd = pa_addr(params.initrd_begin) as *mut _;
-
-    let mut cpio = MaybeUninit::uninit();
-    memiter_init(
-        cpio.get_mut(),
+    let cpio = MemIter::from_raw(
         initrd,
         pa_difference(params.initrd_begin, params.initrd_end),
     );
-    let cpio = cpio.assume_init();
 
     vm_manager_init(VmManager::new());
 
@@ -113,7 +106,7 @@ unsafe fn one_time_init() {
         params.kernel_arg,
         &ppool,
     )
-    .unwrap_or_else(|_| panic!("unable to load primary VM"));
+    .expect("unable to load primary VM");
 
     // load_secondary will add regions assigned to the secondary VMs from
     // mem_ranges to reserved_ranges.
@@ -122,23 +115,19 @@ unsafe fn one_time_init() {
         pa_from_va(va_from_ptr(primary_initrd.limit as usize as *const _)),
     );
 
-    if load_secondary(
+    load_secondary(
         vm_manager_mut(),
         &mut hypervisor_ptable,
         &cpio,
         &params,
         &mut update,
         &mut ppool,
     )
-    .is_err()
-    {
-        panic!("unable to load secondary VMs");
-    }
+    .expect("unable to load secondary VMs");
 
     // Prepare to run by updating bootparams as seen by primary VM.
-    if boot_params::update(&mut hypervisor_ptable, &mut update, &mut ppool).is_err() {
-        panic!("plat_update_boot_params failed");
-    }
+    boot_params::update(&mut hypervisor_ptable, &mut update, &mut ppool)
+        .expect("plat_update_boot_params failed");
 
     hypervisor_ptable.defrag(&ppool);
 
@@ -179,11 +168,7 @@ pub unsafe extern "C" fn cpu_main(mut c: *const Cpu) -> *mut VCpu {
     (*vcpu).set_cpu(c);
 
     // Reset the registers to give a clean start for the primary's vCPU.
-    (*vcpu)
-        .inner
-        .get_mut_unchecked()
-        .regs
-        .reset(true, &*vm, (*c).id);
+    (*vcpu).inner.get_mut().regs.reset(true, &*vm, (*c).id);
 
     vcpu
 }

hfo2/src/load.rs

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-use core::mem::{self, MaybeUninit};
+use core::mem;
 
 use crate::addr::*;
 use crate::arch::*;
@@ -71,18 +71,18 @@ pub unsafe fn load_primary(
     kernel_arg: uintreg_t,
     ppool: &MPool,
 ) -> Result<MemIter, ()> {
-    let mut it = mem::uninitialized();
     let primary_begin = layout_primary_begin();
 
-    if !cpio_find_file(cpio, "vmlinuz\0".as_ptr(), &mut it) {
+    let it = unwrap_or!(find_file(&mut cpio.clone(), "vmlinuz\0".as_ptr()), {
         dlog!("Unable to find vmlinuz\n");
         return Err(());
-    }
+    });
 
     dlog!(
         "Copying primary to {:p}\n",
         pa_addr(primary_begin) as *const u8
     );
+
     if !copy_to_unmapped(
         hypervisor_ptable,
         primary_begin,
@@ -94,13 +94,10 @@ pub unsafe fn load_primary(
         return Err(());
     }
 
-    let mut initrd = MaybeUninit::uninit();
-    if !cpio_find_file(cpio, "initrd.img\0".as_ptr(), initrd.get_mut()) {
+    let initrd = unwrap_or!(find_file(&mut cpio.clone(), "initrd.img\0".as_ptr()), {
         dlog!("Unable to find initrd.img\n");
         return Err(());
-    }
-
-    let initrd = initrd.assume_init();
+    });
 
     let vm = vm_manager
         .new_vm(MAX_CPUS as spci_vcpu_count_t, ppool)
@@ -216,26 +213,23 @@ pub unsafe fn load_secondary(
     mem_ranges_available.clone_from_slice(&params.mem_ranges);
     mem_ranges_available.truncate(params.mem_ranges_count);
 
-    let mut it = mem::uninitialized();
-
-    if !cpio_find_file(cpio, "vms.txt\0".as_ptr(), &mut it) {
+    let mut it = unwrap_or!(find_file(&mut cpio.clone(), "vms.txt\0".as_ptr()), {
         dlog!("vms.txt is missing\n");
         return Ok(());
-    }
+    });
 
     // Round the last addresses down to the page size.
     for mem_range in mem_ranges_available.iter_mut() {
         mem_range.end = pa_init(round_down(pa_addr(mem_range.end), PAGE_SIZE));
     }
 
-    let mut mem = mem::uninitialized();
-    let mut cpu = mem::uninitialized();
-    let mut name = mem::uninitialized();
+    loop {
+        // Note(HfO2): There is `while let (Some(x), Some(y)) = (...) {}` but it
+        // is not short-circuiting.
+        let mut mem = unwrap_or!(it.parse_uint(), break);
+        let cpu = unwrap_or!(it.parse_uint(), break);
+        let name = unwrap_or!(it.parse_str(), break);
 
-    while memiter_parse_uint(&mut it, &mut mem)
-        && memiter_parse_uint(&mut it, &mut cpu)
-        && memiter_parse_str(&mut it, &mut name)
-    {
         dlog!("Loading ");
         let mut p = name.next;
         while p != name.limit {
@@ -244,12 +238,10 @@ pub unsafe fn load_secondary(
         }
         dlog!("\n");
 
-        let mut kernel = mem::uninitialized();
-
-        if !cpio_find_file_memiter(cpio, &name, &mut kernel) {
+        let kernel = unwrap_or!(find_file_memiter(&mut cpio.clone(), &name), {
             dlog!("Unable to load kernel\n");
             continue;
-        }
+        });
 
         // Round up to page size.
         mem = (mem + PAGE_SIZE as u64 - 1) & !(PAGE_SIZE as u64 - 1);
@@ -279,20 +271,14 @@ pub unsafe fn load_secondary(
             continue;
         }
 
-        let vm_id = match vm_manager.new_vm(cpu as spci_vcpu_count_t, ppool) {
-            Some(vm) => vm.id,
+        let vm = match vm_manager.new_vm(cpu as spci_vcpu_count_t, ppool) {
+            Some(vm) => vm,
             None => {
                 dlog!("Unable to initialise VM\n");
                 continue;
             }
         };
 
-        // We have to borrow primary again here, due to conservative Rust borrow
-        // checker.
-        let (primary, vm) = vm_manager.get_mut_with_primary(vm_id).unwrap();
-
-        let secondary_entry = ipa_from_pa(secondary_mem_begin);
-
         // Grant the VM access to the memory.
         if vm
             .inner
@@ -310,6 +296,9 @@ pub unsafe fn load_secondary(
             continue;
         }
 
+        let vm_id = vm.id;
+        let primary = vm_manager.get_mut(HF_PRIMARY_VM_ID).unwrap();
+
         // Deny the primary VM access to this memory.
         if primary
             .inner
@@ -328,6 +317,8 @@ pub unsafe fn load_secondary(
             pa_addr(secondary_mem_begin)
         );
 
+        let vm = vm_manager.get_mut(vm_id).unwrap();
+        let secondary_entry = ipa_from_pa(secondary_mem_begin);
         vcpu_secondary_reset_and_start(
             &mut vm.vcpus[0],
             secondary_entry,

hfo2/src/types.rs

@@ -57,7 +57,7 @@ pub const HF_INVALID_INTID: intid_t = 0xffff_ffff;
 /// The virtual interrupt ID used for the virtual timer.
 pub const HF_VIRTUAL_TIMER_INTID: intid_t = 3;
 
-// These constants are originally from build scripts. (See
+// TODO(HfO2): These constants are originally from build scripts. (See
 // //project/reference/BUILD.gn.)
 pub const HEAP_PAGES: usize = 60;
 

hfo2/src/utils.rs

@@ -26,6 +26,16 @@ macro_rules! ok_or_return {
     }};
 }
 
+#[macro_export]
+macro_rules! unwrap_or {
+    ($e:expr, $err:expr) => {{
+        match $e {
+            Some(r) => r,
+            None => $err,
+        }
+    }};
+}
+
 pub fn spin_loop() -> ! {
     loop {
         spin_loop_hint();