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

All firmware API function pointers in std.os.uefi should be marked with callconv(.Win64) #16309

Closed
oliver-giersch opened this issue Jul 3, 2023 · 6 comments · Fixed by #16339
Labels
Milestone

Comments

@oliver-giersch
Copy link

Example:

raiseTpl: *const fn (new_tpl: usize) callconv(.C) usize,

should become:

pub const BootServices = extern struct {
    raiseTpl: *const fn (new_tpl: usize) callconv(.Win64) usize, 
    // ...
};

Using callconv(.C) works when compiling for an uefi target, but not in, e.g., in an EFI stub with a different ABI, like the Linux kernel with CONFIG_EFI_STUB.

From my understanding, using callconv(.Win64) would still be correct for the uefi target, but would allow also calling these functions in a strictly greater set of programs, such as EFI-stub OS kernels with no downsides, without having to use ABI conversion wrappers.

Am I correct in my assessment? If so, I would be willing to open a corresponding PR, I just wanted to make sure I am not missing something, first.

@billzez
Copy link
Contributor

billzez commented Jul 5, 2023

I also hit this issue. I would like to compile my efi app with a different ABI. There are too many libraries out there that expect LP64.

@wooster0
Copy link
Contributor

wooster0 commented Jul 5, 2023

I would like to compile my efi app with a different ABI. There are too many libraries out there that expect LP64.

Am I understanding correctly that sometimes the .C calling convention is desired but sometimes also the .Win64 CC? In that case I think we might want to make the CC configurable using a standard library option:

pub const options = struct {

It might look like this:

    pub const uefi_cc: std.builtin.CallingConvention = if (@hasDecl(options_override, "uefi_cc"))
        options_override.uefi_cc
    else
        .Win64;

In all the UEFI function definitions it would be used like this:

    raiseTpl: *const fn (new_tpl: usize) callconv(std.options.enable_segfault_handler) usize, 

@oliver-giersch
Copy link
Author

Am I understanding correctly that sometimes the .C calling convention is desired but sometimes also the .Win64 CC? In that case I think we might want to make the CC configurable using a standard library option:

Not quite I don't think. The UEFI spec requires all functions to adhere to the Win64 calling convention. It just happens to be that when compiling for the uefi target, the .C just happens to be the same as the .Win64 callconv. At least that is my understanding. So callconv(.Win64) should always be the correct choice, whereas callconv(.C) only works when compiling for the uefi target.

@jayschwa
Copy link
Contributor

jayschwa commented Jul 6, 2023

From looking at the spec, I don't think callconv(.Win64) would apply universally. There are other platforms to support, including 32-bit x86.

https://uefi.org/specs/UEFI/2.10/02_Overview.html#calling-conventions

@oliver-giersch
Copy link
Author

oliver-giersch commented Jul 6, 2023

Ah, it is, of course, more complicated than I initially thought. And I also just saw .Win64 is x86-64 only. So the combination of target uefi and callconv .C is trivially correct, but for other UEFI supported architectures it is not quite clear to me which calling convention applies, except for x86-64. I will try to look into this further to find out which zig calling convention maps to each platform in UEFI.

FWIW, this how Linux declares EFI functions:

https://github.com/torvalds/linux/blob/c17414a273b81fe4e34e11d69fc30cc8b1431614/include/linux/efi.h#L52-L58

#if defined(CONFIG_X86_64)
#define __efiapi __attribute__((ms_abi))
#elif defined(CONFIG_X86_32)
#define __efiapi __attribute__((regparm(0)))
#else
#define __efiapi
#endif

So it seems that callconv(.C) should be used for all platforms except x86 and x86-64? I have to check which zig calling convention maps to GCC's __attribute__((regparm(0))).

@oliver-giersch
Copy link
Author

oliver-giersch commented Jul 6, 2023

Ok so I've read up on the topic a bit and to me it seems that GCC's regparm, which LLVM also has, is indeed the correct calling convention for 32-bit x86 in UEFI, so I'd propose to add the following to std.os.uefi and use it for all UEFI function pointers:

pub const ueficc = switch (@import("builtin").target.cpu.arch) {
    .x86_64 => .Win64,
    .x86 => .regparm,
    else => .C,
};

Only problem is, zig's std.builtin.CallingConvention does not yet expose LLVM's regparm, so that would have to be added at the compiler level (and the stdlib), but that should not be too hard.

In the meantime, I've found a somewhat acceptable workaround for x86-64:

fn efiCall(fn_ptr: anytype) *const efiABI(@TypeOf(fn_ptr)) {
    return @ptrCast(*const efiABI(@TypeOf(fn_ptr)), fn_ptr);
}

fn efiABI(comptime fn_ptr: type) type {
    const fn_info = @typeInfo(fn_ptr);
    if (fn_info.Pointer) |_| {} else {
        @compileError("type parameter is not a function.");
    }

    var info = @typeInfo(fn_info.Pointer.child);
    if (info.Fn) |_| {} else {
        @compileError("type parameter is not a function.");
    }

    if (info.Fn.calling_convention == .Win64) {
        @compileError("function already has correct calling convention");
    }

    info.Fn.calling_convention = .Win64;
    return @Type(info);
}

(this allows casting any callconv(.C) function pointer to a callconv(.Win64) one)

EDIT:

Some further prior art: Rust has efiapi and this is what EDK2 does. I am beginning to think that regparm is something that is linux specific and not generally applicable. This would simplify things further and require no changes to the compiler itself:

pub const ueficc = switch (@import("builtin").target.cpu.arch) {
    .x86_64 => .Win64,
    else => .C,
};

wooster0 added a commit to wooster0/zig that referenced this issue Jul 6, 2023
I didn't test this at all (should definitely compile though) but if
there's anything wrong it shouldn't be hard to fix.
With this change it's going to be very easy to make further adjustments
ot the calling conventions of all these external UEFI functions.

Closes ziglang#16309
wooster0 added a commit to wooster0/zig that referenced this issue Jul 6, 2023
I didn't test this at all (should definitely compile though and these
changes were done programmatically) but if there's anything wrong
it shouldn't be hard to fix.
With this change it's going to be very easy to make further adjustments
to the calling conventions of all these external UEFI functions.

Closes ziglang#16309
wooster0 added a commit to wooster0/zig that referenced this issue Jul 6, 2023
I tested this and this definitely compiles and these
changes were done programmatically but if there's still anything wrong
it shouldn't be hard to fix.
With this change it's going to be very easy to make further adjustments
to the calling conventions of all these external UEFI functions.

Closes ziglang#16309
@andrewrk andrewrk added this to the 0.11.0 milestone Jul 8, 2023
wooster0 added a commit to wooster0/zig that referenced this issue Jul 8, 2023
I tested this and this definitely compiles and these
changes were done programmatically but if there's still anything wrong
it shouldn't be hard to fix.
With this change it's going to be very easy to make further adjustments
to the calling conventions of all these external UEFI functions.

Closes ziglang#16309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants