Skip to content

Conversation

nicholasbishop
Copy link
Member

Checklist

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

@@ -252,7 +252,7 @@ impl PartialEq for DevicePathInstance {
/// [module-level documentation]: crate::proto::device_path
/// [`END_ENTIRE`]: DeviceSubType::END_ENTIRE
#[repr(C, packed)]
#[unsafe_protocol("09576e91-6d3f-11d2-8e39-00a0c969723b")]
#[unsafe_protocol(uefi_raw::protocol::device_path::DevicePathProtocol::GUID)]
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand this.

We now have DevicePath[Protocol] twice with the same GUID but with different layouts? Can you clarify this confusion, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed confusing! Here's an overview:

The uefi-raw version exactly matches the definition in the spec:

typedef struct _EFI_DEVICE_PATH_PROTOCOL {
  UINT8           Type;
  UINT8           SubType;
  UINT8           Length[2];
 } EFI_DEVICE_PATH_PROTOCOL;

Those fields always exist at the start of a device path, and indeed at the start of each node within the device path. So it's really just a header (and in uefi, we have the corresponding DevicePathHeader struct).

That's not a very convenient way to interact with device paths though, since usually you care about more than just the header of the first node within the device path. So, in uefi, we use a DST instead. Since the DevicePath is a very flexible structure, we represent this struct as just containing [u8], a bag of bytes. Of course, C has no notion of a DST, so the layouts are not directly compatible. That's where ProtocolPointer comes in. That trait is used when opening a dynamically-sized protocol like DevicePath; it parses the device path data to figure out how big it is and creates the wide pointer used to reference a DST.

Incidentally, I originally started handling device paths as DSTs due to Miri. In the Stacked Borrows model, you're not allowed to read bytes past a header after you've created a reference from the raw pointer. In the new Tree Borrows model this is allowed. The exact borrowing model that will eventually be blessed as Rust's official model is not yet known, and will probably land somewhere between Stacked and Tree, but it seems likely that in the future we might not need DSTs here, and at that time we'll have to evaluate whether we want DSTs or not. I think for now we need them, but I do not yet have a strong opinion either way as to whether we want them for this use case.

Copy link
Member

@phip1611 phip1611 Jun 9, 2023

Choose a reason for hiding this comment

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

Ack, thanks for the write-up. Do you think it's worth it to enhance the docstring in Rust with more information?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, some more docs around device paths would be good. I've added that to my todo list.

@nicholasbishop nicholasbishop merged commit 75430f0 into rust-osdev:main Jun 9, 2023
@nicholasbishop nicholasbishop deleted the bishop-dpraw branch June 9, 2023 22:07
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.

2 participants