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>

2 files changed, 26 insertions, 8 deletions

diff --git a/src/cpu/intel/microcode/microcode.c b/src/cpu/intel/microcode/microcode.c
index 2c8a2121f2..51823ca6de 100644
--- a/src/cpu/intel/microcode/microcode.c
+++ b/src/cpu/intel/microcode/microcode.c
@@ -117,18 +117,15 @@ uint32_t get_microcode_checksum(const void *microcode)
 	return ((struct microcode *)microcode)->cksum;
 }
 
-const void *intel_microcode_find(void)
+static const void *find_cbfs_microcode(void)
 {
-	static const struct microcode *ucode_updates;
+	const struct microcode *ucode_updates;
 	size_t microcode_len;
 	u32 eax;
 	u32 pf, rev, sig, update_size;
 	msr_t msr;
 	struct cpuinfo_x86 c;
 
-	if (ucode_updates)
-		return ucode_updates;
-
 	ucode_updates = cbfs_map(MICROCODE_CBFS_FILE, &microcode_len);
 	if (ucode_updates == NULL)
 		return NULL;
@@ -170,11 +167,28 @@ const void *intel_microcode_find(void)
 		microcode_len -= update_size;
 	}
 
-	ucode_updates = NULL;
-
 	return NULL;
 }
 
+const void *intel_microcode_find(void)
+{
+	static bool microcode_checked;
+	static const void *ucode_update;
+
+	if (microcode_checked)
+		return ucode_update;
+
+	/*
+	 * 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. This sequence is not multithread safe otherwise.
+	 */
+	ucode_update = find_cbfs_microcode();
+	microcode_checked = true;
+
+	return ucode_update;
+}
+
 void intel_update_microcode_from_cbfs(void)
 {
 	const void *patch = intel_microcode_find();
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