Implement memory management and mapping functions for PCI Root bridge io protocols by tmvkrpxl0 · Pull Request #1724 · rust-osdev/uefi-rs

tmvkrpxl0 · 2025-07-17T16:29:47Z

This is continuation of #1705
As requested by @phil-opp I decided to split the pr into multiple.
This one only contains wrapper for pages allocated and regions mapped by the protocol.

Motivation

When I was writing demo program for playing sound in uefi, I needed to access audio devices on PCI root bridge. Unfortunately back then It did not have abstraction for communicating with those. In fact it lacked a lot of PCI root bridge protocol's features. I decided to implement all of these. Starting with memory management and mapping. Which was crucial for sending raw audio data.

Checklist

Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
Update the changelog (if necessary)

nicholasbishop · 2025-08-02T20:44:05Z

+
+impl Drop for PciMappedRegion<'_, '_> {
+    fn drop(&mut self) {
+        let status = unsafe { (self.proto.unmap)(self.proto, self.key) };


Would it make sense to add a fn unmap(self) -> Result method, so that callers can choose to handle errors from unmapping? The Drop trait doesn't really allow that. I think the drop implementation should probably not panic on errors, and that we should remove the unreachable; UEFI implementations sometimes return additional errors beyond what the spec includes.

Ditto for the PciPage implementation.

True, I initially put unreachable macro there to indicate that "errors that may happen according to spec are all handled and no other error can occur"
But yeah second thought we can never be sure

Tho, what else should it do when an error occurs? Should I make it undroppable(assuming it's possible) so that users must use functions like unmap?

There's no way to make something undroppable in Rust, unfortunately. There are two options:

A 'drop bomb', i.e. panic unconditionally in drop if unmap wasn't called. The documentation should strongly remind the user that unmap must be called.

Handle errors in drop by ignoring them (or log them and otherwise ignore). The documentation can encourage users to call unmap for better error handling, but it's not required. This is what File does, for example; it suggests calling sync_all before drop if you want to handle errors.

In cases where errors are unlikely (which I think is the case here), I prefer the 2nd option; log errors in drop but otherwise ignore them.

There's no way to make something undroppable in Rust

What about ManuallyDrop::new(val)?

Hello, I'm back. Sorry for this long black period. Anyway, for ManuallyDrop, that's not feasible here because the intention was to make drop calls explicit so that error occurred during clean-up can be handled, and ManuallyDrop doesn't fit for that purpose.

tmvkrpxl0 · 2025-08-03T09:49:31Z

Well test fails on windows because it can't use new pci memory device

nicholasbishop · 2025-08-08T19:38:35Z

Well test fails on windows because it can't use new pci memory device

Might need to just skip this test on Windows. Take a look at https://github.com/rust-osdev/uefi-rs/blob/main/xtask/src/main.rs#L149 for examples of how we control test features.

phip1611 · 2025-10-18T16:06:21Z

+    cmd.arg("-device");
+    cmd.arg("ivshmem-plain,memdev=hostmem,id=hostmem");
+    cmd.arg("-object");
+    cmd.arg("memory-backend-file,size=1M,share,mem-path=/dev/shm/ivshmem,id=hostmem");


well I think if this tests runs under windows, we instead need something like ...mem-path=C:\\temp\\ivshmem,size=1M,share=on here

phip1611 · 2025-10-18T16:07:16Z

Please

rebase your local branch onto the latest main branch
either exlude the failing test on windows as Nicholas suggested or try this
you'll need a force-push after the rebase (please do not merge main into your feature branch)

Then we can get this merged.

tmvkrpxl0 · 2025-12-28T08:08:17Z

@phip1611 I rebased my current branch and updated testing code. I don't have windows machine to locally test it out, but since the file path is now local instead of trying to look it up in /dev, I think it'd work

phip1611 · 2026-01-25T10:57:03Z

@phip1611 I rebased my current branch and updated testing code. I don't have windows machine to locally test it out, but since the file path is now local instead of trying to look it up in /dev, I think it'd work

could you please have another look? CI is still failing

tmvkrpxl0 · 2026-01-26T00:08:17Z

@phip1611

Error: failed to open file `\\.\pipe\serial`: The system cannot find the file specified. (os error 2)

?
I don't even know what is "pipe\serial"
CI recently failed for me too (for some reason it randomly ran without my intervention)
And the reason was it failed to install Qemu so I just reran it and it succeed
Maybe it's something similar?

phip1611 · 2026-02-22T20:54:24Z

@phip1611
Error: failed to open file ``\\.\pipe\serial``: The system cannot find the file specified. (os error 2)
? I don't even know what is "pipe\serial" CI recently failed for me too (for some reason it randomly ran without my intervention) And the reason was it failed to install Qemu so I just reran it and it succeed Maybe it's something similar?

The interesting error is the line above: qemu-system-x86_64: invalid object type: memory-backend-file

Please either fix your CI test for windows or exclude it on Windows so that we can proceed here

tmvkrpxl0 · 2026-03-02T07:45:19Z

I am working on git history. Sorry for working on this so slowly.

tmvkrpxl0 · 2026-03-02T10:39:26Z

It took a while because I installed windows 10 VM and tried to look for alternatives but I couldn't find any. I ended up just excluding the test on windows.

tmvkrpxl0 · 2026-03-04T10:53:20Z

I decided to implement just two protocol seeing that PR only added structs, not functionality

tmvkrpxl0 · 2026-03-16T16:03:37Z

Question, should parts of memory management feature gated behind "alloc"? It doesn't use alloc crate but it does allocate memory, by using UEFI implementation.

phip1611 · 2026-03-16T18:23:52Z

Question, should parts of memory management feature gated behind "alloc"? It doesn't use alloc crate but it does allocate memory, by using UEFI implementation.

no that's fine. Currently, feature alloc is for the alloc crate

tmvkrpxl0 · 2026-03-21T03:13:28Z

I think it's ready for review now. I removed all panic paths from drop as requested and all github ci actions pass

phip1611

Generally, I think it is good to have this. A few remarks and opens:

Please add a motivation to the PR (why you need this, where it is useful)
- One-two sentences are enough, but reviewers should see the value-proposition
Please clean up your git commit history. We do not accept do A, do B, fix A, fix B, fix A even more commits.
The git commit history should cleanly lead reviewers how you got from A to B where
- A is the base
- B your final design
- and there are no interim designs (no fix A, no design A, revert design A, use design B)

tmvkrpxl0 · 2026-03-23T03:01:01Z

I have rewrote git history to just 3 commits. Each of them passes checks done in github CI.
I also changed PciRootBridgeIo::map to accept offset as well. ~~I'll attach current implementation below.~~ Well I thought github is gonna display the whole function but it does not.

tmvkrpxl0 · 2026-03-23T03:11:42Z

+        operation: PciRootBridgeIoProtocolOperation,
+        to_map: &'r PciBuffer<'p, T>,
+        offset: usize,
+    ) -> crate::Result<(PciMappedRegion<'p, 'r, T>, usize)> {


The offset parameter is for calling this function multiple times. UEFI specification mentions that:

In all mapping requests the resulting NumberOfBytes actually mapped may be less than the requested amount. In this case, the DMA operation will have to be broken up into smaller chunks. The Map() function will map as much of the DMA operation as it can at one time. The caller may have to loop on Map() and Unmap() in order to complete a large DMA transfer.

This means users will have to call this function multiple times while moving the pointer forward. We could also provide function for looping until entire buffer is mapped, but bug in PCI implementation can return weird value for length of bytes forward, potentially causing infinite loop. That's probably unlikely though.

tmvkrpxl0 marked this pull request as ready for review July 18, 2025 01:43

nicholasbishop reviewed Aug 2, 2025

View reviewed changes

phip1611 force-pushed the pci_protocol_base branch from 3a85c72 to 556049b Compare October 18, 2025 15:29

phip1611 reviewed Oct 18, 2025

View reviewed changes

tmvkrpxl0 force-pushed the pci_protocol_base branch 3 times, most recently from fc77f22 to 4f6eadd Compare December 28, 2025 08:02

phip1611 mentioned this pull request Jan 25, 2026

Release Plan: v0.37, v0.38, and beyond #1878

Open

11 tasks

phip1611 force-pushed the pci_protocol_base branch from 4f6eadd to 6d51948 Compare January 26, 2026 05:42

phip1611 force-pushed the pci_protocol_base branch from 6d51948 to 754a3d1 Compare February 22, 2026 20:53

tmvkrpxl0 force-pushed the pci_protocol_base branch 5 times, most recently from 19e5754 to 7bdd369 Compare March 2, 2026 10:35

tmvkrpxl0 changed the title ~~Implement base for PCI Root bridge io protocols~~ Implement memory management and mapping functions for PCI Root bridge io protocols Mar 4, 2026

tmvkrpxl0 force-pushed the pci_protocol_base branch 2 times, most recently from aaff544 to c9a37f1 Compare March 4, 2026 10:47

tmvkrpxl0 force-pushed the pci_protocol_base branch 2 times, most recently from 0783864 to c6fbe33 Compare March 4, 2026 12:12

tmvkrpxl0 force-pushed the pci_protocol_base branch from 520653b to c6f7d66 Compare March 16, 2026 16:19

phip1611 requested changes Mar 21, 2026

View reviewed changes

tmvkrpxl0 force-pushed the pci_protocol_base branch 2 times, most recently from 5079ec7 to ffb7358 Compare March 23, 2026 02:48

tmvkrpxl0 force-pushed the pci_protocol_base branch from ffb7358 to 264183f Compare March 23, 2026 03:06

tmvkrpxl0 commented Mar 23, 2026

View reviewed changes

tmvkrpxl0 force-pushed the pci_protocol_base branch 3 times, most recently from dda6680 to d0d0c5b Compare March 23, 2026 03:31

tmvkrpxl0 added 3 commits March 23, 2026 12:32

uefi: Add bitflags for PciRootBridgeIoProtocolAttribute

d8dd7af

uefi: Add wrapper for pages allocated by PCI protocol

b7a5541

uefi: Add wrapper for the mapped region of memory

0c4115d

tmvkrpxl0 force-pushed the pci_protocol_base branch from d0d0c5b to 0c4115d Compare March 23, 2026 03:32

tmvkrpxl0 requested a review from phip1611 March 23, 2026 03:33

Uh oh!

Conversation

tmvkrpxl0 commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmvkrpxl0 commented Aug 3, 2025

Uh oh!

nicholasbishop commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phip1611 commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmvkrpxl0 commented Dec 28, 2025

Uh oh!

phip1611 commented Jan 25, 2026

Uh oh!

tmvkrpxl0 commented Jan 26, 2026

Uh oh!

phip1611 commented Feb 22, 2026

Uh oh!

tmvkrpxl0 commented Mar 2, 2026

Uh oh!

tmvkrpxl0 commented Mar 2, 2026

Uh oh!

tmvkrpxl0 commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmvkrpxl0 commented Mar 16, 2026

Uh oh!

phip1611 commented Mar 16, 2026

Uh oh!

tmvkrpxl0 commented Mar 21, 2026

Uh oh!

phip1611 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmvkrpxl0 commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tmvkrpxl0 commented Jul 17, 2025 •

edited

Loading

phip1611 commented Oct 18, 2025 •

edited

Loading

tmvkrpxl0 commented Mar 4, 2026 •

edited

Loading

phip1611 left a comment •

edited

Loading

tmvkrpxl0 commented Mar 23, 2026 •

edited

Loading