search

riscv / configuration-structure

RISC-V Configuration Structure
https://jira.riscv.org/browse/RVG-50
Creative Commons Attribution 4.0 International
36 stars 16 forks source link

Initial support for scalar crypto v1.0.0-rc3 #52

Closed ben-marshall closed 3 years ago

ben-marshall commented 3 years ago

Hi there

This PR adds support to the configuration structure for the scalar cryptography extensions v1.0.0-rc3. The following extensions are added:

Each extension is a single bit in the config structure. I've tried structure them one file per extension. Looking forward to a world where RISC-V has many extensions, this seemed the most sensible, rather than hiding multiple extensions in fewer files.

One open question: The Zkr extension - access to the entropy source is always possible in M mode, and optionally enabled in S/U mode based on corresponding bits in the mseccfg register. These bits can be hardwired or read/writeable. I have not encoded whether these bits are read/write in the config structure. My rationale was that SW only needs to know if the bits exist (because Zkr is indicated by the config structure), and can go and test them itself. So encoding them in the config structure would be redundant information. I hope this is the right call.

Please let me know if there's anything which would improve this PR, I'm happy iterating on it.

Thanks, Ben

timsifive commented 3 years ago

Thanks for getting this going, Ben!

I think all crypto extensions should live in the same type. As it is, your change will add 10 bits to every hart description, even if no crypto extension is implemented. (Well, 20 as written but we could fix that.) If we group them together, then it will take 1 bit if no crypto extension is implemented, and 11 bits if any of them are implemented. (Then making the crypto type extensible adds one more bit.) That is more scalable, especially as we add more and more extensions.

(Aside: I'm still pondering if we should encode things different at top-level, in which case the logic is something like: No overhead if nothing is used, but 8 bits for every option that is used. By grouping that would change to no overhead if nothing is used, and 19 bits if any subset of options is used. Assuming it's common to implement at least 2 of the crypto extensions together, that is almost a wash and at 3 options the grouped version is much more compact.)

ben-marshall commented 3 years ago

Hi @timsifive

So I think I've implemented your recommendations wrt. grouping all of the scalar crypto extensions into a single extensible type / sequence. Experimenting with the example does indeed show this comes out smaller. I'm still getting my head around ASN1 so if there are more compact ways to encode things per hart, then let me know.

In terms of common groups of crypto extensions we expect people to implement, these would likely be the "macro" extensions which group together smaller extensions. Namely:

In terms of number of crypto extensions implemented then, it's most commonly going to be zero or at-least three.

I'll have a closer look at the top level structure and see if I have any thoughts about it. One thing to note though is that the cryptography community has an extremely low opinion of ASN.1 because it's been the source of many bugs, both functional and security related. There might be a bit of work to do explaining it's usage as part of the RISC-V config structure.

Cheers, Ben

timsifive commented 3 years ago

I'll have a closer look at the top level structure and see if I have any thoughts about it. One thing to note though is that the cryptography community has an extremely low opinion of ASN.1 because it's been the source of many bugs, both functional and security related. There might be a bit of work to do explaining it's usage as part of the RISC-V config structure.

That's an interesting point. Certainly ASN.1 is complex so I can totally see where the bugs come from. Since in this use case software is parsing a structure provided by the hardware (presumably from ROM) I think that reduces the security concern. The argument for ASN.1 is basically that it does exactly what we want (compact binary representation, flexible type system) without us reinventing this particular wheel.

changab commented 3 years ago

Looks good to me.

ben-marshall commented 3 years ago

Thanks for the approvals folks! I've implemented Tim's suggested change and squash/rebased. Hopefully this means you can merge it.