Merge pull request #101 from NVIDIA/rm-dupl-nvpci

[refactor] remove duplicated code in nvpci.go

Author: tariq1890

pkg/nvmdev/mock.go

@@ -22,7 +22,6 @@ import (
 	"path/filepath"
 
 	"github.com/NVIDIA/go-nvlib/pkg/nvpci"
-	"github.com/NVIDIA/go-nvlib/pkg/nvpci/bytes"
 )
 
 // MockNvmdev mock mdev device.
@@ -99,38 +98,7 @@ func (m *MockNvmdev) AddMockA100Parent(address string, numaNode int) error {
 		return err
 	}
 
-	vendor, err := os.Create(filepath.Join(deviceDir, "vendor"))
-	if err != nil {
-		return err
-	}
-	_, err = fmt.Fprintf(vendor, "0x%x", nvpci.PCINvidiaVendorID)
-	if err != nil {
-		return err
-	}
-
-	class, err := os.Create(filepath.Join(deviceDir, "class"))
-	if err != nil {
-		return err
-	}
-	_, err = fmt.Fprintf(class, "0x%x", nvpci.PCI3dControllerClass)
-	if err != nil {
-		return err
-	}
-
-	device, err := os.Create(filepath.Join(deviceDir, "device"))
-	if err != nil {
-		return err
-	}
-	_, err = device.WriteString("0x20bf")
-	if err != nil {
-		return err
-	}
-
-	_, err = os.Create(filepath.Join(deviceDir, "nvidia"))
-	if err != nil {
-		return err
-	}
-	err = os.Symlink(filepath.Join(deviceDir, "nvidia"), filepath.Join(deviceDir, "driver"))
+	err = nvpci.CreateMockA100SysfsFiles(deviceDir)
 	if err != nil {
 		return err
 	}
@@ -153,43 +121,6 @@ func (m *MockNvmdev) AddMockA100Parent(address string, numaNode int) error {
 		return err
 	}
 
-	config, err := os.Create(filepath.Join(deviceDir, "config"))
-	if err != nil {
-		return err
-	}
-	_data := make([]byte, nvpci.PCICfgSpaceStandardSize)
-	data := bytes.New(&_data)
-	data.Write16(0, nvpci.PCINvidiaVendorID)
-	data.Write16(2, uint16(0x20bf))
-	data.Write8(nvpci.PCIStatusBytePosition, nvpci.PCIStatusCapabilityList)
-	_, err = config.Write(*data.Raw())
-	if err != nil {
-		return err
-	}
-
-	bar0 := []uint64{0x00000000c2000000, 0x00000000c2ffffff, 0x0000000000040200}
-	resource, err := os.Create(filepath.Join(deviceDir, "resource"))
-	if err != nil {
-		return err
-	}
-	_, err = fmt.Fprintf(resource, "0x%x 0x%x 0x%x", bar0[0], bar0[1], bar0[2])
-	if err != nil {
-		return err
-	}
-
-	pmcID := uint32(0x170000a1)
-	resource0, err := os.Create(filepath.Join(deviceDir, "resource0"))
-	if err != nil {
-		return err
-	}
-	_data = make([]byte, bar0[1]-bar0[0]+1)
-	data = bytes.New(&_data).LittleEndian()
-	data.Write32(0, pmcID)
-	_, err = resource0.Write(*data.Raw())
-	if err != nil {
-		return err
-	}
-
 	mdevSupportedTypes := []string{"A100-4C", "A100-5C", "A100-8C", "A100-10C",
 		"A100-20C", "A100-40C", "A100-1-5CME", "A100-1-5C", "A100-2-10C", "A100-3-20C",
 		"A100-4-20C", "A100-7-40C"}

pkg/nvpci/mock.go

@@ -79,7 +79,7 @@ func (m *MockNvpci) AddMockA100(address string, numaNode int, sriov *SriovInfo)
 		return err
 	}
 
-	err = createNVIDIAgpuFiles(deviceDir)
+	err = CreateMockA100SysfsFiles(deviceDir)
 	if err != nil {
 		return err
 	}
@@ -139,7 +139,11 @@ func (m *MockNvpci) AddMockA100(address string, numaNode int, sriov *SriovInfo)
 	return nil
 }
 
-func createNVIDIAgpuFiles(deviceDir string) error {
+// CreateMockA100SysfsFiles populates deviceDir with the sysfs attribute files
+// of an A100-like GPU (vendor, class, device, subsystem ids, driver symlink,
+// config space, and resources). It is shared by mock packages that need an
+// NVIDIA PCI device fixture, such as nvmdev.
+func CreateMockA100SysfsFiles(deviceDir string) error {
 	vendor, err := os.Create(filepath.Join(deviceDir, "vendor"))
 	if err != nil {
 		return err
@@ -251,7 +255,7 @@ func (m *MockNvpci) createVf(pfAddress string, id, iommu_group, numaNode int) er
 		return err
 	}
 
-	err = createNVIDIAgpuFiles(deviceDir)
+	err = CreateMockA100SysfsFiles(deviceDir)
 	if err != nil {
 		return err
 	}

pkg/nvpci/nvpci.go

@@ -17,6 +17,7 @@
 package nvpci
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path"
@@ -258,6 +259,28 @@ func (p *nvpci) GetNvidiaDeviceByPciBusID(address string) (*NvidiaPCIDevice, err
 	return p.getNvidiaDeviceByPciBusID(address, nil)
 }
 
+// readPCIFieldString reads a sysfs PCI attribute file and returns its contents with any surrounding whitespaces trimmed.
+func readPCIFieldString(devicePath, field string) (string, error) {
+	raw, err := os.ReadFile(path.Join(devicePath, field))
+	if err != nil {
+		return "", fmt.Errorf("unable to read PCI %s for %s: %w", field, devicePath, err)
+	}
+	return strings.TrimSpace(string(raw)), nil
+}
+
+// readPCIField reads a sysfs PCI attribute file and parses it as an unsigned integer.
+func readPCIField(devicePath, field string, bitSize int) (uint64, error) {
+	str, err := readPCIFieldString(devicePath, field)
+	if err != nil {
+		return 0, err
+	}
+	val, err := strconv.ParseUint(str, 0, bitSize)
+	if err != nil {
+		return 0, fmt.Errorf("unable to parse PCI %s for %s: %w", field, devicePath, err)
+	}
+	return val, nil
+}
+
 func (p *nvpci) getNvidiaDeviceByPciBusID(address string, cache map[string]*NvidiaPCIDevice) (*NvidiaPCIDevice, error) {
 	if cache != nil {
 		if pciDevice, exists := cache[address]; exists {
@@ -266,68 +289,51 @@ func (p *nvpci) getNvidiaDeviceByPciBusID(address string, cache map[string]*Nvid
 	}
 	devicePath := filepath.Join(p.pciDevicesRoot, address)
 
-	vendor, err := os.ReadFile(path.Join(devicePath, "vendor"))
+	vendorID, err := readPCIField(devicePath, "vendor", 16)
 	if err != nil {
-		return nil, fmt.Errorf("unable to read PCI device vendor id for %s: %v", address, err)
-	}
-	vendorStr := strings.TrimSpace(string(vendor))
-	vendorID, err := strconv.ParseUint(vendorStr, 0, 16)
-	if err != nil {
-		return nil, fmt.Errorf("unable to convert vendor string to uint16: %v", vendorStr)
+		return nil, err
 	}
 
 	if uint16(vendorID) != PCINvidiaVendorID && uint16(vendorID) != PCIMellanoxVendorID {
 		return nil, nil
 	}
 
-	class, err := os.ReadFile(path.Join(devicePath, "class"))
+	classID, err := readPCIField(devicePath, "class", 32)
 	if err != nil {
-		return nil, fmt.Errorf("unable to read PCI device class for %s: %v", address, err)
+		return nil, err
 	}
-	classStr := strings.TrimSpace(string(class))
-	classID, err := strconv.ParseUint(classStr, 0, 32)
+
+	deviceID, err := readPCIField(devicePath, "device", 16)
 	if err != nil {
-		return nil, fmt.Errorf("unable to convert class string to uint32: %v", classStr)
+		return nil, err
 	}
 
-	device, err := os.ReadFile(path.Join(devicePath, "device"))
+	numaStr, err := readPCIFieldString(devicePath, "numa_node")
 	if err != nil {
-		return nil, fmt.Errorf("unable to read PCI device id for %s: %v", address, err)
+		return nil, err
 	}
-	deviceStr := strings.TrimSpace(string(device))
-	deviceID, err := strconv.ParseUint(deviceStr, 0, 16)
+	// numa_node is parsed as a signed integer since "-1" is a valid value meaning "no NUMA affinity".
+	numaNode, err := strconv.ParseInt(numaStr, 0, 64)
 	if err != nil {
-		return nil, fmt.Errorf("unable to convert device string to uint16: %v", deviceStr)
+		return nil, fmt.Errorf("unable to parse PCI numa_node for %s: %w", devicePath, err)
 	}
 
-	var subsystemVendorID uint64
-	subsystemVendor, err := os.ReadFile(path.Join(devicePath, "subsystem_vendor"))
-	switch {
-	case err == nil:
-		subsystemVendorStr := strings.TrimSpace(string(subsystemVendor))
-		subsystemVendorID, err = strconv.ParseUint(subsystemVendorStr, 0, 16)
-		if err != nil {
-			return nil, fmt.Errorf("unable to convert subsystem vendor string to uint16: %v", subsystemVendorStr)
+	// Tolerate missing subsystem files: some environments (e.g. certain virtualised or passthrough PCI topologies)
+	// do not expose them, so the IDs will default to 0.
+	subsystemVendorID, err := readPCIField(devicePath, "subsystem_vendor", 16)
+	if err != nil {
+		if !errors.Is(err, os.ErrNotExist) {
+			return nil, err
 		}
-	case os.IsNotExist(err):
 		p.logger.Warningf("subsystem_vendor file not found for %s", address)
-	default:
-		return nil, fmt.Errorf("unable to read PCI subsystem vendor id for %s: %v", address, err)
 	}
 
-	var subsystemDeviceID uint64
-	subsystemDevice, err := os.ReadFile(path.Join(devicePath, "subsystem_device"))
-	switch {
-	case err == nil:
-		subsystemDeviceStr := strings.TrimSpace(string(subsystemDevice))
-		subsystemDeviceID, err = strconv.ParseUint(subsystemDeviceStr, 0, 16)
-		if err != nil {
-			return nil, fmt.Errorf("unable to convert subsystem device string to uint16: %v", subsystemDeviceStr)
+	subsystemDeviceID, err := readPCIField(devicePath, "subsystem_device", 16)
+	if err != nil {
+		if !errors.Is(err, os.ErrNotExist) {
+			return nil, err
 		}
-	case os.IsNotExist(err):
 		p.logger.Warningf("subsystem_device file not found for %s", address)
-	default:
-		return nil, fmt.Errorf("unable to read PCI subsystem device id for %s: %v", address, err)
 	}
 
 	driver, err := getDriver(devicePath)
@@ -346,16 +352,6 @@ func (p *nvpci) getNvidiaDeviceByPciBusID(address string, cache map[string]*Nvid
 		p.logger.Warningf("unable to detect IOMMU FD for %s: %v", address, err)
 	}
 
-	numa, err := os.ReadFile(path.Join(devicePath, "numa_node"))
-	if err != nil {
-		return nil, fmt.Errorf("unable to read PCI NUMA node for %s: %v", address, err)
-	}
-	numaStr := strings.TrimSpace(string(numa))
-	numaNode, err := strconv.ParseInt(numaStr, 0, 64)
-	if err != nil {
-		return nil, fmt.Errorf("unable to convert NUMA node string to int64: %v", numaNode)
-	}
-
 	config := &ConfigSpace{
 		Path: path.Join(devicePath, "config"),
 	}
@@ -532,32 +528,20 @@ func (p *nvpci) GetGPUByIndex(i int) (*NvidiaPCIDevice, error) {
 }
 
 func (p *nvpci) getSriovInfoForPhysicalFunction(devicePath string) (sriovInfo SriovInfo, err error) {
-	totalVfsPath := filepath.Join(devicePath, "sriov_totalvfs")
-	numVfsPath := filepath.Join(devicePath, "sriov_numvfs")
-
 	// No file for sriov_totalvfs exists? Not an SRIOV device, return nil
-	_, err = os.Stat(totalVfsPath)
+	_, err = os.Stat(filepath.Join(devicePath, "sriov_totalvfs"))
 	if err != nil && os.IsNotExist(err) {
 		return sriovInfo, nil
 	}
-	sriovTotalVfs, err := os.ReadFile(totalVfsPath)
-	if err != nil {
-		return sriovInfo, fmt.Errorf("unable to read sriov_totalvfs: %v", err)
-	}
-	totalVfsStr := strings.TrimSpace(string(sriovTotalVfs))
-	totalVfsInt, err := strconv.ParseUint(totalVfsStr, 10, 16)
-	if err != nil {
-		return sriovInfo, fmt.Errorf("unable to convert sriov_totalvfs to uint64: %v", err)
-	}
 
-	sriovNumVfs, err := os.ReadFile(numVfsPath)
+	totalVfsInt, err := readPCIField(devicePath, "sriov_totalvfs", 16)
 	if err != nil {
-		return sriovInfo, fmt.Errorf("unable to read sriov_numvfs for: %v", err)
+		return sriovInfo, err
 	}
-	numVfsStr := strings.TrimSpace(string(sriovNumVfs))
-	numVfsInt, err := strconv.ParseUint(numVfsStr, 10, 16)
+
+	numVfsInt, err := readPCIField(devicePath, "sriov_numvfs", 16)
 	if err != nil {
-		return sriovInfo, fmt.Errorf("unable to convert sriov_numvfs to uint64: %v", err)
+		return sriovInfo, err
 	}
 
 	sriovInfo = SriovInfo{