Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/74777?usp=email )
Change subject: soc/amd/common/block/cpu: Refactor ucode allocation ......................................................................
soc/amd/common/block/cpu: Refactor ucode allocation
Move microcode load/unload to pre_mp_init and post_mp_init callbacks. It allows to make sure that ucode is freed only if all APs updated microcode.
BUG=b:278264488 TEST=Build and run with additional debug prints added to confirm that data are correctly unmapped
Change-Id: I200d24df6157cc6d06bade34809faefea9f0090a Signed-off-by: Grzegorz Bernacki bernacki@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/74777 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/include/cpu/amd/microcode.h M src/soc/amd/cezanne/cpu.c M src/soc/amd/common/block/cpu/smm/smm_relocate.c M src/soc/amd/common/block/cpu/update_microcode.c M src/soc/amd/glinda/cpu.c M src/soc/amd/mendocino/cpu.c M src/soc/amd/phoenix/cpu.c M src/soc/amd/picasso/cpu.c 8 files changed, 62 insertions(+), 37 deletions(-)
Approvals: Karthik Ramasubramanian: Looks good to me, approved build bot (Jenkins): Verified
diff --git a/src/include/cpu/amd/microcode.h b/src/include/cpu/amd/microcode.h index 9e88936..b6b158c 100644 --- a/src/include/cpu/amd/microcode.h +++ b/src/include/cpu/amd/microcode.h @@ -4,6 +4,9 @@ #define CPU_AMD_MICROCODE_H
void amd_update_microcode_from_cbfs(void); +void amd_load_microcode_from_cbfs(void); +void amd_free_microcode(void); +void amd_apply_microcode_patch(void); void preload_microcode(void);
#endif /* CPU_AMD_MICROCODE_H */ diff --git a/src/soc/amd/cezanne/cpu.c b/src/soc/amd/cezanne/cpu.c index 6149a84..9261d54 100644 --- a/src/soc/amd/cezanne/cpu.c +++ b/src/soc/amd/cezanne/cpu.c @@ -39,7 +39,7 @@ check_mca(); set_cstate_io_addr();
- amd_update_microcode_from_cbfs(); + amd_apply_microcode_patch(); }
static struct device_operations cpu_dev_ops = { diff --git a/src/soc/amd/common/block/cpu/smm/smm_relocate.c b/src/soc/amd/common/block/cpu/smm/smm_relocate.c index 1b929c7..2e3bff1 100644 --- a/src/soc/amd/common/block/cpu/smm/smm_relocate.c +++ b/src/soc/amd/common/block/cpu/smm/smm_relocate.c @@ -6,6 +6,7 @@ #include <amdblocks/smm.h> #include <console/console.h> #include <cpu/amd/amd64_save_state.h> +#include <cpu/amd/microcode.h> #include <cpu/amd/msr.h> #include <cpu/amd/mtrr.h> #include <cpu/cpu.h> @@ -22,6 +23,8 @@ else x86_setup_mtrrs_with_detect(); x86_mtrr_check(); + if (CONFIG(SOC_AMD_COMMON_BLOCK_UCODE)) + amd_load_microcode_from_cbfs(); }
static void get_smm_info(uintptr_t *perm_smbase, size_t *perm_smsize, @@ -83,10 +86,17 @@ lock_smm(); }
+static void post_mp_init(void) +{ + if (CONFIG(SOC_AMD_COMMON_BLOCK_UCODE)) + amd_free_microcode(); + global_smi_enable(); +} + const struct mp_ops amd_mp_ops_with_smm = { .pre_mp_init = pre_mp_init, .get_cpu_count = get_cpu_count, .get_smm_info = get_smm_info, .per_cpu_smm_trigger = smm_relocation_handler, - .post_mp_init = global_smi_enable, + .post_mp_init = post_mp_init, }; diff --git a/src/soc/amd/common/block/cpu/update_microcode.c b/src/soc/amd/common/block/cpu/update_microcode.c index 0edb9d2..14c4f36 100644 --- a/src/soc/amd/common/block/cpu/update_microcode.c +++ b/src/soc/amd/common/block/cpu/update_microcode.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <amdblocks/cpu.h> #include <cbfs.h> #include <commonlib/helpers.h> #include <console/console.h> @@ -32,21 +33,30 @@ uint8_t reserved2[4]; } __packed;
-static void apply_microcode_patch(const struct microcode *m) +static const struct microcode *ucode; + +/* This function requires the ucode variable to be initialized by amd_load_microcode_from_cbfs() + and then allocated memory should be freed by the amd_free_microcode(). */ +void amd_apply_microcode_patch(void) { uint32_t new_patch_id; msr_t msr;
- msr.raw = (uintptr_t)m; + if (!ucode) { + printk(BIOS_ERR, "%s: NULL pointer to microcode\n", __func__); + return; + } + + msr.raw = (uintptr_t)ucode;
wrmsr(MSR_PATCH_LOADER, msr);
- printk(BIOS_DEBUG, "microcode: patch id to apply = 0x%08x\n", m->patch_id); + printk(BIOS_DEBUG, "microcode: patch id to apply = 0x%08x\n", ucode->patch_id);
msr = rdmsr(IA32_BIOS_SIGN_ID); new_patch_id = msr.lo;
- if (new_patch_id == m->patch_id) + if (new_patch_id == ucode->patch_id) printk(BIOS_INFO, "microcode: being updated to patch id = 0x%08x succeeded\n", new_patch_id); else @@ -61,11 +71,11 @@ return (uint16_t)((cpuid_family & 0xff0000) >> 8 | (cpuid_family & 0xff)); }
-static const struct microcode *find_microcode(const struct microcode *ucode, +static const struct microcode *find_microcode(const struct microcode *microcode, uint16_t equivalent_processor_rev_id) { - if (ucode->processor_rev_id == equivalent_processor_rev_id) - return ucode; + if (microcode->processor_rev_id == equivalent_processor_rev_id) + return microcode;
printk(BIOS_WARNING, "Failed to find microcode for processor rev: %hx.\n", equivalent_processor_rev_id); @@ -73,39 +83,41 @@ return NULL; }
-void amd_update_microcode_from_cbfs(void) +void amd_load_microcode_from_cbfs(void) { - static const struct microcode *ucode_cache; - static bool cache_valid; - - struct microcode *ucode; char name[] = CPU_MICROCODE_BLOB_NAME; uint16_t equivalent_processor_rev_id;
- /* Cache the buffer so each CPU doesn't need to read the uCode from flash */ - /* Note that this code assumes all cores are the same */ - if (!cache_valid) { - timestamp_add_now(TS_READ_UCODE_START); - equivalent_processor_rev_id = get_equivalent_processor_rev_id(); - snprintf(name, sizeof(name), CPU_MICROCODE_BLOB_FORMAT, equivalent_processor_rev_id); - ucode = cbfs_map(name, NULL); - if (!ucode) { - printk(BIOS_WARNING, "%s not found. Skipping updates.\n", name); - return; - } + if (ucode) + return;
- ucode_cache = find_microcode(ucode, equivalent_processor_rev_id); + /* Store the pointer to the buffer in global variable, so each CPU doesn't need to read + * the uCode from flash. Note that this code assumes all cores are the same */ + timestamp_add_now(TS_READ_UCODE_START); + equivalent_processor_rev_id = get_equivalent_processor_rev_id(); + snprintf(name, sizeof(name), CPU_MICROCODE_BLOB_FORMAT, equivalent_processor_rev_id);
- if (!ucode_cache) { - cbfs_unmap(ucode); - return; - } - - cache_valid = true; + ucode = cbfs_map(name, NULL); + if (!ucode) { + printk(BIOS_WARNING, "%s not found. Skipping updates.\n", name); timestamp_add_now(TS_READ_UCODE_END); + return; }
- apply_microcode_patch(ucode_cache); + if (find_microcode(ucode, equivalent_processor_rev_id) == NULL) { + cbfs_unmap((void *)ucode); + ucode = NULL; + } + + timestamp_add_now(TS_READ_UCODE_END); +} + +void amd_free_microcode(void) +{ + if (ucode) { + cbfs_unmap((void *)ucode); + ucode = NULL; + } }
void preload_microcode(void) diff --git a/src/soc/amd/glinda/cpu.c b/src/soc/amd/glinda/cpu.c index c745b73..49cd11c 100644 --- a/src/soc/amd/glinda/cpu.c +++ b/src/soc/amd/glinda/cpu.c @@ -42,7 +42,7 @@ check_mca(); set_cstate_io_addr();
- amd_update_microcode_from_cbfs(); + amd_apply_microcode_patch(); }
static struct device_operations cpu_dev_ops = { diff --git a/src/soc/amd/mendocino/cpu.c b/src/soc/amd/mendocino/cpu.c index b9297b4..c742db0 100644 --- a/src/soc/amd/mendocino/cpu.c +++ b/src/soc/amd/mendocino/cpu.c @@ -40,7 +40,7 @@ check_mca(); set_cstate_io_addr();
- amd_update_microcode_from_cbfs(); + amd_apply_microcode_patch(); }
static struct device_operations cpu_dev_ops = { diff --git a/src/soc/amd/phoenix/cpu.c b/src/soc/amd/phoenix/cpu.c index 848f667..2c8f7713 100644 --- a/src/soc/amd/phoenix/cpu.c +++ b/src/soc/amd/phoenix/cpu.c @@ -42,7 +42,7 @@ check_mca(); set_cstate_io_addr();
- amd_update_microcode_from_cbfs(); + amd_apply_microcode_patch(); }
static struct device_operations cpu_dev_ops = { diff --git a/src/soc/amd/picasso/cpu.c b/src/soc/amd/picasso/cpu.c index c5fc8a8..c0b918e 100644 --- a/src/soc/amd/picasso/cpu.c +++ b/src/soc/amd/picasso/cpu.c @@ -40,7 +40,7 @@ check_mca(); set_cstate_io_addr();
- amd_update_microcode_from_cbfs(); + amd_apply_microcode_patch(); }
static struct device_operations cpu_dev_ops = {