Skip to content

std: implement basic io for UEFI - #22226

Closed
RossComputerGuy wants to merge 10 commits into
ziglang:masterfrom
RossComputerGuy:feat/uefi-zig-init
Closed

std: implement basic io for UEFI#22226
RossComputerGuy wants to merge 10 commits into
ziglang:masterfrom
RossComputerGuy:feat/uefi-zig-init

Conversation

@RossComputerGuy

Copy link
Copy Markdown
Contributor

Largely based on #19486, this PR marks me starting to work on UEFI support in Zig again. This time, there will be multiple PR's instead of one so it becomes easier to manage. This PR simply makes the zig init example build.

I tested this in QEMU by loading in OVMF, entering the UEFI shell, loading FS0:\bin\$.efi. The zig-out can be shared by adding: -drive file=fat:rw:zig-out.
image

Comment threadlib/std/os/uefi.zig Outdated
@@ -1,5 +1,7 @@
const std = @import("../std.zig");

pub const posix = @import("uefi/posix.zig");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Last I talked with Andrew about the future of std.posix, one thing we agreed on is that probably all Windows code in std.posix should be deleted. I would expect that the exact same thing is true of UEFI, as it is also not a POSIX platform in any meaningful sense?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, I agree. The UEFI POSIX stuff is just a POSIX wrapper around the UEFI protocols. We can always ditch it. UEFI is close to Windows so I think whatever we do for Windows and POSIX, it would apply to UEFI.

Comment threadlib/std/Thread/Futex.zig Outdated
}

const Impl = if (builtin.single_threaded)
const Impl = if (builtin.single_threaded or builtin.os.tag == .uefi)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should UEFI be treated as a proper single-threaded target?

zig/src/target.zig

Lines 66 to 81 in 82f35c5

pubfnalwaysSingleThreaded(target: std.Target) bool {
_=target;
returnfalse;
}
pubfndefaultSingleThreaded(target: std.Target) bool {
switch (target.cpu.arch) {
.wasm32, .wasm64=>returntrue,
else=> {},
}
switch (target.os.tag) {
.haiku=>returntrue,
else=> {},
}
returnfalse;
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I just didn't know where the source was which defined that heh.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've updated this and changed the defaultSingleThreaded for UEFI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, is UEFI not always single threaded instead of just default single threaded?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You technically could implement a form of multithreading, just like with wasm. It's just builtin.single_threaded will default to true when single_threaded is not changed in your build. You probably could write a custom scheduler + mutltithreading stuff and use root.os stuff to get it working but no implementation will be made for upstream.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I learned somewhat recently that UEFI does have EFI_MP_SERVICES_PROTOCOL so there is a way to do multiprocessing but not multithreading but likely is still out of the scope.

Comment threadlib/std/fs.zig Outdated
/// no particular encoding.
pub const max_path_bytes = switch (native_os) {
.linux, .macos, .ios, .freebsd, .openbsd, .netbsd, .dragonfly, .haiku, .solaris, .illumos, .plan9, .emscripten, .wasi => posix.PATH_MAX,
.linux, .macos, .ios, .freebsd, .openbsd, .netbsd, .dragonfly, .haiku, .solaris, .illumos, .plan9, .emscripten, .wasi, .uefi => posix.PATH_MAX,

@linusglinusgDec 19, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks wrong, I'd assume it needs to use similar logic as windows

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think this ties in with #22226 (comment)

Comment threadlib/std/fs/Dir.zig Outdated

pub fn next(self: *Self) Error!?Entry {
_ = self;
return null;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure if turning a compile error into a non-functional stub is that useful? Most people will assume the standard library functions work for a certain target if they compile.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, I hadn't figured out how to implement it just yet. I'll leave it out of this PR and work on it later on.

Comment threadlib/std/os/uefi/allocator.zig Outdated
/// Allocates memory in pages.
///
/// This allocator is backed by `allocatePages` and is therefore only suitable for usage when Boot Services are available.
pub const PageAllocator = struct {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have changed it now, hopefully it doesn't violate the fully qualified namespace stuff.

Comment threadlib/std/start.zig Outdated
if (@errorReturnTrace()) |trace| {
std.debug.dumpStackTrace(trace.*);
}
std.time.sleep(5 * std.time.ns_per_s);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Magic sleep needs a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This part of the code is based on what @truemedian did. I'm not sure either so CC'ing them for some information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Magic sleep here was a debugging aid to prevent the uefi firmware from clearing the screen before I could read the any output because once EfiMain returns it will go to the next boot loader. Not necessary at all, unless that window to eead the error is desired.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ok, I likely will remove it and it'll be up to the program to do that if they want to.

@linusglinusg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'll have to ask you to split this up even more - there are number of changes in here that need more discussion or simply won't be merged as-is. I think the allocator improvements should be their own PR, not touching anything unrelated.

The changes around threading are nonsensical - UEFI is by definition single threaded, so alwaysSingleThreaded() ought to be the function that gets modified, which in turn means the panic handler doesn't need a stub for std.Thread.getCurrentId().

@RossComputerGuy

RossComputerGuy commented Feb 8, 2025

Copy link
Copy Markdown
ContributorAuthor

I'll have to ask you to split this up even more - there are number of changes in here that need more discussion or simply won't be merged as-is. I think the allocator improvements should be their own PR, not touching anything unrelated.

Will do

The changes around threading are nonsensical - UEFI is by definition single threaded, so alwaysSingleThreaded() ought to be the function that gets modified, which in turn means the panic handler doesn't need a stub for std.Thread.getCurrentId().

Welll, kinda. The UEFI PI spec does mention a way to do multiprocessing which in turn can lead to multithreading.

@linusg

Copy link
Copy Markdown
Collaborator

Welll, kinda. The UEFI PI spec does mention a way to do multiprocessing which in turn can lead to multithreading.

You don't even need to run code on another core for "threading", but from the standard library's point of view we always have a single thread. Preparing for a scenario that is unlikely to ever be useful and only creates the need for more stub code isn't desirable, let's start small :)

@RossComputerGuy

Copy link
Copy Markdown
ContributorAuthor

I found out while slicing this PR that we actually do have to mock threading.

/home/ross/zig/lib/std/Thread.zig:558:9: error: Unsupported operating system uefi
@compileError("Unsupported operating system " ++ @tagName(native_os));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
referenced by:
getCurrentId: /home/ross/zig/lib/std/Thread.zig:533:27
getCurrentId: /home/ross/zig/lib/std/Thread.zig:354:29
lock: /home/ross/zig/lib/std/Thread/Mutex/Recursive.zig:50:54
lockStdErr: /home/ross/zig/lib/std/Progress.zig:545:22
lockStdErr: /home/ross/zig/lib/std/debug.zig:198:28
print__anon_1981: /home/ross/zig/lib/std/debug.zig:208:15
main: src/main.zig:7:20
EfiMain: /home/ross/zig/lib/std/start.zig:223:37
comptime: /home/ross/zig/lib/std/start.zig:75:58
start: /home/ross/zig/lib/std/std.zig:97:27
comptime: /home/ross/zig/lib/std/std.zig:168:9

@RossComputerGuy
RossComputerGuyforce-pushed the feat/uefi-zig-init branch 2 times, most recently from 31ebae5 to 3d3cfa3CompareFebruary 8, 2025 04:47
@RossComputerGuy

Copy link
Copy Markdown
ContributorAuthor

Ok, things are split up now. I went through and dropped changes which were not necessary to make this work:

$ zig init
$ zig build -Dtarget=$(uname -m)-uefi

@RossComputerGuy

Copy link
Copy Markdown
ContributorAuthor

I think the allocator improvements should be their own PR, not touching anything unrelated.

Going to do this right now since the UEFI allocators fix PR got merged.

@RossComputerGuyRossComputerGuy changed the title std: implement basic io and improve alloc in uefistd: implement basic ioFeb 8, 2025
@RossComputerGuy

Copy link
Copy Markdown
ContributorAuthor

Allocator stuff is now in #22818

Comment threadlib/std/start.zig Outdated
Comment threadlib/std/os/uefi/posix.zig Outdated
// rudimentary utf16 writer
var index: usize = 0;
var utf16: [256]u16 = undefined;
while (iter.nextCodepoint()) |rune| {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably better to avoid the term rune.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Is codepoint a better name?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Renamed to codepoint

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

codepoint is a good name, it is coming from nextCodepoint after all.

Comment threadlib/std/posix.zig Outdated
@RossComputerGuy
RossComputerGuyforce-pushed the feat/uefi-zig-init branch 2 times, most recently from a3461b7 to 5208154CompareFebruary 10, 2025 05:15
@RossComputerGuy

Copy link
Copy Markdown
ContributorAuthor

Rebased

@andrewrkandrewrk changed the title std: implement basic iostd: implement basic io for UEFIFeb 23, 2025
@RossComputerGuy

Copy link
Copy Markdown
ContributorAuthor

Rebased

@dotcarmendotcarmen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm on first glance, my primary concern is around the change to OpenMode. haven't otherwise read through the posix implementation details which i'll do soon

Comment threadlib/std/fs/File.zig
Comment on lines +191 to +195
.handle = blk: {
if (is_windows) break :blk windows.peb().ProcessParameters.hStdOutput;
if (is_uefi) break :blk .{ .simple_output = std.os.uefi.system_table.con_out.? };
break :blk posix.STDOUT_FILENO;
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpicky but can you do this instead and below:

Suggested change
.handle=blk: {
if (is_windows) break :blkwindows.peb().ProcessParameters.hStdOutput;
if (is_uefi) break :blk .{ .simple_output=std.os.uefi.system_table.con_out.? };
break :blkposix.STDOUT_FILENO;
},
.handle=if (is_windows)
windows.peb().ProcessParameters.hStdOutput
elseif (is_uefi)
.{ .simple_output=std.os.uefi.system_table.con_out.? }
else
posix.STDOUT_FILENO,

it simplifies the control flow and is easier to read

Comment on lines +105 to +108
fn unexpectedError(err: anyerror) error{Unexpected} {
std.log.err("unexpected error: {}\n", .{err});
return error.Unexpected;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you colocate this with std.os.uefi.unexpectedStatus and change the param to err: uefi.Error?

// 0x8000000000000000
create: bool = false,
};
pub const OpenMode = packed struct(u64) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OpenMode only selects 3 options according to UEFI which is why it's an enum. why lift the bits struct?

alternatively you can make this enum open (_) and add toBits and fromBits methods. IMO that's preferred because it's a signal to the user "this is an unsupported value, you probably don't want this"

Comment on lines +452 to +456
const fd = p.open(path_buffer[0..len :0], .{
.read = true,
.write = flags.CREAT or flags.TRUNC or flags.ACCMODE != .RDONLY,
.create = flags.CREAT,
}, .{}) catch |err| switch (err) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to avoid creating an invalid OpenMode you can do something like:

constopen_mode: OpenMode=if (flags.CREAT) mode: {
if (!flags.TRUNCorflags.ACCMODE==.RDONLY) {
// UEFI doesn't allow create without write flagsreturnerror.PermissionDenied;
break :mode.read_write_create;
} elseif (flags.TRUNCorflags.ACCMODE!=.RDONLY)
.read_writeelse.read;

Comment threadlib/std/Thread.zig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so alwaysSingleThreaded() ought to be the function that gets modified, which in turn means the panic handler doesn't need a stub for std.Thread.getCurrentId().

do we need this stub still?

Comment threadlib/std/start.zig
Comment on lines +232 to +241
switch (@TypeOf(result)) {
void => return 0,
u8, usize => {
return result;
},
uefi.Status => {
return @intFromEnum(result);
},
else => @compileError(bad_efi_main_ret),
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We're not reintroducing untyped u8/usize return values. Keep this as is and only touch the existing switch (err) {}.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fundamentally opposed to this, anything that uses std.posix as a generic fallback impl should have a dedicated UEFI impl instead. std.posix's days are numbered anyway: #24329 (comment)

return path;
}

pub fn realpath(pathname: []const u8, out_buffer: *[PATH_MAX]u8) std.posix.RealPathError![]u8 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@andrewrk
andrewrk self-requested a review July 14, 2025 17:07
@andrewrk

Copy link
Copy Markdown
Member

Self-requesting a review to block this from landing until I take a look at it. I think this will likely need to be redone differently depending on how this Io interface shakes out.

@andrewrk

Copy link
Copy Markdown
Member

This pull request is not ready for review because:

  • It has conflicts that must be resolved via rebasing against latest origin/master.

Since we have moved development to Codeberg, please open your pull request there if you would like to continue these efforts.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@RossComputerGuy@linusg@andrewrk@alexrp@Vexu@truemedian@mochalins@dotcarmen