From b91af6fb371fe21f9ef93a0d3f6e73e308336998 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 26 Mar 2025 07:58:56 +0000 Subject: [PATCH 1/4] refactor: stop caching mpidr in struct KvmVcpu This cached field was only accessed precisely once, so the getter might as well compute it instead. While we're at it, merge the getting with the free-standing computation function. Signed-off-by: Patrick Roy --- src/vmm/src/arch/aarch64/mod.rs | 3 ++- src/vmm/src/arch/aarch64/vcpu.rs | 39 +++++++++++--------------------- 2 files changed, 15 insertions(+), 27 deletions(-) diff --git a/src/vmm/src/arch/aarch64/mod.rs b/src/vmm/src/arch/aarch64/mod.rs index 10ffe53c8fa..e19f496f019 100644 --- a/src/vmm/src/arch/aarch64/mod.rs +++ b/src/vmm/src/arch/aarch64/mod.rs @@ -98,7 +98,8 @@ pub fn configure_system_for_boot( let vcpu_mpidr = vcpus .iter_mut() .map(|cpu| cpu.kvm_vcpu.get_mpidr()) - .collect(); + .collect::, _>>() + .map_err(KvmVcpuError::ConfigureRegisters)?; let cmdline = boot_cmdline .as_cstring() .expect("Cannot create cstring from cmdline string"); diff --git a/src/vmm/src/arch/aarch64/vcpu.rs b/src/vmm/src/arch/aarch64/vcpu.rs index a126ab1a3a5..5de35f97286 100644 --- a/src/vmm/src/arch/aarch64/vcpu.rs +++ b/src/vmm/src/arch/aarch64/vcpu.rs @@ -140,16 +140,6 @@ pub fn setup_boot_regs( Ok(()) } -/// Read the MPIDR - Multiprocessor Affinity Register. -pub fn get_mpidr(vcpufd: &VcpuFd) -> Result { - // MPIDR register is 64 bit wide on aarch64 - let mut mpidr = [0_u8; 8]; - match vcpufd.get_one_reg(MPIDR_EL1, &mut mpidr) { - Err(err) => Err(VcpuArchError::GetOneReg(MPIDR_EL1, err)), - Ok(_) => Ok(u64::from_le_bytes(mpidr)), - } -} - /// Saves the states of the system registers into `state`. /// /// # Arguments @@ -277,7 +267,6 @@ pub struct KvmVcpu { pub fd: VcpuFd, /// Vcpu peripherals, such as buses pub peripherals: Peripherals, - mpidr: u64, kvi: kvm_vcpu_init, } @@ -311,14 +300,18 @@ impl KvmVcpu { index, fd: kvm_vcpu, peripherals: Default::default(), - mpidr: 0, kvi, }) } - /// Gets the MPIDR register value. - pub fn get_mpidr(&self) -> u64 { - self.mpidr + /// Read the MPIDR - Multiprocessor Affinity Register. + pub fn get_mpidr(&self) -> Result { + // MPIDR register is 64 bit wide on aarch64 + let mut mpidr = [0_u8; 8]; + match self.fd.get_one_reg(MPIDR_EL1, &mut mpidr) { + Err(err) => Err(VcpuArchError::GetOneReg(MPIDR_EL1, err)), + Ok(_) => Ok(u64::from_le_bytes(mpidr)), + } } /// Configures an aarch64 specific vcpu for booting Linux. @@ -351,8 +344,6 @@ impl KvmVcpu { ) .map_err(KvmVcpuError::ConfigureRegisters)?; - self.mpidr = get_mpidr(&self.fd).map_err(KvmVcpuError::ConfigureRegisters)?; - Ok(()) } @@ -393,7 +384,7 @@ impl KvmVcpu { ..Default::default() }; get_all_registers(&self.fd, &mut state.regs).map_err(KvmVcpuError::SaveState)?; - state.mpidr = get_mpidr(&self.fd).map_err(KvmVcpuError::SaveState)?; + state.mpidr = self.get_mpidr().map_err(KvmVcpuError::SaveState)?; state.kvi = self.kvi; // We don't save power off state in a snapshot, because @@ -757,21 +748,17 @@ mod tests { #[test] fn test_read_mpidr() { - let kvm = Kvm::new(vec![]).unwrap(); - let vm = kvm.fd.create_vm().unwrap(); - let vcpu = vm.create_vcpu(0).unwrap(); - let mut kvi: kvm_bindings::kvm_vcpu_init = kvm_bindings::kvm_vcpu_init::default(); - vm.get_preferred_target(&mut kvi).unwrap(); + let (_, _, vcpu, _) = setup_vcpu(0x10000); // Must fail when vcpu is not initialized yet. - let res = get_mpidr(&vcpu); + let res = vcpu.get_mpidr(); assert!(matches!( res.unwrap_err(), VcpuArchError::GetOneReg(MPIDR_EL1, _) )); - vcpu.vcpu_init(&kvi).unwrap(); - assert_eq!(get_mpidr(&vcpu).unwrap(), 0x8000_0000); + vcpu.init_vcpu().unwrap(); + assert_eq!(vcpu.get_mpidr().unwrap(), 0x8000_0000); } #[test] From 70972694ae34c1e9b8960403ea66edead2713336 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 26 Mar 2025 07:35:01 +0000 Subject: [PATCH 2/4] refactor: move get_manufacturer_id_from_host() to regs.rs Just make this a method on `Aarch64RegisterVec`, and make it return `Option`, because there was only a single error case, and the error message was additionall never displayed because the only call site ignores errors beyond acknowledging that they happened. Signed-off-by: Patrick Roy --- src/vmm/src/arch/aarch64/regs.rs | 8 ++++++++ src/vmm/src/arch/aarch64/vcpu.rs | 16 ---------------- src/vmm/src/persist.rs | 12 ++++++------ 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/vmm/src/arch/aarch64/regs.rs b/src/vmm/src/arch/aarch64/regs.rs index d844fbfb56b..913f352a7d9 100644 --- a/src/vmm/src/arch/aarch64/regs.rs +++ b/src/vmm/src/arch/aarch64/regs.rs @@ -260,6 +260,14 @@ impl Aarch64RegisterVec { data: &mut self.data, } } + + /// Extract the Manufacturer ID from a VCPU state's registers. + /// The ID is found between bits 24-31 of MIDR_EL1 register. + pub fn manifacturer_id(&self) -> Option { + self.iter() + .find(|reg| reg.id == MIDR_EL1) + .map(|reg| ((reg.value::() >> 24) & 0xFF) as u32) + } } impl Serialize for Aarch64RegisterVec { diff --git a/src/vmm/src/arch/aarch64/vcpu.rs b/src/vmm/src/arch/aarch64/vcpu.rs index 5de35f97286..2cbd2f702ab 100644 --- a/src/vmm/src/arch/aarch64/vcpu.rs +++ b/src/vmm/src/arch/aarch64/vcpu.rs @@ -45,22 +45,6 @@ pub enum VcpuArchError { GetMidrEl1(String), } -/// Extract the Manufacturer ID from a VCPU state's registers. -/// The ID is found between bits 24-31 of MIDR_EL1 register. -/// -/// # Arguments -/// -/// * `regs` - reference [`Aarch64RegisterVec`] structure with all registers of a VCPU. -pub fn get_manufacturer_id_from_state(regs: &Aarch64RegisterVec) -> Result { - let midr_el1 = regs.iter().find(|reg| reg.id == MIDR_EL1); - match midr_el1 { - Some(register) => Ok(((register.value::() >> 24) & 0xFF) as u32), - None => Err(VcpuArchError::GetMidrEl1( - "Failed to find MIDR_EL1 in vCPU state!".to_string(), - )), - } -} - /// Extract the Manufacturer ID from the host. /// The ID is found between bits 24-31 of MIDR_EL1 register. pub fn get_manufacturer_id_from_host() -> Result { diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs index 64292612a16..3ffc4355f0b 100644 --- a/src/vmm/src/persist.rs +++ b/src/vmm/src/persist.rs @@ -17,7 +17,7 @@ use userfaultfd::{FeatureFlags, Uffd, UffdBuilder}; use vmm_sys_util::sock_ctrl_msg::ScmSocket; #[cfg(target_arch = "aarch64")] -use crate::arch::aarch64::vcpu::{get_manufacturer_id_from_host, get_manufacturer_id_from_state}; +use crate::arch::aarch64::vcpu::get_manufacturer_id_from_host; use crate::builder::{self, BuildMicrovmFromSnapshotError}; use crate::cpu_config::templates::StaticCpuTemplate; #[cfg(target_arch = "x86_64")] @@ -331,24 +331,24 @@ pub fn validate_cpu_vendor(microvm_state: &MicrovmState) { #[cfg(target_arch = "aarch64")] pub fn validate_cpu_manufacturer_id(microvm_state: &MicrovmState) { let host_cpu_id = get_manufacturer_id_from_host(); - let snapshot_cpu_id = get_manufacturer_id_from_state(µvm_state.vcpu_states[0].regs); + let snapshot_cpu_id = microvm_state.vcpu_states[0].regs.manifacturer_id(); match (host_cpu_id, snapshot_cpu_id) { - (Ok(host_id), Ok(snapshot_id)) => { + (Ok(host_id), Some(snapshot_id)) => { info!("Host CPU manufacturer ID: {host_id:?}"); info!("Snapshot CPU manufacturer ID: {snapshot_id:?}"); if host_id != snapshot_id { warn!("Host CPU manufacturer ID differs from the snapshotted one",); } } - (Ok(host_id), Err(_)) => { + (Ok(host_id), None) => { info!("Host CPU manufacturer ID: {host_id:?}"); warn!("Snapshot CPU manufacturer ID: couldn't get from the snapshot"); } - (Err(_), Ok(snapshot_id)) => { + (Err(_), Some(snapshot_id)) => { warn!("Host CPU manufacturer ID: couldn't get from the host"); info!("Snapshot CPU manufacturer ID: {snapshot_id:?}"); } - (Err(_), Err(_)) => { + (Err(_), None) => { warn!("Host CPU manufacturer ID: couldn't get from the host"); warn!("Snapshot CPU manufacturer ID: couldn't get from the snapshot"); } From 971300b27004ae41edeed3e656e8580e16699ee0 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 26 Mar 2025 09:56:07 +0000 Subject: [PATCH 3/4] refactor: move free functions into impl KvmVcpu block Almost all the free functions in arch::arch64::vcpu were just taking a vcpu reference as a first parameter (in some form). So just turn these into methods implemented on KvmVcpu that take &self. Signed-off-by: Patrick Roy --- src/vmm/src/arch/aarch64/vcpu.rs | 350 +++++++++++++++---------------- 1 file changed, 170 insertions(+), 180 deletions(-) diff --git a/src/vmm/src/arch/aarch64/vcpu.rs b/src/vmm/src/arch/aarch64/vcpu.rs index 2cbd2f702ab..82bbd499deb 100644 --- a/src/vmm/src/arch/aarch64/vcpu.rs +++ b/src/vmm/src/arch/aarch64/vcpu.rs @@ -62,80 +62,6 @@ pub fn get_manufacturer_id_from_host() -> Result { Ok(manufacturer_id >> 24) } -/// Configure relevant boot registers for a given vCPU. -/// -/// # Arguments -/// -/// * `cpu_id` - Index of current vcpu. -/// * `boot_ip` - Starting instruction pointer. -/// * `mem` - Reserved DRAM for current VM. -pub fn setup_boot_regs( - vcpufd: &VcpuFd, - cpu_id: u8, - boot_ip: u64, - mem: &GuestMemoryMmap, - optional_capabilities: &OptionalCapabilities, -) -> Result<(), VcpuArchError> { - let kreg_off = offset_of!(kvm_regs, regs); - - // Get the register index of the PSTATE (Processor State) register. - let pstate = offset_of!(user_pt_regs, pstate) + kreg_off; - let id = arm64_core_reg_id!(KVM_REG_SIZE_U64, pstate); - vcpufd - .set_one_reg(id, &PSTATE_FAULT_BITS_64.to_le_bytes()) - .map_err(|err| VcpuArchError::SetOneReg(id, err))?; - - // Other vCPUs are powered off initially awaiting PSCI wakeup. - if cpu_id == 0 { - // Setting the PC (Processor Counter) to the current program address (kernel address). - let pc = offset_of!(user_pt_regs, pc) + kreg_off; - let id = arm64_core_reg_id!(KVM_REG_SIZE_U64, pc); - vcpufd - .set_one_reg(id, &boot_ip.to_le_bytes()) - .map_err(|err| VcpuArchError::SetOneReg(id, err))?; - - // Last mandatory thing to set -> the address pointing to the FDT (also called DTB). - // "The device tree blob (dtb) must be placed on an 8-byte boundary and must - // not exceed 2 megabytes in size." -> https://www.kernel.org/doc/Documentation/arm64/booting.txt. - // We are choosing to place it the end of DRAM. See `get_fdt_addr`. - let regs0 = offset_of!(user_pt_regs, regs) + kreg_off; - let id = arm64_core_reg_id!(KVM_REG_SIZE_U64, regs0); - vcpufd - .set_one_reg(id, &get_fdt_addr(mem).to_le_bytes()) - .map_err(|err| VcpuArchError::SetOneReg(id, err))?; - - // Reset the physical counter for the guest. This way we avoid guest reading - // host physical counter. - // Resetting KVM_REG_ARM_PTIMER_CNT for single vcpu is enough because there is only - // one timer struct with offsets per VM. - // Because the access to KVM_REG_ARM_PTIMER_CNT is only present starting 6.4 kernel, - // we only do the reset if KVM_CAP_COUNTER_OFFSET is present as it was added - // in the same patch series as the ability to set the KVM_REG_ARM_PTIMER_CNT register. - // Path series which introduced the needed changes: - // https://lore.kernel.org/all/20230330174800.2677007-1-maz@kernel.org/ - // Note: the value observed by the guest will still be above 0, because there is a delta - // time between this resetting and first call to KVM_RUN. - if optional_capabilities.counter_offset { - vcpufd - .set_one_reg(KVM_REG_ARM_PTIMER_CNT, &[0; 8]) - .map_err(|err| VcpuArchError::SetOneReg(id, err))?; - } - } - Ok(()) -} - -/// Saves the states of the system registers into `state`. -/// -/// # Arguments -/// -/// * `regs` - Input/Output vector of registers. -pub fn get_all_registers( - vcpufd: &VcpuFd, - state: &mut Aarch64RegisterVec, -) -> Result<(), VcpuArchError> { - get_registers(vcpufd, &get_all_registers_ids(vcpufd)?, state) -} - /// Saves states of registers into `state`. /// /// # Arguments @@ -143,13 +69,13 @@ pub fn get_all_registers( /// * `ids` - Slice of registers ids to save. /// * `regs` - Input/Output vector of registers. pub fn get_registers( - vcpufd: &VcpuFd, + vcpu_fd: &VcpuFd, ids: &[u64], regs: &mut Aarch64RegisterVec, ) -> Result<(), VcpuArchError> { let mut big_reg = [0_u8; 256]; for id in ids.iter() { - let reg_size = vcpufd + let reg_size = vcpu_fd .get_one_reg(*id, &mut big_reg) .map_err(|e| VcpuArchError::GetOneReg(*id, e))?; let reg_ref = Aarch64RegisterRef::new(*id, &big_reg[0..reg_size]); @@ -158,66 +84,6 @@ pub fn get_registers( Ok(()) } -/// Returns all registers ids, including core and system -pub fn get_all_registers_ids(vcpufd: &VcpuFd) -> Result, VcpuArchError> { - // Call KVM_GET_REG_LIST to get all registers available to the guest. For ArmV8 there are - // less than 500 registers expected, resize to the reported size when necessary. - let mut reg_list = RegList::new(500).map_err(VcpuArchError::Fam)?; - - match vcpufd.get_reg_list(&mut reg_list) { - Ok(_) => Ok(reg_list.as_slice().to_vec()), - Err(e) => match e.errno() { - libc::E2BIG => { - // resize and retry. - let size: usize = reg_list - .as_fam_struct_ref() - .n - .try_into() - // Safe to unwrap as Firecracker only targets 64-bit machines. - .unwrap(); - reg_list = RegList::new(size).map_err(VcpuArchError::Fam)?; - vcpufd - .get_reg_list(&mut reg_list) - .map_err(VcpuArchError::GetRegList)?; - - Ok(reg_list.as_slice().to_vec()) - } - _ => Err(VcpuArchError::GetRegList(e)), - }, - } -} - -/// Set the state of one system register. -/// -/// # Arguments -/// -/// * `reg` - Register to be set. -pub fn set_register(vcpufd: &VcpuFd, reg: Aarch64RegisterRef) -> Result<(), VcpuArchError> { - vcpufd - .set_one_reg(reg.id, reg.as_slice()) - .map_err(|e| VcpuArchError::SetOneReg(reg.id, e))?; - Ok(()) -} - -/// Get the multistate processor. -/// -/// # Arguments -/// -/// * `vcpu` - Structure for the VCPU that holds the VCPU's fd. -pub fn get_mpstate(vcpufd: &VcpuFd) -> Result { - vcpufd.get_mp_state().map_err(VcpuArchError::GetMp) -} - -/// Set the state of the system registers. -/// -/// # Arguments -/// -/// * `vcpu` - Structure for the VCPU that holds the VCPU's fd. -/// * `state` - Structure for returning the state of the system registers. -pub fn set_mpstate(vcpufd: &VcpuFd, state: kvm_mp_state) -> Result<(), VcpuArchError> { - vcpufd.set_mp_state(state).map_err(VcpuArchError::SetMp) -} - /// Errors associated with the wrappers over KVM ioctls. #[derive(Debug, PartialEq, Eq, thiserror::Error, displaydoc::Display)] pub enum KvmVcpuError { @@ -319,9 +185,7 @@ impl KvmVcpu { })?; } - setup_boot_regs( - &self.fd, - self.index, + self.setup_boot_regs( kernel_entry_point.entry_addr.raw_value(), guest_mem, optional_capabilities, @@ -364,10 +228,11 @@ impl KvmVcpu { /// Save the KVM internal state. pub fn save_state(&self) -> Result { let mut state = VcpuState { - mp_state: get_mpstate(&self.fd).map_err(KvmVcpuError::SaveState)?, + mp_state: self.get_mpstate().map_err(KvmVcpuError::SaveState)?, ..Default::default() }; - get_all_registers(&self.fd, &mut state.regs).map_err(KvmVcpuError::SaveState)?; + self.get_all_registers(&mut state.regs) + .map_err(KvmVcpuError::SaveState)?; state.mpidr = self.get_mpidr().map_err(KvmVcpuError::SaveState)?; state.kvi = self.kvi; @@ -393,7 +258,8 @@ impl KvmVcpu { .iter() .find(|reg| reg.id == KVM_REG_ARM64_SVE_VLS) { - set_register(&self.fd, sve_vls_reg).map_err(KvmVcpuError::RestoreState)?; + self.set_register(sve_vls_reg) + .map_err(KvmVcpuError::RestoreState)?; } self.finalize_vcpu()?; @@ -405,21 +271,21 @@ impl KvmVcpu { .iter() .filter(|reg| reg.id != KVM_REG_ARM64_SVE_VLS) { - set_register(&self.fd, reg).map_err(KvmVcpuError::RestoreState)?; + self.set_register(reg).map_err(KvmVcpuError::RestoreState)?; } - set_mpstate(&self.fd, state.mp_state).map_err(KvmVcpuError::RestoreState)?; + self.set_mpstate(state.mp_state) + .map_err(KvmVcpuError::RestoreState)?; Ok(()) } /// Dumps CPU configuration. pub fn dump_cpu_config(&self) -> Result { - let reg_list = get_all_registers_ids(&self.fd).map_err(KvmVcpuError::DumpCpuConfig)?; - let mut regs = Aarch64RegisterVec::default(); - get_registers(&self.fd, ®_list, &mut regs).map_err(KvmVcpuError::DumpCpuConfig)?; - + self.get_all_registers(&mut regs) + .map_err(KvmVcpuError::DumpCpuConfig)?; Ok(CpuConfiguration { regs }) } + /// Initializes internal vcpufd. fn init_vcpu(&self) -> Result<(), KvmVcpuError> { self.fd.vcpu_init(&self.kvi).map_err(KvmVcpuError::Init)?; @@ -437,6 +303,136 @@ impl KvmVcpu { } Ok(()) } + + /// Configure relevant boot registers for a given vCPU. + /// + /// # Arguments + /// + /// * `boot_ip` - Starting instruction pointer. + /// * `mem` - Reserved DRAM for current VM. + /// + `optional_capabilities` - which optional capabilities are enabled that might influence + /// vcpu configuration + pub fn setup_boot_regs( + &self, + boot_ip: u64, + mem: &GuestMemoryMmap, + optional_capabilities: &OptionalCapabilities, + ) -> Result<(), VcpuArchError> { + let kreg_off = offset_of!(kvm_regs, regs); + + // Get the register index of the PSTATE (Processor State) register. + let pstate = offset_of!(user_pt_regs, pstate) + kreg_off; + let id = arm64_core_reg_id!(KVM_REG_SIZE_U64, pstate); + self.fd + .set_one_reg(id, &PSTATE_FAULT_BITS_64.to_le_bytes()) + .map_err(|err| VcpuArchError::SetOneReg(id, err))?; + + // Other vCPUs are powered off initially awaiting PSCI wakeup. + if self.index == 0 { + // Setting the PC (Processor Counter) to the current program address (kernel address). + let pc = offset_of!(user_pt_regs, pc) + kreg_off; + let id = arm64_core_reg_id!(KVM_REG_SIZE_U64, pc); + self.fd + .set_one_reg(id, &boot_ip.to_le_bytes()) + .map_err(|err| VcpuArchError::SetOneReg(id, err))?; + + // Last mandatory thing to set -> the address pointing to the FDT (also called DTB). + // "The device tree blob (dtb) must be placed on an 8-byte boundary and must + // not exceed 2 megabytes in size." -> https://www.kernel.org/doc/Documentation/arm64/booting.txt. + // We are choosing to place it the end of DRAM. See `get_fdt_addr`. + let regs0 = offset_of!(user_pt_regs, regs) + kreg_off; + let id = arm64_core_reg_id!(KVM_REG_SIZE_U64, regs0); + self.fd + .set_one_reg(id, &get_fdt_addr(mem).to_le_bytes()) + .map_err(|err| VcpuArchError::SetOneReg(id, err))?; + + // Reset the physical counter for the guest. This way we avoid guest reading + // host physical counter. + // Resetting KVM_REG_ARM_PTIMER_CNT for single vcpu is enough because there is only + // one timer struct with offsets per VM. + // Because the access to KVM_REG_ARM_PTIMER_CNT is only present starting 6.4 kernel, + // we only do the reset if KVM_CAP_COUNTER_OFFSET is present as it was added + // in the same patch series as the ability to set the KVM_REG_ARM_PTIMER_CNT register. + // Path series which introduced the needed changes: + // https://lore.kernel.org/all/20230330174800.2677007-1-maz@kernel.org/ + // Note: the value observed by the guest will still be above 0, because there is a delta + // time between this resetting and first call to KVM_RUN. + if optional_capabilities.counter_offset { + self.fd + .set_one_reg(KVM_REG_ARM_PTIMER_CNT, &[0; 8]) + .map_err(|err| VcpuArchError::SetOneReg(id, err))?; + } + } + Ok(()) + } + + /// Saves the states of the system registers into `state`. + /// + /// # Arguments + /// + /// * `regs` - Input/Output vector of registers. + pub fn get_all_registers(&self, state: &mut Aarch64RegisterVec) -> Result<(), VcpuArchError> { + get_registers(&self.fd, &self.get_all_registers_ids()?, state) + } + + /// Returns all registers ids, including core and system + pub fn get_all_registers_ids(&self) -> Result, VcpuArchError> { + // Call KVM_GET_REG_LIST to get all registers available to the guest. For ArmV8 there are + // less than 500 registers expected, resize to the reported size when necessary. + let mut reg_list = RegList::new(500).map_err(VcpuArchError::Fam)?; + + match self.fd.get_reg_list(&mut reg_list) { + Ok(_) => Ok(reg_list.as_slice().to_vec()), + Err(e) => match e.errno() { + libc::E2BIG => { + // resize and retry. + let size: usize = reg_list + .as_fam_struct_ref() + .n + .try_into() + // Safe to unwrap as Firecracker only targets 64-bit machines. + .unwrap(); + reg_list = RegList::new(size).map_err(VcpuArchError::Fam)?; + self.fd + .get_reg_list(&mut reg_list) + .map_err(VcpuArchError::GetRegList)?; + + Ok(reg_list.as_slice().to_vec()) + } + _ => Err(VcpuArchError::GetRegList(e)), + }, + } + } + + /// Set the state of one system register. + /// + /// # Arguments + /// + /// * `reg` - Register to be set. + pub fn set_register(&self, reg: Aarch64RegisterRef) -> Result<(), VcpuArchError> { + self.fd + .set_one_reg(reg.id, reg.as_slice()) + .map_err(|e| VcpuArchError::SetOneReg(reg.id, e))?; + Ok(()) + } + + /// Get the multistate processor. + /// + /// # Arguments + /// + /// * `vcpu` - Structure for the VCPU that holds the VCPU's fd. + pub fn get_mpstate(&self) -> Result { + self.fd.get_mp_state().map_err(VcpuArchError::GetMp) + } + + /// Set the state of the system registers. + /// + /// # Arguments + /// + /// * `state` - Structure for returning the state of the system registers. + pub fn set_mpstate(&self, state: kvm_mp_state) -> Result<(), VcpuArchError> { + self.fd.set_mp_state(state).map_err(VcpuArchError::SetMp) + } } impl Peripherals { @@ -511,10 +507,15 @@ mod tests { use crate::vstate::vm::tests::setup_vm_with_memory; fn setup_vcpu(mem_size: usize) -> (Kvm, Vm, KvmVcpu, GuestMemoryMmap) { - let (kvm, mut vm, vm_mem) = setup_vm_with_memory(mem_size); - let mut vcpu = KvmVcpu::new(0, &vm).unwrap(); + let (kvm, mut vm, mut vcpu, vm_mem) = setup_vcpu_no_init(mem_size); vcpu.init(&[]).unwrap(); vm.setup_irqchip(1).unwrap(); + (kvm, vm, vcpu, vm_mem) + } + + fn setup_vcpu_no_init(mem_size: usize) -> (Kvm, Vm, KvmVcpu, GuestMemoryMmap) { + let (kvm, vm, vm_mem) = setup_vm_with_memory(mem_size); + let vcpu = KvmVcpu::new(0, &vm).unwrap(); (kvm, vm, vcpu, vm_mem) } @@ -692,23 +693,20 @@ mod tests { #[test] fn test_setup_regs() { - let kvm = Kvm::new(vec![]).unwrap(); - let vm = kvm.fd.create_vm().unwrap(); - let vcpu = vm.create_vcpu(0).unwrap(); + let (kvm, _, vcpu, _) = setup_vcpu_no_init(0x10000); let mem = arch_mem(layout::FDT_MAX_SIZE + 0x1000); let optional_capabilities = kvm.optional_capabilities(); - let res = setup_boot_regs(&vcpu, 0, 0x0, &mem, &optional_capabilities); + let res = vcpu.setup_boot_regs(0x0, &mem, &optional_capabilities); assert!(matches!( res.unwrap_err(), VcpuArchError::SetOneReg(0x6030000000100042, _) )); - let mut kvi: kvm_bindings::kvm_vcpu_init = kvm_bindings::kvm_vcpu_init::default(); - vm.get_preferred_target(&mut kvi).unwrap(); - vcpu.vcpu_init(&kvi).unwrap(); + vcpu.init_vcpu().unwrap(); - setup_boot_regs(&vcpu, 0, 0x0, &mem, &optional_capabilities).unwrap(); + vcpu.setup_boot_regs(0x0, &mem, &optional_capabilities) + .unwrap(); // Check that the register is reset on compatible kernels. // Because there is a delta in time between we reset the register and time we @@ -716,7 +714,7 @@ mod tests { // small value. if optional_capabilities.counter_offset { let mut reg_bytes = [0_u8; 8]; - vcpu.get_one_reg(SYS_CNTPCT_EL0, &mut reg_bytes).unwrap(); + vcpu.fd.get_one_reg(SYS_CNTPCT_EL0, &mut reg_bytes).unwrap(); let counter_value = u64::from_le_bytes(reg_bytes); // We are reading the SYS_CNTPCT_EL0 right after resetting it. @@ -732,7 +730,7 @@ mod tests { #[test] fn test_read_mpidr() { - let (_, _, vcpu, _) = setup_vcpu(0x10000); + let (_, _, vcpu, _) = setup_vcpu_no_init(0x10000); // Must fail when vcpu is not initialized yet. let res = vcpu.get_mpidr(); @@ -740,28 +738,24 @@ mod tests { res.unwrap_err(), VcpuArchError::GetOneReg(MPIDR_EL1, _) )); - vcpu.init_vcpu().unwrap(); + assert_eq!(vcpu.get_mpidr().unwrap(), 0x8000_0000); } #[test] fn test_get_set_regs() { - let kvm = Kvm::new(vec![]).unwrap(); - let vm = kvm.fd.create_vm().unwrap(); - let vcpu = vm.create_vcpu(0).unwrap(); - let mut kvi: kvm_bindings::kvm_vcpu_init = kvm_bindings::kvm_vcpu_init::default(); - vm.get_preferred_target(&mut kvi).unwrap(); + let (_, _, vcpu, _) = setup_vcpu_no_init(0x10000); // Must fail when vcpu is not initialized yet. let mut regs = Aarch64RegisterVec::default(); - let res = get_all_registers(&vcpu, &mut regs); + let res = vcpu.get_all_registers(&mut regs); assert!(matches!(res.unwrap_err(), VcpuArchError::GetRegList(_))); + vcpu.init_vcpu().unwrap(); - vcpu.vcpu_init(&kvi).unwrap(); - get_all_registers(&vcpu, &mut regs).unwrap(); + vcpu.get_all_registers(&mut regs).unwrap(); for reg in regs.iter() { - set_register(&vcpu, reg).unwrap(); + vcpu.set_register(reg).unwrap(); } } @@ -769,21 +763,17 @@ mod tests { fn test_mpstate() { use std::os::unix::io::AsRawFd; - let kvm = Kvm::new(vec![]).unwrap(); - let vm = kvm.fd.create_vm().unwrap(); - let vcpu = vm.create_vcpu(0).unwrap(); - let mut kvi: kvm_bindings::kvm_vcpu_init = kvm_bindings::kvm_vcpu_init::default(); - vm.get_preferred_target(&mut kvi).unwrap(); + let (_, _, vcpu, _) = setup_vcpu(0x10000); - let res = get_mpstate(&vcpu); - set_mpstate(&vcpu, res.unwrap()).unwrap(); + let res = vcpu.get_mpstate(); + vcpu.set_mpstate(res.unwrap()).unwrap(); - unsafe { libc::close(vcpu.as_raw_fd()) }; + unsafe { libc::close(vcpu.fd.as_raw_fd()) }; - let res = get_mpstate(&vcpu); + let res = vcpu.get_mpstate(); assert!(matches!(res, Err(VcpuArchError::GetMp(_))), "{:?}", res); - let res = set_mpstate(&vcpu, kvm_mp_state::default()); + let res = vcpu.set_mpstate(kvm_mp_state::default()); assert!(matches!(res, Err(VcpuArchError::SetMp(_))), "{:?}", res); // dropping vcpu would double close the fd, so leak it From 1037ab8522ae32873dd3872c8adcf092c5ed44da Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 26 Mar 2025 14:07:12 +0000 Subject: [PATCH 4/4] refactor: eliminate map_err from device registration in builder.rs Remove some more .map_err() noise in the mmio device registration code in builder.rs by using #[from] and returning more specialized error types. Signed-off-by: Patrick Roy --- src/vmm/src/builder.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index 360b255be44..9b5924426a6 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -28,7 +28,7 @@ use crate::cpu_config::templates::{ use crate::device_manager::acpi::ACPIDeviceManager; #[cfg(target_arch = "x86_64")] use crate::device_manager::legacy::PortIODeviceManager; -use crate::device_manager::mmio::MMIODeviceManager; +use crate::device_manager::mmio::{MMIODeviceManager, MmioError}; use crate::device_manager::persist::{ ACPIDeviceManagerConstructorArgs, ACPIDeviceManagerRestoreError, MMIODevManagerConstructorArgs, }; @@ -590,9 +590,7 @@ fn attach_virtio_device( device: Arc>, cmdline: &mut LoaderKernelCmdline, is_vhost_user: bool, -) -> Result<(), StartMicrovmError> { - use self::StartMicrovmError::*; - +) -> Result<(), MmioError> { event_manager.add_subscriber(device.clone()); // The device mutex mustn't be locked here otherwise it will deadlock. @@ -605,21 +603,17 @@ fn attach_virtio_device( device, cmdline, ) - .map_err(RegisterMmioDevice) .map(|_| ()) } pub(crate) fn attach_boot_timer_device( vmm: &mut Vmm, request_ts: TimestampUs, -) -> Result<(), StartMicrovmError> { - use self::StartMicrovmError::*; - +) -> Result<(), MmioError> { let boot_timer = crate::devices::pseudo::BootTimer::new(request_ts); vmm.mmio_device_manager - .register_mmio_boot_timer(&mut vmm.resource_allocator, boot_timer) - .map_err(RegisterMmioDevice)?; + .register_mmio_boot_timer(&mut vmm.resource_allocator, boot_timer)?; Ok(()) } @@ -640,7 +634,7 @@ fn attach_entropy_device( cmdline: &mut LoaderKernelCmdline, entropy_device: &Arc>, event_manager: &mut EventManager, -) -> Result<(), StartMicrovmError> { +) -> Result<(), MmioError> { let id = entropy_device .lock() .expect("Poisoned lock") @@ -710,7 +704,7 @@ fn attach_unixsock_vsock_device( cmdline: &mut LoaderKernelCmdline, unix_vsock: &Arc>>, event_manager: &mut EventManager, -) -> Result<(), StartMicrovmError> { +) -> Result<(), MmioError> { let id = String::from(unix_vsock.lock().expect("Poisoned lock").id()); // The device mutex mustn't be locked here otherwise it will deadlock. attach_virtio_device(event_manager, vmm, id, unix_vsock.clone(), cmdline, false) @@ -721,7 +715,7 @@ fn attach_balloon_device( cmdline: &mut LoaderKernelCmdline, balloon: &Arc>, event_manager: &mut EventManager, -) -> Result<(), StartMicrovmError> { +) -> Result<(), MmioError> { let id = String::from(balloon.lock().expect("Poisoned lock").id()); // The device mutex mustn't be locked here otherwise it will deadlock. attach_virtio_device(event_manager, vmm, id, balloon.clone(), cmdline, false)