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

amshinde · 2023-10-21T13:49:04Z

runtime-rs: change hypervisor add_device trait to return device copy

Block(virtio-blk) and vfio devices are currently not handled correctly
by the agent as the agent is not provided with correct PCI paths for
these devices.

The PCI paths for these devices can be inferred from the PCI information
provided by the hypervisor when the device is added.
Hence changing the add_device trait function to return a device copy
with PCI info potentially provided by the hypervisor. This can then be
provided to the agent to correctly detect devices within the VM.

This commit includes implementation for PCI info update for
cloud-hupervisor for virtio-blk devices with stubs provided for other
hypervisors.

This PR fixes virtio-block device addition bug. The following works now:

$ sudo ctr run --runtime "io.containerd.kata.v2" --snapshotter devmapper --rm -t quay.io/prometheus/busybox:latest foo-dm /bin/true;echo $?
0

as well as

sudo ctr run --runtime "io.containerd.kata.v2" --device=/dev/block1 --rm -t quay.io/prometheus/busybox:latest foo-dm /bin/true;echo $?
0

The PR also fixes bugs around vfio device addition. End to end vfio device add now works:

sudo ctr run --runtime "io.containerd.kata.v2" --device=/dev/vfio/{vfio_group} --rm -t quay.io/prometheus/busybox:latest foo-dm /bin/true;echo $?
0

Fixes: #8283

Apokleos · 2023-10-23T03:50:34Z

Hey @amshinde

Nice work on this PR! I'm also working on this feature, and I've submitted a WIP PR as well.

My PR mainly implements the following features:

(1) Due to the different ways that different VMMs handle PCI devices, we hope to provide a general PCIe topology
processing framework that is as compatible as possible with VMMs such as dragonball, cloud-hyperivor, qemu, and fc.
(2) Provide the correct PCI path for PCI devices, including VFIO-PCI devices and virtio-PCI(virtio-blk/vhost-user-blk...) devices.
(3) Fill in the correct correspondence between host path and guest path for kata-agent devices.

Would you be interested in discussing the difference and a merge?
We can compare the pros and cons of both approaches and see if we can come up with a solution that works for everyone.

Let me know what you think.

Thx.

jodh-intel

Thanks @amshinde - a few comments.

Also, although this works with the default pmem driver, it doesn't wfm using DM:

$ sudo ctr run --runtime "io.containerd.kata.v2"  --rm -t quay.io/prometheus/busybox:latest foo /bin/true;echo $?
0

$ sudo ctr run --runtime "io.containerd.kata.v2" --snapshotter devmapper --rm -t quay.io/prometheus/busybox:latest foo-dm /bin/true;echo $?
ctr: failed to create shim task: Others("failed to handler message create container\n\nCaused by:\n    0: create\n    1: handler rootfs\n    2: new block rootfs\n    3: do handle device failed.\n    4: failed to add deivce\n    5: Unexpected PCI address \"0000:00:05.0\" for clh device add"): unknown
1

jodh-intel · 2023-10-23T09:50:24Z

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

@@ -7,16 +7,19 @@
 use super::inner::CloudHypervisorInner;
 use crate::device::DeviceType;
 use crate::BlockDevice;
-use crate::HybridVsockConfig;
+use crate::HybridVsockDevice;
+//use crate::HybridVsockConfig;


Commented out line.

jodh-intel · 2023-10-23T09:50:45Z

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

+            //DeviceType::ShareFs(sharefs) => self.handle_share_fs_device(sharefs.config).await,
+            //DeviceType::HybridVsock(hvsock) => self.handle_hvsock_device(&hvsock.config).await,
+            //DeviceType::Block(block) => self.handle_block_device(block.config).await,
+            //_ => Err(anyhow!("unhandled device: {:?}", device)),


Can you remove these commented out lines.

jodh-intel · 2023-10-23T09:52:02Z

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

+                Ok(())
+            }
+            Err(e) => {
+                self.decrease_attach_count().await?;


If this fails, we'll return the count error, not the original one (line below).

This is the same as previous behaviour. If we want to change this, lets discuss. It could go as part of a separate change.

jodh-intel · 2023-10-23T10:02:12Z

/cc @quanweiZhou, @studychao for input on the Hypervisor trait change on this PR.

amshinde · 2023-10-23T18:40:47Z

Would you be interested in discussing the difference and a merge? We can compare the pros and cons of both approaches and see if we can come up with a solution that works for everyone.

Let me know what you think.

@Apokleos Yes, lets discuss this and figure out a common approach.

amshinde · 2023-10-25T06:00:27Z

hey @Apokleos I just saw your PR #7932, where you introduce update-device trait with similar idea of the hypervisor providing pcie info. Wanted to discuss with you if we could arrive and agree on a way to do this.

jodh-intel

Thanks @amshinde - Tested and wfm.

We still need input from @Apokleos about his PR and also comments on the very minor hypervisor trait tweak on this PR.

lgtm

jodh-intel · 2023-10-25T13:19:14Z

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

+        .with_context(|| format!("fail to attach passthrough fs {:?}", source));
+
+    match result {
+        Ok(__) => Ok(()),


Nit:

Suggested change

Ok(__) => Ok(()),

Ok(_) => Ok(()),

? 😄

fixed :) It still compiled with that!

amshinde · 2023-10-25T20:31:20Z

/test

amshinde · 2023-10-26T01:13:44Z

Also, although this works with the default pmem driver, it doesn't wfm using DM:

This is now fixed with this PR. This PR fixes the device addition bug seen with:

$ sudo ctr run --runtime "io.containerd.kata.v2" --snapshotter devmapper --rm -t quay.io/prometheus/busybox:latest foo-dm /bin/true;echo $?
0

as well as

sudo ctr run --runtime "io.containerd.kata.v2" --device=/dev/block1 --rm -t quay.io/prometheus/busybox:latest foo-dm /bin/true;echo $?
0

Apokleos

Hi @amshinde comments as below.
It's mainly for the removed code generate_guest_pci_path.

Apokleos · 2023-11-01T16:36:52Z

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

-        }
+                if self.bus_mode == VfioBusMode::PCI {
+                    for hostdev in self.devices.iter() {
+                        if hostdev.guest_pci_path.is_some() {


Hi, @amshinde, in case of dragonball, the guest_pci_path is always None, as dragonball doesn't return device info as clh does.
IMO, the None case of hostdev.guest_pci_path needs be handled.

Apokleos · 2023-11-01T16:41:40Z

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

@@ -47,29 +44,6 @@ lazy_static! {
        Arc::new(RwLock::new(HashMap::new()));
 }

-// map host/guest bdf and the mapping saved into `HOST_GUEST_MAP`,
-// and return PciPath.
-pub fn generate_guest_pci_path(bdf: String) -> Result<PciPath> {


The removed generate_guest_pci_path which will help deduce the correct pci Path, it should be reserved.

Ok, I guess this would be used by dragonball. I'll add it back.

@Apokleos I have added generate_guest_pci_path back in case it is usefull for dragonball. Plese take a look.

Thx @amshinde

amshinde · 2023-11-02T03:49:14Z

/test

amshinde · 2023-11-02T03:53:10Z

@Apokleos I have addressed your comments.

pmores · 2023-11-03T10:33:41Z

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

@@ -40,7 +40,7 @@ pub enum DeviceType {
    ShareFs(ShareFsDevice),
    ShareFsMount(ShareFsMountDevice),
    HybridVsock(HybridVsockDevice),
-    Vsock(VsockDevice),
+    //Vsock(VsockDevice),


Why this change?

I have now taken out the Vsock value entirely from the enum. I have added an explanation for it in the commit message. Please take a look at it and let me know if it makes sense.
To summarize, Vsock value was removed as the Vsock device is not really used anywhere nor can it be used in its current state as it does not implement the Device trait. This means that it cannot be attached or detached from the hypervisor. What I can deduce from the code is that one of the reasons is that Vsock does not really implement the Clone trait which looks like a prerequisite for the Device trait, as the async trait functions require Device copies to be passed between them.(Vsock struct currently has a File handle which does not implement Clone, so this would require some rework) This is also the reason that I decided to remove Vsock for now, as the change I am introducing here requires devices to implement Clone.

@amshinde Vsock is required for QEMU support. Could you add it back? Or @pmores would need to add it back in his PR. Vsock device is not fully implemented but it does not mean that it is not needed.

@bergwolf The implementation is still present, I just removed it from the DeviceType enum. With its current implementation it neither supports attach or detach Device traits, meaning it cant really be hotplugged to a hypervisor. @pmores can add it back in the Device list once he adds support for qemu.

@amshinde qemu support is added by PR #8185 which is broken by this change.

@amshinde It would have been greatly appreciated to hold on a bit, so that @pmores had at least a chance to discuss some more before this change was merged.

@gkurz Was not aware of this change drastiaclly breaking @pmores PR. Apologies, I was eager to get this landed to unblock folks to get started with implementing a CI for clh which is borked without this change.I had a look at @pmores PR for qemu support and here are my suggestions to handle the conflicts:
Like I mentioned I have removed VsockDevice from the DeviceType enum list. I think these enums should only specify devices that implement the Device trait (attach/detach) and are passed down by the DeviceManager. None of these two are true for VsockDevice. VsockDevice is a special case device that is useful internally for qemu for agent communication(currently) and will never be handled/created by the Device Manager. Nor will the device really need to implement the Device trait with attach and detach since it does not make sense to hotplug/unplug this device. What we really want is initializing code to get the guest cid.

From what I have seen from a quick walkthrough of @pmores PR the breakage is due to the fact that he currently maintains a device vector of type DeviceType and stores the VsockDevice inside it which would not work due to change in my PR (and my argument above for the change). My suggestion would be since we only need Vsock for agent communication(for now), would be to store this as an expicit reference to VsockDevice in the QemuInner struct and not handle this in the device list that is semantically meant to handle devices that can be attached/detached. This can then be used in the get_agent_socket and get_agent_vsock_dev methods. Something like:

#[derive(Debug)] pub struct QemuInner { /// sandbox id id: String, qemu_process: Option<Child>, config: HypervisorConfig, devices: Vec<DeviceType>, vsock: VsockDevice, }

Hi @amshinde ! I'm aware that the problem this PR creates in the qemu driver can be solved by special-casing, like just about any problem for that matter. I'm not sure though that this is warranted.

I'm not aware of the close semantic ties in the runtime-rs design and current practice that you describe between DeviceType, the Device trait and DeviceManager. Also from Rust standpoint, DeviceType is an enum so no relations among the variant types are implied. In short, I don't see that a DeviceType variant implies cloneability or attach/detach functionality.

Also, could you explain the specific problem that DeviceType::Vsock causes to you? If add_device() now takes and returns a DeviceType by value, I think you can do that with copyable and non-copyable types just as well.

What do other runtime-rs devs think, @bergwolf , @studychao , @Tim-0731-Hzt perhaps?

amshinde · 2023-11-03T23:36:32Z

/test

Block(virtio-blk) and vfio devices are currently not handled correctly by the agent as the agent is not provided with correct PCI paths for these devices. The PCI paths for these devices can be inferred from the PCI information provided by the hypervisor when the device is added. Hence changing the add_device trait function to return a device copy with PCI info potentially provided by the hypervisor. This can then be provided to the agent to correctly detect devices within the VM. This commit includes implementation for PCI info update for cloud-hupervisor for virtio-blk devices with stubs provided for other hypervisors. Removing Vsock from the DeviceType enum as Vsock currently does not implement the Device Trait, it has no attach and detach trait functions among others. Part of the reason is because these functions require Vsock to implement Clone trait as these functions need cloned copies to be passed down the hypervisor. The change introduced for returning a device copy from the add_device hypervisor trait explicitly requires a device to implement Copy trait. Hence removing Vsock from the DeviceType enum for now, as its implementation is incomplete and not currently used. Note, one of the blockers for adding the Clone trait to Vsock is that it currently includes a file handle which cannot be cloned. For Clone and Device Traits to be implemented for Vsock, it requires an implementation change in the future for it to be cloneable. Fixes: kata-containers#8283 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

If PCI path for block device is not empty for a block device, use that as identifier for agent instead of virt path which is valid only for mmio devices. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Remove earlier functionality that tries to assign PCI path to vfio devices from the host assuming pci slots to start from 1. Get this from the hypervisor instead. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Apokleos

Thx @amshinde LGTM!

amshinde · 2023-11-06T08:14:09Z

/test

cmaf

lgtm @amshinde

This basically just a partial revert of PR kata-containers#8284, the only new code is adding Vsock support to DeviceType::try_clone(). At this point, if any DeviceType but Vsock is handled by an add_device() implementation it will be cloned via try_clone() if necessary as before. If Vsock is ever passed to add_device() the call will properly return an error since Vsock devices apparently aren't meant to be handled by add_device() anyway. Fixes: kata-containers#8413 Signed-off-by: Pavel Mores <pmores@redhat.com>

amshinde requested review from Tim-0731-Hzt and jodh-intel October 21, 2023 13:49

katacontainersbot added the size/large Task of significant size label Oct 21, 2023

amshinde requested review from studychao and bergwolf October 21, 2023 13:49

amshinde added the wip Work in Progress (PR incomplete - needs more work or rework) label Oct 21, 2023

jodh-intel reviewed Oct 23, 2023

View reviewed changes

amshinde removed the wip Work in Progress (PR incomplete - needs more work or rework) label Oct 25, 2023

amshinde mentioned this pull request Oct 25, 2023

runtime-rs: bringing virtio-fs device in device-manager #7932

Merged

amshinde requested a review from quanweiZhou October 25, 2023 06:11

amshinde force-pushed the runtime-rs-update-device-pci-info branch 2 times, most recently from fb382a4 to dce711c Compare October 25, 2023 08:13

jodh-intel approved these changes Oct 25, 2023

View reviewed changes

amshinde force-pushed the runtime-rs-update-device-pci-info branch from dce711c to 46c6795 Compare October 25, 2023 20:23

amshinde added the ok-to-test label Oct 25, 2023

amshinde requested a review from lifupan October 26, 2023 21:30

Apokleos reviewed Nov 1, 2023

View reviewed changes

amshinde force-pushed the runtime-rs-update-device-pci-info branch from 46c6795 to ae321c1 Compare November 2, 2023 03:42

pmores reviewed Nov 3, 2023

View reviewed changes

amshinde force-pushed the runtime-rs-update-device-pci-info branch 2 times, most recently from 444b5c1 to f665869 Compare November 3, 2023 23:36

amshinde added 3 commits November 5, 2023 21:59

runtime-rs: Provide PCI path to the agent for virtio-block

c3ce6a1

If PCI path for block device is not empty for a block device, use that as identifier for agent instead of virt path which is valid only for mmio devices. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

runtime-rs: Use PCI path from hypervisor for vfio devices

036b778

Remove earlier functionality that tries to assign PCI path to vfio devices from the host assuming pci slots to start from 1. Get this from the hypervisor instead. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

amshinde force-pushed the runtime-rs-update-device-pci-info branch from f665869 to 036b778 Compare November 6, 2023 06:00

Apokleos approved these changes Nov 6, 2023

View reviewed changes

cmaf approved these changes Nov 6, 2023

View reviewed changes

amshinde merged commit 3b2fb6a into kata-containers:main Nov 6, 2023
137 of 142 checks passed

pmores mentioned this pull request Nov 9, 2023

Fix PR #8284 #8414

Closed

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

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

Conversation

amshinde commented Oct 21, 2023 • edited

Apokleos commented Oct 23, 2023

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Oct 23, 2023

amshinde commented Oct 23, 2023

amshinde commented Oct 25, 2023

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshinde commented Oct 25, 2023

amshinde commented Oct 26, 2023

Apokleos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshinde commented Nov 2, 2023

amshinde commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshinde Nov 6, 2023 • edited

Choose a reason for hiding this comment

pmores Nov 7, 2023 • edited

Choose a reason for hiding this comment

amshinde commented Nov 3, 2023

Apokleos left a comment

Choose a reason for hiding this comment

amshinde commented Nov 6, 2023

cmaf left a comment

Choose a reason for hiding this comment

amshinde commented Oct 21, 2023 •

edited

amshinde Nov 6, 2023 •

edited

pmores Nov 7, 2023 •

edited