1152625 - Support AES HW acceleration on ARMv8

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Makoto Kato [:m_kato]]

NVIDIA Denver (Nexus 9 etc) uses ARMv8 arch, so it has AES HW acceleration even if 32-bit mode (aarch32).  Also, gcc 4.9 supports AES opcode.

Comment 1

[Makoto Kato [:m_kato]]

sandbox_vars removes all compiler flags on nss's GYP.  So we should remove sandbox_vars dependencies for nss build of Firefox.

Comment 2

[Makoto Kato [:m_kato]]

FYI: My WIP improves 15x for AES CBC on Cortex-A53.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Support AES HW acceleration on ARMv8

Comment 4

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=d6b81dcef4be3f52c0f10bac156d61f0f10225fe

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8953901 [details] [diff] [review]
Support AES HW acceleration on ARMv8

I tested on ThunderX and Cortex-A53 hardware.  bltest is passed.

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #1)
> sandbox_vars removes all compiler flags on nss's GYP.  So we should remove
> sandbox_vars dependencies for nss build of Firefox.

This is already fixed.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

FYI, apparently all armv8 HW has the AES instructions (which are officially optional).

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Bug 1152625 - Support AES HW acceleration on ARMv8.

Comment 9

[Chris Peterson [:cpeterson]]

esr68=affected because we might want to uplift this optimization to Fennec ESR 68.1.

Comment 10

[Chris Peterson [:cpeterson]]

Makoto, are you still working on this bug? Will AES HW acceleration improve TLS speed or just reduce power usage?

P2 because there has not been recent activity on this bug. Depending on the performance results, we'll need to decide if we want to uplift to Fennec ESR 68.x.

Comment 11

[J.C. Jones [:jcj] (he/him)]

There's still work happening here, part due to bug 1556022, and part due to a webcompat issue Kevin caught using the TLS Canary.

The webcompat issue Kevin is soon going to work on - we need to basically run a few terabytes of data through this implementation against other implementations until we find one (or more) of the mismatches that caused connection errors in the Canary run.

Comment 12

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson [:cpeterson] from comment #10)

Makoto, are you still working on this bug? Will AES HW acceleration improve TLS speed or just reduce power usage?

Yes. both is improved.

Comment 13

[J.C. Jones [:jcj] (he/him)]

https://hg.mozilla.org/projects/nss/rev/009a7163c80a711bd681b155ecffa5962fd98a46

Comment 14

[J.C. Jones [:jcj] (he/him)]

Note - This patch may need to be backed out (per Bug 1570991) so I can land the other NSS changes into m-c. I'll be running an upstream try run on it to confirm whether this patch is the offender and report back.

Comment 15

[Makoto Kato [:m_kato]]

(In reply to J.C. Jones [:jcj] (he/him) from comment #14)

Note - This patch may need to be backed out (per Bug 1570991) so I can land the other NSS changes into m-c. I'll be running an upstream try run on it to confirm whether this patch is the offender and report back.

Hmm, is this PGO bug? When I have Pixel 2 and I cannot hit crash during my testing with opt and debug build.

Comment 16

[Makoto Kato [:m_kato]]

Looking PGO crash, since I add __builtin_assume_aligned to improve read performance, iv isn't aligned to 16 for PGO. I have to remove __builtin_assume_aligned for key and iv.

Fix is coming soon.

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Bug 1152625 - Part 2. Remove __builtin_assume to avoid crash on PGO.

AESContext->iv doesn't align to 16 bytes on PGO build, so we should remove
__builtin_assume. Also, I guess that expandedKey has same problem.

Comment 18

[J.C. Jones [:jcj] (he/him)]

Backed out for crash in arm_aes_encrypt_cbc_128 on Android 8.0 Pixel2 pgo

https://hg.mozilla.org/projects/nss/rev/777b6070fe76

Comment 19

[Chris Peterson [:cpeterson]]

status-firefox-esr68=wontfix because we don't plan to uplift these NSS changes to Fennec ESR 68.

Comment 20

[J.C. Jones [:jcj] (he/him)]

This is probably going to have to roll into Firefox 71 since we're late in the cycle and it's risky, so marking for 3.47.

Comment 21

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1152625 - Add gyp flag for disabling ARM HW AES

Depends on D34473

Comment 22

[Makoto Kato [:m_kato]]

Kevin, do I still have any issue to land this?

Comment 23

[J.C. Jones [:jcj] (he/him)]

Feature freeze is today, so if it's going into 3.47, it needs to land today. I'm good with taking it - Kevin will be back in office later today and I presume after brief conference we'll land everything and tag BETA1. Settting a needinfo to myself.

Comment 24

[J.C. Jones [:jcj] (he/him)]

https://hg.mozilla.org/projects/nss/rev/efb895a43899
https://hg.mozilla.org/projects/nss/rev/1b0f5c5335ee
https://hg.mozilla.org/projects/nss/rev/9abcea09fdd4

Comment 25

[Chris Peterson [:cpeterson]]

Setting firefox70=wontfix because I assume it's too late in the Beta cycle to uplift NSS changes to Firefox 70 Beta (release date October 22).