avx instruction detection incorrect on CPUs with avx support but when windows bootloader has xsavedisable=1

VERIFIED FIXED in mozilla37

Status

()

VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: markh, Assigned: bbouvier)

Tracking

unspecified
mozilla37
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Nightly is regularly crashing with an invalid instruction exception.  Slightly edited transcript from #developers where glandium helped me get to the root:

(12:07:25 PM) glandium: markh: what does the disassembly view say the instruction is?
(12:08:57 PM) markh: glandium: 1C040BE6  vaddsd      xmm0,xmm1,xmm0  
(12:09:48 PM) glandium: markh: sounds like the jit is trying to use avx instructions and your cpu doesn't support them
(12:11:44 PM) markh: the crash report says I've got "GenuineIntel family 6 model 58 stepping 9 | 8" - it's an i7-3770
(12:11:48 PM) glandium: markh: can you check the value of js::jit::CPUInfo::avxPresent and avxEnabled?
(12:13:56 PM) glandium: markh: interestingly, the i7-3770 is supposed to support avx...
(12:16:24 PM) markh: glandium: so yeah, it sounds like you aren't surprised to find both those vars are true
(12:17:01 PM) glandium: markh: yeah, so now the question is why a valid avx instruction doesn't run on your cpu that supports avx

(12:22:52 PM) markh: hrm - random googling led me to "bcdedit" to show the boot manager config
(12:23:05 PM) markh: and xsavedisable is true, which *looks* like it disables that
(12:24:36 PM) markh: glandium: see http://msdn.microsoft.com/en-us/library/windows/hardware/ff542202%28v=vs.85%29.aspx - it talks about "XSAVE functionality"
(12:24:44 PM) markh: is that related?
(12:25:25 PM) glandium: markh: http://support.microsoft.com/kb/2568088 suggests it is

and sure enough, toggling xsavedisable to 0 and rebooting caused the problem to stop.  I've no idea how it ended up being disabled (but maybe the vmware installation toggled it - not clear).  Regardless, it sounds like our avx detection code somehow needs to determine if the windows bootloader has disabled that functionality.
We apparently just need to check one more bit (which happens to be the one for XSAVE)
https://software.intel.com/en-us/blogs/2011/04/14/is-avx-enabled/
Actually that code is also checking something in the extended control register.
Thanks for the report. This is probably a regression from bug 1065339.
Blocks: 1065339
Flags: needinfo?(sunfish)
(Assignee)

Comment 4

4 years ago
Created attachment 8535605 [details]
MozReview Request: bz://1110570/bbouvier
Attachment #8535605 - Flags: review?(sunfish)
(Assignee)

Comment 5

4 years ago
/r/1425 - Bug 1110570: AVX instructions should be emitted only if XSAVE is enabled by the OS;

Pull down this commit:

hg pull review -r 9008ada71bfc33283578197e0257b30533c650b2
https://reviewboard.mozilla.org/r/1423/#review859

::: js/src/jit/shared/Assembler-x86-shared.cpp
(Diff revision 1)
> +    avxPresent = (flagsECX & AVXBit) && (flagsECX & XSAVEBit) && avxEnabled;

nice!

Updated

4 years ago
Attachment #8535605 - Flags: review?(sunfish) → review+
https://reviewboard.mozilla.org/r/1423/#review863

::: js/src/jit/shared/Assembler-x86-shared.cpp
(Diff revision 1)
> +    avxPresent = (flagsECX & AVXBit) && (flagsECX & XSAVEBit) && avxEnabled;

lgtm too

Updated

4 years ago
Flags: needinfo?(sunfish)
(Assignee)

Comment 9

4 years ago
Doh! mozext was supposed to post the inbound link here directly, but it didn't...

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9daf256c68
Assignee: nobody → benj
Status: NEW → ASSIGNED
(Assignee)

Comment 10

4 years ago
Mark and/or Mike, does it fix the issue?
Flags: needinfo?(mhammond)
Flags: needinfo?(mh+mozilla)
Shouldn't we also add the "check the AVX registers restore at context switch" from the link in comment 1?
Flags: needinfo?(mh+mozilla)
https://hg.mozilla.org/mozilla-central/rev/9e9daf256c68
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Created attachment 8535952 [details] [diff] [review]
xgetbv.patch

Ah, thanks for pointing that out. Attached is a patch which adds code to check the extended control register for OS support.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=191c4d34f182
Attachment #8535952 - Flags: review?(benj)
(Assignee)

Comment 14

4 years ago
Comment on attachment 8535952 [details] [diff] [review]
xgetbv.patch

Review of attachment 8535952 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/jit/shared/Assembler-x86-shared.cpp
@@ +152,5 @@
> +    size_t xcr0EAX = 0;
> +#if defined(_XCR_XFEATURE_ENABLED_MASK)
> +    xcr0EAX = _xgetbv(_XCR_XFEATURE_ENABLED_MASK);
> +#elif defined(__GNUC__)
> +    asm(".byte 0x0f, 0x01, 0xd0" : "=a"(xcr0EAX) : "c"(0) : "%edx");

can you add a comment that 0x0f 0x01 0xd0 is the encoding for xgetbv and that it stores its results in eax::edx?

@@ +231,5 @@
> +        size_t xcr0EAX = ReadXGETBV();
> +        static const int xcr0SSEBit = 1 << 1;
> +        static const int xcr0AVXBit = 1 << 2;
> +        if (!(xcr0EAX & xcr0SSEBit) || !(xcr0EAX & xcr0AVXBit))
> +            avxPresent = false;

avxPresent = (xcr0EAX & xcr0SSEBit) && (xcr0EAX & xcr0AVXBit);

would allow you to get rid of the |if| statement, but not a big deal so up to you
Attachment #8535952 - Flags: review?(benj) → review+
Depends on: 1111715
(Reporter)

Comment 17

4 years ago
I just set the xsavedisable flag back to 1 and re-created the problem on my nightly channel (which hadn't yet been updated with the fix).  I then tried with a local build that *does* have the fix and was unable to re-create the problem.  So marking verified - thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(mhammond)
(Assignee)

Comment 18

3 years ago
Comment on attachment 8535605 [details]
MozReview Request: bz://1110570/bbouvier
Attachment #8535605 - Attachment is obsolete: true
Attachment #8618902 - Flags: review+
(Assignee)

Comment 19

3 years ago
Created attachment 8618902 [details]
MozReview Request: Bug 1110570: AVX instructions should be emitted only if XSAVE is enabled by the OS;
You need to log in before you can comment on or make changes to this bug.