virtio: switch to bitfield_struct #2207

Brian-Perkins · 2025-10-18T00:17:53Z

Move virtio flags to a bitfield_struct for easier usage

Copilot

Pull Request Overview

This PR moves virtio flags from simple integer values to structured bitfield types for improved type safety and readability. The changes replace raw u64 device features with a new VirtioDeviceFeatures struct that uses bitfield_struct for organized feature management.

Replace raw u64 features with structured VirtioDeviceFeatures type
Introduce bitfield structs for VirtioDeviceStatus and feature banks
Update all virtio device implementations to use the new structured features

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
vm/devices/virtio/virtiofs/src/virtio.rs	Updates VirtioFS device to use VirtioDeviceFeatures::new() instead of 0
vm/devices/virtio/virtio_serial/src/lib.rs	Converts feature constants from u64 to u32 and uses structured features
vm/devices/virtio/virtio_pmem/src/lib.rs	Updates PMEM device to use VirtioDeviceFeatures::new()
vm/devices/virtio/virtio_p9/src/lib.rs	Converts P9 device to use structured features with bank system
vm/devices/virtio/virtio_net/src/lib.rs	Major refactor of network features to use bitfield structs
vm/devices/virtio/virtio/src/transport/pci.rs	Updates PCI transport to use structured device status and features
vm/devices/virtio/virtio/src/transport/mmio.rs	Updates MMIO transport to use structured features and status
vm/devices/virtio/virtio/src/tests.rs	Extensive test updates to use new structured feature types
vm/devices/virtio/virtio/src/spec.rs	Adds bitfield struct definitions for VirtioDeviceFeatures and VirtioDeviceStatus
vm/devices/virtio/virtio/src/queue.rs	Updates queue core to accept VirtioDeviceFeatures instead of u64
vm/devices/virtio/virtio/src/common.rs	Updates common types and traits to use VirtioDeviceFeatures

Copilot · 2025-10-18T00:18:43Z

vm/devices/virtio/virtio_net/src/lib.rs

+//     _reserved: bool,
+//     pub guest_hdrlen: bool,
+//     pub rss: bool,
+//     pub rsc_ext: bool,
+//     pub standby: bool,
+//     pub speed_duplex: bool,
+// }
+// #[bitfield(u32)]
+// #[derive(IntoBytes, Immutable, KnownLayout, FromBytes)]
+// struct NetworkFeaturesBank2 {
+//     pub rss_context: bool // VIRTIO_NET_F_RSS_CONTEXT(64)
+//     pub guest_udp_tunnel_gso: bool,
+//     pub guest_udp_tunnel_gso_csum: bool,
+//     pub host_udp_tunnel_gso: bool,
+//     pub host_udp_tunnel_gso_csum: bool,
+//     pub out_net_header: bool,
+//     pub ipsec: bool,
+// }


[nitpick] This large block of commented-out code should be removed if it's not needed, or converted to proper documentation comments (///) with a TODO if future implementation is planned.

Suggested change

// _reserved: bool,

// pub guest_hdrlen: bool,

// pub rss: bool,

// pub rsc_ext: bool,

// pub standby: bool,

// pub speed_duplex: bool,

// }

// #[bitfield(u32)]

// #[derive(IntoBytes, Immutable, KnownLayout, FromBytes)]

// struct NetworkFeaturesBank2 {

// pub rss_context: bool // VIRTIO_NET_F_RSS_CONTEXT(64)

// pub guest_udp_tunnel_gso: bool,

// pub guest_udp_tunnel_gso_csum: bool,

// pub host_udp_tunnel_gso: bool,

// pub host_udp_tunnel_gso_csum: bool,

// pub out_net_header: bool,

// pub ipsec: bool,

// }

/// TODO: Implement additional network feature banks if needed in the future.

///

/// The following structs are placeholders for future expansion of network features:

/// - `NetworkFeaturesBank1`

/// - `NetworkFeaturesBank2`

///

/// Refer to the Virtio specification for details on feature flags.

Copilot · 2025-10-18T00:18:44Z

vm/devices/virtio/virtio/src/spec.rs

+    pub fn set_bank(&mut self, index: usize, val: u32) {
+        if self.0.len() <= index {
+            self.0.resize(index + 1, 0);
+        }
+        self.0[index] = val;
+    }


[nitpick] The resize operation could be inefficient for large indices. Consider adding bounds checking or documentation about expected index ranges to prevent excessive memory allocation.

Copilot · 2025-10-18T00:18:44Z

vm/devices/virtio/virtio/src/transport/pci.rs

+                } else {
+                    self.driver_feature
+                        .set_bank(bank, val & self.device_feature.bank(bank));
                }


[nitpick] The conditional logic could be simplified by inverting the condition and using early return or guard clause pattern for better readability.

Suggested change

} else {

self.driver_feature

.set_bank(bank, val & self.device_feature.bank(bank));

}

return;

}

self.driver_feature

.set_bank(bank, val & self.device_feature.bank(bank));

mattkur

Couple minor nits, but I don't think they are required to commit this.
Overall this looks good to me. I'd like to see someone more familiar with this code sign off in case of gremlins, but can take another look if other reviewers aren't able to get back to this.

mattkur · 2025-10-20T12:53:54Z

vm/devices/virtio/virtio_serial/src/lib.rs

+        let features = VirtioDeviceFeatures::new().with_bank(
+            0,
+            if self.config.max_ports > 1 {
+                VIRTIO_CONSOLE_F_MULTIPORT
+            } else {
+                0
+            },
+        );


nit: why not this:

Suggested change

let features = VirtioDeviceFeatures::new().with_bank(

0,

if self.config.max_ports > 1 {

VIRTIO_CONSOLE_F_MULTIPORT

} else {

0

},

);

let features = VirtioDeviceFeatures::new().with_bank0(VirtioDeviceFeaturesBank0::new().with_device_specific(

if self.config.max_ports > 1 {

VIRTIO_CONSOLE_F_MULTIPORT

} else {

0

},)

);

mattkur · 2025-10-20T12:54:43Z

vm/devices/virtio/virtio_serial/src/lib.rs

            // if multi-port is set, start the control port thread
            VirtioState::Running(run_state) => {
-                if run_state.features & VIRTIO_CONSOLE_F_MULTIPORT != 0 {
+                if run_state.features.bank(0) & VIRTIO_CONSOLE_F_MULTIPORT != 0 {


nit:

Suggested change

if run_state.features.bank(0) & VIRTIO_CONSOLE_F_MULTIPORT != 0 {

if run_state.features.bank0().device_specific() & VIRTIO_CONSOLE_F_MULTIPORT != 0 {

smalis-msft · 2025-10-20T15:55:29Z

vm/devices/virtio/virtio/src/spec.rs

+    pub fn with_bank(mut self, index: usize, val: u32) -> Self {
+        self.set_bank(index, val);
+        self
+    }


Do we need the non-typed version, or can everything be done with the typed helpers below?

The spec supports up to 256 "banks" (8192 feature bits), though currently I think the high-water mark is the network device with bit 70. We could define explicit accessors for three banks and would be unlikely to hit any problems for a number of years, but I was just going with the spirit of "device specific number of interesting bits"

Gotcha. We should definitely update most of the uses in this PR to use the typed versions instead of hardcoding with_bank(0, or similar though.

smalis-msft · 2025-10-20T15:57:07Z

vm/devices/virtio/virtio/src/spec.rs

-pub const VIRTIO_FAILED: u32 = 0x80;
+#[bitfield(u32)]
+#[derive(IntoBytes, Immutable, KnownLayout, FromBytes)]
+pub struct VirtioDeviceFeaturesBank0 {


Should these bank types be Copy too?

smalis-msft · 2025-10-20T15:57:48Z

vm/devices/virtio/virtio/src/spec.rs

+pub struct VirtioDeviceFeatures(Vec<u32>);
+impl VirtioDeviceFeatures {
+    pub fn new() -> Self {
+        Self(Vec::with_capacity(2))


Do you only want capacity? Or do you want to actually have two zeros in the vec? Should this be a statically sized array?

These feature bits are accessed by setting a selector index and accessing the feature data location. For example, if you set the selector index to 0x10 and access the data you will get bits 0x200-0x21f. In practice though almost every device uses just the first 64 bits, which is why the capacity is set to two. If the driver requests index 0x10 we don't normally need to store any information -- we can just return '0' / no bits set. There is also an address for the driver to write its own bits, but it happens that the driver can also only request/set bits that the device supports, so if they try to write using index 0x10, since the device normally has no bits defined there, it is filtered back to '0', which again we don't actually have to store any value to represent.

Should we consider some sort of sparse storage solution here, or is a Vec the right data structure? Like if a device does actually define a bank 10 does that definitely mean it's going to use banks 0-9? If we expect every device to use banks 0 & 1 maybe they should be hardcoded in as separate fields, and then any additional banks could be in a heap-allocated structure? Just spitballing.

virtio: switch to bitfield_struct

2802e33

Brian-Perkins requested a review from a team as a code owner October 18, 2025 00:17

Copilot AI review requested due to automatic review settings October 18, 2025 00:17

Copilot AI reviewed Oct 18, 2025

View reviewed changes

mattkur reviewed Oct 20, 2025

View reviewed changes

smalis-msft reviewed Oct 20, 2025

View reviewed changes

	if run_state.features.bank(0) & VIRTIO_CONSOLE_F_MULTIPORT != 0 {
	if run_state.features.bank0().device_specific() & VIRTIO_CONSOLE_F_MULTIPORT != 0 {

virtio: switch to bitfield_struct #2207

Are you sure you want to change the base?

virtio: switch to bitfield_struct #2207

Uh oh!

Conversation

Brian-Perkins commented Oct 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

mattkur 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants