Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Espressif HW Improvements #6624

Merged
merged 8 commits into from
Sep 19, 2023
Merged

Conversation

gojimmypi
Copy link
Contributor

Description

This update improves multi-chip support of Espressif ESP32 devices.

There are significant improvements to the hardware acceleration for the ESP32 and ESP32-S3. Provides more graceful fallback to SW for other chipsets.

Adds ESP32-S3 conditional SW fallback for AES192 (not supported in HW), fixing #6375.

Fixes #6028 regarding negative big num values with OpenSSL.

Most importantly: Fixes #6205 and renders #6286 moot as this PR now properly supports all key lengths.

The root cause of the RSA signature problem was HW math errors. I have new math tests coming up in a subsequent PR. The tests would cause the wolfcrypt test to fail with current code in production on the ESP32.

Part of the math problem was the lack of errata mitigation for the ESP32. See esp32_errata_en.pdf.

There was also a problem with TFM handling of Espressif HW results as related to zero padding and lengths. TFM is not changed, but the Espressif results are cleaned up to address #6382 and #6380.

This PR also refactors and cleans up the code contributed from #5950.

There's full support for ESP-IDF v5.0 as noted in #5909.

New debugging code has been added to more readily observe operand overwriting as noted in #6359. This is an expected condition in most (or all?) situation. For example for Z = X x Y: Z = X x Z. (the pointer to Y is the same as Z). Comments added to be aware when falling back to SW after HW function called.

The fp bit num references were all updated to mp in preparation for using HW acceleration outside of TFM.

The Montgomery Math initialization for HW was cleaned up and implemented to be multi-chip aware.

The SINGLE_THREADED or multi-threaded environment has been improved for HW math.

Math HW acceleration code in esp32_mp.c was refactored to have fewer in-function return statements.

A new "hard bottom" for some math functions on the ESP32 classic was implemented. This is turned on by default. See ESP_PROHIBIT_SMALL_X. Although unlikely to be used in the real world cryptography, some small operands (e.g. <= 8 bits) were observed to not compute properly in HW. There's a conditional hard bottom for Y operands, disabled by default.

RSA Key sizes up to 4096 bits are now supported. HW multiplication math falls back to software for keys larger than over 2048 bits.

Fixed the used operand allowed lengths for the ESP32, limited to four operand lengths of N ∈ {512, 1024, 1536, 2048}.

Individual math HW acceleration can now be turned on and off with NO_WOLFSSL_ESP32_CRYPT_RSA_PRI_MP_MUL', NO_WOLFSSL_ESP32_CRYPT_RSA_PRI_EXPTMOD, and NO_WOLFSSL_ESP32_CRYPT_RSA_PRI_MULMOD. See esp32-crypt.h header comments.

Fixed proper handling of negative numbers when WOLFSSL_SP_INT_NEGATIVE or USE_FAST_MATH is detected.

The HW_MATH_ENABLED can be used to determine is any of the hardware math functions have been enabled.

Hardware usage metrics can optionally be observed by turning on WOLFSSL_HW_METRICS.

A variety of other cleanup and improvement updates were made.

This is certainly not the final-and-done version, but is a good release candidate and foundation for future improvements.

See #6234 for Espressif Roadmap and related updates.

Fixes zd# n/a

Testing

How did you test?

Tested on a dozen different ESP32, ESP32-S3, ESP32-C2, ESP32-C3 devices. The wolfcrypt tests were modified to run continuously in a loop.

Of interest to @PaulMartinsen and others interested in key generation and signing is the key test app. The ESP32 generates a certificate, and the inspect_test.sh script signs and inspects it for validity. See the various key lengths in user_settings.h.

Run the ESP32 key_test app, and copy the output to the output/test_request.crt file. See inspect_test.sh#L3

Note that for key sizes larger than 2048 the ESP_HW_RSAMAX_BIT and ESP_HW_MULTI_RSAMAX_BITS values will need to be adjusted. (and possibly ESP_RSA_TIMEOUT_CNT and PSRAM settings).

Most of the time seems to be finding a big prime number. Once found, the math is computed rather quickly, even when falling back to wolfcrypt SW math for multiplication over 64 words long.

#define ESP_HW_RSAMAX_BIT           8192
#define ESP_HW_MULTI_RSAMAX_BITS    4096

Note that the HW acclerator only accepts math multiplication operands up to 64 words, so key sizes over 2048 will not fully benefit from from all the RSA HW accleration.

When successful, the output should look something like this:

gojimmypi:/mnt/c/workspace/wolfssl-gojimmypi/IDE/Espressif/ESP-IDF5/examples/wolfssl_server_6205
$ ./inspect_test.sh
Signing Certificate:
-rwxrwxrwx 1 gojimmypi gojimmypi 1037 Jul 15 11:02 ./output/test_request.crt

-----BEGIN CERTIFICATE REQUEST-----
MIICujCCAaICAQAwdTELMAkGA1UEBhMCTloxEDAOBgNVBAgMB1dhaWthdG8xETAP
BgNVBAcMCEhhbWlsdG9uMSAwHgYDVQQKDBdCbHVlIExlYWYgU29mdHdhcmUsIEx0
ZDEfMB0GA1UEAwwWTHVtb3MgSFcsIFNOID0gQkxTLTAwMjCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBALu5O2UjWgVfbRmLCRa8993ulyDmOHk1YrLqyHqv
cRSGmwhrqIOT3/xvJHKSAXN+bUVy3cZIeUr5bytLUf3dRcTamWRn4iHqtpbNS9ub
EkepC2fQe6CwVvGBsS4P4aX3gWY+cQvCvFzqiMyIbdn5ru5ihAdZHfFOz1nKMa9+
rBTVsjSg1dekSwqEQ0QxsTauzlxYrNJ4AzAKHy5VvcdSoSWmMijJSe6GbBcEd7ID
QfaJkMDLYguWOaFKKYQFt+IA1dmNlGQ7nFbSdC4475JeyDXKoIlOtg49y6dIKwO3
hqIK/bagf7KS8D90/HFZpPmI9GCIaWJONN1QIpzH3b9gi6ECAwEAAaAAMA0GCSqG
SIb3DQEBCwUAA4IBAQAEhRbvH6AxuuoYTvyVti25lpFoPLYlsS/oPKUpRd7tQw7v
mhsbp94blFVYt2sg/NsoBVn45UVzZUHCEPfe4A1o9yAE4eFsL95IsuPebZQ1zmYJ
riXxkTnRS9BBnIqxAjljAd2gq3WUDUpBNiZCWmPu4wr8+VA3UGn+itFnWYNXeYWo
RG2293CtSBj9IKK1HiMDDEDgo6sOKnL2UhdcEJ9mBA57gJKn7XQ+GQON8K3xrWgi
qbtzdUsXTbuAE7TxZfc0F2729mihyEvpqyioOhmFNiEJ1Go1fcpx3mHBq7ycJeLX
SUD0HnlijuW0trdVmpt7465h2JeYa25WUQIaZT5b
-----END CERTIFICATE REQUEST-----
    0:d=0  hl=4 l= 698 cons: SEQUENCE
    4:d=1  hl=4 l= 418 cons: SEQUENCE
    8:d=2  hl=2 l=   1 prim: INTEGER           :00
   11:d=2  hl=2 l= 117 cons: SEQUENCE
   13:d=3  hl=2 l=  11 cons: SET
   15:d=4  hl=2 l=   9 cons: SEQUENCE
   17:d=5  hl=2 l=   3 prim: OBJECT            :countryName
   22:d=5  hl=2 l=   2 prim: PRINTABLESTRING   :NZ
   26:d=3  hl=2 l=  16 cons: SET
   28:d=4  hl=2 l=  14 cons: SEQUENCE
   30:d=5  hl=2 l=   3 prim: OBJECT            :stateOrProvinceName
   35:d=5  hl=2 l=   7 prim: UTF8STRING        :Waikato
   44:d=3  hl=2 l=  17 cons: SET
   46:d=4  hl=2 l=  15 cons: SEQUENCE
   48:d=5  hl=2 l=   3 prim: OBJECT            :localityName
   53:d=5  hl=2 l=   8 prim: UTF8STRING        :Hamilton
   63:d=3  hl=2 l=  32 cons: SET
   65:d=4  hl=2 l=  30 cons: SEQUENCE
   67:d=5  hl=2 l=   3 prim: OBJECT            :organizationName
   72:d=5  hl=2 l=  23 prim: UTF8STRING        :Blue Leaf Software, Ltd
   97:d=3  hl=2 l=  31 cons: SET
   99:d=4  hl=2 l=  29 cons: SEQUENCE
  101:d=5  hl=2 l=   3 prim: OBJECT            :commonName
  106:d=5  hl=2 l=  22 prim: UTF8STRING        :Lumos HW, SN = BLS-002
  130:d=2  hl=4 l= 290 cons: SEQUENCE
  134:d=3  hl=2 l=  13 cons: SEQUENCE
  136:d=4  hl=2 l=   9 prim: OBJECT            :rsaEncryption
  147:d=4  hl=2 l=   0 prim: NULL
  149:d=3  hl=4 l= 271 prim: BIT STRING
  424:d=2  hl=2 l=   0 cons: cont [ 0 ]
  426:d=1  hl=2 l=  13 cons: SEQUENCE
  428:d=2  hl=2 l=   9 prim: OBJECT            :sha256WithRSAEncryption
  439:d=2  hl=2 l=   0 prim: NULL
  441:d=1  hl=4 l= 257 prim: BIT STRING
Certificate request self-signature ok
subject=C = NZ, ST = Waikato, L = Hamilton, O = "Blue Leaf Software, Ltd", CN = "Lumos HW, SN = BLS-002"
We need a private key to sign with, use -key or -CAkey or -CA with private key
Certificate request self-signature ok
subject=C = NZ, ST = Waikato, L = Hamilton, O = "Blue Leaf Software, Ltd", CN = "Lumos HW, SN = BLS-002"
Certificate request self-signature ok
subject=C = NZ, ST = Waikato, L = Hamilton, O = "Blue Leaf Software, Ltd", CN = "Lumos HW, SN = BLS-002"
We need a private key to sign with, use -key or -CAkey or -CA with private key

Also tested with:

make clean && make && ./wolfcrypt/test/testwolfcrypt

See #6234 for Espressif Roadmap and related updates.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@gojimmypi gojimmypi self-assigned this Jul 16, 2023
@gojimmypi gojimmypi marked this pull request as draft July 16, 2023 00:05
@gojimmypi gojimmypi requested review from dgarske and bandi13 July 16, 2023 00:54
@gojimmypi gojimmypi marked this pull request as ready for review July 16, 2023 00:54
@gojimmypi gojimmypi linked an issue Jul 16, 2023 that may be closed by this pull request
@gojimmypi gojimmypi force-pushed the Espressif_HW_Math branch from de3a5e4 to 3bc952a Compare July 18, 2023 15:50
@gojimmypi
Copy link
Contributor Author

I'm converting this to draft while I investigate the PRB failure after merging with upstream conflict, as well as an observed TLS client/server problem that may or may not be related.

@gojimmypi gojimmypi marked this pull request as draft July 18, 2023 18:10
@gojimmypi gojimmypi force-pushed the Espressif_HW_Math branch from 3bc952a to 33dd5b7 Compare July 18, 2023 19:24
@@ -879,7 +904,7 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash)

XMEMCPY(hash, (byte *)&sha->digest[0], WC_SHA_DIGEST_SIZE);

(void)InitSha(sha); /* reset state */
ret = InitSha(sha); /* reset state */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be stepping on another error code? Not sure this is the right thing to do. Might let ShaFinal() return a success code, where in fact it failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that's excellent! thank you! that at least solved a different problem I was working on regarding TLS client/server. Indeed the return code was being stomped on when the HW was busy and needed to fall back to SW.

Not sure it will fix the current PRB problem that was previously passing. The only change I made was to AES here after @dgarske added the !defined(WOLFSSL_SILABS_SE_ACCEL) upstream.

The aes.c logic I had earlier may have been wrong and had been updated in this PR squash. @dgarske please confirm your SILABS is still working as desired with my changes.

@gojimmypi gojimmypi force-pushed the Espressif_HW_Math branch from 33dd5b7 to 25d1d50 Compare July 18, 2023 21:00
@PaulMartinsen
Copy link

Really looking forward to this update. Thanks for all the hard work @gojimmypi & all.

@gojimmypi gojimmypi force-pushed the Espressif_HW_Math branch from 25d1d50 to c18fb50 Compare July 18, 2023 21:24
@gojimmypi
Copy link
Contributor Author

Clarification: This PR includes SW support for AES-192 on the ESP32-S3 which has AES acceleration, just not for AES-192.

When defining NO_AES_192 in user_settings.h (right screen snip, below) on the ESP32-S3 all of the NEED_AES_TABLES are also disabled (left screen snip, below), and there will be no HW and no SW for just AES-192. Other AES acceleration for AES-128 and AES-256 will be available be default.

To completely disable all AES HW acceleration on the ESP32, define NO_WOLFSSL_ESP32_CRYPT_AES.

To completely disable all HW and all SW AES, define NO_AES.

image

@gojimmypi gojimmypi requested a review from bandi13 July 18, 2023 22:07
@gojimmypi gojimmypi marked this pull request as ready for review July 18, 2023 22:11
@gojimmypi gojimmypi removed the request for review from wolfSSL-Bot August 15, 2023 20:50
IDE/Espressif/ESP-IDF/user_settings.h Show resolved Hide resolved
wolfcrypt/src/aes.c Outdated Show resolved Hide resolved
wolfcrypt/src/port/Espressif/esp32_mp.c Outdated Show resolved Hide resolved
wolfcrypt/src/port/Espressif/esp32_mp.c Show resolved Hide resolved
wolfcrypt/src/sha512.c Outdated Show resolved Hide resolved
wolfcrypt/src/tfm.c Outdated Show resolved Hide resolved
wolfssl/wolfcrypt/port/Espressif/esp32-crypt.h Outdated Show resolved Hide resolved

/* exit codes to be used in tfm.c, sp_int.c, integer.c, etc. */
/* TODO what numbers do we really want to use? */
#define MP_HW_ERROR (-106) /* hardware error, consider SW fallback */
Copy link
Contributor

Choose a reason for hiding this comment

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

can WC_HW_WAIT_E and WC_HW_E be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, good suggestion. The WC_HW_E is fairly generic, whereas the MP_HW_ERROR is specific to the math acceleration.

I'm ok with either. Which do you prefer?

This is related to the above code review comment and #6565

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be using the error-crypt.h errors over making a new section for errors that need to be maintained and searched out when comparing return values. Additional errors could be appended to that list if needed. For more verbose error messages though please use the WOLFSSL_MSG() debug macros. For example we have a lot of sections of code that can trigger a BAD_FUNC_ARG error or MEMORY_E, which is pretty generic, and then the issue can be debugged farther in debug mode with additional WOLFSSL_MSG() logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ESP specific fallback error messages can stay here in esp32-crypt.h. Thanks for looking for which error messages could be reused from error-crypt.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken your suggestion for WC_HW_E instead of MP_HW_ERROR and WC_HW_WAIT_E instead of MP_HW_BUSY.

I'll keep MP_HW_FALLBACK that is more of a state hint than an error (e.g. "all is ok, but you need to fall back to SW").

I'll also keep MP_HW_VALIDATION_ACTIVE that is only useful during debugging. This too is an indication that a SW calc is explicitly needed to compare to a prior HW calc for comparison.

Both will remain in error-crypt.h with addition comments regarding usage.

/* Compare MATH_INT_T A to MATH_INT_T B
* During debug, the strings name_A and name_B can help
* identify variable name. */
int esp_mp_cmp(char* name_A, MATH_INT_T* A, char* name_B, MATH_INT_T* B);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should control the visibility of these API, if used only in the wolfSSL library than please use WOLFSSL_LOCAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm. Interesting. Good idea. Do you think this is something an end user would ever want to use, or is it just for our own internal diagnostics and testing?

I think I will make it WOLFSSL_LOCAL until someone requests it, eh?

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to error on the side of WOLFSSL_LOCAL. Please do this though for all functions in the .h file that are only used locally. Keeping a clear boundary of what are public API and private API is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I suppose in theory someone may want to call some function such as a stand-alone esp_mp_mul() operation, but I think that would be rare, if ever. We could address that in a future request. I've flagged everything in esp32-crypt.h as WOLFSSL_LOCAL since the intention is really for acceleration to be used only in wolfcrypt.

This will be included when I push my pending changes.

@@ -2367,8 +2414,14 @@ static WARN_UNUSED_RESULT WC_INLINE word32 PreFetchTd4(void)
#endif

/* Software AES - ECB Decrypt */
#ifdef NEED_AES_HW_FALLBACK
Copy link
Contributor

Choose a reason for hiding this comment

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

can we follow the pattern of AESNI and FREESCALE_LTC here where only the existing NEED_AES_TABLES macro is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. Do you mean to use NEED_AES_TABLES instead of NEED_AES_HW_FALLBACK?

Let me clarify my intentions. The issue here is only with the ESP32-S3, and not the classic ESP32.

The classic ESP32 has all AES algos implemented in hardware. I see how that could follow the FREESCALE_LTC pattern.

However, the ESP32-S3 only has partial AES hardware acceleration implementation. It is missing AES-192. See #6375

Thus for the ESP32, it is all or nothing: either we need AES tables for SW, or not when we use HW.

For the ESP32-S3, we may need tables. So depending on the specific AES (e.g. AES192 enabled), we need both a hardware and software algo enabled. The NEED_AES_HW_FALLBACK is used to indicate this.

For example, here we check NEED_AES_HW_FALLBACK and if enabled, we check if aes parameter is a supported key length for hardware, otherwise fall back to software:

    #ifdef NEED_AES_HW_FALLBACK
        if (wc_esp32AesSupportedKeyLen(aes)) {
            ret = wc_esp32AesDecrypt(aes, inBlock, outBlock);
        }
        else {
            ret = wc_AesDecrypt_SW(aes, inBlock, outBlock);
        }
    #else
        /* if we don't need fallback, always use HW */
        ret = wc_esp32AesDecrypt(aes, inBlock, outBlock);
    #endif

If that was not clear, perhaps I could use a more intuitive name?

It could be I completely misunderstood. :) If so, please clarify. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Am hoping we can get away from having the ifdef around the function name. Either by having the esp function calls instead be in the wc_AesDecrypt() function or by following the wc_AesGcmDecrypt of calling a AES_GCM_decrypt_C function like with AESNI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I've pushed some of the requested changes, but the AES update was a bit more complicated.

My WIP is over on my dev branch. I did get rid of the ifdef around the function name, for example here:

    #if defined(WOLFSSL_ESPIDF) && defined(NEED_AESCBC_HW_FALLBACK)
        if (wc_esp32AesSupportedKeyLen(aes)) {
            ESP_LOGV(TAG, "wc_AesCbcEncrypt calling wc_esp32AesCbcEncrypt");
            return wc_esp32AesCbcEncrypt(aes, out, in, sz);
        }
        else {
            ESP_LOGW(TAG, "wc_AesCbcEncrypt unsupported keylen = %d, "
                          "falling back to SW.", aes->keylen);
        }
    #endif

software algo follows...

It needs some more polishing and testing. I expect to be able to update this PR in the coming week. cc: @PaulMartinsen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @JacobBarthelmeh, I pushed the updates that should address the last of the code review comments. In particular I'm no longer renaming function for software fallback.

Let me know how things are looking. Thank you.


ESP_EM__PRE_DPORT_WRITE;
DPORT_INTERRUPT_DISABLE();
for (int i=0; i < wordSz; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't declare int i in for. Please put declaration at top of function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Done.

#ifdef DEBUG_WOLFSSL
MATH_INT_T rinv2[1];
MATH_INT_T M2[1];
mp_copy(M, M2); /* copy (src = M) to (dst = M2) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Mixed declaration and code. Please put all variable declarations at top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to top.

/* Y (left-extend)
* Accelerator supports large-number multiplication with only
* four operand lengths of N ∈ {512, 1024, 1536, 2048} */
int left_pad_offset = maxWords_sz << 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mixed declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

name_A, name_B);
}
else {
// esp_show_mp(name_A, A);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove or use #if 0 or C style comment /* */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped in DEBUG_WOLFSSL. This condition is unlikely to occur unless there's a math problem; will be handy for future HW work, e.g. RISC-V.

@@ -2158,6 +2251,17 @@ static int _fp_exptmod_ct(fp_int * G, fp_int * X, int digits, fp_int * P,
#ifdef WOLFSSL_SMALL_STACK
XFREE(R, NULL, DYNAMIC_TYPE_BIGINT);
#endif

#if defined(DEBUG_WOLFSSL) && defined(WOLFSSL_ESPIDF)
/* TODO consider moving / removing this */
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps remove this? Not adding much value.

Copy link
Contributor Author

@gojimmypi gojimmypi Sep 18, 2023

Choose a reason for hiding this comment

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

Agreed. I address this now in the esp_clean_result().

@gojimmypi
Copy link
Contributor Author

Hi @dgarske thanks for the code review; I've applied the suggested changes, addressed a few similar instances, and applied other minor polishing changes. Tested on ESP32, ESP32-S3 and ESP32-C6 for ESP-IDF v5.1

Note there's unresolved problem, unrelated to this PR, with RISC-V timers (e.g. ESP32-C3/ESP32-C6) for the benchmark app. For some as-yet unknown reason, the compiler cannot seem to find the file for #include "driver/gptimer.h" file, even though it exists and I have an example for the exact same target chipset that is working. This is only problematic for the wolfssl_benchmark app, only on RISC-V targes, and perhaps specific to ESP-IDF v5.1. Related to this, I removed the unneeded benchmark source from the wolfssl_test cmake file. I will address the RISC-V benchmark timer in an upcoming PR.

@gojimmypi
Copy link
Contributor Author

retest this please

@@ -343,7 +356,7 @@ int ShowExtendedSystemInfo(void)
return 0;
}

int esp_ShowExtendedSystemInfo()
WOLFSSL_LOCAL int esp_ShowExtendedSystemInfo()
Copy link
Contributor

Choose a reason for hiding this comment

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

WOLFSSL_LOCAL in the .c does nothing. WOLFSSL_LOCAL sets a hidden visibility but should only be used in the header. If you don't want the function to be used outside the .c then make it static.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please use (void) in the declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I added those only as a reminder that the WOLFSSL_LOCAL is in the header for the respective function.

I will remove them in future PR's and add a comment reminder instead about the visibility decorator tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't harm anything and we aren't 100% elsewhere so I went ahead and accepted it as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants