RFC: Re-design SystemTable/BootServices/RuntimeServices

[phip1611]

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]

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 (https://github.com/rust-osdev/uefi-rs/issues/563#issuecomment-1484080534) eventually.

[nicholasbishop]

I'm cautiously in favor of this proposal. We did something along these lines in https://github.com/rust-osdev/uefi-rs/pull/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]

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]

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: https://github.com/rust-osdev/uefi-rs/pull/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]

A few more thoughts in favor of adding global pointers:

• The new support in rust-lang for enabling std on the UEFI targets uses global atomic pointers: https://github.com/rust-lang/rust/blob/f654229c27267334023a22233795b88b75fc340e/library/std/src/os/uefi/env.rs#L8
• As seen in https://github.com/rust-osdev/uefi-rs/issues/968#issuecomment-1773883075, we have various APIs that have to pass around an extra BootServices pointer to handle something incidental to the main point of the API (e.g. freeing memory). Would be nice to avoid that.

[nicholasbishop]

General notes on API direction:

1. Add uefi::{system, boot, runtime} modules. These will mostly contain freestanding functions that replace the methods of SystemTable, BootServices, and RuntimeServices. The freestanding functions will use the global system table pointer, so no need for structs. (Akin to std::env).
2. Various structs that currently have a BootServices reference, such as ScopedProtocol and PoolString will no longer need that reference. Ditto for some methods/functions.
3. The user's main function will no longer need to take any arguments, since the global image handle and global system table pointer will be set automatically. This will require changes to the entry macro, or possibly a new macro (uefi_main?) with entry marked deprecated.

Something I'm still unsure about is exactly what the intermediate upgrade path will be. I think ideally we'll just mark the current stuff as deprecated for at least one release, but it may be tricky in some areas to have both APIs present at once. I am working on updating the WIP PR.

[nicholasbishop]

Summarizing recent changes:

1. The published book is now for the latest release instead of main, so breaking changes we make to main won't cause confusion.
2. The entry macro now works with a zero-arg function.
3. The helpers::init function no longer takes an arg. This was one of the functions I was unsure about in my previous comment; it would have been nice to avoid an API-breaking change and instead just do deprecations for a release or two, but I couldn't see any good way to do that here (the alternative would be to have a helpers::init_v2 function that takes no arg and leave helpers::init alone, but that didn't feel like a good long-term change to me).
4. An initial set of functions for the system module has been merged.

Next steps:

1. Fill out the system, boot, and runtime modules, and keep on eye on whether any more API-breaking changes are needed.
2. Mark SystemTable, BootServices, and RuntimeServices as deprecated. For one or two releases users will then be able to upgrade to the new release without immediately rewriting everything to use the new free-standing functions.
3. Eventually, delete the deprecated API.

[nicholasbishop]

Status of the implementation (I'll keep this updated as changes are merged):

System

• [x] as_ptr
• [x] config_table
• [x] firmware_revision
• [x] firmware_vendor
• [x] from_ptr (table::set_system_table)
• [x] stderr
• [x] stdin
• [x] stdout
• [x] uefi_revision

Runtime

• [x] delete_variable
• [x] get_time
• [x] get_time_and_caps
• [x] get_variable
• [x] get_variable_boxed
• [x] get_variable_size (covered by get_variable)
• [x] query_capsule_capabilities
• [x] query_variable_info
• [x] reset
• [x] set_time
• [x] set_variable
• [x] set_virtual_address_map
• [x] update_capsule
• [x] variable_keys

Boot

• [x] allocate_pages
• [x] allocate_pool
• [x] check_event
• [x] close_event
• [x] connect_controller
• [x] create_event
• [x] create_event_ex
• [x] disconnect_controller
• [x] exit
• [x] exit_boot_services
• [x] find_handles
• [x] free_pages
• [x] free_pool
• [x] get_handle_for_protocol
• [x] get_image_file_system
• [x] image_handle
• [x] install_configuration_table
• [x] install_protocol_interface
• [x] load_image
• [x] locate_device_path
• [x] locate_handle
• [x] locate_handle_buffer
• [x] memory_map
• [x] open_protocol
• [x] open_protocol_exclusive
• [x] protocols_per_handle
• [x] raise_tpl
• [x] register_protocol_notify
• [x] reinstall_protocol_interface
• [x] set_image_handle
• [x] set_timer
• [x] set_watchdog_timer
• [x] stall
• [x] start_image
• [x] test_protocol
• [x] uninstall_protocol_interface
• [x] unload_image
• [x] wait_for_event

[phip1611]

Nice, looks good! All the checkboxes are now checked.

[nicholasbishop]

Adding a few more tasks:

• [ ] Update the book to stop referring to SystemTable/BootServices/RuntimeServices
• [x] Update the files under uefi-test-runner/examples to the new style
• [ ] Fix public docstrings that still link to SystemTable/BootServices/RuntimeServices
• [ ] Remove places in the public API that still require SystemTable/BootServices/RuntimeServices references
  • [x] DevicePath <-> text conversion
  • [ ] ComponentName::open
  • [ ] GOP methods
  • [ ] uefi::allocator
  • [ ] TODO: probably some other places
• [ ] Deprecate SystemTable/BootServices/RuntimeServices

[phip1611]

It might be already too late to clean this up, but perhaps, it might be a good idea to make a dedicated release with

• everything that we have right now except for the API deprecation,
• one with only the API deprecation, and
• then another one where we remove all deprecated functionality.

How does that sound to you?

This will make adoption and migration easier.

[nicholasbishop]

I think that's a very good idea. I'll put up a revert for https://github.com/rust-osdev/uefi-rs/pull/1327, if we're not deprecating stuff yet then that change can wait.

[nicholasbishop]

I went ahead and put up the release PR: https://github.com/rust-osdev/uefi-rs/pull/1330, anything else you can think of that we need to do before releasing 0.31?

[phip1611]

I went ahead and put up the release PR: #1330, anything else you can think of that we need to do before releasing 0.31?

Maybe we can get #1326 and #1297 quickly merged for 0.31. Then we can release 0.32 with the deprecation.