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

Fix linuxboot builds #845

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

NHellFire
Copy link
Contributor

@NHellFire NHellFire commented Sep 26, 2020

Tested builds of qemu-linuxboot and qemu-coreboot, both succeeded with gcc 7. With gcc 8+, linuxboot will fail on edk2 (linuxboot/linuxboot#43).

@tlaurion
Copy link
Collaborator

@NHellFire Ci fails because #842 not being merged. This will need to be rebased ton top of master once merged in.

Copy link
Contributor

@MrChromebox MrChromebox left a comment

Choose a reason for hiding this comment

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

wait, why is coreboot being included as a module for a Linuxboot build anyway? Seems like that's the issue that needs to be fixed

@NHellFire
Copy link
Contributor Author

wait, why is coreboot being included as a module for a Linuxboot build anyway? Seems like that's the issue that needs to be fixed

Top Makefile includes all modules (00559de, 1c64e4c). It was fine before the version check was added as it only contained variables.

@tlaurion
Copy link
Collaborator

@MrChromebox I like the fix. Rebasing on master with linixbootboarf added in CI should show the fix working.

@tlaurion
Copy link
Collaborator

@NHellFire can you add the board in CircleCI and rebase on master?

…linuxboot#841)

Signed-off-by: Nathan Rennie-Waldock <nathan.renniewaldock@gmail.com>
Signed-off-by: Nathan Rennie-Waldock <nathan.renniewaldock@Gmail.com>
@NHellFire
Copy link
Contributor Author

@NHellFire can you add the board in CircleCI and rebase on master?

I've rebased (and fixed showing the error).
Currently, readding linuxboot back to CI will just fail as Debian 10 uses gcc 8.3 (linuxboot/edk2#1).

@tlaurion
Copy link
Collaborator

tlaurion commented Sep 30, 2020

@NHellFire So if I understand well, this will work once linuxboot has your PR merged upstream, while linuxboot will not offer reproducibility since not bound to particular commit ID as other modules. Actually, it seems that linuxboot is the only module still not being bound to a commit id


EDIT: confirmed:

user@x230-master:~/heads$ grep "_version := git" modules/*
modules/coreboot:#coreboot_version := git
modules/flashrom:#flashrom_version := git
modules/linuxboot:linuxboot_version := git
modules/msrtools:#msrtools_version := git
modules/pciutils:#pciutils_version := git
modules/tpmtotp:#tpmtotp_version := git

Consequence, it will be impossible to reproduce Head's linuxboot based ROM based on a signle Head's commit ID in the future. Might need another issue and PR, or added to this PR, since linuxboot cannot be build for more then a month now (coreboot 4.8.1 version specification inside of coreboot module), and CircleCI's builds are not built since a while.

It seems that linuxboot builds are not often built for the following platforms:

Leopard OCP node: @osresearch others?
Winterfell OCP node: @osresearch others?
Intel S2600wf : @osresearch others?
r630: @osresearch others?

Signed-off-by: Nathan Rennie-Waldock <nathan.renniewaldock@gmail.com>
@NHellFire NHellFire changed the title modules/coreboot: Only do coreboot version check if we're building boot (fixes #841) Fix linuxboot builds Oct 1, 2020
@NHellFire
Copy link
Contributor Author

NHellFire commented Oct 1, 2020

CI should pass now. Local build passed with gcc 9.3.0 and binutils 2.34 (Ubuntu 20.04.1).

@tlaurion
Copy link
Collaborator

tlaurion commented Oct 2, 2020

@NHellFire not sure about the hack implemented in the linuxboot module to apply patches outside of the root Makefile logic.
Why are those patch not directly applied into linuxboot upstream, or for a specific linuxboot git commit as other modules?

if we look at musl-cross-make module, being dependend on a specific commit for reproducible builds in the future on a specific Heads commit being rebuild, the following module specifies what to download as opposed to downloading master which cannot be replicated in the future if master changes.

As a result for that specific musl-cross-make git version, no patch are currently applied (no patch exists for that specific module-hash), since no patches relative to that specific commit id exits (Duh. there is a past artifact for musl-cross lying around, will fix that).

Does that make sense?

In the present case, linuxboot should be fixed to commit id linuxboot/linuxboot@10be0dc into module, and your linuxboot-ed2k patches should be applyable directly inside of the decompressed archive for that specific commit ID, where the patch would be in a file patches/linuxboot-10be0dc2652e1fc8c11d419ca2c4ff93920d4165.patch or in seperate files under patches/linuxboot-https://github.com/linuxboot-10be0dc2652e1fc8c11d419ca2c4ff93920d4165/1.patch etc following root Makefile patch application rule.

@tlaurion
Copy link
Collaborator

@NHellFire ?

@NHellFire
Copy link
Contributor Author

NHellFire commented Oct 17, 2020 via email

@tlaurion
Copy link
Collaborator

@NHellFire please tag me for followups

> Running edk2 build for OvmfPkgX64
> Usage: build.exe [options] [all|fds|genc|genmake|clean|cleanall|cleanlib|modules|libraries|run]
>
> build.exe: error: option -n: invalid integer value: 'PUS'

Signed-off-by: Nathan Rennie-Waldock <nathan.renniewaldock@gmail.com>
* add linuxboot-edk2 module (if not building latest git)
* patch linuxboot-edk2 Makefile to not conflict with ours
* update linuxboot build dir in circleci and boards/winterfell

Signed-off-by: Nathan Rennie-Waldock <nathan.renniewaldock@gmail.com>
…gcc 8

Signed-off-by: Nathan Rennie-Waldock <nathan.renniewaldock@gmail.com>
@NHellFire
Copy link
Contributor Author

@tlaurion Can you check the latest revision?
Thanks

@@ -52,7 +52,7 @@ dxe_offset := 860000
dxe_size := 6a0000
flash-dxe: $(build)/$(BOARD)/linuxboot.rom
( echo u$(dxe_offset) $(dxe_size) ; \
pv $(build)/linuxboot-git/build/$(BOARD)/dxe.vol \
pv $(build)/linuxboot-b5376a441e8e85cbf722e943bb8294958e87c784/build/$(BOARD)/dxe.vol \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I get what you are doing here.
You can get the build directory by reusing Makefile included module.

In this case: $(linuxboot_base_dir)

command: |
make \
BOARD=qemu-linuxboot \
`/bin/pwd`/build/linuxboot-b5376a441e8e85cbf722e943bb8294958e87c784/build/qemu/.configured \
Copy link
Collaborator

Choose a reason for hiding this comment

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

That will break in future linuxboot version bump. Why static?
$(linuxboot_base_dir)/build/qemu/.configured ?

@@ -1,5 +1,6 @@
modules-$(CONFIG_COREBOOT) += coreboot

ifeq "$(CONFIG_COREBOOT)" "y"
ifeq "$(CONFIG_COREBOOT_VERSION)" "4.8.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation should shift


linuxboot-edk2_tar := linuxboot-edk2-$(linuxboot-edk2_version).tar.gz
linuxboot-edk2_url := https://github.com/linuxboot/edk2/archive/$(linuxboot-edk2_version).tar.gz
linuxboot-edk2_dir := $(linuxboot_base_dir)/edk2
Copy link
Collaborator

Choose a reason for hiding this comment

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

so that is the env variable to reuse $linuxboot-edk2_dir

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 4, 2020

@NHellFire I see that requests for change in this PR are still valid?

@@ -21,7 +27,7 @@ linuxboot_configure := \
if [ "$(linuxboot_board)" = "qemu" ]; then \
echo >&2 "Pre-building edk2 OVMF" ; \
( cd $(build)/$(linuxboot_base_dir)/edk2/OvmfPkg ; \
./build.sh -n $$CPUS \
./build.sh -n $(CPUS) \
Copy link
Contributor

@synackd synackd Dec 23, 2020

Choose a reason for hiding this comment

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

@tlaurion What do you say to making this change a separate PR and merging it in? I was about to make one addressing this before seeing this change in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@synackd please do. Same should be made under coreboot module

Copy link
Contributor

Choose a reason for hiding this comment

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

In hindsight, I probably could have combined #940 and #941 into one PR, though it does categorically separate the linuxboot and coreboot modules...

Copy link
Collaborator

@tlaurion tlaurion Dec 27, 2020

Choose a reason for hiding this comment

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

Have you tested it? The idea here is to pass CPUS to command line
make BOARD= x230 CPUS=2

There is no such invocation as CPUS as a command.
@synackd?
EDIT: nevermind! Tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

These changes can be retracted since they are already merged as of #940.

@tlaurion
Copy link
Collaborator

@NHellFire @synackd ping into cleaning this PR?

@tlaurion
Copy link
Collaborator

@NHellFire not sure about the hack implemented in the linuxboot module to apply patches outside of the root Makefile logic.
Why are those patch not directly applied into linuxboot upstream, or for a specific linuxboot git commit as other modules?

@NHellFire patches are under patches/module-version and applied automatically, if modules respect module statements.
I cannot merge modules pinning to specific version. See requested changes above.

@tlaurion
Copy link
Collaborator

Linked with #971 (comment)

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.

4 participants