Hello Zheng Bao,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to review the following change.
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. "Include external microcode binary" need to be seleceted. The size of each binary is hardcoded.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 5 files changed, 194 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/1
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 27c0232..5c205e7 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -60,6 +60,8 @@ select FSP_USES_CB_STACK select UDK_2017_BINDING select HAVE_CF9_RESET + select SUPPORT_CPU_UCODE_IN_CBFS + select MICROCODE_BLOB_UNDISCLOSED
config AMD_FP5 def_bool y if !AMD_FT5 @@ -382,6 +384,9 @@ depends on HAVE_PSP_WHITELIST_FILE default "3rdparty/blobs/soc/amd/picasso/PSP/wtl-rvn.sbin"
+config CPU_UCODE_BINARIES + default "3rdparty/blobs/soc/amd/picasso/PSP/UcodePatch_PCO_B0.bin 3rdparty/blobs/soc/amd/picasso/PSP/UcodePatch_PCO_B1.bin 3rdparty/blobs/soc/amd/picasso/PSP/UcodePatch_RV2_A0.bin" + endmenu
endif # SOC_AMD_PICASSO diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 9dda834..f88695e 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -81,6 +81,8 @@ ramstage-y += finalize.c ramstage-y += soc_util.c ramstage-y += psp.c +ramstage-y += update_microcode.c +ramstage-y += microcode_picasso.c
all-y += reset.c
@@ -367,9 +369,6 @@ $(OPT_PSP_PMUD_FILE2) \ $(OPT_PSP_PMUD_FILE3) \ $(OPT_PSP_PMUD_FILE4) \ - $(OPT_PSP_UCODE_FILE1) \ - $(OPT_PSP_UCODE_FILE2) \ - $(OPT_PSP_UCODE_FILE3) \ $(OPT_MP2CFG_FILE) \ $(OPT_ABL0_FILE) \ $(OPT_ABL1_FILE) \ diff --git a/src/soc/amd/picasso/cpu.c b/src/soc/amd/picasso/cpu.c index 2325994..c0589f2 100644 --- a/src/soc/amd/picasso/cpu.c +++ b/src/soc/amd/picasso/cpu.c @@ -17,6 +17,7 @@ #include <soc/smi.h> #include <soc/iomap.h> #include <console/console.h> +#include <cpu/amd/microcode.h>
/* * MP and SMM loading initialization. @@ -109,6 +110,8 @@ { check_mca(); setup_lapic(); + + update_microcode(cpuid_eax(1)); }
static struct device_operations cpu_dev_ops = { @@ -117,6 +120,8 @@
static struct cpu_device_id cpu_table[] = { { X86_VENDOR_AMD, 0x810f80 }, + { X86_VENDOR_AMD, 0x810f81 }, + { X86_VENDOR_AMD, 0x820f81 }, { X86_VENDOR_AMD, PICASSO_CPUID }, { X86_VENDOR_AMD, RAVEN2_CPUID }, { 0, 0 }, diff --git a/src/soc/amd/picasso/microcode_picasso.c b/src/soc/amd/picasso/microcode_picasso.c new file mode 100644 index 0000000..fdc0bc6 --- /dev/null +++ b/src/soc/amd/picasso/microcode_picasso.c @@ -0,0 +1,135 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#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, + uint32_t equivalent_processor_rev_id) +{ + const struct microcode *m; + const uint8_t *c = ucode; + + m = (struct microcode *)c; + + while ((uint32_t)m < (uint32_t)c + ucode_len) { + if (m->processor_rev_id == equivalent_processor_rev_id) + apply_microcode_patch(m); + m = (struct microcode *)((uint32_t)m + 3200); + } +} + +void amd_update_microcode_from_cbfs(uint32_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 0 + if (ucode_len > F16H_MPB_MAX_SIZE || + ucode_len < F16H_MPB_DATA_OFFSET) { + printk(BIOS_DEBUG, "microcode file invalid. Skipping " + "updates.\n"); + return; + } + #endif + + amd_update_microcode(ucode, ucode_len, equivalent_processor_rev_id); +} diff --git a/src/soc/amd/picasso/update_microcode.c b/src/soc/amd/picasso/update_microcode.c new file mode 100644 index 0000000..ab7da2c --- /dev/null +++ b/src/soc/amd/picasso/update_microcode.c @@ -0,0 +1,47 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <stdint.h> +#include <cpu/amd/microcode.h> + +struct id_mapping { + uint32_t orig_id; + uint16_t new_id; +}; + +static u16 get_equivalent_processor_rev_id(u32 orig_id) +{ + static const struct id_mapping id_mapping_table[] = { + /* Family 16h */ + + /* TODO This equivalent processor revisions ID needs verification */ + { 0x730f01, 0x7301 }, + { 0x810f81, 0x8181 }, + { 0x820f01, 0x8201 }, + //{ 0x810f81, 0x8180 }, + + /* Array terminator */ + { 0xffffff, 0x0000 }, + }; + + u32 new_id; + int i; + + new_id = 0; + + 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; +} + +void update_microcode(u32 cpu_deviceid) +{ + u32 equivalent_processor_rev_id = + get_equivalent_processor_rev_id(cpu_deviceid); + amd_update_microcode_from_cbfs(equivalent_processor_rev_id); +}
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 1:
The files are present in 3rdparty/amd_blobs/ now. AFAICS, we only need SUPPORT_CPU_UCODE_IN_CBFS. And in the `Makefile.inc` add the files, e.g.:
cpu_microcode_bins += $(wildcard 3rdparty/amd_blobs/picasso/PSP/UcodePatch_*.bin)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 1:
It seems to me that `update_microcode.c` is Picasso specific, but `microcode_picasso.c` is not? Could the latter be used in common/ code?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 102: 3200 Can there be a macro for this somewhere?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 106: uint32_t equivalent_processor_rev_id It might be good to explain this equivalent_processor_rev_id in the commit message?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 125: #if 0 : if (ucode_len > F16H_MPB_MAX_SIZE || : ucode_len < F16H_MPB_DATA_OFFSET) { : printk(BIOS_DEBUG, "microcode file invalid. Skipping " : "updates.\n"); : return; : } : #endif remove?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 23: /* Array terminator */ : { 0xffffff, 0x0000 }, any reason to not use ARRAY_SIZE in the for loop?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 27: u32 new_id; initialize here?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 43: : u32 equivalent_processor_rev_id = : get_equivalent_processor_rev_id(cpu_deviceid); : amd_update_microcode_from_cbfs(equivalent_processor_rev_id); amd_update_microcode_from_cbfs takes a u32 as argument, but get_equivalent_processor_rev_id returns a u16?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG@7 PS1, Line 7: Picasso amd/picasso
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG@11 PS1, Line 11: seleceted selected
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 2: /* This file is part of the coreboot project. */ Please remove the second line.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c File src/soc/amd/picasso/cpu.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c@1... PS1, Line 123: { X86_VENDOR_AMD, 0x810f81 }, duplicate of PICASSO_CPUID
also I've added more CPUIDs in this still unmerged patch https://review.coreboot.org/c/coreboot/+/41641/6/src/soc/amd/picasso/include... Beware though that the current Mandolin code doesn't apply or work with current master. The plan is to land Google/zork in upstream and then getting Mandolin to work again on upstream; there were some breaking changes.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 1:
(16 comments)
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Kconfig... PS1, Line 388: 3rdparty/blobs We're using 3rdparty/amd_blobs now.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... PS1, Line 229: # type = 0x66 : PSP_UCODE_FILE1=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B1.bin : PSP_UCODE_FILE2=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B0.bin : PSP_UCODE_FILE3=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_RV2_A0.bin remove these too
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... PS1, Line 290: OPT_PSP_UCODE_FILE1=$(call add_opt_prefix, $(PSP_UCODE_FILE1), --instance 0 --ucode) : OPT_PSP_UCODE_FILE2=$(call add_opt_prefix, $(PSP_UCODE_FILE2), --instance 1 --ucode) : OPT_PSP_UCODE_FILE3=$(call add_opt_prefix, $(PSP_UCODE_FILE3), --instance 2 --ucode) Remove these too
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c File src/soc/amd/picasso/cpu.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c@1... PS1, Line 122: { X86_VENDOR_AMD, 0x810f80 }, : { X86_VENDOR_AMD, 0x810f81 }, : { X86_VENDOR_AMD, 0x820f81 }, It seems like we should split this into a separate patch and use #define values for the IDs.
0x810f80 is Picasso Step 0, unreleased product 0x810f81 is a duplicate of PICASSO_CPUID? 0x820f81: Not sure what this is. Do you have a board with this?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 12: BKDG for AMD Family 16h Models 30h-3Fh Processors : * 52740 Rev 3.06 - March 18, 2016 It looks like only the NDA version and not the public version AFAICS. If we need to cite an NDA doc, let's use the Picasso one. We may be able to find a public doc at https://developer.amd.com/resources/developer-guides-manuals/ that defines the info, though.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 16: F16H_MPB_MAX_SIZE 3458 : #define F16H_MPB_DATA_OFFSET 32 Even though you've said the MPB definition can be found in a Family 16h BKDG, I wouldn't use F16H in the names
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 20: FAM16h Same idea. I would remove this.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 74: msr.hi = 0; : msr.lo = (uint32_t)m; Maybe
.hi = (uint64_t)(uintptr_t)m >> 32; .lo = (uintptr_t)m & 0xffffffff;
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 77: 0xc0010020 MSR_PATCH_LOADER in cpu/amd/msr.h
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 80: m->patch_id); Nit, I would indent another tab stop here and on lines 87, 88.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 83: 0x8b We should add to cpu/amd/msr.h MSR_PATCH_LEVEL
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 99: uint32_t Maybe cast to uint8_t * to do pointer math?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 113: "Skipping microcode patch!\n"); We're not limited on line width for printk format strings, so maybe consider placing this on the same line if it helps readability.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 121: "Skipping updates.\n"); Same as above.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 15: * Family 16h */ remove
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 18: { 0x730f01, 0x7301 }, remove
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#2).
Change subject: AMD/Picasso: Load x86 microcode from CBFS modules ......................................................................
AMD/Picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. "Include external microcode binary" need to be selected.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com --- M src/include/cpu/amd/microcode.h M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c M src/soc/amd/picasso/include/soc/cpu.h A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 8 files changed, 159 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: AMD/Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 2:
(2 comments)
This also breaks the APU boards.
``` CC ramstage/cpu/amd/pi/00730F01/microcode_fam16h.o src/cpu/amd/pi/00730F01/microcode_fam16h.c:103:6: error: conflicting types for 'amd_update_microcode_from_cbfs' void amd_update_microcode_from_cbfs(uint32_t equivalent_processor_rev_id) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from src/cpu/amd/pi/00730F01/microcode_fam16h.c:7: src/include/cpu/amd/microcode.h:5:6: note: previous declaration of 'amd_update_microcode_from_cbfs' was here void amd_update_microcode_from_cbfs(u16 equivalent_processor_rev_id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ```
https://review.coreboot.org/c/coreboot/+/41719/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/2//COMMIT_MSG@6 PS2, Line 6: : AMD/Picasso Please make it lowercase.
https://review.coreboot.org/c/coreboot/+/41719/2/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/2/src/soc/amd/picasso/microco... PS2, Line 2: /* This file is part of the coreboot project. */ This line should be removed.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#3).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com --- M src/include/cpu/amd/microcode.h M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c M src/soc/amd/picasso/include/soc/cpu.h A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 8 files changed, 152 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/3
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#5).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c M src/soc/amd/picasso/include/soc/cpu.h A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 7 files changed, 151 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/5
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG@11 PS5, Line 11: Why is this Picasso specific? Can’t this be put into common code?
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... PS5, Line 20: #define RAVEN1_B0_CPUID 0x00810f10 Looks exactly as above?
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/update_... PS5, Line 27: int unsigned
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#6).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 6 files changed, 145 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/6
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
(1 comment)
Has rebased on latest change with new revision ID.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c File src/soc/amd/picasso/cpu.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c@1... PS1, Line 123: { X86_VENDOR_AMD, 0x810f81 },
duplicate of PICASSO_CPUID […]
Has rebased on latest change with new revision ID.
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
Do I need to click "resolve" for each comment?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
Patch Set 7:
Do I need to click "resolve" for each comment?
Unfortunately, yes. Gerrit is now set up to check for "All-Comments-Resolved".
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
Patch Set 7:
Do I need to click "resolve" for each comment?
yes. patches can only be submitted when all comments are marked as resolved. that was a change maybe a year or a year and a half ago
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
(1 comment)
Patch Set 7:
Patch Set 7:
Do I need to click "resolve" for each comment?
yes. patches can only be submitted when all comments are marked as resolved. that was a change maybe a year or a year and a half ago
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... PS5, Line 20: #define RAVEN1_B0_CPUID 0x00810f10
Looks exactly as above?
Done
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
Patch Set 7:
Patch Set 7:
Do I need to click "resolve" for each comment?
yes. patches can only be submitted when all comments are marked as resolved. that was a change maybe a year or a year and a half ago
Just for practicing resolving comments.
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
(26 comments)
Resolve comments in a batch.
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG@7 PS1, Line 7: Picasso
amd/picasso
Done. Changed.
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG@11 PS1, Line 11: seleceted
selected
Done. Typo has been fixed.
https://review.coreboot.org/c/coreboot/+/41719/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/2//COMMIT_MSG@6 PS2, Line 6: : AMD/Picasso
Please make it lowercase.
Done. Changed in latest patchset.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Kconfig... PS1, Line 388: 3rdparty/blobs
We're using 3rdparty/amd_blobs now.
Done. This config has been removed. Instead cpu_microcode_bins is defined.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... PS1, Line 229: # type = 0x66 : PSP_UCODE_FILE1=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B1.bin : PSP_UCODE_FILE2=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B0.bin : PSP_UCODE_FILE3=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_RV2_A0.bin
remove these too
Done. removed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... PS1, Line 290: OPT_PSP_UCODE_FILE1=$(call add_opt_prefix, $(PSP_UCODE_FILE1), --instance 0 --ucode) : OPT_PSP_UCODE_FILE2=$(call add_opt_prefix, $(PSP_UCODE_FILE2), --instance 1 --ucode) : OPT_PSP_UCODE_FILE3=$(call add_opt_prefix, $(PSP_UCODE_FILE3), --instance 2 --ucode)
Remove these too
Done
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c File src/soc/amd/picasso/cpu.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c@1... PS1, Line 122: { X86_VENDOR_AMD, 0x810f80 }, : { X86_VENDOR_AMD, 0x810f81 }, : { X86_VENDOR_AMD, 0x820f81 },
It seems like we should split this into a separate patch and use #define values for the IDs. […]
Done. Has rebased on latest change with new revision ID.
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... PS5, Line 20: #define RAVEN1_B0_CPUID 0x00810f10
Done
The duplicated definitions have been removed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 2: /* This file is part of the coreboot project. */
Please remove the second line.
Done
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 12: BKDG for AMD Family 16h Models 30h-3Fh Processors : * 52740 Rev 3.06 - March 18, 2016
It looks like only the NDA version and not the public version AFAICS. […]
Done
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 16: F16H_MPB_MAX_SIZE 3458 : #define F16H_MPB_DATA_OFFSET 32
Even though you've said the MPB definition can be found in a Family 16h BKDG, I wouldn't use F16H in […]
Done. Now it has been changed to #define F17H_MPB_MAX_SIZE 3200 #define F17H_MPB_DATA_OFFSET 32
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 20: FAM16h
Same idea. I would remove this.
Done
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 74: msr.hi = 0; : msr.lo = (uint32_t)m;
Maybe […]
Done.
changed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 77: 0xc0010020
MSR_PATCH_LOADER in cpu/amd/msr. […]
Done
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 80: m->patch_id);
Nit, I would indent another tab stop here and on lines 87, 88.
Done. Fixed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 83: 0x8b
We should add to cpu/amd/msr. […]
Done. Changed. The definition is already there.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 99: uint32_t
Maybe cast to uint8_t * to do pointer math?
Done. Changed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 102: 3200
Can there be a macro for this somewhere?
Done. Changed to macro.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 113: "Skipping microcode patch!\n");
We're not limited on line width for printk format strings, so maybe consider placing this on the sam […]
Done. connected into one line.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 121: "Skipping updates.\n");
Same as above.
Done.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 125: #if 0 : if (ucode_len > F16H_MPB_MAX_SIZE || : ucode_len < F16H_MPB_DATA_OFFSET) { : printk(BIOS_DEBUG, "microcode file invalid. Skipping " : "updates.\n"); : return; : } : #endif
remove?
Done. Removed.
https://review.coreboot.org/c/coreboot/+/41719/2/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/2/src/soc/amd/picasso/microco... PS2, Line 2: /* This file is part of the coreboot project. */
This line should be removed.
Done. This line has been removed. the one in update_microcode.c has also be removed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 23: /* Array terminator */ : { 0xffffff, 0x0000 },
any reason to not use ARRAY_SIZE in the for loop?
No reason. Does it has to?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 27: u32 new_id;
initialize here?
Done. changed in patchset 8.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 43: : u32 equivalent_processor_rev_id = : get_equivalent_processor_rev_id(cpu_deviceid); : amd_update_microcode_from_cbfs(equivalent_processor_rev_id);
amd_update_microcode_from_cbfs takes a u32 as argument, but get_equivalent_processor_rev_id returns […]
Done. changed to u16. And a separated patch has been uploaded and merged.
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/update_... PS5, Line 27: int
unsigned
Change to u8 in Patchset 8.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#8).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com --- M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 7 files changed, 143 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/8
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 8:
(2 comments)
resolve comments.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 15: * Family 16h */
remove
Done. removed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 18: { 0x730f01, 0x7301 },
remove
Done. removed.
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 8:
(3 comments)
Three unresolved comments.
Please review to see if it is OK to click solved?
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG@11 PS5, Line 11:
Why is this Picasso specific? Can’t this be put into common code?
In theory, it can. But it needs too much work to extract generality and individuality. Can we do it later?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 106: uint32_t equivalent_processor_rev_id
It might be good to explain this equivalent_processor_rev_id in the commit message?
It is hard to explain. It is explained in Revision Guide.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 23: /* Array terminator */ : { 0xffffff, 0x0000 },
No reason. […]
Solved?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#9).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 7 files changed, 143 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/9
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG@11 PS5, Line 11:
In theory, it can. But it needs too much work to extract generality and individuality. […]
Yes, please add a note to the commit message though, what it’s too much work.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 23: /* Array terminator */ : { 0xffffff, 0x0000 },
Solved?
If the code is more readable that way, then yes. If you do not need a terminator, if all you need is the size, then please go with the size.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#10).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/cpu/amd/pi/00730F01/update_microcode.c M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 7 files changed, 139 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/10
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/5//COMMIT_MSG@11 PS5, Line 11:
Yes, please add a note to the commit message though, what it’s too much work.
Done. Added a note to the commit message.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 106: uint32_t equivalent_processor_rev_id
It is hard to explain. It is explained in Revision Guide.
Please review the revision guide.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 23: /* Array terminator */ : { 0xffffff, 0x0000 },
If the code is more readable that way, then yes. […]
Done. Changed to ARRAY_SIZE
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 10:
(7 comments)
finally got around to have a look; sorry that it took me so long
https://review.coreboot.org/c/coreboot/+/41719/10/src/cpu/amd/pi/00730F01/up... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/cpu/amd/pi/00730F01/up... PS10, Line 24: u8 i; unrelated? i'd also rather use size_t here. but see my comment on the equivalent function on the file for picasso
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/cpu.c File src/soc/amd/picasso/cpu.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/cpu.c@... PS10, Line 113: cpuid_eax(1) move this cpuid_eax(1) call into the update_microcode function and don't pass it as parameter to the function, since the function won't be called with different parameters.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 13: struct microcode { this struct might need to be marked as packed, because it is used to access to deserialize a data blob
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 39: /* apply patch */ move this comment right before the wrmsr(MSR_PATCH_LOADER, msr); call? at least that's where the ucode patch gets applied
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 48: /* patch authentication */ authentication? it's more a check if the new ucode was successfully applied. ok, which implies that it was also successfully authenticated, but that's likely something the processor does and not what this code does, right?
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 57: static void amd_update_microcode(const void *ucode, size_t ucode_len, this contains quite a bit of typecasts; i'd try to minimize the number of typecasts used. it is guaranteed that all microcode patches have the size F17H_MPB_MAX_SIZE, right? if so what about using const struct microcode[] and assigning *ucode to it after one type cast. ucode_len/F17H_MPB_MAX_SIZE is the number of array elements that can be iterated over then. this should avoid all other typecasts; haven't checked the details though. it would also be really helpful to print a warning or an error if no matching ucode was found
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/update... PS10, Line 12: static u16 get_equivalent_processor_rev_id(u32 orig_id) wouldn't use a lookup table, but extract and rearrange the bytes. also that removes the requirement to update this table from time to time
the whole function should be something like return (uint16_t)((orig_id & 0xff0000) >> 8 | orig_id & 0xff); with some comment explaining what ends up where in the returned value
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 10:
(8 comments)
https://review.coreboot.org/c/coreboot/+/41719/10/src/cpu/amd/pi/00730F01/up... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/cpu/amd/pi/00730F01/up... PS10, Line 24: u8 i;
unrelated? i'd also rather use size_t here. […]
I think this entire file should be unrelated. Hopefully it was included by accident.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 31: remove extra blank line
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 39: /* apply patch */
move this comment right before the wrmsr(MSR_PATCH_LOADER, msr); call? at least that's where the uco […]
I think the source is OK without the comment.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 41: 0xFFFFFFFF If you use lower case here, it'll match coreboot better than AMD's style.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 57: remove extra space
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 68: uint32_t uintptr_t
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 68: F17H_MPB_MAX_SIZE still need to remove the F17h stuff
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/update... PS10, Line 36: u16 It seems like most of the patch use uintnn_t instead of unn. I'd try to make then all similar, or at least match within the same file.
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 11:
Just push with a rebase and categorizing topic.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#12).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 6 files changed, 216 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41719/12/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/12/src/soc/amd/picasso/update... PS12, Line 99: 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/+/41719/12/src/soc/amd/picasso/update... PS12, Line 103: uint16_t equivalent_processor_rev_id = get_equivalent_processor_rev_id(cpuid_eax(1));; Statements terminations use 1 semicolon
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42094 A new patchset has been uploaded, which is the parent change of this one.
https://review.coreboot.org/c/coreboot/+/41719/10/src/cpu/amd/pi/00730F01/up... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/cpu/amd/pi/00730F01/up... PS10, Line 24: u8 i;
I think this entire file should be unrelated. Hopefully it was included by accident.
Done. Removed the file which was added by accident. A new patch set is uploaded. https://review.coreboot.org/c/coreboot/+/42094/1
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/cpu.c File src/soc/amd/picasso/cpu.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/cpu.c@... PS10, Line 113: cpuid_eax(1)
move this cpuid_eax(1) call into the update_microcode function and don't pass it as parameter to the […]
Done. call this amd_update_microcode_from_cbfs() instead.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#13).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/microcode_picasso.c A src/soc/amd/picasso/update_microcode.c 6 files changed, 216 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/13
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#14).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 125 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/14
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 14:
(7 comments)
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 13: struct microcode {
this struct might need to be marked as packed, because it is used to access to deserialize a data bl […]
Done. Add __packed.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 31:
remove extra blank line
Done. removed.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 39: /* apply patch */
I think the source is OK without the comment.
Done. Removed.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 48: /* patch authentication */
authentication? it's more a check if the new ucode was successfully applied. […]
// Do ucode patch Authentication These are the words from AGESA source.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 57:
remove extra space
Done
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 68: uint32_t
uintptr_t
Done. Changed.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 68: F17H_MPB_MAX_SIZE
still need to remove the F17h stuff
What should it be? PICASSO_MPB_MAX_SIZE?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#15).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 124 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/15
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#16).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 104 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/16
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/update... PS10, Line 12: static u16 get_equivalent_processor_rev_id(u32 orig_id)
wouldn't use a lookup table, but extract and rearrange the bytes. […]
Done. yes. That is the way AGESA code does.
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/update... PS10, Line 36: u16
It seems like most of the patch use uintnn_t instead of unn. […]
Done. This function is removed.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 48: /* patch authentication */
// Do ucode patch Authentication […]
had another look at the PPR and this is indeed a check if the new microcode update got applied, which implies that the microcode loading mechanism in the core was able to authenticate the update, and doesn't do the actual microcode update authentication. but since the code is rather self-explaining here, i'd suggest to just drop this comment that i consider to be a bit misleading
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41719/17/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/17/src/soc/amd/picasso/update... PS17, Line 84: if (equivalent_processor_rev_id == 0) { : printk(BIOS_DEBUG, "microcode: rev id not found. Skipping microcode patch!\n"); : return; : } unneeded check, since the lookup table is gone
https://review.coreboot.org/c/coreboot/+/41719/17/src/soc/amd/picasso/update... PS17, Line 92: BIOS_DEBUG I'd rather use BIOS_WARNING or BIOS_INFO here
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#18).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 99 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/18
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 18:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 41: 0xFFFFFFFF
If you use lower case here, it'll match coreboot better than AMD's style.
Done
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 48: /* patch authentication */
had another look at the PPR and this is indeed a check if the new microcode update got applied, whic […]
Done. Deleted.
https://review.coreboot.org/c/coreboot/+/41719/17/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/17/src/soc/amd/picasso/update... PS17, Line 84: if (equivalent_processor_rev_id == 0) { : printk(BIOS_DEBUG, "microcode: rev id not found. Skipping microcode patch!\n"); : return; : }
unneeded check, since the lookup table is gone
Done. Deleted.
https://review.coreboot.org/c/coreboot/+/41719/17/src/soc/amd/picasso/update... PS17, Line 92: BIOS_DEBUG
I'd rather use BIOS_WARNING or BIOS_INFO here
Done. Changed.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#19).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 96 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/19
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 57: static void amd_update_microcode(const void *ucode, size_t ucode_len,
this contains quite a bit of typecasts; i'd try to minimize the number of typecasts used. […]
Done. Changed. Compare the struct pointer. use for loop.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 19: Code-Review+2
looks good to me; depends on the added MSR define in 42094, so that one has to be submitted before this patch
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41719/20/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/20/src/soc/amd/picasso/update... PS20, Line 52: (new_patch_id == m->patch_id) ? "success" : "fail"); microcode: Updating to patch id = 0x%08x %s\n … succeeded/failed
The success message level should be `BIOS_INFO`, the failure message level should be `BIOS_ERR`.
https://review.coreboot.org/c/coreboot/+/41719/20/src/soc/amd/picasso/update... PS20, Line 69: m++) { This fits on the line above.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#21).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 96 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/21
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#22).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 98 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/22
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 22: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/41719/20/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/20/src/soc/amd/picasso/update... PS20, Line 52: (new_patch_id == m->patch_id) ? "success" : "fail");
microcode: Updating to patch id = 0x%08x %s\n … succeeded/failed […]
Done
https://review.coreboot.org/c/coreboot/+/41719/20/src/soc/amd/picasso/update... PS20, Line 69: m++) {
This fits on the line above.
Done
https://review.coreboot.org/c/coreboot/+/41719/22/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/22/src/soc/amd/picasso/update... PS22, Line 54: updated same as i commented on the patch before this one
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#23).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 98 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/23
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41719/22/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/22/src/soc/amd/picasso/update... PS22, Line 54: updated
same as i commented on the patch before this one
Changed to "being updated".
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 23: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/41719/22/src/soc/amd/picasso/update... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/22/src/soc/amd/picasso/update... PS22, Line 54: updated
Changed to "being updated".
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 68: F17H_MPB_MAX_SIZE
What should it be? PICASSO_MPB_MAX_SIZE?
the file is picasso-specific, so the prefix is not really needed, but i also don't see necessity to remove it. I'm fine with either
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#24).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 98 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/24
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 24:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 68: uint32_t
Done. Changed.
Done
https://review.coreboot.org/c/coreboot/+/41719/10/src/soc/amd/picasso/microc... PS10, Line 68: F17H_MPB_MAX_SIZE
the file is picasso-specific, so the prefix is not really needed, but i also don't see necessity to […]
Done. Removed prefix F17.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 24: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 24:
oh, after submitting the common FADT patch this needs a manual rebase due to a removed Kconfig option that was in the line right before the one added here
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#25).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
BUG=b:153580119 TEST=mandolin
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 98 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/25
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 25:
Patch Set 24:
oh, after submitting the common FADT patch this needs a manual rebase due to a removed Kconfig option that was in the line right before the one added here
patchset 24 has resolved the conflict in Kconfig.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Zheng Bao, Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41719
to look at the new patch set (#26).
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
BUG=b:153580119 TEST=mandolin
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 Signed-off-by: Zheng Bao zheng.bao@amd.com Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 98 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41719/26
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 26:
Patch Set 25:
Patch Set 24:
oh, after submitting the common FADT patch this needs a manual rebase due to a removed Kconfig option that was in the line right before the one added here
patchset 24 has resolved the conflict in Kconfig.
Sorry for not notice new conflict. Resolved in patchset 26.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 26: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
amd/picasso: Load x86 microcode from CBFS modules
Combine the Ucode binaries for 3 revisions of CPU into one CBFS module. This should be moved to the AMD common code later.
BUG=b:153580119 TEST=mandolin
Change-Id: Ib08a65b93c045afc97952a809670c85831c0faf7 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/+/41719 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/cpu.c A src/soc/amd/picasso/update_microcode.c 4 files changed, 98 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 5ba0c1b..2bd9c09 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -54,6 +54,7 @@ select FSP_USES_CB_STACK select UDK_2017_BINDING select HAVE_CF9_RESET + select SUPPORT_CPU_UCODE_IN_CBFS
config PRERAM_CBMEM_CONSOLE_SIZE hex diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 8012f97..7fce124 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -64,6 +64,7 @@ ramstage-y += psp.c ramstage-y += fsp_params.c ramstage-y += config.c +ramstage-y += update_microcode.c
all-y += reset.c
@@ -281,9 +282,6 @@ OPT_PSP_PMUD_FILE2=$(call add_opt_prefix, $(PSP_PMUD_FILE2), --subprogram 0 --instance 4 --pmu-data) OPT_PSP_PMUD_FILE3=$(call add_opt_prefix, $(PSP_PMUD_FILE3), --subprogram 1 --instance 1 --pmu-data) OPT_PSP_PMUD_FILE4=$(call add_opt_prefix, $(PSP_PMUD_FILE4), --subprogram 1 --instance 4 --pmu-data) -OPT_PSP_UCODE_FILE1=$(call add_opt_prefix, $(PSP_UCODE_FILE1), --instance 0 --ucode) -OPT_PSP_UCODE_FILE2=$(call add_opt_prefix, $(PSP_UCODE_FILE2), --instance 1 --ucode) -OPT_PSP_UCODE_FILE3=$(call add_opt_prefix, $(PSP_UCODE_FILE3), --instance 2 --ucode) OPT_MP2CFG_FILE=$(call add_opt_prefix, $(PSP_MP2CFG_FILE), --mp2-config)
# Copy prebuild APCBs if they exist @@ -342,9 +340,6 @@ $(call strip_quotes, $(PSP_PMUD_FILE2)) \ $(call strip_quotes, $(PSP_PMUD_FILE3)) \ $(call strip_quotes, $(PSP_PMUD_FILE4)) \ - $(call strip_quotes, $(PSP_UCODE_FILE1)) \ - $(call strip_quotes, $(PSP_UCODE_FILE2)) \ - $(call strip_quotes, $(PSP_UCODE_FILE3)) \ $(call strip_quotes, $(PSP_MP2CFG_FILE)) \ $(call strip_quotes, $(PSP_SMUFW1_SUB1_FILE)) \ $(call strip_quotes, $(PSP_SMUFW1_SUB2_FILE)) \ @@ -399,9 +394,6 @@ $(OPT_PSP_PMUD_FILE2) \ $(OPT_PSP_PMUD_FILE3) \ $(OPT_PSP_PMUD_FILE4) \ - $(OPT_PSP_UCODE_FILE1) \ - $(OPT_PSP_UCODE_FILE2) \ - $(OPT_PSP_UCODE_FILE3) \ $(OPT_MP2CFG_FILE) \ $(OPT_ABL0_FILE) \ $(OPT_ABL1_FILE) \ @@ -461,4 +453,6 @@
$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_M_ADDR)
+cpu_microcode_bins += $(wildcard 3rdparty/amd_blobs/picasso/PSP/UcodePatch_*.bin) + endif # ($(CONFIG_SOC_AMD_PICASSO),y) diff --git a/src/soc/amd/picasso/cpu.c b/src/soc/amd/picasso/cpu.c index 96edd70..97a1780 100644 --- a/src/soc/amd/picasso/cpu.c +++ b/src/soc/amd/picasso/cpu.c @@ -16,6 +16,7 @@ #include <soc/smi.h> #include <soc/iomap.h> #include <console/console.h> +#include <cpu/amd/microcode.h>
/* * MP and SMM loading initialization. @@ -108,6 +109,8 @@ { check_mca(); setup_lapic(); + + amd_update_microcode_from_cbfs(); }
static struct device_operations cpu_dev_ops = { diff --git a/src/soc/amd/picasso/update_microcode.c b/src/soc/amd/picasso/update_microcode.c new file mode 100644 index 0000000..8f3d3e2 --- /dev/null +++ b/src/soc/amd/picasso/update_microcode.c @@ -0,0 +1,91 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#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> + +#define MPB_MAX_SIZE 3200 +#define MPB_DATA_OFFSET 32 + +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[MPB_MAX_SIZE-MPB_DATA_OFFSET]; +} __packed; + +static void apply_microcode_patch(const struct microcode *m) +{ + uint32_t new_patch_id; + msr_t msr; + + 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); + + msr = rdmsr(MSR_PATCH_LEVEL); + new_patch_id = msr.lo; + + 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); +} + +static uint16_t get_equivalent_processor_rev_id(void) +{ + 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; + + for (m = (struct microcode *)ucode; + m < (struct microcode *)ucode + ucode_len/MPB_MAX_SIZE; m++) { + 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; + } + + amd_update_microcode(ucode, ucode_len, equivalent_processor_rev_id); +}
-
Arthur Heymans (Code Review) -
Bao Zheng (Code Review)
-
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Marshall Dawson (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)