Backport tests and documentation for PCI devices

CHANGELOG.md

@@ -35,6 +35,12 @@ and this project adheres to
   requests to `/mmds/config` to enforce MMDS to always respond plain text
   contents in the IMDS format regardless of the `Accept` header in requests.
   Users need to regenerate snapshots.
+- [#5364](https://github.com/firecracker-microvm/firecracker/pull/5364): Added
+  PCI support in Firecracker. PCI support is optional. Users can enable it
+  passing the `--enable-pci` flag when launching the Firecracker process. When
+  Firecracker process is launched with PCI support, it will create all VirtIO
+  devices using a PCI VirtIO transport. If not enabled, Firecracker will use the
+  MMIO transport instead.
 
 ### Changed
 

FAQ.md

@@ -129,7 +129,7 @@ Example of a kernel valid command line that enables the serial console (which
 goes in the `boot_args` field of the `/boot-source` Firecracker API resource):
 
 ```console
-console=ttyS0 reboot=k panic=1 pci=off nomodule
+console=ttyS0 reboot=k panic=1 nomodule
 ```
 
 ### How can I configure multiple Ethernet devices through the kernel command line?

docs/getting-started.md

@@ -194,9 +194,16 @@ API_SOCKET="/tmp/firecracker.socket"
 sudo rm -f $API_SOCKET
 
 # Run firecracker
-sudo ./firecracker --api-sock "${API_SOCKET}"
+sudo ./firecracker --api-sock "${API_SOCKET} --enable-pci"
 ```
 
+The `--enable-pci` flag instructs Firecracker to create all VirtIO devices using
+a PCI VirtIO transport. This flag is optional. If not passed, Firecracker will
+create devices using the legacy MMIO transport. We suggest that users enable the
+PCI transport, as it yields higher throughput and lower latency for VirtIO
+devices. For more information regarding guest kernel requirements for using PCI
+look at our [kernel policy documentation](./kernel-policy.md).
+
 In a new terminal (do not close the 1st one):
 
 ```bash
@@ -240,7 +247,7 @@ sudo curl -X PUT --unix-socket "${API_SOCKET}" \
     "http://localhost/logger"
 
 KERNEL="./$(ls vmlinux* | tail -1)"
-KERNEL_BOOT_ARGS="console=ttyS0 reboot=k panic=1 pci=off"
+KERNEL_BOOT_ARGS="console=ttyS0 reboot=k panic=1"
 
 ARCH=$(uname -m)
 

docs/kernel-policy.md

@@ -67,6 +67,15 @@ The configuration items that may be relevant for Firecracker are:
   - use CPU RNG instructions (if present) to initialize RNG. Available for >=
     5.10
 - ACPI support - `CONFIG_ACPI` and `CONFIG_PCI`
+- PCI support:
+  - `CONFIG_BLK_MQ_PCI`
+  - `CONFIG_PCI`
+  - `CONFIG_PCI_MMCONFIG`
+  - `CONFIG_PCI_MSI`
+  - `CONFIG_PCIEPORTBUS`
+  - `CONFIG_VIRTIO_PCI`
+  - `CONFIG_PCI_HOST_COMMON`
+  - `CONFIG_PCI_HOST_GENERIC`
 
 There are also guest config options which are dependant on the platform on which
 Firecracker is run:
@@ -138,6 +147,63 @@ following configurations:
 - Only legacy mechanisms
 - Both ACPI and legacy mechanisms
 
+##### Booting with PCI:
+
+Firecracker supports booting guest microVMs with PCI support. This option is
+enabled using the `--enable-pci` flag when launching the Firecracker process.
+With PCI enabled, Firecracker will create all VirtIO devices using a PCI VirtIO
+transport. The PCI transport typically achieves higher throughput and lower
+latency for VirtIO devices. No further, per device, configuration is needed to
+enable the PCI transport.
+
+PCI support is optional; if it is not enabled Firecracker will create VirtIO
+devices using the MMIO transport.
+
+For Firecracker microVMs to boot properly with PCI support, use a guest kernel
+built with PCI support. See the relevant Kconfig flags in our list of
+[relevant Kconfig options](#guest-kernel-configuration-items):
+
+> [!IMPORTANT]
+>
+> Make sure that the kernel command line **does NOT** include the `pci=off`
+> slug, which disables PCI support during boot time within the guest. When PCI
+> is disabled, Firecracker will add this slug in the command line to instruct
+> the guest kernel to skip useless PCI checks. For more info, look into the
+> section for [Kernel command line parameters](#kernel-command-line-parameters).
+
+> [!NOTE]
+>
+> On x86_64 systems, `CONFIG_PCI` Kconfig option is needed even when booting
+> microVMs without PCI support in case users want to use ACPI to boot. See
+> [here](#booting-with-acpi-x86_64-only) for more info.
+
+## Kernel command line parameters
+
+By default, Firecracker will boot a guest microVM passing the following command
+line parameters to the kernel:
+
+`reboot=k panic=1 nomodule 8250.nr_uarts=0 i8042.noaux i8042.nomux i8042.dumbkbd swiotlb=noforce`.
+
+- `reboot=k` shut down the guest on reboot, instead of rebooting
+- `panic=1` on panic, reboot after 1 second
+- `nomodule` disable loadable kernel module support
+- `8250.nr_uarts=0` disable 8250 serial interface
+- `i8042.noaux` do not probe the i8042 controller for an attached mouse (save
+  boot time)
+- `i8042.nomux` do not probe i8042 for a multiplexing controller (save boot
+  time)
+- `i8042.dumbkbd` do not attempt to control kbd state via the i8042 (save boot
+  time)
+- `swiotlb=noforce` disable software bounce buffers (SWIOTLB)
+
+When running without [PCI support](#booting-with-pci), Firecracker will also
+append `pci=off` to the above list. This option instructs the guest kernel to
+avoid PCI probing.
+
+Users can provide their own command line parameters through the `boot_args`
+field of the `/boot-source`
+[Firecracker API](../src/firecracker/swagger/firecracker.yaml).
+
 ## Caveats
 
 - [Snapshot compatibility across kernel versions](snapshotting/snapshot-support.md#snapshot-compatibility-across-kernel-versions)

src/pci/src/bus.rs

@@ -161,6 +161,9 @@ impl PciConfigIo {
             return 0xffff_ffff;
         }
 
+        // NOTE: Potential contention among vCPU threads on this lock. This should not
+        // be a problem currently, since we mainly access this when we are setting up devices.
+        // We might want to do some profiling to ensure this does not become a bottleneck.
         self.pci_bus
             .as_ref()
             .lock()
@@ -195,6 +198,9 @@ impl PciConfigIo {
             return None;
         }
 
+        // NOTE: Potential contention among vCPU threads on this lock. This should not
+        // be a problem currently, since we mainly access this when we are setting up devices.
+        // We might want to do some profiling to ensure this does not become a bottleneck.
         let pci_bus = self.pci_bus.as_ref().lock().unwrap();
         if let Some(d) = pci_bus.devices.get(&(device as u32)) {
             let mut device = d.lock().unwrap();

src/vmm/src/builder.rs

@@ -648,9 +648,9 @@ pub(crate) mod tests {
     use super::*;
     use crate::device_manager::tests::default_device_manager;
     use crate::devices::virtio::block::CacheType;
+    use crate::devices::virtio::generated::virtio_ids;
     use crate::devices::virtio::rng::device::ENTROPY_DEV_ID;
-    use crate::devices::virtio::vsock::{TYPE_VSOCK, VSOCK_DEV_ID};
-    use crate::devices::virtio::{TYPE_BALLOON, TYPE_BLOCK, TYPE_RNG};
+    use crate::devices::virtio::vsock::VSOCK_DEV_ID;
     use crate::mmds::data_store::{Mmds, MmdsVersion};
     use crate::mmds::ns::MmdsNetworkStack;
     use crate::utils::mib_to_bytes;
@@ -848,7 +848,7 @@ pub(crate) mod tests {
 
         assert!(
             vmm.device_manager
-                .get_virtio_device(TYPE_VSOCK, &vsock_dev_id)
+                .get_virtio_device(virtio_ids::VIRTIO_ID_VSOCK, &vsock_dev_id)
                 .is_some()
         );
     }
@@ -873,7 +873,7 @@ pub(crate) mod tests {
 
         assert!(
             vmm.device_manager
-                .get_virtio_device(TYPE_RNG, ENTROPY_DEV_ID)
+                .get_virtio_device(virtio_ids::VIRTIO_ID_RNG, ENTROPY_DEV_ID)
                 .is_some()
         );
     }
@@ -907,7 +907,7 @@ pub(crate) mod tests {
 
         assert!(
             vmm.device_manager
-                .get_virtio_device(TYPE_BALLOON, BALLOON_DEV_ID)
+                .get_virtio_device(virtio_ids::VIRTIO_ID_BALLOON, BALLOON_DEV_ID)
                 .is_some()
         );
     }
@@ -958,7 +958,7 @@ pub(crate) mod tests {
             assert!(cmdline_contains(&cmdline, "root=/dev/vda ro"));
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, drive_id.as_str())
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str())
                     .is_some()
             );
         }
@@ -979,7 +979,7 @@ pub(crate) mod tests {
             assert!(cmdline_contains(&cmdline, "root=PARTUUID=0eaa91a0-01 rw"));
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, drive_id.as_str())
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str())
                     .is_some()
             );
         }
@@ -1001,7 +1001,7 @@ pub(crate) mod tests {
             assert!(!cmdline_contains(&cmdline, "root=/dev/vda"));
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, drive_id.as_str())
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str())
                     .is_some()
             );
         }
@@ -1038,17 +1038,17 @@ pub(crate) mod tests {
             assert!(cmdline_contains(&cmdline, "root=PARTUUID=0eaa91a0-01 rw"));
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, "root")
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, "root")
                     .is_some()
             );
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, "secondary")
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, "secondary")
                     .is_some()
             );
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, "third")
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, "third")
                     .is_some()
             );
 
@@ -1077,7 +1077,7 @@ pub(crate) mod tests {
             assert!(cmdline_contains(&cmdline, "root=/dev/vda rw"));
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, drive_id.as_str())
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str())
                     .is_some()
             );
         }
@@ -1098,7 +1098,7 @@ pub(crate) mod tests {
             assert!(cmdline_contains(&cmdline, "root=PARTUUID=0eaa91a0-01 ro"));
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, drive_id.as_str())
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str())
                     .is_some()
             );
         }
@@ -1119,7 +1119,7 @@ pub(crate) mod tests {
             assert!(cmdline_contains(&cmdline, "root=/dev/vda rw"));
             assert!(
                 vmm.device_manager
-                    .get_virtio_device(TYPE_BLOCK, drive_id.as_str())
+                    .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str())
                     .is_some()
             );
         }

src/vmm/src/device_manager/mmio.rs

@@ -468,7 +468,7 @@ pub(crate) mod tests {
     use crate::test_utils::multi_region_mem_raw;
     use crate::vstate::kvm::Kvm;
     use crate::vstate::memory::{GuestAddress, GuestMemoryMmap};
-    use crate::{Vm, arch};
+    use crate::{Vm, arch, impl_device_type};
 
     const QUEUE_SIZES: &[u16] = &[64];
 
@@ -522,6 +522,8 @@ pub(crate) mod tests {
     }
 
     impl VirtioDevice for DummyDevice {
+        impl_device_type!(0);
+
         fn avail_features(&self) -> u64 {
             0
         }
@@ -532,10 +534,6 @@ pub(crate) mod tests {
 
         fn set_acked_features(&mut self, _: u64) {}
 
-        fn device_type(&self) -> u32 {
-            0
-        }
-
         fn queues(&self) -> &[Queue] {
             &self.queues
         }

src/vmm/src/device_manager/mod.rs

@@ -89,7 +89,7 @@ pub enum FindDeviceError {
     /// Device not found
     DeviceNotFound,
     /// Internal Device error: {0}
-    InternalDeviceError(String),
+    InternalDeviceError(anyhow::Error),
 }
 
 #[derive(Debug)]
@@ -355,27 +355,35 @@ impl DeviceManager {
     }
 
     /// Run fn `f()` for the virtio device matching `virtio_type` and `id`.
-    pub fn with_virtio_device_with_id<T, F>(
+    pub fn try_with_virtio_device_with_id<T, F, R, E>(
         &self,
-        virtio_type: u32,
         id: &str,
         f: F,
-    ) -> Result<(), FindDeviceError>
+    ) -> Result<R, FindDeviceError>
     where
         T: VirtioDevice + 'static + Debug,
-        F: FnOnce(&mut T) -> Result<(), String>,
+        E: std::error::Error + 'static + Send + Sync,
+        F: FnOnce(&mut T) -> Result<R, E>,
     {
-        if let Some(device) = self.get_virtio_device(virtio_type, id) {
+        if let Some(device) = self.get_virtio_device(T::const_device_type(), id) {
             let mut dev = device.lock().expect("Poisoned lock");
             f(dev
                 .as_mut_any()
                 .downcast_mut::<T>()
                 .ok_or(FindDeviceError::InvalidDeviceType)?)
-            .map_err(FindDeviceError::InternalDeviceError)?;
+            .map_err(|e| FindDeviceError::InternalDeviceError(e.into()))
         } else {
-            return Err(FindDeviceError::DeviceNotFound);
+            Err(FindDeviceError::DeviceNotFound)
         }
-        Ok(())
+    }
+
+    /// Run fn `f()` for the virtio device matching `virtio_type` and `id`.
+    pub fn with_virtio_device_with_id<T, F, R>(&self, id: &str, f: F) -> Result<R, FindDeviceError>
+    where
+        T: VirtioDevice + 'static + Debug,
+        F: FnOnce(&mut T) -> R,
+    {
+        self.try_with_virtio_device_with_id(id, |dev: &mut T| Ok::<R, FindDeviceError>(f(dev)))
     }
 }