Closed Bug 1152625 Opened 10 years ago Closed 5 years ago

Support AES HW acceleration on ARMv8

Categories

(NSS :: Libraries, defect, P2)

Product:
NSS
Component:
Libraries
Version:
3.47
Platform:
ARM64
Android
Type:
defect

Tracking

(Performance Impact:high, firefox40 wontfix, firefox-esr60 wontfix, firefox-esr68 wontfix, firefox67 wontfix, firefox67.0.1 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 fixed)

Status:
RESOLVED FIXED
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox40 --- wontfix
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(Keywords: feature, perf:pageload, Whiteboard: [geckoview:p2] [bcs:p2])

Attachments

(4 files)

Support AES HW acceleration on ARMv8
(deleted), patch
Details | Diff | Splinter Review
Bug 1152625 - Support AES HW acceleration on ARMv8.
(deleted), text/x-phabricator-request
Details
Bug 1152625 - Part 2. Remove __builtin_assume to avoid crash on PGO.
(deleted), text/x-phabricator-request
Details
Bug 1152625 - Add gyp flag for disabling ARM HW AES
(deleted), text/x-phabricator-request
Details
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.
Depends on: 1439226
Depends on: 1398051
No longer depends on: 1439226
sandbox_vars removes all compiler flags on nss's GYP. So we should remove sandbox_vars dependencies for nss build of Firefox.
FYI: My WIP improves 15x for AES CBC on Cortex-A53.
Assignee: nobody → 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.
Attachment #8953901 - Flags: review?(ttaubert)
(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.
No longer depends on: 1398051
Attachment #8953901 - Flags: review?(ttaubert)
QA Contact: jjones
Whiteboard: [qf:p1:pageload][geckoview]

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

Hardware: ARM → ARM64
Whiteboard: [qf:p1:pageload][geckoview] → [qf:p1:pageload][geckoview:p2]

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

status-firefox40: affected → wontfix
status-firefox67: --- → wontfix
status-firefox67.0.1: --- → wontfix
status-firefox68: --- → wontfix
status-firefox69: --- → affected
status-firefox-esr60: --- → wontfix
status-firefox-esr68: --- → affected
Whiteboard: [qf:p1:pageload][geckoview:p2] → [qf:p1:pageload][geckoview:p2] [bcs:p2]

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.

status-firefox69: affected → fix-optional
status-firefox70: --- → affected
status-firefox-esr68: affected → fix-optional
Flags: needinfo?(m_kato)
Priority: -- → P2
Whiteboard: [qf:p1:pageload][geckoview:p2] [bcs:p2] → [qf:p1:pageload] [geckoview:p2] [bcs:p2]

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.

(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.

Flags: needinfo?(m_kato)

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

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.46
Depends on: 1570991
No longer depends on: 1570991
Regressions: 1570991

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.

(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.

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.

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.

Backed out for crash in arm_aes_encrypt_cbc_128 on Android 8.0 Pixel2 pgo

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 3.46 → ---

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

status-firefox-esr68: fix-optional → wontfix

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.

Version: trunk → 3.47

Depends on D34473

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

Flags: needinfo?(kjacobs.bugzilla)

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.

Flags: needinfo?(jjones)

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

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox71: affected → fixed
Flags: needinfo?(kjacobs.bugzilla)
Flags: needinfo?(jjones)
Resolution: --- → FIXED
Target Milestone: --- → 3.47

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).

status-firefox69: fix-optional → wontfix
status-firefox70: fix-optional → wontfix
Regressions: 1661810
Performance Impact: --- → P1
Keywords: perf:pageload
Whiteboard: [qf:p1:pageload] [geckoview:p2] [bcs:p2] → [geckoview:p2] [bcs:p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: