-
Notifications
You must be signed in to change notification settings - Fork 82
warmreset test for CM command : CM_MLDSA_PUBLIC_KEY, sign /verify for MLDSA and ECDSA, as weel as CM_DERIVE_STABLE_KEY #2825
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
Conversation
…T GET_IDEV_ECC384_INFO POPULATE_IDEV_ECC384_CERT, GET_LDEV_ECC384_CERT, GET_FMC_ALIAS_ECC384_CERT, GET_RT_ALIAS_ECC384_CERT:
…, GET_LDEV_MLDSA87_CERT, GET_FMC_ALIAS_MLDSA87_CERT, GET_RT_ALIAS_MLDSA87_CERT, GET_IDEV_MLDSA87_INFO
…SET_COUNTER, QUOTE_PCRS_ECC384, QUOTE_PCRS_MLDSA87, EXTEND_PCR
…NDED" This reverts commit 41dd0b4.
This reverts commit 40546dd.
…S_ECC384_CSR, GET_FMC_ALIAS_MLDSA87_CSR
…m")))] to tests for attestation
…S_ECC384_CSR, GET_FMC_ALIAS_MLDSA87_CSR
…m")))] to tests for attestation
for warmreset test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR @meilu-git. This PR should be cleaned up significantly before it is further reviewed / merged. Specifically, could you please:
- Clean up your commits? i.e. squash them into one commit or a set of reasonably sequenced commits to make them easily reviewable. You have a PR with 109 commits. While this repo does follow a squash and merge convention it makes reviewing difficult. You can use git's interactive rebase feature to aid you in cleaning this up. See https://hackernoon.com/beginners-guide-to-interactive-rebasing-346a3f9c3a6d
- Cleanup the title and description of this PR. The title should be short and concise and the description more elaborate to describe with this PR contains. See chipsalliance/caliptra-mcu-sw#704 as an example.
- Can this PR be broken into several smaller PRs? Ideal PRs should be <500 lines of cod? See https://github.com/chipsalliance/Caliptra/blob/main/doc/CaliptraContributingProcess.md#submission-of-pull-requests . Unless it is unavoidable to be larger (i.e. cross cutting changes or PRs with bulk cherry-picks between branches) we like to keep PRs small to make them easier to review, reduce the risk of CI breakages, and promote a clean SW supply chain.
|
close this PR as it's too much commit which makes review difficult. will open a new PR |
|
Thanks for the email, Timothy.
Yes, you’re right — the original PR accumulated too much commit history, including multiple merges and rebases. After discussing this with our team, I created a fresh branch from main. Since the warm-reset tests are self-contained in their own folder, I manually copied the reviewed changes into the new branch.
The tests include about 17 test files, but they are isolated from the rest of the codebase. Because of this, we think it is reasonable to keep them together in a single PR rather than splitting them further.
This work has been reviewed multiple times already. At this point, the remaining issue is a single test failure, which I am actively debugging now.
Thanks for the feedback again, appreciated.
Mei
…________________________________
From: Timothy Trippel ***@***.***>
Sent: Monday, January 5, 2026 11:28 PM
To: chipsalliance/caliptra-sw ***@***.***>
Cc: Mei Lu ***@***.***>; Mention ***@***.***>
Subject: Re: [chipsalliance/caliptra-sw] warmreset test for CM command : CM_MLDSA_PUBLIC_KEY, sign /verify for MLDSA and ECDSA, as weel as CM_DERIVE_STABLE_KEY (PR #2825)
@timothytrippel requested changes on this pull request.
Thanks for your PR @meilu-git<https://github.com/meilu-git>. This PR should be cleaned up significantly before it is further reviewed / merged. Specifically, could you please:
1. Clean up your commits? i.e. squash them into one commit or a set of reasonably sequenced commits to make them easily reviewable. You have a PR with 109 commits. While this repo does follow a squash and merge convention it makes reviewing difficult. You can use git's interactive rebase feature to aid you in cleaning this up. See https://hackernoon.com/beginners-guide-to-interactive-rebasing-346a3f9c3a6d
2. Cleanup the title and description of this PR. The title should be short and concise and the description more elaborate to describe with this PR contains. See chipsalliance/caliptra-mcu-sw#704<chipsalliance/caliptra-mcu-sw#704> as an example.
3. Can this PR be broken into several smaller PRs? Ideal PRs should be <500 lines of code (see https://github.com/chipsalliance/Caliptra/blob/main/doc/CaliptraContributingProcess.md#submission-of-pull-requests) unless it is unavoidable to be larger (i.e. cross cutting changes or PRs with bulk cherry-picks between branches).
—
Reply to this email directly, view it on GitHub<#2825 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BXJW25JR6OYTPDY5PFVH2IL4FNP2BAVCNFSM6AAAAACMXXF2SKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMMRZG42TKNJSGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@meilu-git where is this new branch you created? did you create a PR from the new cleaned up branch targeting the main branch? The Caliptra contribution guidelines dictate a "fork and PR" method, meaning all changes need to go through a PR and review process before they can be submitted. Specifically the guidelines dictate: "Fork the repos into your personal repo, and create pull requests from your personal repo. Do not pollute the main repo with your personal branches.": https://github.com/chipsalliance/Caliptra/blob/main/CONTRIBUTING.md#guidelines-for-code-contributions Morever, we request small PRs, not large changes. 17 isolated tests sounds like it could be 17 isolated PRs to me, or more efficiently maybe 3 or 4 PRs. You will find it is easier to get PRs reviewed and merged if they are small. Thanks. |
warmreset test for command : CM_MLDSA_PUBLIC_KEY, CM_MLDSA_SIGN, CM_MLDSA_VERIFY, CM_ECDSA_PUBLIC_KEY, CM_ECDSA_SIGN, CM_ECDSA_VERIFY, CM_DERIVE_STABLE_KEY