virtio-device: Mapping mmio over virtio-iommu

[acarp-crusoe]

Add infrastructure to lookup the host address for mmio regions on external dma mapping requests. This specifically resolves vfio passthrough for virtio-iommu, allowing for nested virtualization to pass external devices through.

Fixes #6110

[rbradford]

@acarp-crusoe If a commit fixes a bug please can you put "Fixes: #" in the commit message itself - if it's just relevant "See: #"

[acarp-crusoe]

@acarp-crusoe If a commit fixes a bug please can you put "Fixes: #" in the commit message itself - if it's just relevant "See: #"

Commit message has been updated to include the fixed bug - thanks

[acarp-crusoe]

From what I can see, the existing workflow also appears to be failing on the main branch so there's not much I can do to address that. Everything else is ready to go though

[rbradford]

@thomasbarrett would you like to review?

[rbradford]

@acarp-crusoe This PR also has some extra whitespace changes - do you mind taking a look?

[acarp-crusoe]

@acarp-crusoe This PR also has some extra whitespace changes - do you mind taking a look?

Sorry about that, whitespace changes should be reverted

[thomasbarrett]

It seems like we are finding ourselves needing to re-implementing a lot of the functionality from the vm_memory crate for mmio. It would be great if we could re-use the functionality from vm_memory (all of which applies to mmio as well as physical RAM). This is more of an observation... I don't think that we need to look into this for this PR.

I missed @thomasbarrett's review.

[thomasbarrett]

Sorry for the slow review @rbradford. @acarp-crusoe and I were at a conference :)

[likebreath]

Thank you for the contribution. I'd suggest to split this change into two commits. The first commit is refactoring (e.g. no functional change) of moving VfioDmaMapping to the pci crate. And the second commit (the actual functional change) is extending the VfioDmamapping struct to support lookup the user address of mmio regions. Separating functional changes from refactoring is generally cleaner and can help a lot with code review.

Also, I wonder why a Cloud Hypervisor L2 VM works while a QEMU L2 VM does not work? Parden me that I have limited understanding of VFIO and vMMIO.

[acarp-crusoe]

Thank you for the contribution. I'd suggest to split this change into two commits. The first commit is refactoring (e.g. no functional change) of moving VfioDmaMapping to the pci crate. And the second commit (the actual functional change) is extending the VfioDmamapping struct to support lookup the user address of mmio regions. Separating functional changes from refactoring is generally cleaner and can help a lot with code review.

Also, I wonder why a Cloud Hypervisor L2 VM works while a QEMU L2 VM does not work? Parden me that I have limited understanding of VFIO and vMMIO.

Sounds good, the change has just been split into two commits (one the refactor and the other functional), just let me know if you'd need anything else.

And to answer your question - both Cloud Hypervisor and Qemu worked as L2 VMs within Cloud Hypervisor. However, if you tried to include any external pci devices as part of the L2 VM, that wouldn't work. The issue lied in the fact that the mapping requests that arrived in the virtio iommu device did not check against mmio regions at all within VfioDmaMapping. Originally VfioDmaMapping just attempted to dma map RAM using the guest_memory mmap stored in the memory_manager, but mmio regions are not included in the guest_memory mmap. This PR adds mmio_regions as a separate object in the device_manager so the VfioDmaMapping is able to check both guest memory (RAM) as well as mmio. With this change, we can now map mmio over virtio iommu properly, whereas before the L1 VM would crash because it receiving a mapping request for a band of mmio memory it couldn't locate.

[likebreath]

The code changes make sense and look good to me. As I am not an VFIO expert, another review will be very helpful. @rbradford @thomasbarrett

[likebreath]

And to answer your question - both Cloud Hypervisor and Qemu worked as L2 VMs within Cloud Hypervisor. However, if you tried to include any external pci devices as part of the L2 VM, that wouldn't work.

This makes sense if both Cloud Hypervisor and QEMU L2 VM with VFIO devices are failing. I thought the observation was that L2 Cloud Hypervisor with VFIO works according to this comment: #6110 (comment). Thanks for clarifying that.

[acarp-crusoe]

And to answer your question - both Cloud Hypervisor and Qemu worked as L2 VMs within Cloud Hypervisor. However, if you tried to include any external pci devices as part of the L2 VM, that wouldn't work.

This makes sense if both Cloud Hypervisor and QEMU L2 VM with VFIO devices are failing. I thought the observation was that L2 Cloud Hypervisor with VFIO works according to this comment: #6110 (comment). Thanks for clarifying that.

No problem, and that previous comment you've linked was when I was trying to get an understanding of the bug that @thomasbarrett produced, so it's actually a little off. I added a followup comment to clarify the behavior of the bug now that we have a good understanding.

[likebreath]

Few more comments below. Also, it is good to use verb to start the commit message. Say:

virtio-devices: Move VfioDmaMapping to be in the pci crate
virtio-devices: Map mmio over virtio-iommu

[likebreath]

The PR looks good to me. @thomasbarrett @rbradford And additional comments?

[thomasbarrett]

No additional comments. Looks good to me :)