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

lib/systems: add UEFI systems for i686, aarch64, x86_64 as examples #226145

Closed
wants to merge 7 commits into from

Conversation

RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Apr 14, 2023

Latest attempt at #228374

Description of changes

It also adds the metadata for Rust.

So @alyssais suggested me the next step should be to create a UEFI stdenv.
I'm not very knowledgeable on adding such stdenvs in nixpkgs, should this be part of pkgs/stdenv ? or pkgs/build-support/uefi ?

It's uncertain to me.

Thanks to @rrbutani help, we were able to get Lanzaboote's stub to build!

Steps to make it acceptable:

  • Acceptable hack for UEFI systems
  • Acceptable solution for NIX_ENFORCE_PURITY
  • Acceptable solution for cc-wrapper at linkage time
  • Acceptable solution to order -z relro -z ... properly and re-enable hardening
  • Acceptable solution for cargo-auditable
  • Acceptable solution for unsupported hardening flags
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

lib/systems/examples.nix Outdated Show resolved Hide resolved
lib/systems/examples.nix Outdated Show resolved Hide resolved
@alyssais
Copy link
Member

Since we're not sure yet what the best triple to use is going to be (since GNU is unlikely to ever support UEFI in the standard toolchain, we might want to use the LLVM ones here), I don't think it's useful to add the examples until we can actually prove we can build stuff on them. The examples should come at the end of the bringup process.

rrbutani and others added 4 commits April 15, 2023 17:30
for GCC: https://wiki.osdev.org/UEFI_App_Bare_Bones

`nix-build --expr "(import ./. { crossSystem = (import ./lib).systems.examples.i686-uefi; }).stdenv"`
`nix-build --expr "(import ./. { crossSystem = (import ./lib).systems.examples.i686-uefi; }).buildPackages.rustc"`
`nix-build --expr "(import ./. { crossSystem = (import ./lib).systems.examples.x86_64-uefi; }).lanzaboote"`

`rustc` wants a linker that supports MSVC link flags; the `cc-wrapper`
wrapped clang does not (though I think the `lld` underneath would be
fine)
isEfi does not really make sense as a name when nixpkgs can target (U)EFI
environments.

Therefore, we rename it `hasUefi`: does the platform has a known UEFI
firmware/environment?

`isUefi` should be used to say: is this platform an UEFI environment?
@RaitoBezarius
Copy link
Member Author

Since we're not sure yet what the best triple to use is going to be (since GNU is unlikely to ever support UEFI in the standard toolchain, we might want to use the LLVM ones here), I don't think it's useful to add the examples until we can actually prove we can build stuff on them. The examples should come at the end of the bringup process.

I transformed this PR into something that will introduce Lanzaboote's stub as an example.

@@ -0,0 +1,31 @@
{ rustPlatform, stdenv, fetchFromGitHub, lib, breakpointHook }: rustPlatform.buildRustPackage {
Copy link
Contributor

Choose a reason for hiding this comment

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

breakpointHook

pkgs/tools/misc/lanzaboote/default.nix Outdated Show resolved Hide resolved

# See: https://github.com/rust-lang/rust/pull/56769/files#r241916113
i686-uefi = {
# config = "i686-unknown-windows-gnu";
Copy link
Member

Choose a reason for hiding this comment

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

What's with those comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please read #226291 first :P

@@ -6,6 +6,8 @@ with lib.lists;
let abis_ = abis; in
let abis = lib.mapAttrs (_: abi: builtins.removeAttrs abi [ "assertions" ]) abis_; in

# TODO: isUefi

Copy link
Member

Choose a reason for hiding this comment

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

Can you write at least a half sentence otherwise we forget what we wanted to do here

Copy link
Member Author

Choose a reason for hiding this comment

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

It was supposed to add a isUefi function here so we can test for it


cargoHash = "sha256-FlnheCgowYsEHcFMn6k8ESxDuggbO4tNdQlOjUIj7oE=";

# TODO: limit supported platforms to UEFI
Copy link
Member

Choose a reason for hiding this comment

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

Missing the full meta and meta should be last.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is more a test package PoC to show the "value" of those changes rather than the real package yet

@RaitoBezarius RaitoBezarius marked this pull request as draft April 15, 2023 17:52
@RaitoBezarius
Copy link
Member Author

Converting to draft to signal WIP state.

@RaitoBezarius RaitoBezarius added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023
clangUseLLVM = wrapCCWith rec {
clangUseLLVM =
if stdenv.targetPlatform.libc == null then
builtins.trace "yo" tools.clangNoLibc # TODO: gate correctly...
Copy link
Member Author

Choose a reason for hiding this comment

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

@rrbutani what did you mean here for the gating? It seems like it's correct to me?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I wasn't sure whether we wanted to check for libc == null here or specifically exempt UEFI targets like we do in all-packages.nix (which will complain if it sees libc == null for a target it doesn't have special handling for):
https://github.com/NixOS/nixpkgs/blob/132b7981ef4ea88d9810ff286c1bee06bb8bf022/pkgs/top-level/all-packages.nix#L20381-L20404


But I think you're right and that this is fine.

cc = tools.clang-unwrapped;
libcxx = targetLlvmLibraries.libcxx;
bintools = bintools';
extraPackages = [
libcxx.cxxabi
targetLlvmLibraries.compiler-rt
] ++ lib.optionals (!stdenv.targetPlatform.isWasm) [
] ++ lib.optionals (!stdenv.targetPlatform.isWasm && !stdenv.targetPlatform.isUefi) [
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're gating the wrapped CC with libc/libcxx/compiler-rt/libunwind on having libc != null, I think this isn't needed anymore for the UEFI targets?

@@ -224,11 +227,12 @@ in let
"-Wno-unused-command-line-argument"
"-B${targetLlvmLibraries.compiler-rt}/lib"
]
++ lib.optional (!stdenv.targetPlatform.isWasm) "--unwindlib=libunwind"
++ lib.optional (!stdenv.targetPlatform.isWasm && !stdenv.targetPlatform.isUefi) "--unwindlib=libunwind"
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto; is this needed?

++ lib.optional
(!stdenv.targetPlatform.isWasm && stdenv.targetPlatform.useLLVM or false)
"-lunwind"
++ lib.optional stdenv.targetPlatform.isWasm "-fno-exceptions";
dontWrap = stdenv.targetPlatform.isUefi;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto; is this needed?

@alyssais alyssais mentioned this pull request Apr 26, 2023
12 tasks
@@ -50,7 +50,7 @@ rec {
else if final.isFreeBSD then "fblibc"
else if final.isNetBSD then "nblibc"
else if final.isAvr then "avrlibc"
else if final.isNone then "newlib"
else if final.isUefi then "none" # TODO: execute can actually be QEMU?
Copy link

Choose a reason for hiding this comment

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

I don't understand what this TODO is saying.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we emulate UEFI programs on QEMU? I guess we could have a remote builder for UEFI if we had baremetal compilers in UEFI, etc. :D ?

Copy link

Choose a reason for hiding this comment

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

Can we emulate UEFI programs on QEMU?

Probably. I just can't figure out what this has to do with the choice of libc.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Please break this up into a minimum of three separate PRs: lib/systems, cc-wrapper, and everything else.

The lib/systems part needs some changes (see below) but is basically feasible. I have very serious concerns about the cc-wrapper approach.

# See: https://github.com/rust-lang/rust/pull/56769/files#r241916113
i686-uefi = {
# config = "i686-unknown-windows-gnu";
config = "i686-pc-win32-coff";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "i686-pc-win32-coff";
config = "i686-w64-mingw32";

This is the config.sub triple; see #226291 (comment)


x86_64-uefi = {
# config = "x86_64-unknown-windows-msvc";
config = "x86_64-pc-win32-coff";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "x86_64-pc-win32-coff";
config = "x86_64-w64-mingw32";

This is the config.sub triple; see #226291 (comment)


aarch64-uefi = {
# config = "aarch64-unknown-windows-msvc";
config = "aarch64-pc-win32-coff";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "aarch64-pc-win32-coff";
config = "aarch64-w64-mingw32";

This is the config.sub triple; see #226291 (comment)

@@ -82,7 +82,10 @@ rec {
isMusl = with abis; map (a: { abi = a; }) [ musl musleabi musleabihf muslabin32 muslabi64 ];
isUClibc = with abis; map (a: { abi = a; }) [ uclibc uclibceabi uclibceabihf ];

isEfi = [
isUefi = { /* kernel = kernels.windows; replace with none? */ abi = abis.coff; vendor = vendors.pc; }; # this may not be restrictive enough? TODO
Copy link

Choose a reason for hiding this comment

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

Suggested change
isUefi = { /* kernel = kernels.windows; replace with none? */ abi = abis.coff; vendor = vendors.pc; }; # this may not be restrictive enough? TODO
isUefi = { abi = abis.uefi; kernel = kernels.none; };

The most distinctive thing about UEFI, from a "linker's point of view" is that it has its own calling convention (ABI). Indeed, when Linux prostrates itself to venerate the firmware, it assumes the EfiApi calling convention.

Comment on lines +332 to +341
# Not really an ABI, rather an object format.
coff = {
assertions = [
{ assertion = platform: platform.isWindows || platform.isUefi;
message = ''
The "COFF" object file format is for Windows or UEFI targets.
'';
}
];
};
Copy link

Choose a reason for hiding this comment

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

Suggested change
# Not really an ABI, rather an object format.
coff = {
assertions = [
{ assertion = platform: platform.isWindows || platform.isUefi;
message = ''
The "COFF" object file format is for Windows or UEFI targets.
'';
}
];
};

isUefi = { /* kernel = kernels.windows; replace with none? */ abi = abis.coff; vendor = vendors.pc; }; # this may not be restrictive enough? TODO

# Those are platforms for known UEFI firmwares
hasUefi = [
Copy link

Choose a reason for hiding this comment

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

This doesn't belong in inspect.nix. The patterns are only for telling you about the platform inspected, not about what other platforms are associated with it. For example canExecute isn't in inspect.nix. hasUefi is in the same bucket as canExecute; it is a binary relation on platforms.

Comment on lines +21 to +23
# It is sometimes useful to expose naked binaries in the standard "wrapped" format.
# For UEFI for example.
, dontWrap ? false
Copy link

Choose a reason for hiding this comment

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

This is totally gross.

👎

@RaitoBezarius
Copy link
Member Author

Only for reference, superseded by #228374.

@Mic92 Mic92 deleted the uefi-systems branch April 28, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 6.topic: rust 6.topic: systemd 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants