Hello Zheng Bao, Zheng Bao,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to review the following change.
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
amd/00730F01: Clean the Microcode updating.
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , Change the code with the same way. Also, 41719 should be based on this.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h 5 files changed, 132 insertions(+), 148 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/1
diff --git a/src/cpu/amd/pi/00730F01/Makefile.inc b/src/cpu/amd/pi/00730F01/Makefile.inc index 45ec1d9..406fafb9 100644 --- a/src/cpu/amd/pi/00730F01/Makefile.inc +++ b/src/cpu/amd/pi/00730F01/Makefile.inc @@ -6,7 +6,6 @@ ramstage-y += chip_name.c ramstage-y += model_16_init.c ramstage-y += update_microcode.c -ramstage-y += microcode_fam16h.c
subdirs-y += ../../mtrr subdirs-y += ../../../x86/tsc diff --git a/src/cpu/amd/pi/00730F01/microcode_fam16h.c b/src/cpu/amd/pi/00730F01/microcode_fam16h.c deleted file mode 100644 index 7d47258..0000000 --- a/src/cpu/amd/pi/00730F01/microcode_fam16h.c +++ /dev/null @@ -1,129 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <stdint.h> -#include <console/console.h> -#include <cpu/x86/msr.h> -#include <cpu/amd/microcode.h> -#include <cbfs.h> - -/* - * Values and header structure from: - * BKDG for AMD Family 16h Models 30h-3Fh Processors - * 52740 Rev 3.06 - March 18, 2016 - */ - -#define F16H_MPB_MAX_SIZE 3458 -#define F16H_MPB_DATA_OFFSET 32 - - /* - * STRUCTURE OF A MICROCODE (UCODE) FILE FOR FAM16h - * Microcode Patch Block - * Microcode Header - * Microcode "Blob" - * ... - * ... - * (end of file) - * - * - * MICROCODE HEADER (offset 0 bytes from start of file) - * Total size = 32 bytes - * [0:3] Date code (32 bits) - * [4:7] Patch level (32 bits) - * [8:9] Microcode patch data ID (16 bits) - * [10:15] Reserved (48 bits) - * [16:19] Chipset 1 device ID (32 bits) - * [20:23] Chipset 2 device ID (32 bits) - * [24:25] Processor Revisions ID (16 bits) - * [26] Chipset 1 revision ID (8 bits) - * [27] Chipset 2 revision ID (8 bits) - * [28:31] Reserved (32 bits) - * - * MICROCODE BLOB (offset += 32) - * Total size = m bytes - * - */ - -struct microcode { - uint32_t date_code; - uint32_t patch_id; - - uint16_t mc_patch_data_id; - uint8_t reserved1[6]; - - uint32_t chipset1_dev_id; - uint32_t chipset2_dev_id; - - uint16_t processor_rev_id; - - uint8_t chipset1_rev_id; - uint8_t chipset2_rev_id; - - uint8_t reserved2[4]; - - uint8_t m_patch_data[F16H_MPB_MAX_SIZE-F16H_MPB_DATA_OFFSET]; - -}; - -static void apply_microcode_patch(const struct microcode *m) -{ - uint32_t new_patch_id; - msr_t msr; - - /* apply patch */ - msr.hi = 0; - msr.lo = (uint32_t)m; - - wrmsr(0xc0010020, msr); - - printk(BIOS_DEBUG, "microcode: patch id to apply = 0x%08x\n", - m->patch_id); - - /* patch authentication */ - msr = rdmsr(0x8b); - new_patch_id = msr.lo; - - printk(BIOS_DEBUG, "microcode: updated to patch id = 0x%08x %s\n", - new_patch_id, - (new_patch_id == m->patch_id) ? "success" : "fail"); -} - -static void amd_update_microcode(const void *ucode, size_t ucode_len, - uint16_t equivalent_processor_rev_id) -{ - const struct microcode *m; - const uint8_t *c = ucode; - - m = (struct microcode *)c; - - if (m->processor_rev_id == equivalent_processor_rev_id) - apply_microcode_patch(m); -} - -void amd_update_microcode_from_cbfs(uint16_t equivalent_processor_rev_id) -{ - const void *ucode; - size_t ucode_len; - - if (equivalent_processor_rev_id == 0) { - printk(BIOS_DEBUG, "microcode: rev id not found. " - "Skipping microcode patch!\n"); - return; - } - ucode = cbfs_boot_map_with_leak("cpu_microcode_blob.bin", - CBFS_TYPE_MICROCODE, - &ucode_len); - if (!ucode) { - printk(BIOS_DEBUG, "cpu_microcode_blob.bin not found. " - "Skipping updates.\n"); - return; - } - - if (ucode_len > F16H_MPB_MAX_SIZE || - ucode_len < F16H_MPB_DATA_OFFSET) { - printk(BIOS_DEBUG, "microcode file invalid. Skipping " - "updates.\n"); - return; - } - - amd_update_microcode(ucode, ucode_len, equivalent_processor_rev_id); -} diff --git a/src/cpu/amd/pi/00730F01/model_16_init.c b/src/cpu/amd/pi/00730F01/model_16_init.c index bdbb37d..2883b17 100644 --- a/src/cpu/amd/pi/00730F01/model_16_init.c +++ b/src/cpu/amd/pi/00730F01/model_16_init.c @@ -87,7 +87,7 @@ msr.lo |= (1 << 0); wrmsr(HWCR_MSR, msr);
- update_microcode(cpuid_eax(1)); + amd_update_microcode_from_cbfs(); }
static struct device_operations cpu_dev_ops = { diff --git a/src/cpu/amd/pi/00730F01/update_microcode.c b/src/cpu/amd/pi/00730F01/update_microcode.c index e3f0c3b..b1cb5af 100644 --- a/src/cpu/amd/pi/00730F01/update_microcode.c +++ b/src/cpu/amd/pi/00730F01/update_microcode.c @@ -2,6 +2,92 @@
#include <stdint.h> #include <cpu/amd/microcode.h> +#include <commonlib/helpers.h> +#include <console/console.h> +#include <cpu/x86/msr.h> +#include <cbfs.h> +#include <cpu/amd/msr.h> + +/* + * Values and header structure from: + * BKDG for AMD Family 16h Models 30h-3Fh Processors + * 52740 Rev 3.06 - March 18, 2016 + */ + +#define F16H_MPB_MAX_SIZE 3458 +#define F16H_MPB_DATA_OFFSET 32 + + /* + * STRUCTURE OF A MICROCODE (UCODE) FILE FOR FAM16h + * Microcode Patch Block + * Microcode Header + * Microcode "Blob" + * ... + * ... + * (end of file) + * + * + * MICROCODE HEADER (offset 0 bytes from start of file) + * Total size = 32 bytes + * [0:3] Date code (32 bits) + * [4:7] Patch level (32 bits) + * [8:9] Microcode patch data ID (16 bits) + * [10:15] Reserved (48 bits) + * [16:19] Chipset 1 device ID (32 bits) + * [20:23] Chipset 2 device ID (32 bits) + * [24:25] Processor Revisions ID (16 bits) + * [26] Chipset 1 revision ID (8 bits) + * [27] Chipset 2 revision ID (8 bits) + * [28:31] Reserved (32 bits) + * + * MICROCODE BLOB (offset += 32) + * Total size = m bytes + * + */ + +struct microcode { + uint32_t date_code; + uint32_t patch_id; + + uint16_t mc_patch_data_id; + uint8_t reserved1[6]; + + uint32_t chipset1_dev_id; + uint32_t chipset2_dev_id; + + uint16_t processor_rev_id; + + uint8_t chipset1_rev_id; + uint8_t chipset2_rev_id; + + uint8_t reserved2[4]; + + uint8_t m_patch_data[F16H_MPB_MAX_SIZE-F16H_MPB_DATA_OFFSET]; + +}; + +static void apply_microcode_patch(const struct microcode *m) +{ + uint32_t new_patch_id; + msr_t msr; + + /* apply patch */ + msr.hi = (uint64_t)(uintptr_t)m >> 32; + msr.lo = (uintptr_t)m & 0xFFFFFFFF; + + wrmsr(MSR_PATCH_LOADER, msr); + + printk(BIOS_DEBUG, "microcode: patch id to apply = 0x%08x\n", + m->patch_id); + + /* patch authentication */ + msr = rdmsr(MSR_PATCH_LEVEL); + new_patch_id = msr.lo; + + printk(BIOS_DEBUG, "microcode: updated to patch id = 0x%08x %s\n", + new_patch_id, + (new_patch_id == m->patch_id) ? "success" : "fail"); +}
struct id_mapping { uint32_t orig_id; @@ -12,20 +98,16 @@ { static const struct id_mapping id_mapping_table[] = { /* Family 16h */ - - /* TODO This equivalent processor revisions ID needs verification */ - { 0x730f01, 0x7301 }, - - /* Array terminator */ - { 0xffffff, 0x0000 }, + /* TODO This equivalent processor revisions ID needs verification */ + { 0x810f81, 0x8181 }, + { 0x820f01, 0x8201 }, + //{ 0x810f81, 0x8180 }, };
- u32 new_id; - int i; + u32 new_id = 0; + u8 i;
- new_id = 0; - - for (i = 0; id_mapping_table[i].orig_id != 0xffffff; i++) { + for (i = 0; i < ARRAY_SIZE(id_mapping_table); i++) { if (id_mapping_table[i].orig_id == orig_id) { new_id = id_mapping_table[i].new_id; break; @@ -35,9 +117,42 @@ return new_id; }
-void update_microcode(u32 cpu_deviceid) +static void amd_update_microcode(const void *ucode, size_t ucode_len, + uint16_t equivalent_processor_rev_id) { - u16 equivalent_processor_rev_id = - get_equivalent_processor_rev_id(cpu_deviceid); - amd_update_microcode_from_cbfs(equivalent_processor_rev_id); + const struct microcode *m; + const uint8_t *c = ucode; + + m = (struct microcode *)c; + + if (m->processor_rev_id == equivalent_processor_rev_id) + apply_microcode_patch(m); +} + +void amd_update_microcode_from_cbfs() +{ + const void *ucode; + size_t ucode_len; + uint16_t equivalent_processor_rev_id = get_equivalent_processor_rev_id(cpuid_eax(1));; + + if (equivalent_processor_rev_id == 0) { + printk(BIOS_DEBUG, "microcode: rev id not found. Skipping microcode patch!\n"); + return; + } + ucode = cbfs_boot_map_with_leak("cpu_microcode_blob.bin", + CBFS_TYPE_MICROCODE, + &ucode_len); + if (!ucode) { + printk(BIOS_DEBUG, "cpu_microcode_blob.bin not found. Skipping updates.\n"); + return; + } + + if (ucode_len > F16H_MPB_MAX_SIZE || + ucode_len < F16H_MPB_DATA_OFFSET) { + printk(BIOS_DEBUG, "microcode file invalid. Skipping " + "updates.\n"); + return; + } + + amd_update_microcode(ucode, ucode_len, equivalent_processor_rev_id); } diff --git a/src/include/cpu/amd/microcode.h b/src/include/cpu/amd/microcode.h index 29b5576..800661b 100644 --- a/src/include/cpu/amd/microcode.h +++ b/src/include/cpu/amd/microcode.h @@ -1,7 +1,6 @@ #ifndef CPU_AMD_MICROCODE_H #define CPU_AMD_MICROCODE_H
-void update_microcode(u32 cpu_deviceid); -void amd_update_microcode_from_cbfs(u16 equivalent_processor_rev_id); +void amd_update_microcode_from_cbfs(void);
#endif /* CPU_AMD_MICROCODE_H */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42094/1/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/1/src/cpu/amd/pi/00730F01/upd... PS1, Line 132: void amd_update_microcode_from_cbfs() Bad function definition - void amd_update_microcode_from_cbfs() should probably be void amd_update_microcode_from_cbfs(void)
https://review.coreboot.org/c/coreboot/+/42094/1/src/cpu/amd/pi/00730F01/upd... PS1, Line 136: uint16_t equivalent_processor_rev_id = get_equivalent_processor_rev_id(cpuid_eax(1));; Statements terminations use 1 semicolon
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Zheng Bao, Zheng Bao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#2).
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
amd/00730F01: Clean the Microcode updating.
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , Change the code with the same way. Also, 41719 should be based on this.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h 5 files changed, 132 insertions(+), 148 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/2
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Zheng Bao, Zheng Bao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#3).
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
amd/00730F01: Clean the Microcode updating.
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , Change the code with the same way. Also, 41719 should be based on this.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h M src/include/cpu/amd/msr.h 6 files changed, 133 insertions(+), 148 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
Patch Set 3: Code-Review-2
(1 comment)
The patch should ADD the support for Picasso and family 17h on top of existing infrastructure, NOT break behavior for older family and add support for Picasso on top of that.
https://review.coreboot.org/c/coreboot/+/42094/3/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/3/src/cpu/amd/pi/00730F01/upd... PS3, Line 17: { 0x730f01, 0x7301 }, Why do you remove support for family 16h?! This is src/cpu/amd/pi/00730F01/update_microcode.c for CPU models 00730F01. I have contributed this code so that the users of PC Engines apu2 (binaryPI 00730F01) could update their microcode: https://github.com/pcengines/apu2-documentation/issues/75
Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#4).
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
amd/00730F01: Clean the Microcode updating.
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , Change the code with the same way. Also, 41719 should be based on this.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h M src/include/cpu/amd/msr.h 6 files changed, 122 insertions(+), 159 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/4
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/3/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/3/src/cpu/amd/pi/00730F01/upd... PS3, Line 17: { 0x730f01, 0x7301 },
Why do you remove support for family 16h?! This is src/cpu/amd/pi/00730F01/update_microcode.c […]
Done. Sorry, I mixed up with picasso patch. Now we get rid of the table and use bitwise check.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
Patch Set 4: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@7 PS4, Line 7: amd/00730F01: Clean the Microcode updating. Please remove the trailing dot.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@10 PS4, Line 10: https://review.coreboot.org/c/coreboot/+/41719 Please summarize the comments in the commit message.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@13 PS4, Line 13: Also, 41719 should be based on this. This should maybe be a review comment, but it doesn’t belong in the commit message. As you are also the owner of that change-set, please ensure the ordering by putting the commit into the same branch.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@14 PS4, Line 14: Tested how?
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 131: "updates.\n"); Should fit on one line.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating. ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 104: m = (struct microcode *)c; : : if (m->processor_rev_id == equivalent_processor_rev_id) : apply_microcode_patch(m); does cpu_microcode_blob.bin only contain exactly one microcode update? if not, then this is likely a bug, since the code only lookes at the first one in the microcode binary
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 116: if (equivalent_processor_rev_id == 0) { : printk(BIOS_DEBUG, "microcode: rev id not found. Skipping microcode patch!\n"); : return; : } since no lookup table is used any more, this check can be removed
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 124: BIOS_DEBUG BIOS_WARNING maybe? should be at least BIOS_INFO though
Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#5).
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
amd/00730F01: Clean the Microcode updating
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , Change the code with the same way. Also, 41719 should be based on this.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h M src/include/cpu/amd/msr.h 6 files changed, 116 insertions(+), 159 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/5
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@7 PS4, Line 7: amd/00730F01: Clean the Microcode updating.
Please remove the trailing dot.
Done. Deleted.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@10 PS4, Line 10: https://review.coreboot.org/c/coreboot/+/41719
Please summarize the comments in the commit message.
Done. Summary has been added.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@13 PS4, Line 13: Also, 41719 should be based on this.
This should maybe be a review comment, but it doesn’t belong in the commit message. […]
Done. Reword the comments. Made it clear that the 41719 depends on this.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@14 PS4, Line 14:
Tested how?
Done. The platform is not available, but all the changes were carefully checked not to change the original flow.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 104: m = (struct microcode *)c; : : if (m->processor_rev_id == equivalent_processor_rev_id) : apply_microcode_patch(m);
does cpu_microcode_blob. […]
Currently only one blob is in the CBFS. I dont change the original code flow.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 116: if (equivalent_processor_rev_id == 0) { : printk(BIOS_DEBUG, "microcode: rev id not found. Skipping microcode patch!\n"); : return; : }
since no lookup table is used any more, this check can be removed
Done. Deleted.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 124: BIOS_DEBUG
BIOS_WARNING maybe? should be at least BIOS_INFO though
Done. Changed.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 131: "updates.\n");
Should fit on one line.
Done. Changed.
Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#6).
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
amd/00730F01: Clean the Microcode updating
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , which is about Microcode patch for amd/picasso. Change the code with the same way. And, 41719 depends on the definition in this patch.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h M src/include/cpu/amd/msr.h 6 files changed, 116 insertions(+), 159 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/6
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 116: if (equivalent_processor_rev_id == 0) { : printk(BIOS_DEBUG, "microcode: rev id not found. Skipping microcode patch!\n"); : return; : }
Done. Deleted.
Done.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@10 PS4, Line 10: https://review.coreboot.org/c/coreboot/+/41719
Done. Summary has been added.
I can’t see the summary in patch set 5.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
The rest looks good to me; would be good if Michał could verify that the microcode update still works on the APU2 board with the patch applied
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 104: m = (struct microcode *)c; : : if (m->processor_rev_id == equivalent_processor_rev_id) : apply_microcode_patch(m);
Currently only one blob is in the CBFS. I dont change the original code flow.
I'm ok with keeping the code in it's current form, but please add a comment about this issue, because this really is unexpected behaviour
Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#7).
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
amd/00730F01: Clean the Microcode updating
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , which is about Microcode patch for amd/picasso. Change the code with the same way.
We do not change the way it is. The code assume only one microcode is integrated in CBFS. 41719 is the example of integrating multiple binaries.
And, 41719 depends on the definition in this patch.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h M src/include/cpu/amd/msr.h 6 files changed, 117 insertions(+), 159 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/7
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@10 PS4, Line 10: https://review.coreboot.org/c/coreboot/+/41719
I can’t see the summary in patch set 5.
Done. It is in patch set 6. I just changed the code and edited gerrit at same time. Sometimes the gerrit is uploaded before git is pushed. Sorry for that.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 104: m = (struct microcode *)c; : : if (m->processor_rev_id == equivalent_processor_rev_id) : apply_microcode_patch(m);
I'm ok with keeping the code in it's current form, but please add a comment about this issue, becaus […]
Done. The comment was added. And more detailed description is added into git log.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/7//COMMIT_MSG@18 PS7, Line 18: And, 41719 depends on the definition in this patch. Sorry to get on your nerves, but reading this commit message, I have no idea what is done without looking at the diff. But that is what a commit message is for.
Maybe split the commit.
1. cpu/amd: Define and use macro for MSR_PATCH_LEVEL 2. amd/00730F01: Rename files to be more in line with … 3. amd/00730F01: Use amd_update_microcode_from_cbfs()
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 7: Code-Review+1
Patchset 4 worked well, also confirm patchset 7 works well.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 7: Code-Review+2
the MSR_PATCH_LEVEL addition could be split in another patch, but I'd be ok with the rest being one patch
Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#8).
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
amd/00730F01: Clean the Microcode updating
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , which is about Microcode patch for amd/picasso. Change the code with the same way.
The changes include: 1. combine the microcode_xxx.c and update_microcode.c into one source. 2. Redefine the microcode updating function to eliminate the parameter. Get the revision ID in the black box. Reduce the depth of function calls. 3. Get the revision ID by bitwise calculation instead of lookup table. 4. Reduce the confusing type casts. 5. Squash some lines.
We do not change the way it used to be. The code assume only one microcode is integrated in CBFS. If needed in future, 41719 is the example of integrating multiple binaries.
And, 41719 depends on the definition in this patch.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h 5 files changed, 115 insertions(+), 160 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/8
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/7//COMMIT_MSG@18 PS7, Line 18: And, 41719 depends on the definition in this patch.
Sorry to get on your nerves, but reading this commit message, I have no idea what is done without lo […]
Ack. MSR_PATCH_LEVEL has been moved to another patch. Other changes are mixed in bunches which is really hard to separate.
Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#9).
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
amd/00730F01: Clean the Microcode updating
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , which is about Microcode patch for amd/picasso. Change the code with the same way.
The changes include: 1. combine the microcode_xxx.c and update_microcode.c into one source. 2. Redefine the microcode updating function to eliminate the parameter. Get the revision ID in the black box. Reduce the depth of function calls. 3. Get the revision ID by bitwise calculation instead of lookup table. 4. Reduce the confusing type casts. 5. Squash some lines.
We do not change the way it used to be. The code assume only one microcode is integrated in CBFS. If needed in future, 41719 is the example of integrating multiple binaries.
And, 41719 depends on the definition in this patch.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h 5 files changed, 118 insertions(+), 160 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/9
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 9: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42094/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/7//COMMIT_MSG@18 PS7, Line 18: And, 41719 depends on the definition in this patch.
Ack. MSR_PATCH_LEVEL has been moved to another patch. […]
Ack
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... PS9, Line 88: updated update instead of updated, since the update failed
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... PS9, Line 88: updated
update instead of updated, since the update failed
i'd be ok with this being fixed in a follow-up patch, so that we can get the microcode update infrastructure submitted today
Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42094
to look at the new patch set (#10).
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
amd/00730F01: Clean the Microcode updating
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , which is about Microcode patch for amd/picasso. Change the code with the same way.
The changes include: 1. combine the microcode_xxx.c and update_microcode.c into one source. 2. Redefine the microcode updating function to eliminate the parameter. Get the revision ID in the black box. Reduce the depth of function calls. 3. Get the revision ID by bitwise calculation instead of lookup table. 4. Reduce the confusing type casts. 5. Squash some lines.
We do not change the way it used to be. The code assume only one microcode is integrated in CBFS. If needed in future, 41719 is the example of integrating multiple binaries.
And, 41719 depends on the definition in this patch.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h 5 files changed, 118 insertions(+), 160 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42094/10
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... PS9, Line 88: updated
i'd be ok with this being fixed in a follow-up patch, so that we can get the microcode update infras […]
Can "past participate" be subject? Use "being updated" in patchset 10.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/9/src/cpu/amd/pi/00730F01/upd... PS9, Line 88: updated
Can "past participate" be subject? […]
yep, that sounds better
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 10:
I'll just submit this one; patchset 9 was published more than 24h ago and patchset 10 only changed the wording of the console output which I consider a non-code change so I don't think that it needs another 24h cooldown
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
amd/00730F01: Clean the Microcode updating
According to the comments of https://review.coreboot.org/c/coreboot/+/41719 , which is about Microcode patch for amd/picasso. Change the code with the same way.
The changes include: 1. combine the microcode_xxx.c and update_microcode.c into one source. 2. Redefine the microcode updating function to eliminate the parameter. Get the revision ID in the black box. Reduce the depth of function calls. 3. Get the revision ID by bitwise calculation instead of lookup table. 4. Reduce the confusing type casts. 5. Squash some lines.
We do not change the way it used to be. The code assume only one microcode is integrated in CBFS. If needed in future, 41719 is the example of integrating multiple binaries.
And, 41719 depends on the definition in this patch.
Change-Id: I8b0da99db0d3189058f75e199f05492c4e6c5881 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42094 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/cpu/amd/pi/00730F01/Makefile.inc D src/cpu/amd/pi/00730F01/microcode_fam16h.c M src/cpu/amd/pi/00730F01/model_16_init.c M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/microcode.h 5 files changed, 118 insertions(+), 160 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/cpu/amd/pi/00730F01/Makefile.inc b/src/cpu/amd/pi/00730F01/Makefile.inc index 45ec1d9..406fafb9 100644 --- a/src/cpu/amd/pi/00730F01/Makefile.inc +++ b/src/cpu/amd/pi/00730F01/Makefile.inc @@ -6,7 +6,6 @@ ramstage-y += chip_name.c ramstage-y += model_16_init.c ramstage-y += update_microcode.c -ramstage-y += microcode_fam16h.c
subdirs-y += ../../mtrr subdirs-y += ../../../x86/tsc diff --git a/src/cpu/amd/pi/00730F01/microcode_fam16h.c b/src/cpu/amd/pi/00730F01/microcode_fam16h.c deleted file mode 100644 index 7d47258..0000000 --- a/src/cpu/amd/pi/00730F01/microcode_fam16h.c +++ /dev/null @@ -1,129 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <stdint.h> -#include <console/console.h> -#include <cpu/x86/msr.h> -#include <cpu/amd/microcode.h> -#include <cbfs.h> - -/* - * Values and header structure from: - * BKDG for AMD Family 16h Models 30h-3Fh Processors - * 52740 Rev 3.06 - March 18, 2016 - */ - -#define F16H_MPB_MAX_SIZE 3458 -#define F16H_MPB_DATA_OFFSET 32 - - /* - * STRUCTURE OF A MICROCODE (UCODE) FILE FOR FAM16h - * Microcode Patch Block - * Microcode Header - * Microcode "Blob" - * ... - * ... - * (end of file) - * - * - * MICROCODE HEADER (offset 0 bytes from start of file) - * Total size = 32 bytes - * [0:3] Date code (32 bits) - * [4:7] Patch level (32 bits) - * [8:9] Microcode patch data ID (16 bits) - * [10:15] Reserved (48 bits) - * [16:19] Chipset 1 device ID (32 bits) - * [20:23] Chipset 2 device ID (32 bits) - * [24:25] Processor Revisions ID (16 bits) - * [26] Chipset 1 revision ID (8 bits) - * [27] Chipset 2 revision ID (8 bits) - * [28:31] Reserved (32 bits) - * - * MICROCODE BLOB (offset += 32) - * Total size = m bytes - * - */ - -struct microcode { - uint32_t date_code; - uint32_t patch_id; - - uint16_t mc_patch_data_id; - uint8_t reserved1[6]; - - uint32_t chipset1_dev_id; - uint32_t chipset2_dev_id; - - uint16_t processor_rev_id; - - uint8_t chipset1_rev_id; - uint8_t chipset2_rev_id; - - uint8_t reserved2[4]; - - uint8_t m_patch_data[F16H_MPB_MAX_SIZE-F16H_MPB_DATA_OFFSET]; - -}; - -static void apply_microcode_patch(const struct microcode *m) -{ - uint32_t new_patch_id; - msr_t msr; - - /* apply patch */ - msr.hi = 0; - msr.lo = (uint32_t)m; - - wrmsr(0xc0010020, msr); - - printk(BIOS_DEBUG, "microcode: patch id to apply = 0x%08x\n", - m->patch_id); - - /* patch authentication */ - msr = rdmsr(0x8b); - new_patch_id = msr.lo; - - printk(BIOS_DEBUG, "microcode: updated to patch id = 0x%08x %s\n", - new_patch_id, - (new_patch_id == m->patch_id) ? "success" : "fail"); -} - -static void amd_update_microcode(const void *ucode, size_t ucode_len, - uint16_t equivalent_processor_rev_id) -{ - const struct microcode *m; - const uint8_t *c = ucode; - - m = (struct microcode *)c; - - if (m->processor_rev_id == equivalent_processor_rev_id) - apply_microcode_patch(m); -} - -void amd_update_microcode_from_cbfs(uint16_t equivalent_processor_rev_id) -{ - const void *ucode; - size_t ucode_len; - - if (equivalent_processor_rev_id == 0) { - printk(BIOS_DEBUG, "microcode: rev id not found. " - "Skipping microcode patch!\n"); - return; - } - ucode = cbfs_boot_map_with_leak("cpu_microcode_blob.bin", - CBFS_TYPE_MICROCODE, - &ucode_len); - if (!ucode) { - printk(BIOS_DEBUG, "cpu_microcode_blob.bin not found. " - "Skipping updates.\n"); - return; - } - - if (ucode_len > F16H_MPB_MAX_SIZE || - ucode_len < F16H_MPB_DATA_OFFSET) { - printk(BIOS_DEBUG, "microcode file invalid. Skipping " - "updates.\n"); - return; - } - - amd_update_microcode(ucode, ucode_len, equivalent_processor_rev_id); -} diff --git a/src/cpu/amd/pi/00730F01/model_16_init.c b/src/cpu/amd/pi/00730F01/model_16_init.c index bdbb37d..2883b17 100644 --- a/src/cpu/amd/pi/00730F01/model_16_init.c +++ b/src/cpu/amd/pi/00730F01/model_16_init.c @@ -87,7 +87,7 @@ msr.lo |= (1 << 0); wrmsr(HWCR_MSR, msr);
- update_microcode(cpuid_eax(1)); + amd_update_microcode_from_cbfs(); }
static struct device_operations cpu_dev_ops = { diff --git a/src/cpu/amd/pi/00730F01/update_microcode.c b/src/cpu/amd/pi/00730F01/update_microcode.c index e3f0c3b..11e2531 100644 --- a/src/cpu/amd/pi/00730F01/update_microcode.c +++ b/src/cpu/amd/pi/00730F01/update_microcode.c @@ -2,42 +2,131 @@
#include <stdint.h> #include <cpu/amd/microcode.h> +#include <commonlib/helpers.h> +#include <console/console.h> +#include <cpu/x86/msr.h> +#include <cpu/amd/msr.h> +#include <cbfs.h>
-struct id_mapping { - uint32_t orig_id; - uint16_t new_id; -}; +/* + * Values and header structure from: + * BKDG for AMD Family 16h Models 30h-3Fh Processors + * 52740 Rev 3.06 - March 18, 2016 + */
-static u16 get_equivalent_processor_rev_id(u32 orig_id) +#define F16H_MPB_MAX_SIZE 3458 +#define F16H_MPB_DATA_OFFSET 32 + + /* + * STRUCTURE OF A MICROCODE (UCODE) FILE FOR FAM16h + * Microcode Patch Block + * Microcode Header + * Microcode "Blob" + * ... + * ... + * (end of file) + * + * + * MICROCODE HEADER (offset 0 bytes from start of file) + * Total size = 32 bytes + * [0:3] Date code (32 bits) + * [4:7] Patch level (32 bits) + * [8:9] Microcode patch data ID (16 bits) + * [10:15] Reserved (48 bits) + * [16:19] Chipset 1 device ID (32 bits) + * [20:23] Chipset 2 device ID (32 bits) + * [24:25] Processor Revisions ID (16 bits) + * [26] Chipset 1 revision ID (8 bits) + * [27] Chipset 2 revision ID (8 bits) + * [28:31] Reserved (32 bits) + * + * MICROCODE BLOB (offset += 32) + * Total size = m bytes + * + */ + +struct microcode { + uint32_t date_code; + uint32_t patch_id; + + uint16_t mc_patch_data_id; + uint8_t reserved1[6]; + + uint32_t chipset1_dev_id; + uint32_t chipset2_dev_id; + + uint16_t processor_rev_id; + + uint8_t chipset1_rev_id; + uint8_t chipset2_rev_id; + + uint8_t reserved2[4]; + + uint8_t m_patch_data[F16H_MPB_MAX_SIZE-F16H_MPB_DATA_OFFSET]; +} __packed; + +static void apply_microcode_patch(const struct microcode *m) { - static const struct id_mapping id_mapping_table[] = { - /* Family 16h */ + uint32_t new_patch_id; + msr_t msr;
- /* TODO This equivalent processor revisions ID needs verification */ - { 0x730f01, 0x7301 }, + msr.hi = (uint64_t)(uintptr_t)m >> 32; + msr.lo = (uintptr_t)m & 0xffffffff;
- /* Array terminator */ - { 0xffffff, 0x0000 }, - }; + wrmsr(MSR_PATCH_LOADER, msr);
- u32 new_id; - int i; + printk(BIOS_DEBUG, "microcode: patch id to apply = 0x%08x\n", + m->patch_id);
- new_id = 0; + msr = rdmsr(MSR_PATCH_LEVEL); + new_patch_id = msr.lo;
- for (i = 0; id_mapping_table[i].orig_id != 0xffffff; i++) { - if (id_mapping_table[i].orig_id == orig_id) { - new_id = id_mapping_table[i].new_id; - break; - } - } - - return new_id; + if (new_patch_id == m->patch_id) + printk(BIOS_INFO, "microcode: being updated to patch id = 0x%08x succeeded\n", + new_patch_id); + else + printk(BIOS_ERR, "microcode: being updated to patch id = 0x%08x failed\n", + new_patch_id); }
-void update_microcode(u32 cpu_deviceid) +static uint16_t get_equivalent_processor_rev_id(void) { - u16 equivalent_processor_rev_id = - get_equivalent_processor_rev_id(cpu_deviceid); - amd_update_microcode_from_cbfs(equivalent_processor_rev_id); + uint32_t cpuid_family = cpuid_eax(1); + + return (uint16_t)((cpuid_family & 0xff0000) >> 8 | (cpuid_family & 0xff)); +} + +static void amd_update_microcode(const void *ucode, size_t ucode_len, + uint16_t equivalent_processor_rev_id) +{ + const struct microcode *m; + const uint8_t *c = ucode; + + m = (struct microcode *)c; + + /* Assume cpu_microcode_blob only contains one microcode. */ + if (m->processor_rev_id == equivalent_processor_rev_id) + apply_microcode_patch(m); +} + +void amd_update_microcode_from_cbfs(void) +{ + const void *ucode; + size_t ucode_len; + uint16_t equivalent_processor_rev_id = get_equivalent_processor_rev_id(); + + ucode = cbfs_boot_map_with_leak("cpu_microcode_blob.bin", + CBFS_TYPE_MICROCODE, &ucode_len); + if (!ucode) { + printk(BIOS_WARNING, "cpu_microcode_blob.bin not found. Skipping updates.\n"); + return; + } + + if (ucode_len > F16H_MPB_MAX_SIZE || + ucode_len < F16H_MPB_DATA_OFFSET) { + printk(BIOS_DEBUG, "microcode file invalid. Skipping updates.\n"); + return; + } + + amd_update_microcode(ucode, ucode_len, equivalent_processor_rev_id); } diff --git a/src/include/cpu/amd/microcode.h b/src/include/cpu/amd/microcode.h index 29b5576..800661b 100644 --- a/src/include/cpu/amd/microcode.h +++ b/src/include/cpu/amd/microcode.h @@ -1,7 +1,6 @@ #ifndef CPU_AMD_MICROCODE_H #define CPU_AMD_MICROCODE_H
-void update_microcode(u32 cpu_deviceid); -void amd_update_microcode_from_cbfs(u16 equivalent_processor_rev_id); +void amd_update_microcode_from_cbfs(void);
#endif /* CPU_AMD_MICROCODE_H */
-
Bao Zheng (Code Review) -
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Michał Żygowski (Code Review)
-
Paul Menzel (Code Review)