Skip to content

Commit

Permalink
virtcontainers: Do not lazy-attach devices
Browse files Browse the repository at this point in the history 
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.

fixes kata-containers#2664

[1] kata-containers#2461

Signed-off-by: Julio Montes <julio.montes@intel.com>
  • Loading branch information
@dgibson
Julio Montes authored and dgibson committed Nov 4, 2020
1 parent 39842b6 commit 0a9be95
Show file tree
Hide file tree
Showing 7 changed files with 17 additions and 216 deletions.
54 changes: 5 additions & 49 deletions virtcontainers/container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -886,38 +886,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
Expand All @@ -930,17 +901,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
Expand Down Expand Up @@ -1443,15 +1403,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
}
Expand Down
16 changes: 0 additions & 16 deletions virtcontainers/device/drivers/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
11 changes: 10 additions & 1 deletion virtcontainers/device/drivers/vfio.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 0 additions & 2 deletions virtcontainers/device/manager/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
101 changes: 0 additions & 101 deletions virtcontainers/device/manager/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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
Expand Down
45 changes: 0 additions & 45 deletions virtcontainers/device/manager/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
package manager

import (
"io/ioutil"
"os"
"testing"

"github.com/kata-containers/runtime/virtcontainers/device/config"
Expand Down Expand Up @@ -105,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 := 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)
}
}
4 changes: 2 additions & 2 deletions virtcontainers/sandbox_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,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()
Expand Down Expand Up @@ -629,7 +629,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()
Expand Down

0 comments on commit 0a9be95

Please # to comment.