cfi: Simpler launder implementation for common types #1714
Merged
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
swenson
requested review from
FerralCoder,
rusty1968,
bluegate010,
mhatrevi,
vsonims,
ajisaxena,
korran and
JohnTraverAmd
as code owners
11 tasks
swenson
commented
Oct 11, 2024
jhand2
reviewed
Oct 11, 2024
swenson
force-pushed
the
cfi-launder
branch
2 times, most recently
from
1921e07 to
05a5902
Compare
jhand2
previously approved these changes
Oct 14, 2024
|
(I've also looked at removing There are more likely a few more types though that would be worth streamlining so we can save more code space, but I leave that to a future PR.) |
jhand2
previously approved these changes
Oct 14, 2024
rusty1968
previously approved these changes
Oct 24, 2024
nquarton
previously approved these changes
Nov 4, 2024
|
@jhand2, @nquarton, @rusty1968 -- approvals were wiped when I rebased and updated the SHAs. No other changes were made. Please re-review and I'll get someone to merge this ASAP for the 1.2 ROM release. |
… value away. Our current implementation uses `core::hint::black_box()`, which is the recommended way in Rust. The problem is, this appears to often force the argument to spill into memory and to be reloaded, which can be a lot of extra instructions. The original inspiration for this function is from, I believe, [OpenTitan's launder* functions](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/base/hardened.h#L193). There, they use an LLVM-specific trick of a blank inline assembly block to force the compiler to keep the argument in a register. After reviewing our code and speaking with @vsonims, it sounds like the intention of the launder in our code is to prevent the compiler from optimizing the value away (as the comments suggest), so the simpler inline assembly trick may be sufficient (since we use the official Rust compiler, which uses LLVM). The biggest problem is that we launder many types of values in our code and not all of them fit into a register. So, this PR represents an incremental change: for `u32`s and similar small types, we implement `cfi_launder` using the inline assembly trick from OpenTitan. For any other types, we have a trait that can be derived that will call `core::hint::black_box` in the same way as today. We can do future follow-up PRs to try to try to clean up some of those other uses of `cfi_launder` to hopefully shrink the code more. I also slipped in avoid a few extra copies in the verifier by using references instead of copies (this saves ~80 bytes of instruction space). This PR appears to shrink the ROM code size by 1232 bytes and the runtime firmware by 700 bytes.
nquarton
previously approved these changes
Nov 4, 2024
mhatrevi
reviewed
Nov 6, 2024
mhatrevi
previously approved these changes
Nov 6, 2024
mhatrevi
approved these changes
Nov 6, 2024
|
Thanks! |
mhatrevi
pushed a commit
that referenced
this pull request
Nov 18, 2024
* `cfi_launder` is meant to prevent the Rust compiler from optimizing a value away. Our current implementation uses `core::hint::black_box()`, which is the recommended way in Rust. The problem is, this appears to often force the argument to spill into memory and to be reloaded, which can be a lot of extra instructions. The original inspiration for this function is from, I believe, [OpenTitan's launder* functions](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/base/hardened.h#L193). There, they use an LLVM-specific trick of a blank inline assembly block to force the compiler to keep the argument in a register. After reviewing our code and speaking with @vsonims, it sounds like the intention of the launder in our code is to prevent the compiler from optimizing the value away (as the comments suggest), so the simpler inline assembly trick may be sufficient (since we use the official Rust compiler, which uses LLVM). The biggest problem is that we launder many types of values in our code and not all of them fit into a register. So, this PR represents an incremental change: for `u32`s and similar small types, we implement `cfi_launder` using the inline assembly trick from OpenTitan. For any other types, we have a trait that can be derived that will call `core::hint::black_box` in the same way as today. We can do future follow-up PRs to try to try to clean up some of those other uses of `cfi_launder` to hopefully shrink the code more. I also slipped in avoid a few extra copies in the verifier by using references instead of copies (this saves ~80 bytes of instruction space). This PR appears to shrink the ROM code size by 1232 bytes and the runtime firmware by 700 bytes. (cherry picked from commit 571d253)
mhatrevi
pushed a commit
that referenced
this pull request
Nov 19, 2024
* `cfi_launder` is meant to prevent the Rust compiler from optimizing a value away. Our current implementation uses `core::hint::black_box()`, which is the recommended way in Rust. The problem is, this appears to often force the argument to spill into memory and to be reloaded, which can be a lot of extra instructions. The original inspiration for this function is from, I believe, [OpenTitan's launder* functions](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/base/hardened.h#L193). There, they use an LLVM-specific trick of a blank inline assembly block to force the compiler to keep the argument in a register. After reviewing our code and speaking with @vsonims, it sounds like the intention of the launder in our code is to prevent the compiler from optimizing the value away (as the comments suggest), so the simpler inline assembly trick may be sufficient (since we use the official Rust compiler, which uses LLVM). The biggest problem is that we launder many types of values in our code and not all of them fit into a register. So, this PR represents an incremental change: for `u32`s and similar small types, we implement `cfi_launder` using the inline assembly trick from OpenTitan. For any other types, we have a trait that can be derived that will call `core::hint::black_box` in the same way as today. We can do future follow-up PRs to try to try to clean up some of those other uses of `cfi_launder` to hopefully shrink the code more. I also slipped in avoid a few extra copies in the verifier by using references instead of copies (this saves ~80 bytes of instruction space). This PR appears to shrink the ROM code size by 1232 bytes and the runtime firmware by 700 bytes. (cherry picked from commit 571d253)
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.
cfi_launderis meant to prevent the Rust compiler from optimizing a value away.Our current implementation uses
core::hint::black_box(), which is the recommended way in Rust. The problem is, this appears to often force the argument to spill into memory and to be reloaded, which can be a lot of extra instructions.The original inspiration for this function is from, I believe, OpenTitan's launder* functions. There, they use an LLVM-specific trick of a blank inline assembly block to force the compiler to keep the argument in a register.
After reviewing our code and speaking with @vsonims, it sounds like the intention of the launder in our code is to prevent the compiler from optimizing the value away (as the comments suggest), so the simpler inline assembly trick may be sufficient (since we use the official Rust compiler, which uses LLVM).
The biggest problem is that we launder many types of values in our code and not all of them fit into a register.
So, this PR represents an incremental change: for
u32s and similar small types, we implementcfi_launderusing the inline assembly trick from OpenTitan. For any other types, we have a trait that can be derived that will callcore::hint::black_boxin the same way as today.We can do future follow-up PRs to try to try to clean up some of those other uses of
cfi_launderto hopefully shrink the code more.I also slipped in avoid a few extra copies in the verifier by using references instead of copies (this saves ~80 bytes of instruction space).
This PR appears to shrink the ROM code size by 1232 bytes and the runtime firmware by 700 bytes.