Closed mjosaarinen closed 1 month ago
The fundamental problem, IMO, is the duplication of the Zbkb, Zbkc, and Zbkx specs between the bitmanip and scalar crypto chapters. I think it makes the most sense to consolidate them into the bitmanip chapter, but fix the mnemonics to correspond to the ones in the scalar crypto chapter, as you suggest. If someone can make a PR that does this, I'd be happy to review it.
I will take care of this PR.
@mjosaarinen My PR fixes the mnemonics: https://github.com/riscv/riscv-isa-manual/pull/1524
Since no good deed goes unpunished, can you sanity-check Zbkb/Zbkc/Zbkx in the bitmanip chapter? I did some spot checks. The opcodes look right, but you might want to check them anyway. The SAIL code is differently formatted, though. (I didn't check its semantics.) If you have any recommendations, please follow up with a PR, which I'll be happy to review.
Thanks,
Here are some things from the review:
There's a typo here: https://github.com/riscv/riscv-isa-manual/blob/568e50adbfbaab478153807e0efed6f3bc908671/src/scalar-crypto.adoc?plain=1#L3356
As you note, it would be great if andn
clmul
clmulh
orn
pack
packh
packw
rev8
rol
rolw
ror
rori
roriw
rorw
unzip
xnor
xperm4
xperm8
zip
would not be defined twice in two different sections.
Typesetting of sail for brev8
, xperm4
, xperm8
, in scalar-crypto.adoc is different from the one in b-st-ext.adoc as all indent has been removed from the crypto definitions of these instructions. https://github.com/riscv/riscv-isa-manual/blob/568e50adbfbaab478153807e0efed6f3bc908671/src/scalar-crypto.adoc?plain=1#L3436
The sail definitions in bitmanip retain the old names in pseudocode; e.g. xpermn_lookup
https://github.com/riscv/riscv-isa-manual/blob/568e50adbfbaab478153807e0efed6f3bc908671/src/b-st-ext.adoc?plain=1#L3568
Furthermore on the bitmanip version retains the old names in links. https://github.com/riscv/riscv-isa-manual/blob/568e50adbfbaab478153807e0efed6f3bc908671/src/b-st-ext.adoc?plain=1#L988
Cheers, -markku
PR from the review https://github.com/riscv/riscv-isa-manual/pull/1531
Many thanks. I'll consider this one resolved for now.
This can be confusing and I don't see why it would be intentional:
I recall that (Krste) requested us to change the names of { rev.b, xperm.n, xperm.b } from the bitmanip names in late stages of scalar crypto ratification -- just before the public review. So we changed those. I guess the old names made a comeback from the old bitmanip specification into the merged spec?
Here's the original issue that motivated the change: https://github.com/riscv/riscv-crypto/issues/115
For scalar crypto the change happened for v1.0.0-rc2 in September 2021: https://github.com/riscv/riscv-crypto/releases/tag/v1.0.0-rc2-scalar