From aaf86ef20955840b83981a739b092e23b5665afe Mon Sep 17 00:00:00 2001 From: Bo Chen Date: Mon, 12 May 2025 22:15:56 +0000 Subject: [PATCH] pci: Reprogram device BAR when its MSE bit is set The Memory Space Enable (MSE) bit from the COMMAND register in the PCI configuration space controls whether a PCI device responds to memory space accesses, e.g. read and write cycles to the device MMIO regions defined by its BARs. The MSE bit is used by the device drivers to ensure the correctness of BAR reprogramming. A common workflow is, the driver first clears the MSE bit, then writes new values to the BAR registers, and finally set the MSE bit to finish the BAR reprogramming. This patch changes how we handle BAR reprogramming for all PCI devices (e.g. virtio-pci, vfio, vfio-user, etc.), so that we follow the same convention, e.g. moving PCI BARs when its MSE bit is set. Note that some device drivers (such as edk2) only clear and set MSE once while reprogramming multiple BARs of a single device. To support such behavior, this patch adds support for multiple pending BAR reprogramming. See: https://github.com/cloud-hypervisor/cloud-hypervisor/issues/7027#issuecomment-2853642959 Signed-off-by: Bo Chen --- devices/src/pvmemcontrol.rs | 2 +- devices/src/pvpanic.rs | 4 +-- pci/src/bus.rs | 6 ++-- pci/src/configuration.rs | 32 ++++++++++++++++++++-- pci/src/device.rs | 4 +-- pci/src/vfio.rs | 6 ++-- pci/src/vfio_user.rs | 2 +- virtio-devices/src/transport/pci_device.rs | 4 +-- 8 files changed, 43 insertions(+), 17 deletions(-) diff --git a/devices/src/pvmemcontrol.rs b/devices/src/pvmemcontrol.rs index 832d83b08..2bb52a9cf 100644 --- a/devices/src/pvmemcontrol.rs +++ b/devices/src/pvmemcontrol.rs @@ -698,7 +698,7 @@ impl PciDevice for PvmemcontrolPciDevice { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>) { + ) -> (Vec, Option>) { ( self.configuration .write_config_register(reg_idx, offset, data), diff --git a/devices/src/pvpanic.rs b/devices/src/pvpanic.rs index dd7cb8329..9300b3041 100644 --- a/devices/src/pvpanic.rs +++ b/devices/src/pvpanic.rs @@ -90,7 +90,7 @@ impl PvPanicDevice { let command: [u8; 2] = [0x03, 0x01]; let bar_reprogram = configuration.write_config_register(1, 0, &command); assert!( - bar_reprogram.is_none(), + bar_reprogram.is_empty(), "No bar reprogrammig is expected from writing to the COMMAND register" ); @@ -160,7 +160,7 @@ impl PciDevice for PvPanicDevice { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>) { + ) -> (Vec, Option>) { ( self.configuration .write_config_register(reg_idx, offset, data), diff --git a/pci/src/bus.rs b/pci/src/bus.rs index 04284b435..e5fe38f22 100644 --- a/pci/src/bus.rs +++ b/pci/src/bus.rs @@ -81,7 +81,7 @@ impl PciDevice for PciRoot { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>) { + ) -> (Vec, Option>) { ( self.config.write_config_register(reg_idx, offset, data), None, @@ -262,7 +262,7 @@ impl PciConfigIo { let (bar_reprogram, ret) = device.write_config_register(register, offset, data); // Move the device's BAR if needed - if let Some(params) = bar_reprogram { + for params in &bar_reprogram { if let Err(e) = pci_bus.device_reloc.move_bar( params.old_base, params.new_base, @@ -387,7 +387,7 @@ impl PciConfigMmio { let (bar_reprogram, _) = device.write_config_register(register, offset, data); // Move the device's BAR if needed - if let Some(params) = bar_reprogram { + for params in &bar_reprogram { if let Err(e) = pci_bus.device_reloc.move_bar( params.old_base, params.new_base, diff --git a/pci/src/configuration.rs b/pci/src/configuration.rs index e10afb47a..6d7d33efe 100644 --- a/pci/src/configuration.rs +++ b/pci/src/configuration.rs @@ -18,6 +18,8 @@ use crate::{MsixConfig, PciInterruptPin}; // The number of 32bit registers in the config space, 4096 bytes. const NUM_CONFIGURATION_REGISTERS: usize = 1024; +const COMMAND_REG: usize = 1; +const COMMAND_REG_MEMORY_SPACE_MASK: u32 = 0x0000_0002; const STATUS_REG: usize = 1; const STATUS_REG_CAPABILITIES_USED_MASK: u32 = 0x0010_0000; const BAR0_REG: usize = 4; @@ -436,6 +438,7 @@ pub struct PciConfiguration { last_capability: Option<(usize, usize)>, msix_cap_reg_idx: Option, msix_config: Option>>, + pending_bar_reprogram: Vec, } /// See pci_regs.h in kernel @@ -630,6 +633,7 @@ impl PciConfiguration { last_capability, msix_cap_reg_idx, msix_config, + pending_bar_reprogram: Vec::new(), } } @@ -912,9 +916,9 @@ impl PciConfiguration { reg_idx: usize, offset: u64, data: &[u8], - ) -> Option { + ) -> Vec { if offset as usize + data.len() > 4 { - return None; + return Vec::new(); } // Handle potential write to MSI-X message control register @@ -944,7 +948,29 @@ impl PciConfiguration { _ => (), } - self.detect_bar_reprogramming(reg_idx, data) + if let Some(param) = self.detect_bar_reprogramming(reg_idx, data) { + self.pending_bar_reprogram.push(param); + } + + if !self.pending_bar_reprogram.is_empty() { + // Return bar reprogramming only if the MSE bit is enabled; + if self.read_config_register(COMMAND_REG) & COMMAND_REG_MEMORY_SPACE_MASK + == COMMAND_REG_MEMORY_SPACE_MASK + { + info!( + "BAR reprogramming parameter is returned: {:x?}", + self.pending_bar_reprogram + ); + return self.pending_bar_reprogram.drain(..).collect(); + } else { + info!( + "MSE bit is disabled. No BAR reprogramming parameter is returned: {:x?}", + self.pending_bar_reprogram + ); + } + } + + Vec::new() } pub fn read_config_register(&self, reg_idx: usize) -> u32 { diff --git a/pci/src/device.rs b/pci/src/device.rs index 1e834ab0e..0bc84208d 100644 --- a/pci/src/device.rs +++ b/pci/src/device.rs @@ -48,7 +48,7 @@ impl Display for Error { } } -#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug)] pub struct BarReprogrammingParams { pub old_base: u64, pub new_base: u64, @@ -87,7 +87,7 @@ pub trait PciDevice: Send { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>); + ) -> (Vec, Option>); /// Gets a register from the configuration space. /// * `reg_idx` - The index of the config register to read. fn read_config_register(&mut self, reg_idx: usize) -> u32; diff --git a/pci/src/vfio.rs b/pci/src/vfio.rs index 41965e445..671efef56 100644 --- a/pci/src/vfio.rs +++ b/pci/src/vfio.rs @@ -1242,7 +1242,7 @@ impl VfioCommon { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>) { + ) -> (Vec, Option>) { // When the guest wants to write to a BAR, we trap it into // our local configuration space. We're not reprogramming // VFIO device. @@ -1292,7 +1292,7 @@ impl VfioCommon { // to the device region to update the MSI Enable bit. self.vfio_wrapper.write_config((reg + offset) as u32, data); - (None, None) + (Vec::new(), None) } pub(crate) fn read_config_register(&mut self, reg_idx: usize) -> u32 { @@ -1852,7 +1852,7 @@ impl PciDevice for VfioPciDevice { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>) { + ) -> (Vec, Option>) { self.common.write_config_register(reg_idx, offset, data) } diff --git a/pci/src/vfio_user.rs b/pci/src/vfio_user.rs index 017864883..3762b2c75 100644 --- a/pci/src/vfio_user.rs +++ b/pci/src/vfio_user.rs @@ -431,7 +431,7 @@ impl PciDevice for VfioUserPciDevice { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>) { + ) -> (Vec, Option>) { self.common.write_config_register(reg_idx, offset, data) } diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 06a21c9fe..c09efacf4 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -912,7 +912,7 @@ impl PciDevice for VirtioPciDevice { reg_idx: usize, offset: u64, data: &[u8], - ) -> (Option, Option>) { + ) -> (Vec, Option>) { // Handle the special case where the capability VIRTIO_PCI_CAP_PCI_CFG // is accessed. This capability has a special meaning as it allows the // guest to access other capabilities without mapping the PCI BAR. @@ -922,7 +922,7 @@ impl PciDevice for VirtioPciDevice { <= self.cap_pci_cfg_info.offset + self.cap_pci_cfg_info.cap.bytes().len() { let offset = base + offset as usize - self.cap_pci_cfg_info.offset; - (None, self.write_cap_pci_cfg(offset, data)) + (Vec::new(), self.write_cap_pci_cfg(offset, data)) } else { ( self.configuration