From 7245ebd353408b3069e70e3b783423143d64f783 Mon Sep 17 00:00:00 2001 From: Xiong Zhang Date: Mon, 11 Apr 2022 14:33:17 +0800 Subject: [PATCH] pcie: hotplug in interrupt for mutli devices When multi pcie endpoint devices are connected to one virtual pcie root port as pcie multi function device, virtual pcie root port couldn't inject hotplug in interrupt for each added pcie device. Only one hotplug in interrupt is needed for all the added pcie devices. So this commit adds a hp_interupt field into VfioCommandRequest, only the last added pcie device sets hp_interrupt to true and requests virtual pcie root port to inject hotplug in interrupt. Ohter added pcie devices just create and configure vfio-pci device. BUG=b:185084350 TEST=Check TBT dock's hotplug in function, TBT dock has multi pcie devices. Change-Id: I5a68ec38af9a32de9d4233b48e0aa3681e5a3ba3 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3604290 Tested-by: kokoro Commit-Queue: Daniel Verkamp Reviewed-by: Daniel Verkamp --- devices/src/pci/pcie/pcie_host.rs | 26 +++++++++++++++++--- devices/src/pci/vfio_pci.rs | 1 + src/linux/mod.rs | 41 +++++++++++++++++++++---------- src/main.rs | 7 +++++- vm_control/src/lib.rs | 7 +++++- 5 files changed, 63 insertions(+), 19 deletions(-) diff --git a/devices/src/pci/pcie/pcie_host.rs b/devices/src/pci/pcie/pcie_host.rs index b4e839d078..222305d19d 100644 --- a/devices/src/pci/pcie/pcie_host.rs +++ b/devices/src/pci/pcie/pcie_host.rs @@ -160,7 +160,24 @@ impl HotplugWorker { let mut children: Vec = Vec::new(); visit_children(host_sysfs.as_path(), &mut children)?; - for child in &children { + // Without reverse children, physical larger BDF device is at the top, it will be + // added into guest first with smaller virtual function number, so physical smaller + // BDF device has larger virtual function number, phyiscal larger BDF device has + // smaller virtual function number. During hotplug out process, host pcie root port + // driver remove physical smaller BDF pcie endpoint device first, so host vfio-pci + // driver send plug out event first for smaller BDF device and wait for this device + // removed from crosvm, when crosvm receives this plug out event, crosvm will remove + // all the children devices, crosvm remove smaller virtual function number device + // first, this isn't the target device which host vfio-pci driver is waiting for. + // Host vfio-pci driver holds a lock when it is waiting, when crosvm remove another + // device throgh vfio-pci which try to get the same lock, so deadlock happens in + // host kernel. + // + // In order to fix the deadlock, children is reversed, so physical smaller BDF + // device has smaller virtual function number, and it will have the same order + // between host kernel and crosvm during hotplug out process. + children.reverse(); + while let Some(child) = children.pop() { // In order to bind device to vfio-pci driver, get device VID and DID let vendor_path = child.join("vendor"); let vendor_id = read(vendor_path.as_path()) @@ -199,6 +216,7 @@ impl HotplugWorker { let request = VmRequest::VfioCommand { vfio_path: child.clone(), add: true, + hp_interrupt: children.is_empty(), }; vm_socket.lock().send(&request).with_context(|| { format!("failed to send hotplug request for {}", child.display()) @@ -206,10 +224,10 @@ impl HotplugWorker { vm_socket.lock().recv::().with_context(|| { format!("failed to receive hotplug response for {}", child.display()) })?; - } - if !children.is_empty() { - *child_exist = true; + if !*child_exist { + *child_exist = true; + } } Ok(()) diff --git a/devices/src/pci/vfio_pci.rs b/devices/src/pci/vfio_pci.rs index 51d5c0f515..c1ed4415ad 100644 --- a/devices/src/pci/vfio_pci.rs +++ b/devices/src/pci/vfio_pci.rs @@ -629,6 +629,7 @@ impl VfioPciWorker { let request = VmRequest::VfioCommand { vfio_path: sysfs_path, add: false, + hp_interrupt: true, }; if self.vm_socket.send(&request).is_ok() { if let Err(e) = self.vm_socket.recv::() { diff --git a/src/linux/mod.rs b/src/linux/mod.rs index a39caec26f..a243ed649f 100644 --- a/src/linux/mod.rs +++ b/src/linux/mod.rs @@ -1514,6 +1514,7 @@ fn add_vfio_device( control_tubes: &mut Vec, iommu_host_tube: &Option, vfio_path: &Path, + hp_interrupt: bool, ) -> Result<()> { let host_os_str = vfio_path .file_name() @@ -1572,7 +1573,9 @@ fn add_vfio_device( let host_key = HostHotPlugKey::Vfio { host_addr }; let mut hp_bus = hp_bus.lock(); hp_bus.add_hotplug_device(host_key, pci_address); - hp_bus.hot_plug(pci_address); + if hp_interrupt { + hp_bus.hot_plug(pci_address); + } Ok(()) } @@ -1581,6 +1584,7 @@ fn remove_vfio_device( sys_allocator: &mut SystemAllocator, iommu_host_tube: &Option, vfio_path: &Path, + _hp_interrupt: bool, ) -> Result<()> { let host_os_str = vfio_path .file_name() @@ -1623,6 +1627,7 @@ fn handle_vfio_command( iommu_host_tube: &Option, vfio_path: &Path, add: bool, + hp_interrupt: bool, ) -> VmResponse { let ret = if add { add_vfio_device( @@ -1632,9 +1637,16 @@ fn handle_vfio_command( add_tubes, iommu_host_tube, vfio_path, + hp_interrupt, ) } else { - remove_vfio_device(linux, sys_allocator, iommu_host_tube, vfio_path) + remove_vfio_device( + linux, + sys_allocator, + iommu_host_tube, + vfio_path, + hp_interrupt, + ) }; match ret { @@ -1952,17 +1964,20 @@ fn run_control( Ok(request) => { let mut run_mode_opt = None; let response = match request { - VmRequest::VfioCommand { vfio_path, add } => { - handle_vfio_command( - &mut linux, - &mut sys_allocator, - &cfg, - &mut add_tubes, - &iommu_host_tube, - &vfio_path, - add, - ) - } + VmRequest::VfioCommand { + vfio_path, + add, + hp_interrupt, + } => handle_vfio_command( + &mut linux, + &mut sys_allocator, + &cfg, + &mut add_tubes, + &iommu_host_tube, + &vfio_path, + add, + hp_interrupt, + ), _ => request.execute( &mut run_mode_opt, balloon_host_tube.as_ref(), diff --git a/src/main.rs b/src/main.rs index b898e02de3..5eaa775f40 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3273,7 +3273,12 @@ fn modify_vfio(mut args: std::env::Args) -> std::result::Result<(), ()> { } }; - let request = VmRequest::VfioCommand { vfio_path, add }; + let hp_interrupt = true; + let request = VmRequest::VfioCommand { + vfio_path, + add, + hp_interrupt, + }; handle_request(&request, socket_path)?; Ok(()) } diff --git a/vm_control/src/lib.rs b/vm_control/src/lib.rs index 3b50f43b39..1955ab8e78 100644 --- a/vm_control/src/lib.rs +++ b/vm_control/src/lib.rs @@ -991,7 +991,11 @@ pub enum VmRequest { /// Command to set battery. BatCommand(BatteryType, BatControlCommand), /// Command to add/remove vfio pci device - VfioCommand { vfio_path: PathBuf, add: bool }, + VfioCommand { + vfio_path: PathBuf, + add: bool, + hp_interrupt: bool, + }, } fn map_descriptor( @@ -1218,6 +1222,7 @@ impl VmRequest { VmRequest::VfioCommand { vfio_path: _, add: _, + hp_interrupt: _, } => VmResponse::Ok, } }