-
Notifications
You must be signed in to change notification settings - Fork 183
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
expose SRIOV information as PCI devices #315
base: main
Are you sure you want to change the base?
Conversation
this PR is reviewable, but tests are still WIP. The alternate API is pretty much stable though. |
@fromanirh did you want to rebase this and have it reviewed? |
Sure, I'll rebase shortly! other than that this is not very urgent. Up to us if we like more this direction or the original one in #230 |
note that most of the tests are missing - deferred until we settle about the direction. Should not be merged without tests, though. |
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.
@fromanirh apologies for the delayed review.
I think I would prefer to have the SRIOV-specific attributes added to the pkg/pci.Device
struct itself instead of having an "extended" Function struct that derives from Device.
You could have a helper method on pkg/pci.Info
called GetSRIOVDevices()
that would return only the PCI devices that were SR-IOV capable. Something like this?
// GetSRIOVDevices returns only the PCI devices that are
// Single Root I/O Virtualization (SR-IOV) capable -- either
// physical of virtual functions.
func (i *Info) GetSRIOVDevices() []*Device {
res := []*Device{}
for _, dev := range i.Devices {
if dev.Parent != nil || len(dev.Functions) != 0 {
res = append(res, dev)
}
}
return res
}
No worries @jaypipes and thank you for the review. I'm kinda reluctant to conflate even more data in the |
1e5efcf
to
c72700e
Compare
@jaypipes eventually I come to like this approach. We may need a couple more iteration to sort out the details of the API (naming, printing) but overall I'm happy to pursue this direction. PR is updated, rebased and ready for review |
depends on #351 |
simplify the code without change in behaviour, using a flow without `else` to reduce the blocks. Signed-off-by: Francesco Romani <[email protected]>
Add support to report SRIOV devices. Differently from GPU devices, we model SRIOV devices as special PCI devices, extending the `pci` package instead of introducing a new top-level package. This design emerged during the review of a previous proposal: jaypipes@9058f61#r755312597 SRIOV devices are either Physical Functions or Virtual functions. The preferred representation for ghw is Physical Functions, whose dependent devices will be Virtual Functions; however, for the sake of practicality, the API also exposes soft references to Virtual Functions, so consumers of the API can access them directly and not navigating the parent devices. This patch also adds support in `ghwc`, to report the sriov information, and in the `snapshot` package, to make sure to capture all the files in sysfs that ghw cares about. Last but not least, lacking access to suitable non-linux systems, support is provided only on linux OS, even though the API tries hard not to be linux-specific. Resolves: jaypipes#92 Signed-off-by: Francesco Romani <[email protected]>
Virtual Functions are available both as entries in the `pci.Functions` slice and as properties of their parent Physical Functions. | ||
Both references are aliases to the same object. |
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.
yuck, missed to update the rest of the docs :\
Now that we agreed on the direction, I need to add tests. Will do ASAP. |
Expose informations about SRIOV devices. After some more design iterations (#230 (comment)), we fixed these goals:
There are few more noteworthy items in this PR:
Fixes: #92
Signed-off-by: Francesco Romani [email protected]