Lock split on VCpuInner

hfo2/src/api.rs

@@ -28,7 +28,8 @@ use crate::types::*;
 // To eliminate the risk of deadlocks, we define a partial order for the acquisition of locks held
 // concurrently by the same physical CPU. Our current ordering requirements are as follows:
 //
-// vcpu::execution_lock -> vm::lock -> vcpu::interrupts_lock -> mm_stage1_lock -> dlog sl
+// VCpu::inner lock -> VCpu::is_on lock -> Vm::inner lock -> VCpu::interrupts lock -> mm_stage1_lock
+// -> dlog sl
 //
 // Locks of the same kind require the lock of lowest address to be locked first, see
 // `sl_lock_both()`.

hfo2/src/cpu.rs

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-use core::mem::{self, ManuallyDrop, MaybeUninit};
+use core::mem::{self, MaybeUninit};
 use core::ops::Deref;
 use core::ptr;
 
@@ -67,7 +67,7 @@ pub const STACK_SIZE: usize = PAGE_SIZE;
 pub const INTERRUPT_REGISTER_BITS: usize = 32;
 
 #[repr(C)]
-#[derive(PartialEq)]
+#[derive(Debug, PartialEq)]
 pub enum VCpuStatus {
     /// The vcpu is switched off.
     Off,
@@ -274,21 +274,21 @@ impl VCpuInner {
         self.regs.set_pc_arg(entry, arg);
         self.state = VCpuStatus::Ready;
     }
-
-    /// Check whether self is an off state, for the purpose of turning vCPUs on
-    /// and off. Note that aborted still counts as on in this context.
-    pub fn is_off(&self) -> bool {
-        // Aborted still counts as ON for the purposes of PSCI, because according to the PSCI
-        // specification (section 5.7.1) a core is only considered to be off if it has been turned
-        // off with a CPU_OFF call or hasn't yet been turned on with a CPU_ON call.
-        self.state == VCpuStatus::Off
-    }
 }
 
 #[repr(C)]
 pub struct VCpu {
     vm: *mut Vm,
 
+    /// Whether the vCPU is on.
+    ///
+    /// Only meaningful for secondary VM. For primary, use Cpu::is_on instead.
+    ///
+    /// Aborted still counts as ON for the purposes of PSCI, because according to the PSCI
+    /// specification (section 5.7.1) a core is only considered to be off if it has been turned off
+    /// with a CPU_OFF call or hasn't yet been turned on with a CPU_ON call.
+    pub is_on: SpinLock<bool>,
+
     /// If a vCPU of secondary VMs is running, its lock is logically held by the running pCPU.
     pub inner: SpinLock<VCpuInner>,
     pub interrupts: SpinLock<Interrupts>,
@@ -298,6 +298,7 @@ impl VCpu {
     pub fn new(vm: *mut Vm) -> Self {
         Self {
             vm,
+            is_on: SpinLock::new(false),
             inner: SpinLock::new(VCpuInner::new()),
             interrupts: SpinLock::new(Interrupts::new()),
         }
@@ -309,8 +310,8 @@ impl VCpu {
 
     pub fn index(&self) -> spci_vcpu_index_t {
         let vcpus = self.vm().vcpus.as_ptr();
-        let index = (self as *const VCpu).wrapping_offset_from(vcpus);
-        assert!(index < core::u16::MAX as isize);
+        let index = (self as *const VCpu as usize - vcpus as usize) / core::mem::size_of::<VCpu>();
+        assert!(index < core::u16::MAX as _);
         index as _
     }
 }
@@ -398,7 +399,7 @@ pub unsafe fn cpu_get_buffer(cpu_id: cpu_id_t) -> &'static mut RawPage {
     static mut MESSAGE_BUFFER: MaybeUninit<[RawPage; MAX_CPUS]> = MaybeUninit::uninit();
     assert!(cpu_id < MAX_CPUS as _);
 
-    &mut MESSAGE_BUFFER.get_mut()[cpu_id as usize]
+    &mut MESSAGE_BUFFER.assume_init_mut()[cpu_id as usize]
 }
 
 pub struct CpuManager {
@@ -440,7 +441,7 @@ impl CpuManager {
     }
 
     pub fn index_of(&self, c: *const Cpu) -> usize {
-        c.wrapping_offset_from(self.cpus.as_ptr()) as _
+        (c as usize - self.cpus.as_ptr() as usize) / mem::size_of::<Cpu>()
     }
 
     pub fn cpu_on(&self, c: &Cpu, entry: ipaddr_t, arg: uintreg_t, vm_manager: &VmManager) -> bool {
@@ -560,18 +561,12 @@ pub unsafe extern "C" fn vcpu_is_interrupted(vcpu: *const VCpu) -> bool {
     (*vcpu).interrupts.lock().is_interrupted()
 }
 
-/// Check whether the given vcpu_inner is an off state, for the purpose of
-/// turning vCPUs on and off. Note that aborted still counts as on in this
-/// context.
-///
-/// # Safety
+/// Check whether the given vcpu is an off state, for the purpose of turning vCPUs on and off.
 ///
-/// This function is intentionally marked as unsafe because `vcpu` should actually be
-/// `VCpuExecutionLocked`.
+/// Note that aborted still counts as on in this context.
 #[no_mangle]
-pub unsafe extern "C" fn vcpu_is_off(vcpu: VCpuExecutionLocked) -> bool {
-    let vcpu = ManuallyDrop::new(vcpu);
-    vcpu.get_inner().is_off()
+pub unsafe extern "C" fn vcpu_is_off(vcpu: *const VCpu) -> bool {
+    *(*vcpu).is_on.lock() == false
 }
 
 /// Starts a vCPU of a secondary VM.
@@ -589,18 +584,20 @@ pub unsafe extern "C" fn vcpu_secondary_reset_and_start(
 
     assert!(vm.id != HF_PRIMARY_VM_ID);
 
-    let mut state = vcpu.inner.lock();
-    let vcpu_was_off = state.is_off();
-    if vcpu_was_off {
-        // Set vCPU registers to a clean state ready for boot. As this
-        // is a secondary which can migrate between pCPUs, the ID of the
-        // vCPU is defined as the index and does not match the ID of the
-        // pCPU it is running on.
-        state.regs.reset(false, vm, cpu_id_t::from(vcpu.index()));
-        state.on(entry, arg);
+    // Running vCPU holds its inner lock.
+    if let Ok(mut inner) = vcpu.inner.try_lock() {
+        if inner.state == VCpuStatus::Off {
+            // As this is a secondary which can migrate between pCPUs, the ID of the vCPU is defined
+            // as the index and does not match the ID of the pCPU it is running on.
+            inner.regs.reset(false, vm, cpu_id_t::from(vcpu.index()));
+            inner.on(entry, arg);
+            *vcpu.is_on.lock() = true;
+
+            return true;
+        }
     }
 
-    vcpu_was_off
+    false
 }
 
 /// Handles a page fault. It does so by determining if it's a legitimate or

hfo2/src/hypervisor.rs

@@ -95,6 +95,11 @@ impl Hypervisor {
             .regs
             .set_retval(primary_ret.into_raw());
 
+        // If the current vCPU is turning off, update `is_on` too.
+        if secondary_state == VCpuStatus::Off {
+            *current.is_on.lock() = false;
+        }
+
         // Mark the current vcpu as waiting.
         current.get_inner_mut().state = secondary_state;
 

hfo2/src/init.rs

@@ -92,7 +92,7 @@ unsafe extern "C" fn one_time_init(c: *const Cpu) -> *const Cpu {
 
     let ppool = MPool::new();
     ppool.free_pages(Pages::from_raw(
-        PTABLE_BUF.get_mut().as_mut_ptr(),
+        PTABLE_BUF.assume_init_mut().as_mut_ptr(),
         HEAP_PAGES,
     ));
 
@@ -106,7 +106,7 @@ unsafe extern "C" fn one_time_init(c: *const Cpu) -> *const Cpu {
 
     /// Note(HfO2): This variable was originally local, but now is static to prevent stack overflow.
     static mut MANIFEST: MaybeUninit<Manifest> = MaybeUninit::uninit();
-    let mut manifest = MANIFEST.get_mut();
+    let mut manifest = MANIFEST.assume_init_mut();
     let mut params: BootParams = MaybeUninit::uninit().assume_init();
 
     // TODO(HfO2): doesn't need to lock, actually
@@ -126,7 +126,7 @@ unsafe extern "C" fn one_time_init(c: *const Cpu) -> *const Cpu {
 
     // Initialise HAFNIUM.
     ptr::write(
-        HYPERVISOR.get_mut(),
+        HYPERVISOR.assume_init_mut(),
         Hypervisor::new(ppool, mm, cpum, VmManager::new()),
     );
 
@@ -165,7 +165,7 @@ unsafe extern "C" fn one_time_init(c: *const Cpu) -> *const Cpu {
 
     // Load all VMs.
     let primary_initrd = load_primary(
-        &mut HYPERVISOR.get_mut().vm_manager,
+        &mut HYPERVISOR.assume_init_mut().vm_manager,
         &mut hypervisor_ptable,
         &cpio,
         params.kernel_arg,
@@ -181,7 +181,7 @@ unsafe extern "C" fn one_time_init(c: *const Cpu) -> *const Cpu {
     );
 
     load_secondary(
-        &mut HYPERVISOR.get_mut().vm_manager,
+        &mut HYPERVISOR.assume_init_mut().vm_manager,
         &mut hypervisor_ptable,
         &mut manifest,
         &cpio,
@@ -211,7 +211,7 @@ unsafe extern "C" fn one_time_init(c: *const Cpu) -> *const Cpu {
 }
 
 pub fn hypervisor() -> &'static Hypervisor {
-    unsafe { HYPERVISOR.get_ref() }
+    unsafe { HYPERVISOR.assume_init_ref() }
 }
 
 // The entry point of CPUs when they are turned on. It is supposed to initialise

hfo2/src/lib.rs

@@ -19,12 +19,8 @@
 #![feature(const_fn)]
 #![feature(const_panic)]
 #![feature(maybe_uninit_ref)]
-#![feature(ptr_offset_from)]
 #![feature(const_raw_ptr_to_usize_cast)]
-#![feature(ptr_wrapping_offset_from)]
-#![feature(slice_from_raw_parts)]
 #![feature(linkage)]
-#![feature(track_caller)]
 #![feature(try_blocks)]
 
 #[macro_use]

hfo2/src/mm.rs

@@ -562,7 +562,7 @@ impl Iterator for BlockIter {
 }
 
 /// Number of page table entries in a page table.
-pub const PTE_PER_PAGE: usize = (PAGE_SIZE / mem::size_of::<PageTableEntry>());
+pub const PTE_PER_PAGE: usize = PAGE_SIZE / mem::size_of::<PageTableEntry>();
 
 #[repr(align(4096))]
 struct RawPageTable {

inc/hf/cpu.h

@@ -111,7 +111,7 @@ const struct arch_regs *vcpu_get_regs_const(const struct vcpu *vcpu);
 struct vm *vcpu_get_vm(struct vcpu *vcpu);
 struct cpu *vcpu_get_cpu(struct vcpu *vcpu);
 bool vcpu_is_interrupted(struct vcpu *vcpu);
-bool vcpu_is_off(struct vcpu_execution_locked vcpu);
+bool vcpu_is_off(struct vcpu *vcpu);
 bool vcpu_secondary_reset_and_start(struct vcpu *vcpu, ipaddr_t entry,
 				    uintreg_t arg);
 

rust-toolchain

@@ -1 +1 @@
-nightly-2019-12-20
+nightly-2020-09-12

src/arch/aarch64/hypervisor/psci_handler.c

@@ -295,7 +295,6 @@ bool psci_secondary_vm_handler(struct vcpu *vcpu, uint32_t func, uintreg_t arg0,
 		uint32_t lowest_affinity_level = arg1;
 		struct vm *vm = vcpu_get_vm(vcpu);
 		struct vcpu *target_vcpu;
-		struct vcpu_execution_locked vcpu_locked;
 		spci_vcpu_index_t target_vcpu_index =
 			vcpu_id_to_index(target_affinity);
 
@@ -311,13 +310,8 @@ bool psci_secondary_vm_handler(struct vcpu *vcpu, uint32_t func, uintreg_t arg0,
 		}
 
 		target_vcpu = vm_get_vcpu(vm, target_vcpu_index);
-		if (!vcpu_try_lock(target_vcpu, &vcpu_locked)) {
-			*ret = PSCI_RETURN_ON;
-		} else {
-			*ret = vcpu_is_off(vcpu_locked) ? PSCI_RETURN_OFF
-							: PSCI_RETURN_ON;
-			vcpu_unlock(&vcpu_locked);
-		}
+		*ret = vcpu_is_off(target_vcpu) ? PSCI_RETURN_OFF
+						: PSCI_RETURN_ON;
 		break;
 	}