Skip to content

Feat: IoMmu protocol - #1728

Merged
phip1611 merged 3 commits into
rust-osdev:mainfrom
PelleKrab:Iommu
Jun 25, 2026
Merged

Feat: IoMmu protocol#1728
phip1611 merged 3 commits into
rust-osdev:mainfrom
PelleKrab:Iommu

Conversation

@PelleKrab

Copy link
Copy Markdown
Contributor

This PR adds the IoMmu Protocol to uefi-rs, enabling easy DMA (Direct Memory Access) for devices. This is a draft to get some initial feedback before writing uefi-rs tests. It is already working in my internal project. The main areas I would like feedback on are:

  • Mapping and DMA Buffer Types: Is this too much abstraction? Should we simplify it by requiring the developer to manually call unmap() and free_buffer()?
  • Should free_buffer_raw() and unmap_raw() be made public?
  • Repository Organization: Is the general organization within the repo appropriate?

General Implementation:

// Allocate DMA buffer
let sz = some_buffer.len();
let page_num = bytes_to_pages(sz);
let mut host_buffer = iommu.allocate_buffer(
    MemoryType::BOOT_SERVICES_DATA,
    page_num,
    EdkiiIommuAttribute::MEMORY_CACHED,
)?;

// Map buffer for DMA
let (device_addr, mapping, wrt_size) = iommu.map(
    EdkiiIommuOperation::BUS_MASTER_COMMON_BUFFER,
    &host_buffer,
    size,
)?;

// Give permissions to the specified device 
iommu.set_attribute(some_device_handle, &mapping, EdkiiIommuAccess::READ)?;

// Copy firmware to DMA buffer
let dma_buf = host_buffer.deref_mut();
if sz > wrt_size {
    return Status::BUFFER_TOO_SMALL;
}
dma_buf[..sz].copy_from_slice(&some_buffer[..sz]);

// Notify the Device and pass the `device_addr`

ref:
#1723

@nicholasbishop

Copy link
Copy Markdown
Member

Thanks for working on this :) I haven't had a chance to look at the uefi wrapper in detail yet, but overall it looks reasonable. Could you split the uefi-raw part out to a separate PR? That should be straightforward to review and merge in isolation.

@nicholasbishop

Copy link
Copy Markdown
Member

Is this ready for review? Currently it's marked as draft still. (No worries if that's intentional, just want to be sure you're not waiting on review.)

@PelleKrab

Copy link
Copy Markdown
Contributor Author

I still need to write some test for it. I have been busy with some other things, so I'll send those over as soon as I can. Feel free to take a look if you want.

@PelleKrab

Copy link
Copy Markdown
Contributor Author

@nicholasbishop I am trying to write some test for this, but I cant seem to get a protocol handle. I went into the uefi shell of the qemu vm, and the IoMmu driver image is present:

dh -p 4e939de9-d948-4b0f-88ed-e6e1ce517c1e -v
Handle dump by protocol '4E939DE9-D948-4B0F-88ED-E6E1CE517C1E'
Shell> dh -p 03c4e603-ac28-11d3-9a2d-0090273fc14d -v
Handle dump by protocol 'PXEBaseCode'
Shell> dh 28 -v
28: 3ECF0C18
ImageDevicePath(3ECF2A98)
  Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(8657015B-EA43-440D-949A-AF3BE365C0FC)
LoadedImage(3ECF20C0)
  Name..........: IoMmuDxe

So I am assuming the driver is failing to find hardware. I have tried a few different things, different EDK2 builds and followed the virtio instructions here, but nothing has worked. Ik it worked on hardware, so if you have any ideas on how to get qemu setup for IoMmu please let me know.

@phip1611

Copy link
Copy Markdown
Member

So I am assuming the driver is failing to find hardware. I have tried a few different things, different EDK2 builds and followed the virtio instructions here, but nothing has worked.

Thanks for trying to find a solution! Is there any update here? Have you tried to add an IOMMU to the QEMU VM? Perhaps one is needed so that the protocol will be installed one some handles (-device intel-iommu)?

@phip1611

Copy link
Copy Markdown
Member

Any update here @PelleKrab ? If not, I'm in favor of closing the PR for now.

@phip1611 phip1611 changed the title Feat: IoMmu protocol [STALE] Feat: IoMmu protocol Jan 25, 2026
@PelleKrab

Copy link
Copy Markdown
Contributor Author

Any update here @PelleKrab ? If not, I'm in favor of closing the PR for now.

No significant progress yet. I've been working on this here and there. Right now, I'm building a custom OVMF image to see if that forces the xtask VM to recognize the IOMMU drivers. I'm not sure if there’s wider interest in that approach, but I'm giving it a try since no other QEMU environment configs seem to help. I'm fine with closing the issue for now, and I can reopen it if I find a solution.

@phip1611

phip1611 commented Jan 26, 2026

Copy link
Copy Markdown
Member

I'm fine with closing the issue for now, and I can reopen it if I find a solution.

Thanks! Good luck with your investigations. We are very happy for every contribution and feel free to reopen / open a new one as soon as you have a solution.

For good project hygiene, we aim for short-living PRs (less than 3 months best)

@phip1611 phip1611 closed this Jan 26, 2026
@PelleKrab

Copy link
Copy Markdown
Contributor Author

@phip1611 I got the tests to run! It will require some changes to omvf-prebuilt, check out my branch here: link. If these additions are fine, let me know. Then I can open a PR there and we can reopen this PR with the new test cases.

@phip1611

Copy link
Copy Markdown
Member

very cool! regarding ovmf-prebuilt I think @nicholasbishop is the expert - from my side it is looking good

@PelleKrab

Copy link
Copy Markdown
Contributor Author

@phip1611 Everything is working now! Could you reopen this and take a look?

@phip1611 phip1611 reopened this Mar 11, 2026
@phip1611

phip1611 commented Mar 11, 2026

Copy link
Copy Markdown
Member

Please rebase / solve the conflict and remove all Merge branch 'main' into Iommu commits! We don't do that, we use the rebase flow for feature branches :)

@phip1611
phip1611 marked this pull request as ready for review March 11, 2026 11:41
@phip1611 phip1611 changed the title [STALE] Feat: IoMmu protocol Feat: IoMmu protocol Mar 11, 2026
@PelleKrab

Copy link
Copy Markdown
Contributor Author

The x86_64 CI jobs now require QEMU 9+ because they use -device intel-iommu. We can pin the CI to Ubuntu 25.10 (QEMU 10.1) for now or wait for the next LTS release. Additionally, both the new OVMF and the versions prior to the IOMMU changes seem to fail the HTTPS test. Let me know if you have any suggestions on that front.

@phip1611

Copy link
Copy Markdown
Member

thanks for working on that! @nicholasbishop any idea why this fails?

@phip1611

Copy link
Copy Markdown
Member

Sorry, I lost focus and forgot to review. What is the status here? Do you still plan to upstream this? @PelleKrab

@PelleKrab

Copy link
Copy Markdown
Contributor Author

Sorry, I lost focus and forgot to review. What is the status here? Do you still plan to upstream this? @PelleKrab

All good, I believe it is the same issue with VM support for IOMMU on the CI OS version. I haven't looked at it recently, but I may be able to take a look at this weekend and see if anything has changed so that we can get the test to work in CI.

@PelleKrab

Copy link
Copy Markdown
Contributor Author

I did some more testing in docker and in order to get the test passing we need a newer version of qemu (v10.1.0+), not necessarily a new Ubuntu version as per my last commit. Are there any dependencies or reasons stopping us from updating to a newer version? Also the current test failure seems to be some network timeout flakiness.

@phip1611
phip1611 self-requested a review June 9, 2026 11:31
phip1611 added a commit that referenced this pull request Jun 10, 2026
This will unblock #1728.
Also, it reverts #1884 which
reverted #1876. We need the
new version to unblock [0].

We however need to take care if the same CI failure causing #1884 occurs
again or now.

[0] #1728 (comment)
@PelleKrab

Copy link
Copy Markdown
Contributor Author

Interesting the invalid opcode failure is gone, but https test is failing. It used to fail on main locally, so I always assumed it was flaky. It appears to be some weird blocking, possibly requiring a bug fix on the OVMF. I have time this weekend to try to get this fixed.

@phip1611

phip1611 commented Jun 12, 2026

Copy link
Copy Markdown
Member

Interesting the invalid opcode failure is gone, but https test is failing. It used to fail on main locally, so I always assumed it was flaky. It appears to be some weird blocking, possibly requiring a bug fix on the OVMF. I have time this weekend to try to get this fixed.

I think it would make sense to also bump ovmf. I can also try to investigate next week

Thanks!

@phip1611

Copy link
Copy Markdown
Member

I'm already looking into the OVMF bump FYI

@PelleKrab

PelleKrab commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

So it appears edk2-stable202511-r2 introduced the HTTPS test failure. EDK2_STABLE202602_R1 introduced IOMMU support for Intel x86, so it inherited the breaking change. I rebuilt EDK2_STABLE202502_R2 with Intel IOMMU support, with a few small changes, and all uefi-rs tests passed locally. Although, I'm not sure what is causing the breaking change.

@phip1611

phip1611 commented Jun 15, 2026

Copy link
Copy Markdown
Member

I am so happy that we can finally unblock this! In the last days, I've

I think if you rebase, we can finally proceed with a green CI 🥳

@phip1611 phip1611 linked an issue Jun 15, 2026 that may be closed by this pull request
@PelleKrab
PelleKrab force-pushed the Iommu branch 2 times, most recently from 49aca86 to 1bc4146 Compare June 17, 2026 04:04
@PelleKrab

Copy link
Copy Markdown
Contributor Author

Finally, all green!!!! Thank you for all the help.

@phip1611 phip1611 self-assigned this Jun 17, 2026

@phip1611 phip1611 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - thanks for the patience!

I found at least two memory issues (assisted by Codex/GPT-5.5). These unit tests currently succeed but they should fail (or not even compile):

fn test_mapping_can_outlive_buffer(iommu: &Iommu) {
    let attributes = EdkiiIommuAttribute::MEMORY_CACHED;

    let mapping = {
        let buffer = iommu
            .allocate_buffer(MemoryType::BOOT_SERVICES_DATA, 1, attributes)
            .expect("Failed to allocate IOMMU buffer");

        let (_device_address, mapping, mapped_bytes) = iommu
            .map(EdkiiIommuOperation::BUS_MASTER_READ, &buffer, buffer.size())
            .expect("Failed to map IOMMU buffer");
        assert_eq!(mapped_bytes, buffer.size());

        mapping
    };

    drop(mapping);
}

fn test_map_accepts_oversized_length(iommu: &Iommu) {
    let attributes = EdkiiIommuAttribute::MEMORY_CACHED;
    let buffer = iommu
        .allocate_buffer(MemoryType::BOOT_SERVICES_DATA, 1, attributes)
        .expect("Failed to allocate IOMMU buffer");
    let oversized_length = buffer.size() + 1;

    let (_device_address, mapping, mapped_bytes) = iommu
        .map(
            EdkiiIommuOperation::BUS_MASTER_READ,
            &buffer,
            oversized_length,
        )
        .expect("IOMMU map accepted an oversized buffer length");

    assert!(oversized_length > buffer.size());
    assert_eq!(mapped_bytes, oversized_length);

    drop(mapping);
}

Problems:

  1. Mapping is only tied to &Iommu, not to the DmaBuffer.
    Safe code can drop/free the DMA buffer while the mapping is still alive, then unmap later.

  2. Iommu::map() accepts an arbitrary number_of_bytes.
    Safe code can pass a length larger than the allocated DMA buffer, causing firmware to map memory outside the buffer.

Suggestions:

  1. Tie Mapping to the mapped buffer lifetime, for example Mapping<'iommu, 'buf> with PhantomData<&'buf DmaBuffer<'iommu>>, and make map() return a mapping that cannot outlive host_buffer.
  2. Remove the free-form length from the safe API, or validate it:
    number_of_bytes <= host_buffer.size() should be enforced before calling firmware. A cleaner API would map the whole buffer by default and provide a checked subrange API if needed.

Comment thread uefi-test-runner/src/proto/iommu.rs
Comment thread uefi-test-runner/src/proto/iommu.rs
Comment thread uefi-test-runner/src/proto/iommu.rs
Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread xtask/src/qemu.rs Outdated

@phip1611 phip1611 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the lifetime issue is gone! I think there are a few more parts of the code that need further safety tightening.

Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs
impl<'a> Deref for DmaBuffer<'a> {
type Target = [u8];

fn deref(&self) -> &[u8] {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure but I think we should replace this with

unsafe fn as_bytes(&self) -> &[u8] as the mapping might be mutuably in use while we are holding a shared reference to it. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are 100% correct, there are scenarios where this memory can be written to or cleared by the system, so exposing a shared slice safely through Deref is too strong.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think making map() take &mut DmaBuffer and tying Mapping to that mutable borrow should be enough here. The problematic case is safe Rust accessing the buffer while an active DMA mapping exists. If Mapping holds a PhantomData<&mut DmaBuffer>, then safe code cannot use Deref/DerefMut on the buffer until the mapping is dropped.

Comment thread uefi/src/proto/dma/mod.rs

@phip1611 phip1611 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this. A few more comments.

Also, please clean up the git commit history. Squash everything into three commits:

  • uefi-raw: Derive more traits for IOMMU protocol
  • uefi: add EDKII IOMMU protocol
  • feat(test-runner): add IOMMU test coverage

Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs Outdated
Comment thread uefi/src/proto/dma/mod.rs Outdated
@phip1611

Copy link
Copy Markdown
Member

Please fix CI. I think this is looking very good now! Thanks for all your patience! 🥳 Does this still work for you use-case, even with all the changes you've applied so far?

@PelleKrab

PelleKrab commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

I am no longer working on anything that needs this, but from what I can tell it should have all the functionality that my old team needs. Thank you for all the code review help!

@phip1611 phip1611 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woop woop here comes the merge train 🚄 🚀

@phip1611
phip1611 added this pull request to the merge queue Jun 25, 2026
Merged via the queue into rust-osdev:main with commit 838b978 Jun 25, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RFC: Add EDK2 IoMmu Support for DMA in uefi-rs

3 participants