Intel AES-NI support for Windows x64 and x86 (32-bit)

RESOLVED DUPLICATE of bug 979703

Status

NSS
Libraries
RESOLVED DUPLICATE of bug 979703
8 years ago
a year ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

trunk
x86_64
Windows Vista
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
By bug 459248, Linux supports Intel AES-NI.  I port it to Windows x64 (MASM).
(Assignee)

Comment 1

8 years ago
Created attachment 422661 [details] [diff] [review]
WIP
(Assignee)

Updated

8 years ago
Depends on: 536485
Makoto, can 32-bit support can be added to this patch?
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Makoto, can 32-bit support can be added to this patch?

Currently code that is writtern by Ulrich uses XMM8-XMM15, so we has to re-write it to support 32-bit.

Also, this WIP has some bugs, I will fix it when reviewing.
(Assignee)

Comment 4

6 years ago
Additional.  I am working new fix that is supported x86 and x64 (required VS2008 or later).
Assignee: nobody → m_kato
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 535006 [details] [diff] [review]
fix v1

testing now.
Attachment #422661 - Attachment is obsolete: true
Depends on: 661061
Summary: Intel AES-NI support for Windows x64 → Intel AES-NI support for Windows x64 and x86 (32-bit)
(Assignee)

Comment 6

6 years ago
Created attachment 554787 [details] [diff] [review]
fix v2 [needs work]
Attachment #535006 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #554787 - Flags: review?(rrelyea)
(Assignee)

Comment 7

6 years ago
Robert, ping.

If you aren't correct reviewer, who is best?
Brian: are you comfortable with reviewing this, at least as a first pass?

Comment 9

6 years ago
Comment on attachment 554787 [details] [diff] [review]
fix v2 [needs work]

r+

Sorry, it's pretty tricky code with lots of Microsoft specific macros.

With the r+ I'd still like to see the following changes:

1) a comment in front of intel_aes_decrypt_init_192 explaining what is going in with the _mm_aesimc_si128 calls. (what is happening is aes192 keys are 1 1/2 the length of a m128i register, so we need to call store 2/3 of the first key with an _mm_aesimc_si128, then store the remaining 1/2 with a second key giving us 2 full m128i blocks, which we then fixup with later _mm_aesimc calls using xmm2 and xmm4.

2) It's not clear that we won't run out of registers on the 32 bit platforms when we load all the xmm registers up to handle the final partial blocks. My guess is the compiler will handle this, though it might be slower than otherwise. Same comment for the _cbc_encrypt calls. (nothing to do here, just a comment).

3) Right now we have a check for compiler versions in rijndael.c We need the same check intel-aes.c.

4) related to 3, we should probably have an environment variable to turn off hardware accelleration at compile time (that would go in Makefile).

I'd at least like to see #3 addresses, otherwise this won't compile.

On another note, it would be good to make sure we have some Windows test machines with hardware aes support. Any problems will be masked on older hardware as this code is turned off at runtime in those cases.

bob
Attachment #554787 - Flags: review?(rrelyea) → review+

Updated

6 years ago
Keywords: checkin-needed
Target Milestone: --- → 3.13.4

Comment 10

6 years ago
I'm removing keyword checkin-needed. Based on comment 9 you seem to be requesting a new patch.
Keywords: checkin-needed

Updated

6 years ago
Attachment #554787 - Attachment description: fix v2 → fix v2 [needs work]

Updated

6 years ago
Target Milestone: 3.13.4 → 3.13.5

Updated

5 years ago
Target Milestone: 3.13.5 → 3.14

Updated

5 years ago
Target Milestone: 3.14 → 3.14.1

Updated

5 years ago
Target Milestone: 3.14.1 → 3.14.2

Updated

5 years ago
Target Milestone: 3.14.2 → 3.14.4

Comment 11

4 years ago
needs a new target milestone
Target Milestone: 3.14.4 → ---

Comment 12

3 years ago
Hmm... I didn't know about this bug. In April and May of this year
I reviewed and checked in Intel AES assembly code for Windows from
Shay Gueron of Intel (bug 979703). Using intrinsics seems like a
good idea because they are easier to use than assembly code. But
unless someone is willing to figure out the actual differences
(ignoring intrinsics vs. assembly code) between the two implementations,
I wonder if we should mark this bug WONTFIX now.

Comment 13

a year ago
Bump.
(Assignee)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 979703
You need to log in before you can comment on or make changes to this bug.