Support 128-bit flash programming units by aidangarske · Pull Request #524 · wolfSSL/wolfHSM

aidangarske · 2026-09-18T20:25:53Z

Add configurable 8-byte and 16-byte flash programming units. Keeps the 8 byte as default. STM32H563 can only program internal flash in 128-bit quadwords this is needed wolfTrust integration

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds configurable 8-byte and 16-byte flash programming units, preserving 8-byte units by default and adapting NVM flash state handling and tests for 128-bit programming targets.

Changes:

Documents and exposes WOLFHSM_CFG_FLASH_UNIT_SIZE with support for 8- and 16-byte units.
Adds a 16-byte whFlashUnit representation and conversion helpers.
Updates NVM flash state encoding and tests to use the configured unit size.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`wolfhsm/wh_settings.h`	Documents the configurable flash unit size.
`wolfhsm/wh_flash_unit.h`	Defines 8-byte and 16-byte flash unit representations.
`src/wh_nvm_flash.c`	Adapts NVM state encoding and decoding to the configurable unit type.
`test/wh_test_nvm_flash.c`	Updates flash and NVM tests for variable unit sizes.
`test/Makefile`	Adds the `FLASH_UNIT_SIZE` build-time override.

Suppressed comments (1)

test/wh_test_nvm_flash.c:1

This description is only correct for the default 8-byte unit size. With FLASH_UNIT_SIZE=16, this read has 14 leading bytes and 7 trailing bytes, with no aligned middle portion, so it does not exercise the claimed three-phase path. Derive the offset and length from WHFU_BYTES_PER_UNIT (or update the test comment and scenario) so the 16-byte configuration also validates an aligned middle read.

/*

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

padelsbach · 2026-09-21T20:50:48Z

+ * 64-bit units. Targets that require 128-bit flash writes may select
+ * WOLFHSM_CFG_FLASH_UNIT_SIZE=16. */
+#ifndef WOLFHSM_CFG_FLASH_UNIT_SIZE
+    #define WOLFHSM_CFG_FLASH_UNIT_SIZE 8


It's not immediately clear if this is bits or bytes. Can you document the units here?

OKay I fixed this the header and configuration documentation now state that WOLFHSM_CFG_FLASH_UNIT_SIZE is measured in bytes.

padelsbach · 2026-09-21T20:59:18Z

+    uint64_t value;
+    uint64_t padding;
+} whFlashUnit;
+    #define WHFU_VALUE(_value) \


Naming of these macros seems inconsistent for such similar logic. When to use WHFU_TO_ vs WHFU_?

Note there is also WHFU_BYTES2UNITS below, which has a different convention.

Ive reworked this with Bills idea as a union with typed u64, u32, and u16 views and removed both scalar conversion macros. WHFU_BYTES2UNITS remains the byte-count-to-unit-count conversion.

padelsbach · 2026-09-21T21:02:08Z

+        else {
+            partition_size = context->cb->PartitionSize(context->flash);
+            if (((partition_size % WHFU_BYTES_PER_UNIT) != 0) ||
+                ((partition_size / WHFU_BYTES_PER_UNIT) <


Should we use <= here to allow the full sized partition?

I think the current < comparison already allows equality. Changing it to <= would reject a partition exactly NF_PARTITION_DATA_OFFSET units long, so I left it unchanged.

padelsbach · 2026-09-21T21:07:24Z

-        } else if ( (part_states[0].status == NF_STATUS_USED) &&
-                    (part_states[1].status == NF_STATUS_USED)) {
+        }
+        else if ((part_states[0].status == NF_STATUS_USED) &&


The both-UNUSED case is not exercised with the new tests. Can you add it?

Also, the preexisting both corrupted case is not covered if you want to add that too

Makes sense, added explicit coverage for both free/erased partitions and both corrupted partitions in the legacy and refactored suites, at both 8-byte and 16-byte unit sizes.

padelsbach · 2026-09-21T21:10:52Z

 #define FLASH_RAM_SIZE (1024 * 1024) /* 1MB */
-#define FLASH_SECTOR_SIZE (4096) /* 4KB */
-#define FLASH_PAGE_SIZE (8) /* 8B */
+#define FLASH_SECTOR_SIZE (4096)     /* 4KB */


Please add the same test cases to test-refactor/ too

Added matching invalid-geometry and initialization-state cases to test-refactor/; the full suite passes at both 8-byte and 16-byte unit sizes.

padelsbach · 2026-09-21T21:19:53Z

+            }
+        }
+
+        if (ret != WH_ERROR_OK) {


Nice addition of this cleanup. But it seems to be missing from the failure paths below too. Could you move this to the bottom of the function to handle them all?

And then in the tests, add calls to wh_NvmFlash_Cleanup on cases where this function returns error.

Thanks, I moved failed initialization cleanup to the common exit path and added cleanup-count checks after each error case in both test suites.

billphipps

Consider an alternate construction?

billphipps · 2026-09-22T13:00:14Z

+    #define WHFU_VALUE(_value) ((whFlashUnit)(_value))
+    #define WHFU_TO_U64(_unit) ((uint64_t)(_unit))
+#elif WOLFHSM_CFG_FLASH_UNIT_SIZE == 16
+typedef struct whFlashUnit_t {


Grumble. I had hoped that we would change whFlashUnit into an array of uint32_t's so that the accessors could rely on at least 32-bit alignment. Initially, I was lazy and avoided adding any alignment attributes to this type by trusting the compiler would always ensure 64-bit was aligned on 64-bit boundaries. At this point, I think we need to have a positive design that allows flash unit sizes to be variable and ensure that accesses to at least the first 2 32-bit values is easy.

Since the minimum size we want to support is 64-bit, let's construct this struct as a union instead. Consider something like:

/* Flash unit size MUST be a power of 2 */ /* Flash units are aligned to 64-bit */ #ifndef WOLFHSM_CFG_FLASH_UNIT_SIZE #define WOLFHSM_CFG_FLASH_UNIT_SIZE 8 #endif #define DIVV_ROUND_UP(v,d) (((v) + (d) - 1) / (d)) #define U64_PER_UNIT DIV_ROUND_UP(WOLFHSM_CFG_FLASH_UNIT_SIZE, 8) #define U32_PER_UNIT DIV_ROUND_UP(WOLFHSM_CFG_FLASH_UNIT_SIZE, 4) #define U16_PER_UNIT DIV_ROUND_UP(WOLFHSM_CFG_FLASH_UNIT_SIZE, 2) typedef union { uint64_t u64[U64_PER_UNIT]; uint32_t u32[U32_PER_UNIT]; uint16_t u16[U16_PER_UNIT]; } whFlashUnit_t; static_assert(sizeof(whFlashUnit_t) >= WOLFHSM_CFG_FLASH_UNIT_SIZE); WHFU_TO_U64(_u) ((_u).u64[0]) /* NVM State accessors now look like */ WHFU_STATE_MAGIC(_u) ((_u).u32[0]); WHFU_STATE_EPOCH(_u) ((_u).u32[1]); WHFU_STATE_CRC16(_u) ((_u).u16[1]);

This does NOT control the variable alignment any better than u64. We'll need either some non-portable attribute code or move to C11 where alignas is supported. Yuck.

Okay this makes sense I reworked it around the union you suggested with the u64, u32, and u16 views and ^2 unit sizes of at least 8 bytes. I kept C90 by adding small compiler specific 8-byte alignment mappings instead of moving to C11. The endian-specific state indexes preserve the existing on flash layout. I tested the 8-byte and 16-byte configurations, then pinned it into wolfTrust and passed the full host suite and m33mu testing

aidangarske requested a review from bigbrett September 18, 2026 20:25

aidangarske assigned bigbrett and aidangarske Sep 18, 2026

Copilot AI lite review requested due to automatic review settings September 18, 2026 20:25

Copilot started reviewing on behalf of aidangarske September 18, 2026 20:41 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Support 128-bit flash programming units

97dbbd7

aidangarske force-pushed the stm32h5-flash-unit branch from 8bad1a4 to 97dbbd7 Compare September 18, 2026 22:02

aidangarske mentioned this pull request Sep 19, 2026

Update wolfHSM pin for STM32H5 flash support wolfSSL/wolfTrust#11

Closed

aidangarske requested a review from padelsbach September 21, 2026 15:15

aidangarske mentioned this pull request Sep 21, 2026

Update wolfHSM pin for STM32H5 flash support wolfSSL/wolfTrust#15

Merged

bigbrett assigned billphipps and unassigned bigbrett and aidangarske Sep 21, 2026

bigbrett requested a review from billphipps September 21, 2026 17:01

padelsbach requested changes Sep 21, 2026

View reviewed changes

billphipps requested changes Sep 22, 2026

View reviewed changes

aidangarske added 2 commits September 22, 2026 10:27

Refine configurable flash unit layout

91818d9

Remove unused flash unit accessor

cb127cb

aidangarske requested review from billphipps and padelsbach September 22, 2026 17:54

aidangarske assigned padelsbach Sep 22, 2026

Conversation

aidangarske commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billphipps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants