diff --git a/devices/src/bus.rs b/devices/src/bus.rs index 755e7e9e7e..e9a09640fa 100644 --- a/devices/src/bus.rs +++ b/devices/src/bus.rs @@ -700,11 +700,15 @@ impl Bus { /// Reads data from the device that owns the range containing `addr` and puts it into `data`. /// - /// Returns true on success, otherwise `data` is untouched. + /// Returns true on success, otherwise `data` is filled with zeroes. pub fn read(&self, addr: u64, data: &mut [u8]) -> bool { #[cfg(feature = "stats")] let start = self.stats.lock().start_stat(); + // Initialize `data` with all zeroes to ensure consistent results even if device `read()` + // implementations don't always fill every byte. + data.fill(0); + let device_index = if let Some((offset, address, entry)) = self.get_device(addr) { let io = BusAccessInfo { address, diff --git a/devices/tests/irqchip/userspace.rs b/devices/tests/irqchip/userspace.rs index 440d8f6612..29015f9551 100644 --- a/devices/tests/irqchip/userspace.rs +++ b/devices/tests/irqchip/userspace.rs @@ -674,13 +674,10 @@ impl Vcpu for FakeVcpu { unimplemented!() } - fn handle_mmio( - &self, - _handle_fn: &mut dyn FnMut(IoParams) -> Result>, - ) -> Result<()> { + fn handle_mmio(&self, _handle_fn: &mut dyn FnMut(IoParams) -> Result<()>) -> Result<()> { unimplemented!() } - fn handle_io(&self, _handle_fn: &mut dyn FnMut(IoParams) -> Option<[u8; 8]>) -> Result<()> { + fn handle_io(&self, _handle_fn: &mut dyn FnMut(IoParams)) -> Result<()> { unimplemented!() } fn on_suspend(&self) -> Result<()> { diff --git a/hypervisor/src/geniezone/mod.rs b/hypervisor/src/geniezone/mod.rs index 00cf469af7..f0240e5fbc 100644 --- a/hypervisor/src/geniezone/mod.rs +++ b/hypervisor/src/geniezone/mod.rs @@ -1210,10 +1210,7 @@ impl Vcpu for GeniezoneVcpu { } } - fn handle_mmio( - &self, - handle_fn: &mut dyn FnMut(IoParams) -> Result>, - ) -> Result<()> { + fn handle_mmio(&self, handle_fn: &mut dyn FnMut(IoParams) -> Result<()>) -> Result<()> { // SAFETY: // Safe because we know we mapped enough memory to hold the gzvm_vcpu_run struct because the // kernel told us how large it was. The pointer is page aligned so casting to a different @@ -1227,29 +1224,22 @@ impl Vcpu for GeniezoneVcpu { // union field to use. let mmio = unsafe { &mut run.__bindgen_anon_1.mmio }; let address = mmio.phys_addr; - - let size = mmio.size as usize; + let data = &mut mmio.data[..mmio.size as usize]; if mmio.is_write != 0 { handle_fn(IoParams { address, - size, - operation: IoOperation::Write { data: mmio.data }, - })?; - Ok(()) - } else if let Some(data) = handle_fn(IoParams { - address, - size, - operation: IoOperation::Read, - })? { - mmio.data[..size].copy_from_slice(&data[..size]); - Ok(()) + operation: IoOperation::Write(data), + }) } else { - Err(Error::new(EINVAL)) + handle_fn(IoParams { + address, + operation: IoOperation::Read(data), + }) } } - fn handle_io(&self, _handle_fn: &mut dyn FnMut(IoParams) -> Option<[u8; 8]>) -> Result<()> { + fn handle_io(&self, _handle_fn: &mut dyn FnMut(IoParams)) -> Result<()> { Err(Error::new(EINVAL)) } } diff --git a/hypervisor/src/gunyah/mod.rs b/hypervisor/src/gunyah/mod.rs index 8987a43cfe..d52b0e1906 100644 --- a/hypervisor/src/gunyah/mod.rs +++ b/hypervisor/src/gunyah/mod.rs @@ -6,7 +6,6 @@ mod aarch64; mod gunyah_sys; -use std::cmp::min; use std::cmp::Reverse; use std::collections::BTreeMap; use std::collections::BinaryHeap; @@ -810,10 +809,7 @@ impl Vcpu for GunyahVcpu { } } - fn handle_mmio( - &self, - handle_fn: &mut dyn FnMut(IoParams) -> Result>, - ) -> Result<()> { + fn handle_mmio(&self, handle_fn: &mut dyn FnMut(IoParams) -> Result<()>) -> Result<()> { // SAFETY: // Safe because we know we mapped enough memory to hold the gh_vcpu_run struct because the // kernel told us how large it was. The pointer is page aligned so casting to a different @@ -826,27 +822,21 @@ impl Vcpu for GunyahVcpu { // union field to use. let mmio = unsafe { &mut run.__bindgen_anon_1.mmio }; let address = mmio.phys_addr; - let size = min(mmio.len as usize, mmio.data.len()); + let data = &mut mmio.data[..mmio.len as usize]; if mmio.is_write != 0 { handle_fn(IoParams { address, - size, - operation: IoOperation::Write { data: mmio.data }, - })?; - Ok(()) - } else if let Some(data) = handle_fn(IoParams { - address, - size, - operation: IoOperation::Read, - })? { - mmio.data[..size].copy_from_slice(&data[..size]); - Ok(()) + operation: IoOperation::Write(data), + }) } else { - Err(Error::new(EINVAL)) + handle_fn(IoParams { + address, + operation: IoOperation::Read(data), + }) } } - fn handle_io(&self, _handle_fn: &mut dyn FnMut(IoParams) -> Option<[u8; 8]>) -> Result<()> { + fn handle_io(&self, _handle_fn: &mut dyn FnMut(IoParams)) -> Result<()> { unreachable!() } diff --git a/hypervisor/src/haxm/vcpu.rs b/hypervisor/src/haxm/vcpu.rs index 00b3387cab..144b54a84b 100644 --- a/hypervisor/src/haxm/vcpu.rs +++ b/hypervisor/src/haxm/vcpu.rs @@ -5,7 +5,6 @@ use core::ffi::c_void; use std::arch::x86_64::CpuidResult; use std::collections::BTreeMap; -use std::intrinsics::copy_nonoverlapping; use std::mem::size_of; use base::errno_result; @@ -176,10 +175,7 @@ impl Vcpu for HaxmVcpu { /// Once called, it will determine whether a mmio read or mmio write was the reason for the mmio /// exit, call `handle_fn` with the respective IoOperation to perform the mmio read or /// write, and set the return data in the vcpu so that the vcpu can resume running. - fn handle_mmio( - &self, - handle_fn: &mut dyn FnMut(IoParams) -> Result>, - ) -> Result<()> { + fn handle_mmio(&self, handle_fn: &mut dyn FnMut(IoParams) -> Result<()>) -> Result<()> { // SAFETY: // Safe because we know we mapped enough memory to hold the hax_tunnel struct because the // kernel told us how large it was. @@ -193,39 +189,34 @@ impl Vcpu for HaxmVcpu { // Safe because the exit_reason (which comes from the kernel) told us which // union field to use. unsafe { ((*mmio).gpa, (*mmio).size as usize, (*mmio).direction) }; + // SAFETY: + // Safe because the exit_reason (which comes from the kernel) told us which + // union field to use. We use `addr_of_mut!()` to get a potentially unaligned u64 pointer, + // but it is then cast via a u8 pointer to a u8 slice, which has no alignment requirements. + let data = unsafe { + assert!(size <= size_of::()); + std::slice::from_raw_parts_mut( + std::ptr::addr_of_mut!((*mmio).__bindgen_anon_1.value) as *mut u8, + size, + ) + }; match direction { HAX_EXIT_DIRECTION_MMIO_READ => { - if let Some(data) = handle_fn(IoParams { + handle_fn(IoParams { address, - size, - operation: IoOperation::Read, + operation: IoOperation::Read(data), }) // We have to unwrap/panic here because HAXM doesn't have a // facility to inject a GP fault here. Once HAXM can do that, we // should inject a GP fault & bubble the error. - .unwrap() - { - let data = u64::from_ne_bytes(data); - // SAFETY: - // Safe because we know this is an mmio read, so we need to put data into the - // "value" field of the hax_fastmmio. - unsafe { - (*mmio).__bindgen_anon_1.value = data; - } - } + .unwrap(); Ok(()) } HAX_EXIT_DIRECTION_MMIO_WRITE => { - // SAFETY: - // safe because we trust haxm to fill in the union properly. - let data = unsafe { (*mmio).__bindgen_anon_1.value }; handle_fn(IoParams { address, - size, - operation: IoOperation::Write { - data: data.to_ne_bytes(), - }, + operation: IoOperation::Write(data), }) // Similarly to the read direction, we MUST panic here. .unwrap(); @@ -241,7 +232,7 @@ impl Vcpu for HaxmVcpu { /// call `handle_fn` with the respective IoOperation to perform the io in or io out, /// and set the return data in the vcpu so that the vcpu can resume running. #[allow(clippy::cast_ptr_alignment)] - fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams) -> Option<[u8; 8]>) -> Result<()> { + fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams)) -> Result<()> { // SAFETY: // Safe because we know we mapped enough memory to hold the hax_tunnel struct because the // kernel told us how large it was. @@ -256,40 +247,29 @@ impl Vcpu for HaxmVcpu { let address = io.port.into(); let size = io.size as usize; let count = io.count as usize; - let mut io_buffer_ptr = self.io_buffer as *mut u8; + let data_len = count * size; + // SAFETY: + // Safe because the exit_reason (which comes from the kernel) told us that this is port io, + // where the iobuf can be treated as a *u8 + let buffer: &mut [u8] = + unsafe { std::slice::from_raw_parts_mut(self.io_buffer as *mut u8, data_len) }; + let data_chunks = buffer.chunks_mut(size); + match io.direction as u32 { HAX_EXIT_DIRECTION_PIO_IN => { - for _ in 0..count { - if let Some(data) = handle_fn(IoParams { + for data in data_chunks { + handle_fn(IoParams { address, - size, - operation: IoOperation::Read, - }) { - // SAFETY: - // Safe because the exit_reason (which comes from the kernel) told us that - // this is port io, where the iobuf can be treated as a *u8 - unsafe { - copy_nonoverlapping(data.as_ptr(), io_buffer_ptr, size); - io_buffer_ptr = io_buffer_ptr.add(size); - } - } + operation: IoOperation::Read(data), + }); } Ok(()) } HAX_EXIT_DIRECTION_PIO_OUT => { - for _ in 0..count { - let mut data = [0; 8]; - // SAFETY: - // safe because we check the size, from what the kernel told us is the max to - // copy. - unsafe { - copy_nonoverlapping(io_buffer_ptr, data.as_mut_ptr(), size); - io_buffer_ptr = io_buffer_ptr.add(size); - } + for data in data_chunks { handle_fn(IoParams { address, - size, - operation: IoOperation::Write { data }, + operation: IoOperation::Write(data), }); } Ok(()) diff --git a/hypervisor/src/kvm/mod.rs b/hypervisor/src/kvm/mod.rs index be9d7aabfa..fc2788b41d 100644 --- a/hypervisor/src/kvm/mod.rs +++ b/hypervisor/src/kvm/mod.rs @@ -16,7 +16,6 @@ mod riscv64; #[cfg(target_arch = "x86_64")] mod x86_64; -use std::cmp::min; use std::cmp::Reverse; use std::collections::BTreeMap; use std::collections::BinaryHeap; @@ -27,7 +26,6 @@ use std::os::raw::c_ulong; use std::os::raw::c_void; use std::os::unix::prelude::OsStrExt; use std::path::Path; -use std::ptr::copy_nonoverlapping; use std::sync::Arc; use base::errno_result; @@ -1039,14 +1037,10 @@ impl Vcpu for KvmVcpu { } } - fn handle_mmio( - &self, - handle_fn: &mut dyn FnMut(IoParams) -> Result>, - ) -> Result<()> { + fn handle_mmio(&self, handle_fn: &mut dyn FnMut(IoParams) -> Result<()>) -> Result<()> { // SAFETY: // Safe because we know we mapped enough memory to hold the kvm_run struct because the - // kernel told us how large it was. The pointer is page aligned so casting to a different - // type is well defined, hence the clippy allow attribute. + // kernel told us how large it was. let run = unsafe { &mut *(self.run_mmap.as_ptr() as *mut kvm_run) }; // Verify that the handler is called in the right context. assert!(run.exit_reason == KVM_EXIT_MMIO); @@ -1055,31 +1049,24 @@ impl Vcpu for KvmVcpu { // union field to use. let mmio = unsafe { &mut run.__bindgen_anon_1.mmio }; let address = mmio.phys_addr; - let size = min(mmio.len as usize, mmio.data.len()); + let data = &mut mmio.data[..mmio.len as usize]; if mmio.is_write != 0 { handle_fn(IoParams { address, - size, - operation: IoOperation::Write { data: mmio.data }, - })?; - Ok(()) - } else if let Some(data) = handle_fn(IoParams { - address, - size, - operation: IoOperation::Read, - })? { - mmio.data[..size].copy_from_slice(&data[..size]); - Ok(()) + operation: IoOperation::Write(data), + }) } else { - Err(Error::new(EINVAL)) + handle_fn(IoParams { + address, + operation: IoOperation::Read(data), + }) } } - fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams) -> Option<[u8; 8]>) -> Result<()> { + fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams)) -> Result<()> { // SAFETY: // Safe because we know we mapped enough memory to hold the kvm_run struct because the - // kernel told us how large it was. The pointer is page aligned so casting to a different - // type is well defined, hence the clippy allow attribute. + // kernel told us how large it was. let run = unsafe { &mut *(self.run_mmap.as_ptr() as *mut kvm_run) }; // Verify that the handler is called in the right context. assert!(run.exit_reason == KVM_EXIT_IO); @@ -1087,52 +1074,42 @@ impl Vcpu for KvmVcpu { // Safe because the exit_reason (which comes from the kernel) told us which // union field to use. let io = unsafe { run.__bindgen_anon_1.io }; + let address = u64::from(io.port); let size = usize::from(io.size); + let count = io.count as usize; + let data_len = count * size; + let data_offset = io.data_offset as usize; + assert!(data_offset + data_len <= self.run_mmap.size()); // SAFETY: // The data_offset is defined by the kernel to be some number of bytes into the kvm_run // structure, which we have fully mmap'd. - let mut data_ptr = unsafe { (run as *mut kvm_run as *mut u8).add(io.data_offset as usize) }; + let buffer: &mut [u8] = unsafe { + std::slice::from_raw_parts_mut( + (run as *mut kvm_run as *mut u8).add(data_offset), + data_len, + ) + }; + let data_chunks = buffer.chunks_mut(size); - match io.direction as u32 { - KVM_EXIT_IO_IN => { - for _ in 0..io.count { - if let Some(data) = handle_fn(IoParams { - address: io.port.into(), - size, - operation: IoOperation::Read, - }) { - // TODO(b/315998194): Add safety comment - #[allow(clippy::undocumented_unsafe_blocks)] - unsafe { - copy_nonoverlapping(data.as_ptr(), data_ptr, size); - data_ptr = data_ptr.add(size); - } - } else { - return Err(Error::new(EINVAL)); - } - } - Ok(()) + if io.direction == KVM_EXIT_IO_IN as u8 { + for data in data_chunks { + handle_fn(IoParams { + address, + operation: IoOperation::Read(data), + }); } - KVM_EXIT_IO_OUT => { - for _ in 0..io.count { - let mut data = [0; 8]; - // TODO(b/315998194): Add safety comment - #[allow(clippy::undocumented_unsafe_blocks)] - unsafe { - copy_nonoverlapping(data_ptr, data.as_mut_ptr(), min(size, data.len())); - data_ptr = data_ptr.add(size); - } - handle_fn(IoParams { - address: io.port.into(), - size, - operation: IoOperation::Write { data }, - }); - } - Ok(()) + } else { + debug_assert_eq!(io.direction, KVM_EXIT_IO_OUT as u8); + for data in data_chunks { + handle_fn(IoParams { + address, + operation: IoOperation::Write(data), + }); } - _ => Err(Error::new(EINVAL)), } + + Ok(()) } } diff --git a/hypervisor/src/lib.rs b/hypervisor/src/lib.rs index ede57408da..8abc20bc34 100644 --- a/hypervisor/src/lib.rs +++ b/hypervisor/src/lib.rs @@ -268,23 +268,22 @@ pub trait Vm: Send { } /// Operation for Io and Mmio -#[derive(Copy, Clone, Debug)] -pub enum IoOperation { - Read, - Write { - /// Data to be written. - /// - /// For 64 bit architecture, Mmio and Io only work with at most 8 bytes of data. - data: [u8; 8], - }, +#[derive(Debug)] +pub enum IoOperation<'a> { + /// Data to be read from a device on the bus. + /// + /// The `handle_fn` should fill the entire slice with the read data. + Read(&'a mut [u8]), + + /// Data to be written to a device on the bus. + Write(&'a [u8]), } /// Parameters describing an MMIO or PIO from the guest. -#[derive(Copy, Clone, Debug)] -pub struct IoParams { +#[derive(Debug)] +pub struct IoParams<'a> { pub address: u64, - pub size: usize, - pub operation: IoOperation, + pub operation: IoOperation<'a>, } /// Handle to a virtual CPU that may be used to request a VM exit from within a signal handler. @@ -350,10 +349,7 @@ pub trait Vcpu: downcast_rs::DowncastSync { /// Once called, it will determine whether a MMIO read or MMIO write was the reason for the MMIO /// exit, call `handle_fn` with the respective IoParams to perform the MMIO read or write, and /// set the return data in the vcpu so that the vcpu can resume running. - fn handle_mmio( - &self, - handle_fn: &mut dyn FnMut(IoParams) -> Result>, - ) -> Result<()>; + fn handle_mmio(&self, handle_fn: &mut dyn FnMut(IoParams) -> Result<()>) -> Result<()>; /// Handles an incoming PIO from the guest. /// @@ -363,7 +359,7 @@ pub trait Vcpu: downcast_rs::DowncastSync { /// Once called, it will determine whether an input or output was the reason for the Io exit, /// call `handle_fn` with the respective IoParams to perform the input/output operation, and set /// the return data in the vcpu so that the vcpu can resume running. - fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams) -> Option<[u8; 8]>) -> Result<()>; + fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams)) -> Result<()>; /// Signals to the hypervisor that this Vcpu is being paused by userspace. fn on_suspend(&self) -> Result<()>; diff --git a/hypervisor/src/whpx/vcpu.rs b/hypervisor/src/whpx/vcpu.rs index 5f6297ced8..8fa7341b48 100644 --- a/hypervisor/src/whpx/vcpu.rs +++ b/hypervisor/src/whpx/vcpu.rs @@ -7,6 +7,7 @@ use std::arch::x86_64::CpuidResult; use std::collections::BTreeMap; use std::convert::TryInto; use std::mem::size_of; +use std::mem::size_of_val; use std::sync::Arc; use base::Error; @@ -105,8 +106,8 @@ trait InstructionEmulatorCallbacks { struct InstructionEmulatorContext<'a> { vm_partition: Arc, index: u32, - handle_mmio: Option<&'a mut dyn FnMut(IoParams) -> Result>>, - handle_io: Option<&'a mut dyn FnMut(IoParams) -> Option<[u8; 8]>>, + handle_mmio: Option<&'a mut dyn FnMut(IoParams) -> Result<()>>, + handle_io: Option<&'a mut dyn FnMut(IoParams)>, } impl InstructionEmulatorCallbacks for SafeInstructionEmulator { @@ -117,46 +118,33 @@ impl InstructionEmulatorCallbacks for SafeInstructionEmulator { // unsafe because windows could decide to call this at any time. // However, we trust the kernel to call this while the vm/vcpu is valid. let ctx = unsafe { &mut *(context as *mut InstructionEmulatorContext) }; + let Some(handle_io) = &mut ctx.handle_io else { + return E_UNEXPECTED; + }; + // safe because we trust the kernel to fill in the io_access let io_access_info = unsafe { &mut *io_access }; let address = io_access_info.Port.into(); let size = io_access_info.AccessSize as usize; + // SAFETY: We trust the kernel to fill in the io_access + let data: &mut [u8] = unsafe { + assert!(size <= size_of_val(&io_access_info.Data)); + std::slice::from_raw_parts_mut(&mut io_access_info.Data as *mut u32 as *mut u8, size) + }; match io_access_info.Direction { WHPX_EXIT_DIRECTION_PIO_IN => { - if let Some(handle_io) = &mut ctx.handle_io { - if let Some(data) = handle_io(IoParams { - address, - size, - operation: IoOperation::Read, - }) { - // Safe because we know this is an io_access_info field of u32, - // so casting as a &mut [u8] of len 4 is safe. - let buffer = unsafe { - std::slice::from_raw_parts_mut( - &mut io_access_info.Data as *mut u32 as *mut u8, - 4, - ) - }; - buffer[..size].copy_from_slice(&data[..size]); - } - S_OK - } else { - E_UNEXPECTED - } + handle_io(IoParams { + address, + operation: IoOperation::Read(data), + }); + S_OK } WHPX_EXIT_DIRECTION_PIO_OUT => { - if let Some(handle_io) = &mut ctx.handle_io { - handle_io(IoParams { - address, - size, - operation: IoOperation::Write { - data: (io_access_info.Data as u64).to_ne_bytes(), - }, - }); - S_OK - } else { - E_UNEXPECTED - } + handle_io(IoParams { + address, + operation: IoOperation::Write(data), + }); + S_OK } _ => E_UNEXPECTED, } @@ -168,49 +156,38 @@ impl InstructionEmulatorCallbacks for SafeInstructionEmulator { // unsafe because windows could decide to call this at any time. // However, we trust the kernel to call this while the vm/vcpu is valid. let ctx = unsafe { &mut *(context as *mut InstructionEmulatorContext) }; + let Some(handle_mmio) = &mut ctx.handle_mmio else { + return E_UNEXPECTED; + }; + // safe because we trust the kernel to fill in the memory_access let memory_access_info = unsafe { &mut *memory_access }; let address = memory_access_info.GpaAddress; let size = memory_access_info.AccessSize as usize; + let data = &mut memory_access_info.Data[..size]; + match memory_access_info.Direction { WHPX_EXIT_DIRECTION_MMIO_READ => { - ctx.handle_mmio - .as_mut() - .map_or(E_UNEXPECTED, |handle_mmio| { - handle_mmio(IoParams { - address, - size, - operation: IoOperation::Read, - }) - .map_err(|e| { - error!("handle_mmio failed with {e}"); - e - }) - .ok() - .flatten() - .map_or(E_UNEXPECTED, |data| { - memory_access_info.Data = data; - S_OK - }) - }) + if let Err(e) = handle_mmio(IoParams { + address, + operation: IoOperation::Read(data), + }) { + error!("handle_mmio failed with {e}"); + E_UNEXPECTED + } else { + S_OK + } } WHPX_EXIT_DIRECTION_MMIO_WRITE => { - ctx.handle_mmio - .as_mut() - .map_or(E_UNEXPECTED, |handle_mmio| { - handle_mmio(IoParams { - address, - size, - operation: IoOperation::Write { - data: memory_access_info.Data, - }, - }) - .map_err(|e| { - error!("handle_mmio failed with {e}"); - e - }) - .map_or(E_UNEXPECTED, |_| S_OK) - }) + if let Err(e) = handle_mmio(IoParams { + address, + operation: IoOperation::Write(data), + }) { + error!("handle_mmio write with {e}"); + E_UNEXPECTED + } else { + S_OK + } } _ => E_UNEXPECTED, } @@ -559,10 +536,7 @@ impl Vcpu for WhpxVcpu { /// Once called, it will determine whether a mmio read or mmio write was the reason for the mmio /// exit, call `handle_fn` with the respective IoOperation to perform the mmio read or /// write, and set the return data in the vcpu so that the vcpu can resume running. - fn handle_mmio( - &self, - handle_fn: &mut dyn FnMut(IoParams) -> Result>, - ) -> Result<()> { + fn handle_mmio(&self, handle_fn: &mut dyn FnMut(IoParams) -> Result<()>) -> Result<()> { let mut status: WHV_EMULATOR_STATUS = Default::default(); let mut ctx = InstructionEmulatorContext { vm_partition: self.vm_partition.clone(), @@ -596,7 +570,7 @@ impl Vcpu for WhpxVcpu { /// Once called, it will determine whether an io in or io out was the reason for the io exit, /// call `handle_fn` with the respective IoOperation to perform the io in or io out, /// and set the return data in the vcpu so that the vcpu can resume running. - fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams) -> Option<[u8; 8]>) -> Result<()> { + fn handle_io(&self, handle_fn: &mut dyn FnMut(IoParams)) -> Result<()> { let mut status: WHV_EMULATOR_STATUS = Default::default(); let mut ctx = InstructionEmulatorContext { vm_partition: self.vm_partition.clone(), diff --git a/hypervisor/tests/hypervisor_virtualization.rs b/hypervisor/tests/hypervisor_virtualization.rs index 85f8122e4e..fb12833a3e 100644 --- a/hypervisor/tests/hypervisor_virtualization.rs +++ b/hypervisor/tests/hypervisor_virtualization.rs @@ -884,27 +884,20 @@ fn test_io_exit_handler() { let exit_matcher = move |_, exit: &VcpuExit, vcpu: &mut dyn VcpuX86_64, _: &mut dyn Vm| match exit { VcpuExit::Io => { - vcpu.handle_io(&mut |IoParams { - address, - size, - operation, - }| { + vcpu.handle_io(&mut |IoParams { address, operation }| { match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; + IoOperation::Read(data) => { assert_eq!(address, 0x20); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); // The original number written below will be doubled and // passed back. data[0] = cached_byte.load(Ordering::SeqCst) * 2; - Some(data) } - IoOperation::Write { data } => { + IoOperation::Write(data) => { assert_eq!(address, 0x10); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); assert_eq!(data[0], 0x34); cached_byte.fetch_add(data[0], Ordering::SeqCst); - None } } }) @@ -964,27 +957,20 @@ fn test_io_rep_string() { let exit_matcher = move |_, exit: &VcpuExit, vcpu: &mut dyn VcpuX86_64, vm: &mut dyn Vm| match exit { VcpuExit::Io => { - vcpu.handle_io(&mut |IoParams { - address, - size, - operation, - }| { + vcpu.handle_io(&mut |IoParams { address, operation }| { match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; + IoOperation::Read(data) => { assert_eq!(address, 0x80); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); // Return 0, 1, 2, 3, 4 for subsequent reads. data[0] = read_data.fetch_add(1, Ordering::SeqCst); - Some(data) } - IoOperation::Write { data } => { + IoOperation::Write(data) => { assert_eq!(address, 0x80); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); // Expect 0, 1, 2, 3, 4 to be written. let expected_write = write_data.fetch_add(1, Ordering::SeqCst); assert_eq!(data[0], expected_write); - None } } }) @@ -1046,14 +1032,10 @@ fn test_mmio_exit_cross_page() { let exit_matcher = |_, exit: &VcpuExit, vcpu: &mut dyn VcpuX86_64, _: &mut dyn Vm| match exit { VcpuExit::Mmio => { - vcpu.handle_mmio(&mut |IoParams { - address, - size, - operation, - }| { + vcpu.handle_mmio(&mut |IoParams { address, operation }| { match operation { - IoOperation::Read => { - match (address, size) { + IoOperation::Read(data) => { + match (address, data.len()) { // First MMIO read asks to load the first 8 bytes // of a new execution page, when an instruction // crosses page boundary. @@ -1062,21 +1044,25 @@ fn test_mmio_exit_cross_page() { (0x1000, 8) => { // Ensure this instruction is the first read // in the sequence. - Ok(Some([0x88, 0x03, 0x67, 0x8a, 0x01, 0xf4, 0, 0])) + data.copy_from_slice(&[0x88, 0x03, 0x67, 0x8a, 0x01, 0xf4, 0, 0]); + Ok(()) } // Second MMIO read is a regular read from an // unmapped memory (pointed to by initial EAX). - (0x3010, 1) => Ok(Some([0x66, 0, 0, 0, 0, 0, 0, 0])), + (0x3010, 1) => { + data.copy_from_slice(&[0x66]); + Ok(()) + } _ => { - panic!("invalid address({:#x})/size({})", address, size) + panic!("invalid address({:#x})/size({})", address, data.len()) } } } - IoOperation::Write { data } => { + IoOperation::Write(data) => { assert_eq!(address, 0x3000); assert_eq!(data[0], 0x33); - assert_eq!(size, 1); - Ok(None) + assert_eq!(data.len(), 1); + Ok(()) } } }) @@ -1159,19 +1145,15 @@ fn test_mmio_exit_readonly_memory() { let exit_matcher = |_, exit: &VcpuExit, vcpu: &mut dyn VcpuX86_64, _: &mut dyn Vm| match exit { VcpuExit::Mmio => { - vcpu.handle_mmio(&mut |IoParams { - address, - size, - operation, - }| match operation { - IoOperation::Read => { + vcpu.handle_mmio(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(_) => { panic!("unexpected mmio read call"); } - IoOperation::Write { data } => { - assert_eq!(size, 1); + IoOperation::Write(data) => { + assert_eq!(data.len(), 1); assert_eq!(address, 0x5000); assert_eq!(data[0], 0x67); - Ok(None) + Ok(()) } }) .expect("failed to set the data"); @@ -2638,7 +2620,6 @@ fn test_interrupt_ready_when_normally_not_interruptible() { } instrumentation_traces.borrow_mut().push(instrumentation); // We are always handling out IO port, so no data to return. - None }) .expect("should handle IO successfully"); if should_inject_interrupt { @@ -2696,7 +2677,6 @@ fn test_interrupt_ready_when_interrupt_enable_flag_not_set() { vcpu.handle_io(&mut |io_params| { addr = io_params.address; // We are always handling out IO port, so no data to return. - None }) .expect("should handle IO successfully"); let regs = vcpu @@ -2942,7 +2922,7 @@ fn test_request_interrupt_window() { VcpuExit::Intr => false, VcpuExit::Io => { // We are always handling out IO port, so no data to return. - vcpu.handle_io(&mut |_| None) + vcpu.handle_io(&mut |_| {}) .expect("should handle IO successfully"); assert!(!vcpu.ready_for_interrupt()); @@ -3098,7 +3078,7 @@ fn test_mmx_state_is_preserved_by_hypervisor() { false } VcpuExit::Io => { - vcpu.handle_io(&mut |_| None) + vcpu.handle_io(&mut |_| {}) .expect("should handle IO successfully"); // kaiyili@ pointed out we should check the XSAVE state exposed by the hypervisor via @@ -3238,7 +3218,7 @@ fn test_avx_state_is_preserved_by_hypervisor() { false } VcpuExit::Io => { - vcpu.handle_io(&mut |_| None) + vcpu.handle_io(&mut |_| {}) .expect("should handle IO successfully"); // kaiyili@ pointed out we should check the XSAVE state exposed by the hypervisor via @@ -3660,7 +3640,7 @@ fn test_slat_on_region_removal_is_mmio() { // // We strictly don't care what this data is, since the VM exits before running any // further instructions. - vcpu.handle_io(&mut |_| None) + vcpu.handle_io(&mut |_| {}) .expect("should handle IO successfully"); // Remove the test memory region to cause a SLAT fault (in the passing case). @@ -3672,21 +3652,18 @@ fn test_slat_on_region_removal_is_mmio() { false } VcpuExit::Mmio => { - vcpu.handle_mmio(&mut |IoParams { - address, - size, - operation, - }| { + vcpu.handle_mmio(&mut |IoParams { address, operation }| { assert_eq!(address, 0x20000, "MMIO for wrong address"); - assert_eq!(size, 1); - assert!( - matches!(operation, IoOperation::Read), - "got unexpected IO operation {:?}", - operation - ); - // We won't vmenter again, so there's no need to actually satisfy the MMIO by - // returning data; however, some hypervisors (WHPX) require it. - Ok(Some([0u8; 8])) + match operation { + IoOperation::Read(data) => { + assert_eq!(data.len(), 1); + data[0] = 0; + Ok(()) + } + IoOperation::Write(_) => { + panic!("got unexpected IO operation {:?}", operation); + } + } }) .unwrap(); true @@ -3891,7 +3868,7 @@ fn test_interrupt_injection_when_not_ready() { VcpuExit::FailEntry { .. } | VcpuExit::Shutdown(..) | VcpuExit::Hlt => true, VcpuExit::Io => { // We are always handling out IO port, so no data to return. - vcpu.handle_io(&mut |_| None) + vcpu.handle_io(&mut |_| {}) .expect("should handle IO successfully"); assert!(!vcpu.ready_for_interrupt()); // We don't care whether we inject the interrupt successfully or not. @@ -3959,7 +3936,6 @@ fn test_ready_for_interrupt_for_intercepted_instructions() { vcpu.handle_io(&mut |params| { io_port = params.address; // We are always handling out IO port, so no data to return. - None }) .expect("should handle port IO successfully"); match io_port { diff --git a/hypervisor/tests/mmio_and_pio.rs b/hypervisor/tests/mmio_and_pio.rs index 2e0f5489da..1956d251cb 100644 --- a/hypervisor/tests/mmio_and_pio.rs +++ b/hypervisor/tests/mmio_and_pio.rs @@ -111,54 +111,42 @@ where loop { match vcpu.run().expect("run failed") { VcpuExit::Mmio => { - vcpu.handle_mmio(&mut |IoParams { - address, - size, - operation, - }| { + vcpu.handle_mmio(&mut |IoParams { address, operation }| { match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; + IoOperation::Read(data) => { assert_eq!(address, 0x3010); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); exits.fetch_add(1, Ordering::SeqCst); // this number will be read into al register - data.copy_from_slice(&0x66_u64.to_ne_bytes()); - Ok(Some(data)) + data.copy_from_slice(&[0x66]); + Ok(()) } - IoOperation::Write { data } => { + IoOperation::Write(data) => { assert_eq!(address, 0x3000); assert_eq!(data[0], 0x33); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); exits.fetch_add(1, Ordering::SeqCst); - Ok(None) + Ok(()) } } }) .expect("failed to set the data"); } VcpuExit::Io => { - vcpu.handle_io(&mut |IoParams { - address, - size, - operation, - }| { + vcpu.handle_io(&mut |IoParams { address, operation }| { match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; + IoOperation::Read(data) => { assert_eq!(address, 0x20); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); exits.fetch_add(1, Ordering::SeqCst); // this number will be read into the al register - data.copy_from_slice(&0x77_u64.to_ne_bytes()); - Some(data) + data.copy_from_slice(&[0x77]); } - IoOperation::Write { data } => { + IoOperation::Write(data) => { assert_eq!(address, 0x19); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); assert_eq!(data[0], 0x66); exits.fetch_add(1, Ordering::SeqCst); - None } } }) @@ -282,27 +270,20 @@ where loop { match vcpu.run().expect("run failed") { VcpuExit::Io => { - vcpu.handle_io(&mut |IoParams { - address, - size, - operation, - }| { - match operation { - IoOperation::Read => panic!("unexpected PIO read"), - IoOperation::Write { data } => { - assert!((1..=4).contains(&address)); - if address % 2 == 0 { - assert_eq!(size, 1); - assert_eq!(data[0], address as u8); - } else { - assert_eq!(size, 2); - assert_eq!(data[0], address as u8); - assert_eq!(data[1], 0); - } - exit_bits.fetch_or(1 << (address - 1), Ordering::SeqCst); - exit_count.fetch_add(1, Ordering::SeqCst); - None + vcpu.handle_io(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(_) => panic!("unexpected PIO read"), + IoOperation::Write(data) => { + assert!((1..=4).contains(&address)); + if address % 2 == 0 { + assert_eq!(data.len(), 1); + assert_eq!(data[0], address as u8); + } else { + assert_eq!(data.len(), 2); + assert_eq!(data[0], address as u8); + assert_eq!(data[1], 0); } + exit_bits.fetch_or(1 << (address - 1), Ordering::SeqCst); + exit_count.fetch_add(1, Ordering::SeqCst); } }) .expect("failed to set the data"); @@ -427,31 +408,23 @@ where loop { match vcpu.run().expect("run failed") { VcpuExit::Io => { - vcpu.handle_io(&mut |IoParams { - address, - size, - operation, - }| { - match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; - assert!((1..=4).contains(&address)); + vcpu.handle_io(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(data) => { + assert!((1..=4).contains(&address)); - if address % 2 == 0 { - assert_eq!(size, 1); - data[0] = address as u8; - } else { - assert_eq!(size, 2); - data[0] = address as u8; - data[1] = address as u8; - } - - exit_bits.fetch_or(1 << (address - 1), Ordering::SeqCst); - exit_count.fetch_add(1, Ordering::SeqCst); - Some(data) + if address % 2 == 0 { + assert_eq!(data.len(), 1); + data[0] = address as u8; + } else { + assert_eq!(data.len(), 2); + data[0] = address as u8; + data[1] = address as u8; } - IoOperation::Write { .. } => panic!("unexpected PIO write"), + + exit_bits.fetch_or(1 << (address - 1), Ordering::SeqCst); + exit_count.fetch_add(1, Ordering::SeqCst); } + IoOperation::Write(_) => panic!("unexpected PIO write"), }) .expect("failed to set the data"); } diff --git a/hypervisor/tests/mmio_fetch_memory.rs b/hypervisor/tests/mmio_fetch_memory.rs index 162c26841c..4da3e7ed9d 100644 --- a/hypervisor/tests/mmio_fetch_memory.rs +++ b/hypervisor/tests/mmio_fetch_memory.rs @@ -80,15 +80,11 @@ fn test_whpx_mmio_fetch_memory() { match vcpu.run().expect("run failed") { VcpuExit::Mmio => { exits.fetch_add(1, Ordering::SeqCst); - vcpu.handle_mmio(&mut |IoParams { - address, - size, - operation, - }| { + vcpu.handle_mmio(&mut |IoParams { address, operation }| { match operation { - IoOperation::Read => { + IoOperation::Read(data) => { memory_reads.fetch_add(1, Ordering::SeqCst); - match (address, size) { + match (address, data.len()) { // First MMIO read from the WHV_EMULATOR asks to // load the first 8 bytes of a new execution // page, when an instruction crosses page @@ -99,22 +95,28 @@ fn test_whpx_mmio_fetch_memory() { // Ensure this instruction is the first read // in the sequence. assert_eq!(memory_reads.load(Ordering::SeqCst), 1); - Ok(Some([0x88, 0x03, 0x67, 0x8a, 0x01, 0xf4, 0, 0])) + data.copy_from_slice(&[ + 0x88, 0x03, 0x67, 0x8a, 0x01, 0xf4, 0, 0, + ]); + Ok(()) } // Second MMIO read is a regular read from an // unmapped memory. - (0x3010, 1) => Ok(Some([0x66, 0, 0, 0, 0, 0, 0, 0])), + (0x3010, 1) => { + data.copy_from_slice(&[0x66]); + Ok(()) + } _ => { - panic!("invalid address({:#x})/size({})", address, size) + panic!("invalid address({:#x})/size({})", address, data.len()) } } } - IoOperation::Write { data } => { + IoOperation::Write(data) => { assert_eq!(address, 0x3000); assert_eq!(data[0], 0x33); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); memory_writes.fetch_add(1, Ordering::SeqCst); - Ok(None) + Ok(()) } } }) diff --git a/hypervisor/tests/read_only_memory.rs b/hypervisor/tests/read_only_memory.rs index 187ba176a1..d44d9be52c 100644 --- a/hypervisor/tests/read_only_memory.rs +++ b/hypervisor/tests/read_only_memory.rs @@ -163,20 +163,16 @@ where VcpuExit::Intr => continue, VcpuExit::Hlt => break, VcpuExit::Mmio => { - vcpu.handle_mmio(&mut |IoParams { - address, - size, - operation, - }| match operation { - IoOperation::Read => { + vcpu.handle_mmio(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(_) => { panic!("unexpected mmio read call"); } - IoOperation::Write { data } => { - assert_eq!(size, 1); + IoOperation::Write(data) => { + assert_eq!(data.len(), 1); assert_eq!(address, vcpu_sregs.es.base); assert_eq!(data[0], 0x67); exits.fetch_add(1, Ordering::SeqCst); - Ok(None) + Ok(()) } }) .expect("failed to set the data"); diff --git a/hypervisor/tests/real_run_addr.rs b/hypervisor/tests/real_run_addr.rs index b84fdf793f..79080f6300 100644 --- a/hypervisor/tests/real_run_addr.rs +++ b/hypervisor/tests/real_run_addr.rs @@ -108,19 +108,14 @@ where loop { match vcpu.run().expect("run failed") { VcpuExit::Io => { - vcpu.handle_io(&mut |IoParams { - address, - size, - operation, - }| match operation { - IoOperation::Read => { + vcpu.handle_io(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(_) => { panic!("unexpected io in call"); } - IoOperation::Write { data } => { + IoOperation::Write(data) => { assert_eq!(address, 0x3f8); - assert_eq!(size, 1); + assert_eq!(data.len(), 1); out.lock().push(data[0] as char); - None } }) .expect("failed to set the data"); diff --git a/hypervisor/tests/remove_memory.rs b/hypervisor/tests/remove_memory.rs index 7384d671e2..5854ee8abe 100644 --- a/hypervisor/tests/remove_memory.rs +++ b/hypervisor/tests/remove_memory.rs @@ -168,19 +168,14 @@ where VcpuExit::Intr => continue, VcpuExit::Hlt => break, VcpuExit::Mmio => { - vcpu.handle_mmio(&mut |IoParams { - address, - size, - operation, - }| match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; + vcpu.handle_mmio(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(data) => { assert_eq!(address, 0x3000); - assert_eq!(size, 1); - data.copy_from_slice(&0x44_u64.to_ne_bytes()); - Ok(Some(data)) + assert_eq!(data.len(), 1); + data.copy_from_slice(&[0x44]); + Ok(()) } - IoOperation::Write { .. } => { + IoOperation::Write(_) => { panic!("unexpected mmio write"); } }) diff --git a/src/crosvm/sys/linux/vcpu.rs b/src/crosvm/sys/linux/vcpu.rs index 80eff68381..29b452f029 100644 --- a/src/crosvm/sys/linux/vcpu.rs +++ b/src/crosvm/sys/linux/vcpu.rs @@ -64,35 +64,6 @@ const SCHED_FLAG_UTIL_CLAMP_MIN: u64 = 0x20; const SCHED_SCALE_CAPACITY: u32 = 1024; const SCHED_FLAG_KEEP_ALL: u64 = SCHED_FLAG_KEEP_POLICY | SCHED_FLAG_KEEP_PARAMS; -fn bus_io_handler(bus: &Bus) -> impl FnMut(IoParams) -> Option<[u8; 8]> + '_ { - |IoParams { - address, - mut size, - operation: direction, - }| match direction { - IoOperation::Read => { - let mut data = [0u8; 8]; - if size > data.len() { - error!("unsupported Read size of {} bytes", size); - size = data.len(); - } - // Ignore the return value of `read()`. If no device exists on the bus at the given - // location, return the initial value of data, which is all zeroes. - let _ = bus.read(address, &mut data[..size]); - Some(data) - } - IoOperation::Write { data } => { - if size > data.len() { - error!("unsupported Write size of {} bytes", size); - size = data.len() - } - let data = &data[..size]; - bus.write(address, data); - None - } - } -} - /// Set the VCPU thread affinity and other per-thread scheduler properties. /// This function will be called from each VCPU thread at startup. #[allow(clippy::unnecessary_cast)] @@ -399,13 +370,31 @@ where if !interrupted_by_signal { match vcpu.run() { Ok(VcpuExit::Io) => { - if let Err(e) = vcpu.handle_io(&mut bus_io_handler(&io_bus)) { + if let Err(e) = + vcpu.handle_io(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(data) => { + io_bus.read(address, data); + } + IoOperation::Write(data) => { + io_bus.write(address, data); + } + }) + { error!("failed to handle io: {}", e) } } Ok(VcpuExit::Mmio) => { if let Err(e) = - vcpu.handle_mmio(&mut |io_params| Ok(bus_io_handler(&mmio_bus)(io_params))) + vcpu.handle_mmio(&mut |IoParams { address, operation }| match operation { + IoOperation::Read(data) => { + mmio_bus.read(address, data); + Ok(()) + } + IoOperation::Write(data) => { + mmio_bus.write(address, data); + Ok(()) + } + }) { error!("failed to handle mmio: {}", e); } diff --git a/src/sys/windows/run_vcpu.rs b/src/sys/windows/run_vcpu.rs index c8f1ace70a..13b91c3c8d 100644 --- a/src/sys/windows/run_vcpu.rs +++ b/src/sys/windows/run_vcpu.rs @@ -732,71 +732,47 @@ where match exit { Ok(VcpuExit::Io) => { let _trace_event = trace_event!(crosvm, "VcpuExit::Io"); - vcpu.handle_io(&mut |IoParams { address, mut size, operation}| { + vcpu.handle_io(&mut |IoParams { address, operation}| { match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; - if size > data.len() { - error!("unsupported IoIn size of {} bytes", size); - size = data.len(); - } - io_bus.read(address, &mut data[..size]); - Some(data) + IoOperation::Read(data) => { + io_bus.read(address, data); } - IoOperation::Write { data } => { - if size > data.len() { - error!("unsupported IoOut size of {} bytes", size); - size = data.len() - } - vm.handle_io_events(IoEventAddress::Pio(address), &data[..size]) + IoOperation::Write(data) => { + vm.handle_io_events(IoEventAddress::Pio(address), data) .unwrap_or_else(|e| error!( "failed to handle ioevent for pio write to {} on vcpu {}: {}", address, context.cpu_id, e )); - io_bus.write(address, &data[..size]); - None + io_bus.write(address, data); } } }).unwrap_or_else(|e| error!("failed to handle io: {}", e)); } Ok(VcpuExit::Mmio) => { let _trace_event = trace_event!(crosvm, "VcpuExit::Mmio"); - vcpu.handle_mmio(&mut |IoParams { address, mut size, operation }| { + vcpu.handle_mmio(&mut |IoParams { address, operation }| { match operation { - IoOperation::Read => { - let mut data = [0u8; 8]; - if size > data.len() { - error!("unsupported MmioRead size of {} bytes", size); - size = data.len(); - } - { - let data = &mut data[..size]; - if !mmio_bus.read(address, data) { - info!( - "mmio read failed: {:x}; trying memory read..", - address - ); - vm.get_memory() - .read_exact_at_addr( - data, - vm_memory::GuestAddress(address), + IoOperation::Read(data) => { + if !mmio_bus.read(address, data) { + info!( + "mmio read failed: {:x}; trying memory read..", + address + ); + vm.get_memory() + .read_exact_at_addr( + data, + vm_memory::GuestAddress(address), + ) + .unwrap_or_else(|e| { + error!( + "guest memory read failed at {:x}: {}", + address, e ) - .unwrap_or_else(|e| { - error!( - "guest memory read failed at {:x}: {}", - address, e - ) - }); - } + }); } - Ok(Some(data)) + Ok(()) } - IoOperation::Write { data } => { - if size > data.len() { - error!("unsupported MmioWrite size of {} bytes", size); - size = data.len() - } - let data = &data[..size]; + IoOperation::Write(data) => { vm.handle_io_events(IoEventAddress::Mmio(address), data) .unwrap_or_else(|e| error!( "failed to handle ioevent for mmio write to {} on vcpu {}: {}", @@ -814,7 +790,7 @@ where address, e )); } - Ok(None) + Ok(()) } } }).unwrap_or_else(|e| error!("failed to handle mmio: {}", e));