[12.0-stable] Revert "domainmgr: do pci-reserve on start of edge app" #4633
Conversation
github-actions
bot
requested review from
eriknordmark,
jsfakian and
uncleDecart
There was a problem hiding this comment.
Don't we need the rest of the commits from the original pull request?
https://github.com/lf-edge/eve/pull/4120/commits
Good point @OhmSpectator , regarding the other 2 commits from the PR:
|
Signed-off-by: Christoph Ostarek <christoph@zededa.com> (cherry picked from commit 8444b3f)
|
Build errors were due to "pull rate limit", triggering workflow again.... |
|
I want @christoph-zededa to take a look at the PR. Even though he was involved in the investigation, I would like him to approve the final result =) |
I am indecisive; on one hand this PR solves the described issue on the other hand it prevents users from using USB-passthrough in certain (perhaps obscure) cases. |
There was a problem hiding this comment.
it prevents users from using USB-passthrough in certain (perhaps obscure) cases.
Good to know, thanks! Is it possible to better understand the consequences? What are these specific cases? Can we expect users to encounter them in real life?
One thing is if this only happens in some very specific and obscure cases — in that case, we can simply describe the possible regression in the release notes and inform users. Another thing is if we expect it to affect a widely used scenario. Then, the cost of fixing it may not be justified, I would say...
It is when they try to combine PCI passthrough of the controller and USB passthrough of devices connected to this controller (i.e. you have two edge apps, one that is using PCI passthrough another one is using USB passthrough; sometimes you run one sometimes the other). As with this PR the PCI controller gets the
From my experience users always try to break the system by using obscure options ;-) |
|
@christoph-zededa , is USB per device passthrough available in 12.0.6-lts? |
There was a problem hiding this comment.
@christoph-zededa , is USB per device passthrough available in 12.0.6-lts?
Yes. The feature that this PR is breaking is also available since 12.0.0: 31b9c8e. The feature was explicitly requested by @eriknordmark.
As far as I undesrand, there is a simple way to fix the original issue and not break the feature. @christoph-zededa said that the current approach will require very little polishing to do so. I hope he can help with it.
|
|
Thanks a lot @christoph-zededa for providing the updated cherry-pick. I've integrated to this PR. Please, take a look. I will also test this PR on the real device and will update the results here. |
|
@OhmSpectator , @christoph-zededa , I've tested latest changes on the real device, it's working as expected. If there are no more comments, PR is ready for merge. |
There was a problem hiding this comment.
Ah, sorry! @christoph-zededa pointed to the fact that there are no changes in the PR since the last time we took a look at it.
that might have a USB device passthrough in the future This is necessary to support PCI passthrough and USB passthrough at the same time, but still do PCIReserve of other devices on startup of pillar Signed-off-by: Christoph Ostarek <christoph@zededa.com> (cherry picked from commit ef7d469)
|
Sorry, I messed up my branches, it's updated now. |
|
Pull rate limits.... 😭 |
On EVE 12.0.6-lts it was observed that USB Controller was not being assigned to vfio-pci driver when
debug.enable.usbis set to false, making USB Input devices available to the host, i.e., not disabling USB keyboards. This behavior doesn't happen on > 13.x versions. It turns out the fix was the commit being cherry-picked by this PR.Solution was tested on real hardware (Karbon-300) + QEMU.
Kudos to @christoph-zededa for helping debugging this issue.