Add packed virtqueue support #301

prakash-shekhar · 2025-06-13T00:59:13Z

Summary of the PR

Adds support for packed virtqueues, implementing the VirtIO packed virtqueue format alongside the existing split virtqueue support. Packed virtqueues offer improved performance through better cache locality and reduced memory overhead compared to traditional split virtqueues.

Changes:

VringConfigData structure (src/backend.rs):
- Updated documentation to clearly distinguish between split and packed virtqueue address semantics
- For packed virtqueues: desc_table_addr becomes the packed descriptor ring, used_ring_addr becomes driver event suppression, avail_ring_addr becomes device event suppression
vhost-user protocol support (src/vhost_user/message.rs):
- Added RING_PACKED feature flag (bit 34, value 0x4_0000_0000)
- Added VHOST_VRING_F_PACKED vring flag (bit 1, value 0x2)
- Implemented packed virtqueue inflight I/O structures (DescStatePacked, QueueRegionPacked)
vhost-kern support (src/vhost_kern/mod.rs):
- Extended kernel backend to support packed virtqueue address configuration
- Added handling of packed layout in vring setup
Test coverage:
- Added test_packed_virtqueue_features() to validate feature flag definitions
- Added test_packed_vring_addr_validation() to test packed virtqueue address validation/alignment

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR have Signed-Off-By trailers (with
git commit -s), and the commit message has max 60 characters for the
summary and max 75 characters for each description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

stefano-garzarella

@prakash-shekhar please instead of merge main, can you rebase?

Also please write better commit messages with description other than title, explaining the why of changes.

stefano-garzarella · 2025-06-23T14:14:32Z

vhost/src/vhost_kern/mod.rs

+        #[cfg(feature = "vhost-user")]
+        let is_packed = config_data.flags & VhostUserVringAddrFlags::VHOST_VRING_F_PACKED.bits() != 0;
+        #[cfg(not(feature = "vhost-user"))]
+        let is_packed = false;


Maybe we should add a new function for this.

BTW, I'm confused, we are in VhostKernBackend why checking for vhost-user feature?

Yep you're right vhost-kern should work independently, I'll fix that.

stefano-garzarella · 2025-06-23T14:16:20Z

vhost/src/vhost_user/message.rs

@@ -386,6 +386,8 @@ bitflags! {
        const LOG_ALL = 0x400_0000;
        /// Feature flag for the protocol feature.
        const PROTOCOL_FEATURES = 0x4000_0000;
+        /// Feature flag for packed virtqueue support (VIRTIO_F_RING_PACKED, bit 34).
+        const RING_PACKED = 0x4_0000_0000;


Is this a virtio feature or a vhost-user feature?

stefano-garzarella · 2025-06-23T14:18:55Z

vhost/src/vhost_kern/mod.rs

@@ -79,7 +79,8 @@ pub trait VhostKernBackend: AsRawFd {

        // Check if packed ring format is being used
        #[cfg(feature = "vhost-user")]
-        let is_packed = config_data.flags & VhostUserVringAddrFlags::VHOST_VRING_F_PACKED.bits() != 0;
+        let is_packed =


Why this change?

Ah, please squash them where you add

Can you clarify what squash means, like squashing commits?

Yes, the unit of review is the commit, each commit should compile, pass all the test, etc. Don't add a new commit to fix a previous one in the same PR.

If you need to change a previous commit, edit, and amend

stefano-garzarella · 2025-06-23T14:19:53Z

vhost/CHANGELOG.md

@@ -3,6 +3,7 @@
 ## [Unreleased]

 ### Added
+- Add support for packed virtqueues in vhost-user and vhost-kern backends


Please follow the style, e.g. link to the pr, etc.

Understood, will fix.

germag · 2025-06-24T15:09:08Z

vhost-user-backend/Cargo.toml

 name = "vhost-user-backend"
-version = "0.18.0"
+version = "0.19.0"
 authors = ["The Cloud Hypervisor Authors"]


@stefano-garzarella is it ok to add a feature and create a release i the same PR?

nope, but I think this come up from merging main in this branch, indeed I asked to rebase it on main instead of merging, otherwise review it's a mess

germag · 2025-06-24T15:23:06Z

vhost/src/vhost_user/message.rs

+            if self.descriptor & 0xf != 0 {
+                return false;
+            }
+            // - driver/device event suppression areas must be 4-byte aligned
+            if self.available & 0x3 != 0 || self.used & 0x3 != 0 {
+                return false;
+            }
+        } else {
+            // Split ring validation (current logic)
+            if self.descriptor & 0xf != 0 {
+                return false;
+            } else if self.available & 0x1 != 0 {
+                return false;
+            } else if self.used & 0x3 != 0 {
+                return false;
+            }


this is almost duplicated code, the only difference is the available's alignment, could this be simplified?

germag · 2025-06-24T15:24:00Z

vhost/src/vhost_kern/mod.rs

+        if is_packed {
+            // Packed ring: single descriptor ring layout
+            let desc_ring_size = 16 * u64::from(queue_size) as GuestUsize;
+            let driver_event_size = 4; // 4 bytes for driver event suppression
+            let device_event_size = 4; // 4 bytes for device event suppression
+
+            // Validate packed descriptor ring
+            if GuestAddress(config_data.desc_table_addr)
+                .checked_add(desc_ring_size)
+                .is_none_or(|v| !m.address_in_range(v))
+            {
+                return false;
+            }
+            // Validate driver event suppression area (available)
+            if GuestAddress(config_data.avail_ring_addr)
+                .checked_add(driver_event_size)
+                .is_none_or(|v| !m.address_in_range(v))
+            {
+                return false;
+            }
+            // Validate device event suppression area (used)
+            if GuestAddress(config_data.used_ring_addr)
+                .checked_add(device_event_size)
+                .is_none_or(|v| !m.address_in_range(v))
+            {
+                return false;
+            }
+        } else {
+            // Split ring validation (existing logic)
+            let desc_table_size = 16 * u64::from(queue_size) as GuestUsize;
+            let avail_ring_size = 6 + 2 * u64::from(queue_size) as GuestUsize;
+            let used_ring_size = 6 + 8 * u64::from(queue_size) as GuestUsize;
+            if GuestAddress(config_data.desc_table_addr)
+                .checked_add(desc_table_size)
+                .is_none_or(|v| !m.address_in_range(v))
+            {
+                return false;
+            }
+            if GuestAddress(config_data.avail_ring_addr)
+                .checked_add(avail_ring_size)
+                .is_none_or(|v| !m.address_in_range(v))
+            {
+                return false;
+            }
+            if GuestAddress(config_data.used_ring_addr)
+                .checked_add(used_ring_size)
+                .is_none_or(|v| !m.address_in_range(v))
+            {
+                return false;
+            }


can we reduce the code duplication?

Implement packed virtqueue support to improve performance through better cache locality and reduced memory overhead compared to split virtqueues. This addresses the VirtIO specification. Changes: - Add shared vring flag constants in backend.rs for architecture independence - Support packed virtqueue detection in both vhost-kern and vhost-user - Implement proper feature classification (device vs transport features) - Add validation for packed ring memory layout - Reduce code duplication in validation logic - Add test coverage for packed virtqueue functionality Signed-off-by: Prakash Shekhar <[email protected]>

prakash-shekhar · 2025-06-25T20:55:53Z

Hey thank you all for the feedback. I appreciate all the comments, I'm still trying to get better at this. I won't make these mistakes again. I consolidated everything into one commit, rebased, and fixed the issues you mentioned. I'd appreciate any further feedback as well.

germag · 2025-06-26T09:20:20Z

I won't make these mistakes again.

don't worry, everyone makes mistakes, we are humans after all

I consolidated everything into one commit, rebased, and fixed the issues you mentioned. I'd appreciate any further feedback as well.

I don't want to give you the wrong impression that we want a single commit PR, the changes (I'm not talking about this particular PR, just in general) should be divided in multiple commits if it makes sense, but always following what I said before each commit must not "fix" previous commits in the same PR(*), break the build, test, etc. Otherwise, we cannot use git bisect when we need to find an issue.

(*) there are some exceptions of course, for instance, let's say in one commit you add a new function that you plan to use in another commit:

+ fn foo() {}

the linter will complain about dead code, so It's ok to add the function:

+ #[allow(dead_code)]
+ fn foo() {}

and in the next commit, when you call the function, to remove that attribute

-#[allow(dead_code)]
..
+ foo()

prakash-shekhar requested review from Ablu, eryugey, germag, jiangliu, sboeuf, slp and stefano-garzarella as code owners June 13, 2025 00:59

prakash-shekhar force-pushed the main branch from 46d3310 to c868655 Compare June 13, 2025 01:06

stefano-garzarella reviewed Jun 23, 2025

View reviewed changes

germag reviewed Jun 24, 2025

View reviewed changes

prakash-shekhar force-pushed the main branch from 70a49ca to b47c656 Compare June 25, 2025 20:53

Add packed virtqueue support #301

Are you sure you want to change the base?

Add packed virtqueue support #301

Uh oh!

Conversation

prakash-shekhar commented Jun 13, 2025

Summary of the PR

Requirements

Uh oh!

stefano-garzarella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prakash-shekhar commented Jun 25, 2025

Uh oh!

germag commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

germag commented Jun 26, 2025 •

edited

Loading