Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Integration test for boot service function load_image #826

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

Merged
nicholasbishop merged 3 commits into rust-osdev:main from phip1611:integration-test-load-image
Jun 10, 2023

Conversation

Copy link
Member

@phip1611 phip1611 commented May 25, 2023
edited
Loading

A colleague of mine (ping @scholzp) was about to use the load_image function of UEFI boot services to load an image into memory. While working on a minimal example, he encountered an unintuitive bug. As a consequence, I thought it's useful to have a valid integration test for that function which others can use as template.

(削除) The test currently fails. What's wrong there with the chosen approach, @nicholasbishop ? UEFI fails with NOT_FOUND when load_image is executed. (削除ここまで)

Checklist

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

@phip1611 phip1611 self-assigned this May 25, 2023
@phip1611 phip1611 force-pushed the integration-test-load-image branch 2 times, most recently from 9ea69f2 to c748974 Compare May 25, 2023 10:57
@phip1611 phip1611 force-pushed the integration-test-load-image branch from c748974 to 88f46be Compare May 25, 2023 11:22
Copy link
Contributor

As to why strategy A fails: get_image_file_system() tries to exclusively open LoadedImage protocol on the handle, which you already have open in this test function.

Personally, I am not a fan of seeing this crate throwing exclusive opening of protocols down everyone's throat.

This is what the spec says about exclusive opening:

EXCLUSIVE Used by applications to gain exclusive access to a protocol interface. If any drivers have the protocol
interface opened with an attribute of BY_DRIVER, then an attempt will be made to remove them by calling the driver’s
Stop() function.

This does not sound like something you'd wanna be doing willy-nilly with any protocol you may wanna open. I can also see child EFI apps not being able to open their own LoadedImage protocol just because the parent didn't bother to put their use of it inside a scope to force dropping it before starting the child.

There is also the matter of firmware/drivers already having an exclusive handle to something (I cannot get a GOP handle exclusively on my ASUS firmware + nvidia combo for example). So nudging everyone to use exclusive mode is just gonna result in a lot of annoyance.

Yes, the spec is annoyingly vague about all these protocol opening things. But the fact is, everyone on the C-side uses the old HandleProtocol or OpenProtocol+GET_PROTOCOL and it seems to work in practice.

@phip1611 phip1611 force-pushed the integration-test-load-image branch 3 times, most recently from e5b6b3e to fd1a9ca Compare May 25, 2023 13:52
Copy link
Member Author

Thanks very much @medhefgo for your help and advices! I've updated the PR.
Also what you've mentioned about open_protocol_exclusive is a very good point. Thanks for the input!

Copy link
Member

Personally, I am not a fan of seeing this crate throwing exclusive opening of protocols down everyone's throat.

Well, that's a bit extreme :) For cases where you don't want an exclusive protocol, open_protocol can be used. We don't force anyone to open in exclusive mode.

Copy link
Member

One note, there will be overlap between this test and https://github.com/rust-osdev/uefi-rs/pull/793/files for testing shell protocols. I'm not necessarily opposed to having a bit of duplication in the tests though, just pointing it out.

phip1611 reacted with thumbs up emoji

@phip1611 phip1611 force-pushed the integration-test-load-image branch from fd1a9ca to 6739187 Compare May 25, 2023 14:07
Copy link
Contributor

medhefgo commented May 25, 2023
edited
Loading

Well, that's a bit extreme :) For cases where you don't want an exclusive protocol, open_protocol can be used. We don't force anyone to open in exclusive mode.

Any rust programmer worth their salt will use the non-unsafe interface, that's as good as forcing them (especially when the unsafe version is also unergonomic wrt to parameters to pass). Whether that interface is better/does the right thing is the question.

Copy link
Member Author

phip1611 commented May 25, 2023
edited
Loading

Any rust programmer worth their salt will use the non-unsafe interface, that's as good as forcing them

I agree, that's a valid point. Cognitive load is already relatively high (削除) in this crate (削除ここまで) when working with UEFI. So users will refrain from using more "unsafety" when there are "safe" interfaces

@phip1611 phip1611 force-pushed the integration-test-load-image branch from 6739187 to 1463fc5 Compare May 25, 2023 14:43
@phip1611 phip1611 force-pushed the integration-test-load-image branch 2 times, most recently from 8df663b to 8a9888a Compare May 25, 2023 15:15
@phip1611 phip1611 marked this pull request as draft June 9, 2023 09:45
Copy link
Member Author

phip1611 commented Jun 9, 2023

Needs a rebase onto #849

@phip1611 phip1611 force-pushed the integration-test-load-image branch from 8a9888a to a3f2c56 Compare June 9, 2023 20:40
@phip1611 phip1611 marked this pull request as ready for review June 9, 2023 20:42
Copy link
Member Author

phip1611 commented Jun 9, 2023

Are we good to go now? @nicholasbishop

@phip1611 phip1611 force-pushed the integration-test-load-image branch 3 times, most recently from 545cf23 to bb18eba Compare June 9, 2023 21:06
@phip1611 phip1611 force-pushed the integration-test-load-image branch 3 times, most recently from aca00d5 to 1d46a39 Compare June 9, 2023 21:19
@phip1611 phip1611 force-pushed the integration-test-load-image branch from 1d46a39 to e9c69cb Compare June 10, 2023 07:37
@nicholasbishop nicholasbishop merged commit 8b21b16 into rust-osdev:main Jun 10, 2023
@phip1611 phip1611 deleted the integration-test-load-image branch June 10, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicholasbishop nicholasbishop nicholasbishop approved these changes

+1 more reviewer

@medhefgo medhefgo medhefgo left review comments

Reviewers whose approvals may not affect merge requirements
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /