Last Comment Bug 540986 - Intel AES-NI support for Windows x64 and x86 (32-bit)
: Intel AES-NI support for Windows x64 and x86 (32-bit)
Status: RESOLVED DUPLICATE of bug 979703
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: x86_64 Windows Vista
-- normal with 5 votes (vote)
: ---
Assigned To: Makoto Kato [:m_kato]
Depends on: 536485 661061
  Show dependency treegraph
Reported: 2010-01-20 17:03 PST by Makoto Kato [:m_kato]
Modified: 2016-05-18 01:11 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

WIP (50.75 KB, patch)
2010-01-20 17:12 PST, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v1 (69.49 KB, patch)
2011-05-25 01:53 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v2 [needs work] (70.20 KB, patch)
2011-08-21 20:51 PDT, Makoto Kato [:m_kato]
rrelyea: review+
Details | Diff | Splinter Review

Description User image Makoto Kato [:m_kato] 2010-01-20 17:03:57 PST
By bug 459248, Linux supports Intel AES-NI.  I port it to Windows x64 (MASM).
Comment 1 User image Makoto Kato [:m_kato] 2010-01-20 17:12:01 PST
Created attachment 422661 [details] [diff] [review]
Comment 2 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-03-11 10:43:46 PST
Makoto, can 32-bit support can be added to this patch?
Comment 3 User image Makoto Kato [:m_kato] 2011-03-13 18:46:57 PDT
(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.
Comment 4 User image Makoto Kato [:m_kato] 2011-05-23 04:23:28 PDT
Additional.  I am working new fix that is supported x86 and x64 (required VS2008 or later).
Comment 5 User image Makoto Kato [:m_kato] 2011-05-25 01:53:59 PDT
Created attachment 535006 [details] [diff] [review]
fix v1

testing now.
Comment 6 User image Makoto Kato [:m_kato] 2011-08-21 20:51:58 PDT
Created attachment 554787 [details] [diff] [review]
fix v2 [needs work]
Comment 7 User image Makoto Kato [:m_kato] 2011-10-24 18:13:14 PDT
Robert, ping.

If you aren't correct reviewer, who is best?
Comment 8 User image Justin Dolske [:Dolske] 2012-02-21 11:42:15 PST
Brian: are you comfortable with reviewing this, at least as a first pass?
Comment 9 User image Robert Relyea 2012-02-27 16:52:04 PST
Comment on attachment 554787 [details] [diff] [review]
fix v2 [needs work]


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.

Comment 10 User image Kai Engert (:kaie) 2012-03-01 10:40:26 PST
I'm removing keyword checkin-needed. Based on comment 9 you seem to be requesting a new patch.
Comment 11 User image Kai Engert (:kaie) 2013-05-30 10:48:48 PDT
needs a new target milestone
Comment 12 User image Wan-Teh Chang 2014-10-28 07:11:47 PDT
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 User image timbugzilla 2016-05-17 21:33:54 PDT
Comment 14 User image Makoto Kato [:m_kato] 2016-05-18 01:11:37 PDT

*** This bug has been marked as a duplicate of bug 979703 ***

Note You need to log in before you can comment on or make changes to this bug.