Dragonball: add pci vfio passthrough, hot(un)plug support #8740

studychao · 2023-12-27T07:44:00Z

dragonball: add InsertHostDevice vmm action

Introduce a new vmm action InsertHostDevice to passthrough
host pci devices like NIC or GPU devices into guest so that
users could have high performance usage of those devices.

dragonball: add pci hotplug / hot-unplug support
    
    Introduce two new vmm action to implement pci hotplug
    and pci hot-unplug: PrepareRemoveHostDevice and RemoveHostDevice.
    
    PrepareRemoveHostDevice is to call upcall to unregister the pci device
    in the guest kernel.
    RemoveHostDevice should be called after PrepareRemoveHostDevice, it is used
    to clean the PCI resource in the Dragonball side.

dragonball: add InsertHostDevice vmm action
    
    Introduce a new vmm action InsertHostDevice to passthrough
    host pci devices like NIC or GPU devices into guest so that
    users could have high performance usage of those devices.

justxuewei · 2023-12-27T08:18:42Z

src/dragonball/Cargo.toml

+vfio-bindings = { version = "0.3.0", optional = true}
+vfio-ioctls =  { version = "0.1.0", optional = true}


Missing a space after true.

Suggested change

vfio-bindings = { version = "0.3.0", optional = true}

vfio-ioctls = { version = "0.1.0", optional = true}

vfio-bindings = { version = "0.3.0", optional = true }

vfio-ioctls = { version = "0.1.0", optional = true }

justxuewei

Thanks @studychao, a few comments here.

justxuewei · 2023-12-27T09:23:14Z

src/dragonball/src/api/v1/vmm_action.rs

+    #[cfg(feature = "host-device")]
+    fn add_vfio_device(&self, vmm: &mut Vmm, config: HostDeviceConfig) -> VmmRequestResult {
+        let vm = vmm.get_vm_mut().ok_or(VmmActionError::HostDeviceConfig(
+            VfioDeviceError::InvalidVMID,


Nit: InvalidVMID -> InvalidVmId

I'll raise a separate PR to fix this.

#8747 is raised for this problem.

justxuewei · 2023-12-27T09:23:36Z

src/dragonball/src/api/v1/vmm_action.rs

+
+        let mut ctx = vm.create_device_op_context(None).map_err(|e| {
+            info!("create device op context error: {:?}", e);
+            if let StartMicroVmError::MicroVMAlreadyRunning = e {


Nit: MicroVMAlreadyRunning -> MicroVmAlreadyRunning

justxuewei · 2023-12-27T09:24:48Z

src/dragonball/src/api/v1/vmm_action.rs

+
+        let (sender, receiver) = unbounded();
+
+        let vfio_manager = vm.device_manager.vfio_manager.lock().unwrap();


Give some comments to describe why it is safe?

justxuewei · 2023-12-27T09:25:41Z

src/dragonball/src/api/v1/vmm_action.rs

+            }
+        })?;
+
+        let mut vfio_manager = vm.device_manager.vfio_manager.lock().unwrap();


justxuewei · 2023-12-27T09:26:55Z

src/dragonball/src/dbs_pci/src/vfio.rs

+
+        self.bus
+            .upgrade()
+            .unwrap()


fixed.
will return error here.

justxuewei · 2023-12-27T09:53:23Z

src/dragonball/src/device_manager/vfio_dev_mgr/pci_vfio.rs

+use crate::device_manager::DeviceManagerContext;
+use crate::resource_manager::ResourceManager;
+
+///we only support one pci bus


Suggested change

///we only support one pci bus

/// we only support one pci bus

justxuewei · 2023-12-27T09:54:02Z

src/dragonball/src/device_manager/vfio_dev_mgr/pci_vfio.rs

+            base: 0xcf8,
+            size: 0x8,


Give some comments?

comment added

justxuewei · 2023-12-27T09:55:24Z

src/dragonball/src/error.rs

@@ -205,6 +207,14 @@ pub enum StartMicroVmError {
    VhostUserNetDeviceError(
        #[source] device_manager::vhost_user_net_dev_mgr::VhostUserNetDeviceError,
    ),
+    #[cfg(feature = "host-device")]
+    /// Failed to create VFIO device
+    #[error("cannot create VFIO device")]


Print error stack?

Suggested change

#[error("cannot create VFIO device")]

#[error("cannot create VFIO device: {0:?}")]

justxuewei · 2023-12-27T09:55:31Z

src/dragonball/src/error.rs

+    CreateVfioDevice(#[source] VfioDeviceError),
+    #[cfg(feature = "host-device")]
+    /// Failed to register DMA memory address range.
+    #[error("failure while registering DMA address range: {0}")]


Suggested change

#[error("failure while registering DMA address range: {0}")]

#[error("failure while registering DMA address range: {0:?}")]

justxuewei · 2023-12-27T09:55:53Z

src/dragonball/src/error.rs

+    #[cfg(feature = "host-device")]
+    /// Failed to register DMA memory address range.
+    #[error("failure while registering DMA address range: {0}")]
+    RegisterDMAAddress(#[source] VfioDeviceError),


Nit: RegisterDMAAddress -> RegisterDmaAddress

ZizhengBian · 2023-12-27T13:10:30Z

src/dragonball/src/api/v1/vmm_action.rs

+
+    #[cfg(feature = "host-device")]
+    /// The action `InsertHostDevice` failed either because of bad user input or an internal error.
+    #[error("failed to add VFIO passthrough device: {0}")]


ZizhengBian · 2023-12-27T13:20:49Z

src/dragonball/src/dbs_device/src/lib.rs

@@ -17,6 +17,8 @@ use std::cmp::{Ord, PartialOrd};
 use std::convert::TryFrom;
 use std::sync::Mutex;

+use downcast_rs::{impl_downcast, Downcast};


use std::any instead of downcast_rs

ZizhengBian · 2023-12-27T13:24:32Z

src/dragonball/src/dbs_pci/src/root_bus.rs

@@ -73,11 +73,7 @@ impl PciDevice for PciHostBridge {
    }
 }

-impl DeviceIo for PciHostBridge {
-    fn as_any(&self) -> &dyn std::any::Any {


we can use as_any, not downcast

ZizhengBian · 2023-12-27T13:24:59Z

src/dragonball/src/dbs_pci/src/vfio.rs

@@ -884,7 +884,7 @@ impl Region {
    }
 }

-struct VfioPciDeviceState<C: PciSystemContext> {
+pub struct VfioPciDeviceState<C: PciSystemContext> {


Why make state a pub struct?

cause we need to get the state of VfioPciDevice, if we make this pub, Rust will complain that we leak a private struct. Error is attached.

justxuewei

A few addtional comments.

justxuewei · 2023-12-28T06:31:59Z