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

std: uefi: Add basic Env variables #127462

Merged
merged 2 commits into from
Oct 19, 2024
Merged

std: uefi: Add basic Env variables #127462

merged 2 commits into from
Oct 19, 2024

Conversation

Ayush1325
Copy link
Contributor

@Ayush1325 Ayush1325 commented Jul 7, 2024

  • Implement environment variable functions
  • Using EFI Shell protocol.

@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2024

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 7, 2024
@jhpratt
Copy link
Member

jhpratt commented Jul 8, 2024

Not familiar with UEFI

r? libs

@rustbot rustbot assigned Amanieu and unassigned jhpratt Jul 8, 2024
@tgross35
Copy link
Contributor

tgross35 commented Jul 8, 2024

@joboet is I think, at least they reviewed #123196

@joboet
Copy link
Member

joboet commented Jul 8, 2024

I guess? I worked with UEFI before, so feel free to
r? @joboet
in the future

@rustbot rustbot assigned joboet and unassigned Amanieu Jul 8, 2024
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember: was there a decision to always use the underlying UEFI calls instead of UEFI-shell calls? The current implementation will interact nicely with the EDK2 shell implementation, but won't work the same way with other UEFI implementations. But we don't have to decide here.

Could you also implement Env using GetNextVariableName? I think we should support that as well.

@Ayush1325
Copy link
Contributor Author

Ayush1325 commented Jul 8, 2024

I don't remember: was there a decision to always use the underlying UEFI calls instead of UEFI-shell calls? The current implementation will interact nicely with the EDK2 shell implementation, but won't work the same way with other UEFI implementations. But we don't have to decide here.

So, the reason for not using shell protocol till now was that it will not be present most of the time. So as long as we do not need to depend on it, we should not.

With that said, as long as we can provide a fallback to default UEFI variables, I think it is fine to depend on the Shell Protocol for environment variables.

Could you also implement Env using GetNextVariableName? I think we should support that as well.

Depending on if we use shell protocol or not, the implementation will look quite different. So will add that once the implementation is decided.

@joboet
Copy link
Member

joboet commented Aug 12, 2024

Sorry about leaving you hanging, life got in the way.

@dvdhrm and @nicholasbishop, what are your thoughts on this? Should the UEFI target require UEFI-shell, use it if present or not use it at all? I think this should be decided for the target as a whole once and for all as the implementation of so many other features (process spawning, arguments, ...) depend on this decision.

@nicholasbishop
Copy link
Contributor

I think it really depends on what the intended use case of uefi-std support is. Here are two possibilities:

  1. uefi-std is implemented to allow the full rust test suite to run, with the primary goal of catching bugs in the uefi targets.
  2. uefi-std is intended to become the normal way you write a UEFI application in Rust, the std support is for end users.

If the goal is 1, then I think targeting the uefi shell sounds good. If the goal is 2, then I don't think it would be very useful to target the uefi shell. The typical UEFI application is a bootloader run directly at boot (or loaded by a previous-stage bootloader), and no shell will be run in that case.

@Ayush1325
Copy link
Contributor Author

I think it really depends on what the intended use case of uefi-std support is. Here are two possibilities:

  1. uefi-std is implemented to allow the full rust test suite to run, with the primary goal of catching bugs in the uefi targets.
  2. uefi-std is intended to become the normal way you write a UEFI application in Rust, the std support is for end users.

If the goal is 1, then I think targeting the uefi shell sounds good. If the goal is 2, then I don't think it would be very useful to target the uefi shell. The typical UEFI application is a bootloader run directly at boot (or loaded by a previous-stage bootloader), and no shell will be run in that case.

I would like to target 2 while enabling 1. I would like to allow using uefi-std to write general uefi applications (like system test software, maybe uefi shell itself) in Rust. And well, to make 2 viable, we kind of need to be able to run tests, at least on the CI at some point in the future.

So, the current philosophy I have is to implement everything in a way that can be used as widely as possible. That is why as long as something is possible with the base UEFI protocol, I would like to stick with it.

Do note that I am not really sure if there are any companies or any other projects that have an interest in uefi-std (or if they even know of it's existence). I am just doing it in my free time because I did the work to allow running Rust test suit during GSoC 2022 and well, I might as well upstream everything. My time to work on this is quite limited nowadays, but I still have plans to at least upstream File IO, TCP, UDP, and pass args to Command.

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to do a bit of soul-searching, sorry for that taking so long. I'm inclined to accept this with the current strategy, given
that

  • UEFI support is still experimental so we won't be locked in by this
  • The implementation is the only reasonable one when UEFI-Shell is unavailable and
  • EDK2 publicly documents its GUID, so we can safely interface with it

Please do however implement Env using GetNextVariableName as well, so that we fully support environment variables.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2024
@Ayush1325 Ayush1325 force-pushed the uefi-env branch 2 times, most recently from 19afb72 to f114cd9 Compare August 28, 2024 20:28
@Ayush1325
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2024
@nicholasbishop
Copy link
Contributor

I would like to target 2 while enabling 1. I would like to allow using uefi-std to write general uefi applications (like system test software, maybe uefi shell itself) in Rust. And well, to make 2 viable, we kind of need to be able to run tests, at least on the CI at some point in the future.

So, the current philosophy I have is to implement everything in a way that can be used as widely as possible. That is why as long as something is possible with the base UEFI protocol, I would like to stick with it.

Do note that I am not really sure if there are any companies or any other projects that have an interest in uefi-std (or if they even know of it's existence). I am just doing it in my free time because I did the work to allow running Rust test suit during GSoC 2022 and well, I might as well upstream everything. My time to work on this is quite limited nowadays, but I still have plans to at least upstream File IO, TCP, UDP, and pass args to Command.

Thanks for the details, that all makes sense. I really appreciate the work you're doing on this!

@Ayush1325
Copy link
Contributor Author

Ayush1325 commented Aug 30, 2024

So, I also did a lot of mulling things over after working on fs implementation and I have started warming upto the inclusion of UEFI Shell Protocol for some things. I am planning to use it in the future for the implementation of getcwd and cwd since I do not think either make sense when shell is not present.

So, the major questions we need to ask are:

  1. Does it make sense to have Environment variables supported when shell is missing?
  2. Is it fine to panic in case of a missing shell since set_var will panic if it fails?

cc @nicholasbishop @dvdhrm @joboet

@Ayush1325 Ayush1325 force-pushed the uefi-env branch 2 times, most recently from f9c9810 to 1579dd3 Compare October 18, 2024 17:05
- Implement environment variable functions
- Using EFI Shell protocol.

Signed-off-by: Ayush Singh <[email protected]>
- Since in almost all cases, there will only be 1 UEFI shell, share the
  shell handle between all functions that require it.

Signed-off-by: Ayush Singh <[email protected]>
@Ayush1325
Copy link
Contributor Author

@bors r=@joboet

@bors
Copy link
Contributor

bors commented Oct 18, 2024

📌 Commit 753536a has been approved by joboet

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2024
@bors bors merged commit 5a3ecd5 into rust-lang:master Oct 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 19, 2024
@Ayush1325 Ayush1325 deleted the uefi-env branch October 20, 2024 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants