Skip to content

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
merged 3 commits into from
Jun 10, 2023

Conversation

phip1611
Copy link
Member

@phip1611 phip1611 commented May 25, 2023

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 requested a review from nicholasbishop May 25, 2023 11:00
@phip1611 phip1611 force-pushed the integration-test-load-image branch from c748974 to 88f46be Compare May 25, 2023 11:22
@medhefgo
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
@phip1611
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!

@nicholasbishop
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.

@nicholasbishop
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 phip1611 force-pushed the integration-test-load-image branch from fd1a9ca to 6739187 Compare May 25, 2023 14:07
@medhefgo
Copy link
Contributor

medhefgo commented May 25, 2023

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.

@phip1611
Copy link
Member Author

phip1611 commented May 25, 2023

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
@phip1611
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
@phip1611
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
@phip1611 phip1611 requested a review from nicholasbishop June 10, 2023 07:39
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants