Merge pull request #9814 from Apokleos/bugfix-pcipath

runtime-rs: bugfix for root bus slot allocation

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

@@ -281,6 +281,8 @@ pub enum VmmData {
     MachineConfiguration(Box<VmConfigInfo>),
     /// Prometheus Metrics represented by String.
     HypervisorMetrics(String),
+    /// Return vfio device's slot number in guest.
+    VfioDeviceData(Option<u8>),
     /// Sync Hotplug
     SyncHotplug((Sender<Option<i32>>, Receiver<Option<i32>>)),
 }
@@ -398,7 +400,9 @@ impl VmmService {
                 self.add_balloon_device(vmm, event_mgr, balloon_cfg)
             }
             #[cfg(feature = "host-device")]
-            VmmAction::InsertHostDevice(hostdev_cfg) => self.add_vfio_device(vmm, hostdev_cfg),
+            VmmAction::InsertHostDevice(mut hostdev_cfg) => {
+                self.add_vfio_device(vmm, &mut hostdev_cfg)
+            }
             #[cfg(feature = "host-device")]
             VmmAction::PrepareRemoveHostDevice(hostdev_id) => {
                 self.prepare_remove_vfio_device(vmm, &hostdev_id)
@@ -850,7 +854,7 @@ impl VmmService {
     }
 
     #[cfg(feature = "host-device")]
-    fn add_vfio_device(&self, vmm: &mut Vmm, config: HostDeviceConfig) -> VmmRequestResult {
+    fn add_vfio_device(&self, vmm: &mut Vmm, config: &mut HostDeviceConfig) -> VmmRequestResult {
         let vm = vmm.get_vm_mut().ok_or(VmmActionError::HostDeviceConfig(
             VfioDeviceError::InvalidVMID,
         ))?;
@@ -873,7 +877,8 @@ impl VmmService {
             .unwrap()
             .insert_device(&mut ctx, config)
             .map_err(VmmActionError::HostDeviceConfig)?;
-        Ok(VmmData::Empty)
+
+        Ok(VmmData::VfioDeviceData(config.dev_config.guest_dev_id))
     }
 
     // using upcall to unplug the pci device in the guest

src/dragonball/src/device_manager/mod.rs

@@ -553,7 +553,7 @@ impl DeviceOpContext {
         &self,
         dev: &Arc<dyn DeviceIo>,
         callback: Option<Box<dyn Fn(UpcallClientResponse) + Send>>,
-    ) -> Result<()> {
+    ) -> Result<u8> {
         if !self.is_hotplug || !self.pci_hotplug_enabled {
             return Err(DeviceMgrError::InvalidOperation);
         }
@@ -561,7 +561,12 @@ impl DeviceOpContext {
         let (busno, devfn) = DeviceManager::get_pci_device_info(dev)?;
         let req = DevMgrRequest::AddPciDev(PciDevRequest { busno, devfn });
 
-        self.call_hotplug_device(req, callback)
+        self.call_hotplug_device(req, callback)?;
+
+        // Extract the slot number from devfn
+        // Right shift by 3 to remove function bits (2:0) and
+        // align slot bits (7:3) to the least significant position
+        Ok(devfn >> 3)
     }
 
     #[cfg(feature = "host-device")]

src/dragonball/src/device_manager/vfio_dev_mgr/mod.rs

@@ -255,7 +255,7 @@ impl VfioDeviceMgr {
     pub fn insert_device(
         &mut self,
         ctx: &mut DeviceOpContext,
-        config: HostDeviceConfig,
+        config: &mut HostDeviceConfig,
     ) -> Result<()> {
         if !cfg!(feature = "hotplug") && ctx.is_hotplug {
             return Err(VfioDeviceError::UpdateNotAllowedPostBoot);
@@ -267,7 +267,7 @@ impl VfioDeviceMgr {
             "hostdev_id" => &config.hostdev_id,
             "bdf" => &config.dev_config.bus_slot_func,
         );
-        let device_index = self.info_list.insert_or_update(&config)?;
+        let device_index = self.info_list.insert_or_update(config)?;
         // Handle device hotplug case
         if ctx.is_hotplug {
             slog::info!(
@@ -277,7 +277,7 @@ impl VfioDeviceMgr {
                 "hostdev_id" => &config.hostdev_id,
                 "bdf" => &config.dev_config.bus_slot_func,
             );
-            self.add_device(ctx, &config, device_index)?;
+            self.add_device(ctx, config, device_index)?;
         }
 
         Ok(())
@@ -438,7 +438,7 @@ impl VfioDeviceMgr {
     fn add_device(
         &mut self,
         ctx: &mut DeviceOpContext,
-        cfg: &HostDeviceConfig,
+        cfg: &mut HostDeviceConfig,
         idx: usize,
     ) -> Result<()> {
         let dev = self.create_device(cfg, ctx, idx)?;
@@ -450,8 +450,13 @@ impl VfioDeviceMgr {
 
             self.register_memory(vm_memory.deref())?;
         }
-        ctx.insert_hotplug_pci_device(&dev, None)
+        let slot = ctx
-            .map_err(VfioDeviceError::VfioDeviceMgr)
+            .insert_hotplug_pci_device(&dev, None)
+            .map_err(VfioDeviceError::VfioDeviceMgr)?;
+
+        cfg.dev_config.guest_dev_id = Some(slot);
+
+        Ok(())
     }
 
     /// Gets the index of the device with the specified `hostdev_id` if it exists in the list.

src/runtime-rs/crates/hypervisor/src/dragonball/inner_device.rs

@@ -4,6 +4,7 @@
 // SPDX-License-Identifier: Apache-2.0
 //
 
+use std::convert::TryFrom;
 use std::path::PathBuf;
 
 use anyhow::{anyhow, Context, Result};
@@ -19,6 +20,7 @@ use dragonball::device_manager::{
 };
 
 use super::{build_dragonball_network_config, DragonballInner};
+use crate::device::pci_path::PciPath;
 use crate::VhostUserConfig;
 use crate::{
     device::DeviceType, HybridVsockConfig, NetworkConfig, ShareFsConfig, ShareFsMountConfig,
@@ -37,46 +39,64 @@ pub(crate) fn drive_index_to_id(index: u64) -> String {
 }
 
 impl DragonballInner {
-    pub(crate) async fn add_device(&mut self, device: DeviceType) -> Result<()> {
+    pub(crate) async fn add_device(&mut self, device: DeviceType) -> Result<DeviceType> {
         if self.state == VmmState::NotReady {
             info!(sl!(), "VMM not ready, queueing device {}", device);
 
             // add the pending device by reverse order, thus the
             // start_vm would pop the devices in an right order
             // to add the devices.
-            self.pending_devices.insert(0, device);
+            self.pending_devices.insert(0, device.clone());
-            return Ok(());
+            return Ok(device);
         }
 
         info!(sl!(), "dragonball add device {:?}", &device);
         match device {
-            DeviceType::Network(network) => self
+            DeviceType::Network(network) => {
-                .add_net_device(&network.config)
+                self.add_net_device(&network.config)
-                .context("add net device"),
+                    .context("add net device")?;
-            DeviceType::Vfio(hostdev) => self.add_vfio_device(&hostdev).context("add vfio device"),
+                Ok(DeviceType::Network(network))
-            DeviceType::Block(block) => self
+            }
-                .add_block_device(
+            DeviceType::Vfio(mut hostdev) => {
+                self.add_vfio_device(&mut hostdev)
+                    .context("add vfio device")?;
+
+                Ok(DeviceType::Vfio(hostdev))
+            }
+            DeviceType::Block(block) => {
+                self.add_block_device(
                     block.config.path_on_host.as_str(),
                     block.device_id.as_str(),
                     block.config.is_readonly,
                     block.config.no_drop,
                 )
-                .context("add block device"),
+                .context("add block device")?;
-            DeviceType::VhostUserBlk(block) => self
+                Ok(DeviceType::Block(block))
-                .add_block_device(
+            }
+            DeviceType::VhostUserBlk(block) => {
+                self.add_block_device(
                     block.config.socket_path.as_str(),
                     block.device_id.as_str(),
                     block.is_readonly,
                     block.no_drop,
                 )
-                .context("add vhost user based block device"),
+                .context("add vhost user based block device")?;
-            DeviceType::HybridVsock(hvsock) => self.add_hvsock(&hvsock.config).context("add vsock"),
+                Ok(DeviceType::VhostUserBlk(block))
-            DeviceType::ShareFs(sharefs) => self
+            }
-                .add_share_fs_device(&sharefs.config)
+            DeviceType::HybridVsock(hvsock) => {
-                .context("add share fs device"),
+                self.add_hvsock(&hvsock.config).context("add vsock")?;
-            DeviceType::VhostUserNetwork(dev) => self
+                Ok(DeviceType::HybridVsock(hvsock))
-                .add_vhost_user_net_device(&dev.config)
+            }
-                .context("add vhost-user-net device"),
+            DeviceType::ShareFs(sharefs) => {
+                self.add_share_fs_device(&sharefs.config)
+                    .context("add share fs device")?;
+                Ok(DeviceType::ShareFs(sharefs))
+            }
+            DeviceType::VhostUserNetwork(dev) => {
+                self.add_vhost_user_net_device(&dev.config)
+                    .context("add vhost-user-net device")?;
+                Ok(DeviceType::VhostUserNetwork(dev))
+            }
             DeviceType::Vsock(_) => todo!(),
         }
     }
@@ -121,56 +141,49 @@ impl DragonballInner {
         }
     }
 
-    fn add_vfio_device(&mut self, device: &VfioDevice) -> Result<()> {
+    fn add_vfio_device(&mut self, device: &mut VfioDevice) -> Result<()> {
-        let vfio_device = device.clone();
-
         // FIXME:
         // A device with multi-funtions, or a IOMMU group with one more
         // devices, the Primary device is selected to be passed to VM.
         // And the the first one is Primary device.
         // safe here, devices is not empty.
-        let primary_device = vfio_device.devices.first().unwrap().clone();
+        let primary_device = device.devices.first_mut().unwrap();
-
+        let vendor_device_id = if let Some(vd) = primary_device.device_vendor.as_ref() {
-        let vendor_device_id = if let Some(vd) = primary_device.device_vendor {
             vd.get_device_vendor_id()?
         } else {
             0
         };
 
-        // It's safe to unwrap the guest_pci_path and get device slot,
-        // As it has been assigned in vfio device manager.
-        let pci_path = primary_device.guest_pci_path.unwrap();
-        let guest_dev_id = pci_path.get_device_slot().unwrap().0;
-
         info!(
             sl!(),
             "insert host device. 
             host device id: {:?}, 
             bus_slot_func: {:?}, 
-            guest device id: {:?}, 
             vendor/device id: {:?}",
             primary_device.hostdev_id,
             primary_device.bus_slot_func,
-            guest_dev_id,
             vendor_device_id,
         );
 
         let vfio_dev_config = VfioPciDeviceConfig {
-            bus_slot_func: primary_device.bus_slot_func,
+            bus_slot_func: primary_device.bus_slot_func.clone(),
             vendor_device_id,
-            guest_dev_id: Some(guest_dev_id),
             ..Default::default()
         };
         let host_dev_config = HostDeviceConfig {
-            hostdev_id: primary_device.hostdev_id,
+            hostdev_id: primary_device.hostdev_id.clone(),
             sysfs_path: primary_device.sysfs_path.clone(),
             dev_config: vfio_dev_config,
         };
 
-        self.vmm_instance
+        let guest_device_id = self
+            .vmm_instance
             .insert_host_device(host_dev_config)
             .context("insert host device failed")?;
 
+        // It's safe to unwrap guest_device_id as we can get a guest device id here.
+        primary_device.guest_pci_path = Some(PciPath::try_from(guest_device_id.unwrap() as u32)?);
+
         Ok(())
     }
 

src/runtime-rs/crates/hypervisor/src/dragonball/mod.rs

@@ -104,10 +104,7 @@ impl Hypervisor for Dragonball {
 
     async fn add_device(&self, device: DeviceType) -> Result<DeviceType> {
         let mut inner = self.inner.write().await;
-        match inner.add_device(device.clone()).await {
+        inner.add_device(device.clone()).await
-            Ok(_) => Ok(device),
-            Err(err) => Err(err),
-        }
     }
 
     async fn remove_device(&self, device: DeviceType) -> Result<()> {

src/runtime-rs/crates/hypervisor/src/dragonball/vmm_instance.rs

@@ -197,12 +197,17 @@ impl VmmInstance {
         Err(anyhow!("Failed to get machine info"))
     }
 
-    pub fn insert_host_device(&self, device_cfg: HostDeviceConfig) -> Result<()> {
+    pub fn insert_host_device(&self, device_cfg: HostDeviceConfig) -> Result<Option<u8>> {
-        self.handle_request_with_retry(Request::Sync(VmmAction::InsertHostDevice(
+        if let VmmData::VfioDeviceData(guest_dev_id) = self.handle_request_with_retry(
-            device_cfg.clone(),
+            Request::Sync(VmmAction::InsertHostDevice(device_cfg.clone())),
-        )))
+        )? {
-        .with_context(|| format!("Failed to insert host device {:?}", device_cfg))?;
+            Ok(guest_dev_id)
-        Ok(())
+        } else {
+            Err(anyhow!(format!(
+                "Failed to insert host device {:?}",
+                device_cfg
+            )))
+        }
     }
 
     pub fn prepare_remove_host_device(&self, id: &str) -> Result<()> {