From 4ce3d22839d1bd15b051b033f7541ede7df58fc6 Mon Sep 17 00:00:00 2001 From: Sanguk Park Date: Mon, 5 Oct 2020 16:44:31 +0900 Subject: [PATCH 1/4] Bump rustc, becuase cargo-xbuild struggles with old rustc. --- hfo2/src/cpu.rs | 8 ++++---- hfo2/src/init.rs | 12 ++++++------ hfo2/src/lib.rs | 4 ---- hfo2/src/mm.rs | 2 +- rust-toolchain | 2 +- 5 files changed, 12 insertions(+), 16 deletions(-) diff --git a/hfo2/src/cpu.rs b/hfo2/src/cpu.rs index 8ca7d2cae..d9f467041 100644 --- a/hfo2/src/cpu.rs +++ b/hfo2/src/cpu.rs @@ -309,8 +309,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::(); + assert!(index < core::u16::MAX as _); index as _ } } @@ -398,7 +398,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 +440,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::() } pub fn cpu_on(&self, c: &Cpu, entry: ipaddr_t, arg: uintreg_t, vm_manager: &VmManager) -> bool { diff --git a/hfo2/src/init.rs b/hfo2/src/init.rs index d05db1593..9c8632234 100644 --- a/hfo2/src/init.rs +++ b/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 = 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 diff --git a/hfo2/src/lib.rs b/hfo2/src/lib.rs index ca858b3b6..7cb595f95 100644 --- a/hfo2/src/lib.rs +++ b/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] diff --git a/hfo2/src/mm.rs b/hfo2/src/mm.rs index cdf2d2719..fc53050e2 100644 --- a/hfo2/src/mm.rs +++ b/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::()); +pub const PTE_PER_PAGE: usize = PAGE_SIZE / mem::size_of::(); #[repr(align(4096))] struct RawPageTable { diff --git a/rust-toolchain b/rust-toolchain index 1a9a5fb78..b32e8f9fc 100644 --- a/rust-toolchain +++ b/rust-toolchain @@ -1 +1 @@ -nightly-2019-12-20 +nightly-2020-09-12 From 1a79b49a5a9e4ba127efb44c4e67fbaecfaf3a90 Mon Sep 17 00:00:00 2001 From: "Park, Sanguk" Date: Sun, 25 Oct 2020 19:05:30 +0900 Subject: [PATCH 2/4] Make an is_on field on VCpu. --- hfo2/src/cpu.rs | 41 ++++++++++------------ hfo2/src/hypervisor.rs | 5 +++ inc/hf/cpu.h | 2 +- src/arch/aarch64/hypervisor/psci_handler.c | 10 ++---- 4 files changed, 27 insertions(+), 31 deletions(-) diff --git a/hfo2/src/cpu.rs b/hfo2/src/cpu.rs index d9f467041..3953d1827 100644 --- a/hfo2/src/cpu.rs +++ b/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; @@ -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, + /// If a vCPU of secondary VMs is running, its lock is logically held by the running pCPU. pub inner: SpinLock, pub interrupts: SpinLock, @@ -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()), } @@ -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,15 +584,17 @@ 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(); + let mut is_on = vcpu.is_on.lock(); + let vcpu_was_off = *is_on == false; 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. + let mut state = vcpu.inner.lock(); state.regs.reset(false, vm, cpu_id_t::from(vcpu.index())); state.on(entry, arg); + *is_on = true; } vcpu_was_off diff --git a/hfo2/src/hypervisor.rs b/hfo2/src/hypervisor.rs index aac2b0d36..dbdba6201 100644 --- a/hfo2/src/hypervisor.rs +++ b/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; diff --git a/inc/hf/cpu.h b/inc/hf/cpu.h index 9084856a7..8b0088a57 100644 --- a/inc/hf/cpu.h +++ b/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); diff --git a/src/arch/aarch64/hypervisor/psci_handler.c b/src/arch/aarch64/hypervisor/psci_handler.c index 0be2d60b6..e16d46b25 100644 --- a/src/arch/aarch64/hypervisor/psci_handler.c +++ b/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; } From 92a0b71d3973b4fca50613d7c9869e8afa8a6dd7 Mon Sep 17 00:00:00 2001 From: "Park, Sanguk" Date: Sun, 25 Oct 2020 20:13:15 +0900 Subject: [PATCH 3/4] Check if the vCPU is off on vcpu_secondary_reset_and_start. --- hfo2/src/cpu.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/hfo2/src/cpu.rs b/hfo2/src/cpu.rs index 3953d1827..315841446 100644 --- a/hfo2/src/cpu.rs +++ b/hfo2/src/cpu.rs @@ -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, @@ -587,11 +587,12 @@ pub unsafe extern "C" fn vcpu_secondary_reset_and_start( let mut is_on = vcpu.is_on.lock(); let vcpu_was_off = *is_on == false; 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. + // Set vCPU registers to a clean state ready for boot. let mut state = vcpu.inner.lock(); + debug_assert_eq!(state.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. state.regs.reset(false, vm, cpu_id_t::from(vcpu.index())); state.on(entry, arg); *is_on = true; From 0c9cee6ff63e1083784d6939685ecc009aaa4a28 Mon Sep 17 00:00:00 2001 From: "Park, Sanguk" Date: Tue, 27 Oct 2020 14:23:59 +0900 Subject: [PATCH 4/4] WIP: lock vcpuinner earlier than is_on --- hfo2/src/api.rs | 3 ++- hfo2/src/cpu.rs | 25 ++++++++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/hfo2/src/api.rs b/hfo2/src/api.rs index add04af6f..e0c226c54 100644 --- a/hfo2/src/api.rs +++ b/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()`. diff --git a/hfo2/src/cpu.rs b/hfo2/src/cpu.rs index 315841446..25b7cd51c 100644 --- a/hfo2/src/cpu.rs +++ b/hfo2/src/cpu.rs @@ -584,21 +584,20 @@ pub unsafe extern "C" fn vcpu_secondary_reset_and_start( assert!(vm.id != HF_PRIMARY_VM_ID); - let mut is_on = vcpu.is_on.lock(); - let vcpu_was_off = *is_on == false; - if vcpu_was_off { - // Set vCPU registers to a clean state ready for boot. - let mut state = vcpu.inner.lock(); - debug_assert_eq!(state.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. - state.regs.reset(false, vm, cpu_id_t::from(vcpu.index())); - state.on(entry, arg); - *is_on = true; + // 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