-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Stdio support for UEFI #116207
Stdio support for UEFI #116207
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,162 @@ | ||
use crate::io; | ||
use crate::iter::Iterator; | ||
use crate::mem::MaybeUninit; | ||
use crate::os::uefi; | ||
use crate::ptr::NonNull; | ||
|
||
const MAX_BUFFER_SIZE: usize = 8192; | ||
|
||
pub struct Stdin; | ||
pub struct Stdout; | ||
pub struct Stderr; | ||
|
||
impl Stdin { | ||
pub const fn new() -> Stdin { | ||
Stdin | ||
} | ||
} | ||
|
||
impl io::Read for Stdin { | ||
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { | ||
let st: NonNull<r_efi::efi::SystemTable> = uefi::env::system_table().cast(); | ||
let stdin = unsafe { (*st.as_ptr()).con_in }; | ||
|
||
// Try reading any pending data | ||
let inp = match read_key_stroke(stdin) { | ||
Ok(x) => x, | ||
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()))? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason you do not loop on NOT_READY? Can we rely on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not suggesting busy-waiting. I am saying to loop on |
||
} | ||
Err(e) => { | ||
return Err(io::Error::from_raw_os_error(e.as_usize())); | ||
} | ||
}; | ||
|
||
// Check if the key is printiable character | ||
if inp.scan_code != 0x00 { | ||
return Err(io::const_io_error!(io::ErrorKind::Interrupted, "Special Key Press")); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why you strip zeros? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not stripping zeros, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If I am not mistaken, your current code suppresses all input.
|
||
|
||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
let ch: char = char::decode_utf16([inp.unicode_char]).next().unwrap().unwrap(); | ||
if ch.len_utf8() > buf.len() { | ||
return Ok(0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it seems like a reasonable strategy. I think an internal buffer of a single character (u16) should be enough. |
||
} | ||
|
||
ch.encode_utf8(buf); | ||
|
||
Ok(ch.len_utf8()) | ||
} | ||
} | ||
|
||
impl Stdout { | ||
pub const fn new() -> Stdout { | ||
Stdout | ||
} | ||
} | ||
|
||
impl io::Write for Stdout { | ||
fn write(&mut self, buf: &[u8]) -> io::Result<usize> { | ||
let st: NonNull<r_efi::efi::SystemTable> = uefi::env::system_table().cast(); | ||
let stdout = unsafe { (*st.as_ptr()).con_out }; | ||
|
||
write(stdout, buf) | ||
} | ||
|
||
fn flush(&mut self) -> io::Result<()> { | ||
Ok(()) | ||
} | ||
} | ||
|
||
impl Stderr { | ||
pub const fn new() -> Stderr { | ||
Stderr | ||
} | ||
} | ||
|
||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not error out if the system-table is not available? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I know. I want to understand what you want the behavior to be when it is called before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
let stderr = unsafe { (*st.as_ptr()).std_err }; | ||
|
||
write(stderr, buf) | ||
} | ||
|
||
fn flush(&mut self) -> io::Result<()> { | ||
Ok(()) | ||
} | ||
} | ||
|
||
// UCS-2 character should occupy 3 bytes at most in UTF-8 | ||
pub const STDIN_BUF_SIZE: usize = 3; | ||
|
||
pub fn is_ebadf(_err: &io::Error) -> bool { | ||
true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reasoning here? You effectively fold every error to the default error by treating everything as EBADF, or what am I missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's just the default unsupported implementation. I am not sure what EBADF corresponds to in UEFI. Feel free to comment about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There is no equivalent to |
||
} | ||
|
||
pub fn panic_output() -> Option<impl io::Write> { | ||
uefi::env::try_system_table().map(|_| Stderr::new()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why checking for the system-table here? It can be invalidated in a parallel call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But why do you check for it? What is the rationale? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid panic inside panic. |
||
} | ||
|
||
fn write( | ||
protocol: *mut r_efi::protocols::simple_text_output::Protocol, | ||
buf: &[u8], | ||
) -> io::Result<usize> { | ||
let mut utf16 = [0; MAX_BUFFER_SIZE / 2]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe use an explicit type-annotation here? You rely on the size of the type by hard-coding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I guess it can be changed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not talking about the sizeof_of, I am talking about the type-annotation of |
||
|
||
// Get valid UTF-8 buffer | ||
let utf8 = match crate::str::from_utf8(buf) { | ||
Ok(x) => x, | ||
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)]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the -1 is so that the string is NULL terminated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. Though, I think this is really not obvious from this code. And |
||
|
||
for (i, ch) in utf8.encode_utf16().enumerate() { | ||
utf16[i] = ch; | ||
} | ||
Comment on lines
+118
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree about reducing the stack pressure but I think it would still be better to use stack than heap. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to explain why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I am not sure about heap size limitations in UEFI. However, I would imagine they are much less than normal OS. Also, the UEFI heap is rather simple compared to the os heap. A Rust application can end up writing a lot of data to stdio at once if there are no limits. I do not mind using the heap itself, but I would prefer specifying the max size of the buffer, even on the heap. |
||
|
||
unsafe { simple_text_output(protocol, &mut utf16) }?; | ||
|
||
Ok(utf8.len()) | ||
} | ||
|
||
unsafe fn simple_text_output( | ||
protocol: *mut r_efi::protocols::simple_text_output::Protocol, | ||
buf: &mut [u16], | ||
) -> io::Result<()> { | ||
let res = unsafe { ((*protocol).output_string)(protocol, buf.as_mut_ptr()) }; | ||
if res.is_error() { Err(io::Error::from_raw_os_error(res.as_usize())) } else { Ok(()) } | ||
} | ||
|
||
fn wait_stdin(stdin: *mut r_efi::protocols::simple_text_input::Protocol) -> io::Result<()> { | ||
let boot_services: NonNull<r_efi::efi::BootServices> = | ||
uefi::env::boot_services().unwrap().cast(); | ||
let wait_for_event = unsafe { (*boot_services.as_ptr()).wait_for_event }; | ||
let wait_for_key_event = unsafe { (*stdin).wait_for_key }; | ||
|
||
let r = { | ||
let mut x: usize = 0; | ||
(wait_for_event)(1, [wait_for_key_event].as_mut_ptr(), &mut x) | ||
}; | ||
if r.is_error() { Err(io::Error::from_raw_os_error(r.as_usize())) } else { Ok(()) } | ||
} | ||
|
||
fn read_key_stroke( | ||
stdin: *mut r_efi::protocols::simple_text_input::Protocol, | ||
) -> Result<r_efi::protocols::simple_text_input::InputKey, r_efi::efi::Status> { | ||
let mut input_key: MaybeUninit<r_efi::protocols::simple_text_input::InputKey> = | ||
MaybeUninit::uninit(); | ||
|
||
let r = unsafe { ((*stdin).read_key_stroke)(stdin, input_key.as_mut_ptr()) }; | ||
|
||
if r.is_error() { | ||
Err(r) | ||
} else { | ||
let input_key = unsafe { input_key.assume_init() }; | ||
Ok(input_key) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this sized based on 2 pages, rather than the more common single page size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that size was picked up from Windows implementation. I am open to a more appropriate size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.