Skip to content

Implement rest of Pci Root Bridge Io protocol #1705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tmvkrpxl0
Copy link

@tmvkrpxl0 tmvkrpxl0 commented Jun 21, 2025

I decided to implement missing features of Pci IO root bridge. It's not complete yet and I'm opening this PR beforehand to get a review.

Protocols to implement

  • allocate
  • free
  • map
  • unmap
  • copy
  • poll io
  • memory io
  • IO register io
  • attributes
  • configuration

Design Questions

All design questions are resolved as of now

I had to use RefCell for allocate, free, map, unmap and copy because I wanted allocated buffer and mapped region to clean themselves after they're dropped by calling free/unmap, and track their lifetimes so that they do not outlive protocol which allocated them (as it's required to free themselves), and for map function, do not outlive region they map.
I couldn't just use normal reference because copy method requires mutable pointer. I wasn't sure if I can turn this into const pointer because some other functions in PciRootBridgeIoProtocol struct takes in const pointer, so the original author for it must considered implications of this and I figured I should follow it.
However it's quite ugly now and users must use these functions like PciRootBridgeIo::map instead of protocol.map
Ideally it should work like this:

let protocol: ScopedProtocol<PciRootBridgeIo> = ...;
let buffer = protocol.allocate_buffer::<[u8; 4096]>(BOOT_SERVICES_DATA, None, PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE);
let mapped = protocol.map(buffer, PciRootBridgeIoProtocolOperation::BUS_MASTER_COMMON_BUFFER);
// mapped cannot be dropped earlier than buffer
// buffer cannot be dropped earlier than protocol

Fixed by switching to GhostCell

Does using GhostCell feel good?
Currently the test methods are written as such:

GhostToken::new(|token| {
    let pci_proto = get_open_protocol::<PciRootBridgeIo>(pci_handle);
    let token: RefCell<_> = token.into();

    let mut buffer = pci_proto.allocate_buffer::<[u8; 4096]>(
        &token,
        MemoryType::BOOT_SERVICES_DATA,
        None,
        PciRootBridgeIoProtocolAttribute::PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE,
    ).unwrap();
    let buffer = unsafe {
        buffer.assume_init_mut().fill(0);
        buffer.assume_init()
    };

    let mapped = pci_proto.map(
        &token,
        PciRootBridgeIoProtocolOperation::BUS_MASTER_COMMON_BUFFER64,
        buffer.as_ref(),
    );
    if mapped.region().device_address == buffer.as_ptr().addr() as u64 {
        info!("This PCI device uses identity mapping");
    } else {
        info!("This PCI device uses different mapping from CPU");
    }
});

Now all methods of PciRootBridgeIo only needs shared reference to itself but all of them also take in either shared or mutable reference to GhostToken
And the GhostToken is wrapped in RefCell as some types like PciMappedRegion or PciBuffer needs mutable reference to GhostToken but I don't want these to block others from using it either.

Fixed by switching everything to shared reference. mutable reference is no longer needed.

Checklist

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

@tmvkrpxl0
Copy link
Author

Currently cargo xtask run fails when testing copy method. The error is EFI_INVALID_PARAMETER

@tmvkrpxl0 tmvkrpxl0 changed the title Pci protocol Pci Root Bridge Io protocol Jun 28, 2025
@tmvkrpxl0 tmvkrpxl0 changed the title Pci Root Bridge Io protocol Implement rest of Pci Root Bridge Io protocol Jun 28, 2025
@tmvkrpxl0
Copy link
Author

I switched to GhostCell and finally got rid of that weird design where the self parameter can only be &RefCell<PciRootBridgeIo> and not &PciRootBridgeIo

@tmvkrpxl0
Copy link
Author

Well that was very messy but I finally cleaned out the git history. I think I'm ready for review.

@tmvkrpxl0 tmvkrpxl0 marked this pull request as ready for review June 30, 2025 01:40
@@ -21,6 +21,7 @@ rust-version = "1.85.1"
[dependencies]
bitflags.workspace = true
uguid.workspace = true
static_assertions.workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of adding a new dependency at this point. For consistency with the code base, please add a unit test instead. Something like

#[test]
fn test_abi() {
    assert_eq(size_of<...>, 0x123);
}

etc

Copy link
Member

Choose a reason for hiding this comment

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

You can also do a static assert manually, e.g. something like:

const _ASSERT_S_SIZE: () = {
    if core::mem::size_of::<S>() != 0x123 {
        panic!("bad size");
    }
};

Example in the playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=4dff15892d952d56edd3b79911f29581

Copy link
Member

Choose a reason for hiding this comment

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

Good point! But for consistency with the code base, I have a tendency towards a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

One potential reason to prefer a static assert is that it ensures the 'test' is run under the correct target. Unit tests are run on the host target, e.g. x86_64-unknown-linux-gnu. Generally this won't matter, but e.g. if the struct contained a usize field, the size of the struct might be different on i686-unknown-uefi. Not sure that's really relevant here though, so +1 to your point about consistency and just doing it as a unit test.

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.

3 participants