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

risc-v/boot: verbosely log boot errors #429

Merged
merged 2 commits into from
Jul 12, 2021

Conversation

axel-h
Copy link
Member

@axel-h axel-h commented Jun 30, 2021

Log an explicit error message for everything that can fail during boot. This is helpful for debugging, e.g. when porting the kernel to a new platform or changing the system configuration.

@axel-h
Copy link
Member Author

axel-h commented Jun 30, 2021

The preprocess test fail because of the printf call that this patch adds.

Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

These look good from my side.

Should not affect the proofs since it's all boot code, but let me start a testboard just to make sure.

@lsf37
Copy link
Member

lsf37 commented Jun 30, 2021

@ssrg-bamboo proof

1 similar comment
@nomadeel
Copy link
Contributor

nomadeel commented Jul 1, 2021

@ssrg-bamboo proof

@ssrg-bamboo
Copy link

Hello, I'm a bot! I've set up a proof testboard for this PR here, and results should show up as pending in status checks in a few minutes.

@nomadeel
Copy link
Contributor

nomadeel commented Jul 1, 2021

I guess the bot didn't receive the first message as a notification.

@axel-h
Copy link
Member Author

axel-h commented Jul 3, 2021

Can I merge this now or is there another manual step needed to update the preprocess templates?

Axel Heider added 2 commits July 9, 2021 15:39
Signed-off-by: Axel Heider <axel.heider@hensoldt-cyber.de>
Signed-off-by: Axel Heider <axel.heider@hensoldt-cyber.de>
@mbrcknl
Copy link
Member

mbrcknl commented Jul 12, 2021

Can I merge this now or is there another manual step needed to update the preprocess templates?

A preprocess test failure just compares this pull request with the current head to determine whether this PR potentially impacts the proofs. Once you merge this, it's the new head, so it will be the basis for future preprocess tests.

TL;DR: no need to do anything to update the preprocess check. We just need a reasonable level of confidence this won't break the proofs. And I'm reasonably confident of that.

Not sure what's going on with the simulation tests, though. I guess most are timeouts? Except maybe clang armv6a, which seems unrelated to this PR.

@lsf37
Copy link
Member

lsf37 commented Jul 12, 2021

Let me re-run the simulations. Only armv6a seems to have actually failed, the others were cancelled because of that failure. There is one test in the test set that is dodgy on GH (see #seL4/ci-actions#112).

@lsf37
Copy link
Member

lsf37 commented Jul 12, 2021

To add on to Matt's explanation: it's Ok to merge it, i.e. no other prework or preconditions, but there is an extra step afterwards (which we can do, but you also have sufficient access to run the script if you want -- usually a verification person should be involved in that, but Matt already gave his Ok).

Basically, the preprocess-base is the last known-good version that has a proof, and that only bumps automatically to other preprocess-equal versions or if we explicitly tell it to.

That latter part is the "preprocess bump" script that people might have mentioned before. It lives in https://github.com/seL4/l4v/tree/master/misc/bump together with a bit more explanation.

@lsf37
Copy link
Member

lsf37 commented Jul 12, 2021

Ok, all simulation runs successful now. It does look like that failure is not that rare and we should do something about it.

@lsf37 lsf37 merged commit 913bf48 into seL4:master Jul 12, 2021
@lsf37
Copy link
Member

lsf37 commented Jul 12, 2021

(preprocess bump done as well)

@axel-h axel-h deleted the patch-axel-20 branch July 12, 2021 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants