feat(virtq): add packed virtio ring primitives#1382
feat(virtq): add packed virtio ring primitives#1382andreiltd wants to merge 7 commits intohyperlight-dev:mainfrom
Conversation
andreiltd
requested review from
danbugs,
dblnz,
devigned,
jprendes,
ludfjig,
marosset,
simongdavies and
syntactically
as code owners
andreiltd
force-pushed
the
tandr/virtq-1-ring
branch
from
3db3820 to
bd2fd96
Compare
andreiltd
added
the
kind/enhancement
andreiltd
force-pushed
the
tandr/virtq-1-ring
branch
2 times, most recently
from
b4c8142 to
16de50c
Compare
dblnz
left a comment
dblnz
left a comment
There was a problem hiding this comment.
Great stuff, @andreiltd !
I left a few comments, most are small remarks, others are things I might have missed.
This is part 1 of my review, I still have some things left to look at that I plan on doing tomorrow.
| /// and readable buffers must be added before writable buffers. | ||
| #[derive(Debug, Default)] | ||
| pub struct BufferChainBuilder<T> { | ||
| elems: SmallVec<[BufferElement; 16]>, |
There was a problem hiding this comment.
Maybe documenting this 16 inline capacity as an optimization for the user to be aware
andreiltd
force-pushed
the
tandr/virtq-1-ring
branch
4 times, most recently
from
857f4bf to
2c6adcb
Compare
andreiltd
force-pushed
the
tandr/virtq-1-ring
branch
3 times, most recently
from
79ee809 to
141fc6b
Compare
| /// | ||
| /// - [`RingError::WouldBlock`] - No free descriptor slots | ||
| /// - [`RingError::OutOfMemory`] - No free descriptor IDs (internal error) | ||
| /// - [`RingError::InvalidState`] - ID tracking corrupted (internal error) |
There was a problem hiding this comment.
This API can also return RingError::MemError
Add low-level packed virtqueue ring implementation in hyperlight_common::virtq, based on the virtio packed ring format. Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
andreiltd
force-pushed
the
tandr/virtq-1-ring
branch
from
141fc6b to
e27dacb
Compare
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
ludfjig
left a comment
ludfjig
left a comment
There was a problem hiding this comment.
This PR is very clean, big fan!
I know we don't really have policy on asserts vs errors etc in hyperlight (I personally like asserts...), but ring.rs has 3 debug_assert! that could be useful in release mode as well, and they're already in methods that return Results. Do you think it makes sense to convert them to errros instead?
| /// Add multiple readable buffers from an iterator. | ||
| pub fn readables( | ||
| mut self, | ||
| elements: impl IntoIterator<Item = impl Into<BufferElement>>, | ||
| ) -> Self { | ||
| for elem in elements { | ||
| self.elems.push(elem.into()); | ||
| self.split += 1; | ||
| } | ||
|
|
||
| self | ||
| } |
There was a problem hiding this comment.
should we set elem.writeable = false here in case a caller passes writeable? Or error out?
| elements: impl IntoIterator<Item = impl Into<BufferElement>>, | ||
| ) -> BufferChainBuilder<Writable> { | ||
| for elem in elements { | ||
| self.elems.push(elem.into()); |
There was a problem hiding this comment.
same here but vice versa, shouldwe set writable = true?
| /// | ||
| /// `ids` contains the descriptor IDs that are in-flight. | ||
| /// Sets cursors, counters, and `id_num` accordingly. The chain lengths are all set to 1. | ||
| pub fn reset_prefilled(&mut self, ids: &[u16]) { |
There was a problem hiding this comment.
if a ring has size 8 and we call reset_prefilled(&[4,5,6,7]), and call producer.submit_one(addr, len, false), it will fail because self.id_free is empty, I'm not sure this is correct?
| } | ||
|
|
||
| // Record chain length for later used submission | ||
| self.id_num[id as usize] = chain_len; |
There was a problem hiding this comment.
this is overwritten unconditionally, should we check that this id is not 0 like the producer already does:
// We should never reuse an ID that is still outstanding
if self.id_num[id as usize] != 0 {
return Err(RingError::InvalidState);
}| #[inline] | ||
| const fn align_up(val: usize, align: usize) -> usize { | ||
| (val + align - 1) & !(align - 1) | ||
| } |
There was a problem hiding this comment.
nit: maybe next_multiple_of() can be used instead?
| desc.write_release(&self.mem, addr) | ||
| .map_err(|_| RingError::MemError)?; |
There was a problem hiding this comment.
nit: lot's of map_err lose their context when going to RingError::MemError, maybe can parametrize RingError<E> on generic E which is some M::Error? Not sure, feel free to ignore
| const NEXT = 1 << 0; | ||
| /// This marks a buffer as device write-only (otherwise device read-only). | ||
| const WRITE = 1 << 1; | ||
| /// This means the buffer contains a list of buffer descriptors (unsupported here). |
There was a problem hiding this comment.
nit: should we add runtimes check for this unsupported?
| let head_idx = self.avail_cursor.head(); | ||
| let head_wrap = self.avail_cursor.wrap(); | ||
|
|
||
| let id = self.id_free.pop().ok_or(RingError::InvalidState)?; |
There was a problem hiding this comment.
nit: this uses RingError::InvalidState but submit_one uses another error for the same error condition
| self.elems.as_slice() | ||
| } | ||
|
|
||
| /// Get writable buffers in chain |
| // addr is unused for used descriptor according to packed-virtqueue spec | ||
| let mut used_desc = Descriptor::new(0, 0, id, DescFlags::empty()); | ||
| used_desc.len = written_len; |
There was a problem hiding this comment.
why not pass written_len to new(..) directly?
4 participants
Add low-level packed virtqueue ring implementation in
hyperlight_common::virtq, based on the virtio packed ring format.This is split from: #1368 and does not include any actual plumbing for guest/host communication.
Useful materials: