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

Add UEFI APIs in std::os::uefi #87

Closed
Ayush1325 opened this issue Aug 18, 2022 · 11 comments
Closed

Add UEFI APIs in std::os::uefi #87

Ayush1325 opened this issue Aug 18, 2022 · 11 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@Ayush1325
Copy link

Ayush1325 commented Aug 18, 2022

Proposal

Problem statement

Hello everyone. I have been working on adding Rust std support for UEFI as a part of Google Summer of Code 2022.

This is a proposal to add some UEFI-specific APIs under std::os::uefi. It is related to PR.

Motivation, use-cases

In the UEFI environment, most of the services/functionality is accessed using System Table, which is passed to the application at its entry. These APIs basically add ways to access the System Table and some of its most common members.

APIs to add

APIs I propose to add under std::os::uefi::env:

  1. pub fn get_system_table() -> NonNull<c_void>
  2. pub fn get_system_handle() -> NonNull<c_void>
  3. pub fn init_globals(handle: NonNull<c_void>, system_table: NonNull<c_void>)

APIs I propose to add under std::os::uefi::ffi (Just re-export std::os::windows::ffi)

  1. trait OsStrExt
  2. trait OsStringExt

Links and related work

  1. PR
  2. My Blog
  3. Tracking Issue
@Ayush1325 Ayush1325 added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Aug 18, 2022
@thomcc
Copy link
Member

thomcc commented Aug 18, 2022

Can you elaborate what SystemTable, BootServices, and RuntimeServices are? These seem to be from the r-efi crate, which is a bit of a problem.

Concretely, I don't think we want to expose public types from another crate without first wrapping them -- even something with an essentially identical API as the underlying implementation are wrapped; for example std::collections::HashMap is implemented as a wrapper around hashbrown::HashMap, and types from libc in std::os::*::raw are re-declared directly (this largely is something we don't want to repeat -- note that this stuff is deprectated).

If they're wrapped or reimplemented, this ACP needs to cover their whole API, and defend their inclusion. Since they're just from the r-efi crate, is there a reason to provide them from std? Could we instead omit them and have people use the r-efi crate off of crates.io instead? This seems closer to what we do for other OSes.

(Note that I still haven't reviewed the implementation in any depth)

@Ayush1325
Copy link
Author

cc @dvdhrm
SystemTable, BootServices and RuntimeServices are structures defined in the UEFI Specification. These structures contain pointers to methods and other things which make a UEFI application/driver function. Pointers to these structures are passed to the application at the entry point.

The reason to provide these methods in std is that when using Rust std (where main has a default signature), there needs to be a way for application developers to get access to the SystemTable and SystemHandle that were passed to the application. So at the very least, the function 1 and 2 need to present in std (3 and 4 are just nice to have)

r-efi is a specification crate, so it only contains structures and function signatures, not any implementation. Also, since the SystemTable and SystemHandle are supplied at the image entry point (which is in std) and stored in std, they have to be acquired from std.

@dvdhrm
Copy link

dvdhrm commented Aug 19, 2022

Exposing the r-efi types basically pulls in the entire r-efi crate into the public API. I agree that we should avoid this if possible. I have two alternatives (which can actually be combined as well):

  1. Make the global functions you proposed return c_void rather than the typed structure-references. This will strip type-safety, but is a very easy way out at minimal cost, IMO.

  2. Allow (or even require?) users to define their UEFI entry-point themselves, rather than doing this in std. This will give them access to the system-table and image handle. You can then have a setup_std(), or register_system_table(), etc., which takes the system-table from the user and keeps it for std to retain.

As I see it, the biggest issue is that UEFI does not allow access to the system-table except through your entry-point. This capability-based model conflicts with the global state expected by rust-std. Most of the rust standard-library just does not allow for a user-supplied context object (Maybe it does? I haven't found a nice way.).

I think we also need to consider what happens to rust code linked into a UEFI C application. In this case the entry-point is provided by the C code, and the rust library/helper is just linked into the application. With your proposed solution, you would be unable to use std on UEFI from within that rust code, since you do not control the entry-point. By not defining the runtime in std, but instead accept a user-supplied context (as suggested in 2.), you allow for such use-cases as well.

On a closing note, with ExitBootServices() in mind, all this can become quite fragile. But I think it would be safe to argue that rust-std on UEFI requires you not to exit boot-services.

@Ayush1325
Copy link
Author

1. Make the global functions you proposed return c_void rather than the typed structure-references. This will strip type-safety, but is a very easy way out at minimal cost, IMO.

That should work fine. Incidentally, it might be possible just make it return NonNull<T>, and leave the T to the user.

2. Allow (or even require?) users to define their UEFI entry-point themselves, rather than doing this in std. This will give them access to the system-table and image handle. You can then have a `setup_std()`, or `register_system_table()`, etc., which takes the system-table from the user and keeps it for `std` to retain.

This is something I would like to allow as well. Basically, not generate the custom entry point if no_main is used. However, I haven't found a way to do that yet. Requiring a custom entry point would break all the tests, so not sure if that is the best approach.

Incidentally, I already have a function to init the global SystemTable and SystemHandle in std::os::uefi::env (currently private), which makes it pretty trivial to allow user-generated entry point with full std support.

@joshtriplett
Copy link
Member

This seems fine to experiment with as an unstable API. (We're not sure if this is the API we want to stabilize, but for nightly it seems fine to experiment.)

I personally would prefer to see these become wrapper newtypes, to make it harder to accidentally pass BootServices to something expecting SystemTable or vice versa. They don't have to be full structure definitions, just an opaque wrapper around NonNull<c_void> with a getter and perhaps an unsafe constructor.

For init_globals, will that be called automatically by std's initialization code?

@Ayush1325
Copy link
Author

I think it would be a good idea to have the wrapper type. I am also thinking if I should convert all access to these types to use read_volatile.

@programmerjake
Copy link
Member

read_volatile is almost never what you want to use -- it is not for when other software might modify values in parallel (different thread or through interrupts) to your code, you want atomics for that. volatile is almost exclusively used when accessing a memory-mapped device which doesn't act like normal memory.

if other code can only modify that memory on the same thread as your code, then either use &Cell<T> or use raw pointers and ptr::read/ptr::write or direct field access to Copy fields of a raw pointer (do be careful to not call methods that implicitly create a mutable reference to those fields -- that would be UB since other code still holds independent pointers that they presumably can/will write through later). if you can guarantee no uefi methods are run in some scope, shared references are fine as long as their lifetime expired before calling uefi methods that modify that state.

@Ayush1325
Copy link
Author

Well, the reason I was suggesting volatile is to avoid compiler caching. In C land, that would mean making the pointer volatile (I think?).

@pitaj
Copy link

pitaj commented May 19, 2023

Why do you need to avoid compiler caching?

@Ayush1325
Copy link
Author

Well, it is possible for the SystemTable to be accessed (and sometimes be modified like the console handle pointers) by interrupts. So it is best to ensure that the compiler reloads the value from memory each time it is accessed by the program. I am not sure how much of a problem this actually is, but I remember hearing that I should use volatile somewhere.

I am currently using thread locals since UEFI is single-threaded.

@pitaj
Copy link

pitaj commented May 19, 2023

Ah okay. In that case, you should probably use a single-threaded CriticalSection Mutex like https://docs.rs/cortex-m/latest/cortex_m/interrupt/struct.Mutex.html

Ayush1325 added a commit to Ayush1325/rust that referenced this issue Sep 4, 2023
Implemented modules:
1. alloc
2. os_str
3. env
4. math

Tracking Issue: rust-lang#100499
API Change Proposal: rust-lang/libs-team#87

This was originally part of rust-lang#100316. Since
that PR was becoming too unwieldy and cluttered, and with suggestion
from @dvdhrm, I have extracted a minimal std implementation to this PR.

Signed-off-by: Ayush Singh <[email protected]>
Ayush1325 added a commit to Ayush1325/rust that referenced this issue Sep 21, 2023
Implemented modules:
1. alloc
2. os_str
3. env
4. math

Tracking Issue: rust-lang#100499
API Change Proposal: rust-lang/libs-team#87

This was originally part of rust-lang#100316. Since
that PR was becoming too unwieldy and cluttered, and with suggestion
from @dvdhrm, I have extracted a minimal std implementation to this PR.

Signed-off-by: Ayush Singh <[email protected]>
Ayush1325 added a commit to Ayush1325/rust that referenced this issue Sep 22, 2023
Implemented modules:
1. alloc
2. os_str
3. env
4. math

Tracking Issue: rust-lang#100499
API Change Proposal: rust-lang/libs-team#87

This was originally part of rust-lang#100316. Since
that PR was becoming too unwieldy and cluttered, and with suggestion
from @dvdhrm, I have extracted a minimal std implementation to this PR.

Signed-off-by: Ayush Singh <[email protected]>
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 24, 2023
…ngjubilee

Add Minimal Std implementation for UEFI

# Implemented modules:
1. alloc
2. os_str
3. env
4. math

# Related Links
Tracking Issue: rust-lang#100499
API Change Proposal: rust-lang/libs-team#87

# Additional Information
This was originally part of rust-lang#100316. Since that PR was becoming too unwieldy and cluttered, and with suggestion from `@dvdhrm,` I have extracted a minimal std implementation to this PR.

The example in `src/doc/rustc/src/platform-support/unknown-uefi.md` has been tested for `x86_64-unknown-uefi` and `i686-unknown-uefi` in OVMF. It would be great if someone more familiar with AARCH64 can help with testing for that target.

Signed-off-by: Ayush Singh <[email protected]>
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 27, 2023
Add Minimal Std implementation for UEFI

# Implemented modules:
1. alloc
2. os_str
3. env
4. math

# Related Links
Tracking Issue: rust-lang/rust#100499
API Change Proposal: rust-lang/libs-team#87

# Additional Information
This was originally part of rust-lang/rust#100316. Since that PR was becoming too unwieldy and cluttered, and with suggestion from `@dvdhrm,` I have extracted a minimal std implementation to this PR.

The example in `src/doc/rustc/src/platform-support/unknown-uefi.md` has been tested for `x86_64-unknown-uefi` and `i686-unknown-uefi` in OVMF. It would be great if someone more familiar with AARCH64 can help with testing for that target.

Signed-off-by: Ayush Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants