Work-around buggy Intel chips erroneously reporting BMI1/BMI2 support #1249
Conversation
|
r? @Amanieu (rust-highfive has picked a reviewer for you, use r? to override) |
| if vendor_id == *b"GenuineIntel" && !value.test(Feature::avx as u32) { | ||
| value.unset(Feature::bmi1 as u32); | ||
| value.unset(Feature::bmi2 as u32); | ||
| } |
There was a problem hiding this comment.
Could you limit this to the specific cpu family that has the bug?
There was a problem hiding this comment.
Probably in theory, I mean there's model/family info in the bits of __cpuid_count(1, 0).eax (and we already have this from earlier in the function)
But it's not clear to me at all which values for these correspond to the impacted chip :(
There was a problem hiding this comment.
Affected Steppings for this appear to be...
| Number | Y | U | - | H | - | S | - |
|---|---|---|---|---|---|---|---|
| - | D-1 | D-1 | K-1 | N-0 | R-0 | R-0 | S-0 |
| SKL052 | X | X | X | X | X | X | X |
So that looks like effectively "all Skylake (non-server)", according to Intel, therefore we can constrain this logic to all Skylake processors. We don't know which of the processors this is actually true for, since it's a "may", so we must apply it to all Skylake "client" chips. The possible values for family, model, etc., are specified on page 21 of this specification update, so we just add another constraint.
New Skylake chips are not being shipped so we can assume this won't change.
Update stdarch 5 commits in 815d55c..cfba59f 2021-11-08 00:58:47 +0000 to 2021-11-19 01:29:04 +0000 - Work-around buggy Intel chips erroneously reporting BMI1/BMI2 support (rust-lang/stdarch#1249) - complete armv8 instructions (rust-lang/stdarch#1256) - Fix i8mm feature with bootstrap compiler. (rust-lang/stdarch#1252) - Fix unused link_name attribute. (rust-lang/stdarch#1251) - Add remaining insturctions (rust-lang/stdarch#1250)
Apparently on some chips,
cpuidis not to be fully trusted. According to http://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/desktop-6th-gen-core-family-spec-update.pdf, some Skylake chips (notably Celerons) will report support for BMI1 and BMI2 without actual support for those operations.Quoting that document's Errata section:
IIUC, the problematic chips are fairly widespread1, and searches online do seem to bring up people hitting this crash in the real world... so it would be nice if a better workaround existed than "ask the user to update their BIOS" (I think the OS could work around it too — if nothing else, by performing a similar hack to what's in this patch).
That said, one seems to exist2. From what I was able to tell3, all current Intel chips with working support for either BMI instruction set also have AVX, and the chips impacted by this bug here do not, so we can just assume any Intel chip without AVX also has no BMI1/BMI2. If this is correct, we can work around this with no downside to any current chip, so even if a more ideal place for a hack like this would be in the OS4, it's worth doing.
This is what's implemented in this patch, which also contains a big comment describing the issue, since it's pretty WTF.
Footnotes
I am pretty sure I know someone with one. This would be great because it would allow me to verify the issue/fix, but the Intel documentation is pretty clear either way. ↩
It's not surprising that Intel wouldn't include this as the suggested workaround, since they might want to keep the ability to ship chips with BMI and not AVX in the future. ↩
Unfortunately, this isn't definitive, since I'm not sure where to find such information concretely, and just went by what I could find on Wikipedia and Agner. That said, it makes sense that this would be the case both now and in the future, and it's safe even if I'm wrong or things change in the future. ↩
And ofc we can't rely on the user's OS or BIOS being up to date, beyond what libstd supports. ↩