feat(virtq): add packed virtio ring primitives#1382
feat(virtq): add packed virtio ring primitives#1382andreiltd wants to merge 8 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.
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
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?
| 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
There was a problem hiding this comment.
I tried this initially yes, and make it generic creates much more API churn especially for the layers on top of virtq. But let me think a bit more how we could preserve context without making ring error generic.
| 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?
There was a problem hiding this comment.
We could, but I think it's perfectly safe if we just ignore this bit (treat as reserved) and document that current implementation does not follow direct tables. The reason I'm skeptical to add runtime checks is that buffer chains are processed in the loop so we will need the check inside the loop for no real benefits.
simongdavies
left a comment
simongdavies
left a comment
There was a problem hiding this comment.
Great stuff a few minor comments but looks good to me
| avail == wrap && used == wrap | ||
| } | ||
|
|
||
| /// Is this descriptor writable by the device? |
There was a problem hiding this comment.
The comments here use the terms "guest" "host" and "device" and "driver" I read the HIP again at https://github.com/andreiltd/hyperlight/blob/tandr/rng-rfc/proposals/0001-rng-buf/README.md#design-details
but still wasn't sure how to interpret these terms here? The HIP suggests that the driver is always the guest and the device is always the host but then goes on to say that for bi-directional communication these roles are reversed , I think it might help us in the future to define explicitly what we are referring to in the code comments.
There was a problem hiding this comment.
Yes, HIP needs to be updated. Initial version of HIP assumed the roles are swapped depending on direction -- but that idea has been later discarded following allocation discussion in the heap. The most recent agreement is: driver - always a guest, device - always host.
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
andreiltd
force-pushed
the
tandr/virtq-1-ring
branch
from
7514b84 to
c384e6f
Compare
5 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: