Correct isPCIeDevice logic#2889
Conversation
|
Corrected commit message format error. |
|
|
||
| // Plain PCI devices hav 256 bytes of configuration space, | ||
| // PCI-Express devices have 4096 bytes | ||
| return fi.Size() > 256 |
There was a problem hiding this comment.
please don't hard code this value, instead declare a const
Currently, isPCIeDevice() attempts to determine if a (host) device is PCI-Express capable by looking up its link speed via the PCI slots information in sysfs. This is a) complicated and b) wrong. PCI-e devices don't have to have slots information, so this frequently fails. Instead determine if devices are PCI-e by checking for the presence of PCIe extended configuration space by looking at the size of the "config" file in sysfs. Fixes: #2678 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
|
@devimc, I've addressed your most recent comments. Can we get this fix merged? |
|
/test |
| return true | ||
| } | ||
| } | ||
| deviceLogger().WithField("dev-bdf", bdf).WithField("error", err).Warning("Couldn't stat() configuration space file") |
There was a problem hiding this comment.
I think you can use WithError(err), instead of WithField("error", err), but that's a nitpick.
Codecov Report
@@ Coverage Diff @@
## master #2889 +/- ##
==========================================
+ Coverage 51.40% 51.46% +0.06%
==========================================
Files 118 118
Lines 17441 17435 -6
==========================================
+ Hits 8965 8973 +8
+ Misses 7393 7379 -14
Partials 1083 1083 |
jodh-intel
left a comment
jodh-intel
left a comment
There was a problem hiding this comment.
Thanks for raising @dgibson !
Please could you consider raising an equivalent PR for the 2.0 branch here too: https://github.com/kata-containers/kata-containers/tree/2.0-dev/src/runtime
|
|
||
| // Plain PCI devices hav 256 bytes of configuration space, | ||
| // PCI-Express devices have 4096 bytes | ||
| return fi.Size() > PCIConfigSpaceSize |
There was a problem hiding this comment.
I was going to say it might be safer to check if the size is exactly 4096. But presumably if another PCI standard is released that would be a superset of PCI-e (as PCI-e is to PCI) so I guess this is "safe" as-is 😄
There was a problem hiding this comment.
Right, and my guess would be that that hypothetical extension will probably behave closer to PCI express than classic PCI.
jodh-intel
added
the
port-to-2.0
fidencio
left a comment
fidencio
left a comment
There was a problem hiding this comment.
LGTM.
Feel free to adapt the code with the nitpick I mentioned or just to ignore that, as it's nothing else than a nitpick.
|
@dgibson Thanks for the fix. Can you please backport this fix to 1.10 and 1.11 branches as well along with 2.x repo. |
5 participants
Currently, isPCIeDevice() attempts to determine if a (host) device is
PCI-Express capable by looking up its link speed via the PCI slots
information in sysfs. This is a) complicated and b) wrong. PCI-e devices
don't have to have slots information, so this frequently fails.
Instead determine if devices are PCI-e by checking for the presence of
PCIe extended configuration space by looking at the size of the "config"
file in sysfs.
Fixes: #2678
Signed-off-by: David Gibson david@gibson.dropbear.id.au