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

Enable reusing coreboot release toolchains for forks #1462

Merged
merged 13 commits into from
Aug 15, 2023

Conversation

JonathonHall-Purism
Copy link
Collaborator

@JonathonHall-Purism JonathonHall-Purism commented Aug 11, 2023

Forks can indicate what release they are based on to reuse the toolchain build from that release. Forks no longer attempt to share build directories.

'purism' and 'talos_2' are both based on coreboot 4.20.1 but they have different architectures, so there is little to be shared - but future forks for the same architecture can share. No boards on regular releases use 4.20.1 yet, but when updated they can share the toolchain with 4.20.1 forks as well.

Also cleaned out coreboot releases/patches that were unused.


@krystian-hebel If you incorporate this into the work adding the 'nitrokey' fork, you should be able to add it as another 4.19 fork in CircleCI to share the toolchain build.

@tlaurion I don't think there's anything to change in CircleCI at this point since there is no actual toolchain sharing to occur. But if you update the Thinkpads to 4.20.1, then we should mark the purism fork as based on 4.20.1 so it will share the toolchain.


This works by defining each coreboot version as a separate module instead of defining the 'coreboot' module differently based on the configured version. Only the needed module(s) (one or two) will become dependencies of the current build - one for the toolchain and one for the ROM, which are the same if not building from a fork. XGCCPATH is used to direct coreboot toward the release's toolchain.

At the moment no sharing actually occurs - we are building 4.11, 4.19, purism 4.20.1 x86, and talos_2 4.20.1 ppc64, none of which share toolchains. Toolchain sharing was tested and can be enabled in the relevant branches adding more forks.

@JonathonHall-Purism
Copy link
Collaborator Author

talos-2 is failing, will check it out:

make[3]: powerpc64-linux-gnu-gcc: No such file or directory
make[3]: *** [/root/project/build/ppc64/coreboot-talos_2/payloads/external/skiboot/skiboot/Makefile.rules:65: asm/asm-offsets.s] Error 127
make[2]: *** [Makefile:17: /root/project/build/ppc64/coreboot-talos_2/payloads/external/skiboot/build/skiboot.elf] Error 2
make[1]: *** [payloads/external/Makefile.inc:377: payloads/external/skiboot/build/skiboot.elf] Error 2
make[1]: *** Waiting for unfinished jobs....

@JonathonHall-Purism
Copy link
Collaborator Author

It should still check the toolchain version, so in the event something was misconfigured the build should break as intended. (Note to self - check this)

Actually it doesn't check the toolchain - I pointed the purism fork to the 4.19 toolchain and it still built. But the fork commit and toolchain version are right next to each other, so there's not much risk of forgetting to update one.

@JonathonHall-Purism
Copy link
Collaborator Author

JonathonHall-Purism commented Aug 11, 2023

librem_mini and librem_mini_v2 fail due to the certificate for https://people.redhat.com/ having expired yesterday (and I invalidated the package cache) - they fetch ioport from here.

x230-maximized-fhd_edp and x230-hotp-maximized-fhd_edp both have make[1]: *** No rule to make target 'src/mainboard/lenovo/haswell/variants/x230_edp/data.vbt', needed by 'x230-hotp-maximized-fhd_edp/coreboot.pre'. Stop. @tlaurion Know offhand where this is supposed to come from? I'm guessing there is another failure earlier that didn't break the build right away It's introduced by a patch to coreboot-4.19, which wasn't applied, checking it out.

@JonathonHall-Purism JonathonHall-Purism changed the title Reuse coreboot release toolchains for forks WIP: Reuse coreboot release toolchains for forks Aug 11, 2023
@JonathonHall-Purism
Copy link
Collaborator Author

The talos-2 build works for me locally, but fails in CI. Might be due to CROSS= which was originally there for historical reasons, but the skiboot payload does check it.

Running CI with CROSS= over lunch, marked WIP until talos-2 is sorted out.

@tlaurion
Copy link
Collaborator

@JonathonHall-Purism rebase on master! ioport was switched, which is why builds are failing here.

@JonathonHall-Purism
Copy link
Collaborator Author

Yup on it thanks, looks like talos-2 works with CROSS= so I will clean that up too, then this is ready for review.

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 11, 2023

@JonathonHall-Purism why would talos2 not be able to share toolchain as of x86 boards right now if another ppc64 based board was added?

Remove coreboot 4.8.1, 4.13, and 4.17, which were all unused.

Remove extra copies of EXTRA_FLAGS which duplicated the common
definition.  The only difference was
-Wno-error=address-of-packed-member, the warning is now disabled
entirely everywhere with -Wno-address-of-packed-member.

Use separate coreboot_version values for talos_2, nitrokey, and purism,
which gives each a separate build directory.

Move conditional blob definitions out of each coreboot version.

Fix condition for coreboot-blobs - whether a module is a git clone
actually depends on non-empty <module>_repo, not <module>_version==git.
Fix the test so git versions of coreboot can have arbitrary names.

Signed-off-by: Jonathon Hall <[email protected]>
At one time coreboot was built using Heads' musl toolchain, but this
was later reverted.  coreboot builds with its own toolchain again.

CROSS= has no effect on coreboot proper (only exception is PPC64
skiboot payload).  It was added to coreboot by a patch that was deleted
in 8e44853.  COREBOOT_IASL was set to the default, that was only needed
when the toolchain was being overridden to override iasl back to the
coreboot one.

ppc64 still specifies CROSS= since skiboot is unable to find coreboot's
toolchain from XGCCPATH but checks CROSS.  This builds skiboot with the
Heads toolchain as before.

Signed-off-by: Jonathon Hall <[email protected]>
Remove patches for coreboot 4.8.1, 4.13, 4.14, and 4.17, which are no
longer used.

Signed-off-by: Jonathon Hall <[email protected]>
Define a separate module for each coreboot version, so the module used
to build the ROM will optionally be able to reference the toolchain
from a different module.

This will allow coreboot fork builds to use the toolchain from the
corresponding release.

Signed-off-by: Jonathon Hall <[email protected]>
Reuse the toolchain from a coreboot release for fork builds.  Either
the fork or the release can be built first, in either case the
release's toolchain is built at the default location and reused for
later builds.

Signed-off-by: Jonathon Hall <[email protected]>
Use .heads-toolchain to mark that the toolchain was built rather than
.xcompile.  coreboot doesn't generate .xcompile until the build step,
so all modules had to build successfully before we would stop trying to
to rebuild the toolchain.  Build steps should generally produce the
indicated outputs too, which was not occurring here.

Signed-off-by: Jonathon Hall <[email protected]>
The skiboot build fails to find the toolchain when it's not in the
default location.  There is only one ppc64 board anyway, so there's no
point trying to share a toolchain for now.

Signed-off-by: Jonathon Hall <[email protected]>
Default the patch version to empty if the module name already includes
the version.  Fixes application of coreboot patches.

Signed-off-by: Jonathon Hall <[email protected]>
These boards get purism-blobs as a submodule of the purism coreboot
fork.  modules/coreboot used to skip the purism-blobs dependency for
this fork, but the module is not needed at all for these boards.

librem_l1um keeps CONFIG_PURISM_BLOBS=y since it is built from patched
coreboot 4.11.

Signed-off-by: Jonathon Hall <[email protected]>
This was spelled wrong - it's actually '_depends'.  'initrd' isn't a
module any more so the value doesn't make sense, remove it.

Signed-off-by: Jonathon Hall <[email protected]>
Two := assignments were factored out together, the second overwrote the
first.  Fix to +=, and remove the nitrokey assignment since it came
from a branch.

Signed-off-by: Jonathon Hall <[email protected]>
@JonathonHall-Purism
Copy link
Collaborator Author

@JonathonHall-Purism why would talos2 not be able to share toolchain as of x86 boards right now if another ppc64 based board was added?

@tlaurion It could, since the CROSS= variable fixed building skiboot. But since there is only one PPC64 build, it will only make the build a little longer/slower to also prepare coreboot 4.20.1 for PPC64, we are better off just building the toolchain from the talos-2 commit.

If we add another ppc64 board based on the talos-2 fork, then we don't need to do anything, they'll share the toolchain since they use the same coreboot fork.

If we add another ppc64 board based on an upstream release or a different fork, then it would be beneficial to prepare a common toolchain.

@JonathonHall-Purism JonathonHall-Purism changed the title WIP: Reuse coreboot release toolchains for forks Reuse coreboot release toolchains for forks Aug 11, 2023
@JonathonHall-Purism
Copy link
Collaborator Author

@tlaurion Ready for review as long as CI passes. Mini builds should be fixed since it was rebased, x230 eDP was fixed, librem_l1um builds locally (with make 4.2.1), talos-2 worked in CI after adding CROSS=.

The Purism fork of coreboot now prepares and uses a 4.20.1 toolchain in anticipation of other boards using it (though none do yet).

CROSS= is needed for skiboot on PPC64 due to different endianness
relative to coreboot.

The talos_2 fork doesn't share the toolchain because it is the only
_fork_, not board, to be precise.  We could add more boards using that
fork without having to create a shared toolchain, it only matters if we
add another fork or start building boards from the upstream release
too.

Signed-off-by: Jonathon Hall <[email protected]>
Nothing else shares the 4.20.1 toolchain yet, and upcoming forks are
based on older releases.  We'll share it when other boards update to
4.20.1.

Signed-off-by: Jonathon Hall <[email protected]>
@JonathonHall-Purism JonathonHall-Purism changed the title Reuse coreboot release toolchains for forks Enable reusing coreboot release toolchains for forks Aug 11, 2023
@JonathonHall-Purism
Copy link
Collaborator Author

On Friday I triggered another pipeline for this branch to test cache reuse: https://app.circleci.com/pipelines/github/JonathonHall-Purism/heads/88/workflows/e4bd226b-4ed2-4587-ae84-477ab86ba925

Looks like it worked! x230-hotp-maximized reproduced an identical ROM and the toolchains were reused from cache.

@tlaurion
Copy link
Collaborator

Will do a testing PR basing #1417 on top of this just to make sure everything is kosher. @krystian-hebel you have something against reusing coreboot 4.19 buildstack to build nv41/ns50?

@krystian-hebel
Copy link
Contributor

I haven't really worked on nv41/ns50 implementation, but I would say that as long as it builds with 4.19 it should be good. The code seems to be from around that time period so I don't expect any problems.

tlaurion added a commit to tlaurion/heads that referenced this pull request Aug 14, 2023
…reboot module, patch and circleci fix
@tlaurion
Copy link
Collaborator

tlaurion commented Aug 14, 2023

I haven't really worked on nv41/ns50 implementation, but I would say that as long as it builds with 4.19 it should be good. The code seems to be from around that time period so I don't expect any problems.

@krystian-hebel I expect some propblems later on the line; sooner then later, if dasharo is not having an internal util/crossgcc buildstack that is self-maintained to match upstream for coreboot tip used in the fork (I understand that coreboot-sdk is used under docker, where coreboot configs used explicitly say ANY_TOOLCHAIN under coreboot configs, expecting the host buildstack to be the good one and not using in tree crossgcc config as strongly recommended per upstream recommendation otherwise "being on your own").

As of now, Heads will benefit reusing 4.19 buildstack, simply because sandy/ivy/haswell is building from upstream 4.19 as of today. By doing so, thanks to @JonathonHall-Purism ingeniously of this PR, we can rely on a first board building crossgcc to be reused under CI, where crossgcc is taking the most compilation time and where CI is able to pass that workspace down to other build steps which reuses it happily. Next cpu time consumer is building linux, which can also be reused if we have targets in upper workspaces on which we can depend.

But what will happen when Heads bump coreboot version to 4.20.1+? Well, Dasharo/coreboot/clevo_release crossgcc will then be used to build itself, since no other common buildstack will be usable. Not a problem per se, taking for granted that clevo_release includes upstream crossgcc buildstack requirements to build itself. This is expected normally for any coreboot fork and expected to be maintained to build itself. Meaning that a fork's HEAD commit is expected to contain crossgcc changes to have the tip of the branch being able to be built.

I will pinpoint this discussion in PR aimed to merge nv41/ns50 nitropad boards to keep track of this issue.

tlaurion added a commit to tlaurion/heads that referenced this pull request Aug 15, 2023
tlaurion added a commit to tlaurion/heads that referenced this pull request Aug 15, 2023
…d on top of linuxboot#1417 and linuxboot#1462

PoC: reuse 4.19 buildstack to make clear if there is a gain or not, since nv41/ns50 are based on x230-hotp-maximized which wil be cached after clean build
@tlaurion
Copy link
Collaborator

So as of now, the "time" cost of passing buildstack on CircleCI is really similar to the cost of building (cpu=time) nv41's own buildstack. There is only a difference of one minute difference and will need to be addressed later on.

One discussed idea would be to have step under CircleCI which would be reponsible to build the different buildstacks we need, and pass it as workspace to all boards which could then be parallelized. It is also thought that it would be possible to do a "combine caches" step prior of save_cache so that the cache contains all build modules caches to be reused if modules don't change.

Otherwise, as of now:

  • A clean build takes around 2h without cache
  • A build/rebuild with available full cache as with today dependencies with CircleCI caching takes 1h22 if we were to reuse 4.19 buildstack to or 1:40 if we use each forks buildstack.

Since we want ot be warned early if forks buildstacks fail, I suggest we use forks buildstacks ofr now and try to implement a better parallelization as described above in parallel in further steps.

I will consequently merge this PR and rebase my branch to be used as a base for #1417.

@tlaurion tlaurion merged commit fbc0993 into linuxboot:master Aug 15, 2023
tlaurion added a commit to tlaurion/heads that referenced this pull request Aug 15, 2023
tlaurion added a commit to tlaurion/heads that referenced this pull request Aug 18, 2023
daringer pushed a commit to Nitrokey/heads that referenced this pull request Aug 30, 2023
daringer pushed a commit to Nitrokey/heads that referenced this pull request Sep 5, 2023
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.

3 participants