-
Notifications
You must be signed in to change notification settings - Fork 177
Trust assigngrp as received from the controller #5488
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
This runtime check will get in the way if we remove the ACS override patch. Signed-off-by: eriknordmark <erik@zededa.com>
This will log warnings if the running kernel's IOMMU groups include more PCI devices that the IoBundleList from the hardware model. Signed-off-by: eriknordmark <erik@zededa.com>
Trusting the controller to send the correct assigngrp in the EVE API. Signed-off-by: eriknordmark <erik@zededa.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5488 +/- ##
==========================================
+ Coverage 19.52% 28.08% +8.55%
==========================================
Files 19 19
Lines 3021 2314 -707
==========================================
+ Hits 590 650 +60
+ Misses 2310 1520 -790
- Partials 121 144 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
XXX squash with previous commits Signed-off-by: eriknordmark <erik@zededa.com>
Signed-off-by: eriknordmark <erik@zededa.com>
| // expand list to include other PCI functions on the same PCI controller | ||
| // since they need to be treated as part of the same bundle even if the | ||
| // EVE controller doesn't know it | ||
| list = aa.ExpandControllers(log, list, hyper.PCISameController) |
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.
I don't understand exactly why are you removing this check.... on systems without the ACS patch, we might have an I/O adapter in a certain IOMMU group which has more elements, we can allow the passthrough of the single adapter, but QEMU will fail to initialize the VM anyways (if there are more devices not attached to the vfio-driver in the same IOMMU group)....
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.
The question we need to answer is whether we can have all systems run without the ACS override patch and only rely on the controller indicating the groups.
If you are correct that QEMU will fail to boot if not all devices in the IOMMU group are assigned to the booted VM then clearly this will not work, but my memory from years back was that the failures were more subtle meaning that there wasn't such an explicit check in QEMU/kvm at boot time.
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.
I've seen at least an error like the following in a recent past:
- USB Controller + thermal device under the same IOMMU group
- USB Controller assigned to vfio-pci driver
- USB Controller passedthrough to the Edge App VM
- Thermal device assigned to its own driver (no passing through)
With this configuration, QEMU fails to initialize the VM....
Description
Allow the kernel's IOMMU groups be different than the assigngrp received from the controller, but log warnings when a IOMMU group has larger scope than an assigngrp.
Also, do not expand the assigngrp based on the IOMMU groups for the purposes of determining which network adapters need to be kept in the host.
Testing all TBD.
How to test and validate this PR
TBD
Changelog notes
TBD this makes it possible to remove the ACS override patch.
Checklist
And the last but not least:
check them.
Please, check the boxes above after submitting the PR in interactive mode.