-
Notifications
You must be signed in to change notification settings - Fork 83
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
Scalar Cryptography v1.0.0-rc2. Incorrect ZIP/UNZIP insns encoding. #130
Comments
Hi @bulat242 Thanks for spotting this! Just to be clear - it looks like the pseudocode for I'll have to do some research on how to handle this, since although it's very simple to swap the descriptions in the spec, because its frozen, there might be other implications. Cheers, |
Yes, the descriptions for zip and unzip have to be swapped in a general sense. (UNZIP operation in spec is described as "inverse zip"). |
Yeah, that's a rubbish description for sure. I'll fix that. Okay, I'll follow this up at the weekly CETG meeting tomorrow and ask how to handle it. |
And I would like to clarify about rev8 and brev8 descriptions. |
The intent is to step through every byte in the register, so yes the |
Okey, thank you for explanation! |
@bulat242 - The plan for fixing this is to make the necessary changes in the spec and do a Out of curiosity - how did you catch the problem? |
@ben-marshall Okey :) |
- fix sail code for zip/unzip - improve descriptions On branch dev/ben/zbk Your branch is up-to-date with 'ben-marshall/dev/ben/zbk'. Changes to be committed: modified: bitmanip/insns/unzip.adoc modified: bitmanip/insns/zip.adoc
- Fix zip/unzip sail code and descriptions On branch master Your branch is up-to-date with 'origin/master'. Changes to be committed: modified: ../../extern/riscv-bitmanip Changes not staged for commit: modified: ../../extern/riscv-gnu-toolchain (modified content) modified: ../../extern/sail-riscv (new commits, untracked content)
@bulat242 - Here's the fix, hopefully this is a bit clearer than before! |
I'm going to leave the issue open for now in case other people spot the same problem and try to report it. Thanks again for the catch! |
Okay, you are welcome. |
@ben-marshall Hi! |
Hi @bulat242 - The encodings were not modified from the proposed Bitmanip ones. If they are different that's likely a mistake. Note that we are only standardising the RV32 versions of zip/unzip. I think the RV64 versions indeed had a different length shamt field. Could that be where they difference came from? I think the 0.93 Bitmanip spec in table 2.7 only gave RV64 listings for the SHAMT field. |
Hi @ben-marshall |
Yes, the encoding of zip/unzip (rv32 only) insns should look like below: |
Ah yes I see now. It looks like bits 24:20 in the scalar crypto spec were taken from the wrong part of the tables in Bitmanip 0.93. I'll fix this in the |
Okey, happy to help! |
- See riscv/riscv-crypto#130 On branch dev/ben/zbk Your branch is up-to-date with 'ben-marshall/dev/ben/zbk'. Changes to be committed: modified: bitmanip/insns/unzip.adoc modified: bitmanip/insns/zip.adoc
See #130 On branch master Your branch is up-to-date with 'origin/master'. Changes to be committed: modified: ../../extern/riscv-bitmanip
Hi @bulat242 , @marcfedorow - See here for the fix to the immediates. I've checked to see this is also what GCC emits for |
Hi @ben-marshall |
Yes, the descriptions of zip/unzip operations in crypto spec v1.0.0-rc2 were correct. |
@ben-marshall please note that this issue is not resolved yet |
Hi @marcfedorow - yes it's on my list to fix (again). I'll get to it tomorrow. |
Hi @marcfedorow , @bulat242
Please confirm that the new zip and unzip definitions match your expectations. You can see the diff here. I've compared these with the output of the instructions in Spike, and everything seems to match up. I'll be asking some others to double check this too, because I am so confused by the whole thing I no longer trust myself! |
Hi, @ben-marshall ! |
Hi @ben-marshall |
Chapter 3 -> 3.49 (Page 58):
Operation
foreach (i from 0 to xlen/2-1) {
X(rd)[2i] = X(rs1)[i]
X(rd)[2i+1] = X(rs1)[i+xlen/2]
}
This description is suitable to UNZIP operation.
Correct description of ZIP:
Operation
foreach (i from 0 to xlen/2-1) {
X(rd)[i] = X(rs1)[2i]
X(rd)[i+xlen/2] = X(rs1)[2i+1]
}
The text was updated successfully, but these errors were encountered: