[WIP] Use pcie-pci-bridge for the same iommu group #3369
Conversation
Rebase original PR lf-edge#2544 Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com> Signed-off-by: Mikhail Malyshev <mikem@zededa.com>
mikem-zed
changed the title
| for _, t := range list { | ||
| if t.pciLong == tap.pciLong { | ||
| return list | ||
| func addNoDuplicatePCI(list map[string][]typeAndPCI, tap typeAndPCI, group string) map[string][]typeAndPCI { |
There was a problem hiding this comment.
Why do you need to change it from an ordered list to a (intentionally randomized by golang) map?
I suspect that the order matters e.g., if the DomainConfig has both eth0 with PCI-id x:y and eth1 with PCI-id w:z, then we need to pass them to kvm/xen in the order they were listed in DomainConfig, since I suspect a Linux guest VM will enumerate them based on the order they where in the kvm/xen/qemu config.
There was a problem hiding this comment.
Good point, seems I did not pay attention to it during original PR.It looks like we need more complex structure to store pciAssignments in CreateDomConfig (not so pretty, but something like list of list of typeAndPCI + list of groups will work). Also, seems test should be modified to check the order as well.
There was a problem hiding this comment.
Good point, I want to test it as-is on NUC to see if it solves current problem and then I'll fix it
There was a problem hiding this comment.
actually this approach is not correct IMO. By adding pcie-pci bridge we start a legacy PCI topology which may be not correct for some PCIe devices. Besides some multi function devices must remain multi-function in VM e.g. GPU+Sound or Thunderbolt controllers. I have a POC code but I'll test the approach on real HW first
There was a problem hiding this comment.
I agree with @mikem-zed , also, according to the description in the original PR, it seems to me that the mentioned error it's being caused due to the ACS patch, am I right?
There was a problem hiding this comment.
@mikem-zed , please, add some overview explanation to the commit's message. The old PR number information should be on the PR description at GH, where it can be traceable.
Rebase original PR #2544
Here is the latest comment from @eriknordmark in the original PR
We can detect address space overlapping by looking at BAR registers. It can be done at runtime. But this is exactly why Linux kernel put these devices into a single IOMMU group even with ACS override patch. Basically this check was already done by linux kernel. Are there any other reasons for putting >1 device in IOMMU group with ACS patch override? Good question. We can add a logic to check for address space overlaping and ad pci-pci bridge only in this case but the rule remains the same: the entier iommu group must be assigned to an app with or without extra pci-pci bridge