From 1ba2b340195e351eda0207a8a46f134cfcdf8d23 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Thu, 7 May 2026 14:21:32 +0200 Subject: [PATCH] vm-allocator: free GSIs in GsiAllocator The old implementation used an ever monotonically increasing u32 counter to allocate new GSIs. The counter increased every time a new GSI was allocated, and freeing GSIs was not possible. Thus, Cloud Hypervisor can run out of GSIs and panics. This currently happened at the 1024th GSI [0]. Further, this caused the `KVM_SET_GSI_ROUTING` ioctl to carry much more payload than needed. This new implementation uses a bitmap for proper tracking of resources and can gracefully free GSIs - this is abstracted in type InterruptAllocator. Please note that this commit only replaces the old mechanism. The next commit will introduce freeing used GSIs automatically when an InterruptRoute is dropped. While being on this, we also propagate the errors that the allocator may throw where necessary. Co-authored-by: Sebastian Eydam On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam On-behalf-of: Philipp Schuster@sap.com Signed-off-by: Philipp Schuster --- vm-allocator/src/gsi.rs | 248 +++++++++++++++++++++++++++++-------- vm-allocator/src/lib.rs | 2 +- vm-allocator/src/system.rs | 22 ++-- vmm/src/device_manager.rs | 4 +- vmm/src/interrupt.rs | 2 +- vmm/src/pci_segment.rs | 2 +- 6 files changed, 215 insertions(+), 65 deletions(-) diff --git a/vm-allocator/src/gsi.rs b/vm-allocator/src/gsi.rs index 30e20e7e7..800913917 100644 --- a/vm-allocator/src/gsi.rs +++ b/vm-allocator/src/gsi.rs @@ -2,27 +2,28 @@ // // SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause -// Only for this commit -#![expect(unused)] +//! Interrupt-number allocation for interrupts. +//! +//! See [`GsiAllocator`]. #[cfg(target_arch = "x86_64")] use std::collections::btree_map::BTreeMap; +#[cfg(test)] +use std::ops::Range; use std::result; use thiserror::Error; -#[derive(Debug)] -pub enum Error { - Overflow, -} +pub type Result = result::Result; -pub type Result = result::Result; - -/// GsiApic +/// Describes one APIC interrupt input range in the global system interrupt +/// namespace. #[cfg(target_arch = "x86_64")] #[derive(Copy, Clone)] pub struct GsiApic { + /// The offset from 0. base: u32, + /// The number of interrupts in the range. irqs: u32, } @@ -54,6 +55,39 @@ pub enum InterruptAllocError { ), } +/// Maximum number of IRQ routes supported by KVM. +/// +/// See . +const KVM_MAX_IRQ_ROUTES: u32 = { + #[cfg(feature = "kvm")] + { + 4096 + } + #[cfg(not(feature = "kvm"))] + { + 0 + } +}; + +/// Maximum number of IRQ routes supported by MSHV. +/// +/// See . +const MSHV_MAX_GUEST_IRQS: u32 = 4096; + +/// The effective max number of IRQs. +/// +/// This affects the number of interrupts that can be allocated. This number +/// alone doesn't mean that the backend necessarily accepts all the IRQs. +#[allow(clippy::absurd_extreme_comparisons)] +const MAX_GUEST_IRQS: u32 = { + // cmp::max is not const compatible + if KVM_MAX_IRQ_ROUTES > MSHV_MAX_GUEST_IRQS { + KVM_MAX_IRQ_ROUTES + } else { + MSHV_MAX_GUEST_IRQS + } +}; + /// Simple bitmap-backed interrupt allocator. /// /// The allocator can be configured with an offset. For example, to allocate @@ -110,7 +144,7 @@ impl InterruptAllocator { /// Allocates a vector by setting its bit in the bitmap. /// /// Returns an error if the allocator is exhausted. - fn alloc(&mut self) -> result::Result { + fn alloc(&mut self) -> Result { // Find the next word with capacity for allocating a vector. let Some(idx) = self.words.iter().position(|&w| w != usize::MAX) else { return Err(InterruptAllocError::ExhaustedError(self.size)); @@ -134,7 +168,7 @@ impl InterruptAllocator { /// This vector is assumed to include the internal `offset`. /// /// Returns an error if the vector is already free. - fn free(&mut self, vector: u32) -> result::Result<(), InterruptAllocError> { + fn free(&mut self, vector: u32) -> Result<()> { // At first we make sure that the vector is not out of range. let begin = self.offset; let end = begin + self.size; @@ -164,82 +198,96 @@ impl InterruptAllocator { fn size(&self) -> u32 { self.size } + + #[cfg(test)] + fn range(&self) -> Range { + self.offset..(self.offset + self.size) + } } -/// GsiAllocator +/// Coordinates graceful resource allocation of IRQs and GSIs from the interrupt +/// namespace. +/// +/// Ensures that interrupt numbers either for IRQs or GSIs are not overlapping. +/// +/// Check out the [module documentation](super::gsi) for more info. pub struct GsiAllocator { #[cfg(target_arch = "x86_64")] - apics: BTreeMap, - next_irq: u32, - next_gsi: u32, + apics: BTreeMap, + irqs: InterruptAllocator, + gsis: InterruptAllocator, } impl GsiAllocator { #[cfg(target_arch = "x86_64")] - /// New GSI allocator + /// Creates a new GSI allocator with the proper interrupt number ranges + /// for IRQs and GSIs. + /// + /// Respects the provided [`GsiApic`]s + // On x86, the interrupt number space starts with IRQs and is followed by + // GSI. pub fn new(apics: &[GsiApic]) -> Self { - let mut allocator = GsiAllocator { - apics: BTreeMap::new(), - next_irq: 0xffff_ffff, - next_gsi: 0, - }; + let next_irq = apics.iter().map(|apic| apic.base).min().unwrap_or(0); - for apic in apics { - if apic.base < allocator.next_irq { - allocator.next_irq = apic.base; - } + let next_gsi = apics + .iter() + .map(|apic| apic.base + apic.irqs) + .max() + .unwrap_or(0); - if apic.base + apic.irqs > allocator.next_gsi { - allocator.next_gsi = apic.base + apic.irqs; - } + let irqs = apics.iter().map(|apic| apic.irqs).sum(); - allocator.apics.insert(apic.base, apic.irqs); + let allocator_apics = apics.iter().map(|apic| (apic.base, apic.irqs)).collect(); + + let gsis = MAX_GUEST_IRQS - next_gsi; + + Self { + apics: allocator_apics, + irqs: InterruptAllocator::new(irqs, next_irq), + gsis: InterruptAllocator::new(gsis, next_gsi), } - - allocator } #[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))] - /// New GSI allocator + /// New GSI allocator. + // On aarch 64 and riscv x86, the IRQs and GSIs use independent interrupt + // number namespaces. pub fn new() -> Self { GsiAllocator { - next_irq: arch::IRQ_BASE, - next_gsi: arch::IRQ_BASE, + irqs: InterruptAllocator::new(MAX_GUEST_IRQS - arch::IRQ_BASE, arch::IRQ_BASE), + gsis: InterruptAllocator::new(MAX_GUEST_IRQS - arch::IRQ_BASE, arch::IRQ_BASE), } } /// Allocate a GSI pub fn allocate_gsi(&mut self) -> Result { - let gsi = self.next_gsi; - self.next_gsi = self.next_gsi.checked_add(1).ok_or(Error::Overflow)?; - Ok(gsi) + self.gsis.alloc() + } + + /// Frees a GSI + pub fn free_gsi(&mut self, vector: u32) -> Result<()> { + self.gsis.free(vector) } #[cfg(target_arch = "x86_64")] /// Allocate an IRQ pub fn allocate_irq(&mut self) -> Result { - let mut irq: u32 = 0; + let next_irq = self.irqs.alloc()?; for (base, irqs) in self.apics.iter() { // HACKHACK - This only works with 1 single IOAPIC... - if self.next_irq >= *base && self.next_irq < *base + *irqs { - irq = self.next_irq; - self.next_irq += 1; + if next_irq >= *base && next_irq < *base + *irqs { + return Ok(next_irq); } } - if irq == 0 { - return Err(Error::Overflow); - } - - Ok(irq) + self.irqs.free(next_irq)?; + Err(InterruptAllocError::ExhaustedError(self.irqs.size())) } #[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))] /// Allocate an IRQ pub fn allocate_irq(&mut self) -> Result { - let irq = self.next_irq; - self.next_irq = self.next_irq.checked_add(1).ok_or(Error::Overflow)?; - Ok(irq) + self.irqs.alloc() } } @@ -355,4 +403,106 @@ mod unit_tests { } } } + + #[cfg(target_arch = "x86_64")] + /// [`GsiAllocator`] tests for x86, where IRQs and GSIs are consecutive + /// in a single interrupt number namespace. + mod gsi_allocator { + use super::*; + + fn single_apic_allocator() -> GsiAllocator { + // One IOAPIC: GSI 0..24 are pin-based IRQs, GSIs start after that. + GsiAllocator::new(&[GsiApic::new(5, 19)]) + } + + #[test] + fn test_allocator_uses_apic_irq_and_gsi_ranges() { + let mut allocator = single_apic_allocator(); + + assert_eq!(allocator.irqs.range(), 5..24); + assert_eq!(allocator.gsis.range(), 24..MAX_GUEST_IRQS); + assert_eq!(allocator.allocate_irq(), Ok(5)); + assert_eq!(allocator.allocate_irq(), Ok(6)); + assert_eq!(allocator.allocate_gsi(), Ok(24)); + assert_eq!(allocator.allocate_gsi(), Ok(25)); + } + + #[test] + fn test_allocator_exhausts_irqs_at_apic_boundary() { + let mut allocator = single_apic_allocator(); + + for expected_irq in 5..24 { + assert_eq!(allocator.allocate_irq(), Ok(expected_irq)); + } + + assert_eq!( + allocator.allocate_irq(), + Err(InterruptAllocError::ExhaustedError(19)) + ); + assert_eq!(allocator.allocate_gsi(), Ok(24)); + } + + #[test] + fn test_allocator_can_free_and_reuse_gsis() { + let mut allocator = single_apic_allocator(); + + assert_eq!( + allocator.free_gsi(24), + Err(InterruptAllocError::AlreadyFree(24)) + ); + + let gsi = allocator.allocate_gsi().unwrap(); + assert_eq!(gsi, 24); + + allocator.free_gsi(gsi).unwrap(); + assert_eq!(allocator.allocate_gsi(), Ok(gsi)); + } + } + + /// [`GsiAllocator`] tests for aarch64 and RISC-V, where IRQs and GSIs + /// have independent interrupt number namespaces. + #[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))] + mod gsi_allocator { + use super::*; + + fn single_apic_allocator() -> GsiAllocator { + GsiAllocator::new() + } + + #[test] + fn test_allocator_uses_arch_irq_base() { + let mut allocator = single_apic_allocator(); + + assert_eq!(allocator.irqs.range(), ::arch::IRQ_BASE..MAX_GUEST_IRQS); + assert_eq!(allocator.gsis.range(), ::arch::IRQ_BASE..MAX_GUEST_IRQS); + assert_eq!(allocator.allocate_irq(), Ok(::arch::IRQ_BASE)); + assert_eq!(allocator.allocate_irq(), Ok(::arch::IRQ_BASE + 1)); + assert_eq!(allocator.allocate_gsi(), Ok(::arch::IRQ_BASE)); + assert_eq!(allocator.allocate_gsi(), Ok(::arch::IRQ_BASE + 1)); + } + + #[test] + fn test_allocator_keeps_irq_and_gsi_namespaces_independent() { + let mut allocator = single_apic_allocator(); + + assert_eq!(allocator.allocate_irq(), Ok(::arch::IRQ_BASE)); + assert_eq!(allocator.allocate_gsi(), Ok(::arch::IRQ_BASE)); + } + + #[test] + fn test_allocator_can_free_and_reuse_gsis() { + let mut allocator = single_apic_allocator(); + + assert_eq!( + allocator.free_gsi(::arch::IRQ_BASE), + Err(InterruptAllocError::AlreadyFree(::arch::IRQ_BASE)) + ); + + let gsi = allocator.allocate_gsi().unwrap(); + assert_eq!(gsi, ::arch::IRQ_BASE); + + allocator.free_gsi(gsi).unwrap(); + assert_eq!(allocator.allocate_gsi(), Ok(gsi)); + } + } } diff --git a/vm-allocator/src/lib.rs b/vm-allocator/src/lib.rs index d56048288..ecc59ec16 100644 --- a/vm-allocator/src/lib.rs +++ b/vm-allocator/src/lib.rs @@ -17,9 +17,9 @@ pub mod page_size; mod system; pub use crate::address::AddressAllocator; -pub use crate::gsi::GsiAllocator; #[cfg(target_arch = "x86_64")] pub use crate::gsi::GsiApic; +pub use crate::gsi::{GsiAllocator, InterruptAllocError}; pub use crate::system::SystemAllocator; mod memory_slot; pub use memory_slot::MemorySlotAllocator; diff --git a/vm-allocator/src/system.rs b/vm-allocator/src/system.rs index 02ea86c6a..0956e16ae 100644 --- a/vm-allocator/src/system.rs +++ b/vm-allocator/src/system.rs @@ -10,9 +10,9 @@ use vm_memory::{GuestAddress, GuestUsize}; use crate::address::AddressAllocator; -use crate::gsi::GsiAllocator; #[cfg(target_arch = "x86_64")] use crate::gsi::GsiApic; +use crate::gsi::{GsiAllocator, InterruptAllocError}; use crate::page_size::get_page_size; /// Manages allocating system resources such as address space and interrupt numbers. @@ -31,17 +31,17 @@ use crate::page_size::get_page_size; /// GuestAddress(0x10000000), 0x10000000, /// #[cfg(target_arch = "x86_64")] &[GsiApic::new(5, 19)]).unwrap(); /// #[cfg(target_arch = "x86_64")] -/// assert_eq!(allocator.allocate_irq(), Some(5)); +/// assert_eq!(allocator.allocate_irq(), Ok(5)); /// #[cfg(target_arch = "aarch64")] -/// assert_eq!(allocator.allocate_irq(), Some(32)); +/// assert_eq!(allocator.allocate_irq(), Ok(32)); /// #[cfg(target_arch = "riscv64")] -/// assert_eq!(allocator.allocate_irq(), Some(0)); +/// assert_eq!(allocator.allocate_irq(), Ok(0)); /// #[cfg(target_arch = "x86_64")] -/// assert_eq!(allocator.allocate_irq(), Some(6)); +/// assert_eq!(allocator.allocate_irq(), Ok(6)); /// #[cfg(target_arch = "aarch64")] -/// assert_eq!(allocator.allocate_irq(), Some(33)); +/// assert_eq!(allocator.allocate_irq(), Ok(33)); /// #[cfg(target_arch = "riscv64")] -/// assert_eq!(allocator.allocate_irq(), Some(1)); +/// assert_eq!(allocator.allocate_irq(), Ok(1)); /// assert_eq!(allocator.allocate_platform_mmio_addresses(None, 0x1000, Some(0x1000)), Some(GuestAddress(0x1fff_f000))); /// /// ``` @@ -82,13 +82,13 @@ impl SystemAllocator { } /// Reserves the next available system irq number. - pub fn allocate_irq(&mut self) -> Option { - self.gsi_allocator.allocate_irq().ok() + pub fn allocate_irq(&mut self) -> Result { + self.gsi_allocator.allocate_irq() } /// Reserves the next available GSI. - pub fn allocate_gsi(&mut self) -> Option { - self.gsi_allocator.allocate_gsi().ok() + pub fn allocate_gsi(&mut self) -> Result { + self.gsi_allocator.allocate_gsi() } /// Reserves a section of `size` bytes of IO address space. diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 04197107d..2d145fa99 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -91,7 +91,7 @@ use virtio_devices::{ AccessPlatformMapping, ActivateError, Block, Endpoint, IommuMapping, VdpaDmaMapping, VirtioMemMappingSource, }; -use vm_allocator::{AddressAllocator, SystemAllocator}; +use vm_allocator::{AddressAllocator, InterruptAllocError, SystemAllocator}; use vm_device::dma_mapping::ExternalDmaMapping; use vm_device::interrupt::{ InterruptIndex, InterruptManager, LegacyIrqGroupConfig, MsiIrqGroupConfig, @@ -272,7 +272,7 @@ pub enum DeviceManagerError { /// Cannot allocate IRQ. #[error("Cannot allocate IRQ")] - AllocateIrq, + AllocateIrq(#[from] InterruptAllocError), /// Cannot configure the IRQ. #[error("Cannot configure the IRQ")] diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs index f08aaab7f..e5e4b8c8e 100644 --- a/vmm/src/interrupt.rs +++ b/vmm/src/interrupt.rs @@ -46,7 +46,7 @@ impl InterruptRoute { None => { let new_gsi = allocator .allocate_gsi() - .ok_or_else(|| io::Error::other("Failed allocating new GSI"))?; + .map_err(|e| io::Error::other(format!("Failed allocating new GSI: {e}")))?; self.gsi = Some(new_gsi); Ok(new_gsi) } diff --git a/vmm/src/pci_segment.rs b/vmm/src/pci_segment.rs index 6a4f10aa7..d214cf7c8 100644 --- a/vmm/src/pci_segment.rs +++ b/vmm/src/pci_segment.rs @@ -209,7 +209,7 @@ impl PciSegment { .lock() .unwrap() .allocate_irq() - .ok_or(DeviceManagerError::AllocateIrq)? as u8, + .map_err(DeviceManagerError::AllocateIrq)? as u8, ); }