Skip to content

Enforce WOLFSSL_SP_NO_UMAAL with _CORTEX_M_ASM#6793

Merged
SparkiDev merged 1 commit intowolfSSL:masterfrom
danielinux:no_umaal_for_cortexm
Sep 21, 2023
Merged

Enforce WOLFSSL_SP_NO_UMAAL with _CORTEX_M_ASM#6793
SparkiDev merged 1 commit intowolfSSL:masterfrom
danielinux:no_umaal_for_cortexm

Conversation

@danielinux
Copy link
Copy Markdown
Member

Description

Implicitly add WOLFSSL_SP_NO_UMAAL when WOLFSSL_SP_ARM_CORTEX_M_ASM is enabled

Testing

Tested via wolfBoot build

Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Looks good. I'd like @SparkiDev to review also.

Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

The UMAAL exists on M4, not M3.

Here is the macro to tell difference:

M3: #define __ARM_ARCH_7M__ 1
M4: #define __ARM_ARCH_7EM__ 1

Perhaps this logic should go into sp.h?

@dgarske dgarske assigned danielinux and unassigned SparkiDev Sep 20, 2023
@dgarske dgarske force-pushed the no_umaal_for_cortexm branch from 4a6feb3 to e73fcc2 Compare September 20, 2023 22:39
dgarske
dgarske previously approved these changes Sep 20, 2023
Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Over to @SparkiDev

@dgarske dgarske assigned SparkiDev and unassigned danielinux Sep 20, 2023
Comment thread wolfssl/wolfcrypt/sp_int.h Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be:
#if defined(__ARM_ARCH) && (__ARM_ARCH < 8) || defined(ARM_ARCH_7M)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, that would disable it for M4. It could be just #if defined(ARM_ARCH_7M).

These are set exclusively (only one or the other) for each platform:

M3: #define __ARM_ARCH_7M__ 1
M4: #define __ARM_ARCH_7EM__ 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want I could rewrite to:

#if defined(WOLFSSL_SP_ARM_CORTEX_M_ASM) && defined(__ARM_ARCH_7M__)
    #undef  WOLFSSL_SP_NO_UMAAL
    #define WOLFSSL_SP_NO_UMAAL
#endif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK make it just ARM_ARCH_7M like in the your last comment.

@dgarske dgarske requested a review from SparkiDev September 20, 2023 23:38
@dgarske dgarske force-pushed the no_umaal_for_cortexm branch from e73fcc2 to 347394c Compare September 21, 2023 00:11
@dgarske dgarske requested review from SparkiDev and removed request for SparkiDev September 21, 2023 00:11
@dgarske
Copy link
Copy Markdown
Member

dgarske commented Sep 21, 2023

Retest this please

@dgarske dgarske assigned SparkiDev and wolfSSL-Bot and unassigned SparkiDev Sep 21, 2023
@SparkiDev SparkiDev merged commit 9442ec4 into wolfSSL:master Sep 21, 2023
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.

4 participants