From f5a52eda2bd54c211bbc65cee932a15bfb33e0c7 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 24 Jan 2020 07:51:15 +0100 Subject: [PATCH] arch: Fix map_err losing the inner error Signed-off-by: Sebastien Boeuf --- arch/src/lib.rs | 4 ++-- arch/src/x86_64/mod.rs | 10 ++------- arch/src/x86_64/mptable.rs | 42 +++++++++++++++++++------------------- arch/src/x86_64/regs.rs | 28 ++++++++++++------------- 4 files changed, 39 insertions(+), 45 deletions(-) diff --git a/arch/src/lib.rs b/arch/src/lib.rs index bf0e52e46..75f78696f 100644 --- a/arch/src/lib.rs +++ b/arch/src/lib.rs @@ -22,7 +22,7 @@ extern crate vm_memory; use std::result; -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum Error { #[cfg(target_arch = "x86_64")] /// X86_64 specific error triggered during system configuration. @@ -30,7 +30,7 @@ pub enum Error { /// The zero page extends past the end of guest_mem. ZeroPagePastRamEnd, /// Error writing the zero page of guest memory. - ZeroPageSetup, + ZeroPageSetup(vm_memory::GuestMemoryError), } pub type Result = result::Result; diff --git a/arch/src/x86_64/mod.rs b/arch/src/x86_64/mod.rs index b2299be01..223d742a7 100644 --- a/arch/src/x86_64/mod.rs +++ b/arch/src/x86_64/mod.rs @@ -32,7 +32,7 @@ struct BootParamsWrapper(boot_params); // It is safe to initialize BootParamsWrap which is a wrapper over `boot_params` (a series of ints). unsafe impl ByteValued for BootParamsWrapper {} -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum Error { /// Invalid e820 setup params. E820Configuration, @@ -177,7 +177,7 @@ pub fn configure_system( .ok_or(super::Error::ZeroPagePastRamEnd)?; guest_mem .write_obj(params, zero_page_addr) - .map_err(|_| super::Error::ZeroPageSetup)?; + .map_err(super::Error::ZeroPageSetup)?; Ok(()) } @@ -229,12 +229,6 @@ mod tests { let gm = GuestMemoryMmap::new(&vec![(GuestAddress(0), 0x10000)]).unwrap(); let config_err = configure_system(&gm, GuestAddress(0), 0, 1, None, None); assert!(config_err.is_err()); - assert_eq!( - config_err.unwrap_err(), - super::super::Error::X86_64Setup(super::Error::MpTableSetup( - mptable::Error::NotEnoughMemory - )) - ); // Now assigning some memory that falls before the 32bit memory hole. let mem_size = 128 << 20; diff --git a/arch/src/x86_64/mptable.rs b/arch/src/x86_64/mptable.rs index 54fceaa0c..11d83d71c 100644 --- a/arch/src/x86_64/mptable.rs +++ b/arch/src/x86_64/mptable.rs @@ -14,7 +14,7 @@ use libc::c_char; use arch_gen::x86::mpspec; use layout::{APIC_START, IOAPIC_START, MPTABLE_START}; -use vm_memory::{Address, ByteValued, Bytes, GuestMemory, GuestMemoryMmap}; +use vm_memory::{Address, ByteValued, Bytes, GuestMemory, GuestMemoryError, GuestMemoryMmap}; // This is a workaround to the Rust enforcement specifying that any implementation of a foreign // trait (in this case `ByteValued`) where: @@ -45,30 +45,30 @@ unsafe impl ByteValued for MpcTableWrapper {} unsafe impl ByteValued for MpcLintsrcWrapper {} unsafe impl ByteValued for MpfIntelWrapper {} -#[derive(Debug, PartialEq)] +#[derive(Debug)] pub enum Error { /// There was too little guest memory to store the entire MP table. NotEnoughMemory, /// The MP table has too little address space to be stored. AddressOverflow, /// Failure while zeroing out the memory for the MP table. - Clear, + Clear(GuestMemoryError), /// Number of CPUs exceeds the maximum supported CPUs TooManyCpus, /// Failure to write the MP floating pointer. - WriteMpfIntel, + WriteMpfIntel(GuestMemoryError), /// Failure to write MP CPU entry. - WriteMpcCpu, + WriteMpcCpu(GuestMemoryError), /// Failure to write MP ioapic entry. - WriteMpcIoapic, + WriteMpcIoapic(GuestMemoryError), /// Failure to write MP bus entry. - WriteMpcBus, + WriteMpcBus(GuestMemoryError), /// Failure to write MP interrupt source entry. - WriteMpcIntsrc, + WriteMpcIntsrc(GuestMemoryError), /// Failure to write MP local interrupt source entry. - WriteMpcLintsrc, + WriteMpcLintsrc(GuestMemoryError), /// Failure to write MP table header. - WriteMpcTable, + WriteMpcTable(GuestMemoryError), } pub type Result = result::Result; @@ -145,7 +145,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { } mem.read_exact_from(base_mp, &mut io::repeat(0), mp_size) - .map_err(|_| Error::Clear)?; + .map_err(Error::Clear)?; { let mut mpf_intel = MpfIntelWrapper(mpspec::mpf_intel::default()); @@ -156,7 +156,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { mpf_intel.0.physptr = (base_mp.raw_value() + size) as u32; mpf_intel.0.checksum = mpf_intel_compute_checksum(&mpf_intel.0); mem.write_obj(mpf_intel, base_mp) - .map_err(|_| Error::WriteMpfIntel)?; + .map_err(Error::WriteMpfIntel)?; base_mp = base_mp.unchecked_add(size); } @@ -181,7 +181,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { mpc_cpu.0.cpufeature = CPU_STEPPING; mpc_cpu.0.featureflag = CPU_FEATURE_APIC | CPU_FEATURE_FPU; mem.write_obj(mpc_cpu, base_mp) - .map_err(|_| Error::WriteMpcCpu)?; + .map_err(Error::WriteMpcCpu)?; base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_cpu.0)); } @@ -193,7 +193,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { mpc_bus.0.busid = 0; mpc_bus.0.bustype = BUS_TYPE_ISA; mem.write_obj(mpc_bus, base_mp) - .map_err(|_| Error::WriteMpcBus)?; + .map_err(Error::WriteMpcBus)?; base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_bus.0)); } @@ -206,7 +206,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { mpc_ioapic.0.flags = mpspec::MPC_APIC_USABLE as u8; mpc_ioapic.0.apicaddr = IOAPIC_START.0 as u32; mem.write_obj(mpc_ioapic, base_mp) - .map_err(|_| Error::WriteMpcIoapic)?; + .map_err(Error::WriteMpcIoapic)?; base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_ioapic.0)); } @@ -222,7 +222,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { mpc_intsrc.0.dstapic = ioapicid; mpc_intsrc.0.dstirq = i; mem.write_obj(mpc_intsrc, base_mp) - .map_err(|_| Error::WriteMpcIntsrc)?; + .map_err(Error::WriteMpcIntsrc)?; base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_intsrc.0)); } @@ -237,7 +237,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { mpc_lintsrc.0.destapic = 0; mpc_lintsrc.0.destapiclint = 0; mem.write_obj(mpc_lintsrc, base_mp) - .map_err(|_| Error::WriteMpcLintsrc)?; + .map_err(Error::WriteMpcLintsrc)?; base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_lintsrc.0)); } @@ -252,7 +252,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { mpc_lintsrc.0.destapic = 0xFF; /* to all local APICs */ mpc_lintsrc.0.destapiclint = 1; mem.write_obj(mpc_lintsrc, base_mp) - .map_err(|_| Error::WriteMpcLintsrc)?; + .map_err(Error::WriteMpcLintsrc)?; base_mp = base_mp.unchecked_add(size as u64); checksum = checksum.wrapping_add(compute_checksum(&mpc_lintsrc.0)); } @@ -271,7 +271,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { checksum = checksum.wrapping_add(compute_checksum(&mpc_table.0)); mpc_table.0.checksum = (!checksum).wrapping_add(1) as i8; mem.write_obj(mpc_table, table_base) - .map_err(|_| Error::WriteMpcTable)?; + .map_err(Error::WriteMpcTable)?; } Ok(()) @@ -393,7 +393,7 @@ mod tests { let cpus = MAX_SUPPORTED_CPUS + 1; let mem = GuestMemoryMmap::new(&[(MPTABLE_START, compute_mp_size(cpus as u8))]).unwrap(); - let result = setup_mptable(&mem, cpus as u8).unwrap_err(); - assert_eq!(result, Error::TooManyCpus); + let result = setup_mptable(&mem, cpus as u8); + assert!(result.is_err()); } } diff --git a/arch/src/x86_64/regs.rs b/arch/src/x86_64/regs.rs index 1f65ed1f9..e0cc9b2da 100644 --- a/arch/src/x86_64/regs.rs +++ b/arch/src/x86_64/regs.rs @@ -12,7 +12,7 @@ use arch_gen::x86::msr_index; use kvm_bindings::{kvm_fpu, kvm_msr_entry, kvm_regs, kvm_sregs, Msrs}; use kvm_ioctls::VcpuFd; use layout::{BOOT_GDT_START, BOOT_IDT_START, PDE_START, PDPTE_START, PML4_START}; -use vm_memory::{Address, Bytes, GuestMemory, GuestMemoryMmap}; +use vm_memory::{Address, Bytes, GuestMemory, GuestMemoryError, GuestMemoryMmap}; // MTRR constants const MTRR_ENABLE: u64 = 0x800; // IA32_MTRR_DEF_TYPE MSR: E (MTRRs enabled) flag, bit 11 @@ -30,16 +30,18 @@ pub enum Error { SetModelSpecificRegisters(kvm_ioctls::Error), /// Failed to set SREGs for this CPU. SetStatusRegisters(kvm_ioctls::Error), + /// Checking the GDT address failed. + CheckGDTAddr, /// Writing the GDT to RAM failed. - WriteGDT, + WriteGDT(GuestMemoryError), /// Writing the IDT to RAM failed. - WriteIDT, + WriteIDT(GuestMemoryError), /// Writing PDPTE to RAM failed. - WritePDPTEAddress, + WritePDPTEAddress(GuestMemoryError), /// Writing PDE to RAM failed. - WritePDEAddress, + WritePDEAddress(GuestMemoryError), /// Writing PML4 to RAM failed. - WritePML4Address, + WritePML4Address(GuestMemoryError), } pub type Result = result::Result; @@ -121,10 +123,8 @@ fn write_gdt_table(table: &[u64], guest_mem: &GuestMemoryMmap) -> Result<()> { for (index, entry) in table.iter().enumerate() { let addr = guest_mem .checked_offset(boot_gdt_addr, index * mem::size_of::()) - .ok_or(Error::WriteGDT)?; - guest_mem - .write_obj(*entry, addr) - .map_err(|_| Error::WriteGDT)?; + .ok_or(Error::CheckGDTAddr)?; + guest_mem.write_obj(*entry, addr).map_err(Error::WriteGDT)?; } Ok(()) } @@ -133,7 +133,7 @@ fn write_idt_value(val: u64, guest_mem: &GuestMemoryMmap) -> Result<()> { let boot_idt_addr = BOOT_IDT_START; guest_mem .write_obj(val, boot_idt_addr) - .map_err(|_| Error::WriteIDT) + .map_err(Error::WriteIDT) } fn configure_segments_and_sregs(mem: &GuestMemoryMmap, sregs: &mut kvm_sregs) -> Result<()> { @@ -177,16 +177,16 @@ fn setup_page_tables(mem: &GuestMemoryMmap, sregs: &mut kvm_sregs) -> Result<()> // Entry covering VA [0..512GB) mem.write_obj(PDPTE_START.raw_value() | 0x03, PML4_START) - .map_err(|_| Error::WritePML4Address)?; + .map_err(Error::WritePML4Address)?; // Entry covering VA [0..1GB) mem.write_obj(PDE_START.raw_value() | 0x03, PDPTE_START) - .map_err(|_| Error::WritePDPTEAddress)?; + .map_err(Error::WritePDPTEAddress)?; // 512 2MB entries together covering VA [0..1GB). Note we are assuming // CPU supports 2MB pages (/proc/cpuinfo has 'pse'). All modern CPUs do. for i in 0..512 { mem.write_obj((i << 21) + 0x83u64, PDE_START.unchecked_add(i * 8)) - .map_err(|_| Error::WritePDEAddress)?; + .map_err(Error::WritePDEAddress)?; } sregs.cr3 = PML4_START.raw_value();