block: Use PciPath type through block code

BlockDrive::PCIAddr doesn't actually store a PCI address (DDDD:BB:DD.F) but a PCI path. Use the PciPath type and rename things to make that clearer. TestHandleBlockVolume() previously used a bizarre value "0002:01" for the "PCI address" which was neither an actual PCI address, nor a PCI path. Update it to use a PCI path - the actual value appears not to matter in this test, as long as its consistent throughout. fixes #3002 Signed-off-by: David Gibson <[email protected]>
kata-containers · Oct 27, 2020 · 64751f3 · 64751f3
1 parent 3e58971
commit 64751f3
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 27 deletions.
diff --git a/virtcontainers/acrn.go b/virtcontainers/acrn.go
@@ -23,6 +23,7 @@ import (
 
 	"github.com/kata-containers/runtime/virtcontainers/device/config"
 	persistapi "github.com/kata-containers/runtime/virtcontainers/persist/api"
+	vcTypes "github.com/kata-containers/runtime/virtcontainers/pkg/types"
 	"github.com/kata-containers/runtime/virtcontainers/pkg/uuid"
 	"github.com/kata-containers/runtime/virtcontainers/types"
 	"github.com/kata-containers/runtime/virtcontainers/utils"
@@ -550,8 +551,8 @@ func (a *Acrn) updateBlockDevice(drive *config.BlockDrive) error {
 
 	slot := AcrnBlkdDevSlot[drive.Index]
 
-	//Explicitly set PCIAddr to NULL, so that VirtPath can be used
-	drive.PCIAddr = ""
+	//Explicitly set PCIPath to NULL, so that VirtPath can be used
+	drive.PCIPath = vcTypes.PciPath{}
 
 	args := []string{"blkrescan", a.acrnConfig.Name, fmt.Sprintf("%d,%s", slot, drive.File)}
 

diff --git a/virtcontainers/clh.go b/virtcontainers/clh.go
@@ -29,6 +29,7 @@ import (
 	log "github.com/sirupsen/logrus"
 
 	"github.com/kata-containers/runtime/virtcontainers/device/config"
+	vcTypes "github.com/kata-containers/runtime/virtcontainers/pkg/types"
 	"github.com/kata-containers/runtime/virtcontainers/types"
 	"github.com/kata-containers/runtime/virtcontainers/utils"
 )
@@ -422,8 +423,8 @@ func (clh *cloudHypervisor) hotplugAddBlockDevice(drive *config.BlockDrive) erro
 
 	driveID := clhDriveIndexToID(drive.Index)
 
-	//Explicitly set PCIAddr to NULL, so that VirtPath can be used
-	drive.PCIAddr = ""
+	//Explicitly set PCIPath to NULL, so that VirtPath can be used
+	drive.PCIPath = vcTypes.PciPath{}
 
 	if drive.Pmem {
 		err = fmt.Errorf("pmem device hotplug not supported")

diff --git a/virtcontainers/device/config/config.go b/virtcontainers/device/config/config.go
@@ -15,6 +15,7 @@ import (
 	"strings"
 
 	"github.com/go-ini/ini"
+	vcTypes "github.com/kata-containers/runtime/virtcontainers/pkg/types"
 	"golang.org/x/sys/unix"
 )
 
@@ -153,8 +154,8 @@ type BlockDrive struct {
 	// MmioAddr is used to identify the slot at which the drive is attached (order?).
 	MmioAddr string
 
-	// PCIAddr is the PCI address used to identify the slot at which the drive is attached.
-	PCIAddr string
+	// PCIPath is the PCI path used to identify the slot at which the drive is attached.
+	PCIPath vcTypes.PciPath
 
 	// SCSI Address of the block device, in case the device is attached using SCSI driver
 	// SCSI address is in the format SCSI-Id:LUN

diff --git a/virtcontainers/device/drivers/block.go b/virtcontainers/device/drivers/block.go
@@ -169,7 +169,7 @@ func (device *BlockDevice) Save() persistapi.DeviceState {
 			ID:       drive.ID,
 			Index:    drive.Index,
 			MmioAddr: drive.MmioAddr,
-			PCIAddr:  drive.PCIAddr,
+			PCIPath:  drive.PCIPath,
 			SCSIAddr: drive.SCSIAddr,
 			NvdimmID: drive.NvdimmID,
 			VirtPath: drive.VirtPath,
@@ -195,7 +195,7 @@ func (device *BlockDevice) Load(ds persistapi.DeviceState) {
 		ID:       bd.ID,
 		Index:    bd.Index,
 		MmioAddr: bd.MmioAddr,
-		PCIAddr:  bd.PCIAddr,
+		PCIPath:  bd.PCIPath,
 		SCSIAddr: bd.SCSIAddr,
 		NvdimmID: bd.NvdimmID,
 		VirtPath: bd.VirtPath,

diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go
@@ -1225,7 +1225,7 @@ func (k *kataAgent) appendBlockDevice(dev ContainerDevice, c *Container) *grpc.D
 		kataDevice.Id = d.DevNo
 	case config.VirtioBlock:
 		kataDevice.Type = kataBlkDevType
-		kataDevice.Id = d.PCIAddr
+		kataDevice.Id = d.PCIPath.String()
 		kataDevice.VmPath = d.VirtPath
 	case config.VirtioSCSI:
 		kataDevice.Type = kataSCSIDevType
@@ -1329,10 +1329,10 @@ func (k *kataAgent) buildContainerRootfs(sandbox *Sandbox, c *Container, rootPat
 			rootfs.Source = blockDrive.DevNo
 		case sandbox.config.HypervisorConfig.BlockDeviceDriver == config.VirtioBlock:
 			rootfs.Driver = kataBlkDevType
-			if blockDrive.PCIAddr == "" {
+			if blockDrive.PCIPath.IsNil() {
 				rootfs.Source = blockDrive.VirtPath
 			} else {
-				rootfs.Source = blockDrive.PCIAddr
+				rootfs.Source = blockDrive.PCIPath.String()
 			}
 
 		case sandbox.config.HypervisorConfig.BlockDeviceDriver == config.VirtioSCSI:
@@ -1603,10 +1603,10 @@ func (k *kataAgent) handleDeviceBlockVolume(c *Container, device api.Device) (*g
 		vol.Source = blockDrive.DevNo
 	case c.sandbox.config.HypervisorConfig.BlockDeviceDriver == config.VirtioBlock:
 		vol.Driver = kataBlkDevType
-		if blockDrive.PCIAddr == "" {
+		if blockDrive.PCIPath.IsNil() {
 			vol.Source = blockDrive.VirtPath
 		} else {
-			vol.Source = blockDrive.PCIAddr
+			vol.Source = blockDrive.PCIPath.String()
 		}
 	case c.sandbox.config.HypervisorConfig.BlockDeviceDriver == config.VirtioMmio:
 		vol.Driver = kataMmioBlkDevType

diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go
@@ -45,7 +45,7 @@ var (
 	testBlockDeviceCtrPath = "testBlockDeviceCtrPath"
 	testDevNo              = "testDevNo"
 	testNvdimmID           = "testNvdimmID"
-	testPCIAddr            = "04/02"
+	testPCIPath, _         = vcTypes.PciPathFromString("04/02")
 	testSCSIAddr           = "testSCSIAddr"
 	testVirtPath           = "testVirtPath"
 )
@@ -447,13 +447,13 @@ func TestHandleDeviceBlockVolume(t *testing.T) {
 			BlockDeviceDriver: config.VirtioBlock,
 			inputDev: &drivers.BlockDevice{
 				BlockDrive: &config.BlockDrive{
-					PCIAddr:  testPCIAddr,
+					PCIPath:  testPCIPath,
 					VirtPath: testVirtPath,
 				},
 			},
 			resultVol: &pb.Storage{
 				Driver: kataBlkDevType,
-				Source: testPCIAddr,
+				Source: testPCIPath.String(),
 			},
 		},
 		{
@@ -527,13 +527,14 @@ func TestHandleBlockVolume(t *testing.T) {
 	vDestination := "/VhostUserBlk/destination"
 	bDestination := "/DeviceBlock/destination"
 	vPCIAddr := "0001:01"
-	bPCIAddr := "0002:01"
+	bPCIPath, err := vcTypes.PciPathFromString("03/04")
+	assert.NoError(t, err)
 
 	vDev := drivers.NewVhostUserBlkDevice(&config.DeviceInfo{ID: vDevID})
 	bDev := drivers.NewBlockDevice(&config.DeviceInfo{ID: bDevID})
 
 	vDev.VhostUserDeviceAttrs = &config.VhostUserDeviceAttrs{PCIAddr: vPCIAddr}
-	bDev.BlockDrive = &config.BlockDrive{PCIAddr: bPCIAddr}
+	bDev.BlockDrive = &config.BlockDrive{PCIPath: bPCIPath}
 
 	var devices []api.Device
 	devices = append(devices, vDev, bDev)
@@ -581,7 +582,7 @@ func TestHandleBlockVolume(t *testing.T) {
 		Fstype:     "bind",
 		Options:    []string{"bind"},
 		Driver:     kataBlkDevType,
-		Source:     bPCIAddr,
+		Source:     bPCIPath.String(),
 	}
 
 	assert.Equal(t, vStorage, volumeStorages[0], "Error while handle VhostUserBlk type block volume")
@@ -617,7 +618,7 @@ func TestAppendDevices(t *testing.T) {
 				ID: id,
 			},
 			BlockDrive: &config.BlockDrive{
-				PCIAddr: testPCIAddr,
+				PCIPath: testPCIPath,
 			},
 		},
 	}
@@ -644,7 +645,7 @@ func TestAppendDevices(t *testing.T) {
 		{
 			Type:          kataBlkDevType,
 			ContainerPath: testBlockDeviceCtrPath,
-			Id:            testPCIAddr,
+			Id:            testPCIPath.String(),
 		},
 	}
 	updatedDevList := k.appendDevices(devList, c)
@@ -664,7 +665,7 @@ func TestAppendVhostUserBlkDevices(t *testing.T) {
 			},
 			VhostUserDeviceAttrs: &config.VhostUserDeviceAttrs{
 				Type:    config.VhostUserBlk,
-				PCIAddr: testPCIAddr,
+				PCIAddr: testPCIPath.String(),
 			},
 		},
 	}
@@ -692,7 +693,7 @@ func TestAppendVhostUserBlkDevices(t *testing.T) {
 		{
 			Type:          kataBlkDevType,
 			ContainerPath: testBlockDeviceCtrPath,
-			Id:            testPCIAddr,
+			Id:            testPCIPath.String(),
 		},
 	}
 	updatedDevList := k.appendDevices(devList, c)

diff --git a/virtcontainers/persist/api/device.go b/virtcontainers/persist/api/device.go
@@ -6,6 +6,8 @@
 
 package persistapi
 
+import vcTypes "github.com/kata-containers/runtime/virtcontainers/pkg/types"
+
 // ============= sandbox level resources =============
 
 // BlockDrive represents a block storage drive which may be used in case the storage
@@ -26,8 +28,8 @@ type BlockDrive struct {
 	// MmioAddr is used to identify the slot at which the drive is attached (order?).
 	MmioAddr string
 
-	// PCIAddr is the PCI address used to identify the slot at which the drive is attached.
-	PCIAddr string
+	// PCIPath is the PCI path used to identify the slot at which the drive is attached.
+	PCIPath vcTypes.PciPath
 
 	// SCSI Address of the block device, in case the device is attached using SCSI driver
 	// SCSI address is in the format SCSI-Id:LUN

diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go
@@ -1153,8 +1153,18 @@ func (q *qemu) hotplugAddBlockDevice(drive *config.BlockDrive, op operation, dev
 			}
 		}()
 
-		// PCI address is in the format bridge-addr/device-addr eg. "03/02"
-		drive.PCIAddr = fmt.Sprintf("%02x", bridge.Addr) + "/" + addr
+		bridgeSlot, err := vcTypes.PciSlotFromInt(bridge.Addr)
+		if err != nil {
+			return err
+		}
+		devSlot, err := vcTypes.PciSlotFromString(addr)
+		if err != nil {
+			return err
+		}
+		drive.PCIPath, err = vcTypes.PciPathFromSlots(bridgeSlot, devSlot)
+		if err != nil {
+			return err
+		}
 
 		if err = q.qmpMonitorCh.qmp.ExecutePCIDeviceAdd(q.qmpMonitorCh.ctx, drive.ID, devID, driver, addr, bridge.ID, romFile, 0, true, defaultDisableModern); err != nil {
 			return err