[prim] xoshiro primitive #7944

vrozic · 2021-08-26T09:15:29Z

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

imphil · 2021-08-26T12:13:28Z

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.

tjaychen · 2021-09-07T17:41:34Z

hw/ip/prim/rtl/prim_xoshiro256ss.sv

+    //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]};


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.

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).

tjaychen · 2021-09-07T17:43:13Z

hw/ip/prim/rtl/prim_xoshiro256ss.sv

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


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.

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.

tjaychen

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

hw/ip/prim/rtl/prim_xoshiro256pp.sv

GregAC

Some small comments but otherwise LGTM, thanks @vrozic

GregAC · 2021-11-02T12:51:12Z

hw/ip/prim/doc/prim_xoshiro256pp.md

+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.


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:

opentitan/hw/ip/aes/doc/_index.md

Lines 282 to 303 in 3cb62ec

{{< 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 >}}

)

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

GregAC · 2021-11-02T12:53:11Z

hw/ip/prim/rtl/prim_xoshiro256pp.sv

+  end
+
+  assign next_xoshiro_state = entropy_i ^ unrolled_state[NumStages];
+  assign xoshiro_d = (seed_en_i)              ? seed_i          :


Does this need to be a priority mux?

GregAC · 2021-11-02T12:57:49Z

hw/ip/prim/rtl/prim_xoshiro256pp.sv

+  end
+
+  // lockup condition is all-zero
+  assign lockup = ~(|xoshiro_q);


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.

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.

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.

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

msfschaffner

LGTM overall, just a few small comments.

hw/ip/prim/doc/prim_xoshiro256pp.md

msfschaffner · 2021-11-02T19:56:36Z

hw/ip/prim/rtl/prim_xoshiro256pp.sv

+  assign lockup = ~(|xoshiro_q);
+
+  // check that seed is not all-zero
+  `ASSERT_INIT(DefaultSeedNzCheck_A, |DefaultSeed)


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

hw/ip/otbn/data/otbn.hjson

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>

msfschaffner

LGTM from my side, thanks.

vrozic requested review from eunchan and tjaychen as code owners August 26, 2021 09:15

vrozic marked this pull request as draft August 26, 2021 09:15

vrozic force-pushed the xoshiro branch 3 times, most recently from c8a5b25 to 1434ea7 Compare August 26, 2021 09:54

vrozic force-pushed the xoshiro branch from 1434ea7 to 12a9778 Compare September 6, 2021 12:51

tjaychen reviewed Sep 7, 2021

View reviewed changes

vrozic force-pushed the xoshiro branch from 12a9778 to b2b192f Compare September 16, 2021 16:32

msfschaffner mentioned this pull request Sep 16, 2021

Questions about LFSR's for providing high bandwidth moderate-security randomized data. #1920

Closed

vrozic force-pushed the xoshiro branch from b2b192f to d687592 Compare October 26, 2021 12:20

github-actions bot reviewed Oct 26, 2021

View reviewed changes

hw/ip/prim/rtl/prim_xoshiro256pp.sv Outdated Show resolved Hide resolved

vrozic force-pushed the xoshiro branch 5 times, most recently from 50bb190 to 8a1f7d6 Compare October 28, 2021 10:13

vrozic marked this pull request as ready for review October 28, 2021 13:13

vrozic force-pushed the xoshiro branch 3 times, most recently from 298dcf8 to 4254f7f Compare October 29, 2021 09:26

ctopal mentioned this pull request Oct 29, 2021

[otbn,dv] Full URND Implementation in ISS #8962

Merged

vrozic requested a review from GregAC October 29, 2021 12:30

GregAC mentioned this pull request Nov 1, 2021

[otbn] Finish up new PRNG #8992

Closed

GregAC approved these changes Nov 2, 2021

View reviewed changes

GregAC mentioned this pull request Nov 2, 2021

[tracking][otbn] New fatal alert for PRNG lockup #9011

Closed

4 tasks

msfschaffner reviewed Nov 2, 2021

View reviewed changes

vrozic added 2 commits November 11, 2021 12:11

[prim] Add xoshiro256pp primitive.

297d78f

Add xoshiro256pp pseudo-random number generator. Signed-off-by: Vladimir Rozic <vrozic@lowrisc.org>

[otbn] Updated uRNG in OTBN

1c98b6b

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

vrozic force-pushed the xoshiro branch from 4254f7f to 1c98b6b Compare November 11, 2021 12:16

msfschaffner approved these changes Nov 12, 2021

View reviewed changes

GregAC merged commit dd3a05b into lowRISC:master Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[prim] xoshiro primitive #7944

[prim] xoshiro primitive #7944

vrozic commented Aug 26, 2021 •

edited

imphil commented Aug 26, 2021

tjaychen Sep 7, 2021

vrozic Nov 11, 2021

tjaychen Sep 7, 2021

vrozic Nov 11, 2021

tjaychen left a comment

GregAC left a comment

GregAC Nov 2, 2021

vrozic Nov 11, 2021

GregAC Nov 2, 2021

GregAC Nov 2, 2021

vrozic Nov 2, 2021

GregAC Nov 2, 2021

vrozic Nov 11, 2021

msfschaffner left a comment

msfschaffner Nov 2, 2021

msfschaffner left a comment

	{{< 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 >}}

[prim] xoshiro primitive #7944

[prim] xoshiro primitive #7944

Conversation

vrozic commented Aug 26, 2021 • edited

imphil commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjaychen left a comment

Choose a reason for hiding this comment

GregAC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfschaffner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfschaffner left a comment

Choose a reason for hiding this comment

vrozic commented Aug 26, 2021 •

edited