Support cpuid for win32 #8339

lpy4105 · 2023-10-10T10:43:57Z

Description

cpuid.h and intrin.h define __cpuid with different signatures. We include intrin.h for WIN32 but we only use the correct signature for MSVC. This cause problem when build WIN32 target with non-MSVC compilers (i.g clang or i686-w64-mingw32).

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 done, Backport 2.28: Support cpuid for win32 #8392
tests provided

`__cpuid` has two kinds of signatures in different headers depending on the target OS. We make it consistent between the usages ang the included header. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

Change the type of array that stores the cpuinfo data to int[4] to match the signature of `__cpuinfo` in `intrin.h` header file. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

tom-cosgrove-arm

LGTM

yuhaoth · 2023-10-11T02:30:19Z

library/aesni.c

-        static unsigned info[4] = { 0, 0, 0, 0 };
-#if defined(_MSC_VER)
+        static int info[4] = { 0, 0, 0, 0 };
+#if defined(_WIN32)


That's difference between GCC and MSC. I prefer change ln 36 to #if !defined(_MSC_VER)

This might not work when using clang compiler driver under Visual Studio, which is becoming more common.

(When clang is run under Linux, it sets __GNUC__, but when run as compiler driver under Visual Studio, it sets _MSC_VER. This definitely causes a problem for inline assembly (since clang format is not the same as MSVC format) - I need to test the intrinsics)

@tom-cosgrove-arm , Yes, that might be a problem. Does that mean both macro will be defined in this case ?

This might not work when using clang compiler driver under Visual Studio, which is becoming more common.

I've tested it, and no related issue. When clang is running in MSVC compatibility mode, it sets _MSC_VER and provide a compatible intrin.h.

Thanks for checking

MinGW provides both kinds of implementations of `__cpuid`, but since `cpuid.h` is provided by GNUC, so we should choose the implementation by the compiler type instead of OS type. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

tom-cosgrove-arm · 2023-10-12T12:44:30Z

tests/scripts/all.sh

@@ -4626,8 +4626,7 @@ component_test_m32_o0 () {
    # build) and not the i386-specific inline assembly.
    msg "build: i386, make, gcc -O0 (ASan build)" # ~ 30s
    scripts/config.py full
-    scripts/config.py unset MBEDTLS_AESNI_C # AESNI depends on cpu modifiers
-    make CC=gcc CFLAGS="$ASAN_CFLAGS -m32 -O0" LDFLAGS="-m32 $ASAN_CFLAGS"
+    make CC=gcc CFLAGS="$ASAN_CFLAGS -m32 -O0 -maes -msse2 -mpclmul" LDFLAGS="-m32 $ASAN_CFLAGS"


Why is this change being made?

The comment at line 4625 is explicit: this component is intended to test the portable C code in a 32-bit build

I was trying to revert the workarrounds added in #7384, that's why I removed the previous line 4629, and the additional CFLAGS were just for a successful build.
I have just realize that all AESNI m32 test are in a separate component, so only reverting the work arround in component_build_mingw should be sufficient?

BTW, I don't quite understand the comments in line 4625 and line 4642. Why different optimization levels make a difference on using or not using i386-specific inline assembly?

beni-sandu · 2023-10-13T18:36:04Z

At the moment, 3.5.0 compilation fails on 32-bit x86 for both clang and gcc, on Linux. Not sure if this is related in any way to this issue, but I will try to have a closer look next week.

Error is the same on both gcc/clang:

In file included from /buildarea/bsandu/poky-container/poky/build/tmp/work/core2-32-poky-linux/mbedtls/3.5.0/git/library/aesni.c:29:
| /buildarea/bsandu/poky-container/poky/build/tmp/work/core2-32-poky-linux/mbedtls/3.5.0/git/library/aesni.h:81:5: error: #error "Must use `-mpclmul -msse2 -maes` for MBEDTLS_AESNI_C"
|    81 | #   error "Must use `-mpclmul -msse2 -maes` for MBEDTLS_AESNI_C"
|       |     ^~~~~

AESNI for x86 (32-bit) have been tested in a seperate component, we don't need to test twice. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

yuhaoth

LGTM

yuhaoth · 2023-10-17T02:25:18Z

tests/scripts/all.sh


    # note Make tests only builds the tests, but doesn't run them
-    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror' WINDOWS_BUILD=1 tests
+    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -maes -msse2 -mpclmul' WINDOWS_BUILD=1 tests


This change should be replace with Pragma or function attribute in future.

yuhaoth

LGTM

yuhaoth

LGTM.

Please verify the VS compiler driver case raised by @tom-cosgrove-arm. I guess this change can cover the case

tom-cosgrove-arm · 2023-10-17T08:57:28Z

tests/scripts/all.sh

@@ -5114,16 +5114,15 @@ component_test_tls13_only_record_size_limit () {

 component_build_mingw () {
    msg "build: Windows cross build - mingw64, make (Link Library)" # ~ 30s
-    scripts/config.py unset MBEDTLS_AESNI_C # AESNI depends on cpu modifiers
-    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 lib programs
+    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra -maes -msse2 -mpclmul' WINDOWS_BUILD=1 lib programs


Do we need to make lib without MBEDTLS_AESNI_C to check that it works on mingw?

Do you mean to test the build of pure C impl of AES? I think they have been covered because MBEDTLS_AES_USE_HARDWARE_ONLY is unset in defaul config.

Yes, but I mean to check that the C code builds under mingw, which is quite a "strange" environment - just lib, not programs, which shouldn't take long

Sorry, I didn't get the point. We do build lib and programs and later tests for both static and dynamic linked libraries in this mingw test...

So with this PR, under mingw we are now building only with MBEDTLS_AESNI_C set, and not without it? So we are no longer checking that we build correctly under mingw without MBEDTLS_AESNI_C. If that's right, I think we should add a build of lib on its own without MBEDTLS_AESNI_C

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

tom-cosgrove-arm · 2023-10-19T02:55:06Z

tests/scripts/all.sh

+    msg "build: Windows cross build - mingw64, make (Link Library, default config without MBEDTLS_AESNI_C)" # ~ 30s
+    ./scripts/config.py unset MBEDTLS_AESNI_C #
+    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 lib programs
+    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror' WINDOWS_BUILD=1 tests


For the config without MBEDTLS_AESNI_C we only need to make lib - we don't need to build programs or tests. The main reason for doing this is to ensure that the strange mingw environment (not quite *nix, not quite Windows) doesn't cause something related to AES-NI to get mis-compiled in. Outside of the aes*.c modules this is very unlikely, and will add unnecessary time to CI runs

We do not need to test DLL either, I think?

tom-cosgrove-arm · 2023-10-19T03:23:57Z

Also: do we need a ChangeLog entry for this?

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

tom-cosgrove-arm · 2023-10-19T08:24:59Z

ChangeLog.d/fix-mingw32-build.txt

+Bugfix
+  * Fix an inconsistency between implementations and usages of `__cpuid`,
+    which mainly causes failures when building Windows target using
+    mingw or clang. Fix #8334 & #8332.


(minor) We normally says "Fixes #nnnn and #nnn2"

$ fgrep 'Fixes #' ChangeLog | wc -l 184 $ fgrep 'Fix #' ChangeLog | wc -l 9

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2023-10-19T08:26:01Z

Can you update the "backport done, or not required" text with "backport not required because ..." at the top, please?

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2023-10-20T07:36:02Z

@yuhaoth can you re-review please? Also, do you have bandwidth to look at the backport?

yuhaoth

LGTM.

lpy4105 added 2 commits October 10, 2023 17:56

aesni: support cpuid on WIN32

308cb23

`__cpuid` has two kinds of signatures in different headers depending on the target OS. We make it consistent between the usages ang the included header. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

aesni: declare cpuinfo as int

e8c4bf1

Change the type of array that stores the cpuinfo data to int[4] to match the signature of `__cpuinfo` in `intrin.h` header file. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

Andarwinux mentioned this pull request Oct 10, 2023

Unable to compile mbedtls 3.5.0 with mingw-w64 clang17 #8332

Closed

tom-cosgrove-arm previously approved these changes Oct 10, 2023

View reviewed changes

yuhaoth reviewed Oct 11, 2023

View reviewed changes

yuhaoth removed the needs-reviewer This PR needs someone to pick it up for review label Oct 11, 2023

lpy4105 added 2 commits October 11, 2023 11:09

aesni: select __cpuid impl based on compiler type

0ecb635

MinGW provides both kinds of implementations of `__cpuid`, but since `cpuid.h` is provided by GNUC, so we should choose the implementation by the compiler type instead of OS type. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

all.sh: Re-enable MBEDTLS_AESNI_C in some components

64cca2f

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 dismissed tom-cosgrove-arm’s stale review via 64cca2f October 11, 2023 04:08

lpy4105 requested review from yuhaoth and tom-cosgrove-arm October 11, 2023 05:59

tom-cosgrove-arm reviewed Oct 12, 2023

View reviewed changes

all.sh: revert changes in test_m32*

1322613

AESNI for x86 (32-bit) have been tested in a seperate component, we don't need to test twice. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

yuhaoth previously approved these changes Oct 16, 2023

View reviewed changes

lpy4105 dismissed yuhaoth’s stale review via 1322613 October 16, 2023 06:07

lpy4105 requested review from tom-cosgrove-arm and yuhaoth October 16, 2023 06:38

gilles-peskine-arm mentioned this pull request Oct 16, 2023

Error compiling AESNI in Mbed-TLS with clang on Windows #8372

Closed

This was referenced Oct 16, 2023

Backport 2.28: Fix compiling AESNI in Mbed-TLS with clang on Windows #8373

Merged

Fix compiling AESNI in Mbed-TLS with clang on Windows #8374

Merged

yuhaoth reviewed Oct 17, 2023

View reviewed changes

yuhaoth approved these changes Oct 17, 2023

View reviewed changes

yuhaoth previously approved these changes Oct 17, 2023

View reviewed changes

tom-cosgrove-arm reviewed Oct 17, 2023

View reviewed changes

lpy4105 dismissed yuhaoth’s stale review via 3a26529 October 19, 2023 02:40

all.sh: build_mingw: test default config without MBEDTLS_AESNI_C

bb4f63c

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 force-pushed the issue/support-cpuid-for-win32 branch from 3a26529 to bb4f63c Compare October 19, 2023 02:42

lpy4105 requested review from tom-cosgrove-arm and yuhaoth October 19, 2023 02:47

tom-cosgrove-arm reviewed Oct 19, 2023

View reviewed changes

lpy4105 added 2 commits October 19, 2023 11:27

all.sh: build_mingw: only test build lib without MBEDTLS_AESNI_C

158eead

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

Add a changelog entry

74f2c15

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 requested a review from tom-cosgrove-arm October 19, 2023 07:02

tom-cosgrove-arm reviewed Oct 19, 2023

View reviewed changes

tom-cosgrove-arm previously approved these changes Oct 19, 2023

View reviewed changes

Reword the changelog entry

d2d3d63

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 dismissed tom-cosgrove-arm’s stale review via d2d3d63 October 19, 2023 08:52

lpy4105 mentioned this pull request Oct 19, 2023

Backport 2.28: Support cpuid for win32 #8392

Merged

3 tasks

tom-cosgrove-arm added the needs-backports Backports are missing or are pending review and approval. label Oct 19, 2023

lpy4105 requested a review from tom-cosgrove-arm October 19, 2023 08:57

tom-cosgrove-arm approved these changes Oct 19, 2023

View reviewed changes

yuhaoth approved these changes Oct 23, 2023

View reviewed changes

tom-cosgrove-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 Oct 23, 2023

tom-cosgrove-arm added this pull request to the merge queue Oct 23, 2023

Merged via the queue into Mbed-TLS:development with commit 235e361 Oct 23, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cpuid for win32 #8339

Support cpuid for win32 #8339

lpy4105 commented Oct 10, 2023 •

edited

Loading

tom-cosgrove-arm left a comment

yuhaoth Oct 11, 2023

tom-cosgrove-arm Oct 11, 2023

yuhaoth Oct 17, 2023

lpy4105 Oct 17, 2023

tom-cosgrove-arm Oct 17, 2023

tom-cosgrove-arm Oct 12, 2023 •

edited

Loading

lpy4105 Oct 13, 2023

beni-sandu commented Oct 13, 2023

yuhaoth left a comment

yuhaoth Oct 17, 2023

yuhaoth left a comment

yuhaoth left a comment

tom-cosgrove-arm Oct 17, 2023 •

edited

Loading

lpy4105 Oct 17, 2023

tom-cosgrove-arm Oct 17, 2023

lpy4105 Oct 18, 2023

tom-cosgrove-arm Oct 18, 2023

tom-cosgrove-arm Oct 19, 2023 •

edited

Loading

lpy4105 Oct 19, 2023

tom-cosgrove-arm commented Oct 19, 2023

tom-cosgrove-arm Oct 19, 2023

tom-cosgrove-arm left a comment

tom-cosgrove-arm commented Oct 19, 2023

tom-cosgrove-arm left a comment

tom-cosgrove-arm commented Oct 20, 2023

yuhaoth left a comment

Support cpuid for win32 #8339

Support cpuid for win32 #8339

Conversation

lpy4105 commented Oct 10, 2023 • edited Loading

Description

PR checklist

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beni-sandu commented Oct 13, 2023

yuhaoth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Oct 19, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Oct 19, 2023

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Oct 20, 2023

yuhaoth left a comment

Choose a reason for hiding this comment

lpy4105 commented Oct 10, 2023 •

edited

Loading

tom-cosgrove-arm Oct 12, 2023 •

edited

Loading

tom-cosgrove-arm Oct 17, 2023 •

edited

Loading

tom-cosgrove-arm Oct 19, 2023 •

edited

Loading