Proposal: Rework UEFI API

[Exonorid]

At the moment, std.os.uefi is a very barebones wrapper around UEFI. I'm proposing to rework it in the following ways:

• Make functions that can fail return error unions
• Pass slices to functions instead of separate lengths and pointers
• Make reading and writing functions return the number of bytes read or written respectively
• Use packed struct bitfields instead of ORed integers

As an example, here is the current implementation of std.os.uefi.protocols.FileProtocol.read:

zig/lib/std/os/uefi/protocols/file_protocol.zig

Lines 36 to 38 in 132b18e

	pub fn read(self: *const FileProtocol, buffer_size: *usize, buffer: [*]u8) Status {
	return self._read(self, buffer_size, buffer);
	}

There are two main issues with this as I see it:

1. The function returns an enum instead of an error union. This allows the error to be fairly easily discarded using _ = . On top of that, without looking at the UEFI specification itself, it's very unclear what errors the functions could return.
2. When passing the output buffer to the function, it takes a pointer to the buffer length as a parameter. The actual UEFI function then reads the file into the buffer and writes the number of bytes read to said pointer. On top of passing the pointer and length separately being completely unnecessary while slices exist, having the length of the buffer passed and then changed can lead to all kinds of confusion.

Under this proposal, that function would look like this:

pub fn read(self: *const FileProtocol, buffer: []u8) !usize { 
    var buffer_size = buffer.len;
    return switch(self._read(self, buffer_size, buffer.ptr)) {
        .Success => buffer_size,
        .NoMedia => UEFIError.NoMedia,
        .DeviceError => UEFIError.DeviceError,
        .VolumeCorrupted => UEFIError.VolumeCorrupted,
        .BufferTooSmall => buffer_size, //Didn't read to the end of the file, which Reader.read doesn't consider an error
        else => unreachable,
    };
} 

This implementation, while a bit more complex, is much nicer to work with as an end user, as well as being more idiomatic.

[Exonorid]

I forgot to add this, but it might be good to change functions taking a protocol GUID and pointer as parameters to instead take the protocol type and return the protocol instead of passing the GUID and a pointer to the uninitialized protocol interface. Something like:

pub fn locateProtocol(self: *const BootServices, comptime T: type, registration: ?Registration) !T {
    var handle: ?T = null;
    return switch(self._locate_protocol(&T.guid, registration, @ptrCast(*?*c_void, &handle))) {
        .Success => handle.?, //Handle is only null if the protocol is not found
        .InvalidParameter => UefiError.InvalidParameter,
        .NotFound => UefiError.NotFound,
        else => unreachable,
    };
}

[daurnimator]

See #6600

[fifty-six]

Forgot to say this earlier, but one thing of note regarding error unions and unreachable, is that the unreachable case isn't actually always unreachable, annoyingly enough. If I remember correctly, I've had a case where when running on actual hardware I got a return that wasn't under spec, and I'd imagine people do want to be able to handle that type of thing when possible. Aside from it being a lot easier, that's the main reason I added .err() to Status as opposed to returning actual error unions.

[ben-krieger]

Forgot to say this earlier, but one thing of note regarding error unions and unreachable, is that the unreachable case isn't actually always unreachable, annoyingly enough. If I remember correctly, I've had a case where when running on actual hardware I got a return that wasn't under spec, and I'd imagine people do want to be able to handle that type of thing when possible. Aside from it being a lot easier, that's the main reason I added .err() to Status as opposed to returning actual error unions.

Confirming that you cannot expect status codes in the spec to be comprehensive. See https://uefi.org/specs/UEFI/2.10/01_Introduction.html#procedure-descriptions:

Status Codes Returned: A description of any codes returned by the interface. The procedure is required to implement any status codes listed in this table. Additional error codes may be returned, but they will not be tested by standard compliance tests, and any software that uses the procedure cannot depend on any of the extended error codes that an implementation may provide.

[fifty-six]

I think ideally, we'd wait for #2647 - and then we could have something like

pub fn read(self: *const FileProtocol, buffer: []u8) !usize { 
    var buffer_size = buffer.len;
    return switch(self._read(self, buffer_size, buffer.ptr)) {
        .Success => buffer_size,
        .NoMedia => UEFIError.NoMedia,
        .DeviceError => UEFIError.DeviceError,
        .VolumeCorrupted => UEFIError.VolumeCorrupted,
        .BufferTooSmall => buffer_size, 
        else => |s| UEFIError.Unexpected { status = s }
    };
}

as then we can still have a much better ux, but we'll still be able to handle manufacturer specific stuff without a panic and all, which is definitely an improvement over unreachable. Though if people have reasons against this, I'd love to hear feedback.

On an aside, while it's not exactly locate, for future reference to readers, we do have openProtocolSt now,

zig/lib/std/os/uefi/tables/boot_services.zig

Line 162 in 7abf9b3

pub fn openProtocolSt(self: *BootServices, comptime protocol: type, handle: Handle) !*protocol {

which does follow the pattern of passing in a type, which I will agree was definitely a huge improvement to usability. I'm not sure how much other people use locateProtocol, but if there's a want I can definitely just PR a quick impl, if the one in the issue isn't fine already.

[fifty-six]

For what it's worth, I think it'd probably still be worth having things like File.read and similar return the result that's usually passed by pointer and just giving the error union version of Status back as it's a fairly substantial improvement in UX, even if we don't have a nicer error type. Considering like

var root: *const protocols.File = undefined;
try fp.openVolume(&root).err();

var drivers: *const protocols.File = undefined;
root.open(&drivers, "zbl\\drivers") catch {
    return;
};

would be much nicer as just

const root = try fp.openVolume();
const drivers = root.open("zbl\\drivers") catch return;

This would be a fairly substantial breaking change though - but I think it's worth improving the UEFI ux at some point, especially as the 'raw' functions are available as _func anyways. I'm curious - would you accept a PR for something like this @andrewrk ? Or is it something that'd be better done in the future with possible error payloads or just not at all?