virtcontainers: Do not lazy-attach devices by devimc · Pull Request #2670 · kata-containers/runtime

devimc · 2020-05-07T20:43:09Z

The "lazy attach" mechanism [1] was added to hotplugs LBS (Large BAR space)
devices after re-scanning the PCI bus, fixing LBS hotplug in kata containers.
Since PCI rescan is removed in kata-containers/agent#782, lazy attach is not
longer needed.

Depends-on: github.com/kata-containers/agent#782
fixes #2664

[1] #2461

Signed-off-by: Julio Montes julio.montes@intel.com

PCI bus rescan code was added long time ago in Clear Containers due to lack of ACPI support in QEMU 2.9 + q35 [1]. Now this code is messing up PCIe hotplug in Kata Containers. A workaround to this issue is the "lazy attach" mechanism [2] that hotplugs LBS (Large BAR space) devices after re-scanning the PCI bus, unfourtunately some non-LBS devices are being affected too, for instance SR-IOV devices. It would not make sense to lazy-attach non-LBS devices because kata will end up lazy-attaching all the devices, having said that, the PCI bus rescan code and the "lazy attach" mechanism should be removed Depends-on: github.com/kata-containers/runtime#2670 fixes kata-containers#781 fixes kata-containers/runtime#2664 [1] clearcontainers/agent#139 [2] kata-containers/runtime#2461 Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc · 2020-05-07T20:44:51Z

/test

The "lazy attach" mechanism [1] was added to hotplugs LBS (Large BAR space) devices after re-scanning the PCI bus, fixing LBS hotplug in kata containers. Since PCI rescan is removed in kata-containers/agent#782, lazy attach is not longer needed. Depends-on: github.com/kata-containers/agent#782 fixes kata-containers#2664 [1] kata-containers#2461 Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc · 2020-05-08T00:35:25Z

/test

codecov · 2020-05-08T02:08:27Z

Codecov Report

Merging #2670 into master will increase coverage by 4.96%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2670      +/-   ##
==========================================
+ Coverage   45.58%   50.55%   +4.96%     
==========================================
  Files         118      118              
  Lines       17131    17074      -57     
==========================================
+ Hits         7810     8631     +821     
+ Misses       8456     7388    -1068     
- Partials      865     1055     +190

fidencio

This whole re-scan / lazy-attach scenario is rather complicated, isn't it?

Here, similarly to what I've done for kata-containers/agent#782, as the code and the concept do look good, I'll "Approve" the PR, when @amorenoz finishes his tests, using again the AaaS ("Ack-as-a-Service") concept. :-)

fidencio · 2020-05-08T08:37:49Z

Btw, before we have this merged, would be possible to also have an opinion from @Jimmy-Xu that it doesn't regress on their use case?

PCI bus rescan code was added long time ago in Clear Containers due to lack of ACPI support in QEMU 2.9 + q35 [1]. Now this code is messing up PCIe hotplug in Kata Containers. A workaround to this issue is the "lazy attach" mechanism [2] that hotplugs LBS (Large BAR space) devices after re-scanning the PCI bus, unfourtunately some non-LBS devices are being affected too, for instance SR-IOV devices. It would not make sense to lazy-attach non-LBS devices because kata will end up lazy-attaching all the devices, having said that, the PCI bus rescan code and the "lazy attach" mechanism should be removed Depends-on: github.com/kata-containers/runtime#2670 fixes kata-containers#781 fixes kata-containers/runtime#2664 [1] clearcontainers/agent#139 [2] kata-containers/runtime#2461 Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc · 2020-05-08T18:04:47Z

all green, just waiting for review

bpradipt · 2020-05-10T11:59:11Z

I took this PR and the associated agent PR and here is a summary of my verification results
Test environment
Intel system with Fedora 31, kernel-5.6.8-200.fc31.x86_64, cgroup v2 disabled and intel_iommu is turned on

Device for passthrough

IOMMU Group 16:
        16:00.0 Ethernet controller [0200]: Emulex Corporation OneConnect 10Gb NIC (be3) [19a2:0710] (rev 02)

# lspci -nnk -d 19a2:0710
[snip]
16:00.0 Ethernet controller [0200]: Emulex Corporation OneConnect 10Gb NIC (be3) [19a2:0710] (rev 02)
        Subsystem: Emulex Corporation Device [10df:e731]
        Kernel driver in use: vfio-pci
        Kernel modules: be2net
[snip]

# ls -l /dev/vfio/
total 0
crw-------. 1 root root 237,   0 May 10 00:13 16
crw-rw-rw-. 1 root root  10, 196 May 10 00:13 vfio

Kata container execution command

podman run -it --runtime=/usr/local/bin/kata-runtime --device /dev/vfio/16 fedora bash

Kata configuration-1

machine_type = "pc"
#hotplug_vfio_on_root_bus = true
#pcie_root_port = 2

This config works fine and dmesg inside the container/Kata-VM shows the PCI device

[snip]

[    2.066592] pci 0000:01:01.0: [19a2:0710] type 00 class 0x020000
[    2.067263] pci 0000:01:01.0: reg 0x10: [mem 0x00000000-0x00003fff 64bit]
[    2.067647] pci 0000:01:01.0: reg 0x18: [mem 0x00000000-0x0001ffff 64bit]
[    2.068060] pci 0000:01:01.0: reg 0x20: [mem 0x00000000-0x0001ffff 64bit]
[    2.069305] pci 0000:01:01.0: PME# supported from D3hot D3cold
[    2.069784] pci 0000:01:01.0: 0.000 Gb/s available PCIe bandwidth, limited by Unknown speed x0 link at 0000:00:02.0 (capable of 32.000 Gb/s with 5 GT/s x8 link)
[    2.070412] pci 0000:01:01.0: BAR 2: assigned [mem 0xfe400000-0xfe41ffff 64bit]
[    2.070836] pci 0000:01:01.0: BAR 4: assigned [mem 0xfe420000-0xfe43ffff 64bit]
[    2.071190] pci 0000:01:01.0: BAR 0: assigned [mem 0xfe440000-0xfe443fff 64bit]

[snip]

Kata configuration-2

machine_type = "q35"
hotplug_vfio_on_root_bus = true
pcie_root_port = 2

This configuration fails with the following error

Bus 'pcie.0' does not support hotplugging: OCI runtime error

This should have worked but I'm not sure at this moment why it's failing in my setup. Will continue debugging.

jodh-intel · 2020-05-11T08:11:46Z

+
 func getVFIODetails(deviceFileName, iommuDevicesPath string) (deviceBDF, deviceSysfsDev string, vfioDeviceType config.VFIODeviceType, err error) {
-	vfioDeviceType = GetVFIODeviceType(deviceFileName)
+	tokens := strings.Split(deviceFileName, ":")


It might be worth putting this logic into a new function that can be unit-tested with various numbers of colons and dashes in deviceFileName.

func getVFIODeviceType(deviceFileName string) string

amorenoz · 2020-05-11T08:27:39Z

Thanks @bpradipt for testing this.
I reported two issues squashed in one, that may have caused some confusion, sorry about that. I have now split them. Your error is now reported in #2678.
As for this PR (and the associated agent PR), I have also verified that fixes the problem in bridge hotplug.

fidencio

Approving based on both @amorenoz and @bpradipt feedback!

devimc · 2020-05-13T12:31:03Z

/test-vfio

PCI bus rescan code was added long time ago in Clear Containers due to lack of ACPI support in QEMU 2.9 + q35 [1]. Now this code is messing up PCIe hotplug in Kata Containers. A workaround to this issue is the "lazy attach" mechanism [2] that hotplugs LBS (Large BAR space) devices after re-scanning the PCI bus, unfourtunately some non-LBS devices are being affected too, for instance SR-IOV devices. It would not make sense to lazy-attach non-LBS devices because kata will end up lazy-attaching all the devices, having said that, the PCI bus rescan code and the "lazy attach" mechanism should be removed Depends-on: github.com/kata-containers/runtime#2670 fixes kata-containers#781 fixes kata-containers/runtime#2664 [1] clearcontainers/agent#139 [2] kata-containers/runtime#2461 Signed-off-by: Julio Montes <julio.montes@intel.com>

cmaf · 2020-09-30T00:05:44Z

/test-vfio

cmaf · 2020-09-30T00:25:12Z

@devimc not sure why but this PR still has a pending DCO and WIP check. Do we still want to merge it?

dgibson · 2020-09-30T03:52:07Z

@cmaf I think this is superseded by #2981

devimc · 2020-09-30T12:36:06Z

closing. #2981 includes this

devimc added the do-not-merge PR has problems or depends on another label May 7, 2020

devimc mentioned this pull request May 7, 2020

device: Do not rescan PCI bus kata-containers/agent#782

Closed

devimc force-pushed the topic/virtcontainers/doNotLazyAttach branch from 39d5113 to 0b1c99d Compare May 8, 2020 00:34

fidencio reviewed May 8, 2020

View reviewed changes

devimc removed the do-not-merge PR has problems or depends on another label May 8, 2020

jodh-intel approved these changes May 11, 2020

View reviewed changes

fidencio approved these changes May 11, 2020

View reviewed changes

Jimmy-Xu self-requested a review June 2, 2020 02:46

Jimmy-Xu approved these changes Jun 2, 2020

View reviewed changes

devimc closed this Sep 30, 2020

Conversation

devimc commented May 7, 2020

Uh oh!

devimc commented May 7, 2020

Uh oh!

devimc commented May 8, 2020

Uh oh!

codecov Bot commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fidencio left a comment

Choose a reason for hiding this comment

Uh oh!

fidencio commented May 8, 2020

Uh oh!

devimc commented May 8, 2020

Uh oh!

bpradipt commented May 10, 2020

Uh oh!

jodh-intel May 11, 2020

Choose a reason for hiding this comment

Uh oh!

amorenoz commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fidencio left a comment

Choose a reason for hiding this comment

Uh oh!

devimc commented May 13, 2020

Uh oh!

cmaf commented Sep 30, 2020

Uh oh!

cmaf commented Sep 30, 2020

Uh oh!

dgibson commented Sep 30, 2020

Uh oh!

devimc commented Sep 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov Bot commented May 8, 2020 •

edited

Loading

amorenoz commented May 11, 2020 •

edited

Loading