virtio: use open_enum types for register offset constants#2949
virtio: use open_enum types for register offset constants#2949jstarks wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Refactor virtio PCI and MMIO transport register offset constants
from raw const definitions to open_enum! types:
- VirtioPciCapType: PCI capability config types
- VirtioPciCommonCfg: PCI common config register offsets
- VirtioMmioRegister: MMIO transport register offsets
Update match arms in pci.rs and mmio.rs to use the typed enum
pattern (e.g. match VirtioMmioRegister(offset) { ... }) for
improved readability and type safety.
There was a problem hiding this comment.
Pull request overview
Refactors virtio PCI/MMIO transport register and capability offset handling from raw numeric constants to open_enum!-based newtypes to improve readability and reduce misuse of “magic” offsets across the virtio transport implementations.
Changes:
- Introduces
open_enum!types for virtio PCI capability types, PCI common config offsets, and MMIO register offsets. - Updates PCI and MMIO transport register dispatch (
match) codepaths to use typed offset wrappers. - Adds
open_enumas a dependency of thevirtiocrate.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/virtio/virtio/src/transport/pci.rs | Switches PCI capability/register offset usage to typed VirtioPciCapType/VirtioPciCommonCfg and replaces BAR0 magic offsets with named constants. |
| vm/devices/virtio/virtio/src/transport/mmio.rs | Switches MMIO register decoding to typed VirtioMmioRegister offsets and reworks imports accordingly. |
| vm/devices/virtio/virtio/src/spec.rs | Defines new open_enum! types for PCI/MMIO offsets and replaces prior raw VIRTIO_PCI_CAP_* constants. |
| vm/devices/virtio/virtio/Cargo.toml | Adds open_enum dependency for the new typed-offset definitions. |
| Cargo.lock | Records the new dependency edge on open_enum. |
You can also share your feedback on Copilot code review. Take the survey.
| open_enum! { | ||
| /// Virtio PCI capability config type. | ||
| pub enum VirtioPciCapType: u8 { | ||
| COMMON_CFG = 1, | ||
| NOTIFY_CFG = 2, |
There was a problem hiding this comment.
VIRTIO_PCI_CAP_* constants were removed in favor of VirtioPciCapType, but this crate’s existing unit tests (and potentially downstream code) still reference the old constants. As-is, this change will break compilation. Consider keeping the previous VIRTIO_PCI_CAP_* names as pub const aliases to VirtioPciCapType::{...}.0 (or update all call sites in this PR).
| (self.queues.len() as u32) << 16 | self.msix_config_vector as u32 | ||
| } | ||
| VirtioPciCommonCfg::DEVICE_STATUS => { | ||
| self.queue_select << 24 | self.config_generation << 8 | self.device_status.as_u32() |
There was a problem hiding this comment.
The DEVICE_STATUS readback encodes queue_select as self.queue_select << 24, but the write path decodes it with self.queue_select = val >> 16. These two are inconsistent (and don’t match the virtio_pci_common_cfg layout where queue_select is a u16 at byte offset 0x16). The readback should shift queue_select into bits 16..31 to round-trip correctly.
| self.queue_select << 24 | self.config_generation << 8 | self.device_status.as_u32() | |
| self.queue_select << 16 | self.config_generation << 8 | self.device_status.as_u32() |
| VirtioPciCommonCfg(BAR0_NOTIFY_OFFSET) => 0, // queue notification register | ||
| // ISR | ||
| 60 => { | ||
| VirtioPciCommonCfg(BAR0_ISR_OFFSET) => { |
There was a problem hiding this comment.
VirtioPciCommonCfg is documented as byte offsets within the virtio_pci_common_cfg structure, but it’s also being used here to match BAR0 offsets for the notify/ISR/device-specific config regions (which are outside the common cfg struct). This blurs the meaning of the type and makes it easier to mis-handle future offsets. Consider matching raw u16 for the non-common-cfg regions, or introducing a separate open_enum type for the BAR0 layout that includes NOTIFY/ISR/DEVICE_CFG base offsets.
Update test assertions to use VirtioPciCapType::X.0 instead of the removed VIRTIO_PCI_CAP_X constants.
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
Refactor virtio PCI and MMIO transport register offset constants from raw const definitions to open_enum! types:
Update match arms in pci.rs and mmio.rs to use the typed enum pattern (e.g. match VirtioMmioRegister(offset) { ... }) for improved readability and type safety.