From 6087a79074264e01e01362be48d303826dbb2e62 Mon Sep 17 00:00:00 2001 From: Dapeng Mi Date: Wed, 24 Nov 2021 11:17:36 +0000 Subject: [PATCH] Devices: vfio: Fix the PCIe BAR region mmap issue Intel SSD 660P Series NVME block device has an overlapping msix table and msix pba table as the following information shows. ab:00.0 Non-Volatile memory controller: Intel Corporation SSD 660P Series (rev 03) Capabilities: [b0] MSI-X: Enable- Count=22 Masked- Vector table: BAR=0 offset=00002000 PBA: BAR=0 offset=00002100 In curruent add_bar_mmap_msix function, these two table would occupy the same page. So when add_bar_mmap_msix trys to find the msix table corresponding pages for the second time, it fails to find the page and lead to the entire mmaps are cleared and return null mmaps. Thus, there is no real memory region are mmaped and lead to quite low I/O performance for this pass-through NVME disk. BUG=None TEST=Unit test TEST=pass-through the 660P SSD into guest and run FIO read/write test, the throught increases to ~500MBps form ~20MBps. Change-Id: I1571e694b0a1f01a738650b361eaef93554a8c55 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3213315 Reviewed-by: Daniel Verkamp Tested-by: kokoro Commit-Queue: Daniel Verkamp --- devices/src/pci/vfio_pci.rs | 100 ++++++++++++++++++++++++++++++------ 1 file changed, 84 insertions(+), 16 deletions(-) diff --git a/devices/src/pci/vfio_pci.rs b/devices/src/pci/vfio_pci.rs index f4b1253175..d1510b4fbc 100644 --- a/devices/src/pci/vfio_pci.rs +++ b/devices/src/pci/vfio_pci.rs @@ -410,7 +410,7 @@ impl VfioMsixAllocator { } // Allocates a range of addresses from the managed region with a required location. - // Returns OutOfSpace if requested range is not available (e.g. already allocated). + // Returns a new range of addresses excluding the required range. fn allocate_at(&mut self, start: u64, size: u64) -> Result<(), PciDeviceError> { if size == 0 { return Err(PciDeviceError::MsixAllocatorSizeZero); @@ -418,24 +418,21 @@ impl VfioMsixAllocator { let end = start .checked_add(size - 1) .ok_or(PciDeviceError::MsixAllocatorOutOfSpace)?; - match self + while let Some(slot) = self .regions .iter() - .find(|range| range.0 <= start && range.1 >= end) + .find(|range| (start <= range.1 && end >= range.0)) .cloned() { - Some(slot) => { - self.regions.remove(&slot); - if slot.0 < start { - self.regions.insert((slot.0, start - 1)); - } - if slot.1 > end { - self.regions.insert((end + 1, slot.1)); - } - Ok(()) + self.regions.remove(&slot); + if slot.0 < start { + self.regions.insert((slot.0, start - 1)); + } + if slot.1 > end { + self.regions.insert((end + 1, slot.1)); } - None => Err(PciDeviceError::MsixAllocatorOutOfSpace), } + Ok(()) } } @@ -720,9 +717,7 @@ impl VfioPciDevice { (min(msix_offset + msix_size, mmap_offset + mmap_size) + pgmask) & !pgmask; if end > begin { if let Err(e) = to_mmap.allocate_at(begin, end - begin) { - error!("{} add_bar_mmap_msix failed: {}", self.debug_label(), e); - mmaps.clear(); - return mmaps; + error!("add_bar_mmap_msix failed: {}", e); } } } @@ -1267,3 +1262,76 @@ impl PciDevice for VfioPciDevice { self.close(); } } + +#[cfg(test)] +mod tests { + use super::VfioMsixAllocator; + + #[test] + fn no_overlap() { + // regions [32, 95] + let mut memory = VfioMsixAllocator::new(32, 64).unwrap(); + memory.allocate_at(0, 16).unwrap(); + memory.allocate_at(100, 16).unwrap(); + + let mut iter = memory.regions.iter(); + assert_eq!(iter.next(), Some(&(32, 95))); + } + + #[test] + fn full_overlap() { + // regions [32, 95] + let mut memory = VfioMsixAllocator::new(32, 64).unwrap(); + // regions [32, 47], [64, 95] + memory.allocate_at(48, 16).unwrap(); + // regions [64, 95] + memory.allocate_at(32, 16).unwrap(); + + let mut iter = memory.regions.iter(); + assert_eq!(iter.next(), Some(&(64, 95))); + } + + #[test] + fn partial_overlap_one() { + // regions [32, 95] + let mut memory = VfioMsixAllocator::new(32, 64).unwrap(); + // regions [32, 47], [64, 95] + memory.allocate_at(48, 16).unwrap(); + // regions [32, 39], [64, 95] + memory.allocate_at(40, 16).unwrap(); + + let mut iter = memory.regions.iter(); + assert_eq!(iter.next(), Some(&(32, 39))); + assert_eq!(iter.next(), Some(&(64, 95))); + } + + #[test] + fn partial_overlap_two() { + // regions [32, 95] + let mut memory = VfioMsixAllocator::new(32, 64).unwrap(); + // regions [32, 47], [64, 95] + memory.allocate_at(48, 16).unwrap(); + // regions [32, 39], [72, 95] + memory.allocate_at(40, 32).unwrap(); + + let mut iter = memory.regions.iter(); + assert_eq!(iter.next(), Some(&(32, 39))); + assert_eq!(iter.next(), Some(&(72, 95))); + } + + #[test] + fn partial_overlap_three() { + // regions [32, 95] + let mut memory = VfioMsixAllocator::new(32, 64).unwrap(); + // regions [32, 39], [48, 95] + memory.allocate_at(40, 8).unwrap(); + // regions [32, 39], [48, 63], [72, 95] + memory.allocate_at(64, 8).unwrap(); + // regions [32, 35], [76, 95] + memory.allocate_at(36, 40).unwrap(); + + let mut iter = memory.regions.iter(); + assert_eq!(iter.next(), Some(&(32, 35))); + assert_eq!(iter.next(), Some(&(76, 95))); + } +}