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

Add architecture detection macros #8093

Merged
merged 5 commits into from
Sep 13, 2023
Merged

Add architecture detection macros #8093

merged 5 commits into from
Sep 13, 2023

Conversation

yuhaoth
Copy link
Contributor

@yuhaoth yuhaoth commented Aug 18, 2023 •
edited by daverodgman
Loading

Description

There are many place to detect target architecture, this PR add utility macros for this purpose.

relative to: #7078 .
see also :#7384 (comment)
preceding-PR: #7384

PR checklist

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

  • changelog provided, or not required
  • backport not a bugfix - not required
  • tests provided, or not required

@yuhaoth yuhaoth marked this pull request as draft August 18, 2023 09:42
@yuhaoth yuhaoth added needs-work needs-preceding-pr Requires another PR to be merged first labels Aug 18, 2023
@yuhaoth yuhaoth mentioned this pull request Aug 18, 2023
Draft
3 tasks
@yuhaoth yuhaoth force-pushed the pr/add-target-architecture-macros branch from 1e87af5 to 9f2946a Compare August 21, 2023 02:33
@yuhaoth yuhaoth removed the needs-preceding-pr Requires another PR to be merged first label Aug 21, 2023
@yuhaoth yuhaoth force-pushed the pr/add-target-architecture-macros branch 2 times, most recently from 1f75339 to 7610221 Compare August 21, 2023 08:34
@yuhaoth yuhaoth added the component-platform Portability layer and build scripts label Aug 21, 2023
@yuhaoth yuhaoth force-pushed the pr/add-target-architecture-macros branch from 7610221 to 3e7fce7 Compare August 22, 2023 02:15
@yuhaoth yuhaoth added component-crypto Crypto primitives and low-level interfaces 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 and removed needs-work labels Aug 22, 2023
@yuhaoth yuhaoth marked this pull request as ready for review August 23, 2023 02:12
@yuhaoth yuhaoth force-pushed the pr/add-target-architecture-macros branch from 3e7fce7 to 5000ac5 Compare August 23, 2023 09:11
@yuhaoth yuhaoth force-pushed the pr/add-target-architecture-macros branch from 5000ac5 to 782b966 Compare August 23, 2023 09:16
lpy4105
lpy4105 reviewed Aug 24, 2023
View reviewed changes
Copy link
Contributor

@lpy4105 lpy4105 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Several suggested improvements.

As we are adding architecture detection macros globally, it would be good to replace all the architecture checkings with these macros. For example:

mbedtls/library/sha256.c

Line 59 in 41d689f

#if defined(__aarch64__)

It might be out of scope of this PR, so it's fine to do the replacement in a sperate PR.

library/aesni.h Outdated Show resolved Hide resolved
include/mbedtls/build_info.h Outdated Show resolved Hide resolved
library/aesce.h Outdated Show resolved Hide resolved
library/padlock.h Outdated Show resolved Hide resolved
library/aesce.c
@@ -46,7 +46,7 @@

#include "aesce.h"

#if defined(MBEDTLS_HAVE_ARM64)
#if defined(MBEDTLS_ARCH_IS_ARM64)
Copy link
Contributor

Choose a reason for hiding this comment

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

In aesni.c and padlock.c, we guard the implementations with MBEDTLS_XXX_HAVE_CODE, I think we should do it similarly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is addressed in #7078 .

@yuhaoth
Copy link
Contributor Author

yuhaoth commented Aug 28, 2023

Several suggested improvements.

As we are adding architecture detection macros globally, it would be good to replace all the architecture checkings with these macros. For example:

mbedtls/library/sha256.c

Line 59 in 41d689f

#if defined(__aarch64__)

It might be out of scope of this PR, so it's fine to do the replacement in a sperate PR.

Yes. I think it is out of scope. It should be replaced in separate PRs

@yuhaoth
improve various issues
f65f71e 
- duplicate definition
- wrong comments
- redundant include statement

Signed-off-by: Jerry Yu <[email protected]>
lpy4105
lpy4105 approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@lpy4105 lpy4105 left a comment

Choose a reason for hiding this comment

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

LGTM

@gabor-mezei-arm gabor-mezei-arm self-requested a review September 5, 2023 09:10
@yuhaoth yuhaoth removed the needs-reviewer This PR needs someone to pick it up for review label Sep 8, 2023
gabor-mezei-arm
gabor-mezei-arm approved these changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-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

@gabor-mezei-arm gabor-mezei-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, labels Sep 8, 2023
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Sep 13, 2023
Merged via the queue into Mbed-TLS:development with commit f22999e Sep 13, 2023
@yuhaoth yuhaoth deleted the pr/add-target-architecture-macros branch December 6, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabor-mezei-arm gabor-mezei-arm gabor-mezei-arm approved these changes

@lpy4105 lpy4105 lpy4105 approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts
Projects
Non-roadmap pull requests
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yuhaoth @lpy4105 @gabor-mezei-arm @gilles-peskine-arm