From 75b0fe5a215fb9380167dd50bee0455c10139983 Mon Sep 17 00:00:00 2001 From: wuxinyue Date: Thu, 11 Jun 2026 11:51:27 +0800 Subject: [PATCH] vmm: defer PCI device visibility to fix hotplug race condition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split `add_pci_device()` into two phases: `allocate_pci_bars()` which only allocates BAR address space, and `commit_pci_device()` which makes the device visible to the guest on the PCI bus. All callers now follow the pattern: allocate BARs → perform device- specific setup (ioeventfd, device_tree, mmio mapping) → commit device. This eliminates a race window where the guest could discover a partially-initialized device via `acpiphp_check_bridge()` during rapid sequential hotplug, causing BAR reprogramming to fail because ioeventfds and device_tree entries were not yet in place. Signed-off-by: wuxinyue Assisted-by: Claude:Opus-4.6 --- vmm/src/device_manager.rs | 155 ++++++++++++++++++++++---------------- 1 file changed, 92 insertions(+), 63 deletions(-) diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 7bca1ed78..ce8b699c5 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -3653,13 +3653,8 @@ impl DeviceManager { let pvmemcontrol_pci_device = Arc::new(Mutex::new(pvmemcontrol_pci_device)); let pvmemcontrol_bus_device = Arc::new(pvmemcontrol_bus_device); - let new_resources = self.add_pci_device( - pvmemcontrol_bus_device.clone(), - pvmemcontrol_pci_device.clone(), - pci_segment_id, - pci_device_bdf, - resources, - )?; + let (bars, new_resources) = + self.allocate_pci_bars(pvmemcontrol_pci_device.clone(), pci_segment_id, resources)?; let mut node = device_node!(id, pvmemcontrol_pci_device); @@ -3669,6 +3664,14 @@ impl DeviceManager { self.device_tree.lock().unwrap().insert(id, node); + self.commit_pci_device( + pvmemcontrol_bus_device.clone(), + pvmemcontrol_pci_device.clone(), + pci_segment_id, + pci_device_bdf, + bars, + )?; + Ok((pvmemcontrol_bus_device, pvmemcontrol_pci_device)) } @@ -4066,13 +4069,8 @@ impl DeviceManager { let vfio_pci_device = Arc::new(Mutex::new(vfio_pci_device)); - let new_resources = self.add_pci_device( - vfio_pci_device.clone(), - vfio_pci_device.clone(), - pci_segment_id, - pci_device_bdf, - resources, - )?; + let (bars, new_resources) = + self.allocate_pci_bars(vfio_pci_device.clone(), pci_segment_id, resources)?; vfio_pci_device .lock() @@ -4089,7 +4087,7 @@ impl DeviceManager { // Update the device tree with correct resource information. node.resources = new_resources; node.pci_bdf = Some(pci_device_bdf); - node.pci_device_handle = Some(PciDeviceHandle::Vfio(vfio_pci_device)); + node.pci_device_handle = Some(PciDeviceHandle::Vfio(vfio_pci_device.clone())); self.device_tree .lock() @@ -4100,17 +4098,24 @@ impl DeviceManager { self.device_id_to_bdf .insert(vfio_name.clone(), pci_device_bdf); + self.commit_pci_device( + vfio_pci_device.clone(), + vfio_pci_device, + pci_segment_id, + pci_device_bdf, + bars, + )?; + Ok((pci_device_bdf, vfio_name)) } - fn add_pci_device( + #[expect(clippy::needless_pass_by_value)] + fn allocate_pci_bars( &mut self, - bus_device: Arc, pci_device: Arc>, segment_id: u16, - bdf: PciBdf, resources: Option>, - ) -> DeviceManagerResult> { + ) -> DeviceManagerResult<(Vec, Vec)> { let bars = pci_device .lock() .unwrap() @@ -4128,19 +4133,8 @@ impl DeviceManager { ) .map_err(DeviceManagerError::AllocateBars)?; - self.pci_segments[segment_id as usize] - .pci_bus - .lock() - .unwrap() - .add_device(bdf.device(), pci_device) - .map_err(DeviceManagerError::AddPciDevice)?; - - self.bus_devices.push(Arc::clone(&bus_device)); - - self.register_bar_mapping(bus_device, &bars)?; - let mut new_resources = Vec::new(); - for bar in bars { + for bar in &bars { new_resources.push(Resource::PciBar { index: bar.idx(), base: bar.addr(), @@ -4150,7 +4144,30 @@ impl DeviceManager { }); } - Ok(new_resources) + Ok((bars, new_resources)) + } + + #[expect(clippy::needless_pass_by_value)] + fn commit_pci_device( + &mut self, + bus_device: Arc, + pci_device: Arc>, + segment_id: u16, + bdf: PciBdf, + bars: Vec, + ) -> DeviceManagerResult<()> { + self.bus_devices.push(Arc::clone(&bus_device)); + + self.register_bar_mapping(bus_device, &bars)?; + + self.pci_segments[segment_id as usize] + .pci_bus + .lock() + .unwrap() + .add_device(bdf.device(), pci_device) + .map_err(DeviceManagerError::AddPciDevice)?; + + Ok(()) } #[expect(clippy::needless_pass_by_value)] @@ -4276,15 +4293,10 @@ impl DeviceManager { let vfio_user_pci_device = Arc::new(Mutex::new(vfio_user_pci_device)); - let new_resources = self.add_pci_device( - vfio_user_pci_device.clone(), - vfio_user_pci_device.clone(), - pci_segment_id, - pci_device_bdf, - resources, - )?; + let (bars, new_resources) = + self.allocate_pci_bars(vfio_user_pci_device.clone(), pci_segment_id, resources)?; - // Note it is required to call 'add_pci_device()' in advance to have the list of + // Note it is required to call 'allocate_pci_bars()' in advance to have the list of // mmio regions provisioned correctly vfio_user_pci_device .lock() @@ -4297,7 +4309,7 @@ impl DeviceManager { // Update the device tree with correct resource information. node.resources = new_resources; node.pci_bdf = Some(pci_device_bdf); - node.pci_device_handle = Some(PciDeviceHandle::VfioUser(vfio_user_pci_device)); + node.pci_device_handle = Some(PciDeviceHandle::VfioUser(vfio_user_pci_device.clone())); self.device_tree .lock() @@ -4308,6 +4320,14 @@ impl DeviceManager { self.device_id_to_bdf .insert(vfio_user_name.clone(), pci_device_bdf); + self.commit_pci_device( + vfio_user_pci_device.clone(), + vfio_user_pci_device, + pci_segment_id, + pci_device_bdf, + bars, + )?; + Ok((pci_device_bdf, vfio_user_name)) } @@ -4442,13 +4462,8 @@ impl DeviceManager { .map_err(DeviceManagerError::VirtioDevice)?, )); - let new_resources = self.add_pci_device( - virtio_pci_device.clone(), - virtio_pci_device.clone(), - pci_segment_id, - pci_device_bdf, - resources, - )?; + let (bars, new_resources) = + self.allocate_pci_bars(virtio_pci_device.clone(), pci_segment_id, resources)?; let bar_addr = virtio_pci_device.lock().unwrap().config_bar_addr(); for (event, addr) in virtio_pci_device.lock().unwrap().ioeventfds(bar_addr) { @@ -4463,9 +4478,17 @@ impl DeviceManager { node.resources = new_resources; node.migratable = Some(Arc::clone(&virtio_pci_device) as Arc>); node.pci_bdf = Some(pci_device_bdf); - node.pci_device_handle = Some(PciDeviceHandle::Virtio(virtio_pci_device)); + node.pci_device_handle = Some(PciDeviceHandle::Virtio(virtio_pci_device.clone())); self.device_tree.lock().unwrap().insert(id, node); + self.commit_pci_device( + virtio_pci_device.clone(), + virtio_pci_device, + pci_segment_id, + pci_device_bdf, + bars, + )?; + Ok(pci_device_bdf) } @@ -4488,13 +4511,8 @@ impl DeviceManager { let pvpanic_device = Arc::new(Mutex::new(pvpanic_device)); - let new_resources = self.add_pci_device( - pvpanic_device.clone(), - pvpanic_device.clone(), - pci_segment_id, - pci_device_bdf, - resources, - )?; + let (bars, new_resources) = + self.allocate_pci_bars(pvpanic_device.clone(), pci_segment_id, resources)?; let mut node = device_node!(id, pvpanic_device); @@ -4504,6 +4522,14 @@ impl DeviceManager { self.device_tree.lock().unwrap().insert(id, node); + self.commit_pci_device( + pvpanic_device.clone(), + pvpanic_device.clone(), + pci_segment_id, + pci_device_bdf, + bars, + )?; + Ok(Some(pvpanic_device)) } @@ -4543,13 +4569,8 @@ impl DeviceManager { ) .map_err(DeviceManagerError::IvshmemCreate)?, )); - let new_resources = self.add_pci_device( - ivshmem_device.clone(), - ivshmem_device.clone(), - pci_segment_id, - pci_device_bdf, - resources, - )?; + let (bars, new_resources) = + self.allocate_pci_bars(ivshmem_device.clone(), pci_segment_id, resources)?; let start_addr = ivshmem_device.lock().unwrap().data_bar_addr(); let (region, mapping) = ivshmem_ops @@ -4565,6 +4586,14 @@ impl DeviceManager { node.pci_device_handle = None; self.device_tree.lock().unwrap().insert(id, node); + self.commit_pci_device( + ivshmem_device.clone(), + ivshmem_device.clone(), + pci_segment_id, + pci_device_bdf, + bars, + )?; + Ok(Some(ivshmem_device)) }