From 77c29bfd3bf13f8f8c0e28d9f9ad6055bbfe3b59 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Tue, 23 Nov 2021 13:57:59 +0100 Subject: [PATCH] container: Remove VFIO lazy attach handling With the recently added VFIO fixes and support, we should not need that anymore. Fixes #3108 Signed-off-by: Samuel Ortiz --- src/runtime/virtcontainers/container.go | 50 ++------- .../virtcontainers/device/manager/utils.go | 101 ------------------ .../device/manager/utils_test.go | 44 -------- src/runtime/virtcontainers/sandbox_test.go | 4 +- 4 files changed, 8 insertions(+), 191 deletions(-) diff --git a/src/runtime/virtcontainers/container.go b/src/runtime/virtcontainers/container.go index 87a2a23a50..13ec548553 100644 --- a/src/runtime/virtcontainers/container.go +++ b/src/runtime/virtcontainers/container.go @@ -904,38 +904,11 @@ func (c *Container) create(ctx context.Context) (err error) { } } - var ( - machineType = c.sandbox.config.HypervisorConfig.HypervisorMachineType - normalAttachedDevs []ContainerDevice //for q35: normally attached devices - delayAttachedDevs []ContainerDevice //for q35: delay attached devices, for example, large bar space device - ) - // Fix: https://github.com/kata-containers/runtime/issues/2460 - if machineType == QemuQ35 { - // add Large Bar space device to delayAttachedDevs - for _, device := range c.devices { - var isLargeBarSpace bool - isLargeBarSpace, err = manager.IsVFIOLargeBarSpaceDevice(device.ContainerPath) - if err != nil { - return - } - if isLargeBarSpace { - delayAttachedDevs = append(delayAttachedDevs, device) - } else { - normalAttachedDevs = append(normalAttachedDevs, device) - } - } - } else { - normalAttachedDevs = c.devices - } - c.Logger().WithFields(logrus.Fields{ - "machine_type": machineType, - "devices": normalAttachedDevs, - }).Info("normal attach devices") - if len(normalAttachedDevs) > 0 { - if err = c.attachDevices(ctx, normalAttachedDevs); err != nil { - return - } + "devices": c.devices, + }).Info("Attach devices") + if err = c.attachDevices(ctx); err != nil { + return } // Deduce additional system mount info that should be handled by the agent @@ -948,17 +921,6 @@ func (c *Container) create(ctx context.Context) (err error) { } c.process = *process - // lazy attach device after createContainer for q35 - if machineType == QemuQ35 && len(delayAttachedDevs) > 0 { - c.Logger().WithFields(logrus.Fields{ - "machine_type": machineType, - "devices": delayAttachedDevs, - }).Info("lazy attach devices") - if err = c.attachDevices(ctx, delayAttachedDevs); err != nil { - return - } - } - if err = c.setContainerState(types.StateReady); err != nil { return } @@ -1398,7 +1360,7 @@ func (c *Container) removeDrive(ctx context.Context) (err error) { return nil } -func (c *Container) attachDevices(ctx context.Context, devices []ContainerDevice) error { +func (c *Container) attachDevices(ctx context.Context) error { // there's no need to do rollback when error happens, // because if attachDevices fails, container creation will fail too, // and rollbackFailingContainerCreation could do all the rollbacks @@ -1406,7 +1368,7 @@ func (c *Container) attachDevices(ctx context.Context, devices []ContainerDevice // since devices with large bar space require delayed attachment, // the devices need to be split into two lists, normalAttachedDevs and delayAttachedDevs. // so c.device is not used here. See issue https://github.com/kata-containers/runtime/issues/2460. - for _, dev := range devices { + for _, dev := range c.devices { if err := c.sandbox.devManager.AttachDevice(ctx, dev.ID, c.sandbox); err != nil { return err } diff --git a/src/runtime/virtcontainers/device/manager/utils.go b/src/runtime/virtcontainers/device/manager/utils.go index ade0b6c9d3..61488ef9fd 100644 --- a/src/runtime/virtcontainers/device/manager/utils.go +++ b/src/runtime/virtcontainers/device/manager/utils.go @@ -7,16 +7,10 @@ package manager import ( - "fmt" - "os" "path/filepath" - "strconv" "strings" - "github.com/sirupsen/logrus" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/drivers" ) const ( @@ -42,101 +36,6 @@ func isBlock(devInfo config.DeviceInfo) bool { return devInfo.DevType == "b" } -// IsVFIOLargeBarSpaceDevice checks if the device is a large bar space device. -func IsVFIOLargeBarSpaceDevice(hostPath string) (bool, error) { - if !isVFIO(hostPath) { - return false, nil - } - - iommuDevicesPath := filepath.Join(config.SysIOMMUPath, filepath.Base(hostPath), "devices") - deviceFiles, err := os.ReadDir(iommuDevicesPath) - if err != nil { - return false, err - } - - // Pass all devices in iommu group - for _, deviceFile := range deviceFiles { - vfioDeviceType := drivers.GetVFIODeviceType(deviceFile.Name()) - var isLarge bool - switch vfioDeviceType { - case config.VFIODeviceNormalType: - sysfsResource := filepath.Join(iommuDevicesPath, deviceFile.Name(), "resource") - if isLarge, err = isLargeBarSpace(sysfsResource); err != nil { - return false, err - } - deviceLogger().WithFields(logrus.Fields{ - "device-file": deviceFile.Name(), - "device-type": vfioDeviceType, - "resource": sysfsResource, - "large-bar-space": isLarge, - }).Info("Detect large bar space device") - return isLarge, nil - case config.VFIODeviceMediatedType: - //TODO: support VFIODeviceMediatedType - deviceLogger().WithFields(logrus.Fields{ - "device-file": deviceFile.Name(), - "device-type": vfioDeviceType, - }).Warn("Detect large bar space device is not yet supported for VFIODeviceMediatedType") - default: - deviceLogger().WithFields(logrus.Fields{ - "device-file": deviceFile.Name(), - "device-type": vfioDeviceType, - }).Warn("Incorrect token found when detecting large bar space devices") - } - } - - return false, nil -} - -func isLargeBarSpace(resourcePath string) (bool, error) { - buf, err := os.ReadFile(resourcePath) - if err != nil { - return false, fmt.Errorf("failed to read sysfs resource: %v", err) - } - - // The resource file contains host addresses of PCI resources: - // For example: - // $ cat /sys/bus/pci/devices/0000:04:00.0/resource - // 0x00000000c6000000 0x00000000c6ffffff 0x0000000000040200 - // 0x0000383800000000 0x0000383bffffffff 0x000000000014220c - // Refer: - // resource format: https://github.com/torvalds/linux/blob/63623fd44972d1ed2bfb6e0fb631dfcf547fd1e7/drivers/pci/pci-sysfs.c#L145 - // calculate size : https://github.com/pciutils/pciutils/blob/61ecc14a327de030336f1ff3fea9c7e7e55a90ca/lspci.c#L388 - for rIdx, line := range strings.Split(string(buf), "\n") { - cols := strings.Fields(line) - // start and end columns are required to calculate the size - if len(cols) < 2 { - deviceLogger().WithField("resource-line", line).Debug("not enough columns to calculate PCI size") - continue - } - start, _ := strconv.ParseUint(cols[0], 0, 64) - end, _ := strconv.ParseUint(cols[1], 0, 64) - if start > end { - deviceLogger().WithFields(logrus.Fields{ - "start": start, - "end": end, - }).Debug("start is greater than end") - continue - } - // Use right shift to convert Bytes to GBytes - // This is equivalent to ((end - start + 1) / 1024 / 1024 / 1024) - gbSize := (end - start + 1) >> 30 - deviceLogger().WithFields(logrus.Fields{ - "resource": resourcePath, - "region": rIdx, - "start": cols[0], - "end": cols[1], - "gb-size": gbSize, - }).Debug("Check large bar space device") - //size is large than 4G - if gbSize > 4 { - return true, nil - } - } - - return false, nil -} - // isVhostUserBlk checks if the device is a VhostUserBlk device. func isVhostUserBlk(devInfo config.DeviceInfo) bool { return devInfo.DevType == "b" && devInfo.Major == config.VhostUserBlkMajor diff --git a/src/runtime/virtcontainers/device/manager/utils_test.go b/src/runtime/virtcontainers/device/manager/utils_test.go index 76239340f6..ec518ce7ad 100644 --- a/src/runtime/virtcontainers/device/manager/utils_test.go +++ b/src/runtime/virtcontainers/device/manager/utils_test.go @@ -7,7 +7,6 @@ package manager import ( - "os" "testing" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" @@ -104,46 +103,3 @@ func TestIsVhostUserSCSI(t *testing.T) { assert.Equal(t, d.expected, isVhostUserSCSI) } } - -func TestIsLargeBarSpace(t *testing.T) { - assert := assert.New(t) - - // File not exist - bs, err := isLargeBarSpace("/abc/xyz/123/rgb") - assert.Error(err) - assert.False(bs) - - f, err := os.CreateTemp("", "pci") - assert.NoError(err) - defer f.Close() - defer os.RemoveAll(f.Name()) - - type testData struct { - resourceInfo string - error bool - result bool - } - - for _, d := range []testData{ - {"", false, false}, - {"\t\n\t ", false, false}, - {"abc zyx", false, false}, - {"abc zyx rgb", false, false}, - {"abc\t zyx \trgb", false, false}, - {"0x00015\n0x0013", false, false}, - {"0x00000000c6000000 0x00000000c6ffffff 0x0000000000040200", false, false}, - {"0x0000383bffffffff 0x0000383800000000", false, false}, // start greater than end - {"0x0000383800000000 0x0000383bffffffff", false, true}, - {"0x0000383800000000 0x0000383bffffffff 0x000000000014220c", false, true}, - } { - f.WriteAt([]byte(d.resourceInfo), 0) - bs, err = isLargeBarSpace(f.Name()) - assert.NoError(f.Truncate(0)) - if d.error { - assert.Error(err, d.resourceInfo) - } else { - assert.NoError(err, d.resourceInfo) - } - assert.Equal(d.result, bs, d.resourceInfo) - } -} diff --git a/src/runtime/virtcontainers/sandbox_test.go b/src/runtime/virtcontainers/sandbox_test.go index eb33148577..9c64c8b227 100644 --- a/src/runtime/virtcontainers/sandbox_test.go +++ b/src/runtime/virtcontainers/sandbox_test.go @@ -582,7 +582,7 @@ func TestSandboxAttachDevicesVFIO(t *testing.T) { containers[c.id].sandbox = &sandbox - err = containers[c.id].attachDevices(context.Background(), c.devices) + err = containers[c.id].attachDevices(context.Background()) assert.Nil(t, err, "Error while attaching devices %s", err) err = containers[c.id].detachDevices(context.Background()) @@ -677,7 +677,7 @@ func TestSandboxAttachDevicesVhostUserBlk(t *testing.T) { containers[c.id].sandbox = &sandbox - err = containers[c.id].attachDevices(context.Background(), c.devices) + err = containers[c.id].attachDevices(context.Background()) assert.Nil(t, err, "Error while attaching vhost-user-blk devices %s", err) err = containers[c.id].detachDevices(context.Background())