Closed Bug 1110570 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla37

People

(Reporter: markh, Assigned: bbouvier)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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)
Attachment #8535605 - Flags: review?(sunfish)
/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!
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
Flags: needinfo?(sunfish)
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
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attached patch xgetbv.patchSplinter Review
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)
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+
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)
Attachment #8535605 - Attachment is obsolete: true
Attachment #8618902 - Flags: review+
You need to log in before you can comment on or make changes to this bug.