virtio-devices: Bump config_generation on device config read

Set a config_changed flag in VirtioInterruptMsix when a Config
interrupt fires, and increment config_generation only when the
driver next reads the device specific configuration region. The
flag is cleared by that read so the driver observes a stable
value across the read and a fresh value on any later read.

This avoids the wrap hazard of incrementing on every Config
event, where a burst of interrupts could roll the 8 bit counter
back to its previous value between two driver reads.

Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>

virtio-devices/src/transport/pci_common_config.rs

@@ -6,7 +6,7 @@
 //
 // SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause
 
-use std::sync::atomic::{AtomicU8, AtomicU16, Ordering};
+use std::sync::atomic::{AtomicBool, AtomicU8, AtomicU16, Ordering};
 use std::sync::{Arc, Mutex};
 
 use byteorder::{ByteOrder, LittleEndian};
@@ -127,6 +127,10 @@ pub struct VirtioPciCommonConfig {
     pub device: Arc<Mutex<dyn VirtioDevice>>,
     pub driver_status: Arc<AtomicU8>,
     pub config_generation: Arc<AtomicU8>,
+    /// Set when a Config interrupt fires. Cleared on the next read of
+    /// the device specific configuration region, which also bumps
+    /// config_generation.
+    pub config_changed: Arc<AtomicBool>,
     pub device_feature_select: u32,
     pub driver_feature_select: u32,
     pub queue_select: u16,
@@ -140,6 +144,7 @@ impl VirtioPciCommonConfig {
             device,
             driver_status: Arc::new(AtomicU8::new(state.driver_status)),
             config_generation: Arc::new(AtomicU8::new(state.config_generation)),
+            config_changed: Arc::new(AtomicBool::new(false)),
             device_feature_select: state.device_feature_select,
             driver_feature_select: state.driver_feature_select,
             queue_select: state.queue_select,
@@ -148,6 +153,16 @@ impl VirtioPciCommonConfig {
         }
     }
 
+    /// If a Config interrupt has fired since the last device specific
+    /// configuration read, increment config_generation and clear the
+    /// pending flag.
+    pub fn consume_config_change(&self) {
+        if self.config_changed.swap(false, Ordering::AcqRel) {
+            // Wrap at u8 max is intentional per the virtio spec.
+            self.config_generation.fetch_add(1, Ordering::Release);
+        }
+    }
+
     fn state(&self) -> VirtioPciCommonConfigState {
         VirtioPciCommonConfigState {
             driver_status: self.driver_status.load(Ordering::Acquire),
@@ -165,6 +180,7 @@ impl VirtioPciCommonConfig {
     /// observe.
     pub fn reset(&mut self) {
         self.driver_status.store(0, Ordering::Release);
+        self.config_changed.store(false, Ordering::Release);
         self.device_feature_select = 0;
         self.driver_feature_select = 0;
         self.queue_select = 0;
@@ -462,6 +478,7 @@ mod unit_tests {
             device: dev.clone(),
             driver_status: Arc::new(AtomicU8::new(0xaa)),
             config_generation: Arc::new(AtomicU8::new(0x55)),
+            config_changed: Arc::new(AtomicBool::new(false)),
             device_feature_select: 0x0,
             driver_feature_select: 0x0,
             queue_select: 0xff,
@@ -514,6 +531,7 @@ mod unit_tests {
             device: dev.clone(),
             driver_status: Arc::new(AtomicU8::new(0)),
             config_generation: Arc::new(AtomicU8::new(0)),
+            config_changed: Arc::new(AtomicBool::new(false)),
             device_feature_select: 0,
             driver_feature_select: 0,
             queue_select: 0,
@@ -542,6 +560,7 @@ mod unit_tests {
             device: dev,
             driver_status: Arc::new(AtomicU8::new(0x55)),
             config_generation: Arc::new(AtomicU8::new(0xab)),
+            config_changed: Arc::new(AtomicBool::new(true)),
             device_feature_select: 1,
             driver_feature_select: 1,
             queue_select: 7,
@@ -553,6 +572,7 @@ mod unit_tests {
 
         assert_eq!(regs.driver_status.load(Ordering::Acquire), 0);
         assert_eq!(regs.config_generation.load(Ordering::Acquire), 0xab); // unchanged across reset
+        assert!(!regs.config_changed.load(Ordering::Acquire));
         assert_eq!(regs.device_feature_select, 0);
         assert_eq!(regs.driver_feature_select, 0);
         assert_eq!(regs.queue_select, 0);

virtio-devices/src/transport/pci_device.rs

@@ -591,6 +591,7 @@ impl VirtioPciDevice {
         let virtio_interrupt = Arc::new(VirtioInterruptMsix::new(
             msix_config.clone(),
             common_config.msix_config.clone(),
+            common_config.config_changed.clone(),
             common_config.msix_queues.clone(),
             interrupt_source_group.clone(),
         ));
@@ -851,6 +852,7 @@ impl VirtioTransport for VirtioPciDevice {
 pub struct VirtioInterruptMsix {
     msix_config: Arc<Mutex<MsixConfig>>,
     config_vector: Arc<AtomicU16>,
+    config_changed: Arc<AtomicBool>,
     queues_vectors: Arc<Mutex<Vec<u16>>>,
     interrupt_source_group: MaybeMutInterruptSourceGroup,
     msix_table_size: usize,
@@ -860,6 +862,7 @@ impl VirtioInterruptMsix {
     pub fn new(
         msix_config: Arc<Mutex<MsixConfig>>,
         config_vector: Arc<AtomicU16>,
+        config_changed: Arc<AtomicBool>,
         queues_vectors: Arc<Mutex<Vec<u16>>>,
         interrupt_source_group: MaybeMutInterruptSourceGroup,
     ) -> Self {
@@ -867,6 +870,7 @@ impl VirtioInterruptMsix {
         VirtioInterruptMsix {
             msix_config,
             config_vector,
+            config_changed,
             queues_vectors,
             interrupt_source_group,
             msix_table_size,
@@ -876,6 +880,10 @@ impl VirtioInterruptMsix {
 
 impl VirtioInterrupt for VirtioInterruptMsix {
     fn trigger(&self, int_type: VirtioInterruptType) -> std::result::Result<(), std::io::Error> {
+        if matches!(int_type, VirtioInterruptType::Config) {
+            self.config_changed.store(true, Ordering::Release);
+        }
+
         let vector = match int_type {
             VirtioInterruptType::Config => self.config_vector.load(Ordering::Acquire),
             VirtioInterruptType::Queue(queue_index) => {
@@ -1159,6 +1167,7 @@ impl PciDevice for VirtioPciDevice {
             o if (DEVICE_CONFIG_BAR_OFFSET..DEVICE_CONFIG_BAR_OFFSET + DEVICE_CONFIG_SIZE)
                 .contains(&o) =>
             {
+                self.common_config.consume_config_change();
                 let device = self.device.lock().unwrap();
                 device.read_config(o - DEVICE_CONFIG_BAR_OFFSET, data);
             }
@@ -1362,11 +1371,13 @@ mod unit_tests {
             .unwrap(),
         ));
         let config_vector = Arc::new(AtomicU16::new(VIRTQ_MSI_NO_VECTOR));
+        let config_changed = Arc::new(AtomicBool::new(false));
         let queues_vectors = Arc::new(Mutex::new(vec![VIRTQ_MSI_NO_VECTOR; 1]));
 
         VirtioInterruptMsix::new(
             msix_config,
             config_vector,
+            config_changed,
             queues_vectors,
             MaybeMutInterruptSourceGroup::Immutable(isg),
         )