From df8cad514e0005a12c2776b941d7edfb3af96372 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Thu, 15 Aug 2024 15:31:21 +0000 Subject: [PATCH 1/2] chore(bindgen): parse ring features from virtio_ring.h VIRTIO_RING_F_INDIRECT_DESC is declared in the virtio_ring.h, so modify bindgen to parse all `VIRTIO_RING_` constants. Signed-off-by: Egor Lazarchuk --- src/vmm/src/devices/virtio/gen/virtio_ring.rs | 1 + tools/bindgen.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vmm/src/devices/virtio/gen/virtio_ring.rs b/src/vmm/src/devices/virtio/gen/virtio_ring.rs index 9d228ea9ee8..0df7b8f7e80 100644 --- a/src/vmm/src/devices/virtio/gen/virtio_ring.rs +++ b/src/vmm/src/devices/virtio/gen/virtio_ring.rs @@ -14,4 +14,5 @@ clippy::tests_outside_test_module )] +pub const VIRTIO_RING_F_INDIRECT_DESC: u32 = 28; pub const VIRTIO_RING_F_EVENT_IDX: u32 = 29; diff --git a/tools/bindgen.sh b/tools/bindgen.sh index e2698b81daf..90a278194b7 100755 --- a/tools/bindgen.sh +++ b/tools/bindgen.sh @@ -67,7 +67,7 @@ fc-bindgen \ info "BINDGEN virtio_ring.h" fc-bindgen \ - --allowlist-var "VIRTIO_RING_F_EVENT_IDX" \ + --allowlist-var "VIRTIO_RING_.*" \ "$KERNEL_HEADERS_HOME/include/linux/virtio_ring.h" >src/vmm/src/devices/virtio/gen/virtio_ring.rs info "BINDGEN virtio_blk.h" From f1d07a2850a7e360e0e193aa2e1fa8d58ce85733 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Fri, 16 Aug 2024 17:28:26 +0100 Subject: [PATCH 2/2] feat(iovec): add support for VIRTQ_DESC_F_INDIRECT to IoVecBuffer Now IoVecBuffer/Mut can be built from descriptor chains utilizing VIRTQ_DESC_F_INDIRECT flag. The way indirect descriptors work is: - Descriptors from descriptor table instead of pointing to the buffers where data needs to be written to/read from now can point to buffers that contain other descriptor table. That 'indirect' descriptor table contains descriptor which point to actual buffers for data. - All descriptor in the 'indirect' descriptor table are processed sequentially. - The `VIRTQ_DESC_F_WRITE` flag is ignored for the main descriptor (the one from original descriptor table) Signed-off-by: Egor Lazarchuk --- src/vmm/src/devices/virtio/iovec.rs | 86 ++++++++++++++++++------ src/vmm/src/devices/virtio/net/device.rs | 9 ++- src/vmm/src/devices/virtio/queue.rs | 26 +++++++ src/vmm/src/devices/virtio/vsock/mod.rs | 3 + 4 files changed, 101 insertions(+), 23 deletions(-) diff --git a/src/vmm/src/devices/virtio/iovec.rs b/src/vmm/src/devices/virtio/iovec.rs index 064660a4b54..399127e6237 100644 --- a/src/vmm/src/devices/virtio/iovec.rs +++ b/src/vmm/src/devices/virtio/iovec.rs @@ -7,10 +7,12 @@ use libc::{c_void, iovec, size_t}; use serde::{Deserialize, Serialize}; use vm_memory::bitmap::Bitmap; use vm_memory::{ - GuestMemory, GuestMemoryError, ReadVolatile, VolatileMemoryError, VolatileSlice, WriteVolatile, + GuestAddress, GuestMemory, GuestMemoryError, ReadVolatile, VolatileMemoryError, VolatileSlice, + WriteVolatile, }; use super::iov_deque::{IovDeque, IovDequeError}; +use super::queue::Descriptor; use crate::devices::virtio::queue::DescriptorChain; use crate::vstate::memory::GuestMemoryMmap; @@ -24,6 +26,8 @@ pub enum IoVecError { OverflowedDescriptor, /// Tried to push to full IovDeque. IovDequeOverflow, + /// Nested indirect descriptor + NestedIndirectDescriptor, /// Guest memory error: {0} GuestMemory(#[from] GuestMemoryError), /// Error with underlying `IovDeque`: {0} @@ -62,26 +66,43 @@ impl IoVecBuffer { let mut next_descriptor = Some(head); while let Some(desc) = next_descriptor { - if desc.is_write_only() { - return Err(IoVecError::WriteOnlyDescriptor); - } + if desc.is_indirect() { + // We use get_slice instead of `get_host_address` here in order to have the whole + // range of the descriptor chain checked, i.e. [addr, addr + len) is a valid memory + // region in the GuestMemoryMmap. + let indirect_desc_slice = mem + .get_slice(desc.addr, desc.len as usize) + .map_err(IoVecError::GuestMemory)?; + + // SAFETY: + // We checked the slice above. We just transform it into + // a slice of Descriptors. + let indirect_desc_slice: &[Descriptor] = unsafe { + std::slice::from_raw_parts( + indirect_desc_slice.ptr_guard().as_ptr().cast(), + desc.len as usize / std::mem::size_of::(), + ) + }; - // We use get_slice instead of `get_host_address` here in order to have the whole - // range of the descriptor chain checked, i.e. [addr, addr + len) is a valid memory - // region in the GuestMemoryMmap. - let iov_base = mem - .get_slice(desc.addr, desc.len as usize)? - .ptr_guard_mut() - .as_ptr() - .cast::(); - self.vecs.push(iovec { - iov_base, - iov_len: desc.len as size_t, - }); - self.len = self - .len - .checked_add(desc.len) - .ok_or(IoVecError::OverflowedDescriptor)?; + for d in indirect_desc_slice.iter() { + if desc.is_write_only() { + return Err(IoVecError::WriteOnlyDescriptor); + } + if d.is_indirect() { + return Err(IoVecError::NestedIndirectDescriptor); + } + self.add_descriptor(mem, GuestAddress(d.addr), d.len)?; + if !d.has_next() { + break; + } + } + } else { + if desc.is_write_only() { + return Err(IoVecError::WriteOnlyDescriptor); + } + + self.add_descriptor(mem, desc.addr, desc.len)?; + } next_descriptor = desc.next_descriptor(); } @@ -89,6 +110,31 @@ impl IoVecBuffer { Ok(()) } + fn add_descriptor( + &mut self, + mem: &GuestMemoryMmap, + addr: GuestAddress, + len: u32, + ) -> Result<(), IoVecError> { + // We use get_slice instead of `get_host_address` here in order to have the whole + // range of the descriptor chain checked, i.e. [addr, addr + len) is a valid memory + // region in the GuestMemoryMmap. + let iov_base = mem + .get_slice(addr, len as usize)? + .ptr_guard_mut() + .as_ptr() + .cast::(); + self.vecs.push(iovec { + iov_base, + iov_len: len as size_t, + }); + self.len = self + .len + .checked_add(len) + .ok_or(IoVecError::OverflowedDescriptor)?; + Ok(()) + } + /// Create an `IoVecBuffer` from a `DescriptorChain` /// /// # Safety diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index deb6976b2af..c35d7cb91e9 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -21,7 +21,9 @@ use crate::devices::virtio::gen::virtio_net::{ VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_MAC, VIRTIO_NET_F_MRG_RXBUF, }; -use crate::devices::virtio::gen::virtio_ring::VIRTIO_RING_F_EVENT_IDX; +use crate::devices::virtio::gen::virtio_ring::{ + VIRTIO_RING_F_EVENT_IDX, VIRTIO_RING_F_INDIRECT_DESC, +}; use crate::devices::virtio::iovec::{ IoVecBuffer, IoVecBufferMut, IoVecError, ParsedDescriptorChain, }; @@ -281,9 +283,10 @@ impl Net { | 1 << VIRTIO_NET_F_HOST_TSO4 | 1 << VIRTIO_NET_F_HOST_TSO6 | 1 << VIRTIO_NET_F_HOST_UFO - | 1 << VIRTIO_F_VERSION_1 | 1 << VIRTIO_NET_F_MRG_RXBUF - | 1 << VIRTIO_RING_F_EVENT_IDX; + | 1 << VIRTIO_RING_F_INDIRECT_DESC + | 1 << VIRTIO_RING_F_EVENT_IDX + | 1 << VIRTIO_F_VERSION_1; let mut config_space = ConfigSpace::default(); if let Some(mac) = guest_mac { diff --git a/src/vmm/src/devices/virtio/queue.rs b/src/vmm/src/devices/virtio/queue.rs index b80b2571c12..8488c3e9951 100644 --- a/src/vmm/src/devices/virtio/queue.rs +++ b/src/vmm/src/devices/virtio/queue.rs @@ -14,6 +14,7 @@ use crate::vstate::memory::{Address, Bitmap, ByteValued, GuestAddress, GuestMemo pub const VIRTQ_DESC_F_NEXT: u16 = 0x1; pub const VIRTQ_DESC_F_WRITE: u16 = 0x2; +pub const VIRTQ_DESC_F_INDIRECT: u16 = 0x4; /// Max size of virtio queues offered by firecracker's virtio devices. pub(super) const FIRECRACKER_MAX_QUEUE_SIZE: u16 = 256; @@ -49,6 +50,26 @@ pub struct Descriptor { pub next: u16, } +impl Descriptor { + /// Gets if this descriptor chain has another descriptor chain linked after it. + pub fn has_next(&self) -> bool { + self.flags & VIRTQ_DESC_F_NEXT != 0 + } + + /// If the driver designated this as a write only descriptor. + /// + /// If this is false, this descriptor is read only. + /// Write only means the emulated device can write and the driver can read. + pub fn is_write_only(&self) -> bool { + self.flags & VIRTQ_DESC_F_WRITE != 0 + } + + /// If the driver designated this as a indirect descriptor. + pub fn is_indirect(&self) -> bool { + self.flags & VIRTQ_DESC_F_INDIRECT != 0 + } +} + // SAFETY: `Descriptor` is a POD and contains no padding. unsafe impl ByteValued for Descriptor {} @@ -138,6 +159,11 @@ impl DescriptorChain { self.flags & VIRTQ_DESC_F_WRITE != 0 } + /// If the driver designated this as a indirect descriptor. + pub fn is_indirect(&self) -> bool { + self.flags & VIRTQ_DESC_F_INDIRECT != 0 + } + /// Gets the next descriptor in this descriptor chain, if there is one. /// /// Note that this is distinct from the next descriptor chain returned by `AvailIter`, which is diff --git a/src/vmm/src/devices/virtio/vsock/mod.rs b/src/vmm/src/devices/virtio/vsock/mod.rs index 8ffaaa8db0d..6258e719ec8 100644 --- a/src/vmm/src/devices/virtio/vsock/mod.rs +++ b/src/vmm/src/devices/virtio/vsock/mod.rs @@ -125,6 +125,8 @@ pub enum VsockError { DescChainTooShortForHeader(usize), /// The descriptor chain length was greater than the max ([u32::MAX]) DescChainOverflow, + /// Nested indirect descriptor + NestedIndirectDescriptor, /// The vsock header `len` field holds an invalid value: {0} InvalidPktLen(u32), /// A data fetch was attempted when no data was available. @@ -154,6 +156,7 @@ impl From for VsockError { IoVecError::OverflowedDescriptor => VsockError::DescChainOverflow, IoVecError::IovDeque(err) => VsockError::IovDeque(err), IoVecError::IovDequeOverflow => VsockError::IovDequeOverflow, + IoVecError::NestedIndirectDescriptor => VsockError::NestedIndirectDescriptor, } } }