riscv-crypto
riscv-crypto copied to clipboard
riscv
Scalar Cryptography v1.0.0-rc2. Incorrect ZIP/UNZIP insns encoding.
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] }
Hi @bulat242
Thanks for spotting this! Just to be clear - it looks like the pseudocode for zip and unzip are the wrong way around, right?
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, Ben
Yes, the descriptions for zip and unzip have to be swapped in a general sense. (UNZIP operation in spec is described as "inverse zip").
(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. Are these descriptions same? rev8: foreach (i from 0 to (xlen - 8) by 8) brev8: foreach (i from 0 to sizeof(xlen) by 8)
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 foreach statements are supposed to be equivalent. It makes no sense to express the same thing two different ways, so I'll make sure we use something consistent in the final versions.
@bulat242 - The plan for fixing this is to make the necessary changes in the spec and do a v1.0.0-rc3 release, and publicise this on the isa-dev mailing list. I'm double and triple checking things at the moment, but hopefully it'll be out if not today then early next week.
Out of curiosity - how did you catch the problem?
@ben-marshall Okey :) I compared these descriptions of the instructions with old descriptions from bitmanip spec (v0.93).
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!
@ben-marshall Hi! I have a new question about encoding of zip/unzip insns. In current scalar cryptography spec ZIP and UNZIP insns have different values in 20th bit. I understand the 20th bit in Bitmanip spec (v. 0.93) was included in shamt field of shfli/unshfli insns and was set for ZIP/UNZIP cases. Was ZIP/UNZIP insns encoding in current crypto spec derived from bitmanip spec (v 0.93) and is it correct?
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
Actually 0.9.3 bitmanip spec gave RV128 encodings — you can easily assure by checking 7-bit immediate for SLLIU.W (RV32 and RV64 require 5 and 6 bits respectively).
Due to this reason (and explicit placing of inv bit to the generalization table) someone might have mistakenly decided that inv bit is a part of immediate.
This is not true tho — immediate only contains shift amount (shamt), the inv bit is the 14th one.
Thus both zip and unzip have to have 01111 bits in RS2 position (RV32 only! 11111 for RV64).
Yes, the encoding of zip/unzip (rv32 only) insns should look like below: 0000100 01111 rs 001 rd 0010011 -> zip 0000100 01111 rs 101 rd 0010011 - > unzip
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 rc3 release so that for both, bits 24:20 are 01111 as you say.
Thanks again and good catch!
Cheers,
Ben
Hi @bulat242 , @marcfedorow - See here for the fix to the immediates. I've checked to see this is also what GCC emits for zip and unzip on RV32. I'll do a release later today and make sure it gets publicised on isa-dev. Thanks for all your help with this!
Hi @ben-marshall Thank you for ping! Current descriptions seem to be incorrect tho. shfl (zip) should do 3, 2, 1, 0 stages. unshfl (unzip) should do 0, 1, 2, 3 stages. This could be mixed up because B-ext 0.9.3. graphs should be read upwards (from down to up) -- check C code or other graphs for cross-check.
Yes, the descriptions of zip/unzip operations in crypto spec v1.0.0-rc2 were correct. I'm sorry.
Hi @marcfedorow - yes it's on my list to fix (again). I'll get to it tomorrow.
Hi @marcfedorow , @bulat242 So, I'd like to get this completely fixed today. My understanding of this mess so far is that:
- The RC1 encodings for
zipandunzipwere wrong, but they pseudo code and English descriptions were correct. - It was pointed out that the encodings for
zipandunzipwere wrong, and that the descriptions & pseudo-code where "swapped" between Zip and Unzip. This wasn't actually the case, but I convinced myself that I'd made a mistake copying things. This was compounded by knowing that these instructions had been the wrong way around before. - In RC3, the encodings became correct, but the pseudo-code and English descriptions became wrong, because I incorrectly swapped them.
- Now, in what will be RC4, the encodings and pseudo-code should be correct with respect to the original Bitmanip specs.
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 ! You are right. The encodings now (in spec rc3) are correct, but unfortunately (because of my inattention) the pseudo-code of zip/unzip is incorrect.
Hi @ben-marshall Probably the word "respectively" could stay in both descriptions. Besides this, zip/unzip look good to me now in pre-rc4.