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

Rust 1.83 #1862

Draft
wants to merge 6 commits into
base: main-2.x
Choose a base branch
from
Draft

Rust 1.83 #1862

wants to merge 6 commits into from

Conversation

ArthurHeymans
Copy link
Contributor

@ArthurHeymans ArthurHeymans commented Dec 18, 2024

The goal of updating the rust library is to be able to use Mldsa from
rustCrypto repo: RustCrypto/signatures#877

The goal of using this code is to replace openssl to generate TBS and
CSR tempaltes. Openssl does not yet support Mldsa

The minimum required rust version is 1.81.

It looks like this toolchain needs a lot more stack than 1.70.

Rust 1.83 warns about unused stuff a lot more often which is addressed
in this commit.

This also updates DPE to compile without warnings and errors.
The changes are on github.com/caliptra-dpe but is not in the main
branch.

dpe Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

This commit isn't upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment in the PR description. Let me know how you think this should be dealt with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's merge the caliptra-dpe PR first and then use the upstream commit. It looks like there's an open comment from zach on that one.

@jhand2
Copy link
Collaborator

jhand2 commented Dec 18, 2024

Can you update the PR description with the motivation for doing this?

In general, all PRs should have descriptions.

Also, this PR seems to have 2 classes of changes:

  1. Things required because the compiler version changed (e.g. formatting, new clippy annotations)
  2. Things that leverage new rust features that are now available.

Ideally all the things in (2) would be in a separate PR.

Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

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

There seem to be some unrelated changes in this PR related to the manifest-based auth and ROM size changes.

*/
ROM_RELAXATION_PADDING = 8k;
ROM_SIZE = 96K;
ICCM_SIZE = 128K;
DCCM_SIZE = 256K;
DATA_SIZE = 996;
STACK_SIZE = 40K;
STACK_SIZE = 46K;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the stack and ROM size need to change in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it looks like with the newer toolchain significantly more stack is used. The stack overflow detection code in the emulator tripped on it and this was the minimal increase necessary.

@@ -65,9 +64,6 @@ pub struct RomEnv {
/// PCR Bank
pub pcr_bank: PcrBank,

/// FHT Data Store
pub fht_data_store: FhtDataStore,
Copy link
Contributor

Choose a reason for hiding this comment

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

This also seems unrelated to upgrading to Rust 1.83

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 unused and rust 1.83 complains about it.

@ArthurHeymans ArthurHeymans added the Caliptra v2.0 Items to be considered for v2.0 Release label Dec 19, 2024
Newer versions of rust complain about this

Signed-off-by: Arthur Heymans <arthur.heymans@9elements.com>
Newer version of rust warn about this.

Signed-off-by: Arthur Heymans <arthur.heymans@9elements.com>
The goal of updating the rust library is to be able to use Mldsa from
rustCrypto repo: RustCrypto/signatures#877

The goal of using this code is to replace openssl to generate TBS and
CSR tempaltes. Openssl does not yet support Mldsa

The minimum required rust version is 1.81.

It looks like this toolchain needs a lot more stack than 1.70.

Rust 1.83 warns about unused stuff a lot more often which is addressed
in this commit.

Signed-off-by: Arthur Heymans <arthur.heymans@9elements.com>
This makes DPE compile without warnings and errors.
The changes are on github.com/caliptra-dpe but is not in the main
branch.

Signed-off-by: Arthur Heymans <arthur.heymans@9elements.com>
@ArthurHeymans
Copy link
Contributor Author

Can you update the PR description with the motivation for doing this?

In general, all PRs should have descriptions.

Also, this PR seems to have 2 classes of changes:

  1. Things required because the compiler version changed (e.g. formatting, new clippy annotations)
  2. Things that leverage new rust features that are now available.

Ideally all the things in (2) would be in a separate PR.

This PR only attempts to fix warnings and errors with the newer toolchain. I added some motivation documentation to the PR.

@swenson
Copy link
Contributor

swenson commented Dec 19, 2024

This update appears to add a panic because Rust can't prove the indexes are correct in a ufmt macro. This should fix it I think:

diff --git a/drivers/src/printer.rs b/drivers/src/printer.rs
index 4b6d86c9..45f47e23 100644
--- a/drivers/src/printer.rs
+++ b/drivers/src/printer.rs
@@ -68,8 +68,19 @@ impl uDisplay for HexBytes<'_> {
     where
         W: uWrite + ?Sized,
     {
-        for byte in self.0.iter() {
-            ufmt::uwrite!(f, "{:02X}", *byte)?;
+        for &x in self.0.iter() {
+            let c = x >> 4;
+            if c < 10 {
+                f.write_char((c + b'0') as char)?;
+            } else {
+                f.write_char((c - 10 + b'A') as char)?;
+            }
+            let c = x & 0xf;
+            if c < 10 {
+                f.write_char((c + b'0') as char)?;
+            } else {
+                f.write_char((c - 10 + b'A') as char)?;
+            }
         }
         Ok(())
     }

The compiler now generates panics in a few places that it would know how
to handle previously.

Signed-off-by: Arthur Heymans <arthur.heymans@9elements.com>
The rust 1.83 generates code that is a bit larger and uses more stack.

Signed-off-by: Arthur Heymans <arthur.heymans@9elements.com>
@mhatrevi mhatrevi marked this pull request as draft December 25, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Caliptra v2.0 Items to be considered for v2.0 Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants