Skip to content
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

RFC: Re-design SystemTable/BootServices/RuntimeServices #893

Open
phip1611 opened this issue Jul 12, 2023 · 5 comments
Open

RFC: Re-design SystemTable/BootServices/RuntimeServices #893

phip1611 opened this issue Jul 12, 2023 · 5 comments

Comments

@phip1611
Copy link
Contributor

phip1611 commented Jul 12, 2023

Let's discuss #841 again which was unfortunately closed without further discussions.

Personally, I am in strong favor of the proposed API design. I copy the key points here again:

The application's entry-point is changed to a simple fn main() -> Result signature. The #[entry] (which I would prefer renamed to efi_main) provides the real entry-point internally, stores the SystemTable and image handle somewhere, and calls into the marked main function. On return, it does the right thing(tm) with the Result (print out a message and return status code).

All functions in BootServices/RuntimeServices are changed to associated functions that access the global table pointer internally. As they don't take a this pointer, they are just global functions as is basically done in EDK2 (well, they provide a global pointer to the table struct, same difference). It's more ergonomic and will eliminate some annoying lifetime issues. It also obviates the need for some structs like ScopedProtocol to keep a ref to the table.

To protect against use after exiting boot services, the BootServices table pointer can be replaced by the exit handler with a None or replaced with a custom implementation that panics if actually used.

Access to stdin/stdout/stderr should be provided via associated functions from the Input/Output types (and panic after boot services exit).

An associated function would be provided to get direct access to the tables if needed.

This would also remove the need for a View on SystemTable: any members that go away should be Options and set to None once boot services are exited by the exit handler. (In fact, the UEFI spec recommends doing so.)

I'm especially in strong favor of renaming #[entry] to #[efi_main], simplify the function signature and not having to manually pass bs to so many functions. Instead, it should be provided by a global static.

@phip1611 phip1611 pinned this issue Jul 12, 2023
@phip1611
Copy link
Contributor Author

phip1611 commented Jul 12, 2023

This would also make the uefi-services crate (in its current form) deprecated as we'd already have a static global of boot services in the uefi crate. I'd be in favor of deprecating uefi-services as well (#563 (comment)) eventually.

@nicholasbishop
Copy link
Contributor

I'm cautiously in favor of this proposal. We did something along these lines in #478, where we added a global image handle that gets set automatically by the entry macro.

The only downsides I can think of so far are:

  1. We would lose the compile-time transition from boot services to runtime services, where the user can statically assure that only runtime-services calls are allowed. Instead of compile-time errors when calling boot services after the transition, we'll return an error or panic as appropriate. I think that's probably an OK tradeoff, but worth thinking about carefully to make sure we're not missing any cases where it would make a big difference in usability.
  2. It's a pretty big change in the public API. I'm thinking we can probably mostly keep the old API around temporarily with #[deprecated] attributes to help ease the transition for one or two releases?

@nicholasbishop
Copy link
Contributor

Expanding on downside 1 from my list above with a related problem:

Structs that currently have a lifetime tied to BootServices (like ScopedProtocol and various others) presumably wouldn't have that lifetime in the new design. But that means that you could hold on to such an object after exiting boot services, which seems unfortunate. I guess we could have all accesses to such objects internally check for null bootservices on each access or something like that.

@nicholasbishop
Copy link
Contributor

nicholasbishop commented Jul 29, 2023

I've been playing around with this a bit, to get an idea of what it would look like in practice. I put up what I have for discussion: #905

A few notes:

  • The original proposal had "All functions in BootServices/RuntimeServices are changed to associated functions". In this implementation I went with free-standing functions. This has a couple advantages:
    1. The existing BootServices/RuntimeServices/SystemTable API can remain unchanged for a bit, so we could mark it deprecated but keep it available.
    2. It's a bit shorter to call, e.g. boot::load_image or even just load_image instead of BootServices::load_image.
  • I put the new boot/runtime/system modules at the top level instead of nested under table, again just to shorten the usage a bit. Not something I feel super strongly about though.
  • In a few places I've tried to avoid lifetime issues by adding with methods to get access to things temporarily, rather than returning a reference. For example, system::with_config_table(). That does avoid having to return a 'static lifetime, but I'm not sure it's sufficient; you could still call the boot services function that mutates the config table while that callback is running. In fact, I think this is already a problem with our current API.
  • exit_boot_services becomes unsafe. There are so many potential ways to violate safety by holding on to allocations, handles, protocols, etc... seems very hard to make it safe, and I think it would probably just make other parts of the API worse if we tried to do so.

@nicholasbishop
Copy link
Contributor

A few more thoughts in favor of adding global pointers:

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

No branches or pull requests

2 participants