On 01/08/20 11:58, Fu, Siyuan wrote: > Hi, Laszlo > >> -----Original Message----- >> From: Laszlo Ersek <ler...@redhat.com> >> Sent: 2020年1月8日 17:43 >> To: Fu, Siyuan <siyuan...@intel.com>; devel@edk2.groups.io >> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming >> <liming....@intel.com>; Dong, Eric <eric.d...@intel.com>; Ni, Ray >> <ray...@intel.com>; Tom Lendacky <thomas.lenda...@amd.com> >> Subject: Re: [Patch 0/2] Shadow microcode patch according to FIT microcode >> table. >> >> (+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 >> http://mid.mail- >> archive.com/ddc10e014822c9a87a7dc9a44ee854751ffcf442.1574280425.git. >> thomas.lenda...@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 >> http://mid.mail- >> archive.com/706f17aa0ea3ed3f57c1e933e93164a94ba1a0cf.1574280425.git. >> thomas.lenda...@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: > > I think a Feature PCD should be enough for this since whether using > the FIT based booting flow is decided by platform owner and not expected > to be changed once decided. > > And considering that the FIT table is very critical to processor init flow, > I propose to add a DEBUG ASSERT if the Feature PCD is true, but couldn't > find a valid FIT table structure. This could reminder platform developer > that the FIT address may have been incorrectly override by someone else, > like the extensible structure as you mentioned above. > > So the checking in ShadowMicrocodePatchByFit() should be as follows: > > if (!FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) { > return EFI_UNSUPPORTED; > } > > if (FIRMWARE_INTERFACE_TABLE_ENTRY doesn't look right) { > ASSERT(FALSE); > } > > What's your opinion on this?
Works fine for me. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53013): https://edk2.groups.io/g/devel/message/53013 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] -=-=-=-=-=-=-=-=-=-=-=-
