-
Notifications
You must be signed in to change notification settings - Fork 35
VF VFIO device support #146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@adrianchiris @SchSeba could you please review the changes? |
|
Hi @adrianchiris will NVIDIA be able to check this one I don't have an env to test infiniband. |
almaslennikov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrianchiris PTAL at this PR
cc34bd7 to
1210b64
Compare
almaslennikov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @adrianchiris PTAL
|
Will review on sunday. sorry for the delay. |
|
@adrianchiris PTAL |
cmd/ib-sriov-cni/main.go
Outdated
| return nil, nil, err | ||
| } | ||
|
|
||
| // If vfioPciMode is true (either set manually or auto-detected), skip SR-IOV setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do want the check related to netConf.RdmaIso
i would just skip the call for config.LoadDeviceInfo() if netConf.VfioPciMode is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack. I will update the PR.
cmd/ib-sriov-cni/main.go
Outdated
| // Only handle GUID setting for VF devices, keep previous code path for PF devices | ||
| if isVF && netConf.GUID != "" { | ||
| // Load device info needed for GUID setting (VFIO VF version - no network interface) | ||
| err = config.LoadDeviceInfoVfioVF(netConf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why this should not be rolled into configLoadDeviceInfo which will handle both regular and vfio cases ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I should put them together, if it's VF only, many codes can be reused.
cmd/ib-sriov-cni/main.go
Outdated
| if err != nil { | ||
| return err | ||
| sm := sriov.NewSriovManager() | ||
| if err := sm.ApplyVFConfig(netConf); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this handles also applying linkState why do you call it conditionally if its a VF and GUID was provided ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's related to the PF related code path. I agree, this part need be optimized.
maybe I need submit a VF VFIO only change fist for this PR
pkg/sriov/sriov.go
Outdated
| // For VFIO VF devices, we don't have a network interface, so skip VF link operations | ||
| if conf.VfioPciMode && conf.HostIFNames == "" { | ||
| // VFIO VF: Just set the GUID directly via PF, no VF link to query | ||
| return s.setVfGUID(conf, pfLink, conf.GUID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this currently breaks the non-vfio use-case.
for both cases you need to set the node and port GUID and only for the non-vfio case you need to rebind the VF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
pkg/sriov/sriov.go
Outdated
| // Validate that we have container interface name to work with | ||
| if conf.ContIFNames == "" { | ||
| // Silently skip cleanup if container interface name is missing | ||
| // This can happen with corrupted or incomplete cached configs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cache config failed to load we exit early.
i really find it hard to follow this PR. IMO this PR should focus around VFIO support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I will address this in next update.
Add VFIO mode support for VF devices, which is needed by kubevirt. Signed-off-by: Zhen(Winson) Wang <[email protected]>
1210b64 to
6a69941
Compare
|
@adrianchiris thanks for your review comments. I update my PR with VF VFIO support only. PTAL when you have time. |
|
Hi, guys) I have built and installed |
|
Hi @kozmod , Thanks for validating my PR. This PR is pending @adrianchiris 's review for my latest changes. |
|
@adrianchiris , could you continue validate this PR, please? |

Add both PF and VF VFIO mode support, which is needed by kubevirt support.