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

std.os.uefi: use std.os.uefi.cc instead of .C as calling convention #16339

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

wooster0
Copy link
Contributor

@wooster0 wooster0 commented 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 #16309

@wooster0
Copy link
Contributor Author

wooster0 commented Jul 8, 2023

Well, I don't think .Win64 really works with variadic functions at all right now. Not on Windows or in the UEFI.

On x86_64-linux with .C calling convention:

$ cat uefi.zig
pub fn main() void {
    variadic(@as(u32, 1), @as(u8, 2), @as(i16, -3), @as([*:0]const u8, "hello"), false);
    while (true) {}
}

fn variadic(...) callconv(.C) void {
    var va_list = @cVaStart();
    defer @cVaEnd(&va_list);
    const h = @cVaArg(&va_list, u32);
    const e = @cVaArg(&va_list, u8);
    const l1 = @cVaArg(&va_list, i16);
    const l2 = @cVaArg(&va_list, [*:0]const u8);
    const o = @cVaArg(&va_list, bool);
    @import("std").debug.panic("{} {} {} {s} {}\n", .{ h, e, l1, l2, o });
}
$ build/stage3/bin/zig run uefi.zig
thread 124435 panic: 1 2 -3 hello false

Works.


Change .C to .Win64 and:

$ build/stage3/bin/zig build-exe uefi.zig -target x86_64-windows
$ wine uefi.exe
Segmentation fault at address 0xffffffffffffffff

Doesn't work (no panic output).


Now as for the UEFI:
Patch build/stage3/lib/zig/std/builtin.zig and change std_err to con_out so you actually see something (this is addressed by #16094 BTW).

$ build/stage3/bin/zig build-exe uefi.zig --name bootx64 -target x86_64-uefi-msvc; mkdir -p Boot/EFI/BOOT; mv bootx64.efi bootx64.pdb Boot/EFI/BOOT/
$ qemu-system-x86_64 -bios /usr/share/OVMF/OVMF_CODE.fd -drive format=raw,file=fat:rw:Boot

image

Doesn't work (no panic output).

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
…c functions

Now you can add new calling conventions that you confirmed to work with
variadic functions simply in a single place and the rest will work
automatically.
@sengir
Copy link
Contributor

sengir commented Jul 10, 2023

Does this correctly handle UEFI on ARM? ARM UEFI apparently uses the SMC calling convention, but Zig doesn't have such a calling convention. I haven't read enough of the spec PDF to know if that maps to another calling convention directly or if it's on LLVM to intuit the calling convention contextually.

@wooster0
Copy link
Contributor Author

I don't think either LLVM or Zig has a calling convention called "SMC". This PR does not do any changes in that regard and for ARM it will keep using .C.

@andrewrk andrewrk merged commit 2b8c1f0 into ziglang:master Jul 10, 2023
@truemedian
Copy link
Contributor

truemedian commented Jul 13, 2023

This change seems wrong. UEFI explicitly states that the Calling Convention used for ia32, x64, aarch32, aarch64, riscv and loongarch ("Intel Itanium-Based Platforms" use SAL) is the "the C language calling convention" (Sections 2.3.2 through 2.3.8). If the functions are not compiling correctly then the C calling convention is incorrect on UEFI.

This shouldn't change anything because Win64 is the C calling convention for x86_64 UEFI.

If the intent here was to make this work for OS stubs, then .C is likely never the correct calling convention (expect possibly ARM, which has standardized it).

@wooster0
Copy link
Contributor Author

wooster0 commented Jul 13, 2023

CC @oliver-giersch @billzez (if you got any input)

Perhaps only the first commit of this PR could be reverted then.

@oliver-giersch
Copy link

This change seems wrong. UEFI explicitly states that the Calling Convention used for ia32, x64, aarch32, aarch64, riscv and loongarch ("Intel Itanium-Based Platforms" use SAL) is the "the C language calling convention" (Sections 2.3.2 through 2.3.8). If the functions are not compiling correctly then the C calling convention is incorrect on UEFI.

This shouldn't change anything because Win64 is the C calling convention for x86_64 UEFI.

If the intent here was to make this work for OS stubs, then .C is likely never the correct calling convention (expect possibly ARM, which has standardized it).

Please re-read the original issue #16309, where I tried to make sense of things as best I could and collected some references on what other languages/projects (including the Linux kernel) do.
I am not 100% sure what you are arguing for, that the change should be reverted back to callconv(.C) because the UEFI spec says so or that callconv(.C) was the wrong choice in the first place?

In the former case, this will produce incorrect assembly for any .{ .cpu_arch = .x86_64, .os_tag = .freestanding, .abi = .none } target (e.g. an EFI-stub OS), because the "standard" C ABI is (I think) just assumed to be SystemV, and will put function call arguments into %rdi, %rsi, %rdx, ..., unlike what the UEFI API functions expect.

@truemedian
Copy link
Contributor

My argument is that this only fixes a very small part of the problem. std.os.uefi should either work for only the uefi target or all targets for the supported architectures. This change only allows for stubs on x86_64, and leaves the existing uefi-only semantics for the rest of the architectures.

Another thing to mention is that std.os.uefi will only work in a stub if the stub's EfiMain exactly mimics the EfiMain in start.zig, which may be important in the future.

It may be easier for an EFI stub to actually target uefi and then link the stub into your kernel (like Linux without a compressed kernel)

@oliver-giersch
Copy link

My argument is that this only fixes a very small part of the problem. std.os.uefi should either work for only the uefi target or all targets for the supported architectures. This change only allows for stubs on x86_64, and leaves the existing uefi-only semantics for the rest of the architectures.

Ok I see. I guess it is at least some incremental progress and everything that is still broken was also broken before. I personally do not have the stomach right now to dive into various calling conventions for architectures I have no familiarity with. That said, I am not even sure, if there are any issues with other platforms. In #16309 I referenced the Linux definition for EFIAPI which also has a special case treatment for x86-64. So either Linux does not permit the CONFIG_EFI_STUB option for Risc-V, ARM32/64, etc. or the "standard" C ABI for these platforms matches with the UEFI ABI just fine. At least I would expect if anyone runs efi-stub OS kernels for these platforms at all it would be Linux.

Another thing to mention is that std.os.uefi will only work in a stub if the stub's EfiMain exactly mimics the EfiMain in start.zig, which may be important in the future.

Not sure what you mean, but in my test efi-stub kernel I've defined it as pub export fn efiMain(handle: Handle, system_table: *SystemTable) callconv(.Win64) uefi.Status, which works as an entry point, since the x86-64 EFI handoff state puts these pointers in registers which conform with the Win64 ABI.

It may be easier for an EFI stub to actually target uefi and then link the stub into your kernel (like Linux without a compressed kernel)

Also not sure why that would be easier, I'd say it adds additional complexity to the build setup, but it may justified for other reasons that may not be obvious to me.

TUSF pushed a commit to TUSF/zig that referenced this pull request May 9, 2024
std.os.uefi: use std.os.uefi.cc instead of .C as calling convention
Sign up for free to 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.

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