Provide information to agent to let it safely wait for VFIO devices to complete hotplug

Gopkg.toml

@@ -48,7 +48,7 @@
 
 [[constraint]]
   name = "github.com/intel/govmm"
-  revision = "547a8518098aaa71c5d5d7a370f211e00161016b"
+  revision = "6fa954a506730087919a5272ed2a9e4515cd7bc8"
 
 [[constraint]]
   name = "github.com/kata-containers/agent"

virtcontainers/clh.go

@@ -444,15 +444,33 @@ func (clh *cloudHypervisor) hotplugAddBlockDevice(drive *config.BlockDrive) erro
 	return err
 }
 
-func (clh *cloudHypervisor) hotPlugVFIODevice(device config.VFIODev) error {
+func (clh *cloudHypervisor) hotPlugVFIODevice(device *config.VFIODev) error {
 	cl := clh.client()
 	ctx, cancel := context.WithTimeout(context.Background(), clhHotPlugAPITimeout*time.Second)
 	defer cancel()
 
-	_, _, err := cl.VmAddDevicePut(ctx, chclient.VmAddDevice{Path: device.SysfsDev, Id: device.ID})
+	pciInfo, _, err := cl.VmAddDevicePut(ctx, chclient.VmAddDevice{Path: device.SysfsDev, Id: device.ID})
 	if err != nil {
-		err = fmt.Errorf("Failed to hotplug device %+v %s", device, openAPIClientError(err))
+		return fmt.Errorf("Failed to hotplug device %+v %s", device, openAPIClientError(err))
 	}
+
+	// clh doesn't use bridges, so the PCI path is simply the slot
+	// number of the device.  This will break if clh starts using
+	// bridges (including PCI-E root ports), but so will the clh
+	// API, since there's no way it can reliably a guest Bdf when
+	// bridges are present.
+	tokens := strings.Split(pciInfo.Bdf, ":")
+	if len(tokens) != 3 || tokens[0] != "0000" || tokens[1] != "00" {
+		return fmt.Errorf("Unexpected PCI address %q from clh hotplug", pciInfo.Bdf)
+	}
+
+	tokens = strings.Split(tokens[2], ".")
+	if len(tokens) != 2 || tokens[1] != "0" || len(tokens[0]) != 2 {
+		return fmt.Errorf("Unexpected PCI address %q from clh hotplug", pciInfo.Bdf)
+	}
+
+	device.GuestPciPath, err = vcTypes.PciPathFromString(tokens[0])
+
 	return err
 }
 
@@ -466,7 +484,7 @@ func (clh *cloudHypervisor) hotplugAddDevice(devInfo interface{}, devType device
 		return nil, clh.hotplugAddBlockDevice(drive)
 	case vfioDev:
 		device := devInfo.(*config.VFIODev)
-		return nil, clh.hotPlugVFIODevice(*device)
+		return nil, clh.hotPlugVFIODevice(device)
 	default:
 		return nil, fmt.Errorf("cannot hotplug device: unsupported device type '%v'", devType)
 	}

virtcontainers/container.go

@@ -883,38 +883,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
@@ -927,17 +898,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
@@ -1440,15 +1400,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/config/config.go

@@ -223,6 +223,9 @@ type VFIODev struct {
 
 	// Bus of VFIO PCIe device
 	Bus string
+
+	// Guest PCI address as <bridge slot>/<device slot>
+	GuestPciPath vcTypes.PciPath
 }
 
 // RNGDev represents a random number generator device

virtcontainers/device/drivers/utils.go

@@ -92,19 +92,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

@@ -210,7 +210,16 @@ 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.DevType == "b" && devInfo.Major == config.VhostUserBlkMajor