From e38c5c434038776a7c2cc01d9dbe72d3c057d493 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 13 Apr 2026 17:23:08 +0800 Subject: [PATCH] pci: rollback BAR address on failed move_bar When BAR reprogramming is detected, detect_bar_reprogramming() eagerly updates the BAR address in config space before the actual MMIO remapping occurs. If the subsequent move_bar() fails (e.g. the new address falls outside the allocator range), the config register retains the new address while the MMIO bus still uses the old one, leaving the device broken. Add restore_bar_addr() to undo the config space update when move_bar() fails, so the device remains functional at its original address. For 64-bit BARs, restore both the low and high BAR slots as well as the corresponding config registers, mirroring the two-slot update logic in detect_bar_reprogramming(). Implement restore_bar_addr() for all PciDevice implementations (VirtioPciDevice, VfioPciDevice, VfioUserPciDevice, IvshmemDevice, PvPanicDevice, and PvmemcontrolPciDevice) by delegating to their respective PciConfiguration::restore_bar_addr(). Signed-off-by: CMGS --- devices/src/ivshmem.rs | 4 ++ devices/src/pvmemcontrol.rs | 4 ++ devices/src/pvpanic.rs | 4 ++ pci/src/bus.rs | 16 ++++--- pci/src/configuration.rs | 49 ++++++++++++++++++++++ pci/src/device.rs | 4 ++ pci/src/vfio.rs | 4 ++ pci/src/vfio_user.rs | 4 ++ virtio-devices/src/transport/pci_device.rs | 4 ++ 9 files changed, 88 insertions(+), 5 deletions(-) diff --git a/devices/src/ivshmem.rs b/devices/src/ivshmem.rs index 98291c74e..932e0d9eb 100644 --- a/devices/src/ivshmem.rs +++ b/devices/src/ivshmem.rs @@ -382,6 +382,10 @@ impl PciDevice for IvshmemDevice { Ok(()) } + fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) { + self.configuration.restore_bar_addr(params); + } + fn as_any_mut(&mut self) -> &mut dyn Any { self } diff --git a/devices/src/pvmemcontrol.rs b/devices/src/pvmemcontrol.rs index d4b37456b..171fdf154 100644 --- a/devices/src/pvmemcontrol.rs +++ b/devices/src/pvmemcontrol.rs @@ -712,6 +712,10 @@ impl PciDevice for PvmemcontrolPciDevice { self.configuration.read_config_register(reg_idx) } + fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) { + self.configuration.restore_bar_addr(params); + } + fn as_any_mut(&mut self) -> &mut dyn std::any::Any { self } diff --git a/devices/src/pvpanic.rs b/devices/src/pvpanic.rs index 9540a9125..3b9c9d5a8 100644 --- a/devices/src/pvpanic.rs +++ b/devices/src/pvpanic.rs @@ -231,6 +231,10 @@ impl PciDevice for PvPanicDevice { Ok(()) } + fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) { + self.configuration.restore_bar_addr(params); + } + fn read_bar(&mut self, _base: u64, _offset: u64, data: &mut [u8]) { data[0] = self.events; } diff --git a/pci/src/bus.rs b/pci/src/bus.rs index eaae23a4d..1fa7bd866 100644 --- a/pci/src/bus.rs +++ b/pci/src/bus.rs @@ -10,7 +10,7 @@ use std::ops::DerefMut; use std::sync::{Arc, Barrier, Mutex}; use byteorder::{ByteOrder, LittleEndian}; -use log::error; +use log::warn; use thiserror::Error; use vm_device::{Bus, BusDevice, BusDeviceSync}; @@ -280,10 +280,15 @@ impl PciConfigIo { device.deref_mut(), params.region_type, ) { - error!( - "Failed moving device BAR: {}: 0x{:x}->0x{:x}(0x{:x})", + warn!( + "Failed moving device BAR: {}: 0x{:x}->0x{:x}(0x{:x}), keeping old BAR", e, params.old_base, params.new_base, params.len ); + // Rollback: the config register was already updated to + // new_base by detect_bar_reprogramming(). Restore it by + // writing back the old address so device state stays + // consistent with the MMIO bus mapping. + device.restore_bar_addr(params); } } @@ -405,10 +410,11 @@ impl PciConfigMmio { device.deref_mut(), params.region_type, ) { - error!( - "Failed moving device BAR: {}: 0x{:x}->0x{:x}(0x{:x})", + warn!( + "Failed moving device BAR: {}: 0x{:x}->0x{:x}(0x{:x}), keeping old BAR", e, params.old_base, params.new_base, params.len ); + device.restore_bar_addr(params); } } } diff --git a/pci/src/configuration.rs b/pci/src/configuration.rs index f506017b2..2a905e19b 100644 --- a/pci/src/configuration.rs +++ b/pci/src/configuration.rs @@ -1093,6 +1093,55 @@ impl PciConfiguration { pub(crate) fn clear_pending_bar_reprogram(&mut self) { self.pending_bar_reprogram = Vec::new(); } + + /// Restore BAR address after a failed move. This undoes the premature + /// address update in detect_bar_reprogramming() so that config space + /// stays consistent with the actual MMIO mapping. + pub fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) { + match params.region_type { + PciBarRegionType::Memory64BitRegion => { + // 64-bit BAR spans two slots: bars[i] (low, type Memory64BitRegion) + // and bars[i+1] (high, type None). Mirror detect_bar_reprogramming + // by matching the combined address and restoring both halves. + for i in 0..NUM_BAR_REGS - 1 { + if self.bars[i].r#type != Some(PciBarRegionType::Memory64BitRegion) { + continue; + } + let low_mask = self.writable_bits[BAR0_REG + i]; + let high_mask = self.writable_bits[BAR0_REG + i + 1]; + let current = (u64::from(self.bars[i + 1].addr & high_mask) << 32) + | u64::from(self.bars[i].addr & low_mask); + if current == params.new_base { + let old_low = params.old_base as u32; + let old_high = (params.old_base >> 32) as u32; + self.bars[i].addr = old_low; + self.bars[i + 1].addr = old_high; + self.registers[BAR0_REG + i] = + (self.registers[BAR0_REG + i] & !low_mask) | (old_low & low_mask); + self.registers[BAR0_REG + i + 1] = (self.registers[BAR0_REG + i + 1] + & !high_mask) + | (old_high & high_mask); + return; + } + } + } + _ => { + // 32-bit Memory or IO BAR + for i in 0..NUM_BAR_REGS { + let mask = self.writable_bits[BAR0_REG + i]; + if self.bars[i].r#type == Some(params.region_type) + && u64::from(self.bars[i].addr & mask) == params.new_base + { + let old = params.old_base as u32; + self.bars[i].addr = old; + self.registers[BAR0_REG + i] = + (self.registers[BAR0_REG + i] & !mask) | (old & mask); + return; + } + } + } + } + } } impl Pausable for PciConfiguration {} diff --git a/pci/src/device.rs b/pci/src/device.rs index 29c89b8c4..482e15e40 100644 --- a/pci/src/device.rs +++ b/pci/src/device.rs @@ -93,6 +93,10 @@ pub trait PciDevice: Send { fn move_bar(&mut self, _old_base: u64, _new_base: u64) -> result::Result<(), io::Error> { Ok(()) } + /// Restore BAR address in config space after a failed move_bar. + /// This rolls back the address update made by detect_bar_reprogramming() + /// so that the config register stays consistent with the MMIO bus mapping. + fn restore_bar_addr(&mut self, _params: &BarReprogrammingParams) {} /// Provides a mutable reference to the Any trait. This is useful to let /// the caller have access to the underlying type behind the trait. fn as_any_mut(&mut self) -> &mut dyn Any; diff --git a/pci/src/vfio.rs b/pci/src/vfio.rs index b49ed5198..e0c9110a8 100644 --- a/pci/src/vfio.rs +++ b/pci/src/vfio.rs @@ -2034,6 +2034,10 @@ iova 0x{:x}, size 0x{:x}: {}, ", Ok(()) } + fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) { + self.common.configuration.restore_bar_addr(params); + } + fn as_any_mut(&mut self) -> &mut dyn Any { self } diff --git a/pci/src/vfio_user.rs b/pci/src/vfio_user.rs index 456047d42..27c7dc040 100644 --- a/pci/src/vfio_user.rs +++ b/pci/src/vfio_user.rs @@ -414,6 +414,10 @@ impl PciDevice for VfioUserPciDevice { .free_bars(allocator, mmio32_allocator, mmio64_allocator) } + fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) { + self.common.configuration.restore_bar_addr(params); + } + fn as_any_mut(&mut self) -> &mut dyn Any { self } diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 36975e3f7..54a29caa5 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -1133,6 +1133,10 @@ impl PciDevice for VirtioPciDevice { Ok(()) } + fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) { + self.configuration.restore_bar_addr(params); + } + fn read_bar(&mut self, _base: u64, offset: u64, data: &mut [u8]) { match offset { o if o < COMMON_CONFIG_BAR_OFFSET + COMMON_CONFIG_SIZE => self.common_config.read(