virtio: update core queue to have separate work and completion types #2686
Conversation
…ents Move split queue logic into specific split queue implementations.
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
This PR refactors the virtio queue implementation to separate work fetching (reading from available ring) from work completion (writing to used ring), preparing the codebase for future packed queue support. The refactoring isolates split queue-specific logic and introduces cleaner abstractions.
Changes:
- Split
QueueCoreintoQueueCoreGetWorkandQueueCoreCompleteWorkto separate read and write operations - Introduced
QueueWork,QueueCompletionContext, andQueueDescriptortypes to encapsulate queue work and completion state - Renamed
DescriptortoSplitDescriptorto distinguish split queue format from future packed queue format - Added power-of-2 validation for queue sizes and added
VIRTIO_F_VERSION_1feature flag support - Updated tests to use valid queue sizes (changed from 5 to 8 to satisfy power-of-2 requirement)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| vm/devices/virtio/virtio/src/queue.rs | Core refactoring: split queue operations into get/complete, added new types, isolated split queue logic |
| vm/devices/virtio/virtio/src/common.rs | Updated to use new split queue APIs, modified work completion flow |
| vm/devices/virtio/virtio/src/spec.rs | Renamed Descriptor to SplitDescriptor for clarity |
| vm/devices/virtio/virtio/src/tests.rs | Added indirect descriptor feature flag, fixed queue sizes to be power-of-2 |
| pub struct QueueDescriptor { | ||
| address: u64, | ||
| length: u32, | ||
| flags: DescriptorFlags, | ||
| next: Option<u16>, | ||
| } |
There was a problem hiding this comment.
QueueDescriptor is marked as pub but has all private fields and no public methods, making it an opaque type from outside the module. Since it's only used internally and converted to VirtioQueuePayload for external use, it should be marked pub(crate) instead of pub to clarify that it's an internal type not intended for external use.
There was a problem hiding this comment.
clippy disagrees
| Ok((get_work, complete_work)) | ||
| } | ||
|
|
||
| pub struct SplitQueueCompletionContext { |
There was a problem hiding this comment.
SplitQueueCompletionContext is marked as pub but is only used internally within the queue module (accessed via pattern matching in QueueCoreCompleteWork::complete_descriptor). It should be marked pub(crate) to indicate that it's an internal implementation detail not intended for external use.
| pub struct SplitQueueCompletionContext { | |
| pub(crate) struct SplitQueueCompletionContext { |
There was a problem hiding this comment.
clippy disagrees
other minor pr feedback from copilot
|
The unsafe blocks are in the tests.rs module so not part of production. |
1 participant
In preparation for adding the packed queue type, update how work is fetched and completed from a queue. Additionally, isolate split queue specific logic.