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 NovaCustom V560TU board #1846

Merged
merged 37 commits into from
Jan 15, 2025
Merged

Conversation

mkopec
Copy link
Contributor

@mkopec mkopec commented Nov 14, 2024

Add NovaCustom V560TU (16") laptop.

This machinee is based on the Meteor Lake platform and has integrated graphics. The dGPU variant nor the 14" variant is not supported at this point.

coreboot-dasharo revision is bumped for V560TU support and support for PR0 lockdown by Heads. The base coreboot release is 24.02.1.

Since the NV4x and NS5x boards use the same coreboot rev, their defconfigs are refreshed, too.

@macpijan
Copy link
Contributor

@mkopec Do we plan to add here the rest of the variants as well?

@mkopec
Copy link
Contributor Author

mkopec commented Nov 14, 2024

@macpijan sure, after one starts working I'll do the rest too

@tlaurion
Copy link
Collaborator

@macpijan @mkopec similar commit needed for #1835

@tlaurion
Copy link
Collaborator

@mkopec this comment might be helpful #1835 (comment)

@tlaurion
Copy link
Collaborator

@mkopec

mkc: seems like kernel 6.7+ is bare minimal for meteor lake, with efifb fixes having landed under 6.8+?

Also, if you pass serial to kernel boot options under coreboot config, you should get logs of the kernel?

At https://matrix.to/#/!pAlHOfxQNPXOgFGTmo:matrix.org/$bVtsV3M_sz0MroFsDkXRGQ3ow7OhumKqtAllchkAyKs?via=matrix.org&via=nitro.chat&via=envs.net

@mkopec
Copy link
Contributor Author

mkopec commented Nov 22, 2024

mkc: seems like kernel 6.7+ is bare minimal for meteor lake, with efifb fixes having landed under 6.8+?

possible, in Ubuntu and Fedora we found it best to have Linux 6.8 or newer (6.9 also has a fix for s0ix but that's irrelevant for Heads)

@tlaurion do you think you can help with updating the kernel?

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 22, 2024

I read that 6.12 will be next Lts but that is only speculation.

edit: We choose 6.11.9 for all novacustom boards?
I can give it a shot, yes, and leave traces in commit, taking this PR state as a base.

@mkopec did serial console passed per coreboot config to kernel gave you more info?
With debug as well. See qemu coreboot config, non-prod.

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 22, 2024

@mkopec there is a lot to tune under 6.11.9 kernel config which writes over shared linux config with other novacustom (config/linux-nitrokey-x.config, should probably renamed and all board configs point to new name as well).

See branch https://github.com/tlaurion/heads/tree/dasharo-add_novacustom_v540tu and feel free to steal adapt/change whatever. If you have questions, poke me where/if needed.

Note: compare config changes in commit 510bc6e to see all the new olddefconfig stuff applied silently when using defconfig. Ie, what happened with TRUST_CPU and so many other stuff to care about once you have something that boots.

See also that I changed your coreboot config to pass linux kernel options to be in debug, with probably wrong serial ttyS0, adapt accordingly and keep me posted. Small iterations in board porting has known good track history.

If you decided to tackle #1835 instead, I could help (if you guide me into how you get serial console output [even though this work wasn't planned nor in my plate up to now]).

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 22, 2024

Edit:yep. Cannot bump Sandy to 6.11.9: as can be seen, all those do not have enough free spi space without another phase of #590 (not planned.)

@mkopec
Copy link
Contributor Author

mkopec commented Nov 27, 2024

Thanks for the patches @tlaurion , I'm testing them now, i'm not getting a serial console yet, I suspect the LPSS UART driver is missing, will try to fix

@tlaurion
Copy link
Collaborator

@mkopec #1865 merged, please rebase

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 28, 2024

Thanks for the patches @tlaurion , I'm testing them now, i'm not getting a serial console yet, I suspect the LPSS UART driver is missing, will try to fix

@mkopec This needs updates, since reported off-channel to be all good :)

@mkopec mkopec force-pushed the add_novacustom_v540tu branch from aeb7fc7 to 005c519 Compare November 29, 2024 11:37
@mkopec
Copy link
Contributor Author

mkopec commented Nov 29, 2024

@tlaurion rebased

@mkopec mkopec force-pushed the add_novacustom_v540tu branch from 005c519 to 6964713 Compare November 29, 2024 12:15
@mkopec mkopec marked this pull request as ready for review November 29, 2024 12:16
@tlaurion
Copy link
Collaborator

tlaurion commented Nov 29, 2024

@mkopec awesome!!!


Trying to dodge politic issues from #1818 (comment) and going practical and specific.

@macpijan @mkopec : Can https://review.coreboot.org/c/coreboot/+/85278 be added to Dasharo fork (which nv41/ns50/v540TU/v560TU should build upon (modules/coreboot dasharo pointed commit), tested against #1818 so that we can merge #1818 (which changes configs for nv41) and here changes needed for PR0 for v540TU/V560TU?

Makes sense? Otherwise patches need do be under patches/coreboot-dasahro-unreleased, which I do not want to maintain since we talk of a new coreboot release.

Also, MSI(#1753 should build from same coreboot Dasharo fork commit pointed under modules/coreboot without breaking (otherwise multiple coreboot Dasahro forks will be built under Heads???? Is that the plan because multiple forks for different boards under Dasharo?)

cc @pietrushnic @macpijan

modules/coreboot Outdated
@@ -93,8 +93,7 @@ $(eval $(call coreboot_module,purism,24.02.01))

# MSI and Nitropad NV41 / NS50 boards are based on Dasharo coreboot port
coreboot-dasharo_repo := https://github.com/dasharo/coreboot
coreboot-dasharo_commit_hash := 3a9aa3a4692f3dd49732f5b4e3ec54be385f0969
coreboot-dasharo_patch_version := unreleased
coreboot-dasharo_commit_hash := db1d9cd59d0d7d5b708bbdf8670780914061410c
Copy link
Collaborator

@tlaurion tlaurion Nov 29, 2024

Choose a reason for hiding this comment

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

@mkopec this is shared between MSI/nv41/ns50/v540Tu/v560TU: tested for oldconfig expension/run time regression? #1818 related patch inclusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not regression tested yet, will report here with results

modules/linux Outdated
@@ -34,6 +34,9 @@ linux_hash := 40f014d53e81f204f6d2a364aae4201ae07970dd1b70dc602d7c66c1a140f558
else ifeq "$(CONFIG_LINUX_VERSION)" "6.1.8"
linux_version := 6.1.8
linux_hash := b60bb53ab8ba370a270454b11e93d41af29126fc72bd6ede517673e2e57b816d
else ifeq "$(CONFIG_LINUX_VERSION)" "6.11.9"
Copy link
Collaborator

@tlaurion tlaurion Nov 29, 2024

Choose a reason for hiding this comment

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

Was 6.11.9 verified needed for meteor lake as opposed to 6.1.8 which all other Heads boards depend on after serial console issue being fixed and ACPI related Kconfig addition under linux config added that was the cause of linux hang, maybe not efifb nor 6.1.8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cause of hang was X2APIC not being enabled in kernel config, now with that sorted I think I can try 6.1 again to check if we need 6.11 after all

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any news on this @mkopec ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed 6.11.9 leftovers in 313e38a

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some patches against linux were removed and not readded with adaptation as compared to 6.1.8. If 6.11.9 required for meteor lake, missing patches need to be brought back if still needed

Copy link
Collaborator

@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.

@mkopec Review dropped :)

modules/coreboot Outdated Show resolved Hide resolved
@tlaurion
Copy link
Collaborator

tlaurion commented Nov 29, 2024

@mkopec I decided to not do politics in words, but as code from now on. What is coreboot fork related will be coreboot fork related from now on.

#1818 was merged, if you rebase from/merge master in your PR, you will see that added PR0 patch under patches/coreboot-dasharo_unreleased don't apply as for nv4x_adl on master, where only ns50/n4x_adl were the only Heads master consumers of Dasharo forks, showing problem of having multiple forks for different platforms and delegating problem to whom it belongs, making sure patches/coreboot-dasharo_unreleased patches that were pending next downsteam fork release are picked up/adtapted in next downstream release, and for Heads to take for granted that coreboot base is working as expected for Heads to do its Heads stuff.

I will then participate/help adapting changes to coreboot configs/board configs if needed for Heads under #1868, which points to needed knowledge for Heads for PR0 to work as expected on skylake+, and will be used as base so coreboot upstream patch evolves to be useful for all and prevent OS persistent threat to be pushed to firmware (at least under Heads use case) leaving physical attack of tampering bootblock the only threat model to SPI flash for advanced users activating Heads Authenticated Heads, which I intend to push on next feature freeze (next downstream release).

Support for V54 series is not added at this time.

Signed-off-by: Michał Kopeć <[email protected]>
@mkopec
Copy link
Contributor Author

mkopec commented Jan 15, 2025

Removed V540TU as per #1846 (comment)

@mkopec mkopec changed the title Add NovaCustom V540TU and V560TU boards Add NovaCustom V560TU board Jan 15, 2025
@mkopec
Copy link
Contributor Author

mkopec commented Jan 15, 2025

Reverted changes to NV4x and NS5x in commit: 7445527

It may be reverted at a later date when needed

@tlaurion
Copy link
Collaborator

tlaurion commented Jan 15, 2025

I'm not sur I follow what is happening here, reverting changes for boards that depend of the same coreboot branch, defined under modules/coreboot for ns50/nv41/v540tu and v560tu.

I understand that Dasharo coreboot's fork use branch for each board in their philosophy as opposed to coreboot master, but doing so under Heads explodes build time from CI, where here n540tu is same gen as n560tu, and where nv41 was tested, leaving ns50 to be the only one needing regression test for current PR, and branch, named v540tu initially. I'm getting confused,was there a regression justifying last commits?

If the desire is to have one dasharo branch per board as per dasharo git tree, then modules/coreboot will need to be adapted in such manner, also meaning one patches/branch patches to track and way more regression testing and changes in the future.

Can someone explain the reasoning of this seperarion and revert of past testing and changes of this PR to solely touch v560tu prior of going forward that path?

CC @BeataZdunczyk @macpijan @mkopec

Edit: see comments for dasharo's branch applied patches application:

Edit: cache reusal of coreboot version's buildstack per CI (needs reordering, since not reusing 24.02.01 coreboot buildstack):

@tlaurion
Copy link
Collaborator

Can someone explain the reasoning of this seperarion and revert of past testing and changes of this PR to solely touch v560tu prior of going forward that path?

CC @BeataZdunczyk @macpijan @mkopec

Edit: see comments for dasharo's branch applied patches application:

* [ ]  [7445527#r151372370](https://github.com/linuxboot/heads/commit/74455278ab21858e58530e694ab42ef47446c5e0#r151372370)

* [ ]  [7445527#r151372430](https://github.com/linuxboot/heads/commit/74455278ab21858e58530e694ab42ef47446c5e0#r151372430)

Edit: cache reusal of coreboot version's buildstack per CI (needs reordering, since not reusing 24.02.01 coreboot buildstack):

* [ ]  [7445527#r151372770](https://github.com/linuxboot/heads/commit/74455278ab21858e58530e694ab42ef47446c5e0#r151372770)

CC @macpijan @mkopec @BeataZdunczyk

@macpijan
Copy link
Contributor

macpijan commented Jan 15, 2025

@tlaurion

I understand that Dasharo coreboot's fork use branch for each board in their philosophy as opposed to coreboot master,

It is really not, we are doing our best to have all the supported boards in the main branch in coreboot (named: dasharo).

Untangling this. We propose to modify this MR in a following way:

  1. We will keep NV41 and NS50 changes here - using the very same commit for all of them. That should resolve the patches/branches/cache concerns of yours.
  2. We will drop V54 from this MR right now - since NovaCustom is not interested in releasing this unit with heads right now. Since there will be no users right now, it seems like a waste of time for it to be a blocker here. We will come back with these patches once NovaCustom decides to release.

IIUC, at this state, the missing step for merging, would be regression of NS50. I have asked @daringer whether NK can help here.

Once this MR is merged, we would sync heads Dasharo fork and make a release for V56. Sounds ok?

@tlaurion
Copy link
Collaborator

@jans23 @nestire @daringer : Will the ns50 be tested for regression in time or should ns50 be removed from Heads master?

@tlaurion
Copy link
Collaborator

tlaurion commented Jan 15, 2025

@tlaurion

I understand that Dasharo coreboot's fork use branch for each board in their philosophy as opposed to coreboot master,

It is really not, we are doing our best to have all the supported boards in the main branch in coreboot (named: dasharo).

Untangling this. We propose to modify this MR in a following way:

1. We will keep NV41 and NS50 changes here - using the very same commit for all of them. That should resolve the patches/branches/cache concerns of yours.

2. We will drop V54 from this MR right now - since NovaCustom is not interested in releasing this unit with heads right now. Since there will be no users right now, it seems like a waste of time for it to be a blocker here. We will come back with these patches once NovaCustom decides to release.

IIUC, at this state, the missing step for merging, would be regression of NS50. I have asked @daringer whether NK can help here.

Once this MR is merged, we would sync heads Dasharo fork and make a release for V56. Sounds ok?

No problem with this plan. I'm still waiting for feedback on #1875. Release (at least automated QA), should be #1846 + #1875.

@mkopec mkopec force-pushed the add_novacustom_v540tu branch from 313e38a to b59c0e2 Compare January 15, 2025 16:42
@mkopec
Copy link
Contributor Author

mkopec commented Jan 15, 2025

nv4x and ns5x restored, moved back to using a single coreboot revision for all clevo boards

@tlaurion
Copy link
Collaborator

nv4x and ns5x restored, moved back to using a single coreboot revision for all clevo boards

Alright, https://github.com/linuxboot/heads/compare/a80d6da99ba15fa9d1462e084c9065b4ec3c8e9f..b59c0e2e3369a87b1f84ddad887dd821268c7e57 looks sane.

Waiting for @jans23 @nestire @daringer to confirm no regression. Off channel response was for next week,
@mkopec @macpijan @BeataZdunczyk : We could also remove ns50 from Heads master at this point, too, using helpers to move to UNTESTED, as it was the case under Heads for all untested boards, and leaving Nitrokey to create PR when the board will be tested and confirmed without regression.

Blockers here are affecting #1875 as well, since v560tu is not under master, and required changes for v560tu under #1875 are not there nor testing possible for that platform, which Nitrokey is also expecting to work (for Christmas per blob post).

What do we do? I have no problem dropping ns50, which Nitrokey supports with their fork and rebranding as of now. I have no testers for ns50 under https://github.com/linuxboot/heads/blob/master/BOARD_TESTERS.md which is also problematic. If a board is untested, it should be marked as such and we should move forward.

@tlaurion
Copy link
Collaborator

tlaurion commented Jan 15, 2025

nv4x and ns5x restored, moved back to using a single coreboot revision for all clevo boards

Alright, https://github.com/linuxboot/heads/compare/a80d6da99ba15fa9d1462e084c9065b4ec3c8e9f..b59c0e2e3369a87b1f84ddad887dd821268c7e57 looks sane.

Waiting for @jans23 @nestire @daringer to confirm no regression. Off channel response was for next week, @mkopec @macpijan @BeataZdunczyk : We could also remove ns50 from Heads master at this point, too, using helpers to move to UNTESTED, as it was the case under Heads for all untested boards, and leaving Nitrokey to create PR when the board will be tested and confirmed without regression.

Blockers here are affecting #1875 as well, since v560tu is not under master, and required changes for v560tu under #1875 are not there nor testing possible for that platform, which Nitrokey is also expecting to work (for Christmas per blob post).

What do we do? I have no problem dropping ns50, which Nitrokey supports with their fork and rebranding as of now. I have no testers for ns50 under https://github.com/linuxboot/heads/blob/master/BOARD_TESTERS.md which is also problematic. If a board is untested, it should be marked as such and we should move forward.

user@heads-tests-deb12-nix-V560TU:~/heads$ ./docker_repro.sh make BOARD=nitropad-ns50 board.move_tested_to_untested
Using CircleCI Docker image: tlaurion/heads-dev-env:v0.2.4
----
Usage reminder: The minimal command is 'make BOARD=XYZ', where additional options, including 'V=1' or 'CPUS=N' are optional.
For more advanced QEMU testing options, refer to targets/qemu.md and boards/qemu-*/*.config.

Type exit within docker image to get back to host if launched interactively!
----

--->Launching container without access to host's USB buses (no USB devices was connected to host)...
----------------------------------------------------------------------
!!!!!! BUILD SYSTEM INFO !!!!!!
System CPUS: 12
System Available Memory: 16867 GB
System Load Average: 0.00
----------------------------------------------------------------------
Used **CPUS**: 12
Used **LOADAVG**: 18
Used **AVAILABLE_MEM_GB**: 16866 GB
----------------------------------------------------------------------
**MAKE_JOBS**: -j12 --load-average=18 

Variables available for override (use 'make VAR_NAME=value'):
**CPUS** (default: number of processors, e.g., 'make CPUS=4')
**LOADAVG** (default: 1.5 times CPUS, e.g., 'make LOADAVG=54')
**AVAILABLE_MEM_GB** (default: memory available on the system in GB, e.g., 'make AVAILABLE_MEM_GB=4')
**MEM_PER_JOB_GB** (default: 1GB per job, e.g., 'make MEM_PER_JOB_GB=2')
----------------------------------------------------------------------
!!!!!! Build starts !!!!!!
Makefile:270: warning: overriding recipe for target 'all'
Makefile:251: warning: ignoring old recipe for target 'all'
NEW_BOARD variable will add UNTESTED_ prefix to nitropad-ns50
changing nitropad-ns50 name under boards/nitropad-ns50/nitropad-ns50.config to UNTESTED_nitropad-ns50
sed: can't find label for jump to `oards/nitropad-ns50/nitropad-ns50.config'
Renaming boards/nitropad-ns50/nitropad-ns50.config to boards/nitropad-ns50/UNTESTED_nitropad-ns50.config
Renaming boards/nitropad-ns50 to boards/UNTESTED_nitropad-ns50
Replacing nitropad-ns50 with UNTESTED_nitropad-ns50 in .circleci/config.yml

tlaurion added a commit to tlaurion/heads that referenced this pull request Jan 15, 2025
Move linuxboot#1846 forward.

Signed-off-by: Thierry Laurion <[email protected]>
@tlaurion
Copy link
Collaborator

@macpijan @mkopec @BeataZdunczyk @jans23 @nestire @daringer I advise cherry-picking tlaurion@d70b01d and merging this PR so that #1875 moves forward.

@macpijan
Copy link
Contributor

Sounds good to us.

IIUC NS50 can be always moved back from UNTESTED - this will let us move forward here, and do not put pressure on NK if not priority at this moment.

@tlaurion
Copy link
Collaborator

tlaurion commented Jan 15, 2025

Sounds good to us.

IIUC NS50 can be always moved back from UNTESTED - this will let us move forward here, and do not put pressure on NK if not priority at this moment.

@mkopec I apply my maintainer veto here. Please cherry-pick, let's wait for Ci to build, and merge this PR.

Move linuxboot#1846 forward.

Signed-off-by: Thierry Laurion <[email protected]>
@macpijan
Copy link
Contributor

Michał is offline already. I have cherry-picked I hope I have done it correctly.

@tlaurion
Copy link
Collaborator

Oh @macpijan

Who should I add for v560tu under https://github.com/linuxboot/heads/blob/master/BOARD_TESTERS.md ?

(Can be added later after merge of this PR)

@macpijan
Copy link
Contributor

You can add @mkopec and me as a backup/contact right now.

@tlaurion tlaurion self-assigned this Jan 15, 2025
@tlaurion tlaurion self-requested a review January 15, 2025 20:05
@tlaurion
Copy link
Collaborator

tlaurion commented Jan 15, 2025

@tlaurion tlaurion merged commit 49e0849 into linuxboot:master Jan 15, 2025
48 checks passed
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.

5 participants