addition of uart and uartns550 to swtpm #301

JacobBarthelmeh · 2023-10-04T21:45:40Z

No description provided.

dgarske · 2023-10-04T22:13:23Z

For there UART transport it seems odd to still use the swtpm feature. Is there a reason you didn't set it up as its own transport like SPI/I2C as I did in this PR #286 ?

JacobBarthelmeh · 2023-10-04T23:01:13Z

Not sure I fully understand, is this pulling the uartns550 additions out of swtpm and placing it in its own location. Possibly something under hal/tpm_io_uartns550 or the like? Which re-looking at it may be a better fit for it. No reason for it to be in the swtpm code other than familiarity with that section of code when doing the work, happy to move it to a better spot.

dgarske · 2023-10-04T23:22:58Z

Not sure I fully understand, is this pulling the uartns550 additions out of swtpm and placing it in its own location. Possibly something under hal/tpm_io_uartns550 or the like? Which re-looking at it may be a better fit for it. No reason for it to be in the swtpm code other than familiarity with that section of code when doing the work, happy to move it to a better spot.

I see... you are still using the SWPTM packet framing (locality, TX size, packet, RX size, etc..) but you are adding UART transport instead of socket.
For some reason I was environing this solution looking more like a physical TPM, not SWTPM.
Keep this as-is for now.

dgarske

Looks great and tested it out. Just one minor suggestion.

dgarske · 2024-01-17T22:01:26Z

src/tpm2.c

 /******************************************************************************/

-static THREAD_LS_T TPM2_CTX* gActiveTPM;
+#if defined(WOLFTPM_SWTPM_UARTNS550)


Would prefer a different build option to make this a non-static non-thread local. Maybe WOLFTPM2_PUBLIC_CTX?

gojimmypi · 2024-05-09T22:11:39Z

Hi @dgarske and @JacobBarthelmeh - just wondering what's the status of this PR?

I see there's a useful DEBUG_PRINTF proposed. I almost when down a similar path, but ended up changing my mind.

I have a similar change in the works, but using the ERROR_OUT(rc, exit) pattern found in the wolfCrypt test.

This is all in relation to my #351 adding Espressif TPM support, and in particular the ability to use the colorization of messages, such as the red ESP_LOGE() messages.

Any preference here?

JacobBarthelmeh · 2024-05-09T22:55:29Z

@gojimmypi it's been blocked on me. I went back and rebased it but after the rebase it was not working like I expected. Have not resolved that yet.

Yeah the abstraction of printf's saved a large amount of data when trying to debug in a resource constrained device that had a light version of printf.

My preference is not having ERROR_OUT and goto's.....that a preference though.

gojimmypi · 2024-05-09T23:45:41Z

@JacobBarthelmeh I hear ya on the goto's. I too, have mixed feeling about using them. It can be acceptable if used properly.

@dgarske has quite a few instances of the goto exit pattern, which of course lend themselves to easily adapting to ERROR_OUT.

In any case, I'll hold off on that for now.

Refactor debug printf to new macro to support Xilinx FSBL.

Copilot

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

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (1)

src/tpm2_swtpm.c:133

The macro definitions are redefined at lines 126-133 even though they were already defined conditionally at lines 78-124. This creates duplicate definitions that will override the conditional ones. The second block starting at line 126 should be removed as it conflicts with the properly conditional definitions above. This appears to be leftover code from refactoring.

#ifndef TPM2_SWTPM_HOST
#define TPM2_SWTPM_HOST         "localhost"
#endif
#ifndef TPM2_SWTPM_PORT
#define TPM2_SWTPM_PORT         2321
#endif

static TPM_RC SwTpmTransmit(TPM2_CTX* ctx, const void* buffer, ssize_t bufSz)

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

Copilot · 2026-01-21T23:56:40Z

src/tpm2_swtpm.c

+        int sz = (rspSz < packet->size) ? rspSz : packet->size;
+        DEBUG_PRINTF("Response size: %d\n", rspSz);
+        TPM2_PrintBin(packet->buf, sz);
+        (void)sz;


The variable 'sz' is defined and used within WOLFTPM_DEBUG_VERBOSE but then explicitly marked as unused with (void)sz. This is unnecessary - if the variable is only used for debugging, the void cast makes sense, but the logic could be cleaner by removing the explicit void cast or restructuring to avoid the unused variable warning in a different way.

Suggested change

(void)sz;

Copilot · 2026-01-21T23:56:41Z

configure.ac

 AM_CONDITIONAL([BUILD_AUTODETECT], [test "x$ENABLED_AUTODETECT" = "xyes"])
 AM_CONDITIONAL([BUILD_FIRMWARE], [test "x$ENABLED_FIRMWARE" = "xyes"])
-AM_CONDITIONAL([BUILD_HAL], [test "x$ENABLED_EXAMPLE_HAL" = "xyes" || test "x$ENABLED_MMIO" = "xyes"])
+AM_CONDITIONAL([BUILD_HAL], [test "x$ENABLED_EXAMPLE_HAL" = "xyes"])


The condition has been changed from testing if ENABLED_SWTPM equals "yes" to testing if it does not equal "no". However, on line 498, AM_CONDITIONAL for BUILD_SWTPM uses the same "!= xno" test. This means BUILD_HAL will now be set incorrectly - it was reverted from the logical OR condition with ENABLED_MMIO back to only testing ENABLED_EXAMPLE_HAL. This appears to be an unintended regression that removes the MMIO check that was present on line 504 in the original code.

Suggested change

AM_CONDITIONAL([BUILD_HAL], [test "x$ENABLED_EXAMPLE_HAL" = "xyes"])

AM_CONDITIONAL([BUILD_HAL], [test "x$ENABLED_EXAMPLE_HAL" = "xyes" -o "x$ENABLED_MMIO" = "xyes"])

Copilot · 2026-01-21T23:56:41Z

src/tpm2_swtpm.c

+        if (rc == 0) {
+            if (sendAck) {
+                unsigned char tmpBuf[1] = {0x01};
+
+                sendAck = 0;
+                WOLFTPM_WRITE(ctx->tcpCtx.fd, tmpBuf, 1);
+            }
+
+            if (rxBuffIdx >= rxRemain || rxRemain == 0) {
+                break;
+            }
+
+            if ((timeOut--) <= 0) {
+                rxBuffIdx = 0; /* reset read state */
+                rc = TPM_RC_FAILURE; /* timeout */
+            #if DEBUG_WOLFTPM
+                DEBUG_PRINTF("Connection timed out\r\n");
+            #endif
+                break;
+            }
+            continue; /* keep trying */


The timeout decrement logic is incorrect. The timeout is being decremented on every iteration when rc == 0 (no data received), but since there's no sleep or delay in the loop when rc == 0 (except when sendAck is true), this will cause a busy loop that will count down the timeout extremely quickly, potentially timing out in microseconds instead of the intended TPM2_TIMEOUT_SECONDS. A delay should be added before decrementing the timeout, or the timeout logic should be redesigned to use actual time-based checks.

Copilot · 2026-01-21T23:56:41Z

src/tpm2_swtpm.c

+            #if DEBUG_WOLFTPM
+                DEBUG_PRINTF("Connection timed out\r\n");
+            #endif


The DEBUG_PRINTF directive is placed inside the conditional block, which means it will try to print even when rc is 0 but before checking if rc indicates an actual timeout. This could print a timeout message prematurely. The debug print should be moved outside the conditional check or restructured to only print when an actual timeout has occurred.

Copilot · 2026-01-21T23:56:41Z

src/tpm2_swtpm.c

+    #ifndef TPM2_TIMEOUT_SECONDS
+    #define TPM2_TIMEOUT_SECONDS    7200
+    #endif
+    #define WOLFTPM_WRITE write
+    #define WOLFTPM_READ  read
+    #define WOLFTPM_CLOSE close
+#elif defined(WOLFTPM_SWTPM_UARTNS550)
+    /* Xilinx 16550 UART */
+    #ifndef TPM2_SWTPM_HOST
+        #define TPM2_SWTPM_HOST XPAR_MB0_AXI_UART16550_2_DEVICE_ID
+    #endif
+    #ifndef TPM2_SWTPM_PORT
+    #define TPM2_SWTPM_PORT         115200
+    #endif
+    #ifndef TPM2_TIMEOUT_SECONDS
+    #define TPM2_TIMEOUT_SECONDS    21600


The TPM2_TIMEOUT_SECONDS value varies significantly between implementations: 10 seconds for TCP, 7200 seconds (2 hours) for UART, and 21600 seconds (6 hours) for UARTNS550. While these different timeouts may be intentional due to different communication speeds, such large values (especially 6 hours) seem excessive and could cause applications to hang indefinitely if communication fails. These should be documented or reconsidered to ensure they're appropriate for the use case.

Copilot · 2026-01-21T23:56:42Z

configure.ac

    [ ENABLED_EXAMPLE_HAL=yes ]
    )
-if test "x$ENABLED_EXAMPLE_HAL" = "xyes"
+if test "x$ENABLED_EXAMPLE_HAL" = "xyes" || test "x$ENABLED_MMIO" = "xyes"


The change from "x$ENABLED_EXAMPLE_HAL" = "xyes" on line 438 now includes "|| test x$ENABLED_MMIO = xyes", but line 504 removes the same MMIO check from BUILD_HAL. This creates an inconsistency: WOLFTPM_EXAMPLE_HAL will be defined when MMIO is enabled (line 440), but BUILD_HAL won't be set for MMIO. This will cause build issues where the HAL code is conditionally compiled but not linked.

Suggested change

if test "x$ENABLED_EXAMPLE_HAL" = "xyes" || test "x$ENABLED_MMIO" = "xyes"

if test "x$ENABLED_EXAMPLE_HAL" = "xyes"

Copilot · 2026-01-21T23:56:42Z

src/tpm2_swtpm.c

+
+        /* send ack */
+        if (rc > 0 ) {
+            usleep(500);


The usleep(500) call introduces a fixed 500 microsecond delay after receiving data. This hardcoded delay may not be optimal for all baud rates and could impact performance. Consider making this configurable or calculating it based on the baud rate, or explaining why this specific value was chosen.

Copilot · 2026-01-21T23:56:42Z

.github/workflows/test-uart-swtpm.yml

+        echo "Building with UART device: $CLIENT_PTY"
+        ./configure \
+          --enable-swtpm=uart \
+          --with-wolfcrypt=$PWD/../wolfssl-install


The configure command uses "--with-wolfcrypt=$PWD/../wolfssl-install" but the current working directory is already in the workspace. The path should be "--with-wolfcrypt=$PWD/wolfssl-install" (without the "..") since the wolfssl-install directory is created as a sibling to the current directory, or use an absolute path like "${GITHUB_WORKSPACE}/wolfssl-install" for clarity.

Suggested change

--with-wolfcrypt=$PWD/../wolfssl-install

--with-wolfcrypt=${GITHUB_WORKSPACE}/wolfssl-install

Copilot · 2026-01-21T23:56:42Z

wolftpm/tpm2_types.h

 #ifndef XSTRINGIFY
-#define XSTRINGIFY(s) STRINGIFY(s)
+#ifndef STRINGIFY
 #define STRINGIFY(s)  #s
 #endif
+#define XSTRINGIFY(s) STRINGIFY(s)
+#endif


The nested ifndef for STRINGIFY creates a potential issue. If XSTRINGIFY is already defined but STRINGIFY is not, this code will define STRINGIFY, but XSTRINGIFY won't use this new definition since it's already defined. This breaks the expected relationship between the two macros. The order should be: check if STRINGIFY is defined first, define it if not, then check and define XSTRINGIFY using STRINGIFY.

Copilot · 2026-01-21T23:56:43Z

src/tpm2_swtpm.c

+
+        memcpy(buffer, rxBuff, minSz);
+        if (rxBuffIdx > minSz) {
+            memmove(rxBuff, rxBuff + rxBuffIdx - minSz, rxBuffIdx - minSz);


The memmove operation uses incorrect parameters. The second parameter should be "rxBuff + minSz", not "rxBuff + rxBuffIdx - minSz". When copying the remaining data to the beginning of the buffer after consuming minSz bytes, you should move from position minSz (the start of unconsumed data) to position 0. The current formula "rxBuffIdx - minSz" would calculate the wrong source offset.

Suggested change

memmove(rxBuff, rxBuff + rxBuffIdx - minSz, rxBuffIdx - minSz);

memmove(rxBuff, rxBuff + minSz, rxBuffIdx - minSz);

JacobBarthelmeh self-assigned this Oct 4, 2023

JacobBarthelmeh force-pushed the uart branch from 73a1fe0 to 2a1d768 Compare December 12, 2023 07:41

JacobBarthelmeh force-pushed the uart branch from 2a1d768 to 88007b2 Compare January 17, 2024 20:29

JacobBarthelmeh assigned wolfSSL-Bot and unassigned JacobBarthelmeh Jan 17, 2024

dgarske self-requested a review January 17, 2024 21:58

dgarske requested changes Jan 17, 2024

View reviewed changes

dgarske assigned JacobBarthelmeh and unassigned wolfSSL-Bot Jan 17, 2024

dgarske mentioned this pull request Jan 25, 2024

Added TPM simulator support for UART transport #286

Closed

dgarske force-pushed the uart branch from 395c056 to fc5ff50 Compare April 30, 2024 20:58

dgarske self-requested a review April 30, 2024 20:59

dgarske removed their request for review September 25, 2024 19:43

JacobBarthelmeh marked this pull request as draft December 2, 2025 21:20

JacobBarthelmeh force-pushed the uart branch from fc5ff50 to 38f0ac5 Compare December 2, 2025 21:20

dgarske and others added 2 commits January 21, 2026 15:44

Added TPM simulator support for UART transport (--enable-swtpm=uart).

4b0e13e

Refactor debug printf to new macro to support Xilinx FSBL.

remove duplicate code section after rebase

dc2caed

JacobBarthelmeh force-pushed the uart branch from a4c7b67 to dc2caed Compare January 21, 2026 22:45

JacobBarthelmeh requested a review from Copilot January 21, 2026 22:59

JacobBarthelmeh force-pushed the uart branch from e82a339 to 4821b86 Compare January 21, 2026 23:03

Copilot started reviewing on behalf of JacobBarthelmeh January 21, 2026 23:03 View session

JacobBarthelmeh force-pushed the uart branch from 4821b86 to 05986be Compare January 21, 2026 23:10

Copilot AI reviewed Jan 21, 2026

View reviewed changes

JacobBarthelmeh force-pushed the uart branch from 05986be to 4699d73 Compare January 21, 2026 23:22

JacobBarthelmeh force-pushed the uart branch 3 times, most recently from 5f49d65 to 59b8344 Compare January 21, 2026 23:52

JacobBarthelmeh requested a review from Copilot January 21, 2026 23:53

Copilot started reviewing on behalf of JacobBarthelmeh January 21, 2026 23:53 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

JacobBarthelmeh force-pushed the uart branch 11 times, most recently from 3735ba0 to d813641 Compare January 22, 2026 17:37

add uart CI test

1bc3d1b

JacobBarthelmeh force-pushed the uart branch from d813641 to 1bc3d1b Compare January 22, 2026 17:55

	AM_CONDITIONAL([BUILD_HAL], [test "x$ENABLED_EXAMPLE_HAL" = "xyes"])
	AM_CONDITIONAL([BUILD_HAL], [test "x$ENABLED_EXAMPLE_HAL" = "xyes" -o "x$ENABLED_MMIO" = "xyes"])

	if test "x$ENABLED_EXAMPLE_HAL" = "xyes" \|\| test "x$ENABLED_MMIO" = "xyes"
	if test "x$ENABLED_EXAMPLE_HAL" = "xyes"

	--with-wolfcrypt=$PWD/../wolfssl-install
	--with-wolfcrypt=${GITHUB_WORKSPACE}/wolfssl-install

	memmove(rxBuff, rxBuff + rxBuffIdx - minSz, rxBuffIdx - minSz);
	memmove(rxBuff, rxBuff + minSz, rxBuffIdx - minSz);

addition of uart and uartns550 to swtpm #301

Are you sure you want to change the base?

addition of uart and uartns550 to swtpm #301

Conversation

JacobBarthelmeh commented Oct 4, 2023

Uh oh!

dgarske commented Oct 4, 2023

Uh oh!

JacobBarthelmeh commented Oct 4, 2023

Uh oh!

dgarske commented Oct 4, 2023

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Uh oh!

dgarske Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

gojimmypi commented May 9, 2024

Uh oh!

JacobBarthelmeh commented May 9, 2024

Uh oh!

gojimmypi commented May 9, 2024

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants