feat(virtq): add packed virtio ring primitives#1382
Conversation
3db3820 to
bd2fd96
Compare
b4c8142 to
16de50c
Compare
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.
857f4bf to
2c6adcb
Compare
79ee809 to
141fc6b
Compare
141fc6b to
e27dacb
Compare
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?
simongdavies
left a comment
There was a problem hiding this comment.
Great stuff a few minor comments but looks good to me
c384e6f to
6541fea
Compare
6541fea to
519472d
Compare
ludfjig
left a comment
There was a problem hiding this comment.
LGTM, not sure if you saw this comment but not blocking
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?
danbugs
left a comment
There was a problem hiding this comment.
Only minor NITs. Really great work, @andreiltd !
657da05 to
032ac0b
Compare
Hey @ludfjig, sorry - there were few back and forth with usage of asserts so I thought I already addressed that - we had CI checks that banned usage of assert macro at some point but that has been now relaxed. I converted the asserts in question into invalid state errors - I think it makes sense for dealing with malicious driver. Thanks for pointing that out! |
dblnz
left a comment
There was a problem hiding this comment.
Great work! I have nothing else to add.
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>
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>
032ac0b to
130bb27
Compare
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: