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

Add option to build against coreboot 4.12 #721

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

MrChromebox
Copy link
Contributor

@MrChromebox MrChromebox commented May 14, 2020

Update Heads to allow building with coreboot 4.12:

Build/boot tested on Librem 13v2/13v4/15v3/15v4, x230, qemu-coreboot-fbwhiptail

Outstanding issues:

  • Qubes/Xen fails to boot on SKL/KBL Librems (fine on x230 and out of tree WHL-based Librem Mini)
  • Individual boards can be migrated to coreboot 4.12 in subsequent PRs

@snmcmillan
Copy link
Contributor

CBFS size on T420 and X220 is already at absolute maximum (pending PR).

@MrChromebox
Copy link
Contributor Author

@SebastianMcMillan #693 you mean? I'll rebase on top of that once merged

@MrChromebox
Copy link
Contributor Author

@SebastianMcMillan all good now I think

@tlaurion
Copy link
Collaborator

tlaurion commented May 18, 2020

This PR is the followup of #709?

@tlaurion
Copy link
Collaborator

@MrChromebox @SebastianMcMillan : 4.12 is populating exactly the same as here?

Will try to test in the next days on x230.

@MrChromebox
Copy link
Contributor Author

This PR is the followup of #721

bit of a circular reference :) Assume you meant a different PR?

@tlaurion
Copy link
Collaborator

tlaurion commented May 19, 2020

@MrChromebox corrected past reference. Questioning it now.

Well I do not know if it was expected to work but from my results, neither of the following are populated PCRs supposedly measured by Heads:

0: Boot block
1: ROM stage
3: Heads Linux kernel and initrd
4: Boot mode (0 during /init, then recovery or normal-boot)
5: Heads Linux kernel modules

While PCRs 0-3 are supposed to be filled by coreboot and do not represent what Heads is supposed to measure by its coreboot patch:

0: Heads: Boot block. Coreboot: Google vboot GBB flags.
1: Heads: ROM stage. Coreboot: Google vboot GBB HWID
2: Heads: RAM stage. Coreboot: Core Root of Trust for Measurement which includes all stages, data and blobs.
3: Heads: Linux kernel and initrd.  Coreboot: Runtime data like hwinfo.hex or MRC cache.

While PCRs 6-7 of Heads, not conflicting by coreboot, are still measured here per screenshot:

6: Drive LUKS headers
7: Heads user-specific config files

Heads measurements doc
Coreboot measurement docs

@MrChromebox only done functional testing here, setting a default boot option and mystyping disk unlock key passphrase to have pcrs called from /etc/functions. I do not know a functional working testing path outside of this since going to the recovery invalidates the measurements by extending them, while going forward just trust measurements to be valid. This is not the case here.

Was it thought to be functional and a simple repackaging of #709?
@PatrickRudolph?

Screenshot:
signal-attachment-2020-05-18-223758

@tlaurion
Copy link
Collaborator

tlaurion commented May 19, 2020

@MrChromebox :
I see patches removed to coreboot but none added from #709, so no measured boot because needed patch not in, or core logic of heads needing to change? @PatrickRudolph?

@MrChromebox
Copy link
Contributor Author

@tlaurion I don't see any coreboot-related patches in #709, with the exception of the SMM patch, which is already merged in coreboot 4.12.

And with 4.12, measured boot is decoupled from vboot, so vboot isn't necessary (though it would certainly make sense to rebase #709 to add vboot in addition to measured boot).

@PatrickRudolph
Copy link
Contributor

@MrChromebox corrected past reference. Questining it now.

Well I do not know if it was expected to work but from my results, neither of the following are populated PCRs:

0: Boot block
1: ROM stage
3: Heads Linux kernel and initrd
4: Boot mode (0 during /init, then recovery or normal-boot)
5: Heads Linux kernel modules

Ref

@MrChromebox only done functional testing here, setting a default boot option and mystyping disk unlock key passphrase to have pcrs called from /etc/functions. I do not know a functional working testing path outside of this since going to the recovery invalidates the measurements by extending them, while going forward just trust measurements to be valid. This is not the case here.

Was it thought to be functional and a simple repackaging of #709?
@PatrickRudolph?

Screenshot:
signal-attachment-2020-05-18-223758

The PCRs are extended like it's documented here: https://doc.coreboot.org/security/vboot/measured_boot.html
That's not how heads extens the PCRs, so you need another patch on top of 4.12 to keep the current behaviour.

@tlaurion
Copy link
Collaborator

@MrChromebox : i'm not sure everybody will agree here, but since patches are applied to versions and commit ids now, I do not think removing patches is a good idea.

coreboot 4.8.1 patches could stay in repo forever.
coreboot 4.11 patches, 4.12 patches...

We could force boards to use specific versions of coreboot (kgpe-d16) instead of deleting boards.

@PatrickRudolph @MrChromebox : I'm not sure what are the next steps here.
@MrChromebox : 4.11 patches for vboot+measured boot are here

@MrChromebox
Copy link
Contributor Author

@tlaurion are you suggesting we keep older versions of modules and patches in tree and buildable? that's a nightmare to maintain w/r/t build tools etc. If anything, branch off for older devices and they can be maintained by interested parties, but let's not carry everything forward forever

The coreboot 4.11+vboot PR has the same issue with PCRs being used differently by coreboot.

I'll take a look at coreboot 4.11+'s measured boot and see if I can extend the PCRs to match the previous implementation, but won't have time to do that until next week

@tlaurion
Copy link
Collaborator

tlaurion commented May 24, 2020

@MrChromebox I was talking about a way to say:

  • Coreboot version in board config (4.8.1, 4.11, 4.12)
  • keeping multiple coreboot modules with version appended (module/coreboot-4.8.1)
  • keeping multiple coreboot config consequentially
  • keeping multiple coreboot specific patch directories/files

This way, someone wanting to build kgpe-d16 could still make BOARD=kgpe-d16 in CI when #571 goes forward without having to branch. Heads would continue to support boards, while if support is brought back by patches or newer coreboot version, boards could simply specify newer coreboot version.

Limitation:

  • Patches are applied at module package decompression. So adding a new patch requires to delete build/module_ver else patch is not applied.

The outcome of this would be to be able to produce reproducible board roms as artifacts. As of today, the x230-hotp-board is build from CircleCI (debian) and GitlabCI (Fedora).

@MrChromebox
Copy link
Contributor Author

@tlaurion but it's not just keeping coreboot 4.8.1 and patches, it's maintaining the toolchain, and likely a fixed version of flashrom and the kernel as well. If we decide we want to fix those for all boards, then there's an exponential increase in work to maintain, and an inertia to keep boards on older versions if they work there.

I feel like the way coreboot does things with periodic versioned releases, and dropping non-compliant boards after a new release is the only sane way to do things. Tag/branch a release before we move to coreboot 4.12, and let legacy boards live there.

@tlaurion
Copy link
Collaborator

tlaurion commented May 24, 2020

What I propose is to keep/modify:

Limitations:

  • Patches are applied at module's package decompression only. So adding new patches would require CI's cache clearance and/or manual build/coreboot-* removal if the patches list is different.
  • This approach would permit old coreboot versions to be maintained with backported patches for older boards when required.

Effect:

  • kgpe-d16 board and other boards that will be dropped by coreboot could still be built by CIs, while other boards still maintained would require board version switch upon being tested functional to change versioning to newer version.
    • I do not think other modules require the same principle, while the implementated logic for coreboot could probably be applied to linux module, so the danger of breaking a board would be limited, while augmenting a tiny bit the maintainership needed per contributors to test module upgrades. The good side affect of doing this is that boards that are working now would continue to work, while maintained boards could benefit from new features and new modules.

@MrChromebox @SebastianMcMillan @merge : thoughts?

@MrChromebox
Copy link
Contributor Author

my main concern is with the toolchain, and changes to it now affecting 2 (or more) versions of coreboot

@scholzri
Copy link

Hi. Compiling of this completes succesfull, but flashing the rom gives me no video output on my x230 with modded fhd display even if I swap the data.vbt and gma-mainboard.ads in the coreboot files with their patched equivalents. It used to work with the merge:coreboot_next patch. Any ideas what could cause this? How can I get this to work on the x230 with fhd mod?

@MrChromebox
Copy link
Contributor Author

@AdmerStroh does coreboot 4.12 + SeaBIOS/Tianocore work properly with the FHD modifications? I don't know what changes might have affected the FHD mod between 4.11 and 4.12, but I'm guessing something on the libgfxinit side. What's the modded gma-mainboard file look like?

@scholzri
Copy link

scholzri commented Jun 1, 2020

@MrChromebox It does (the latest version of Skulls works just fine without any additional modifications). The modded gma-mainboard file has just LVDS output removed (see https://review.coreboot.org/c/coreboot/+/28950/10/src/mainboard/lenovo/x230/variants/x230_fhd/gma-mainboard.ads). It is strange regarding that stock coreboot 4.12 works just fine.. Maybe someone with a fhd-modded x230 could test if it works for them?..

@MrChromebox
Copy link
Contributor Author

@AdmerStroh is the screen backlight coming on? the default config for the x230 w/coreboot 4.12 under Heads should leave libgfxinit as the default display init. Unfortunately I only have a standard x230 here to test, not a FHD one

@scholzri
Copy link

scholzri commented Jun 1, 2020

@MrChromebox The backlight is coming on and can be dimmed via press on the power button. I will try to force output over the mini-dp port and get something on an external monitor. I will report the results.

@scholzri
Copy link

scholzri commented Jun 2, 2020

@MrChromebox unfortunately, even with only HDMI1 in gma-mainboard file enabled, no video output over external monitor (mini-dp). Maybe I made a fault merging your patch? For now, I will just use skulls or revert back to 4.11. Maybe someone more experienced than me with a x230 fhd mod could try and share their experience. Nonetheless thank you!

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 2, 2020

@MrChromebox unfortunately, even with only HDMI1 in gma-mainboard file enabled, no video output over external monitor (mini-dp). Maybe I made a fault merging your patch? For now, I will just use skulls or revert back to 4.11. Maybe someone more experienced than me with a x230 fhd mod could try and share their experience. Nonetheless thank you!

@AdmerStroh : my intuition here may be how you cleaned me.
Have you followed https://github.com/osresearch/heads-wiki/blob/master/Clean-the-ME-firmware.md ?

Edit: maybe not, since this seems valid even if ME wasn't cleaned completely: https://github.com/osresearch/heads/pull/721/files#diff-2d4ed2860b55bb308eacc61e6c18a86fR5

@scholzri
Copy link

scholzri commented Jun 2, 2020

@tlaurion I used the skulls script with the me-cleaning option on the bottom spi chip. Will it be reinstalled when I internally flash the full 12mb rom image? By the way, compiling and externally flashing the 4MB x230-flash rom with this patch works and I get a console output

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 2, 2020

@AdmerStroh @MrChromebox this is why I'm wondering where your absence of console comes from on full rom image (230's coreboot.rom) since x230-flash.rom image gives console output.

@scholzri
Copy link

scholzri commented Jun 2, 2020

@tlaurion @MrChromebox Ok. I could succesfully resolve my issue by applying this patch #703 and flashing both the top and the bottom image externally. So probably tlaurion was right with the me not properly cleaned in the bottom chip.. Thank you very much for the help :)

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 2, 2020

@tlaurion @MrChromebox Ok. I could succesfully resolve my issue by applying this patch #703 and flashing both the top and the bottom image externally. So probably tlaurion was right with the me not properly cleaned in the bottom chip.. Thank you very much for the help :)

@AdmerStroh : please make sure you read the big fat warning I just added here

If you intend to flash through x230-flash image, you are not supposed to play with #703 at all.

#703 is WiP and is not ready, hence why it is not merged in master.
#703 results in a board that flashes the whole 12 SPI, considering that ME was fully neutered and all space was freed; images internally flashed are wholesome. This is not the case with x230 images, that leaves IFD unfreed space unused and only flash the BIOS region of SPI...

If you come from x230-flash or Skulls, you need to apply x230 image from within x230-flash, and stay with x230 board upgrades.

Images built from master can be downloaded from artifacts here and here.

Note that as you can see, the builds are not yet reproducible since this issue is not yet resolved

@tlaurion
Copy link
Collaborator

28d3b7c being merged, #721 #709 are next.
@MrChromebox @PatrickRudolph

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 12, 2020

@MrChromebox ok. So logic is inverted and fallsback to 4.12 default for boards not specifying other version.

Testing with x230-htop-verification board removing coreboot 4.8.1 version here: https://app.circleci.com/pipelines/github/tlaurion/heads/292/workflows/8cb5121b-dc5d-4099-8aed-ad80cd1cdea0/jobs/317

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 12, 2020

@MrChromebox : not specifying the coreboot version under test board x230-hotp-verification resulted in this error:

make[1]: Entering directory '/root/project'
modules/coreboot:10: *** "x230-hotp-verification: does not specify coreboot version under CONFIG_COREBOOT_VERSION".  Stop.
make[1]: Leaving directory '/root/project'
make: *** [Makefile:587: all] Error 2

Src: https://app.circleci.com/pipelines/github/tlaurion/heads/292/workflows/8cb5121b-dc5d-4099-8aed-ad80cd1cdea0/jobs/317/parallel-runs/0/steps/0-123

Doing manual tests locally.
Edit: Misunderstood that 4.12 needed to be specified in board configs. Doing.

@MrChromebox
Copy link
Contributor Author

@tlaurion yeah was about to say, the behavior w/r/t specifying the coreboot version is unchanged. No logic inverted, no fallback

@MrChromebox
Copy link
Contributor Author

@tlaurion I don't see why you need to patch anything outside of the board/coreboot configs, unless your fork isn't up to date. The 'libremkey-hotp-verification' module doesn't exist anymore so that tells me it isn't.

Any reason the Librem boards (which need to stay on 4.8.1 for now) aren't being built (without blobs) as part of CI testing?

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 12, 2020

@PatrickRudolph : unfortunately, the x230-hotp-verification build here (and I suspect all other xx30 boards) still suffer from #770. As said in other tickets, that regression came out after changing the buildchain from musl-cross to musl-cross-make, since https://gitlab.com/tlaurion/heads/-/jobs/312958349/artifacts/raw/build/x230-libremkey/coreboot.rom (same coreboot 4.8.1, same QubesOS) is able to resume from suspend. Would need help here.

Interestingly enough, #765 is gone. Any idea? Rest is feature pair in terms of measurements and basic testing works as a direct replacement.

Personally, as for @jans23 and the general community, #770 is a show stopper.

@MrChromebox
Copy link
Contributor Author

also, even if issues migrating boards to 4.12, we should still go ahead and merge this, since no impact to existing boards and it will allow adding of other new boards (like the Librem Mini) which require 4.12

@PatrickRudolph
Copy link
Contributor

I don't see why #770 would block this PR.

@tlaurion
Copy link
Collaborator

@tlaurion I don't see why you need to patch anything outside of the board/coreboot configs, unless your fork isn't up to date. The 'libremkey-hotp-verification' module doesn't exist anymore so that tells me it isn't.

@MrChromebox rebased my branch over osresearch/master just to make sure. Here is comparison over this branch
https://github.com/MrChromebox/heads/compare/coreboot-4.12...tlaurion:x230-hotp-verification_coreboot412?expand=1

Any reason the Librem boards (which need to stay on 4.8.1 for now) aren't being built (without blobs) as part of CI testing?

Just because I didn't added them yet and thought that blobs needed to be present as specified into blobs dir and extracted from full roms, just like the xx20 boards. With #798, adding those boards into CIs configs will not add much build time. Want to do provide a PR? Please put boards in blocks with similarities of coreboot+linux near of each other in CircleCI and GitlabCI configs.

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 13, 2020

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 13, 2020

@MrChromebox

Any reason the Librem boards (which need to stay on 4.8.1 for now) aren't being built (without blobs) as part of CI testing?

But the build will obviously fail because blobs are not provided in tree:
https://github.com/osresearch/heads/blob/master/config/coreboot-librem13v2.config#L13-L23
https://github.com/osresearch/heads/blob/master/config/coreboot-librem13v4.config#L13-L23
https://github.com/osresearch/heads/blob/master/config/coreboot-librem15v3.config#L13-L23
https://github.com/osresearch/heads/blob/master/config/coreboot-librem15v4.config#L13-L23

So I will remove those boards in next commit once that point is proven with current board+coreboot configs, which is why the x220 and t420 (which have blobs/* corresponding directories) are not built from CIs since not blob free as of now.
https://github.com/osresearch/heads/blob/master/config/coreboot-x220.config#L13-L18
https://github.com/osresearch/heads/blob/master/config/coreboot-t420.config#L13-L18

You can join forces #307, that ticket would feel less lonely.

@MrChromebox
Copy link
Contributor Author

MrChromebox commented Aug 13, 2020

@tlaurion comparing branches isn't helpful when commits get merged out of order (changing the hashes) and there is no unique ID associated with a given commit. We should think about adding one as part of a commit hook like coreboot does.

I still maintain that this PR is completely sufficient as-is. It touches a single file and only adds the option to build against coreboot 4.12. Building boards using it and the changes needed there should be handed in separate PRs and not affect this one.

edit: I've rebased this patch on master to avoid any confusion, though it shouldn't have been required

Add version and hash for coreboot and coreboot-blobs modules.
Adjust to use own toolchain, fix blobs path and extraction depth.

Test: build Librem 13v4 using both coreboot 4.8.1 and coreboot 4.12
(after adjusting board defconfig), verify correct toolchains used to
build each, and that teh result is a bootable ROM.

Signed-off-by: Matt DeVillier <matt.devillier@puri.sm>
@tlaurion
Copy link
Collaborator

tlaurion commented Aug 13, 2020

@MrChromebox agreed. This needs to be changed and it builds for all current boards.

I'll do a seperate PR for CI changes and coreboot CBFS change needed for x230 to build this was simply to provide roms for people to test prior of merging.

@MrChromebox
Copy link
Contributor Author

@MrChromebox agreed. This needs to be changed and it builds for all current boards.

well, as part of migrating boards to 4.12, we should update the defconfigs as well -- increasing the CBFS size as needed, dropping config items that make no sense, etc

tlaurion added a commit to tlaurion/heads that referenced this pull request Aug 13, 2020
…ble boards (x230 and qemu-coreboot boards CBFS regions expended)
tlaurion added a commit to tlaurion/heads that referenced this pull request Aug 13, 2020
@tlaurion tlaurion merged commit 9eff5c5 into linuxboot:master Aug 17, 2020
Thrilleratplay pushed a commit to Thrilleratplay/heads that referenced this pull request Oct 15, 2020
…ble boards (x230 and qemu-coreboot boards CBFS regions expended)
Thrilleratplay pushed a commit to Thrilleratplay/heads that referenced this pull request Oct 15, 2020
Thrilleratplay pushed a commit to Thrilleratplay/heads that referenced this pull request Oct 15, 2020
…ble boards (x230 and qemu-coreboot boards CBFS regions expended)
Thrilleratplay pushed a commit to Thrilleratplay/heads that referenced this pull request Oct 15, 2020
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.

7 participants