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.

Sign up for GitHub

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

Jump to bottom

Disable p256-m asm on aarch64 #8126

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Disable p256-m asm on aarch64 #8126

merged 1 commit into from
Aug 29, 2023

Conversation

daverodgman
Copy link
Contributor

Description

The optimised assembly in p256-m does not support aarch64. With this PR, it compiles cleanly for Thumb 1, Thumb 2, arm and aarch64 (tested with mtest MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED thumb1 thumb2 arm aarch64 clang gcc).

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog not previously released
  • backport not in 2.28
  • tests out of scope for this PR - we should add build tests as part of p256-m productisation

@daverodgman
Disable p256-m asm on aarch64
024a3b3 
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
@daverodgman daverodgman added bug needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Aug 29, 2023
tom-cosgrove-arm
tom-cosgrove-arm reviewed Aug 29, 2023
View reviewed changes
3rdparty/p256-m/p256-m/p256-m.c
*/
#if defined(__GNUC__) &&\
defined(__ARM_ARCH) && __ARM_ARCH >= 6 && defined(__ARM_ARCH_PROFILE) && \
( __ARM_ARCH_PROFILE == 77 || __ARM_ARCH_PROFILE == 65 ) /* 'M' or 'A' */
( __ARM_ARCH_PROFILE == 77 || __ARM_ARCH_PROFILE == 65 ) /* 'M' or 'A' */ && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know why this uses ASCII values rather than 'M' and 'A' directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering. I guess it has the theoretical advantage of working on non-ASCII systems, but I don't think there's any non-ASCII systemwhere __ARM_ARCH is defined.

tom-cosgrove-arm
tom-cosgrove-arm approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@daverodgman
Copy link
Contributor Author

@mpg you may want to bring this fix into the upstream project?

gilles-peskine-arm
gilles-peskine-arm approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

3rdparty/p256-m/p256-m/p256-m.c
*/
#if defined(__GNUC__) &&\
defined(__ARM_ARCH) && __ARM_ARCH >= 6 && defined(__ARM_ARCH_PROFILE) && \
( __ARM_ARCH_PROFILE == 77 || __ARM_ARCH_PROFILE == 65 ) /* 'M' or 'A' */
( __ARM_ARCH_PROFILE == 77 || __ARM_ARCH_PROFILE == 65 ) /* 'M' or 'A' */ && \
!defined(__aarch64__)
Copy link
Contributor

Choose a reason for hiding this comment

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

On a related note, I think we should make MUL64_IS_CONSTANT_TIME default on or forced on on 64-bit architectures (defined by ULONG_MAX >= UINT64_MAX, I guess). But this doesn't have to be done now, and it's filed in my list of non-critical improvements which I'll submit as part of my review of p256-m.

3rdparty/p256-m/p256-m/p256-m.c
*/
#if defined(__GNUC__) &&\
defined(__ARM_ARCH) && __ARM_ARCH >= 6 && defined(__ARM_ARCH_PROFILE) && \
( __ARM_ARCH_PROFILE == 77 || __ARM_ARCH_PROFILE == 65 ) /* 'M' or 'A' */
( __ARM_ARCH_PROFILE == 77 || __ARM_ARCH_PROFILE == 65 ) /* 'M' or 'A' */ && \
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering. I guess it has the theoretical advantage of working on non-ASCII systems, but I don't think there's any non-ASCII systemwhere __ARM_ARCH is defined.

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Aug 29, 2023
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Aug 29, 2023
@daverodgman daverodgman removed the needs-ci Needs to pass CI tests label Aug 29, 2023
Merged via the queue into Mbed-TLS:development with commit f3a4168 Aug 29, 2023
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Sep 13, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daverodgman @gilles-peskine-arm @tom-cosgrove-arm