Skip to content

linux: expose KBUILD_IMAGE as a property of the kernel#244706

Draft
RaitoBezarius wants to merge 8 commits intoNixOS:masterfrom
RaitoBezarius:kernel-targets
Draft

linux: expose KBUILD_IMAGE as a property of the kernel#244706
RaitoBezarius wants to merge 8 commits intoNixOS:masterfrom
RaitoBezarius:kernel-targets

Conversation

@RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Jul 21, 2023

Description of changes

Instead of the platform, see lib/systems/examples.nix for examples (no pun intended).

Nowadays, most architectures supports KBUILD_IMAGE which is
set by the architecture-specific Makefile or can be overriden by the user

Before that, we were using Makefile install targets such as "uinstall" for uImage,
"zinstall" for zImage, etc.

This PR aims multiple delicate things:

  • Expose KBUILD_IMAGE knob to build kernel with a desired image using the vanilla install Makefile target.
  • Do not break backward compatibility with lib/systems/examples.nix until we phase them out by keeping an override knob for Makefile install targets and passing the legacy ones.
  • Drop installTarget for x86, aarch64-multiplatform, riscv64 as a first "target" in this PR.

Once, we are able to do this, phase 2 is to move up the knob into NixOS to offer the capability to choose the kernel image to advanced users, i.e. compressed or non-compressed kernel.

Phase 3 is to deprecate lib/systems/examples.nix when nixos-hardware is ready to receive them, we will do it with a proper deprecation notice, etc.

In the meantime, grabbing #239721 is interesting to test the effects of this PR for platforms that supports UEFI.

Expected results after some work:

  • people can enjoy compressed kernels in NixOS on UEFI-enabled platforms.
  • people can build kernels with less override shenigans.
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.11 Release Notes (or backporting 23.05 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.

@RaitoBezarius RaitoBezarius requested review from a user, Ericson2314 and alyssais and removed request for alyssais July 21, 2023 19:32
@github-actions github-actions bot added the 6.topic: kernel The Linux kernel label Jul 21, 2023
@RaitoBezarius RaitoBezarius requested a review from roberth July 21, 2023 19:32
@RaitoBezarius RaitoBezarius changed the title linux: expose KBUILD_IMAGE as a property of the kernel linux: expose KBUILD_IMAGE as a property of the kernel Jul 21, 2023
…tall targets

Nowadays, most architectures supports `KBUILD_IMAGE` which is
set by the architecture-specific Makefile or can be overriden by the user
Before that, we were using Makefile "install targets" such as "uinstall" for uImage,
"zinstall" for zImage, etc.

We move them into a "legacy" list which is used by default if `kernelTarget` is null.
For completeness, this may be needed for some legacy software or
to simplify further refactors.

Prefer KBUILD_IMAGE and kernelTarget.
Starting 6.1, it is possible to have generic compression for aarch64 and riscv64
via CONFIG_EFI_ZBOOT which implements apparently its own decompression.

https://lwn.net/Articles/907679/
This is useful to distinguish between SPARC64 and SPARC whatever,
because SPARC64 do support compressed kernels.
Before, we were sending the uncompressed kernel into the dev output.

I'm not sure there is a good reason for that, I am moving it in the debug output.
Asserts that either we have a kernel image target or an non-empty install target.
This should probably use also kernel versions
…nd riscv-multiplatform

To exercise our KBUILD_IMAGE setup.
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Jul 21, 2023
EFI_GENERIC_STUB_INITRD_CMDLINE_LOADER =
whenOlder "6.2" (whenAtLeast "5.8" yes); # initrd kernel parameter for EFI
whenOlder "6.2" (whenAtLeast "5.8" yes); # initrd kernel parameter for EFI
EFI_ZBOOT = whenAtLeast "6.1" yes; # Generic compression support for EFI payloads
Copy link
Member Author

Choose a reason for hiding this comment

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

Will be removed, this is only here for tests.

@RaitoBezarius RaitoBezarius requested a review from arianvp July 21, 2023 19:39
@roberth
Copy link
Member

roberth commented Jul 23, 2023

Doesn't look wrong to me but I haven't done any work on the linux package.
Maybe @lovesegfault could review instead of me.

@roberth roberth requested review from lovesegfault and removed request for roberth July 23, 2023 10:22
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.

Nowadays, most architectures supports KBUILD_IMAGE which is
set by the architecture-specific Makefile or can be overriden by the user

This is not true, a lot of them don't.

Please don't do this.

Our kernel builds got broken on both mips64 and powerpc64 by this:

And they're still broken on PowerPC. f521f46 was merged two days afterwards and makes reverting anything before it massive pain because it was such a huge change.

More "works on aarch64 and x86" yolo-merges to our kernel builder make this worse.

First we need to get back to actually having a working kernel expression for the major architectures. Right now we can't even tell if this breaks them because they aren't building in the first place.

@ghost
Copy link

ghost commented Jul 24, 2023

@ofborg build pkgsCross.powernv.linux

@RaitoBezarius
Copy link
Member Author

RaitoBezarius commented Jul 24, 2023

Nowadays, most architectures supports KBUILD_IMAGE which is
set by the architecture-specific Makefile or can be overriden by the user

This is not true, a lot of them don't.

Please don't do this.

Our kernel builds got broken on both mips64 and powerpc64 by this:

And they're still broken on PowerPC. f521f46 was merged two days afterwards and makes reverting anything before it massive pain because it was such a huge change.

More "works on aarch64 and x86" yolo-merges to our kernel builder make this worse.

First we need to get back to actually having a working kernel expression for the major architectures. Right now we can't even tell if this breaks them because they aren't building in the first place.

To be fair, we discussed this with @alyssais and it made sense to have this (because of what I am saying on the lib/systems stuff later on.)

I'm fine with trying our best to not break other architectures, but we have to get a design goal here and I think it's hard if we cannot communicate actively in development channels… :P.

Let's go back to design board I guess now.

My goal here is to get rid of the "kernel's image install target" property that lives in lib/systems which makes few sense.

Kernel image install target should be a property of the kernel itself, because not all kernels supports: whateverImage.
Also, kernel can be configured to support extra targets, e.g. EFI_ZBOOT can give you compressed kernels on EFI-enabled architectures.

In this situation, KBUILD_IMAGE gives us a reliable for the consumer to provide the desired image. Install targets are still available if you read the diff, albeit, they are explicitly marked as legacy.

Is it sufficient to make them non-legacy and let them co-exist?

isRiscV64 = { cpu = { family = "riscv"; bits = 64; }; };
isRx = { cpu = { family = "rx"; }; };
isSparc = { cpu = { family = "sparc"; }; };
isSparc64 = { cpu = { family = "sparc"; bits = 64; }; };
Copy link

Choose a reason for hiding this comment

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

This is independently useful; could you please split it out in a separate PR? I'll merge it. I'm surprised we don't already have this.

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.

My goal here is to get rid of the "kernel's image install target" property that lives in lib/systems which makes few sense.

Okay, I see where you're going with this.

I support moving this out of lib/systems. That's a great idea. 👍

I don't think we'll ever get rid of it entirely (your PR moves it into a big if-then-else block in manual-config.nix). I'm very much opposed to pretending we can wish it away without upstream's cooperation, because that's just another way of saying "we're gonna break everything that isn't the two most popular CPUs". The Linux kernel build system is old and weird and crusty and totally custom -- they don't use autoconf or meson or even cmake; they invented their own thing. They don't seem to accept "should be buildable on every architecture with the exact same sequence of commands" as a design goal, and unless they do I dont think we should assume that's possible.

Next steps

You've done a lot of good work here; can we scope this down to simply "move kernelTarget/installTarget out of lib/systems and into linux/kernel"? I think that's an easily achievable short-term goal -- except for one problem, see next paragraph -- and once that part of the PR is merged there will be far less material left and it will be easier to focus on it and understand the consequences.

The "one problem"

Here's the one problem: #221707 broke the build on a bunch of non-top-two architectures, and some of them still haven't been fixed. So we have no way to know if your PR causes additional breakage. I find this to be a fairly absurd situation TBH. We really can't merge additional refactorings without knowing if they break things or not. I don't really know what to do here; I messaged @alyssais on IRC to ask what she thinks should be done about this and the reply I got was sort of a shrug and her saying she's taking some time off from nixpkgs. So I really don't know what to do about this.

@@ -1,3 +1,12 @@
let
# Map of images representations to legacy Makefile 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
# Map of images representations to legacy Makefile targets.
# Map of images representations to Makefile targets; i.e. run
# `make ${defaultMapImageToTargets imageName}` to get `imageName`

# easy overrides to stdenv.hostPlatform.linux-kernel members
, autoModules ? stdenv.hostPlatform.linux-kernel.autoModules or true
, preferBuiltin ? stdenv.hostPlatform.linux-kernel.preferBuiltin or false
, kernelTarget ? null
Copy link

Choose a reason for hiding this comment

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

Suggested change
, kernelTarget ? null
, kernelTarget ? defaultKernelTarget

(defaultMapImageToTargets.${stdenv.hostPlatform.linux-kernel.target or "vmlinux"})
];

# Default kernel target in the case no install target or kernelTarget is mentioned
Copy link

Choose a reason for hiding this comment

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

Suggested change
# Default kernel target in the case no install target or kernelTarget is mentioned
# Default kernel target in the case no installTarget or kernelTarget is mentioned

Comment on lines +4 to +7
"uImage" = "uinstall";
"zImage" = "zinstall";
"Image.gz" = "zinstall";
"vmlinux" = "install";
Copy link

Choose a reason for hiding this comment

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

Suggested change
"uImage" = "uinstall";
"zImage" = "zinstall";
"Image.gz" = "zinstall";
"vmlinux" = "install";
"uImage" = "uinstall"; # U-Boot wrapped image
"zImage" = "zinstall"; # self-extracting images
"Image.gz" = "zinstall";
"vmlinux" = "install"; # the generic binary image

Comment on lines -335 to +344
ln -s $dev/lib/modules/${modDirVersion}/build/vmlinux $dev
mkdir $debug/
ln -s $dev/lib/modules/${modDirVersion}/build/vmlinux $debug
Copy link

Choose a reason for hiding this comment

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

What's going on here? Seems unrelated. Should this be part of a separate PR?

Comment on lines -427 to +436
outputs = [ "out" "dev" ];
outputs = [ "out" "dev" "debug" ];
Copy link

Choose a reason for hiding this comment

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

What's going on here? Seems unrelated. Should this be part of a separate PR?

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: kernel The Linux kernel 6.topic: lib The Nixpkgs function library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants