Merge pull request #8284 from amshinde/runtime-rs-update-device-pci-info

runtime-rs: update device pci info for vfio and virtio-blk devices

src/runtime-rs/crates/hypervisor/ch-config/src/ch_api.rs

@@ -70,7 +70,7 @@ pub async fn cloud_hypervisor_vm_stop(mut socket: UnixStream) -> Result<Option<S
     .await?
 }
 
-#[derive(Serialize, Deserialize)]
+#[derive(Deserialize, Debug)]
 pub struct PciDeviceInfo {
     pub id: String,
     pub bdf: String,

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

@@ -7,16 +7,18 @@
 use super::inner::CloudHypervisorInner;
 use crate::device::DeviceType;
 use crate::BlockDevice;
-use crate::HybridVsockConfig;
+use crate::HybridVsockDevice;
 use crate::NetworkConfig;
+use crate::PciPath;
+use crate::ShareFsDevice;
 use crate::ShareFsDeviceConfig;
 use crate::VfioDevice;
 use crate::VmmState;
 use anyhow::{anyhow, Context, Result};
+use ch_config::ch_api::cloud_hypervisor_vm_device_add;
 use ch_config::ch_api::{
-    cloud_hypervisor_vm_blockdev_add, cloud_hypervisor_vm_device_add,
-    cloud_hypervisor_vm_device_remove, cloud_hypervisor_vm_fs_add, PciDeviceInfo,
-    VmRemoveDeviceData,
+    cloud_hypervisor_vm_blockdev_add, cloud_hypervisor_vm_device_remove,
+    cloud_hypervisor_vm_fs_add, PciDeviceInfo, VmRemoveDeviceData,
 };
 use ch_config::convert::{DEFAULT_DISK_QUEUES, DEFAULT_DISK_QUEUE_SIZE, DEFAULT_NUM_PCI_SEGMENTS};
 use ch_config::DiskConfig;
@@ -31,7 +33,7 @@ pub const DEFAULT_FS_QUEUES: usize = 1;
 const DEFAULT_FS_QUEUE_SIZE: u16 = 1024;
 
 impl CloudHypervisorInner {
-    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::VmRunning {
             // If the VM is not running, add the device to the pending list to
             // be handled later.
@@ -55,8 +57,8 @@ impl CloudHypervisorInner {
             // - Network details need to be saved for later application.
             //
             match device {
-                DeviceType::ShareFs(_) => self.pending_devices.insert(0, device),
-                DeviceType::Network(_) => self.pending_devices.insert(0, device),
+                DeviceType::ShareFs(_) => self.pending_devices.insert(0, device.clone()),
+                DeviceType::Network(_) => self.pending_devices.insert(0, device.clone()),
                 _ => {
                     debug!(
                         sl!(),
@@ -65,20 +67,18 @@ impl CloudHypervisorInner {
                 }
             }
 
-            return Ok(());
+            return Ok(device);
         }
 
-        self.handle_add_device(device).await?;
-
-        Ok(())
+        self.handle_add_device(device).await
     }
 
-    async fn handle_add_device(&mut self, device: DeviceType) -> Result<()> {
+    async fn handle_add_device(&mut self, device: DeviceType) -> Result<DeviceType> {
         match device {
-            DeviceType::ShareFs(sharefs) => self.handle_share_fs_device(sharefs.config).await,
-            DeviceType::HybridVsock(hvsock) => self.handle_hvsock_device(&hvsock.config).await,
+            DeviceType::ShareFs(sharefs) => self.handle_share_fs_device(sharefs).await,
+            DeviceType::HybridVsock(hvsock) => self.handle_hvsock_device(hvsock).await,
             DeviceType::Block(block) => self.handle_block_device(block).await,
-            DeviceType::Vfio(vfiodev) => self.handle_vfio_device(&vfiodev).await,
+            DeviceType::Vfio(vfiodev) => self.handle_vfio_device(vfiodev).await,
             _ => Err(anyhow!("unhandled device: {:?}", device)),
         }
     }
@@ -108,9 +108,13 @@ impl CloudHypervisorInner {
         }
     }
 
-    async fn handle_share_fs_device(&mut self, cfg: ShareFsDeviceConfig) -> Result<()> {
-        if cfg.fs_type != VIRTIO_FS {
-            return Err(anyhow!("cannot handle share fs type: {:?}", cfg.fs_type));
+    async fn handle_share_fs_device(&mut self, sharefs: ShareFsDevice) -> Result<DeviceType> {
+        let device: ShareFsDevice = sharefs.clone();
+        if device.config.fs_type != VIRTIO_FS {
+            return Err(anyhow!(
+                "cannot handle share fs type: {:?}",
+                device.config.fs_type
+            ));
         }
 
         let socket = self
@@ -119,26 +123,26 @@ impl CloudHypervisorInner {
             .ok_or("missing socket")
             .map_err(|e| anyhow!(e))?;
 
-        let num_queues: usize = if cfg.queue_num > 0 {
-            cfg.queue_num as usize
+        let num_queues: usize = if device.config.queue_num > 0 {
+            device.config.queue_num as usize
         } else {
             DEFAULT_FS_QUEUES
         };
 
-        let queue_size: u16 = if cfg.queue_num > 0 {
-            u16::try_from(cfg.queue_size)?
+        let queue_size: u16 = if device.config.queue_num > 0 {
+            u16::try_from(device.config.queue_size)?
         } else {
             DEFAULT_FS_QUEUE_SIZE
         };
 
-        let socket_path = if cfg.sock_path.starts_with('/') {
-            PathBuf::from(cfg.sock_path)
+        let socket_path = if device.config.sock_path.starts_with('/') {
+            PathBuf::from(device.config.sock_path)
         } else {
-            scoped_join(&self.vm_path, cfg.sock_path)?
+            scoped_join(&self.vm_path, device.config.sock_path)?
         };
 
         let fs_config = FsConfig {
-            tag: cfg.mount_tag,
+            tag: device.config.mount_tag,
             socket: socket_path,
             num_queues,
             queue_size,
@@ -157,14 +161,16 @@ impl CloudHypervisorInner {
             debug!(sl!(), "fs add response: {:?}", detail);
         }
 
-        Ok(())
+        Ok(DeviceType::ShareFs(sharefs))
     }
 
-    async fn handle_hvsock_device(&mut self, _cfg: &HybridVsockConfig) -> Result<()> {
-        Ok(())
+    async fn handle_hvsock_device(&mut self, device: HybridVsockDevice) -> Result<DeviceType> {
+        Ok(DeviceType::HybridVsock(device))
     }
 
-    async fn handle_vfio_device(&mut self, device: &VfioDevice) -> Result<()> {
+    async fn handle_vfio_device(&mut self, device: VfioDevice) -> Result<DeviceType> {
+        let mut vfio_device: VfioDevice = device.clone();
+
         // 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.
@@ -204,9 +210,15 @@ impl CloudHypervisorInner {
                 serde_json::from_str(detail.as_str()).map_err(|e| anyhow!(e))?;
             self.device_ids
                 .insert(device.device_id.clone(), dev_info.id);
+
+            // Update PCI path for the vfio host device. It is safe to directly access the slice element
+            // here as we have already checked if it exists.
+            // Todo: Handle vfio-ap mediated devices - return error for them.
+            vfio_device.devices[0].guest_pci_path =
+                Some(Self::clh_pci_info_to_path(&dev_info.bdf)?);
         }
 
-        Ok(())
+        Ok(DeviceType::Vfio(vfio_device))
     }
 
     async fn remove_vfio_device(&mut self, device: &VfioDevice) -> Result<()> {
@@ -242,7 +254,31 @@ impl CloudHypervisorInner {
         Ok(())
     }
 
-    async fn handle_block_device(&mut self, device: BlockDevice) -> Result<()> {
+    // Various cloud-hypervisor APIs report a PCI address in "BB:DD.F"
+    // form within the PciDeviceInfo struct.
+    // eg "0000:00:DD.F"
+    fn clh_pci_info_to_path(bdf: &str) -> Result<PciPath> {
+        let tokens: Vec<&str> = bdf.split(':').collect();
+        if tokens.len() != 3 || tokens[0] != "0000" || tokens[1] != "00" {
+            return Err(anyhow!(
+                "Unexpected PCI address {:?} for clh device add",
+                bdf
+            ));
+        }
+
+        let toks: Vec<&str> = tokens[2].split('.').collect();
+        if toks.len() != 2 || toks[1] != "0" || toks[0].len() != 2 {
+            return Err(anyhow!(
+                "Unexpected PCI address {:?} for clh device add",
+                bdf
+            ));
+        }
+
+        PciPath::convert_from_string(toks[0])
+    }
+
+    async fn handle_block_device(&mut self, device: BlockDevice) -> Result<DeviceType> {
+        let mut block_dev = device.clone();
         let socket = self
             .api_socket
             .as_ref()
@@ -272,9 +308,10 @@ impl CloudHypervisorInner {
             let dev_info: PciDeviceInfo =
                 serde_json::from_str(detail.as_str()).map_err(|e| anyhow!(e))?;
             self.device_ids.insert(device.device_id, dev_info.id);
+            block_dev.config.pci_path = Some(Self::clh_pci_info_to_path(dev_info.bdf.as_str())?);
         }
 
-        Ok(())
+        Ok(DeviceType::Block(block_dev))
     }
 
     pub(crate) async fn get_shared_devices(

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

@@ -79,7 +79,7 @@ impl Hypervisor for CloudHypervisor {
         inner.save_vm().await
     }
 
-    async fn add_device(&self, device: DeviceType) -> Result<()> {
+    async fn add_device(&self, device: DeviceType) -> Result<DeviceType> {
         let mut inner = self.inner.write().await;
         inner.add_device(device).await
     }

src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs

@@ -389,7 +389,7 @@ impl VfioDevice {
             .get_vfio_device_vendor(&dev_bdf)
             .context("get property device and vendor failed")?;
 
-        let mut vfio_dev = HostDevice {
+        let vfio_dev = HostDevice {
             bus_slot_func: dev_bdf.clone(),
             device_vendor: Some(dev_vendor),
             sysfs_path: vfio_dev_details.1,
@@ -397,18 +397,6 @@ impl VfioDevice {
             ..Default::default()
         };
 
-        // when vfio pci, kata-agent handles with device_options, and its
-        // format: "DDDD:BB:DD.F=<pcipath>"
-        // DDDD:BB:DD.F is the device's PCI address on host
-        // <pcipath> is the device's PCI path in the guest
-        if self.bus_mode == VfioBusMode::PCI {
-            let pci_path =
-                generate_guest_pci_path(dev_bdf.clone()).context("generate pci path failed")?;
-            vfio_dev.guest_pci_path = Some(pci_path.clone());
-            self.device_options
-                .push(format!("0000:{}={}", dev_bdf, pci_path.convert_to_string()));
-        }
-
         Ok(vfio_dev)
     }
 
@@ -493,14 +481,43 @@ impl Device for VfioDevice {
         }
 
         // do add device for vfio deivce
-        if let Err(e) = h.add_device(DeviceType::Vfio(self.clone())).await {
-            self.decrease_attach_count().await?;
+        match h.add_device(DeviceType::Vfio(self.clone())).await {
+            Ok(dev) => {
+                // Update device info with the one received from device attach
+                if let DeviceType::Vfio(vfio) = dev {
+                    self.config = vfio.config;
+                    self.devices = vfio.devices;
+                }
 
-            return Err(e);
+                if self.bus_mode == VfioBusMode::PCI {
+                    for hostdev in self.devices.iter_mut() {
+                        if hostdev.guest_pci_path.is_none() {
+                            // guest_pci_path may be empty for certain hypervisors such as
+                            // dragonball
+                            hostdev.guest_pci_path = Some(
+                                generate_guest_pci_path(hostdev.bus_slot_func.clone())
+                                    .map_err(|e| anyhow!("generate pci path failed: {:?}", e))?,
+                            );
+                        }
+
+                        // Safe to call unwrap here because of previous assignment.
+                        let pci_path = hostdev.guest_pci_path.clone().unwrap();
+                        self.device_options.push(format!(
+                            "0000:{}={}",
+                            hostdev.bus_slot_func.clone(),
+                            pci_path.convert_to_string()
+                        ));
+                    }
                 }
 
                 Ok(())
             }
+            Err(e) => {
+                self.decrease_attach_count().await?;
+                return Err(e);
+            }
+        }
+    }
 
     async fn detach(&mut self, h: &dyn hypervisor) -> Result<Option<u64>> {
         if self

src/runtime-rs/crates/hypervisor/src/device/driver/virtio_blk.rs

@@ -7,6 +7,7 @@
 use crate::device::Device;
 use crate::device::DeviceType;
 use crate::Hypervisor as hypervisor;
+use crate::PciPath;
 use anyhow::{anyhow, Context, Result};
 use async_trait::async_trait;
 
@@ -39,6 +40,9 @@ pub struct BlockConfig {
     /// device path in guest
     pub virt_path: String,
 
+    /// pci path is the slot at which the drive is attached
+    pub pci_path: Option<PciPath>,
+
     /// device attach count
     pub attach_count: u64,
 
@@ -78,11 +82,20 @@ impl Device for BlockDevice {
         {
             return Ok(());
         }
-        if let Err(e) = h.add_device(DeviceType::Block(self.clone())).await {
+
+        match h.add_device(DeviceType::Block(self.clone())).await {
+            Ok(dev) => {
+                // Update device info with the one received from device attach
+                if let DeviceType::Block(blk) = dev {
+                    self.config = blk.config;
+                }
+                Ok(())
+            }
+            Err(e) => {
                 self.decrease_attach_count().await?;
                 return Err(e);
             }
-        return Ok(());
+        }
     }
 
     async fn detach(&mut self, h: &dyn hypervisor) -> Result<Option<u64>> {

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

@@ -10,7 +10,7 @@ use crate::device::driver::vhost_user_blk::VhostUserBlkDevice;
 use crate::{
     BlockConfig, BlockDevice, HybridVsockConfig, HybridVsockDevice, Hypervisor as hypervisor,
     NetworkConfig, NetworkDevice, ShareFsDevice, ShareFsDeviceConfig, ShareFsMountConfig,
-    ShareFsMountDevice, VfioConfig, VfioDevice, VhostUserConfig, VsockConfig, VsockDevice,
+    ShareFsMountDevice, VfioConfig, VfioDevice, VhostUserConfig, VsockConfig,
 };
 use anyhow::Result;
 use async_trait::async_trait;
@@ -31,7 +31,7 @@ pub enum DeviceConfig {
     HybridVsockCfg(HybridVsockConfig),
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub enum DeviceType {
     Block(BlockDevice),
     VhostUserBlk(VhostUserBlkDevice),
@@ -40,7 +40,6 @@ pub enum DeviceType {
     ShareFs(ShareFsDevice),
     ShareFsMount(ShareFsMountDevice),
     HybridVsock(HybridVsockDevice),
-    Vsock(VsockDevice),
 }
 
 impl fmt::Display for DeviceType {

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

@@ -74,9 +74,6 @@ impl DragonballInner {
             DeviceType::ShareFsMount(sharefs_mount) => self
                 .add_share_fs_mount(&sharefs_mount.config)
                 .context("add share fs mount"),
-            DeviceType::Vsock(_) => {
-                todo!()
-            }
         }
     }
 

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

@@ -92,9 +92,12 @@ impl Hypervisor for Dragonball {
         inner.resize_vcpu(old_vcpus, new_vcpus).await
     }
 
-    async fn add_device(&self, device: DeviceType) -> Result<()> {
+    async fn add_device(&self, device: DeviceType) -> Result<DeviceType> {
         let mut inner = self.inner.write().await;
-        inner.add_device(device).await
+        match 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/lib.rs

@@ -85,7 +85,7 @@ pub trait Hypervisor: std::fmt::Debug + Send + Sync {
     async fn resize_vcpu(&self, old_vcpus: u32, new_vcpus: u32) -> Result<(u32, u32)>; // returns (old_vcpus, new_vcpus)
 
     // device manager
-    async fn add_device(&self, device: DeviceType) -> Result<()>;
+    async fn add_device(&self, device: DeviceType) -> Result<DeviceType>;
     async fn remove_device(&self, device: DeviceType) -> Result<()>;
 
     // utils

src/runtime-rs/crates/hypervisor/src/qemu/inner.rs

@@ -146,9 +146,9 @@ use crate::device::DeviceType;
 
 // device manager part of Hypervisor
 impl QemuInner {
-    pub(crate) async fn add_device(&mut self, device: DeviceType) -> Result<()> {
+    pub(crate) async fn add_device(&mut self, device: DeviceType) -> Result<DeviceType> {
         info!(sl!(), "QemuInner::add_device() {}", device);
-        todo!()
+        Ok(device)
     }
 
     pub(crate) async fn remove_device(&mut self, device: DeviceType) -> Result<()> {

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

@@ -74,7 +74,7 @@ impl Hypervisor for Qemu {
         inner.save_vm().await
     }
 
-    async fn add_device(&self, device: DeviceType) -> Result<()> {
+    async fn add_device(&self, device: DeviceType) -> Result<DeviceType> {
         let mut inner = self.inner.write().await;
         inner.add_device(device).await
     }

src/runtime-rs/crates/resource/src/manager_inner.rs

@@ -297,8 +297,16 @@ impl ResourceManagerInner {
                     // create block device for kata agent,
                     // if driver is virtio-blk-pci, the id will be pci address.
                     if let DeviceType::Block(device) = device_info {
+                        // The following would work for drivers virtio-blk-pci and mmio.
+                        // Once scsi support is added, need to handle scsi identifiers.
+                        let id = if let Some(pci_path) = device.config.pci_path {
+                            pci_path.convert_to_string()
+                        } else {
+                            device.config.virt_path.clone()
+                        };
+
                         let agent_device = Device {
-                            id: device.config.virt_path.clone(),
+                            id,
                             container_path: d.path.clone(),
                             field_type: device.config.driver_option,
                             vm_path: device.config.virt_path,

src/runtime-rs/crates/resource/src/share_fs/share_virtio_fs.rs

@@ -89,9 +89,16 @@ pub(crate) async fn setup_inline_virtiofs(id: &str, h: &dyn Hypervisor) -> Resul
             prefetch_list_path: None,
         },
     };
-    h.add_device(DeviceType::ShareFsMount(virtio_fs))
+
+    let result = h
+        .add_device(DeviceType::ShareFsMount(virtio_fs))
         .await
-        .with_context(|| format!("fail to attach passthrough fs {:?}", source))
+        .with_context(|| format!("fail to attach passthrough fs {:?}", source));
+
+    match result {
+        Ok(_) => Ok(()),
+        Err(e) => Err(e),
+    }
 }
 
 pub async fn rafs_mount(