1315213 - CPU microcode detection is not working with AMD CPUs

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Marco Castelluccio [:marco]]

It looks like the CPU microcode detection is not working with AMD CPUs: https://crash-stats.mozilla.com/search/?cpu_info=~AuthenticAMD&product=Firefox&_sort=-date&_facets=signature&_facets=cpu_info&_facets=cpu_microcode_version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-cpu_microcode_version.

Comment 1

[Leif-AMO]

OS: Windows 7 x64 SP1 + all updates
Firefox: 53.0a1 - Nightly (2016-11-26) - tested Win32 and Win64 versions


I get this error in JS console:

> 12:55:05.663 Error getting CPU revision. Exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWindowsRegKey.readBinaryValue]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Program%20Files/Mozilla/Firefox/53.0a1/browser/features/[e-mail]!/bootstrap.js :: startup :: line 67"  data: no] 1 bootstrap.js:89
>	startup jar:file:///C:/Program%20Files/Mozilla/Firefox/53.0a1/browser/features/[e-mail]!/bootstrap.js:89:11
>	this.XPIProvider.callBootstrapMethod resource://gre/modules/addons/XPIProvider.jsm:4975:9
>	this.XPIProvider.startup resource://gre/modules/addons/XPIProvider.jsm:2880:13
>	callProvider resource://gre/modules/AddonManager.jsm:235:12
>	_startProvider resource://gre/modules/AddonManager.jsm:788:5
>	AddonManagerInternal.startup resource://gre/modules/AddonManager.jsm:974:9
>	this.AddonManagerPrivate.startup resource://gre/modules/AddonManager.jsm:3026:5
>	amManager.prototype.observe resource://gre/components/addonManager.js:65:9


The offending code seems to be this:

> In browser/features/[e-mail]!/bootstrap.js
>
>  let microCodeVersions = [0xe, 0x11, 0x12, 0x13, 0x16, 0x18, 0x19];
>  let cpuRevMatch = null;
>  try {
>    let keyNames = ["Update Revision", "Update Signature"];
>    for (let i = 0; i < keyNames.length; ++i) {
>      try {
>        let regVal = wrk.readBinaryValue(keyNames[i]);

The problem is that there are no key names "Update Revision" or "Update Signature" in the registry ( HKEY_LOCAL_MACHINE\HARDWARE\DESCRIPTION\System\CentralProcessor\0 ) for my AMD CPU.  Are these specific to Intel CPUs?  Should this have a conditional or something?  Check that is it an Intel CPU before looking for Intel specific registry entries.  If there are other AMD CPUs that do have an "Update (Revision|Signature)", then check that we are on one of those specific CPUs before looking deeper?

The available registry entries are:

~MHz (REG_DWORD, CPU MHz )
Component Information (REG_BINARY, 16 bytes)
Configuration Data (REG_FULL_RESOURCE_DESCRIPTOR, 16 bytes)
CurrentPatchLevel (REG_DWORD)
FeatureSet (REG_DWORD)
Identifier (REG_SZ, AMD64 Family 21 Model 2 Stepping 0)
MicrocodeUpdateStatus (REG_SZ, Newer Patch Not Available)
PreferredPatchLevel (REG_DWORD)
PreviousPatchLevel (REG_DWORD, same as PreferredPatchLevel)
ProcessorNameString (REG_SZ, "AMD FX(tm)-8350 Eight-Core Processor           ")
VendorIdentifier (REG_SZ)

Comment 2

[Leif-AMO]

This was actually in 49.0 and 50.0.  The Mercurial repo says bootstrap.js has a 2016-10-08 creation date, so I am assuming somewhere in the 49.0a1 nightlies.

The bug seems to do nothing but print an annoying message on AMD systems.  Cosmetic?

Looking at the code again, it seems to check for a bunch of intel-specific registry and other values, and sets each of three flags to true, false, or null, and then checks them all at the end, after the catch block generates the caught error message noise.

But perhaps to make this quiet, the checks need to be done incrementally, after each flag is set.  Thus, after the cpuVendorIDMatch === false, we KNOW that BOTH cpuIDMatch AND cpuRevMatch WILL also === false, because they CANNOT be true for non-Intel CPUs, and we should not even test, because we know they WILL fail the try and trigger the catch and be set to === null, and trigger the error message erroneously.  Likewise, if cpuVendorIDMatch === true, but cpuIDMatch === false, then we KNOW the stepping will be irrelevant (=== false), as each family has their own stepping sequence.

Thus, there should be no need for the redundant checking "if-matrix" done at the end.

But anyways, the absolute minimal change possible would simply be an if-else around the 3rd try.

>  let microCodeVersions = [0xe, 0x11, 0x12, 0x13, 0x16, 0x18, 0x19];
>  let cpuRevMatch = null;
>  if ( ! cpuVendorIDMatch ) {
>    cpuRevMatch = false;
>  } else {
>    try {
>      let keyNames = ["Update Revision", "Update Signature"];
>      // ...
>    } catch // ... {
>  }

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch

This is just cosmetic. If any of the entries read doesn't match it sets the url to false so removing the reportError is all that needs to be done.

Comment 4

[Marco Castelluccio [:marco]]

Actually, I opened this bug to add the CPU microcode detection for AMD too.

Comment 5

[Robert Strong (they/them - no direct email)]

Hmmm... I think I moved to fast with this in that Marco is reporting an error with breakpad and the additional comments are for the aushelper. I'll move this back to breakpad and file a new bug for the error message.

Comment 6

[Robert Strong (they/them - no direct email)]

Removing dependency on bug 1311515 which is what made me think this had to do with the aushelper which was specifically requested to only detect GenuineIntel and not to include AMD. If AMD needs to be included in the aushelper a new bug along with the checks that need to be performed should be filed under Firefox -> General for Firefox's aushelper extension.

Comment 7

[Robert Strong (they/them - no direct email)]

Filed bug 1320590 for the cosmetic issue

Comment 8

[Marco Castelluccio [:marco]]

I think I know what's wrong. I'll write a patch.

Comment 9

[Marco Castelluccio [:marco]]

Attachment: WIP patch

This is my WIP patch, still untested.

Comment 10

[[:philipp]]

hi marco, i've tested your second try build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e3ddef50ebd2ac98cd86e351b7494d1052cc3c and it does seem to generate a CPUMicrocodeVersion field in a report from an AMD cpu now:
https://crash-stats.mozilla.com/report/index/2476a7cd-1b3b-44ca-8e36-18e982161130#tab-metadata

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Patch

Could any of you review this patch?

It's the same as the WIP one, just added a new possible value name ("CurrentPatchLevel") that appears to be used for some AMD CPUs.

Comment 12

[Marco Castelluccio [:marco]]

Attachment: AMD1

Comment 13

[Marco Castelluccio [:marco]]

Attachment: AMD2

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8815772 [details] [diff] [review]
Patch

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

Looks good to me - I'm wondering if there is value in recording which registry entry we ended up with, or if all of the numbers can be used the same way, so we don't really need it.  I guess we start with this, and if it turns out we need more, we can add it.

Comment 15

[Marco Castelluccio [:marco]]

Comment on attachment 8815772 [details] [diff] [review]
Patch

Thanks Milan, that was a quick review!

I've pushed the patch to try, just to be sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b5f03f461ab3e4caaf1e766ff7fa734afb42740.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0dcd3dc893
Support reading microcode version from the registry when it is a REG_DWORD value (sometimes used for AMD CPUs). r=milan

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46f6ea9600f0
Add "CurrentPatchLevel" to the list of possible values where the microcode version is stored. r=milan

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/bb0dcd3dc893
https://hg.mozilla.org/mozilla-central/rev/46f6ea9600f0

Comment 19

[Marco Castelluccio [:marco]]

It appears to be working as expected: https://crash-stats.mozilla.com/search/?cpu_info=~AMD&build_id=%3E%3D20161202030204&product=Firefox&version=53.0a1&date=%3E%3D2016-12-08T05%3A52%3A00.000Z&date=%3C2016-12-15T05%3A52%3A00.000Z&_sort=-date&_facets=signature&_facets=cpu_name&_facets=cpu_info&_facets=cpu_microcode_version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-cpu_microcode_version.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Marco, is that something that you were planning to uplift to 52?

Comment 21

[Marco Castelluccio [:marco]]

It's a simple patch and it would give us more information when making decisions. I guess we can safely uplift to 52.
I'll create an uplift request.

Comment 22

[Marco Castelluccio [:marco]]

Comment on attachment 8815772 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression.
[User impact if declined]: We won't have detailed information about CPU microcode for AMD CPUs in Socorro.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: I have verified that on Socorro we now have the cpu_microcode_version value also for AMD CPUs. IIRC philipp verified the patch on his AMD machine. So, I suppose additional verification isn't really needed.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple change that is only adding a new possible Windows registry key (and type) to read the CPU microcode version.
[String changes made/needed]: None.

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8815772 [details] [diff] [review]
Patch

get cpu microcode revision in crash reports for more cpus, beta52+ (both patches that landed on m-c)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f8207e7d0e6b
https://hg.mozilla.org/releases/mozilla-beta/rev/07e5979a0222