Skip to content

Implement HW/SW crypto affinity#281

Open
AlexLanzano wants to merge 1 commit intowolfSSL:mainfrom
AlexLanzano:crypto-hw-sw-affinity
Open

Implement HW/SW crypto affinity#281
AlexLanzano wants to merge 1 commit intowolfSSL:mainfrom
AlexLanzano:crypto-hw-sw-affinity

Conversation

@AlexLanzano
Copy link
Member

Add SetCryptoAffinity API for runtime HW/SW crypto selection

Summary

This PR adds a new client API to dynamically switch between hardware and software cryptographic implementations at runtime. This allows clients to control whether the server uses crypto callbacks (hardware acceleration) or pure software wolfCrypt implementations.

Changes

  • New Client API (wh_client.c, wh_client.h)

    • wh_Client_SetCryptoAffinity() - blocking call
    • wh_Client_SetCryptoAffinityRequest() / wh_Client_SetCryptoAffinityResponse() - async API
  • New Message Types (wh_message_comm.c, wh_message_comm.h)

    • WH_MESSAGE_COMM_ACTION_SET_CRYPTO_AFFINITY action
    • WH_CRYPTO_AFFINITY_SW / WH_CRYPTO_AFFINITY_HW enum values
    • Request/response structures with translation functions
  • Server Handler (wh_server.c, wh_server.h)

    • Added configDevId field to preserve the original configured device ID
    • Handler switches devId between INVALID_DEVID (SW) and configDevId (HW)
  • New Error Code (wh_error.h)

    • WH_ERROR_BADCONFIG (-2010) - returned when HW affinity is requested but no HW crypto was configured
  • Tests (wh_test_clientserver.c)

    • Added _testCryptoAffinity() to verify SW/HW switching behavior
  • Documentation (docs/draft/crypto_affinity.md)

    • API usage guide with examples

Usage

int32_t server_rc;
uint32_t affinity;

// Switch to software crypto
wh_Client_SetCryptoAffinity(client, WH_CRYPTO_AFFINITY_SW, &server_rc, &affinity);

// Switch to hardware crypto (if configured)
wh_Client_SetCryptoAffinity(client, WH_CRYPTO_AFFINITY_HW, &server_rc, &affinity);

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a new client API to dynamically switch between hardware and software cryptographic implementations at runtime. The feature allows clients to control whether the server uses hardware acceleration (via crypto callbacks) or pure software wolfCrypt implementations by modifying the server's devId field.

Changes:

  • New client API with both blocking and async variants for setting crypto affinity
  • Server handler to process affinity change requests with proper validation and error handling
  • New message types and translation functions for client-server communication
  • Comprehensive test coverage including edge cases and error conditions

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
wolfhsm/wh_server.h Added configDevId field to preserve original device ID configuration
wolfhsm/wh_message_comm.h Defined message action, enum values, and request/response structures
wolfhsm/wh_error.h Added WH_ERROR_BADCONFIG error code for configuration failures
wolfhsm/wh_client.h Declared new client API functions with comprehensive documentation
src/wh_server.c Implemented initialization of configDevId and request handler with validation
src/wh_message_comm.c Implemented message translation functions following existing patterns
src/wh_client.c Implemented client APIs with proper error handling and retry logic
test/wh_test_clientserver.c Added comprehensive test coverage for various scenarios
docs/draft/crypto_affinity.md Created API documentation with usage examples and return code reference

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AlexLanzano AlexLanzano force-pushed the crypto-hw-sw-affinity branch from 9949065 to 506f420 Compare February 5, 2026 19:08
@AlexLanzano AlexLanzano requested a review from Copilot February 5, 2026 19:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Contributor

@bigbrett bigbrett left a comment

Choose a reason for hiding this comment

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

Nice work. Some tweaks required and some deeper architectural questions.

```c
enum WH_CRYPTO_AFFINITY_ENUM {
WH_CRYPTO_AFFINITY_SW = 0, // Use software crypto (devId = INVALID_DEVID)
WH_CRYPTO_AFFINITY_HW = 1, // Use hardware crypto (devId = configured value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WH_CRYPTO_AFFINITY_HW = 1, // Use hardware crypto (devId = configured value)
WH_CRYPTO_AFFINITY_HW = 1, // Attempt hardware crypto (devId = configured value)

@@ -0,0 +1,92 @@
# SetCryptoAffinity Client API

The SetCryptoAffinity feature allows a client to control whether the server uses **software** or **hardware** cryptographic implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The SetCryptoAffinity feature allows a client to control whether the server uses **software** or **hardware** cryptographic implementations.
The SetCryptoAffinity feature allows a client to control whether the server should use **software** or attempt to use a **hardware** cryptographic implementation when processing crypto requests.

Also add a blurb describing how it applies to future requests, vs just one. Also mention default values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for flexibility, should we add a GetCryptoAffinity too?

resp.rc = WH_ERROR_OK;
break;
case WH_CRYPTO_AFFINITY_HW:
#ifdef WOLF_CRYPTO_CB
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to check against WOLF_CRYPTO_CB, we error out in wh_settings.h if this isn't defined. All crypto depends on it.

Comment on lines +292 to +295
if (server->crypto->configDevId != INVALID_DEVID) {
server->crypto->devId = server->crypto->configDevId;
resp.rc = WH_ERROR_OK;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing to reason about, not because of anything you are doing, but because we really aren't saying "prefer HW" here, but rather "prefer the devId you were initialized with, vs overriding it to software". I think from the clients perspective this is fine, but could you add a comment here explaining what is going on?

I appreciate the thoughtful handling of if the server's configDevId is INVALID_DEVID though. This will help provide feedback to the client for annoying dumb scenarios like if you have native HW support in wolfCrypt for a platform (e.g. part of the underlying algo implementation, not supplied via cryptoCb) where toggling this doesn't do anything.

I think adding a comment to briefly summarize this and why we are checking would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setting up YANTH (yet another test harness) do you think we could integrate this into wh_test_clientserver.c:whTest_ClientServerSequential()`? Fine to just register the test crypto callback as part of that function as long as it isn't destructive or interferes with other stuff? I don't believe that test runs crypto tests anyway. Could always register it on the fly too if needed.

Ideally we have as few harnesses as possible, and a bunch of generic test drivers that can be run within any harness

case WH_MESSAGE_COMM_ACTION_SET_CRYPTO_AFFINITY: {
whMessageCommSetCryptoAffinityRequest req = {0};
whMessageCommSetCryptoAffinityResponse resp = {0};

Copy link
Contributor

Choose a reason for hiding this comment

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

need to check req_size == sizeof(req) to guard against underflow. If a short packet arrives, the translate function reads past the buffer


static int whTest_CryptoAffinityNoCb(void)
{
int rc = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused

resp.affinity = WH_CRYPTO_AFFINITY_SW;
}
else {
switch (req.affinity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

currently only have 2 cases so perhaps just if() else {}? Otherwise logic on 308 is broken now too.

Comment on lines +271 to +320
case WH_MESSAGE_COMM_ACTION_SET_CRYPTO_AFFINITY: {
whMessageCommSetCryptoAffinityRequest req = {0};
whMessageCommSetCryptoAffinityResponse resp = {0};

wh_MessageComm_TranslateSetCryptoAffinityRequest(
magic, (const whMessageCommSetCryptoAffinityRequest*)req_packet,
&req);

#ifndef WOLFHSM_CFG_NO_CRYPTO
if (server->crypto == NULL) {
resp.rc = WH_ERROR_ABORTED;
resp.affinity = WH_CRYPTO_AFFINITY_SW;
}
else {
switch (req.affinity) {
case WH_CRYPTO_AFFINITY_SW:
server->crypto->devId = INVALID_DEVID;
resp.rc = WH_ERROR_OK;
break;
case WH_CRYPTO_AFFINITY_HW:
#ifdef WOLF_CRYPTO_CB
if (server->crypto->configDevId != INVALID_DEVID) {
server->crypto->devId = server->crypto->configDevId;
resp.rc = WH_ERROR_OK;
}
else {
resp.rc = WH_ERROR_BADCONFIG;
}
break;
#else
resp.rc = WH_ERROR_NOTIMPL;
break;
#endif
default:
resp.rc = WH_ERROR_BADARGS;
break;
}
resp.affinity = (server->crypto->devId == INVALID_DEVID)
? WH_CRYPTO_AFFINITY_SW
: WH_CRYPTO_AFFINITY_HW;
}
#else
resp.rc = WH_ERROR_NOTIMPL;
resp.affinity = WH_CRYPTO_AFFINITY_SW;
#endif

wh_MessageComm_TranslateSetCryptoAffinityResponse(
magic, &resp, (whMessageCommSetCryptoAffinityResponse*)resp_packet);
*out_resp_size = sizeof(resp);
}; break;
Copy link
Contributor

Choose a reason for hiding this comment

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

in light of my other comment on this being part of the comm group, all of this begs the question: should the affinity persist across client comm connect/disconnects? Or should a disconnect trigger a restoration of the affinity to the default config value? I'm leaning towards the latter.

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.

3 participants