initrd/bin/talos-init: send IPL complete message to BMC #1313
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@krystian-hebel now we talk! Thank you so much for PR pointing to coreboot and Heads changes. Will test produced CircleCI binaries tomorrow and report back |
|
@krystian-hebel can you just please have heads master circleci config as well so that builds are triggered automatically? |
|
Or better, have CircleCI follow you Heads fork so that builds happens on your CircleCI time (not osresearch's). This is better for continuous contributors, while not an issue. Not sure what is the issue here? |
|
@macpijan ^ can we do this? |
|
@krystian-hebel create a circleci account, linked with your github account. Then in project screen on circleci, select heads as a project to be followed. Amend commit and force push. Enjoy circleci free hour build steps for free. |
krystian-hebel
force-pushed
the
talos_fan_speed
branch
from
d1ffdd0
to
b137dd3
Compare
|
@krystian-hebel Works, fan is slowed down upon early Heads init, (now talos-init pointing to gui-ini) as expected. Tested: Downloading artifacts from CircleCI: Uploading artifacts: Testing firmware: Gives attached log: Exit to recovery shell from Heads to check CPU freq: |
|
@krystian-hebel Putting as draft since you said to me in upstream Dasharo/dasharo-issues#35 that this should not be merged (some TPM code might not be under this dasharo/coreboot commit). Please poke me in next PR superseding this. Dasharo/dasharo-issues#35 can be closed meanwhile. |
|
Will test alongside of #1339 |
|
Again didn't have time to test yet. But since changes here are considered maybe unstable, Would it be possible to simply rebase and use merged #1339 in the future? TPM1/2 unification is merged under master now, so rebasing should happen anyway going further. |
|
It does not work as expected, I'm debugging it now. I've found some rather unrelated errors in the process that would impact all non-x86 platforms, will create another PR with fixes soon. |
krystian-hebel
force-pushed
the
talos_fan_speed
branch
from
995c69a
to
d4b1a87
Compare
|
I've updated this PR so my changes won't get lost (again), I know I still have to do a rebase. @tlaurion how bad of an idea would be to push all coreboot commits related to Talos in form of patches to this repo? There are almost 200 commits right now, but some of them ("fix typo" kind) can be squashed. |
|
@krystian-hebel why mot simply have modules/coreboot point to that squashed commit to dasharo/coreboot? |
|
This is what we've been doing until now, but it proved to be hard to manage. Right now our last connection with upstream is from 2 years ago, with a lot to rebase, and it doesn't give us a clean way of changing one commit that was changed after review upstream. We would have to keep each release in a separate branch instead of tag, because each rebase results in a forced push. As we're getting close to production-ready version, we would like to make future maintenance as easy as possible, while we are still in pre-v1.0.0 and such changes are possible. For PC Engines we had yet another approach, which worked great until changes were actually merged upstream. That left us with either empty commits or commits with just whitespace/variable name/comment style different in PC Engines repo than upstream. The purpose of that was slightly different, we knew that there would be some changes specific to those releases that wouldn't make sense for upstream, or at least we thought so at that time. Now we want to see if patches would be better for this case. They would allow us to make changes to older commits while keeping those changes in history, instead of doing a hard rewrite. The same goes for preparation for upstream - all changes in one piece of logic are limited to one patch file, that can be modified without having to rebase every later commit, and more importantly, without adding separate commits for simple fixes. In some cases, squashing such fix isn't trivial and produces a conflict. I will probably keep those patches somewhere anyway, I'm just asking if Heads may be where they live for now. I'll understand if you don't want to keep so many additional files, since this would stay forever in the git history, making this repo larger. I guess we can also keep a separate branch with patch-like commits, updated in parallel to our usual develop-release pair of branches. |
|
Discussed off channel. Possibilities are:
I have no strong opinion against either of them, of course preferring coreboot work happening outside of Heads where heads can only refer to a commit/release tarball, as for all other modules. The only problem with actual modules/coreboot git approach as of now is foreseeing multiple boards abusing of the coreboot-git locally created directory, which could eventually be used for parallel builds of multiple boards. It would be better that Makefile+modules/coreboot could use a named approach for git clone directory, and that coreboot-git being named coreboot-talos in present case, where circleci config would point to coreboot-talos (as opposed to coreboot-git) for cache saving and reusal. Ideally:
|
krystian-hebel
force-pushed
the
talos_fan_speed
branch
2 times, most recently
from
293a31d
to
d8f6b08
Compare
|
@tlaurion could you test https://app.circleci.com/pipelines/github/Dasharo/heads/11/workflows/5d083562-57ae-4095-8c5c-b46b6cca4f8f/jobs/309/artifacts? Things to check:
If everything is fine so far I'll proceed with rebasing. This also depends on #1352, for now I just cherry-picked that change here, but it would be nice if that PR gets merged first. I also noticed that |
|
@krystian-hebel this is with agetty back in? |
|
@tlaurion I think this may even be before it was removed. I can use |
|
Ok, I see 122 files changes differences between this and master. But yeah, agetty is in without util-linux removed from Makefile so would not be impacted by this. Once again, testing server board here since same as single talos board now in master. |
Hmmm. Will check things out, bmc in weird state cannot change backend it seems... |
Wait what? |
|
I've seen something like this, noted in comment. Do |
|
Better |
|
Signed-off-by: Krystian Hebel <[email protected]>
…efconfig Signed-off-by: Krystian Hebel <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
BMC awaits this message before it takes control over CPU fans speed. Signed-off-by: Krystian Hebel <[email protected]>
Signed-off-by: Krystian Hebel <[email protected]>
@krystian-hebel not on my side. :/ |
|
Is the adapter plugged in to CPU1 PCIe ports? Unfortunately most of them belong to CPU2, and I don't think it's clearly marked on the board.
|
|
Ha! No was connected to lowest and smallesy connector. Will reconnect |
krystian-hebel
force-pushed
the
talos_fan_speed
branch
from
d8f6b08
to
1a69b1d
Compare
|
Rebase done, when CI finishes it should be ready for testing. Depending on results it may be identical to final release, +/- hash in revision. |
|
@krystian-hebel nvme drive discovered :) |
|
@krystian-hebel seems like usb hid was lost, so no usb keyboard support under Heads. Will check that |
|
Houla @krystian-hebel Ok, after manually having followed #1360 to kexec manually debian netinstall and install from network to nvme (success), host rebooted. It asked to setup new boot device, selected /dev/nvme1p2 (unencrypted boot), reflashed internally, success. Seems like TPM is not solid in this tested commit. Again, we are talking about reboot under Heads here, which uses sysrq: |
Then But then, no TPM: |
|
cat /tmp/config: Meaning that dynamic code detection for TPM was triggered because no tpm was found. @krystian-hebel And this is because linux didn't find the tpm nor created /dev/tpm0 |
|
|
@krystian-hebel Hmmm. |
|
@krystian-hebel #1362 is merged, fixing usb keyboard which were tested with https://app.circleci.com/pipelines/github/tlaurion/heads/1580/workflows/16321a18-0fb8-4cbf-ad49-c5d0ec6c5288/jobs/20158/artifacts This PR should be rebased on top of master when some more troubleshooting done on my platform from your side. |
|
@krystian-hebel since thi PR fixes successfully the issue it was targeting, would you want me to open new issues upstream for TPM being unreliable and we merge+close this, or is it easier for you to add more commits here for me to test? As of now, which might need a seperate issue:
|
|
@tlaurion since I don't have ideas what can be wrong with that TPM and what can be checked right now I think we can merge it as it is and get back to debugging later, if that's OK with you.
Yes, this is because Skiboot doesn't understand TPM1.2 log format. Its whole approach to extending PCRs is specific, to say the least, but as I understand it, specifications don't forbid it. What Skiboot does is it hashes blocks of memory using SHA512, but extends PCR banks for SHA1 and SHA256 (only SHA1 in TPM1.2, added by us) with appropriate number of first bytes of SHA512. It has the advantage of not having to hash the data twice. |
|
@krystian-hebel issues should be opened linking to previous comments on Dasharo side for TPM issues. |
This was referenced Apr 20, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BMC awaits this message before it takes control over CPU fans speed.