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

[prim] xoshiro primitive #7944

Merged
merged 2 commits into from Nov 12, 2021
Merged

[prim] xoshiro primitive #7944

merged 2 commits into from Nov 12, 2021

Conversation

vrozic
Copy link
Contributor

@vrozic vrozic commented Aug 26, 2021

This PR has two commits.
The first commit adds xoshiro256++ primitive PRNG.
The second commit replaces LFSR in OTBN's URNG with xoshiro256++.

Signed-off-by: Vladimir Rozic vrozic@lowrisc.org

@vrozic vrozic requested review from eunchan and tjaychen as code owners Aug 26, 2021
@vrozic vrozic marked this pull request as draft Aug 26, 2021
@vrozic vrozic force-pushed the xoshiro branch 3 times, most recently from c8a5b25 to 1434ea7 Compare Aug 26, 2021
@imphil
Copy link
Contributor

@imphil imphil commented Aug 26, 2021

Thanks for your work on this primitive, Vladimir! As I said in the meeting, can you make sure that there is only a single module in the source file? You can either use functions, or just inline the code into the generate block (they only have a single call site).

Further comments:

  • Please configure your editor to add a newline to the end of the file.
  • Add a core file for the new primitive.
  • Expand the description in the header to include a description of the algorithm, e.g. a link to a paper.

//State output function
assign mid1[k] = unrolled_state[k][191:128];
assign mid2[k] = mid1[k] + (mid1[k] << 2);
assign mid3[k] = {mid2[k][56:0], mid2[k][63:57]};
Copy link
Contributor

@tjaychen tjaychen Sep 7, 2021

Choose a reason for hiding this comment

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

are the selections of mid2 here always supposed to be fixed? OR is it based on the output width? Just curious since it all seems to be around 64 at the moment.

Copy link
Contributor Author

@vrozic vrozic Nov 11, 2021

Choose a reason for hiding this comment

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

Yes, these intermediate signals are always supposed to be 64 bits, regardless the output width.
Xoshiro is originally designed as a 64b PRNG. This implementation unrolls the state-update loop to provide larger output width (always a multiple of 64).

@@ -0,0 +1,91 @@
// Copyright lowRISC contributors.
Copy link
Contributor

@tjaychen tjaychen Sep 7, 2021

Choose a reason for hiding this comment

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

o interesting, so is it fair to say this is meant to be more of a replacement from prim_lfsr than a tacked on post processing stage?

If that's the case, i think it would make sense to create a prim_prng.core, and maybe both these modules can be placed under that core file.

And lastly, do we have any general sense how big this module is compared to prim_lfsr? I think even if we just had some rough data from yosys it would be really helpful to guide people's selection of this, especially against the full prim_lfsr that has both permutation and a non-linear portion.

Copy link
Contributor Author

@vrozic vrozic Nov 11, 2021

Choose a reason for hiding this comment

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

I've done a preliminary yosys area analysis of the module otbn_rnd.sv which includes a PRNG and some additional logic for reseeding the generator and storing the output. The results are:
Version using LFSR + Sboxes: 6.39 kGE
Version using xoshiro256++: 11.64 kGE

So the change adds a little more than 5kGE.

Copy link
Contributor

@tjaychen tjaychen left a comment

this looks pretty good, but i do have a couple of questions.

@vrozic vrozic force-pushed the xoshiro branch 5 times, most recently from 50bb190 to 8a1f7d6 Compare Oct 28, 2021
@vrozic vrozic marked this pull request as ready for review Oct 28, 2021
@vrozic vrozic force-pushed the xoshiro branch 3 times, most recently from 298dcf8 to 4254f7f Compare Oct 29, 2021
@vrozic vrozic requested a review from GregAC Oct 29, 2021
@GregAC GregAC mentioned this pull request Nov 1, 2021
GregAC
GregAC approved these changes Nov 2, 2021
Copy link
Contributor

@GregAC GregAC left a comment

Some small comments but otherwise LGTM, thanks @vrozic

XOR'ed into the PRNG state (connect to zero if this feature is unused).
As the PRNG may jump into the all-zero state (e.g. due to an active attack), the PRNG
state-update function contains a lockup protection which re-seeds the state with
`DefaultSeed` once this condition is detected.
Copy link
Contributor

@GregAC GregAC Nov 2, 2021

Choose a reason for hiding this comment

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

Making the operation of seed_en_i and xoshiro_en_i more explicit would be nice, e.g. explain that when seed_en_i is set the PRNG is reseeded with seed_i and when xoshiro_en_i is set the PRNG advances. Also explain what both being set does.

You could also add a waveform to illustrate using wavedrom: https://wavedrom.com/. The wavedrom description can be inserted directly into the document (e.g. see the AES docs here:

{{< wavejson >}}
{
signal: [
{ name: 'clk', wave: 'p........|.......'},
['TL-UL IF',
{ name: 'write', wave: '01...0...|.......'},
{ name: 'addr', wave: 'x2345xxxx|xxxxxxx', data: 'K0 K1 K2 K3'},
{ name: 'wdata', wave: 'x2345xxxx|xxxxxxx', data: 'K0 K1 K2 K3'},
],
{},
['AES Unit',
{ name: 'Config op', wave: 'x4...............', data: 'DECRYPT'},
{ name: 'AES op', wave: '2........|.4.....', data: 'IDLE DECRYPT'},
{ name: 'KEM op', wave: '2....3...|.4.....', data: 'IDLE ENCRYPT DECRYPT'},
{ name: 'round', wave: 'xxxxx2.22|22.2222', data: '0 1 2 9 0 1 2 3 4'},
{ name: 'key_init', wave: 'xxxx5....|.......', data: 'K0-3'},
{ name: 'key_full', wave: 'xxxxx5222|4.22222', data: 'K0-3 f(K) f(K) f(K) K0-3\' f(K) f(K) f(K) f(K) f(K)'},
{ name: 'key_dec', wave: 'xxxxxxxxx|4......', data: 'K0-3\''},
]
]
}
{{< /wavejson >}}
)

Copy link
Contributor Author

@vrozic vrozic Nov 11, 2021

Choose a reason for hiding this comment

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

Good idea. I've added the more explanation and the waveforms to the document.

end

assign next_xoshiro_state = entropy_i ^ unrolled_state[NumStages];
assign xoshiro_d = (seed_en_i) ? seed_i :
Copy link
Contributor

@GregAC GregAC Nov 2, 2021

Choose a reason for hiding this comment

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

Does this need to be a priority mux?

end

// lockup condition is all-zero
assign lockup = ~(|xoshiro_q);
Copy link
Contributor

@GregAC GregAC Nov 2, 2021

Choose a reason for hiding this comment

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

Is an all zero state a possibility under normal operation?

We should add an output that signals a lockup has occured. If its possible, but rare, it should probably be a recoverable error for OTBN. If it's only possible when an attack is occurring we should trigger a fatal error.

Copy link
Contributor Author

@vrozic vrozic Nov 2, 2021

Choose a reason for hiding this comment

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

All-zero state is possible (but still extremely unlikely) only when input entropy_i for additional entropy mixing is used.
However, in otbn_rnd.sv, this feature is not used (this input is always tied to zero), so reaching the all-zero state is impossible during the normal operation.
So I think that a fatal error would make sense.

Copy link
Contributor

@GregAC GregAC Nov 2, 2021

Choose a reason for hiding this comment

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

Ok for now let's just add an output that indicates a lockup has occurred and we'll add a fatal error based on it with another PR, I'll create an issue to track.

Copy link
Contributor Author

@vrozic vrozic Nov 11, 2021

Choose a reason for hiding this comment

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

OK, I've added the all_zero_o output to the primitive. Currently it is left unused in the otbn_rnd.sv.

hw/ip/otbn/rtl/otbn_pkg.sv Outdated Show resolved Hide resolved
Copy link
Contributor

@msfschaffner msfschaffner left a comment

LGTM overall, just a few small comments.

hw/ip/prim/doc/prim_xoshiro256pp.md Outdated Show resolved Hide resolved
assign lockup = ~(|xoshiro_q);

// check that seed is not all-zero
`ASSERT_INIT(DefaultSeedNzCheck_A, |DefaultSeed)
Copy link
Contributor

@msfschaffner msfschaffner Nov 2, 2021

Choose a reason for hiding this comment

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

Does it make sense to extend this (in a follow up) with more SVAs so we can verify this primitive with FPV?

hw/ip/otbn/rtl/otbn_pkg.sv Outdated Show resolved Hide resolved
hw/ip/otbn/data/otbn.hjson Show resolved Hide resolved
vrozic added 2 commits Nov 11, 2021
Add xoshiro256pp pseudo-random number generator.

Signed-off-by: Vladimir Rozic <vrozic@lowrisc.org>
Replaced OTBN's LFSR-based PRNG with xoshiro256pp.

Signed-off-by: Vladimir Rozic <vrozic@lowrisc.org>
Copy link
Contributor

@msfschaffner msfschaffner left a comment

LGTM from my side, thanks.

@GregAC GregAC merged commit dd3a05b into lowRISC:master Nov 12, 2021
18 checks passed
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

5 participants