(+Tom) On 01/08/20 05:25, Siyuan Fu wrote: > The existing MpInitLib will shadow the microcode update patches from > flash to memory and this is done by searching microcode region > specified by PCD PcdCpuMicrocodePatchAddress and > PcdCpuMicrocodePatchRegionSize. > This brings a limition to platform FW that all the microcode patches > must be placed in one continuous flash space. > > This patch set shadows microcode update according to FIT microcode > entries if it's present, otherwise it will fallback to original logic > (by PCD). > > Patch 1/2: Add FIT header file to MdePkg. > Patch 2/2: Update microcode loader to shadow microcode according to FIT. > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > > Siyuan Fu (2): > MdePkg: Add header file for Firmware Interface Table specification. > UefiCpuPkg: Shadow microcode patch according to FIT microcode entry. > > .../IndustryStandard/FirmwareInterfaceTable.h | 76 ++++++ > UefiCpuPkg/Library/MpInitLib/Microcode.c | 255 +++++++++++++----- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 4 +- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 7 +- > 4 files changed, 276 insertions(+), 66 deletions(-) > create mode 100644 MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h >
I have now checked https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios-specification.pdf that is referenced in https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449#c0 I think it is wrong for MpInitLib to deduce anything from the flash contents at fixed physical address 0xFFFFFFC0 *unless* the platform advertizes up-front adherence to the FIT specification. The proposed logic in patch#2 goes like this: (1) read the UINT64 at physical address 0xFFFFFFC0 (2) If the value read is zero, all-nibles-F, or all-nibbles-E, then assume "no FIT" (3) otherwise, dereference the value read, as a pointer-to-FIRMWARE_INTERFACE_TABLE_ENTRY (4) if the assumed FIRMWARE_INTERFACE_TABLE_ENTRY doesn't look right, assume "no FIT" (5) if "no FIT", then fall back to previous PCDs (which describe if there is a microcode update in the flash, and if so, where) This approach is wrong. If a platform has never heard of the FIT specification, it may have any value at all at address 0xFFFFFFC0. The value there may easily differ from the three values that step (2) equates to "no FIT". Therefore the whole procedure above needs to be gated with a new FeaturePCD (default value FALSE). This is not a theoretical risk. Please see the following patches: * [edk2-devel] [RFC PATCH v3 36/43] UefiCpuPkg: Allow AP booting under SEV-ES https://edk2.groups.io/g/devel/message/50976 ddc10e014822c9a87a7dc9a44ee854751ffcf442.1574280425.git.thomas.lendacky@amd.com">http://mid.mail-archive.com/ddc10e014822c9a87a7dc9a44ee854751ffcf442.1574280425.git.thomas.lendacky@amd.com * [edk2-devel] [RFC PATCH v3 37/43] OvmfPkg: Reserve a page in memory for the SEV-ES AP reset vector https://edk2.groups.io/g/devel/message/50977 706f17aa0ea3ed3f57c1e933e93164a94ba1a0cf.1574280425.git.thomas.lendacky@amd.com">http://mid.mail-archive.com/706f17aa0ea3ed3f57c1e933e93164a94ba1a0cf.1574280425.git.thomas.lendacky@amd.com Those patches describe a structure whose *last* field, and EFI_GUID, is placed ath 0xffffffd0. This structure is extensible, and new fields are supposed to be *prepended* to it. The currently defined first field starts at 0xffffffca. Whereas the (exclusive) end of the FIT pointer is at 0xffffffc8. This means there is a 2 (two) bytes gap between them. If more than two bytes are prepended to the extensible structure in the future, that may easily lead step (2) above to incorrectly determine "yes FIT". So please introduce a new FeaturePCD in MdePkg (or even a dynamic boolean PCD, if you think that's more flexible), and add logic similar to: > VOID > ShadowMicrocodeUpdatePatch ( > IN OUT CPU_MP_DATA *CpuMpData > ) > { > EFI_STATUS Status; > > if (FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) { > Status = ShadowMicrocodePatchByFit (CpuMpData); > } else { > Status = EFI_UNSUPPORTED; > } > > if (EFI_ERROR (Status)) { > ShadowMicrocodePatchByPcd (CpuMpData); > } > } Alternatively, leave ShadowMicrocodeUpdatePatch() as it is currently proposed, but start ShadowMicrocodePatchByFit() as follows: > if (!FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) { > return EFI_UNSUPPORTED; > } In closing: it seems short-sighted that the FIT specification placed a "naked" pointer at a fixed offset in flash, rather than a three-field structure consisting of: - a GUID, - preceded by a structure size, - preceded by the FIT pointer. Because, using a GUID-ed approach, the chance to *incorrectly* deduce "yes FIT" would be 1 in (2^128) -- all 128-bit values except one magic value would indicate "no FIT". That's good. Whereas, with the spec's current "naked pointer" approach, the chance to *correctly* deduce "yes FIT" is 3 in (2^64) -- all 64-bit values except three magic values indicate "yes FIT". Not good. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53007): https://edk2.groups.io/g/devel/message/53007 Mute This Topic: https://groups.io/mt/69521538/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
