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

Add the CC64R format #28

Merged
merged 18 commits into from
Feb 6, 2025
Merged

Add the CC64R format #28

merged 18 commits into from
Feb 6, 2025

Conversation

arichardson
Copy link
Member

This implements bounds decoding and encoding for the 32-bit RISC-V format.

It does not yet include the decoding and encoding of permissions.
Changes taken from https://github.com/CHERI-Alliance/cheri-compressed-cap and adapted for the refactorings that have been made to the library since.

CC: @martin-kaiser @buxtonpaul

@jrtc27
Copy link
Member

jrtc27 commented Jan 28, 2025

Can we squash the fixup commits that fix commits also in this PR? They add little value.

@arichardson
Copy link
Member Author

Can we squash the fixup commits that fix commits also in this PR? They add little value.

Sure we can do this - I just kept them separate to show which test input now decodes correctly.

arichardson and others added 9 commits January 28, 2025 16:07
Test the encoding of a capability from base, address and top for
MXLEN=64. This test case uses exponent 0.

Co-authored-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
Test the encoding of a capability from base, address and top for
MXLEN=64. This test case uses an internal exponent.

Co-authored-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
This is used in the RV32 format to reduce the number of exponents bits
taken from the T/B fields (L8 in the spec).
We will have to relax these static_asserts before adding
the new cc64r format.

Co-authored-by: Martin Kaiser <martin.kaiser@codasip.com>
This is an initial draft of the 64r capability format for 64-bit
capabilities as defined in the RISC-V CHERI specification.

Note that this does not handle the decoding of raw permissions to the
architectural ones.
Future commits will add a new encode and decode function, but for now
just set CC64R_PERMS_ALL to the infinite cap value.

Co-authored-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
In preparation for the 32-bit format
Very minor changes needed compared to the 128r version.
This matches the current version of the sail that does not yet support the
levels extension.
@jrtc27
Copy link
Member

jrtc27 commented Jan 29, 2025

Can we squash the fixup commits that fix commits also in this PR? They add little value.

Sure we can do this - I just kept them separate to show which test input now decodes correctly.

Incremental additions can be ok, it's when it's "fix a bug in an earlier commit", that history doesn't help (and if the history is useful to document why the code is the way it is rather than something that seems simpler but isn't quite right, that belongs in a comment).

Martin Kaiser and others added 9 commits January 28, 2025 16:16
This fixes the encoding of base, length and top for CHERI RISC-V with
MXLEN=32. In addition to EF, E, B[9:0] and T[7:0], we also have to
store bit 8 of the length in the L8 field.
Also explain in detail why L8 is needed for recovering T[9:8].

Co-authored-by: Paul Buxton <paul.buxton@codasip.com>
Co-authored-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
When EXPONENT_FORMAT=0, the MSB of the exponent is stored in the
LEN_MSB field.

Co-authored-by: Martin Kaiser <martin.kaiser@codasip.com>
Test the encoding of a capability from base, address and top for
MXLEN=32. This test case uses an internal exponent such that T8 = 1.

fixup test
Test the encoding of a capability from base, address and top for
MXLEN=32. This test case uses an internal exponent such that T8 = 0.
Add a bounds encoding test case for MXLEN=32 where the base correction
c_b is not 0.
We have to read the LEN_MSB for the cc64r format to infer the correct
top. Add a regression test found via fuzzing.
We also have to reject `IE && E==0` for formats that use LEN_MSB.
@arichardson
Copy link
Member Author

Can we squash the fixup commits that fix commits also in this PR? They add little value.

Sure we can do this - I just kept them separate to show which test input now decodes correctly.

Incremental additions can be ok, it's when it's "fix a bug in an earlier commit", that history doesn't help (and if the history is useful to document why the code is the way it is rather than something that seems simpler but isn't quite right, that belongs in a comment).

I've dropped some of the ones above. The current fixups are "handle a special case in the 64r format that didn't exist before". Hopefully the new commits are better?

@martin-kaiser
Copy link

Basically, this looks ok to me.

It's great that the sail-based tests are now available for the risc-v formats.

It seems that you prefer re-using existing fields instead of adding new ones
that match the names in the risc-v cheri specification. I can live with this
although it might be confusing in some cases (e.g. people have to remember that
CT used to be part of OTYPE). When I looked at the code, it seemed quite simple
to mark a field as unused for a particular format. I would have thought you'd
be happy to use this feature here.

I'm curious about your plans for AP encoding in 64r and for configuring the
infinite capability (it depends on Zcherihybrid or Zcherilevels support). These
two will have the greatest impact on qemu, I guess.

@arichardson
Copy link
Member Author

Basically, this looks ok to me.

It's great that the sail-based tests are now available for the risc-v formats.

It seems that you prefer re-using existing fields instead of adding new ones that match the names in the risc-v cheri specification. I can live with this although it might be confusing in some cases (e.g. people have to remember that CT used to be part of OTYPE). When I looked at the code, it seemed quite simple to mark a field as unused for a particular format. I would have thought you'd be happy to use this feature here.

I'm curious about your plans for AP encoding in 64r and for configuring the infinite capability (it depends on Zcherihybrid or Zcherilevels support). These two will have the greatest impact on qemu, I guess.

I was planning to lift your changes for the decoding and encoding of the permissions as a follow-up change.

@arichardson arichardson merged commit db44ebf into master Feb 6, 2025
2 checks passed
@arichardson arichardson deleted the cc64r branch February 6, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants