[edk2-devel] [Patch 0/2] Shadow microcode patch according to FIT microcode table.
Siyuan, Fu
siyuan.fu at intel.com
Wed Jan 8 10:58:03 UTC 2020
Hi, Laszlo
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: 2020年1月8日 17:43
> To: Fu, Siyuan <siyuan.fu at intel.com>; devel at edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney at intel.com>; Gao, Liming
> <liming.gao at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni, Ray
> <ray.ni at intel.com>; Tom Lendacky <thomas.lendacky at 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.kinney at intel.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Cc: Eric Dong <eric.dong at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Laszlo Ersek <lersek at 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.lendacky at 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.lendacky at 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?
Best Regards,
Siyuan
>
> > 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 (#53011): https://edk2.groups.io/g/devel/message/53011
Mute This Topic: https://groups.io/mt/69521538/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list