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

[un]install_multiple_protocol_interfaces() requires but is currently incompatible with feature c_variadic #302

Open
timrobertsdev opened this issue Oct 19, 2021 · 11 comments
Labels

Comments

@timrobertsdev
Copy link
Contributor

timrobertsdev commented Oct 19, 2021

Tracking issue

While implementing the missing protocol handler functions in BootServices, I found that extern "efiapi" is incompatible with Rust's current (unstable) implementation of c variadics. Rustc indicates that C variadics must be extern "C" or extern "cdecl", thus install_multiple_protocol_interfaces() and uninstall_multiple_protocol_interfaces() cannot be implemented at this time.

I'll bring this up on the C variadic tracking issue in the next couple days if it hasn't been already.

Rust tracking issue: rust-lang/rust#44930

EDIT: technically I guess this could be possible with some clever and wildly unsafe asm but I'd much rather wait for Rust to support it.

@timrobertsdev
Copy link
Contributor Author

Update: Per Josh Triplett on the rustlang zulip, extern "C" on the "x86_64-unknown-uefi" target should be the proper UEFI ABI, so we can actually implement this, using extern "C" instead of extern "efiapi".

@GabrielMajeri
Copy link
Collaborator

@timrobertsdev does this mean we should replace all extern "efiapi" declarations in the crate with extern "C", and everything will just work correctly? Or is this workaround just for the protocol handler functions?

@timrobertsdev
Copy link
Contributor Author

The way I understood it, since we're targeting EFI, extern "C" will always be the correct calling convention. I've replaced all extern "efiapi" with extern "C" locally and everything still builds and runs/tests correctly.

@GabrielMajeri
Copy link
Collaborator

@timrobertsdev Ah, I've realized there's a little problem which stops us from switching everything over to extern "C".

The reason we've introduced the efiabi calling convention was to avoid issues like #38 -- if uefi-rs is used as a library in an OS kernel/driver which interacts with UEFI structures but which is built as an ELF binary, then it won't be able to call the methods correctly, unless they're declared with efiabi.

I'm not sure what the correct solution would be here.

@nicholasbishop
Copy link
Contributor

Perhaps the [un]install_multiple_protocol_interfaces function pointers could be declared with extern "C", and then the wrapper methods that call them could be guarded behind a #[cfg(...)] that only allows them to be called from uefi arches, until such time as efiabi supports variadics.

@timrobertsdev
Copy link
Contributor Author

@nicholasbishop That sounds like a good solution to me.

Would an ELF binary be capable of calling these functions (or any boot service functions, I guess)? I had assumed that boot services would be exited by the time a kernel is running.

@nicholasbishop
Copy link
Contributor

Fair point, I think Linux at least exits boot services quite early in the EFI stub.

@GabrielMajeri
Copy link
Collaborator

Would an ELF binary be capable of calling these functions (or any boot service functions, I guess)? I had assumed that boot services would be exited by the time a kernel is running.

That's the issue... we can't really guarantee this. It's what a logical, sane UEFI-based application/kernel would do: first load an ELF binary, then exist boot services and jump into it. But if we want to be pedantic and respect Rust's safety guarantees in all cases, something with a cfg guard like @nicholasbishop suggested would be more appropriate.

@nicholasbishop
Copy link
Contributor

Quick update here, on current nightly it looks like this is now possible to use efiabi with variadics. However, it's currently gated by the extended_varargs_abi_support feature so I would recommend we not add it for now to avoid requiring more unstable features.

(Also, as far as I can tell, InstallMultipleProtocolInterfaces is just a thin wrapper around calling InstallProtocolServices. We don't yet have the latter implemented, but we could add that easily I think.)

@medhefgo
Copy link
Contributor

Just chiming in here to say that getting the calling convention right isn't your only concern if you want to (correctly) support cross ABI calls. If any structs are involved you have to make sure that they are laid out correctly according to the EFI ABI.
In particular, x86 sysv ABI has long long 4-byte aligned if it appears within a struct. The MS/EFI ABI expects it to be 8-byte aligned.

This will cause loaded image protocol (in particular) to have a bad memory layout if the uefi-rs definition were used in ELF ABI context. Whether that is a use-case you want to support or not is a different matter.

@nicholasbishop
Copy link
Contributor

At least with the install_multiple_protocol_interfaces API, that shouldn't be a problem. It's only callable while boot services are active, which means you are running a UEFI executable that uses the efiabi calling convention. I think we only need to worry about cross-ABI calls when in runtime-services mode after an OS loads.

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

No branches or pull requests

4 participants