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

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 1

[Mike Hommey [:glandium]]

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/

Comment 2

[Mike Hommey [:glandium]]

Actually that code is also checking something in the extended control register.

Comment 3

[Jan de Mooij [:jandem]]

Thanks for the report. This is probably a regression from bug 1065339.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: bz://1110570/bbouvier

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

/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

Comment 6

[Dan Gohman [:sunfish]]

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!

Comment 7

[Dan Gohman [:sunfish]]

https://reviewboard.mozilla.org/r/1423/#review861

Ship It!

Comment 8

[Dan Gohman [:sunfish]]

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

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9daf256c68

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Mark and/or Mike, does it fix the issue?

Comment 11

[Mike Hommey [:glandium]]

Shouldn't we also add the "check the AVX registers restore at context switch" from the link in comment 1?

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9e9daf256c68

Comment 13

[Dan Gohman [:sunfish]]

Attachment: 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

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 15

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19edf4b9d338

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19edf4b9d338

Comment 17

[Mark Hammond [:markh] [:mhammond]]

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!

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: Bug 1110570: AVX instructions should be emitted only if XSAVE is enabled by the OS;