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