diff options
author | Furquan Shaikh <furquan@google.com> | 2021-03-08 22:08:42 -0800 |
---|---|---|
committer | Furquan Shaikh <furquan@google.com> | 2021-03-12 17:33:01 +0000 |
commit | f14c05f14493f8960559172b4653f33e26357703 (patch) | |
tree | c0c1ab2091b3068fded3a5652a0d488c1b06af1e /src/include/cpu | |
parent | 105d91e11476c8889fc7c5f0048c325fef3288fd (diff) |
cpu/intel/microcode: Fix caching logic in intel_microcode_find
CB:49896 added support in `intel_microcode_find()` to cache the found
microcode for faster subsequent accesses. This works okay when the
function succeeds in finding the microcode on BSP. However, if for any
reason, `cpu_microcode_blob.bin` does not contain a valid microcode
for the given processor, then the logic ends up attempting to find
microcode again and again every time it is called (because
`ucode_updates` is set to NULL on failed find, thus retriggering the
whole find sequence every time). This leads to a weird race condition
when multiple APs are running in parallel and executing this
function.
A snippet of the issues observed in the scenario described above:
```
...
microcode: Update skipped, already up-to-date
...
Microcode header corrupted!
...
```
1. AP reports that microcode update is being skipped since the current
version matches the version in CBFS (even though there is no matching
microcode update in CBFS).
2. AP reports microcode header is corrupted because it thinks that the
data size reported in the microcode is larger than the file read from
CBFS.
Above issues occur because each time an AP calls
`intel_microcode_find()`, it might end up seeing some intermittent
state of `ucode_updates` and taking incorrect action.
This change fixes this race condition by separating the logic for
finding microcode into an internal function `find_cbfs_microcode()`
and maintaining the caching logic in `intel_microcode_find()` using a
boolean flag `microcode_checked`.
BUG=b:182232187
TEST=Verified that `intel_microcode_find()` no longer makes repeated
attempts to find microcode from CBFS if it failed the first time.
Change-Id: I8600c830ba029e5cb9c0d7e0f1af18d87c61ad3a
Signed-off-by: Furquan Shaikh <furquan@google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/51371
Reviewed-by: Patrick Rudolph
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Aaron Durbin <adurbin@chromium.org>
Reviewed-by: Tim Wawrzynczak <twawrzynczak@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Diffstat (limited to 'src/include/cpu')
-rw-r--r-- | src/include/cpu/intel/microcode.h | 6 |
1 files changed, 5 insertions, 1 deletions
diff --git a/src/include/cpu/intel/microcode.h b/src/include/cpu/intel/microcode.h index a0ab7fd13b..d977e5836f 100644 --- a/src/include/cpu/intel/microcode.h +++ b/src/include/cpu/intel/microcode.h @@ -7,7 +7,11 @@ void intel_update_microcode_from_cbfs(void); /* Find a microcode that matches the revision and platform family returning * NULL if none found. The found microcode is cached for faster access on - * subsequent calls of this function. */ + * subsequent calls of this function. + * + * Since this function caches the found microcode (NULL or a valid microcode + * pointer), it is expected to be run from BSP before starting any other APs. + * It is not multithread safe otherwise. */ const void *intel_microcode_find(void); /* It is up to the caller to determine if parallel loading is possible as |