diff --git a/src/runtime/virtcontainers/clh.go b/src/runtime/virtcontainers/clh.go index 95642dd39688..c86bcc28b5ee 100644 --- a/src/runtime/virtcontainers/clh.go +++ b/src/runtime/virtcontainers/clh.go @@ -685,10 +685,15 @@ func (clh *cloudHypervisor) hotPlugVFIODevice(device *config.VFIODev) error { ctx, cancel := context.WithTimeout(context.Background(), clhHotPlugAPITimeout*time.Second) defer cancel() + pciDevice, ok := (*device).(config.VFIOPCIDev) + if !ok { + return fmt.Errorf("VFIO device %+v is not PCI, only PCI is supported in Cloud Hypervisor", device) + } + // Create the clh device config via the constructor to ensure default values are properly assigned clhDevice := *chclient.NewVmAddDevice() - clhDevice.Path = &device.SysfsDev - clhDevice.Id = &device.ID + clhDevice.Path = &pciDevice.SysfsDev + clhDevice.Id = &pciDevice.ID pciInfo, _, err := cl.VmAddDevicePut(ctx, clhDevice) if err != nil { return fmt.Errorf("Failed to hotplug device %+v %s", device, openAPIClientError(err)) @@ -709,7 +714,9 @@ func (clh *cloudHypervisor) hotPlugVFIODevice(device *config.VFIODev) error { return fmt.Errorf("Unexpected PCI address %q from clh hotplug", pciInfo.Bdf) } - device.GuestPciPath, err = vcTypes.PciPathFromString(tokens[0]) + guestPciPath, err := vcTypes.PciPathFromString(tokens[0]) + pciDevice.GuestPciPath = guestPciPath + *device = pciDevice return err } @@ -741,7 +748,7 @@ func (clh *cloudHypervisor) HotplugRemoveDevice(ctx context.Context, devInfo int case BlockDev: deviceID = clhDriveIndexToID(devInfo.(*config.BlockDrive).Index) case VfioDev: - deviceID = devInfo.(*config.VFIODev).ID + deviceID = *(*(devInfo.(*config.VFIODev))).GetID() default: clh.Logger().WithFields(log.Fields{"devInfo": devInfo, "deviceType": devType}).Error("HotplugRemoveDevice: unsupported device") diff --git a/src/runtime/virtcontainers/clh_test.go b/src/runtime/virtcontainers/clh_test.go index 9bfd2e62ee9d..1a8014c01955 100644 --- a/src/runtime/virtcontainers/clh_test.go +++ b/src/runtime/virtcontainers/clh_test.go @@ -410,7 +410,7 @@ func TestCloudHypervisorHotplugRemoveDevice(t *testing.T) { _, err = clh.HotplugRemoveDevice(context.Background(), &config.BlockDrive{}, BlockDev) assert.NoError(err, "Hotplug remove block device expected no error") - _, err = clh.HotplugRemoveDevice(context.Background(), &config.VFIODev{}, VfioDev) + _, err = clh.HotplugRemoveDevice(context.Background(), &config.VFIOPCIDev{}, VfioDev) assert.NoError(err, "Hotplug remove vfio block device expected no error") _, err = clh.HotplugRemoveDevice(context.Background(), nil, NetDev) diff --git a/src/runtime/virtcontainers/device/config/config.go b/src/runtime/virtcontainers/device/config/config.go index 1278895e8e98..899c555908ee 100644 --- a/src/runtime/virtcontainers/device/config/config.go +++ b/src/runtime/virtcontainers/device/config/config.go @@ -234,8 +234,14 @@ const ( VFIOPCIDeviceMediatedType ) -// VFIODev represents a VFIO drive used for hotplugging -type VFIODev struct { +type VFIODev interface { + GetID() *string + GetType() VFIODeviceType + GetSysfsDev() *string +} + +// VFIOPCIDev represents a VFIO PCI device used for hotplugging +type VFIOPCIDev struct { // ID is used to identify this drive in the hypervisor options. ID string @@ -267,6 +273,18 @@ type VFIODev struct { IsPCIe bool } +func (d VFIOPCIDev) GetID() *string { + return &d.ID +} + +func (d VFIOPCIDev) GetType() VFIODeviceType { + return d.Type +} + +func (d VFIOPCIDev) GetSysfsDev() *string { + return &d.SysfsDev +} + // RNGDev represents a random number generator device type RNGDev struct { // ID is used to identify the device in the hypervisor options. diff --git a/src/runtime/virtcontainers/device/drivers/vfio.go b/src/runtime/virtcontainers/device/drivers/vfio.go index c17c0aad7ad4..8ae786e75eae 100644 --- a/src/runtime/virtcontainers/device/drivers/vfio.go +++ b/src/runtime/virtcontainers/device/drivers/vfio.go @@ -86,19 +86,29 @@ func (device *VFIODevice) Attach(ctx context.Context, devReceiver api.DeviceRece if err != nil { return err } - vfio := &config.VFIODev{ - ID: utils.MakeNameID("vfio", device.DeviceInfo.ID+strconv.Itoa(i), maxDevIDSize), - Type: vfioDeviceType, - BDF: deviceBDF, - SysfsDev: deviceSysfsDev, - IsPCIe: isPCIeDevice(deviceBDF), - Class: getPCIDeviceProperty(deviceBDF, PCISysFsDevicesClass), - } - device.VfioDevs = append(device.VfioDevs, vfio) - if vfio.IsPCIe { - vfio.Bus = fmt.Sprintf("%s%d", pcieRootPortPrefix, len(AllPCIeDevs)) - AllPCIeDevs[vfio.BDF] = true + id := utils.MakeNameID("vfio", device.DeviceInfo.ID+strconv.Itoa(i), maxDevIDSize) + + var vfio config.VFIODev + + switch vfioDeviceType { + case config.VFIOPCIDeviceNormalType, config.VFIOPCIDeviceMediatedType: + isPCIe := isPCIeDevice(deviceBDF) + // Do not directly assign to `vfio` -- need to access field still + vfioPCI := config.VFIOPCIDev{ + ID: id, + Type: vfioDeviceType, + BDF: deviceBDF, + SysfsDev: deviceSysfsDev, + IsPCIe: isPCIe, + Class: getPCIDeviceProperty(deviceBDF, PCISysFsDevicesClass), + } + if isPCIe { + vfioPCI.Bus = fmt.Sprintf("%s%d", pcieRootPortPrefix, len(AllPCIeDevs)) + AllPCIeDevs[deviceBDF] = true + } + vfio = vfioPCI } + device.VfioDevs = append(device.VfioDevs, &vfio) } coldPlug := device.DeviceInfo.ColdPlug @@ -181,11 +191,15 @@ func (device *VFIODevice) Save() persistapi.DeviceState { devs := device.VfioDevs for _, dev := range devs { if dev != nil { + bdf := "" + if pciDev, ok := (*dev).(config.VFIOPCIDev); ok { + bdf = pciDev.BDF + } ds.VFIODevs = append(ds.VFIODevs, &persistapi.VFIODev{ - ID: dev.ID, - Type: uint32(dev.Type), - BDF: dev.BDF, - SysfsDev: dev.SysfsDev, + ID: *(*dev).GetID(), + Type: uint32((*dev).GetType()), + BDF: bdf, + SysfsDev: *(*dev).GetSysfsDev(), }) } } @@ -198,12 +212,13 @@ func (device *VFIODevice) Load(ds persistapi.DeviceState) { device.GenericDevice.Load(ds) for _, dev := range ds.VFIODevs { - device.VfioDevs = append(device.VfioDevs, &config.VFIODev{ + var vfioDev config.VFIODev = config.VFIOPCIDev{ ID: dev.ID, Type: config.VFIODeviceType(dev.Type), BDF: dev.BDF, SysfsDev: dev.SysfsDev, - }) + } + device.VfioDevs = append(device.VfioDevs, &vfioDev) } } diff --git a/src/runtime/virtcontainers/kata_agent.go b/src/runtime/virtcontainers/kata_agent.go index e70d94842496..e4143b870524 100644 --- a/src/runtime/virtcontainers/kata_agent.go +++ b/src/runtime/virtcontainers/kata_agent.go @@ -1086,8 +1086,10 @@ func (k *kataAgent) appendVfioDevice(dev ContainerDevice, device api.Device, c * kataDevice.Type = kataVfioPciGuestKernelDevType } - for i, pciDev := range devList { - kataDevice.Options[i] = fmt.Sprintf("0000:%s=%s", pciDev.BDF, pciDev.GuestPciPath) + kataDevice.Options = make([]string, len(devList)) + for i, device := range devList { + pciDevice := (*device).(config.VFIOPCIDev) + kataDevice.Options[i] = fmt.Sprintf("0000:%s=%s", pciDevice.BDF, pciDevice.GuestPciPath) } return kataDevice diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 1417b9c35ca9..b0317c906653 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -1543,7 +1543,7 @@ func (q *qemu) hotplugVFIODevice(ctx context.Context, device *config.VFIODev, op return err } - devID := device.ID + devID := *(*device).GetID() machineType := q.HypervisorConfig().HypervisorMachineType if op == AddDevice { @@ -1560,29 +1560,29 @@ func (q *qemu) hotplugVFIODevice(ctx context.Context, device *config.VFIODev, op // for pc machine type instead of bridge. This is useful for devices that require // a large PCI BAR which is a currently a limitation with PCI bridges. if q.state.HotplugVFIOOnRootBus { - - // In case MachineType is q35, a PCIe device is hotplugged on a PCIe Root Port. - switch machineType { - case QemuQ35: - if device.IsPCIe && q.state.PCIeRootPort <= 0 { - q.Logger().WithField("dev-id", device.ID).Warn("VFIO device is a PCIe device. It's recommended to add the PCIe Root Port by setting the pcie_root_port parameter in the configuration for q35") - device.Bus = "" + switch (*device).GetType() { + case config.VFIOPCIDeviceNormalType, config.VFIOPCIDeviceMediatedType: + // In case MachineType is q35, a PCIe device is hotplugged on a PCIe Root Port. + pciDevice, ok := (*device).(config.VFIOPCIDev) + if !ok { + return fmt.Errorf("VFIO device %+v is not PCI, but its Type said otherwise", device) } - default: - device.Bus = "" - } + switch machineType { + case QemuQ35: + if pciDevice.IsPCIe && q.state.PCIeRootPort <= 0 { + q.Logger().WithField("dev-id", (*device).GetID()).Warn("VFIO device is a PCIe device. It's recommended to add the PCIe Root Port by setting the pcie_root_port parameter in the configuration for q35") + pciDevice.Bus = "" + } + default: + pciDevice.Bus = "" + } + *device = pciDevice - switch device.Type { - case config.VFIOPCIDeviceNormalType: - err = q.qmpMonitorCh.qmp.ExecuteVFIODeviceAdd(q.qmpMonitorCh.ctx, devID, device.BDF, device.Bus, romFile) - case config.VFIOPCIDeviceMediatedType: - if utils.IsAPVFIOMediatedDevice(device.SysfsDev) { - err = q.qmpMonitorCh.qmp.ExecuteAPVFIOMediatedDeviceAdd(q.qmpMonitorCh.ctx, device.SysfsDev) + if pciDevice.Type == config.VFIOPCIDeviceNormalType { + err = q.qmpMonitorCh.qmp.ExecuteVFIODeviceAdd(q.qmpMonitorCh.ctx, devID, pciDevice.BDF, pciDevice.Bus, romFile) } else { - err = q.qmpMonitorCh.qmp.ExecutePCIVFIOMediatedDeviceAdd(q.qmpMonitorCh.ctx, devID, device.SysfsDev, "", device.Bus, romFile) + err = q.qmpMonitorCh.qmp.ExecutePCIVFIOMediatedDeviceAdd(q.qmpMonitorCh.ctx, devID, *(*device).GetSysfsDev(), "", pciDevice.Bus, romFile) } - default: - return fmt.Errorf("Incorrect VFIO device type found") } } else { addr, bridge, err := q.arch.addDeviceToBridge(ctx, devID, types.PCI) @@ -1596,15 +1596,15 @@ func (q *qemu) hotplugVFIODevice(ctx context.Context, device *config.VFIODev, op } }() - switch device.Type { + switch (*device).GetType() { case config.VFIOPCIDeviceNormalType: - err = q.qmpMonitorCh.qmp.ExecutePCIVFIODeviceAdd(q.qmpMonitorCh.ctx, devID, device.BDF, addr, bridge.ID, romFile) - case config.VFIOPCIDeviceMediatedType: - if utils.IsAPVFIOMediatedDevice(device.SysfsDev) { - err = q.qmpMonitorCh.qmp.ExecuteAPVFIOMediatedDeviceAdd(q.qmpMonitorCh.ctx, device.SysfsDev) - } else { - err = q.qmpMonitorCh.qmp.ExecutePCIVFIOMediatedDeviceAdd(q.qmpMonitorCh.ctx, devID, device.SysfsDev, addr, bridge.ID, romFile) + pciDevice, ok := (*device).(config.VFIOPCIDev) + if !ok { + return fmt.Errorf("VFIO device %+v is not PCI, but its Type said otherwise", device) } + err = q.qmpMonitorCh.qmp.ExecutePCIVFIODeviceAdd(q.qmpMonitorCh.ctx, devID, pciDevice.BDF, addr, bridge.ID, romFile) + case config.VFIOPCIDeviceMediatedType: + err = q.qmpMonitorCh.qmp.ExecutePCIVFIOMediatedDeviceAdd(q.qmpMonitorCh.ctx, devID, *(*device).GetSysfsDev(), addr, bridge.ID, romFile) default: return fmt.Errorf("Incorrect VFIO device type found") } @@ -1612,13 +1612,24 @@ func (q *qemu) hotplugVFIODevice(ctx context.Context, device *config.VFIODev, op if err != nil { return err } - // XXX: Depending on whether we're doing root port or - // bridge hotplug, and how the bridge is set up in - // other parts of the code, we may or may not already - // have information about the slot number of the - // bridge and or the device. For simplicity, just - // query both of them back from qemu - device.GuestPciPath, err = q.qomGetPciPath(devID) + + switch (*device).GetType() { + case config.VFIOPCIDeviceNormalType, config.VFIOPCIDeviceMediatedType: + pciDevice, ok := (*device).(config.VFIOPCIDev) + if !ok { + return fmt.Errorf("VFIO device %+v is not PCI, but its Type said otherwise", device) + } + // XXX: Depending on whether we're doing root port or + // bridge hotplug, and how the bridge is set up in + // other parts of the code, we may or may not already + // have information about the slot number of the + // bridge and or the device. For simplicity, just + // query both of them back from qemu + guestPciPath, err := q.qomGetPciPath(devID) + pciDevice.GuestPciPath = guestPciPath + *device = pciDevice + return err + } return err } else { q.Logger().WithField("dev-id", devID).Info("Start hot-unplug VFIO device") diff --git a/src/runtime/virtcontainers/qemu_arch_base.go b/src/runtime/virtcontainers/qemu_arch_base.go index 62fec60a7dd1..465b694ddd65 100644 --- a/src/runtime/virtcontainers/qemu_arch_base.go +++ b/src/runtime/virtcontainers/qemu_arch_base.go @@ -643,16 +643,17 @@ func (q *qemuArchBase) appendVhostUserDevice(ctx context.Context, devices []govm } func (q *qemuArchBase) appendVFIODevice(devices []govmmQemu.Device, vfioDev config.VFIODev) []govmmQemu.Device { - if vfioDev.BDF == "" { + pciDevice := vfioDev.(config.VFIOPCIDev) + if pciDevice.BDF == "" { return devices } devices = append(devices, govmmQemu.VFIODevice{ - BDF: vfioDev.BDF, - VendorID: vfioDev.VendorID, - DeviceID: vfioDev.DeviceID, - Bus: vfioDev.Bus, + BDF: pciDevice.BDF, + VendorID: pciDevice.VendorID, + DeviceID: pciDevice.DeviceID, + Bus: pciDevice.Bus, }, ) diff --git a/src/runtime/virtcontainers/qemu_arch_base_test.go b/src/runtime/virtcontainers/qemu_arch_base_test.go index ff20ba4479ba..3294e8b22a22 100644 --- a/src/runtime/virtcontainers/qemu_arch_base_test.go +++ b/src/runtime/virtcontainers/qemu_arch_base_test.go @@ -434,7 +434,7 @@ func TestQemuArchBaseAppendVFIODevice(t *testing.T) { }, } - vfDevice := config.VFIODev{ + vfDevice := config.VFIOPCIDev{ BDF: bdf, } @@ -454,7 +454,7 @@ func TestQemuArchBaseAppendVFIODeviceWithVendorDeviceID(t *testing.T) { }, } - vfDevice := config.VFIODev{ + vfDevice := config.VFIOPCIDev{ BDF: bdf, VendorID: vendorID, DeviceID: deviceID, diff --git a/src/runtime/virtcontainers/sandbox.go b/src/runtime/virtcontainers/sandbox.go index 9e1ac02b502c..6983d9944d1e 100644 --- a/src/runtime/virtcontainers/sandbox.go +++ b/src/runtime/virtcontainers/sandbox.go @@ -1789,11 +1789,15 @@ func (s *Sandbox) HotplugAddDevice(ctx context.Context, device api.Device, devTy // adding a group of VFIO devices for _, dev := range vfioDevices { if _, err := s.hypervisor.HotplugAddDevice(ctx, dev, VfioDev); err != nil { + bdf := "" + if pciDevice, ok := (*dev).(config.VFIOPCIDev); ok { + bdf = pciDevice.BDF + } s.Logger(). WithFields(logrus.Fields{ "sandbox": s.id, - "vfio-device-ID": dev.ID, - "vfio-device-BDF": dev.BDF, + "vfio-device-ID": (*dev).GetID(), + "vfio-device-BDF": bdf, }).WithError(err).Error("failed to hotplug VFIO device") return err } @@ -1842,11 +1846,15 @@ func (s *Sandbox) HotplugRemoveDevice(ctx context.Context, device api.Device, de // remove a group of VFIO devices for _, dev := range vfioDevices { if _, err := s.hypervisor.HotplugRemoveDevice(ctx, dev, VfioDev); err != nil { + bdf := "" + if pciDevice, ok := (*dev).(config.VFIOPCIDev); ok { + bdf = pciDevice.BDF + } s.Logger().WithError(err). WithFields(logrus.Fields{ "sandbox": s.id, - "vfio-device-ID": dev.ID, - "vfio-device-BDF": dev.BDF, + "vfio-device-ID": (*dev).GetID(), + "vfio-device-BDF": bdf, }).Error("failed to hot unplug VFIO device") return err } diff --git a/src/runtime/virtcontainers/utils/utils_linux.go b/src/runtime/virtcontainers/utils/utils_linux.go index 265f10d11b58..2835d6669098 100644 --- a/src/runtime/virtcontainers/utils/utils_linux.go +++ b/src/runtime/virtcontainers/utils/utils_linux.go @@ -88,8 +88,7 @@ func FindContextID() (*os.File, uint64, error) { const ( procMountsFile = "/proc/mounts" - fieldsPerLine = 6 - vfioAPSysfsDir = "vfio_ap" + fieldsPerLine = 6 ) const ( @@ -140,15 +139,3 @@ func GetDevicePathAndFsTypeOptions(mountPoint string) (devicePath, fsType string } } } - -// IsAPVFIOMediatedDevice decides whether a device is a VFIO-AP device -// by checking for the existence of "vfio_ap" in the path -func IsAPVFIOMediatedDevice(sysfsdev string) bool { - split := strings.Split(sysfsdev, string(os.PathSeparator)) - for _, el := range split { - if el == vfioAPSysfsDir { - return true - } - } - return false -} diff --git a/src/runtime/virtcontainers/utils/utils_linux_test.go b/src/runtime/virtcontainers/utils/utils_linux_test.go index dbf9fde38bf1..c8e213c2c735 100644 --- a/src/runtime/virtcontainers/utils/utils_linux_test.go +++ b/src/runtime/virtcontainers/utils/utils_linux_test.go @@ -63,19 +63,3 @@ func TestGetDevicePathAndFsTypeOptionsSuccessful(t *testing.T) { assert.Equal(fstype, fstypeOut) assert.Equal(fsOptions, optsOut) } - -func TestIsAPVFIOMediatedDeviceFalse(t *testing.T) { - assert := assert.New(t) - - // Should be false for a PCI device - isAPMdev := IsAPVFIOMediatedDevice("/sys/bus/pci/devices/0000:00:02.0/a297db4a-f4c2-11e6-90f6-d3b88d6c9525") - assert.False(isAPMdev) -} - -func TestIsAPVFIOMediatedDeviceTrue(t *testing.T) { - assert := assert.New(t) - - // Typical AP sysfsdev - isAPMdev := IsAPVFIOMediatedDevice("/sys/devices/vfio_ap/matrix/a297db4a-f4c2-11e6-90f6-d3b88d6c9525") - assert.True(isAPMdev) -}