Add RNG #37

Wratox · 2025-01-23T13:28:52Z

Added RNG as per #34, it compiles but I haven't been able to test it since I don't have access to a processor yet.

astapleton · 2025-01-23T17:31:00Z

Thanks for the PR! Will you be getting hardware to be able to test this? I'll take more of a look in the next few days to a week, but my first inclination is that it would be nice to use generics instead of macros here. From my cursory look, it seems like it would be possible to create a generic RNG type that takes a numeric type as a parameter. That would probably require a trait bound that informs the compiler that the type implements whatever functionality is needed.

usbalbin · 2025-01-23T17:50:25Z

We have ordered a Nucleo-H533 which should arrive any day now. The plan is to later make a custom pcb with the H523. So we will most likely shift our focus to #38 before we ca do any real tests with this

astapleton · 2025-01-23T18:08:21Z

We have ordered a Nucleo-H533 which should arrive any day now. The plan is to later make a custom pcb with the H523. So we will most likely shift our focus to #38 before we ca do any real tests with this

Great. I created #39 to track what we need to do to integrate STM32H523/STM32H533

astapleton

Thanks! I think this is a good start, but would like to see some seed error recovery, a non-blocking API and removal of the CryptoRng marker trait as mentioned in the comments. I know you brought this directly over from the H7 where it is already in use, but these feel like important changes to me.

examples/blinky_random.rs

src/rng.rs

astapleton · 2025-01-25T19:37:01Z

src/rng.rs

+
+impl Rng {
+    /// Returns 32 bits of randomness, or error
+    pub fn value(&mut self) -> Result<u32, ErrorKind> {


Can you provide a non-blocking version of this function (value_nb) that returns an nb::Result (see nb crate) indicating that it would block if the data is not ready, and then call the non-blocking function using the nb::block! macro here?

Note: nb is being brought in as a dependency in #27. There is also an open comment thread about it's use here: #27 (comment) (I'm hoping to get some closure on that soon!)

astapleton · 2025-01-25T19:38:32Z

src/rng.rs

+                return Err(ErrorKind::ClockError);
+            }
+            if status.secs().bit() {
+                return Err(ErrorKind::SeedError);


There's a seed error recovery process documented in the manual. Would you mind implementing that so when this error occurs, the recovery process can be run? As it stands, if this occurs the RNG would hang and be unrecoverable afterwards, except by using the PAC directly.

Do you want this implemented as its own method on the Rng type which the user then calls whenever they get an ErrorKind::SeedError?

So the general approach I've been taking is to expose non-blocking and blocking functions for each driver (you can see that in the I2C driver, but the SPI and USART drivers I have written and am intending to publish as PRs when the I2C driver lands follow then same pattern). This is intended to facilitate various use cases (interrupts vs spin-loop polling) and async extensions later.

For this case I can actually see 3 behaviours that are worth exposing:

Non-blocking calls ((e.g. value_nb + recover_nb) that return nb::Result and which do not automatically recover from the error

Blocking calls (e.g. value + recover, which probably just call the above functions in a loop) that do not automatically recover from the error

Blocking calls that automatically recover

The last is necessary for the module to be useful for the rand_core trait implementations, as they don't return Results

astapleton · 2025-01-25T19:40:14Z

src/rng.rs

+    }
+}
+
+impl core::iter::Iterator for Rng {


astapleton · 2025-01-29T20:02:13Z

src/rng.rs

+
+#[derive(Debug)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub enum ErrorKind {


nit: I've been using Error for Error enums

usbalbin · 2025-01-30T15:14:58Z

We have now added the three configurations described in the reference manuals and the application note. All three configurations seems to work on the Nucleo-H533 (the nist configuration when the #[cfg] is commented out). The led blinks sporadically in all cases and we have seen no errors yet.

We still have the recovery thing left to do and your other comments

astapleton · 2025-01-30T19:05:42Z

We have now added the three configurations described in the reference manuals and the application note. All three configurations seems to work on the Nucleo-H533 (the nist configuration when the #[cfg] is commented out). The led blinks sporadically in all cases and we have seen no errors yet.

We still have the recovery thing left to do and your other comments

This looks great so far. Really appreciate the effort in implementing the different configurations that ST notes. I realized that I was looking at an older version of RM0492 that did not directly state that configuration B and C were AIS-31 compliant, but Rev 3 does. I'm not really familiar with crypto standards, but according to some cursory internet searching, that does lend some credibility to using those configurations as entropy sources for key generation, and given that the note that they are for both the STM32H503 and STM32H56x/STM32H573, perhaps we can implement the CryptoRng trait for those implementations too? Sorry for the back and forth there. I also think it's worth noting in the documentation that they are AIS-31 compliant according to ST and can be used as entropy sources if that is the standard the user requires.

astapleton · 2025-01-30T19:06:26Z

src/rng.rs

+#[cfg(any(
+    feature = "stm32h562",
+    feature = "stm32h563",
+    feature = "stm32h573"
+))]


There's a nist configuration for the STM32H503 isn't there?

Not sure how we want to do here,
taken from rm0492:

RNG configurations are described in Table 156. Note that only configuration A can be
certified NIST SP800-90B. Refer to Table 157 to select the best configuration for the
application.
• Configuration C can be used when configuration A is not flagged as certified

and according to an4230, stm32h503 configuration A is not certified.

Should we still use configuration A for stm32503 or configuration C?
It's a bit unclear to me atleast.

Oh interesting. I hadn't noticed that. Looking at AN4230, the way I understand it is that those are the recommended settings for NIST verification, but only the processor lines indicated were actually certified by ST, but that you could certify using those settings. Maybe we should include it but note that ST recommends, but did not certify themselves with these settings?

Sounds good to me 👍

astapleton · 2025-01-30T19:23:44Z

src/rng.rs

+        self.htcr().write(|w| unsafe { w.bits(0xAAC7) });
+
+        // Set noise source control register
+        #[cfg(not(feature = "stm32h503"))] // Not available on H503


Huh. I see this register is missing from the PAC, and the SVD, but it is documented in the reference manual. This is probably not a blocker, because the STM32H503 configurations for B, C require that the register stays at default, and if the register is not exposed via the PAC it is hard to modify. I'll submit a PR to stm32-rs to add it, but we don't need to depend on it to merge this, IMO. Can you leave a note in a comment to this effect here?

astapleton · 2025-01-30T19:28:08Z

src/rng.rs

+    /// This uses the register values specified in AN4230 but have not
+    /// performed the verification (buyer beware, users can/should do their own verification)


nit:

Suggested change

/// This uses the register values specified in AN4230 but have not

/// performed the verification (buyer beware, users can/should do their own verification)

/// Note:

/// This uses the register values specified in AN4230 but verification

/// using this HAL has not been performed. Users can/should do their.

/// own verification or request documentation from ST directly.

astapleton requested changes Jan 25, 2025

View reviewed changes

astapleton reviewed Jan 29, 2025

View reviewed changes

Henrik Snöman added 4 commits January 30, 2025 15:59

Added RNG

f5f0392

Added example

e464b34

Fixed some requested changes

ff7b858

Follow example configurations in RM0481 and RM0492

1f4ca8c

Wratox force-pushed the rng branch from 37f93f5 to 1f4ca8c Compare January 30, 2025 15:02

fmt

5c61a6b

astapleton reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RNG #37

Add RNG #37

Wratox commented Jan 23, 2025

astapleton commented Jan 23, 2025

usbalbin commented Jan 23, 2025

astapleton commented Jan 23, 2025

astapleton left a comment

astapleton Jan 25, 2025

astapleton Jan 25, 2025

astapleton Jan 25, 2025

usbalbin Jan 29, 2025

astapleton Jan 29, 2025

astapleton Jan 25, 2025

astapleton Jan 29, 2025

usbalbin commented Jan 30, 2025

astapleton commented Jan 30, 2025

astapleton Jan 30, 2025

Wratox Jan 30, 2025

astapleton Jan 30, 2025

Wratox Jan 30, 2025

astapleton Jan 30, 2025

astapleton Jan 30, 2025 •

edited

Loading

		/// This uses the register values specified in AN4230 but have not
		/// performed the verification (buyer beware, users can/should do their own verification)

-    /// This uses the register values specified in AN4230 but have not
-    /// performed the verification (buyer beware, users can/should do their own verification)
+    /// Note:
+    /// This uses the register values specified in AN4230 but verification
+    /// using this HAL has not been performed. Users can/should do their.
+    /// own verification or request documentation from ST directly.

Add RNG #37

Are you sure you want to change the base?

Add RNG #37

Conversation

Wratox commented Jan 23, 2025

astapleton commented Jan 23, 2025

usbalbin commented Jan 23, 2025

astapleton commented Jan 23, 2025

astapleton 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

usbalbin commented Jan 30, 2025

astapleton commented Jan 30, 2025

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

astapleton Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

astapleton Jan 30, 2025 •

edited

Loading