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

Better support for MSVC aarch64 aka ARM64 and ARM64EC #8044

Merged
merged 15 commits into from
Dec 1, 2023
Merged

Better support for MSVC aarch64 aka ARM64 and ARM64EC #8044

merged 15 commits into from
Dec 1, 2023

Conversation

daverodgman
Copy link
Contributor

Description

Enable NEON and unaligned accesses on ARM64 and ARM64EC.

Note: testing has been pretty limited.

PR checklist

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

  • changelog not required - not user-visible
  • backport not required - not a bugfix
  • tests not required - covered by existing

@daverodgman daverodgman marked this pull request as draft August 8, 2023 09:48
@daverodgman daverodgman changed the title Better support for MSVC aarch64 Better support for MSVC aarch64 aka ARM64 and ARM64EC Aug 8, 2023
@daverodgman
Copy link
Contributor Author

Rebase & tidy-up

tom-cosgrove-arm
tom-cosgrove-arm reviewed Sep 15, 2023
View reviewed changes
include/mbedtls/build_info.h
@@ -74,6 +74,10 @@
#define MBEDTLS_ARCH_IS_X86
#endif

#if defined(_M_ARM64) || defined(_M_ARM64EC)
#define MBEDTLS_PLATFORM_IS_WINDOWS_ON_ARM64
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We should be careful gating any asm() blocks on this, as the ABI is different, and certain AArch64 registers cannot be used, as they are not saved during context switches - see "blocked registers" at https://learn.microsoft.com/en-us/windows/arm/arm64ec-abi

ARM64 intrinsics should be supported, see: https://techcommunity.microsoft.com/t5/windows-os-platform-blog/getting-to-know-arm64ec-defines-and-intrinsic-functions/ba-p/2957235

Copy link
Contributor Author

@daverodgman daverodgman Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we have any MSVS asm - this would only affect MSVS-with-clang (which I would expect is probably not working anyway)

@daverodgman
Copy link
Contributor Author

Given that this could potentially be de-stabilising for Windows-on-Arm, I propose to try and get this in straight after 3.5.

@daverodgman
Define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS on Windows-on-Arm
78fc0bd 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Support ARM64EC in the same way as ARM64 in sha256 and sha512
ad71b6a 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Enable 8-byte fastpath in mbedtls_xor on ARM64 and ARM64EC
be09286 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Introduce MBEDTLS_HAVE_NEON_INTRINSICS and simplify NEON header inclu…
4ffd7c7 
…sion

Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Use MBEDTLS_HAVE_NEON_INTRINSICS instead of __ARM_NEON
a0f10da 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Use new MBEDTLS_ARCH_IS_xxx macros
c5cc727 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Simplify Windows-on-Arm macros
0a48717 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Make macro definition more consistent with similar defns
3e52184 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Copy link
Contributor Author

Given that this could potentially be de-stabilising for Windows-on-Arm, I propose to try and get this in straight after 3.5.

@tom-cosgrove-arm WDYT?

@daverodgman daverodgman marked this pull request as ready for review October 16, 2023 08:30
@tom-cosgrove-arm
Copy link
Contributor

Sorry, this dropped off my review list. Will get back to it now!

@daverodgman
Copy link
Contributor Author

Sorry, this dropped off my review list. Will get back to it now!

That's fine, it was never an urgent one. My question was more, do you agree this is something we want, and is this the right time to merge given limited ability to test?

@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) labels Oct 16, 2023
@daverodgman
Copy link
Contributor Author

@yuhaoth would you be able to review this?

@tom-cosgrove-arm
Copy link
Contributor

My question was more, do you agree this is something we want, and is this the right time to merge given limited ability to test?

Yes, and yes.

Regarding "limited ability to test" the longer between code going in and release, the likelier that any issues will be found before release, either by the team or externally

@daverodgman daverodgman removed the needs-ci Needs to pass CI tests label Oct 16, 2023
@daverodgman daverodgman added needs-work needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 24, 2023
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Oct 25, 2023
@daverodgman
Copy link
Contributor Author

@yuhaoth could you TAL at this one please?

@tom-cosgrove-arm tom-cosgrove-arm self-requested a review November 1, 2023 13:23
@daverodgman
Copy link
Contributor Author

@tom-cosgrove-arm @yuhaoth given that we want to get this in early, PTAL

@yuhaoth
Copy link
Contributor

yuhaoth commented Nov 30, 2023 •
edited
Loading

Just found it . It looks good to me . But it just got conflict

@daverodgman daverodgman requested a review from yuhaoth November 30, 2023 10:48
@daverodgman daverodgman removed the needs-reviewer This PR needs someone to pick it up for review label Nov 30, 2023
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm 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.

LGTM

yuhaoth
yuhaoth approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@yuhaoth yuhaoth left a comment

Choose a reason for hiding this comment

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

LGTM

include/mbedtls/build_info.h
Comment on lines +65 to +68
#if !defined(MBEDTLS_PLATFORM_IS_WINDOWS_ON_ARM64) && \
(defined(_M_ARM64) || defined(_M_ARM64EC))
#define MBEDTLS_PLATFORM_IS_WINDOWS_ON_ARM64
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

For the time being, it is Okay.

But I think we should introduce target os macros in future. after that it should be

Suggested change
#if !defined(MBEDTLS_PLATFORM_IS_WINDOWS_ON_ARM64) && \
(defined(_M_ARM64) || defined(_M_ARM64EC))
#define MBEDTLS_PLATFORM_IS_WINDOWS_ON_ARM64
#endif
#if !defined(MBEDTLS_PLATFORM_IS_WINDOWS_ON_ARM64) && \
(defined(MBEDTLS_ARCH_IS_ARM64) && defined(MBEDTLS_OS_IS_WINDOWS))
#define MBEDTLS_PLATFORM_IS_WINDOWS_ON_ARM64
#endif

@daverodgman daverodgman added this pull request to the merge queue Dec 1, 2023
@daverodgman daverodgman 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 Dec 1, 2023
Merged via the queue into Mbed-TLS:development with commit 422951b Dec 1, 2023
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

@yuhaoth yuhaoth yuhaoth approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts size-xs Estimated task size: extra small (a few hours at most)
Projects
Non-roadmap pull requests
Archived in project
Milestone
No milestone
Development

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