Stdio support for UEFI #116207

Ayush1325 · 2023-09-27T19:50:45Z

Uses Simple Text Output Protocol and Simple Text Input Protocol
Reading is done one character at a time
Writing is done with max 4096 characters

Quirks

Output Newline

UEFI uses CRLF for newline. So when running the application in UEFI shell (qemu VGA), the output of println looks weird.
However, since the UEFI shell supports piping output, I am unsure if doing any output post-processing is a good idea. UEFI shell cat command seems to work fine with just LF.

Input Newline

Stdin.read_line() method is broken in UEFI shell. Pressing enter seems to be read as CR, which means LF is never encountered.
Works fine with input redirection from file.

CC @dvdhrm

rustbot · 2023-09-27T19:50:52Z

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2023-09-30T17:51:03Z

cc @nicholasbishop as well as the other target maintainer

- Mention stdio support - Update the example Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>

- Uses Simple Text Output Protocol and Simple Text Input Protocol - Reading is done one character at a time - Writing is done with max 4096 characters Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>

Mark-Simulacrum · 2023-10-01T22:37:08Z

Code looks reasonable to me, though I haven't checked the UEFI ~FFI calls. I'll go ahead and approve as well.

@bors r+

bors · 2023-10-01T22:37:11Z

📌 Commit 3f4a289 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2023-10-02T00:03:54Z

⌛ Testing commit 3f4a289 with merge 79bfd93...

bors · 2023-10-02T02:04:38Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 79bfd93 to master...

rust-timer · 2023-10-02T04:03:55Z

Finished benchmarking commit (79bfd93): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.0%	[1.1%, 2.5%]	16
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[1.1%, 2.5%]	16

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 629.592s -> 626.835s (-0.44%)
Artifact size: 273.34 MiB -> 273.31 MiB (-0.01%)

dvdhrm · 2023-10-06T09:35:08Z

library/std/src/sys/uefi/stdio.rs

+use crate::os::uefi;
+use crate::ptr::NonNull;
+
+const MAX_BUFFER_SIZE: usize = 8192;


Why is this sized based on 2 pages, rather than the more common single page size?

Well, that size was picked up from Windows implementation. I am open to a more appropriate size.

The windows backend picked 8k due to behavior of the windows system libraries (also see f411852). I don't think any of this reasoning applies to UEFI.

dvdhrm · 2023-10-06T09:43:42Z

library/std/src/sys/uefi/stdio.rs

+}
+
+pub fn panic_output() -> Option<impl io::Write> {
+    uefi::env::try_system_table().map(|_| Stderr::new())


Why checking for the system-table here? It can be invalidated in a parallel call.

I am not sure what you mean. My intention was to check if the code panics before the system table is initialized in Rust (which can happen) or after it. It is not really meant to check if system table is valid for complete UEFI.

But why do you check for it? What is the rationale?

To avoid panic inside panic.

dvdhrm · 2023-10-06T09:44:13Z

library/std/src/sys/uefi/stdio.rs

+pub const STDIN_BUF_SIZE: usize = 3;
+
+pub fn is_ebadf(_err: &io::Error) -> bool {
+    true


What is the reasoning here? You effectively fold every error to the default error by treating everything as EBADF, or what am I missing?

That's just the default unsupported implementation. I am not sure what EBADF corresponds to in UEFI. Feel free to comment about it.

is_ebadf() is used to suppress EBADF errors, and avoid panicking on println!() if the output-stream does not exist. Your current implementation for UEFI causes ALL errors for output-streams to be suppressed, because you unconditionally return true.

There is no equivalent to EBADF on UEFI, since you operate on devices directly, rather than on virtualized FDs. However, you likely want to suppress EFI_UNSUPPORTED, since this is triggered when text-mode is currently disabled. But is_ebadf() seems like the wrong place to do this.

dvdhrm · 2023-10-06T09:45:17Z

library/std/src/sys/uefi/stdio.rs

+    protocol: *mut r_efi::protocols::simple_text_output::Protocol,
+    buf: &[u8],
+) -> io::Result<usize> {
+    let mut utf16 = [0; MAX_BUFFER_SIZE / 2];


Maybe use an explicit type-annotation here? You rely on the size of the type by hard-coding 2, yet no explicit type is used.

Yeah, I guess it can be changed to size_of::<u16>(). I started with the windows implementation so it is from that.

I am not talking about the sizeof_of, I am talking about the type-annotation of utf16 (let mut utf16: u16 = ...).

dvdhrm · 2023-10-06T09:53:45Z

library/std/src/sys/uefi/stdio.rs

+        Err(e) => unsafe { crate::str::from_utf8_unchecked(&buf[..e.valid_up_to()]) },
+    };
+    // Clip UTF-8 buffer to max UTF-16 buffer we support
+    let utf8 = &utf8[..utf8.floor_char_boundary(utf16.len() - 1)];


The - 1 should be dropped. floor_char_boundary() will correctly use the entire string if the index is beyond the last character.

So the -1 is so that the string is NULL terminated.

Fair enough. Though, I think this is really not obvious from this code. And simple_text_output() takes a slice but silently expects it to be NULL-terminated. I think this is very likely to lead to problems. I would prefer at least an assertion in simple_text_output() to catch such errors in the future.

dvdhrm · 2023-10-06T09:58:00Z

library/std/src/sys/uefi/stdio.rs

+
+impl io::Write for Stderr {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        let st: NonNull<r_efi::efi::SystemTable> = uefi::env::system_table().cast();


Why not error out if the system-table is not available?

So the current implementation has a default behaviour to panic if system table (and brothers) is not available. The exceptions are when the function might be called before sys::init.

Yes, I know. I want to understand what you want the behavior to be when it is called before sys::init? You check for validity in the panic-path, but not here, which seems inconsistent. I am simply trying to understand what you want it to behave like?

So the basic assumptions is that SystemTable and ImageHandle are always valid except in panic-path. Thus, anything that uses Stderr outside of panic-path should panic if it is unavailable.

dvdhrm · 2023-10-06T10:05:00Z

library/std/src/sys/uefi/stdio.rs

+        // Check if the key is printiable character
+        if inp.scan_code != 0x00 {
+            return Err(io::const_io_error!(io::ErrorKind::Interrupted, "Special Key Press"));
+        }


Can you explain why you strip zeros?

It's not stripping zeros, scan_code not 0 indicates a non-printable character. See Section 12.3.3 in UEFI Spec

unicode_char == 0x00 represents control characters. The scan-code is always set, yet you check the scan-code rather than unicode_char.

If I am not mistaken, your current code suppresses all input.

The ReadKeyStroke() function reads the next keystroke from the input device. If there is no pending keystroke the function returns EFI_NOT_READY. If there is a pending keystroke, then ScanCode is the EFI scan code defined in EFI Scan Codes for EFI_SIMPLE_TEXT_INPUT_PROTOCOL . The UnicodeChar is the actual printable character or is zero if the key does not represent a printable character (control key, function key, etc.)

dvdhrm · 2023-10-06T10:09:48Z

library/std/src/sys/uefi/stdio.rs

+        }
+
+        // SAFETY: Iterator will have only 1 character since we are reading only 1 Key
+        // SAFETY: This character will always be UCS-2 and thus no surrogates.


I think it is overly optimistic to rely on firmware never returning invalid data. I would prefer to forward errors, but I will not insist.

Since we are reading a printable key press (due to the above check), I would hope it would be valid. But since it is possible to do file piping for input, I guess it might be good to return an error.

dvdhrm · 2023-10-06T10:13:16Z

library/std/src/sys/uefi/stdio.rs

+        // SAFETY: This character will always be UCS-2 and thus no surrogates.
+        let ch: char = char::decode_utf16([inp.unicode_char]).next().unwrap().unwrap();
+        if ch.len_utf8() > buf.len() {
+            return Ok(0);


You drop data if the buffer is short. This is really unfortunate. I think the better alternative would be to have an internal buffer in struct Stdin where you keep the last char and keep trying to return it to the caller until they provide a suitable buffer. This does busy-loop if the caller never provides a suitable buffer, but at least you do not drop data silently.

Alternatively, return exactly the requested amount of bytes, and return the remaining ones in following calls. There is no reason why the caller would expect every read-call to split exactly at UTF8-boundaries, or is there?

Yes, it seems like a reasonable strategy. I think an internal buffer of a single character (u16) should be enough.

dvdhrm · 2023-10-06T10:14:16Z

library/std/src/sys/uefi/stdio.rs

+            Err(e) if e == r_efi::efi::Status::NOT_READY => {
+                // Wait for keypress for new data
+                wait_stdin(stdin)?;
+                read_key_stroke(stdin).map_err(|x| io::Error::from_raw_os_error(x.as_usize()))?


Any particular reason you do not loop on NOT_READY? Can we rely on wait_stdin to never report spurious wakeups?

Well, I am not sure busy waiting would be the best idea. In all the examples of reading I have seen online, everyone seems to rely on the con_in->wait_for_key event.
The spec also states the following: "Event to use with EFI_BOOT_SERVICES.WaitForEvent() to wait for a key to be available." So it should be the best solution.

I am not suggesting busy-waiting. I am saying to loop on NOT_READY. So after wait_stdin(), you still check for NOT_READY and wait again if it keeps getting reported.

…ingjubilee Improve UEFI stdio Fixed some things suggested in last PR: rust-lang#116207 cc `@dvdhrm` cc `@nicholasbishop`

rustbot assigned Mark-Simulacrum Sep 27, 2023

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 Sep 27, 2023

Ayush1325 added 2 commits October 2, 2023 00:06

Update UEFI docs

b0a7217

- Mention stdio support - Update the example Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>

Stdio for UEFI

3f4a289

- Uses Simple Text Output Protocol and Simple Text Input Protocol - Reading is done one character at a time - Writing is done with max 4096 characters Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>

Ayush1325 force-pushed the uefi_stdio branch from 217ad0c to 3f4a289 Compare October 1, 2023 18:37

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 1, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 2, 2023

bors merged commit 79bfd93 into rust-lang:master Oct 2, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 2, 2023

Ayush1325 deleted the uefi_stdio branch October 2, 2023 06:20

pvdrz mentioned this pull request Oct 4, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

dvdhrm reviewed Oct 6, 2023

View reviewed changes

Ayush1325 mentioned this pull request Oct 25, 2023

Improve UEFI stdio #117174

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2024

Auto merge of rust-lang#117174 - Ayush1325:uefi-stdio-improve, r=work…

026b3b8

…ingjubilee Improve UEFI stdio Fixed some things suggested in last PR: rust-lang#116207 cc `@dvdhrm` cc `@nicholasbishop`

Stdio support for UEFI #116207

Stdio support for UEFI #116207

Conversation

Ayush1325 commented Sep 27, 2023 • edited

Quirks

Output Newline

Input Newline

rustbot commented Sep 27, 2023

Mark-Simulacrum commented Sep 30, 2023

Mark-Simulacrum commented Oct 1, 2023

bors commented Oct 1, 2023

bors commented Oct 2, 2023

bors commented Oct 2, 2023

rust-timer commented Oct 2, 2023

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ayush1325 commented Sep 27, 2023 •

edited