[RFC] fix incorrect GSI numbering causing device limit on aarch64

CHANGELOG.md

@@ -40,6 +40,9 @@ and this project adheres to
 - [#5260](https://github.com/firecracker-microvm/firecracker/pull/5260): Fixed a
   bug allowing the block device to starve all other devices when backed by a
   sufficiently slow drive.
+- [#4207](https://github.com/firecracker-microvm/firecracker/issues/4207): Fixed
+  GSI numbering on aarch64 to correctly allow up to 96 devices being attached
+  simultaneously.
 
 ## [1.12.0]
 

docs/device-api.md

@@ -125,12 +125,6 @@ specification:
 |                        | track_dirty_pages |    O     |       O        |      O       |        O         |     O      |      O       |
 |                        | vcpu_count        |    O     |       O        |      O       |        O         |     O      |      O       |
 
-## Known device limitations
-
-If more than 64 devices are configured for a VM in total on aarch64, only first
-64 of them are functional
-([related issue](https://github.com/firecracker-microvm/firecracker/issues/4207)).
-
 ## Instance Actions
 
 All instance actions can be found in the [Swagger](https://swagger.io)

src/vmm/src/arch/aarch64/fdt.rs

@@ -362,7 +362,7 @@ fn create_virtio_node(fdt: &mut FdtWriter, dev_info: &MMIODeviceInfo) -> Result<
         "interrupts",
         &[
             GIC_FDT_IRQ_TYPE_SPI,
-            dev_info.irq.unwrap().into(),
+            dev_info.irq.unwrap(),
             IRQ_TYPE_EDGE_RISING,
         ],
     )?;
@@ -383,7 +383,7 @@ fn create_serial_node(fdt: &mut FdtWriter, dev_info: &MMIODeviceInfo) -> Result<
         "interrupts",
         &[
             GIC_FDT_IRQ_TYPE_SPI,
-            dev_info.irq.unwrap().into(),
+            dev_info.irq.unwrap(),
             IRQ_TYPE_EDGE_RISING,
         ],
     )?;
@@ -439,7 +439,6 @@ fn create_devices_node(
 #[cfg(test)]
 mod tests {
     use std::ffi::CString;
-    use std::num::NonZeroU32;
 
     use kvm_ioctls::Kvm;
 
@@ -470,23 +469,23 @@ mod tests {
                 (DeviceType::Serial, DeviceType::Serial.to_string()),
                 MMIODeviceInfo {
                     addr: 0x00,
-                    irq: NonZeroU32::new(1),
+                    irq: Some(1u32),
                     len: LEN,
                 },
             ),
             (
                 (DeviceType::Virtio(1), "virtio".to_string()),
                 MMIODeviceInfo {
                     addr: LEN,
-                    irq: NonZeroU32::new(2),
+                    irq: Some(2u32),
                     len: LEN,
                 },
             ),
             (
                 (DeviceType::Rtc, "rtc".to_string()),
                 MMIODeviceInfo {
                     addr: 2 * LEN,
-                    irq: NonZeroU32::new(3),
+                    irq: Some(3u32),
                     len: LEN,
                 },
             ),

src/vmm/src/arch/aarch64/layout.rs

@@ -80,5 +80,13 @@ pub const IRQ_MAX: u32 = 128;
 /// First usable interrupt on aarch64.
 pub const IRQ_BASE: u32 = 32;
 
+// The Linux kernel automatically shifts the GSI by 32 if it is an SPI,
+// allowing us to start numbering from 0 instead of 32.
+/// The first usable GSI on aarch64.
+pub const GSI_BASE: u32 = 0;
+
+/// The maximum usable GSI on aarch64.
+pub const GSI_MAX: u32 = IRQ_MAX - IRQ_BASE - 1;
+
 /// Below this address will reside the GIC, above this address will reside the MMIO devices.
 pub const MAPPED_IO_START: u64 = 1 << 30; // 1 GB

src/vmm/src/arch/mod.rs

@@ -22,8 +22,8 @@ pub use aarch64::vm::{ArchVm, ArchVmError, VmState};
 pub use aarch64::{
     ConfigurationError, MMIO_MEM_SIZE, MMIO_MEM_START, arch_memory_regions,
     configure_system_for_boot, get_kernel_start, initrd_load_addr, layout::CMDLINE_MAX_SIZE,
-    layout::IRQ_BASE, layout::IRQ_MAX, layout::SYSTEM_MEM_SIZE, layout::SYSTEM_MEM_START,
-    load_kernel,
+    layout::GSI_BASE, layout::GSI_MAX, layout::IRQ_BASE, layout::IRQ_MAX, layout::SYSTEM_MEM_SIZE,
+    layout::SYSTEM_MEM_START, load_kernel,
 };
 
 /// Module for x86_64 related functionality.
@@ -41,8 +41,9 @@ pub use x86_64::vm::{ArchVm, ArchVmError, VmState};
 pub use crate::arch::x86_64::{
     ConfigurationError, MMIO_MEM_SIZE, MMIO_MEM_START, arch_memory_regions,
     configure_system_for_boot, get_kernel_start, initrd_load_addr, layout::APIC_ADDR,
-    layout::CMDLINE_MAX_SIZE, layout::IOAPIC_ADDR, layout::IRQ_BASE, layout::IRQ_MAX,
-    layout::SYSTEM_MEM_SIZE, layout::SYSTEM_MEM_START, load_kernel,
+    layout::CMDLINE_MAX_SIZE, layout::GSI_BASE, layout::GSI_MAX, layout::IOAPIC_ADDR,
+    layout::IRQ_BASE, layout::IRQ_MAX, layout::SYSTEM_MEM_SIZE, layout::SYSTEM_MEM_START,
+    load_kernel,
 };
 
 /// Types of devices that can get attached to this platform.

src/vmm/src/arch/x86_64/layout.rs

@@ -24,6 +24,12 @@ pub const IRQ_BASE: u32 = 5;
 /// Last usable IRQ ID for virtio device interrupts on x86_64.
 pub const IRQ_MAX: u32 = 23;
 
+/// The first usable GSI on x86_64 is the same as the first usable IRQ ID.
+pub const GSI_BASE: u32 = IRQ_BASE;
+
+/// The maximum usable GSI on x86_64 is the same as the last usable IRQ ID.
+pub const GSI_MAX: u32 = IRQ_MAX;
+
 /// Address for the TSS setup.
 pub const KVM_TSS_ADDRESS: u64 = 0xfffb_d000;
 

src/vmm/src/device_manager/mmio.rs

@@ -7,7 +7,6 @@
 
 use std::collections::HashMap;
 use std::fmt::Debug;
-use std::num::NonZeroU32;
 use std::sync::{Arc, Mutex};
 
 #[cfg(target_arch = "x86_64")]
@@ -79,7 +78,7 @@
     /// Mmio addr range length.
     pub len: u64,
     /// Used Irq line for the device.
-    pub irq: Option<NonZeroU32>, // NOTE: guaranteed to be a value not 0, 0 is not allowed
+    pub irq: Option<u32>,
 }
 
 #[cfg(target_arch = "x86_64")]
@@ -89,7 +88,7 @@
     len: u64,
     irq: u32,
 ) -> Result<(), aml::AmlError> {
-    let dev_id = irq - crate::arch::IRQ_BASE;
+    let dev_id = irq - crate::arch::GSI_BASE;
     debug!(
         "acpi: Building AML for VirtIO device _SB_.V{:03}. memory range: {:#010x}:{} irq: {}",
         dev_id, addr, len, irq
@@ -151,7 +150,7 @@
     ) -> Result<MMIODeviceInfo, MmioError> {
         let irq = match resource_allocator.allocate_gsi(irq_count)?[..] {
             [] => None,
-            [irq] => NonZeroU32::new(irq),
+            [irq] => Some(irq),
             _ => return Err(MmioError::InvalidIrqConfig),
         };
 
@@ -205,7 +204,7 @@
                 vm.register_ioevent(queue_evt, &io_addr, u32::try_from(i).unwrap())
                     .map_err(MmioError::RegisterIoEvent)?;
             }
-            vm.register_irqfd(&locked_device.interrupt_trigger().irq_evt, irq.get())
+            vm.register_irqfd(&locked_device.interrupt_trigger().irq_evt, irq)
                 .map_err(MmioError::RegisterIrqFd)?;
         }
 
@@ -231,7 +230,7 @@
             .add_virtio_mmio_device(
                 device_info.len,
                 GuestAddress(device_info.addr),
-                device_info.irq.unwrap().get(),
+                device_info.irq.unwrap(),
                 None,
             )
             .map_err(MmioError::Cmdline)
@@ -258,7 +257,7 @@
                 device_info.len,
                 // We are sure that `irqs` has at least one element; allocate_mmio_resources makes
                 // sure of it.
-                device_info.irq.unwrap().get(),
+                device_info.irq.unwrap(),
             )?;
         }
         Ok(device_info)
@@ -290,7 +289,7 @@
                 .unwrap()
                 .serial
                 .interrupt_evt(),
-            device_info.irq.unwrap().get(),
+            device_info.irq.unwrap(),
         )
         .map_err(MmioError::RegisterIrqFd)?;
 
@@ -693,7 +692,7 @@
         #[cfg(target_arch = "aarch64")]
         vm.setup_irqchip(1).unwrap();
 
-        for _i in crate::arch::IRQ_BASE..=crate::arch::IRQ_MAX {
+        for _i in crate::arch::GSI_BASE..=crate::arch::GSI_MAX {
             device_manager
                 .register_virtio_test_device(
                     vm.fd(),
@@ -773,11 +772,10 @@
             device_manager.id_to_dev_info[&(DeviceType::Virtio(type_id), id.clone())].addr
         );
         assert_eq!(
-            crate::arch::IRQ_BASE,
+            crate::arch::GSI_BASE,
             device_manager.id_to_dev_info[&(DeviceType::Virtio(type_id), id)]
                 .irq
                 .unwrap()
-                .get()
         );
 
         let id = "bar";
@@ -834,7 +832,7 @@
         let device_info = device_manager
             .allocate_mmio_resources(&mut resource_allocator, 1)
             .unwrap();
-        assert_eq!(device_info.irq.unwrap().get(), crate::arch::IRQ_BASE);
+        assert_eq!(device_info.irq.unwrap(), crate::arch::GSI_BASE);
     }
 
     #[test]

src/vmm/src/device_manager/resources.rs

@@ -27,7 +27,7 @@ impl ResourceAllocator {
     /// Create a new resource allocator for Firecracker devices
     pub fn new() -> Result<Self, vm_allocator::Error> {
         Ok(Self {
-            gsi_allocator: IdAllocator::new(arch::IRQ_BASE, arch::IRQ_MAX)?,
+            gsi_allocator: IdAllocator::new(arch::GSI_BASE, arch::GSI_MAX)?,
             mmio_memory: AddressAllocator::new(arch::MMIO_MEM_START, arch::MMIO_MEM_SIZE)?,
             system_memory: AddressAllocator::new(arch::SYSTEM_MEM_START, arch::SYSTEM_MEM_SIZE)?,
         })
@@ -102,7 +102,7 @@ mod tests {
     use super::ResourceAllocator;
     use crate::arch;
 
-    const MAX_IRQS: u32 = arch::IRQ_MAX - arch::IRQ_BASE + 1;
+    const MAX_IRQS: u32 = arch::GSI_MAX - arch::GSI_BASE + 1;
 
     #[test]
     fn test_allocate_gsi() {
@@ -117,7 +117,7 @@ mod tests {
         // But allocating all of them at once should work
         assert_eq!(
             allocator.allocate_gsi(MAX_IRQS),
-            Ok((arch::IRQ_BASE..=arch::IRQ_MAX).collect::<Vec<_>>())
+            Ok((arch::GSI_BASE..=arch::GSI_MAX).collect::<Vec<_>>())
         );
         // And now we ran out of GSIs
         assert_eq!(
@@ -129,16 +129,16 @@ mod tests {
 
         let mut allocator = ResourceAllocator::new().unwrap();
         // We should be able to allocate 1 GSI
-        assert_eq!(allocator.allocate_gsi(1), Ok(vec![arch::IRQ_BASE]));
+        assert_eq!(allocator.allocate_gsi(1), Ok(vec![arch::GSI_BASE]));
         // We can't allocate MAX_IRQS any more
         assert_eq!(
             allocator.allocate_gsi(MAX_IRQS),
             Err(vm_allocator::Error::ResourceNotAvailable)
         );
         // We can allocate another one and it should be the second available
-        assert_eq!(allocator.allocate_gsi(1), Ok(vec![arch::IRQ_BASE + 1]));
+        assert_eq!(allocator.allocate_gsi(1), Ok(vec![arch::GSI_BASE + 1]));
         // Let's allocate the rest in a loop
-        for i in arch::IRQ_BASE + 2..=arch::IRQ_MAX {
+        for i in arch::GSI_BASE + 2..=arch::GSI_MAX {
             assert_eq!(allocator.allocate_gsi(1), Ok(vec![i]));
         }
     }

tests/integration_tests/functional/test_max_devices.py

@@ -6,23 +6,27 @@
 
 import pytest
 
-# IRQs are available from 5 to 23. We always use one IRQ for VMGenID device, so
-# the maximum number of devices supported at the same time is 18.
-MAX_DEVICES_ATTACHED = 18
+# On x86_64, IRQs are available from 5 to 23. We always use one IRQ for VMGenID
+# device, so the maximum number of devices supported at the same time is 18.
 
+# On aarch64, IRQs are available from 32 to 127. We always use one IRQ each for
+# the VMGenID and RTC devices, so the maximum number of devices supported
+# at the same time is 94.
+MAX_DEVICES_ATTACHED = {"x86_64": 18, "aarch64": 94}.get(platform.machine())
 
-@pytest.mark.skipif(
-    platform.machine() != "x86_64", reason="Firecracker supports 24 IRQs on x86_64."
-)
-def test_attach_maximum_devices(uvm_plain_any):
+
+def test_attach_maximum_devices(microvm_factory, guest_kernel, rootfs):
     """
     Test attaching maximum number of devices to the microVM.
     """
-    test_microvm = uvm_plain_any
+    if MAX_DEVICES_ATTACHED is None:
+        pytest.skip("Unsupported platform for this test.")
+
+    test_microvm = microvm_factory.build(guest_kernel, rootfs, monitor_memory=False)
     test_microvm.spawn()
 
-    # Set up a basic microVM.
-    test_microvm.basic_config()
+    # The default 256mib is not enough for 94 ssh connections on aarch64.
+    test_microvm.basic_config(mem_size_mib=512)
 
     # Add (`MAX_DEVICES_ATTACHED` - 1) devices because the rootfs
     # has already been configured in the `basic_config()`function.
@@ -36,14 +40,14 @@ def test_attach_maximum_devices(uvm_plain_any):
         test_microvm.ssh_iface(i).check_output("sync")
 
 
-@pytest.mark.skipif(
-    platform.machine() != "x86_64", reason="Firecracker supports 24 IRQs on x86_64."
-)
-def test_attach_too_many_devices(uvm_plain):
+def test_attach_too_many_devices(microvm_factory, guest_kernel, rootfs):
     """
     Test attaching to a microVM more devices than available IRQs.
     """
-    test_microvm = uvm_plain
+    if MAX_DEVICES_ATTACHED is None:
+        pytest.skip("Unsupported platform for this test.")
+
+    test_microvm = microvm_factory.build(guest_kernel, rootfs, monitor_memory=False)
     test_microvm.spawn()
 
     # Set up a basic microVM.