Move PCI from bitflags to bitfield-struct (#462)

Part of moving all of our bitfield types to bitfield-struct. Only IDE is
left I think. There could definitely be more cleanup here to make things
more idiomatic, but I'm just trying to do a minimum viable change here
for now.

Author: smalis-msft

Cargo.lock

@@ -4953,7 +4953,7 @@ dependencies = [
 name = "pci_core"
 version = "0.0.0"
 dependencies = [
- "bitflags 1.3.2",
+ "bitfield-struct",
  "chipset_device",
  "guestmem",
  "inspect",

vm/devices/pci/pci_core/Cargo.toml

@@ -11,10 +11,10 @@ chipset_device.workspace = true
 guestmem.workspace = true
 vmcore.workspace = true
 
+bitfield-struct.workspace = true
 inspect.workspace = true
 mesh.workspace = true
 open_enum.workspace = true
-bitflags.workspace = true
 parking_lot.workspace = true
 thiserror.workspace = true
 tracelimit.workspace = true

vm/devices/pci/pci_core/src/bar_mapping.rs

@@ -42,7 +42,7 @@ impl BarMappings {
             let bar_address;
             let mut bar_mask;
             let len;
-            if bar_masks[i] & cfg_space::BarEncodingBits::TYPE_64_BIT.bits() != 0 {
+            if cfg_space::BarEncodingBits::from_bits(bar_masks[i]).type_64_bit() {
                 bar_mask = (bar_masks[i + 1] as u64) << 32 | bar_masks[i] as u64;
                 bar_address = (base_addresses[i + 1] as u64) << 32 | base_addresses[i] as u64;
                 len = 2;

vm/devices/pci/pci_core/src/cfg_space_emu.rs

@@ -23,6 +23,19 @@ use std::sync::atomic::Ordering;
 use std::sync::Arc;
 use vmcore::line_interrupt::LineInterrupt;
 
+const SUPPORTED_COMMAND_BITS: u16 = cfg_space::Command::new()
+    .with_pio_enabled(true)
+    .with_mmio_enabled(true)
+    .with_bus_master(true)
+    .with_special_cycles(true)
+    .with_enable_memory_write_invalidate(true)
+    .with_vga_palette_snoop(true)
+    .with_parity_error_response(true)
+    .with_enable_serr(true)
+    .with_enable_fast_b2b(true)
+    .with_intx_disable(true)
+    .into_bits();
+
 /// A wrapper around a [`LineInterrupt`] that considers PCI configuration space
 /// interrupt control bits.
 #[derive(Debug, Inspect)]
@@ -98,7 +111,7 @@ impl ConfigSpaceType0EmulatorState {
     fn new() -> Self {
         Self {
             latency_timer: 0,
-            command: cfg_space::Command::empty(),
+            command: cfg_space::Command::new(),
             base_addresses: [0; 6],
             interrupt_line: 0,
         }
@@ -247,7 +260,9 @@ impl ConfigSpaceType0Emulator {
             const MIN_BAR_SIZE: u64 = 4096;
             let len = std::cmp::max(len.next_power_of_two(), MIN_BAR_SIZE);
             let mask64 = !(len - 1);
-            bar_masks[bar_index] = mask64 as u32 | cfg_space::BarEncodingBits::TYPE_64_BIT.bits();
+            bar_masks[bar_index] = cfg_space::BarEncodingBits::from_bits(mask64 as u32)
+                .with_type_64_bit(true)
+                .into_bits();
             bar_masks[bar_index + 1] = (mask64 >> 32) as u32;
             mapped_memory[bar_index] = Some(mapped);
         }
@@ -264,7 +279,7 @@ impl ConfigSpaceType0Emulator {
             intx_interrupt: None,
 
             state: ConfigSpaceType0EmulatorState {
-                command: cfg_space::Command::empty(),
+                command: cfg_space::Command::new(),
                 base_addresses: [0; 6],
                 interrupt_line: 0,
                 latency_timer: 0,
@@ -332,18 +347,16 @@ impl ConfigSpaceType0Emulator {
                 (self.hardware_ids.device_id as u32) << 16 | self.hardware_ids.vendor_id as u32
             }
             HeaderType00::STATUS_COMMAND => {
-                let mut status = cfg_space::Status::empty();
-                if !self.capabilities.is_empty() {
-                    status |= cfg_space::Status::CAPABILITIES_LIST;
-                }
+                let mut status =
+                    cfg_space::Status::new().with_capabilities_list(!self.capabilities.is_empty());
 
                 if let Some(intx_interrupt) = &self.intx_interrupt {
                     if intx_interrupt.interrupt_status.load(Ordering::SeqCst) {
-                        status |= cfg_space::Status::INTERRUPT_STATUS;
+                        status.set_interrupt_status(true);
                     }
                 }
 
-                (status.bits() as u32) << 16 | self.state.command.bits() as u32
+                (status.into_bits() as u32) << 16 | self.state.command.into_bits() as u32
             }
             HeaderType00::CLASS_REVISION => {
                 (u8::from(self.hardware_ids.base_class) as u32) << 24
@@ -436,12 +449,12 @@ impl ConfigSpaceType0Emulator {
 
     fn update_intx_disable(&mut self, command: cfg_space::Command) {
         if let Some(intx_interrupt) = &self.intx_interrupt {
-            intx_interrupt.set_disabled(command.contains(cfg_space::Command::INTX_DISABLE))
+            intx_interrupt.set_disabled(command.intx_disable())
         }
     }
 
     fn update_mmio_enabled(&mut self, command: cfg_space::Command) {
-        if command.contains(cfg_space::Command::MMIO_ENABLED) {
+        if command.mmio_enabled() {
             self.active_bars = BarMappings::parse(&self.state.base_addresses, &self.bar_masks);
             for (bar, mapping) in self.mapped_memory.iter_mut().enumerate() {
                 if let Some(mapping) = mapping {
@@ -478,30 +491,19 @@ impl ConfigSpaceType0Emulator {
 
         match HeaderType00(offset) {
             HeaderType00::STATUS_COMMAND => {
-                let command = match cfg_space::Command::from_bits(val as u16) {
-                    Some(command) => command,
-                    None => {
-                        tracelimit::warn_ratelimited!(offset, val, "setting invalid command bits");
-                        // still do our best
-                        cfg_space::Command::from_bits_truncate(val as u16)
-                    }
+                let mut command = cfg_space::Command::from_bits(val as u16);
+                if command.into_bits() & !SUPPORTED_COMMAND_BITS != 0 {
+                    tracelimit::warn_ratelimited!(offset, val, "setting invalid command bits");
+                    // still do our best
+                    command =
+                        cfg_space::Command::from_bits(command.into_bits() & SUPPORTED_COMMAND_BITS);
                 };
 
-                if self
-                    .state
-                    .command
-                    .contains(cfg_space::Command::INTX_DISABLE)
-                    != command.contains(cfg_space::Command::INTX_DISABLE)
-                {
+                if self.state.command.intx_disable() != command.intx_disable() {
                     self.update_intx_disable(command)
                 }
 
-                if self
-                    .state
-                    .command
-                    .contains(cfg_space::Command::MMIO_ENABLED)
-                    != command.contains(cfg_space::Command::MMIO_ENABLED)
-                {
+                if self.state.command.mmio_enabled() != command.mmio_enabled() {
                     self.update_mmio_enabled(command)
                 }
 
@@ -518,15 +520,13 @@ impl ConfigSpaceType0Emulator {
             | HeaderType00::BAR3
             | HeaderType00::BAR4
             | HeaderType00::BAR5 => {
-                if !self
-                    .state
-                    .command
-                    .contains(cfg_space::Command::MMIO_ENABLED)
-                {
+                if !self.state.command.mmio_enabled() {
                     let bar_index = (offset - HeaderType00::BAR0.0) as usize / 4;
                     let mut bar_value = val & self.bar_masks[bar_index];
                     if bar_index & 1 == 0 && self.bar_masks[bar_index] != 0 {
-                        bar_value |= cfg_space::BarEncodingBits::TYPE_64_BIT.bits();
+                        bar_value = cfg_space::BarEncodingBits::from_bits(bar_value)
+                            .with_type_64_bit(true)
+                            .into_bits();
                     }
                     self.state.base_addresses[bar_index] = bar_value;
                 }
@@ -623,7 +623,7 @@ mod save_restore {
             } = self.state;
 
             let saved_state = state::SavedState {
-                command: command.bits(),
+                command: command.into_bits(),
                 base_addresses,
                 interrupt_line,
                 latency_timer,
@@ -650,16 +650,18 @@ mod save_restore {
             } = state;
 
             self.state = ConfigSpaceType0EmulatorState {
-                command: cfg_space::Command::from_bits(command).ok_or(
-                    RestoreError::InvalidSavedState(
-                        ConfigSpaceRestoreError::InvalidConfigBits.into(),
-                    ),
-                )?,
+                command: cfg_space::Command::from_bits(command),
                 base_addresses,
                 interrupt_line,
                 latency_timer,
             };
 
+            if command & !SUPPORTED_COMMAND_BITS != 0 {
+                return Err(RestoreError::InvalidSavedState(
+                    ConfigSpaceRestoreError::InvalidConfigBits.into(),
+                ));
+            }
+
             self.sync_command_register(self.state.command);
             for (id, entry) in capabilities {
                 tracing::debug!(save_id = id.as_str(), "restoring pci capability");

vm/devices/pci/pci_core/src/spec.rs

@@ -186,6 +186,7 @@ pub mod hwid {
 /// Sources: PCI 2.3 Spec - Chapter 6
 #[allow(missing_docs)] // primarily enums/structs with self-explanatory variants
 pub mod cfg_space {
+    use bitfield_struct::bitfield;
     use inspect::Inspect;
     use zerocopy::AsBytes;
     use zerocopy::FromBytes;
@@ -236,63 +237,92 @@ pub mod cfg_space {
 
     pub const HEADER_TYPE_00_SIZE: u16 = 0x40;
 
-    bitflags::bitflags! {
-        /// BAR in-band encoding bits.
-        ///
-        /// The low bits of the BAR are not actually part of the address.
-        /// Instead, they are used to in-band encode various bits of
-        /// metadata about the BAR, and are masked off when determining the
-        /// actual address.
-        pub struct BarEncodingBits: u32 {
-            const USE_PIO = 1 << 0;
-            // only used in MMIO
-            const TYPE_32_BIT = 0b00 << 1;
-            const TYPE_64_BIT = 0b10 << 1;
-            const PREFETCHABLE = 1 << 3;
-        }
+    /// BAR in-band encoding bits.
+    ///
+    /// The low bits of the BAR are not actually part of the address.
+    /// Instead, they are used to in-band encode various bits of
+    /// metadata about the BAR, and are masked off when determining the
+    /// actual address.
+    #[bitfield(u32)]
+    pub struct BarEncodingBits {
+        pub use_pio: bool,
+
+        _reserved: bool,
+
+        /// False indicates 32 bit.
+        /// Only used in MMIO
+        pub type_64_bit: bool,
+        pub prefetchable: bool,
+
+        #[bits(28)]
+        _reserved2: u32,
     }
 
-    bitflags::bitflags! {
-        /// Command Register
-        #[derive(AsBytes, FromBytes, FromZeroes, Inspect)]
-        #[repr(transparent)]
-        #[inspect(debug)]
-        pub struct Command: u16 {
-            const PIO_ENABLED                    = 1 << 0;
-            const MMIO_ENABLED                   = 1 << 1;
-            const BUS_MASTER                     = 1 << 2;
-            const SPECIAL_CYCLES                 = 1 << 3;
-            const ENABLE_MEMORY_WRITE_INVALIDATE = 1 << 4;
-            const VGA_PALETTE_SNOOP              = 1 << 5;
-            const PARITY_ERROR_RESPONSE          = 1 << 6;
-            // const RESERVED                    = 1 << 7; // must be 0
-            const ENABLE_SERR                    = 1 << 8;
-            const ENABLE_FAST_B2B                = 1 << 9;
-            const INTX_DISABLE                   = 1 << 10;
-            // rest of bits are reserved
-        }
+    /// Command Register
+    #[derive(Inspect)]
+    #[bitfield(u16)]
+    #[derive(AsBytes, FromBytes, FromZeroes)]
+    pub struct Command {
+        pub pio_enabled: bool,
+        pub mmio_enabled: bool,
+        pub bus_master: bool,
+        pub special_cycles: bool,
+        pub enable_memory_write_invalidate: bool,
+        pub vga_palette_snoop: bool,
+        pub parity_error_response: bool,
+        /// must be 0
+        #[bits(1)]
+        _reserved: u16,
+        pub enable_serr: bool,
+        pub enable_fast_b2b: bool,
+        pub intx_disable: bool,
+        #[bits(5)]
+        _reserved2: u16,
+    }
+
+    /// Status Register
+    #[bitfield(u16)]
+    #[derive(AsBytes, FromBytes, FromZeroes)]
+    pub struct Status {
+        #[bits(3)]
+        _reserved: u16,
+        pub interrupt_status: bool,
+        pub capabilities_list: bool,
+        pub capable_mhz_66: bool,
+        _reserved2: bool,
+        pub capable_fast_b2b: bool,
+        pub err_master_parity: bool,
+
+        #[bits(2)]
+        pub devsel: DevSel,
+
+        pub abort_target_signaled: bool,
+        pub abort_target_received: bool,
+        pub abort_master_received: bool,
+        pub err_signaled: bool,
+        pub err_detected_parity: bool,
     }
 
-    bitflags::bitflags! {
-        /// Status Register
-        #[derive(AsBytes, FromBytes, FromZeroes)]
-        #[repr(transparent)]
-        pub struct Status: u16 {
-            // const RESERVED           = 0b000 << 0;
-            const INTERRUPT_STATUS      = 1 << 3;
-            const CAPABILITIES_LIST     = 1 << 4;
-            const CAPABLE_MHZ_66        = 1 << 5;
-            // const RESERVED           = 1 << 6;
-            const CAPABLE_FAST_B2B      = 1 << 7;
-            const ERR_MASTER_PARITY     = 1 << 8;
-            const DEVSEL_FAST           = 0b00 << 10;
-            const DEVSEL_MED            = 0b01 << 10;
-            const DEVSEL_SLOW           = 0b10 << 10;
-            const ABORT_TARGET_SIGNALED = 1 << 11;
-            const ABORT_TARGET_RECEIVED = 1 << 12;
-            const ABORT_MASTER_RECEIVED = 1 << 13;
-            const ERR_SIGNALED          = 1 << 14;
-            const ERR_DETECTED_PARITY   = 1 << 15;
+    #[derive(Debug)]
+    #[repr(u16)]
+    pub enum DevSel {
+        Fast = 0b00,
+        Medium = 0b01,
+        Slow = 0b10,
+    }
+
+    impl DevSel {
+        const fn from_bits(bits: u16) -> Self {
+            match bits {
+                0b00 => DevSel::Fast,
+                0b01 => DevSel::Medium,
+                0b10 => DevSel::Slow,
+                _ => unreachable!(),
+            }
+        }
+
+        const fn into_bits(self) -> u16 {
+            self as u16
         }
     }
 }