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

Staging commits for v540tu/v560tu/ns50/n4x_adl: pr0 patch missing, ME HAP bit, s3, review + unify #1871

Closed
wants to merge 17 commits into from

Conversation

tlaurion
Copy link
Collaborator

@tlaurion tlaurion commented Dec 4, 2024

input for #1846


2024-12-04:

  • review of coreboot configs to HAP disable, s3 preferred, missing bits for PR0, oldconfig saved format
  • unification of ns50/nv4x_adl v560tu v540tu coreboot config and boards configs

If builds and doesn't cause brick, should be base for testing after @mkopec corrections from b78c174 state


Old:

  • Ci : adds v560tu
  • CI: have all boards depending on Dasharo fork built atop x230-hotp-maximized workspace cache (24.02.01 coreboot's crossgc pinned buildstack)
  • modules/coreboot: readd dasharo-coreboot-unreleased patch dir containing pr0 patch, since not in dasharo fork tree pointed by commit

@mkopec :
This shows PR0 patch not applying, see last commit log. CI will fail for same reason building this PR.
Once all pending patches are gone under patches/coreboot-dasahro-unreleased, then modules/coreboot can have that patch dir removed and patches needed between dasahro coreboot release and upstream fixes will land in that directory until dasharo coreboot fork git hash is updated to contain content of patches under that directory.

@tlaurion tlaurion marked this pull request as draft December 4, 2024 14:57
modules/coreboot Outdated Show resolved Hide resolved
@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 4, 2024

Once patch added, can help uniform coreboot configs against n4x_adl to prepare for pr0, and then board configs to add same tooling related changes to apply pr0 prior of kexec call.

@mkopec see https://github.com/linuxboot/heads/compare/70bed5f659be985c31cd348793ae74b04620ddcb..0f339496a715db4a44629854b8d208b7b010ad12

@mkopec
Copy link
Contributor

mkopec commented Dec 4, 2024

@tlaurion here's the pr0 patch Dasharo/coreboot@3fcffce

mkopec and others added 5 commits December 4, 2024 18:11
…ximized to reuse coreboot 24.02.01 utils/crossgcc buildstack build for x230-hotp-maximized to skip rebuilding buildstack for novacustom boards

Signed-off-by: Thierry Laurion <[email protected]>
… (might not show much more gain then if based on x230-hotp-maximized)

Signed-off-by: Thierry Laurion <[email protected]>
…pport for PRR/PR0

remove patches/coreboot-dasharo-unreleased/0002-pr0_chipset_locking-post_skylake.patch

Note: modules/coreboot commented the patchdir, but left exisiting under Heads tree so state no patches added on that fork

Signed-off-by: Thierry Laurion <[email protected]>
@tlaurion tlaurion force-pushed the dasharo-fork_pr0_missing branch from 93100fc to ea22fae Compare December 4, 2024 18:23
…ig with helper, adding some missing PR0 settings

Repro
./docker_repro.sh make BOARD=novacustom-v560tu coreboot.save_in_oldconfig_format_in_place
./docker_repro.sh make BOARD=novacustom-v540tu coreboot.save_in_oldconfig_format_in_place
git status | grep modified | awk -F ":" {'print $2'}| xargs git add

Signed-off-by: Thierry Laurion <[email protected]>
…with helper

Repro:
./docker_repro.sh make BOARD=novacustom-nv4x_adl coreboot.save_in_oldconfig_format_in_place
./docker_repro.sh make BOARD=nitropad-ns50 coreboot.save_in_oldconfig_format_in_place
git add config/coreboot-novacustom-v540tu.config config/coreboot-novacustom-v560tu.config

Important changes there to review @mkopec

Signed-off-by: Thierry Laurion <[email protected]>
…sabling ME, saved in oldconfig

Signed-off-by: Thierry Laurion <[email protected]>
@tlaurion tlaurion force-pushed the dasharo-fork_pr0_missing branch from ea22fae to b78c174 Compare December 4, 2024 18:43
@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 4, 2024

@mkopec see https://github.com/linuxboot/heads/compare/93100fcade2e7de6c21119dc400b3a5709cc41fa..b78c1745f94f479dae262488f0bbc22f027b4ddb and their associated commit messages, most importantly b78c174: no bootsplash, ME Enabled, s3 wasn't preferred and LOCALVERSION fixed (not rebrandeable)

@tlaurion

This comment was marked as off-topic.

Copy link
Collaborator Author

@tlaurion tlaurion left a comment

Choose a reason for hiding this comment

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

To be reviewed, commits stolen/modified/cherry-picked @mkopec :)

# General setup
#
CONFIG_COREBOOT_BUILD=y
CONFIG_LOCALVERSION=""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rebranding related. Maybe Dasharo wants to fixate this so LOCALVERSION is defined across all branding, but otherwise branding related snippets to understand:

Heads branding if not defined:

heads/Makefile

Line 13 in 99157f2

BRAND_NAME ?= Heads

Override by localsite

heads/Makefile

Lines 92 to 97 in 99157f2

# Include site-local/config only if it exists, downstreams can set configs for
# all boards, including overriding values specified by boards. site-local is
# not a part of the upstream distribution but is for downstreams to insert
# customizations at well-defined points, like in coreboot:
# https://doc.coreboot.org/tutorial/managing_local_additions.html
-include $(pwd)/site-local/config

And then if not defined, populated

heads/modules/coreboot

Lines 115 to 119 in 99157f2

# coreboot builds are specialized on a per-target basis.
# The builds are done in a per-target subdirectory
CONFIG_COREBOOT_CONFIG ?= config/coreboot-$(BOARD).config
CONFIG_COREBOOT_LOCALVERSION ?= "$(BRAND_NAME)-$(HEADS_GIT_VERSION)"
CONFIG_COREBOOT_SMBIOS_PRODUCT_NAME ?= $(BOARD)

CONFIG_TSEG_STAGE_CACHE=y
# CONFIG_UPDATE_IMAGE is not set
CONFIG_BOOTSPLASH_IMAGE=y
CONFIG_BOOTSPLASH_FILE="@BRAND_DIR@/bootsplash.jpg"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bootsplash related stuff missing

CONFIG_DRIVERS_INTEL_WIFI=y
CONFIG_IFD_BIN_PATH="3rdparty/dasharo-blobs/novacustom/v5x0tu/descriptor.bin"
CONFIG_ME_BIN_PATH="3rdparty/dasharo-blobs/novacustom/v5x0tu/me.bin"
CONFIG_GBE_BIN_PATH="3rdparty/dasharo-blobs/novacustom/v5x0tu/gbe.bin"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mac address? DE:AD:C0:FF:EE?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a clean NVM from Intel, the default MAC is 88:88:88:88:87:88. It's expected that this region is not flashed during regular firmware deployment or update.

If someone accidentally overwrites or wipes their GbE region, they can recover their mac using https://github.com/dasharo/dcu?tab=readme-ov-file#usage

#
# Dasharo Configuration
#
CONFIG_DASHARO_PREFER_S3_SLEEP=y
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

S3, Yes?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, there's no S3 support on Meteor Lake :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@tlaurion tlaurion Dec 6, 2024

Choose a reason for hiding this comment

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

@mkopec updates on this? That would be the first Heads platform with ME enabled, that was not expected/planned #1801 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

S3 enabled now

# CONFIG_STITCH_ME_BIN is not set
# CONFIG_ME_REGION_ALLOW_CPU_READ_ACCESS is not set
CONFIG_HAVE_INTEL_ME_HAP=y
# CONFIG_INTEL_ME_DISABLED_HECI is not set
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HAP disabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

with ME disabled, S0ix will stop working correctly (CPU won't enter deepest sleepstates and fans will stay on), and there's no S3, so I think it has to stay enabled for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@tlaurion tlaurion Dec 6, 2024

Choose a reason for hiding this comment

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

@mkopec updates on this? That would be the first Heads platform with ME enabled, that was not expected/planned #1801 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what happened here, summoning @macpijan for help

Copy link
Collaborator

@macpijan macpijan Dec 10, 2024

Choose a reason for hiding this comment

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

Indeed, we have supported/tested S0ix only in UEFI firmware, which was the reference here. I think this is now clarified: #1871 (comment)

I hope this thread can be resolved once @mkopec changes these settings

Copy link
Contributor

@mkopec mkopec Dec 10, 2024

Choose a reason for hiding this comment

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

ME disabled, S3 enabled in commit b5fe899

@tlaurion tlaurion changed the title Dasharo fork pr0 missing Staging commits for v540tu/v560tu/ns50/n4x_adl: Dasharo fork pr0 missing, ME HAP bit, s3 etc Dec 5, 2024
@tlaurion tlaurion changed the title Staging commits for v540tu/v560tu/ns50/n4x_adl: Dasharo fork pr0 missing, ME HAP bit, s3 etc Staging commits for v540tu/v560tu/ns50/n4x_adl: pr0 patch missing, ME HAP bit, s3 etc Dec 5, 2024
@tlaurion tlaurion changed the title Staging commits for v540tu/v560tu/ns50/n4x_adl: pr0 patch missing, ME HAP bit, s3 etc Staging commits for v540tu/v560tu/ns50/n4x_adl: pr0 patch missing, ME HAP bit, s3, review + unify Dec 5, 2024
@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 6, 2024

Added comment at #1801 (comment) for ME being now required, wasn't planned/expected @macpijan

@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 9, 2024

@mkopec

Found ref from @marmarek stating s3 works, meaning ME can be disabled with HAP on this Heads Matrix room permalink

@mkopec
Copy link
Contributor

mkopec commented Dec 10, 2024

S3 seems to be working correctly, at least with Qubes, with ~10 cycles so far, and using various wake sources (pwrbtn, lid, keyboard)

If that is sufficient then we may enable it for Heads indeed. There's still a risk that some future ucode / pmc / me / fsp / board revision breaks it but at this point it appears to be working correctly

@tlaurion @macpijan

@macpijan
Copy link
Collaborator

@mkopec Let's proceed with this change. If anything changes in the future, we will retest/explain/inform what broke it and inform that it simply there is no suspend on given board, or it can be optionally enabled by user as @tlaurion has suggested, but that is another thread.

I think it's clear for now for everyone that each new Intel board should come with ME HAP-disabled in first PR. Thanks for raising this @tlaurion

tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 10, 2024
Redoing diffs already proposed under linuxboot#1871 but not taken yet....

Signed-off-by: Thierry Laurion <[email protected]>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 10, 2024
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 10, 2024
…linuxboot#1871 but not yet taken under linuxboot#1846

BOOTSPLASH section missing, as well as ME still enabled...

Signed-off-by: Thierry Laurion <[email protected]>
@tlaurion tlaurion closed this Dec 11, 2024
@tlaurion
Copy link
Collaborator Author

Closing in favor of staging #1876

mkopec pushed a commit to Dasharo/heads that referenced this pull request Dec 18, 2024
Redoing diffs already proposed under linuxboot#1871 but not taken yet....

Signed-off-by: Thierry Laurion <[email protected]>
mkopec pushed a commit to Dasharo/heads that referenced this pull request Dec 18, 2024
mkopec pushed a commit to Dasharo/heads that referenced this pull request Dec 18, 2024
…linuxboot#1871 but not yet taken under linuxboot#1846

BOOTSPLASH section missing, as well as ME still enabled...

Signed-off-by: Thierry Laurion <[email protected]>
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