virtcontainers: POC support for coldplugging PCI devices #705

eguzman3 · 2018-09-06T19:33:33Z

Adds support for coldplugging PCI devices at VM boot rather than hotplugging.
Adds two cli config options
pci_coldplug_groups defines a list of IOMMU groups that should be coldplugged when supplied via --device=/dev/vfio/XX
pci_coldplug_device_opts defines a list of per-device vfio-pci options to be passed on the qemu cli when coldplugging these devices

Fixes: #704

Signed-off-by: Edward Guzman [email protected]

Adds support for coldplugging PCI devices at VM boot rather than hotplugging. Fixes: kata-containers#704 Signed-off-by: Edward Guzman <[email protected]>

katacontainersbot · 2018-09-06T19:48:08Z

PSS Measurement:
Qemu: 167170 KB
Proxy: 4033 KB
Shim: 8717 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003316 KB

opendev-zuul · 2018-09-06T21:32:42Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : TIMED_OUT in 1h 01m 25s

caoruidong · 2018-09-07T01:28:17Z

Thank you for raising this! But if you want to modify govmm codes, you should also raise a PR in https://github.com/intel/govmm. When that PR gets merged, change this PR to 2 commits. The first one uses dep to import vendor changes, the second one is changes in this repo.

jodh-intel

Thanks for raising @eguzman3! I appreciate this is a POC but have a few questions.

Ideally, we'd also have some unit tests to supplement the code changes and as noted the vendor changes would need to be made upstream first.

jodh-intel · 2018-09-07T07:28:21Z

virtcontainers/pkg/oci/utils.go

+}
+
+// getBDF returns the BDF of pci device
+// Expected input strng format is [<domain>]:[<bus>][<slot>].[<func>] eg. 0000:02:10.0


Shouldn't that be (missing colon after [<bus>]?):

// Expected input strng format is [<domain>]:[<bus>]:[<slot>].[<func>] eg. 0000:02:10.0

jodh-intel · 2018-09-07T07:30:55Z

virtcontainers/pkg/oci/utils.go

+}
+
+func checkForPCIColdPlug(device spec.LinuxDevice, hConfig *vc.HypervisorConfig) bool {
+	if !strings.Contains(device.Path, "/dev/vfio/") {


Should this be strings.HasPrefix()?

jodh-intel · 2018-09-07T07:31:20Z

virtcontainers/pkg/oci/utils.go

+	}
+
+	pathTokens := strings.Split(device.Path, "/")
+	if len(pathTokens) != 4 {


Could you add a comment documenting the expected format so this token value is clearer?

jodh-intel · 2018-09-07T07:40:23Z

cli/config/configuration.toml.in

+# A list of parameters to pass to the vfio-pci driver when coldplugging
+# devices. This is a semicolon-separated list of device option lists where
+# a device option list is comma-separated list where the first entry is
+# the device's BDF. All devices that will be coldplugged should be added


I'm finding this a little difficult to read. Could it be changed to something like this without losing the meaning?:

A semi-colon separated list of parameters to pass to the vfio-pci driver when coldplugging
devices. Each list element is itself a comma separated list of options which must start with the BDF of the device to be cold-plugged and optionally followed by one or more device-specific options.

jodh-intel · 2018-09-07T07:43:35Z

virtcontainers/hypervisor.go

+
+	// PCIColdplugDeviceOpts is used to indicate what paramaters
+	// should be passed to vfio-pci for coldplugged devices
+	PCIColdplugDeviceOpts []string


I'm not really clear why PCIColdplugDeviceOpts is separate from PCIColdplugDeviceOpts. How about something like:

type PCIDevice { Name string Options []string } type HypervisorConfig struct { : PCIColdplugDevices []PCIDevice }

Splitting the bdf and its options apart would also solve the issue in checkForPCIColdPlug() I think.

jodh-intel · 2018-09-07T07:44:46Z

virtcontainers/pkg/oci/utils.go

+	tokens := strings.Split(deviceSysStr, ":")
+
+	if len(tokens) != 3 {
+		return "", fmt.Errorf("Incorrect number of tokens found while parsing bdf for device : %s", deviceSysStr)


I'd consider making the 3 a const and adding that to the returned error so we log 3 vs. len(tokens).

jodh-intel · 2018-09-07T07:47:30Z

virtcontainers/pkg/oci/utils.go

+
+	for _, bdf := range bdfs {
+		for _, opts := range hConfig.PCIColdplugDeviceOpts {
+			if strings.Contains(opts, bdf) {


What if a devices options contained a reference to another BDF? If that's possible, this won't be reliable.

WeiZhang555 · 2018-09-10T14:52:59Z

Hi @eguzman3 , thanks for your PR! Can you address comment #704 (comment) ? We need a clear understanding about the scenario.

raravena80 · 2018-09-24T14:54:40Z

Hi, @eguzman3 any updates on this? thx!

jodh-intel · 2018-09-28T10:32:08Z

Ping @eguzman3.

flx42 · 2018-10-02T23:37:42Z

I will try to run some tests on a system we have that has 16 GPUs with a large BAR size:
https://www.nvidia.com/en-us/data-center/dgx-2/
But I'm likely to encounter other issues unrelated to coldplug/hotplug, so if it's not conclusive we will have to table this PR for now.

caoruidong · 2018-11-05T02:17:19Z

@flx42 Do you solve other problems?

raravena80 · 2018-12-18T05:19:31Z

@eguzman3 any updates? (from your weekly kata herder)

flx42 · 2018-12-18T05:22:20Z

Nope, no progress on this side. Thanks for all the feedback in #704, I think we'll reopen if/when needed.

ghost · 2018-12-18T09:57:18Z

/do-not-merge
/close

jodh-intel · 2018-12-19T08:53:58Z

Ok - closing. Let us know if you want this re-opened...

New Firecracker v0.20.0 has changed the host-initiated vsock connection protocol to include a trivial handshake. The new protocol looks like this: - [host] CONNECT <port><LF> - [guest/success] OK <assigned_host_port><LF> Fixes: kata-containers#705 Signed-off-by: Penny Zheng <[email protected]>

This file is mainly used by kata-runtime and missing logging part. We add one new "agent-client" log field here, and Plz notice that, you should turn on the `kata-runtime` debug option to see the output. Fixes: kata-containers#705 Depends-on: github.com/kata-containers#2379 Signed-off-by: Penny Zheng <[email protected]>

virtcontainers: POC support for coldplugging PCI devices

08fbc67

Adds support for coldplugging PCI devices at VM boot rather than hotplugging. Fixes: kata-containers#704 Signed-off-by: Edward Guzman <[email protected]>

jodh-intel reviewed Sep 7, 2018

View reviewed changes

jodh-intel closed this Dec 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virtcontainers: POC support for coldplugging PCI devices #705

virtcontainers: POC support for coldplugging PCI devices #705

eguzman3 commented Sep 6, 2018

katacontainersbot commented Sep 6, 2018

opendev-zuul bot commented Sep 6, 2018

caoruidong commented Sep 7, 2018

jodh-intel left a comment

jodh-intel Sep 7, 2018

jodh-intel Sep 7, 2018

jodh-intel Sep 7, 2018

jodh-intel Sep 7, 2018

jodh-intel Sep 7, 2018

jodh-intel Sep 7, 2018

jodh-intel Sep 7, 2018

WeiZhang555 commented Sep 10, 2018

raravena80 commented Sep 24, 2018

jodh-intel commented Sep 28, 2018

flx42 commented Oct 2, 2018 •

edited

Loading

caoruidong commented Nov 5, 2018

raravena80 commented Dec 18, 2018

flx42 commented Dec 18, 2018

ghost commented Dec 18, 2018

jodh-intel commented Dec 19, 2018

virtcontainers: POC support for coldplugging PCI devices #705

virtcontainers: POC support for coldplugging PCI devices #705

Conversation

eguzman3 commented Sep 6, 2018

katacontainersbot commented Sep 6, 2018

opendev-zuul bot commented Sep 6, 2018

caoruidong commented Sep 7, 2018

jodh-intel left a comment

Choose a reason for hiding this comment

jodh-intel Sep 7, 2018

Choose a reason for hiding this comment

jodh-intel Sep 7, 2018

Choose a reason for hiding this comment

jodh-intel Sep 7, 2018

Choose a reason for hiding this comment

jodh-intel Sep 7, 2018

Choose a reason for hiding this comment

jodh-intel Sep 7, 2018

Choose a reason for hiding this comment

jodh-intel Sep 7, 2018

Choose a reason for hiding this comment

jodh-intel Sep 7, 2018

Choose a reason for hiding this comment

WeiZhang555 commented Sep 10, 2018

raravena80 commented Sep 24, 2018

jodh-intel commented Sep 28, 2018

flx42 commented Oct 2, 2018 • edited Loading

caoruidong commented Nov 5, 2018

raravena80 commented Dec 18, 2018

flx42 commented Dec 18, 2018

ghost commented Dec 18, 2018

jodh-intel commented Dec 19, 2018

flx42 commented Oct 2, 2018 •

edited

Loading