Update vfio-manage to choose best VFIO driver #128
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 19655564782Details
💛 - Coveralls |
cdesiniotis
force-pushed
the
support-variant-vfio-modules
branch
2 times, most recently
from
fcfa6cf to
7829c7f
Compare
internal/nvpci/nvpci.go
Outdated
Comment on lines
208
to
256
| modAliasPath := filepath.Join(d.Path, "modalias") | ||
| modAliasContent, err := os.ReadFile(modAliasPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read modalias file for %s: %w", d.Address, err) | ||
| } | ||
|
|
||
| modAliasStr := strings.TrimSpace(string(modAliasContent)) | ||
| modAlias, err := parseModAliasString(modAliasStr) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to parse modalias string %q for device %q: %w", modAliasStr, d.Address, err) | ||
| } | ||
| logrus.Debugf("modalias for device %q: %+v", d.Address, modAlias) | ||
|
|
||
| kernelVersion, err := getKernelVersion() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get kernel version: %w", err) | ||
| } | ||
| logrus.Debugf("kernel version: %s", kernelVersion) | ||
|
|
||
| modulesAliasFilePath := filepath.Join("/lib/modules", kernelVersion, "modules.alias") |
Member
There was a problem hiding this comment.
Could we extract our file paths into constants?
const (
kernelModulesRoot = "/lib/modules"
modulesAliasFileName = "modules.alias"
)
We can also create helper functions to create the paths:
func getModulesAliasPath(kernelVersion string) string {
return filepath.Join(kernelModulesRoot, kernelVersion, modulesAliasFileName)
}
func getDeviceModaliasPath(devicePath string) string {
return filepath.Join(devicePath, "modalias")
}
This way, callers don't need to know the exact path structure.
internal/nvpci/modalias.go
Outdated
Comment on lines
203
to
261
| if matches, score := matchField(deviceModAlias.vendor, patternModAlias.vendor); !matches { | ||
| return false, 0 | ||
| } else { | ||
| specificity += score | ||
| } | ||
|
|
||
| if matches, score := matchField(deviceModAlias.device, patternModAlias.device); !matches { | ||
| return false, 0 | ||
| } else { | ||
| specificity += score | ||
| } | ||
|
|
||
| if matches, score := matchField(deviceModAlias.subvendor, patternModAlias.subvendor); !matches { | ||
| return false, 0 | ||
| } else { | ||
| specificity += score | ||
| } | ||
|
|
||
| if matches, score := matchField(deviceModAlias.subdevice, patternModAlias.subdevice); !matches { | ||
| return false, 0 | ||
| } else { | ||
| specificity += score | ||
| } | ||
|
|
||
| if matches, score := matchField(deviceModAlias.baseClass, patternModAlias.baseClass); !matches { | ||
| return false, 0 | ||
| } else { | ||
| specificity += score | ||
| } | ||
|
|
||
| if matches, score := matchField(deviceModAlias.subClass, patternModAlias.subClass); !matches { | ||
| return false, 0 | ||
| } else { | ||
| specificity += score | ||
| } | ||
|
|
||
| if matches, score := matchField(deviceModAlias.interface_, patternModAlias.interface_); !matches { | ||
| return false, 0 | ||
| } else { | ||
| specificity += score | ||
| } | ||
|
|
||
| return true, specificity |
Member
There was a problem hiding this comment.
Would it make sense to use slices of getters here to shorten this code?
type fieldGetter func(*modAlias) string
fields := []struct {
getter fieldGetter
}{
{func(m *modAlias) string { return m.vendor }},
{func(m *modAlias) string { return m.device }},
// ... etc
}
for _, field := range fields {
deviceVal := field.getter(deviceModAlias)
patternVal := field.getter(patternModAlias)
if matches, score := matchField(deviceVal, patternVal); !matches {
return false, 0
}
specificity += score
}
Collaborator
Author
There was a problem hiding this comment.
I have updated the implementation significantly. I believe it is simpler now. Let me know what you think.
| for _, line := range lines { | ||
| line = strings.TrimSpace(line) | ||
|
|
||
| if !strings.HasPrefix(line, "alias vfio_pci:") { |
Member
There was a problem hiding this comment.
Can we extract this to a named constant?
cdesiniotis
force-pushed
the
support-variant-vfio-modules
branch
2 times, most recently
from
ff28e1c to
b5da7d9
Compare
cdesiniotis
changed the title
cdesiniotis
force-pushed
the
support-variant-vfio-modules
branch
from
b5da7d9 to
a95b741
Compare
Rather than always binding GPUs to the vfio-pci driver, this commit
introduces logic to see if the running kernel has a VFIO variant
driver available that is a better match for the device. This is required
on Grace-based systems where the nvgrace_gpu_vfio_pci module is required
to be used in favor of the vfio-pci module.
We read the mod.alias file for a given device, then we look through
/lib/modules/${kernel_version}/modules.alias for the vfio_pci alias
that matches with the least number of wildcard ('*') fields.
The code introduced in this commit is inspired by:
https://gitlab.com/libvirt/libvirt/-/commit/82e2fac297105f554f57fb589002933231b4f711
Signed-off-by: Christopher Desiniotis <[email protected]>
cdesiniotis
force-pushed
the
support-variant-vfio-modules
branch
from
a95b741 to
ad775b1
Compare
tariq1890
reviewed
Nov 25, 2025
| // extractField extracts the value before the next delimiter from the input string. | ||
| // Returns the extracted value, the remaining string (without the delimiter), and any error. | ||
| func extractField(input, delimiter string) (string, string, error) { | ||
| idx := strings.Index(input, delimiter) |
Contributor
There was a problem hiding this comment.
There might be other methods in the strings package you could use to simplify this further
tariq1890
reviewed
Nov 25, 2025
| subdevice string // sd | ||
| baseClass string // bc | ||
| subClass string // sc | ||
| interface_ string // i |
Contributor
There was a problem hiding this comment.
Suggested change
| interface_ string // i | |
| pciDevInterface string // i |
OR
Suggested change
| interface_ string // i | |
| deviceInterface string // i |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rather than always binding GPUs to the vfio-pci driver, this commit
introduces logic to see if the running kernel has a VFIO variant
driver available that is a better match for the device. This is required
on Grace-based systems where the nvgrace_gpu_vfio_pci module is required
to be used in favor of the vfio-pci module.
We read the mod.alias file for a given device, then we look through
/lib/modules/${kernel_version}/modules.alias for the vfio_pci alias
that matches with the least number of wildcard ('*') fields.
The code introduced in this commit is inspired by:
https://gitlab.com/libvirt/libvirt/-/commit/82e2fac297105f554f57fb589002933231b4f711
Depends on #127
Testing
On a GB200 compute tray:
On a system with one L40 (configured in graphics mode) and one L4 GPU: