Implement additional `BootServices` functions #550

timrobertsdev · 2022-11-06T22:35:25Z

This PR implements basic protocol handler services such as (re/un)install_protocol_services and allows a user to be notified when a protocol is installed through the implementation of register_protocol_notify. To support the implementation of register_protocol_notify, a variant was added to SearchType and any functions using it updated. A new type SearchKey was added to abstract over the void pointer given by register_protocol_notify.

There was one enum added to support install_protocol_services that currently has only one variant, 'InterfaceType'. I have chosen to make this enum public to be forwards-compatible, but in the implementation of install_protocol_services I don't include an interface_type parameter yet as InterfaceType and simply pass the only variant. I'm not quite sure if this "middle of the road" solution is ideal and am looking for feedback.

Tests will be coming soon.

Checklist

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

phip1611 · 2022-11-07T09:46:35Z

Hi @timrobertsdev,
regarding the checklist, because I already saw this in your last PR:

the markup regarding the checkboxes is intended to be used like this:

- [X] like this
- [ X ] and not like this

as this way, checkboxes will be rendered properly, as here

check

phip1611

LGTM. What do @GabrielMajeri and @nicholasbishop think?

GabrielMajeri · 2022-11-07T09:50:33Z

The code looks very good, just waiting for some tests to be added and it's good to go.

timrobertsdev · 2022-11-07T16:33:00Z

@phip1611 Thanks for the heads up, I wasn't aware of the markup formatting.

Tests are in now. Some changes to install_protocol_interface had to be made as well.

src/data_types/mod.rs

CHANGELOG.md

src/data_types/mod.rs

src/table/boot.rs

nicholasbishop · 2022-11-10T21:27:33Z

Please rebase on latest main as well to fix the conflict in the changelog.

timrobertsdev · 2022-11-10T22:07:12Z

I thought that what I just did would rebase onto main and then put my commits over it but I guess I didn't do that quite right. @nicholasbishop would you be able to help out my failing git-fu?

nicholasbishop · 2022-11-11T21:29:52Z

I think maybe you rebased but your main branch wasn't up to date? Steps to rebase should be something like this:

# Update your local main branch
git checkout main
git pull

# Rebase your branch on main and push
git checkout your-branch
git rebase main
git push -f

Or, equivalently but without updating the local main branch:

git checkout your-branch
git fetch origin  # Assuming "origin" is the name of the rust-osdev/uefi-rs remote
git rebase origin/main  # Where origin is again the remote name
git push -f

timrobertsdev · 2022-11-12T14:26:44Z

That was it, I had sync'd my fork in github but forgot to actually update my local main. Thanks!

Edit: About to do it again because I didn't notice the last merge to main.

- Implement `BootServices::install_protocol_interface` - Implement `BootServices::uninstall_protocol_interface` - Implement `BootServices::reinstall_protocol_interface` - Implement `BootServices::register_protocol_notify` - Add `SearchType::ByRegisterNotify` variant. - Add `SearchType::from_search_key` - Add `SearchKey` type that abstracts over an opaque pointer given by `BootServices::register_protocol_notify`.

phip1611 · 2022-11-12T14:45:12Z

uefi-test-runner/src/main.rs

@@ -1,6 +1,7 @@
 #![no_std]
 #![no_main]
 #![feature(abi_efiapi)]
+#![feature(negative_impls)]


I can't see why this is required. Can you please give some details?

Yes, rustc said it was required when I was implementing my dummy protocol with #[derive(Protocol].

Specifically in uefi-test-runner/src/boot/misc.rs:

/// Dummy protocol for tests #[unsafe_guid("1a972918-3f69-4b5d-8cb4-cece2309c7f5")] #[derive(Protocol)] struct TestProtocol {}

Alright, thanks for the pointer. What does @nicholasbishop say? Is this fine?

I think this is fine, just a consequence of the way we implement protocols.

(There's a little bit of commentary on negative_impl in #452; it might be something we want to drop in the future. But for this PR it's fine.)

phip1611 · 2022-11-12T14:46:24Z

uefi-test-runner/src/boot/misc.rs

@@ -112,7 +112,7 @@ fn test_install_protocol_interface(bt: &BootServices) {
            .expect("Failed to install protocol interface")
    };

-    let exists = bt
+    let _ = bt


nit: These changes could be squashed with the commit(s) where the changes were introduced.

However, this is not a hard requirement.

I need to learn git a bit better anyway, so I'll see what I can do about cleaning up the history a bit today.

👍 Moving things between existing git commits is kinda hard without experience. In the last months, I've spent some time on improving my skills. The following workflow worked for me. Maybe it helps you as well, hopefully:

git rebase --interactive HEAD~4 (for four commits)

in the editor, mark all commits you want to change with e/edit

run cargo fmt && cargo clippy plus git add all changes

run git rebase --continue

While commits are left, goto 3.

Ideally, git should discard the "code style only commit" if you can pull these changes into earlier commits this way. Let me know what worked for you! :)

That actually was a really easy way to understand how to split commits apart, thanks! I had no idea that git would detect and let you remove empty commits.

phip1611 · 2022-11-12T14:51:39Z

Thanks for the contribution. I'd wait with the merge until @nicholasbishop addressed the comment above.

Adjust changelog Rename `SearchKey` to `ProtocolSearchKey` and move to `boot.rs`. Fixup doclinks. Adjust signatures of the `[re/un]install_protocol_interface` functions

nicholasbishop

Lgtm, thanks :)

phip1611 reviewed Nov 7, 2022

View reviewed changes

nicholasbishop requested changes Nov 10, 2022

View reviewed changes

phip1611 reviewed Nov 12, 2022

View reviewed changes

phip1611 approved these changes Nov 12, 2022

View reviewed changes

timrobertsdev added 3 commits November 12, 2022 10:11

Correct implementation of install_protocol_interface

89688e6

Add tests for the protocol handler services.

94b5ee1

Apply suggestions from @nicholasbishop

4d6aabe

Adjust changelog Rename `SearchKey` to `ProtocolSearchKey` and move to `boot.rs`. Fixup doclinks. Adjust signatures of the `[re/un]install_protocol_interface` functions

nicholasbishop approved these changes Nov 12, 2022

View reviewed changes

nicholasbishop merged commit 8f74777 into rust-osdev:main Nov 12, 2022

timrobertsdev deleted the protocol-handler-services branch November 13, 2022 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement additional `BootServices` functions #550

Implement additional `BootServices` functions #550

timrobertsdev commented Nov 6, 2022 •

edited by phip1611

Loading

phip1611 commented Nov 7, 2022

phip1611 left a comment

GabrielMajeri commented Nov 7, 2022

timrobertsdev commented Nov 7, 2022 •

edited

Loading

nicholasbishop commented Nov 10, 2022

timrobertsdev commented Nov 10, 2022

nicholasbishop commented Nov 11, 2022

timrobertsdev commented Nov 12, 2022 •

edited

Loading

phip1611 Nov 12, 2022

timrobertsdev Nov 12, 2022 •

edited

Loading

phip1611 Nov 12, 2022

nicholasbishop Nov 12, 2022

phip1611 Nov 12, 2022 •

edited

Loading

timrobertsdev Nov 12, 2022

phip1611 Nov 12, 2022 •

edited

Loading

timrobertsdev Nov 12, 2022

phip1611 commented Nov 12, 2022

nicholasbishop left a comment

Implement additional BootServices functions #550

Implement additional BootServices functions #550

Conversation

timrobertsdev commented Nov 6, 2022 • edited by phip1611 Loading

Checklist

phip1611 commented Nov 7, 2022

phip1611 left a comment

Choose a reason for hiding this comment

GabrielMajeri commented Nov 7, 2022

timrobertsdev commented Nov 7, 2022 • edited Loading

nicholasbishop commented Nov 10, 2022

timrobertsdev commented Nov 10, 2022

nicholasbishop commented Nov 11, 2022

timrobertsdev commented Nov 12, 2022 • edited Loading

phip1611 Nov 12, 2022

Choose a reason for hiding this comment

timrobertsdev Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

phip1611 Nov 12, 2022

Choose a reason for hiding this comment

nicholasbishop Nov 12, 2022

Choose a reason for hiding this comment

phip1611 Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

timrobertsdev Nov 12, 2022

Choose a reason for hiding this comment

phip1611 Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

timrobertsdev Nov 12, 2022

Choose a reason for hiding this comment

phip1611 commented Nov 12, 2022

nicholasbishop left a comment

Choose a reason for hiding this comment

Implement additional `BootServices` functions #550

Implement additional `BootServices` functions #550

timrobertsdev commented Nov 6, 2022 •

edited by phip1611

Loading

timrobertsdev commented Nov 7, 2022 •

edited

Loading

timrobertsdev commented Nov 12, 2022 •

edited

Loading

timrobertsdev Nov 12, 2022 •

edited

Loading

phip1611 Nov 12, 2022 •

edited

Loading

phip1611 Nov 12, 2022 •

edited

Loading