Skip to content

Implement HW/SW crypto affinity#281

Open
AlexLanzano wants to merge 5 commits intowolfSSL:mainfrom
AlexLanzano:crypto-hw-sw-affinity
Open

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

Conversation

@AlexLanzano
Copy link
Member

@AlexLanzano AlexLanzano commented Feb 4, 2026

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

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?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

For readability and organization sake I believe it's best to keep this separate from wh_test_clientserver.c. From a user perspective it's immediately apparent where how this specific affinity feature is tested instead of having to reason about a generic test function.

src/wh_server.c Outdated
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This would need to be handled in the user's server app, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

not necessarily. Right now it is all managed internally. Use case being, a client connects, sets affinity to SW, does crypto, then disconnects. Currently the affinity state is still implicitly held in the server devId. When the client reconnects, the server context isn't reinitialized, it just sets the connection state in the server context so a polling loop can start processing requests

@bigbrett bigbrett marked this pull request as draft February 10, 2026 15:49
@AlexLanzano AlexLanzano force-pushed the crypto-hw-sw-affinity branch 3 times, most recently from d426556 to b109054 Compare February 10, 2026 19:31
@AlexLanzano AlexLanzano requested a review from Copilot February 10, 2026 19:39
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 29 out of 30 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

wolfhsm/wh_server.h:72

  • Removing devId from whServerCryptoContext is an API/ABI breaking change for downstream users that initialize this public struct (previously .devId = INVALID_DEVID). If this is intentional, it should be called out in release notes/migration docs; otherwise consider providing a compatibility shim (e.g., keep the field behind a deprecated macro) to avoid breaking existing integrations.
#ifndef WOLFHSM_CFG_NO_CRYPTO

typedef struct whServerCryptoContext {
#ifndef WC_NO_RNG
    WC_RNG rng[1];
#endif
} whServerCryptoContext;

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

@AlexLanzano AlexLanzano marked this pull request as ready for review February 10, 2026 19:53
@AlexLanzano AlexLanzano requested a review from bigbrett February 10, 2026 19:53

/* Initialize RNG */
ret = wc_InitRng_ex(crypto->rng, NULL, crypto->devId);
ret = wc_InitRng_ex(crypto->rng, NULL, INVALID_DEVID);
Copy link
Contributor

Choose a reason for hiding this comment

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

per our discussion we talked about removing the global devId, but I'm now realizing that we do still need it to exist for this use case. RNG is the one thing that does make sense to keep global, since initialization is expensive. I think we need to keep the global crypto->devId so the RNG can be initialized with it, but the server stack allocated crypto can instead hold its own "active" devId that is either INVALID_DEVID for SW affinity or use crypto->devId for HW affinity. Key point being, the devId is still globally held, used for RNG, but not otherwise able to have cross-client effects for crypto requested by a client

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think in the long run it may be simpler and more efficient to just handle the affinity state in the client and add an affinity field in the crypto requests that the server processes.

My reasoning for this is that it will remove the need for the this additional request that the server will need to process and the client will need to wait on before sending out a crypto request. For instance, imagine if the client wants to do a crypto op on some periodic input data where the size of the data can vary. To be efficient, the client determines to use HW or SW based the data size. It could be the case that the client is constantly swapping the affinity between crypto requests. It would be much quicker to update this affinity state in the client than requesting the server to change the affinity and waiting.

This also keeps the server side configuration exactly the same. And will simplify HW vs SW request handling in a multi-threaded/multi-client scenario

Thoughts?

Copy link
Contributor

@bigbrett bigbrett Feb 17, 2026

Choose a reason for hiding this comment

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

@AlexLanzano I like removing state from the server when possible, but how do you see this working with the wolfCrypt API though? How would the afinity get passed in when working with wolfCrypt structs? That state needs to be stored somewhere and it isn't going to be in the wolfCrypt algorithm contexts. Do you envision this being held in the client context and then the crypto messaging automatically including that along with every crypto message? Wait, OK I actually think that could work.

I was initially going to push back as it would unfortunately require adding an affinity field for EVERY single crypto request for every single algorithm, however I forgot that I already implemented a "crypto subprotocol" with its own headers in wh_message_crypto.h. So perhaps we could just implement an affinity field to whMessageCrypto_GenericRequestHeader? In this scheme, the "set affinity" call just sets the state in the client context, instead of requiring a round-trip message, and then in each client crypto API call when packing the message to send, it would just stuff the current affinity state from the client context into the generic crypto request header.

Then on the server side, the initial translation of the generic header would make this field available, so we would just need each server-side handler (e.g. _HandleXXX()) to take that as an argument and operate accordingly (e.g. initialize the relevant stack-allocated wolfCrypt struct with the corresponding devId from the server context).

This would eliminate the round-trip back and forth and push the state to the client side. OK, I like this and think it would work...

@bigbrett bigbrett force-pushed the crypto-hw-sw-affinity branch from c72c40c to cbc3ea7 Compare February 18, 2026 00:14
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

Comments