virtcontainers: Do not lazy-attach devices

virtcontainers/container.go

@@ -885,38 +885,9 @@ func (c *Container) create() (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(normalAttachedDevs); err != nil {
-			return
-		}
+	// Attach devices
+	if err = c.attachDevices(); err != nil {
+		return
 	}
 
 	// Deduce additional system mount info that should be handled by the agent
@@ -929,17 +900,6 @@ func (c *Container) create() (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(delayAttachedDevs); err != nil {
-			return
-		}
-	}
-
 	if !rootless.IsRootless() && !c.sandbox.config.SandboxCgroupOnly {
 		if err = c.cgroupsCreate(); err != nil {
 			return
@@ -1428,15 +1388,11 @@ func (c *Container) removeDrive() (err error) {
 	return nil
 }
 
-func (c *Container) attachDevices(devices []ContainerDevice) error {
+func (c *Container) attachDevices() 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
-
-	// 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(dev.ID, c.sandbox); err != nil {
 			return err
 		}

virtcontainers/device/drivers/utils.go

@@ -106,19 +106,3 @@ func readPCIProperty(propertyPath string) (string, error) {
 	}
 	return strings.Split(string(buf), "\n")[0], nil
 }
-
-func GetVFIODeviceType(deviceFileName string) config.VFIODeviceType {
-	//For example, 0000:04:00.0
-	tokens := strings.Split(deviceFileName, ":")
-	vfioDeviceType := config.VFIODeviceErrorType
-	if len(tokens) == 3 {
-		vfioDeviceType = config.VFIODeviceNormalType
-	} else {
-		//For example, 83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
-		tokens = strings.Split(deviceFileName, "-")
-		if len(tokens) == 5 {
-			vfioDeviceType = config.VFIODeviceMediatedType
-		}
-	}
-	return vfioDeviceType
-}

virtcontainers/device/drivers/vfio.go

@@ -187,8 +187,18 @@ func (device *VFIODevice) Load(ds persistapi.DeviceState) {
 
 // It should implement GetAttachCount() and DeviceID() as api.Device implementation
 // here it shares function from *GenericDevice so we don't need duplicate codes
+
 func getVFIODetails(deviceFileName, iommuDevicesPath string) (deviceBDF, deviceSysfsDev string, vfioDeviceType config.VFIODeviceType, err error) {
-	vfioDeviceType = GetVFIODeviceType(deviceFileName)
+	tokens := strings.Split(deviceFileName, ":")
+	vfioDeviceType = config.VFIODeviceErrorType
+	if len(tokens) == 3 {
+		vfioDeviceType = config.VFIODeviceNormalType
+	} else {
+		tokens = strings.Split(deviceFileName, "-")
+		if len(tokens) == 5 {
+			vfioDeviceType = config.VFIODeviceMediatedType
+		}
+	}
 
 	switch vfioDeviceType {
 	case config.VFIODeviceNormalType:

virtcontainers/device/manager/manager.go

@@ -78,8 +78,6 @@ func NewDeviceManager(blockDriver string, vhostUserStoreEnabled bool, vhostUserS
 		dm.blockDriver = VirtioSCSI
 	}
 
-	drivers.AllPCIeDevs = make(map[string]bool)
-
 	for _, dev := range devices {
 		dm.devices[dev.DeviceID()] = dev
 	}

virtcontainers/device/manager/utils.go

@@ -7,16 +7,10 @@
 package manager
 
 import (
-	"fmt"
-	"io/ioutil"
 	"path/filepath"
-	"strconv"
 	"strings"
 
-	"github.com/sirupsen/logrus"
-
 	"github.com/kata-containers/runtime/virtcontainers/device/config"
-	"github.com/kata-containers/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 := ioutil.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 := ioutil.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.Major == config.VhostUserBlkMajor

virtcontainers/device/manager/utils_test.go

@@ -7,8 +7,6 @@
 package manager
 
 import (
-	"io/ioutil"
-	"os"
 	"testing"
 
 	"github.com/kata-containers/runtime/virtcontainers/device/config"
@@ -91,46 +89,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 := ioutil.TempFile("", "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)
-	}
-}

virtcontainers/sandbox_test.go

@@ -732,7 +732,7 @@ func TestSandboxAttachDevicesVFIO(t *testing.T) {
 
 	containers[c.id].sandbox = &sandbox
 
-	err = containers[c.id].attachDevices(c.devices)
+	err = containers[c.id].attachDevices()
 	assert.Nil(t, err, "Error while attaching devices %s", err)
 
 	err = containers[c.id].detachDevices()
@@ -827,7 +827,7 @@ func TestSandboxAttachDevicesVhostUserBlk(t *testing.T) {
 
 	containers[c.id].sandbox = &sandbox
 
-	err = containers[c.id].attachDevices(c.devices)
+	err = containers[c.id].attachDevices()
 	assert.Nil(t, err, "Error while attaching vhost-user-blk devices %s", err)
 
 	err = containers[c.id].detachDevices()