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

[aes] Add intermodule connections and estimated gates for hardening #2140

Merged
merged 3 commits into from
Jul 13, 2020

Conversation

vogelpi
Copy link
Contributor

@vogelpi vogelpi commented May 6, 2020

This adds non-functional changes for the bronze netlist. More precisely, it adds:

What is still missing:

  • gate count estimate for additional modes (if we decide to go for it)

// request interfaces. Once this is merged into the the CSRNG bronze PR
// (see lowRISC/OpenTitan#1947) and this PR is merged, it will be removed.

package csrng_pkg;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mwbranstad, @martin-lueker, I went ahead and defined a prototype for the peripheral CSRNG interface according to the figures in your spec. This is for peripherals to request entropy from the CSRNG. Obviously, the interface defined here is just preliminary. I will update this according to what you decide to use/support. Just let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @vogelpi. I looked at it quickly. We need to talk about a number of items, but this starts that discussion. Once more work has been done on csrng, we probably need to set up a meeting to discuss a number of these issues that I have in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks for the feedback @mwbranstad .

@vogelpi
Copy link
Contributor Author

vogelpi commented May 7, 2020

I updated the gate generators based on yesterdays discussion. Also the local, high-bandwidth PRNG is now accounted for.

@vogelpi vogelpi force-pushed the aes-v1-bronze branch 5 times, most recently from c1ac16a to c54a0bb Compare May 8, 2020 15:56
@vogelpi
Copy link
Contributor Author

vogelpi commented May 8, 2020

Update: I also added the alert handler interface here (split out from #2152).

@eunchan
Copy link
Contributor

eunchan commented Jun 8, 2020

So, whether csrng or aes is merged first, I think eventually csrng_pkg in this PR should be removed . right? :)

@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 8, 2020

Yep, the csrng_pkg.sv in this PR should eventually be removed. The same also holds for the keymgr_pkg.sv.

@mwbranstad
Copy link
Contributor

@vogelpi why do you think the csrng_pkg will be removed? I would expect that it will not be removed.

@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 8, 2020

@vogelpi why do you think the csrng_pkg will be removed? I would expect that it will not be removed.

My understanding is that there will be a PR for the csrng first that contains the crsng_pkg.sv (because the struct definitions currently contained in there, are csrng-specific but aes uses them, probably like other modules that connect to csrng). This is similar to the altert handler interface definitions.

@imphil
Copy link
Contributor

imphil commented Jun 8, 2020

Since we have multiple blocks depending on the CSRNG interface as defined in (a variant of) csrng_pkg.sv (bignum is another example, see #2393), I'd appreciate if we can get the interface definition into OT master as soon as the interface signals have stabilized enough (which seems to be the case already?). The actual implementation and all of that can then come at a later point.

@mwbranstad
Copy link
Contributor

Since there was some urgency to get the csrng package into the v1-bronze level, that is all that has been promoted so far. At this point, indeed a separate PR into the master will be done as soon at the related markdown documentation has matured and will be submitted under the same PR.

@martin-lueker
Copy link
Contributor

martin-lueker commented Jun 8, 2020

Hi @mwbranstad, @imphil: I can certainly see the reason for urgency. However, I think our recent experience has shown that trying to push documentation and RTL in at the same time is a recipe for live revisions and general heartburn ;) . I'm not sure that approach is the best means of effective collaboration.

If possible I'd like to try to keep the next CSRNG PR compact, and really keep the comments focused. I think starting with the doc has to come first. In fact, I could start by migrating my own (background/non-technical) documentation contributions to a MD PR ASAP (which likely means starting tomorrow afternoon). @mwbranstad this would leave you with just the actual IP documentation.

@imphil: I don't know if you can recommend any other way to better move through the PR flow for priority items, but we're truly more than willing to take suggestions.

@imphil
Copy link
Contributor

imphil commented Jun 8, 2020

Thanks @mwbranstad and @martin-lueker for your comments.

I understand that the CSRNG documentation and implementation are rather tricky and large beasts, so things will take time -- no worries about that.

What I'm trying to figure out if we can split out some of that work in a preliminary form that will help us move forward with other IP blocks.

I'm missing one critical piece of information to be able to suggest something more concrete: how stable is the CSRNG interface at this point? Do you expect some small changes like adding auxiliary signals or renamings, or a major overhaul of the interface?

If the interface proposed by Pirmin in https://github.com/vogelpi/opentitan-1/blob/aes-v1-bronze/hw/ip/aes/rtl/csrng_pkg.sv is roughly the one you're aiming to use, I'd be fine with a pull request which puts this file into the hw/ip/csrng/rtl folder, even if there is no documentation or other implementation next to it. This way we can start using the interface definitions in AES, Bignum, and other blocks which need random numbers without duplicating code. You can of course still revise the interface freely as the spec matures, committing things doesn't mean they are frozen.

Please let me know if there's anything I can help with.

@mwbranstad
Copy link
Contributor

@imphil trying to communicate the current plan for the csrng interface. The version going into the v1-bronze is what I would consider near done. The version that the pointer to the aes/rtl is really not what the csrng app interface will be. I have been meaning to get with @vogelpi to discuss this, but that has not happened yet. While this latest version gets moved into v1-bronze, I was planning on a PR for this same package plus other supporting documentation into OT master for a normal review. This is expected to happen fairly soon. This should supply all of the information needed to plan for how to use the interface, and as anticipated, discuss what additional support might be needed.

@imphil
Copy link
Contributor

imphil commented Jun 9, 2020

@mwbranstad sounds good, thanks!

Comment on lines 40 to 46
{ name: "entropy",
type: "req_rsp",
act: "req",
package: "csrng_pkg",
struct: "csrng_entropy",
width: "1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please take a look at the csrng interface on v1-bronze and revise the interface accordingly? I assume AES talks to CSRNG directly not to Entropy source. the new interface is csrng_pkg::csrng_req/rsp_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eunchan for pinging me. If I understand this correctly, the csrng_req/rsp_t structs in the new csrng_pkg.sv actually describe the more advanced application interface to be used by e.g. the key manager or the rng distribution network. Peripherals such as aes and otbn will not interface to the csrng directly but via the rng distribution network instead and thus use a simplified interface.

It would be great if @mwbranstad or @martin-lueker could clarify this please.

@mwbranstad
Copy link
Contributor

What @vogelpi describes is accurate. @eunchan are meeting soon to sort this out.

@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 23, 2020

What @vogelpi describes is accurate. @eunchan are meeting soon to sort this out.

Thanks @mwbranstad for the quick feedback!

@tjaychen
Copy link

tjaychen commented Jun 23, 2020 via email

The csrng peripheral interface/rng distribution network interface is
not yet defined. This task is tracked in lowRISC#2693.

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
@vogelpi
Copy link
Contributor Author

vogelpi commented Jul 13, 2020

Update:

@vogelpi vogelpi marked this pull request as ready for review July 13, 2020 13:32
@vogelpi vogelpi changed the title WIP [aes] Add intermodule connections and estimated gates for hardening [aes] Add intermodule connections and estimated gates for hardening Jul 13, 2020
@vogelpi vogelpi merged commit 98dfec4 into lowRISC:v1-bronze Jul 13, 2020
@vogelpi vogelpi deleted the aes-v1-bronze branch July 15, 2020 07:57
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.

None yet

6 participants