* [Patch 1/2] MdePkg: Add header file for Firmware Interface Table specification.
2020-01-08 4:25 [Patch 0/2] Shadow microcode patch according to FIT microcode table Siyuan, Fu
@ 2020-01-08 4:25 ` Siyuan, Fu
2020-01-08 4:25 ` [Patch 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry Siyuan, Fu
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Siyuan, Fu @ 2020-01-08 4:25 UTC (permalink / raw)
To: devel; +Cc: michael.d.kinney, liming.gao, eric.dong, ray.ni, lersek
This patch add FirmwareInterfaceTable.h for the Firmware Interface Table
BIOS specification.
This is to remove future edk2 dependency on edk2-platforms repo. The file
content comes from
edk2-platforms\Silicon\Intel\IntelSiliconPkg\Include\IndustryStandard
BZ link: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
---
.../IndustryStandard/FirmwareInterfaceTable.h | 76 +++++++++++++++++++
1 file changed, 76 insertions(+)
create mode 100644 MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
diff --git a/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
new file mode 100644
index 0000000000..be3e34ae1b
--- /dev/null
+++ b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
@@ -0,0 +1,76 @@
+/** @file
+ Industry Standard Definitions of Firmware Interface Table BIOS Specification 1.0.
+
+ Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __FIRMWARE_INTERFACE_TABLE_H__
+#define __FIRMWARE_INTERFACE_TABLE_H__
+
+//
+// FIT Entry type definitions
+//
+#define FIT_TYPE_00_HEADER 0x00
+#define FIT_TYPE_01_MICROCODE 0x01
+#define FIT_TYPE_02_STARTUP_ACM 0x02
+#define FIT_TYPE_07_BIOS_STARTUP_MODULE 0x07
+#define FIT_TYPE_08_TPM_POLICY 0x08
+#define FIT_TYPE_09_BIOS_POLICY 0x09
+#define FIT_TYPE_0A_TXT_POLICY 0x0A
+#define FIT_TYPE_0B_KEY_MANIFEST 0x0B
+#define FIT_TYPE_0C_BOOT_POLICY_MANIFEST 0x0C
+#define FIT_TYPE_10_CSE_SECURE_BOOT 0x10
+#define FIT_TYPE_2D_TXTSX_POLICY 0x2D
+#define FIT_TYPE_2F_JMP_DEBUG_POLICY 0x2F
+#define FIT_TYPE_7F_SKIP 0x7F
+
+#define FIT_POINTER_ADDRESS 0xFFFFFFC0 ///< Fixed address at 4G - 40h
+
+#define FIT_TYPE_VERSION 0x0100
+
+#define FIT_TYPE_00_SIGNATURE SIGNATURE_64 ('_', 'F', 'I', 'T', '_', ' ', ' ', ' ')
+
+#pragma pack(1)
+
+typedef struct {
+ //
+ // Address is the base address of the firmware component
+ // must be aligned on 16 byte boundary
+ //
+ UINT64 Address;
+ //
+ // Size is the span of the component in multiple of 16 bytes
+ //
+ UINT8 Size[3];
+ //
+ // Reserved must be set to 0
+ //
+ UINT8 Reserved;
+ //
+ // Component's version number in binary coded decimal (BCD) format.
+ // For the FIT header entry, the value in this field will indicate the revision
+ // number of the FIT data structure. The upper byte of the revision field
+ // indicates the major revision and the lower byte indicates the minor revision.
+ //
+ UINT16 Version;
+ //
+ // FIT types 0x00 to 0x7F
+ //
+ UINT8 Type : 7;
+ //
+ // Checksum Valid indicates whether component has valid checksum.
+ //
+ UINT8 C_V : 1;
+ //
+ // Component's checksum. The modulo sum of all the bytes in the component and
+ // the value in this field (Chksum) must add up to zero. This field is only
+ // valid if the C_V flag is non-zero.
+ //
+ UINT8 Chksum;
+} FIRMWARE_INTERFACE_TABLE_ENTRY;
+
+#pragma pack()
+
+#endif
--
2.19.1.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Patch 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry.
2020-01-08 4:25 [Patch 0/2] Shadow microcode patch according to FIT microcode table Siyuan, Fu
2020-01-08 4:25 ` [Patch 1/2] MdePkg: Add header file for Firmware Interface Table specification Siyuan, Fu
@ 2020-01-08 4:25 ` Siyuan, Fu
2020-01-08 9:42 ` [Patch 0/2] Shadow microcode patch according to FIT microcode table Laszlo Ersek
2020-01-09 2:04 ` Liming Gao
3 siblings, 0 replies; 9+ messages in thread
From: Siyuan, Fu @ 2020-01-08 4:25 UTC (permalink / raw)
To: devel; +Cc: michael.d.kinney, liming.gao, eric.dong, ray.ni, lersek
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 shadows microcode update according to FIT microcode entries if
it's present, otherwise it will fallback to original logic (by PCD).
TEST: Tested on FIT enabled platform.
BZ: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
---
UefiCpuPkg/Library/MpInitLib/Microcode.c | 255 +++++++++++++++++------
UefiCpuPkg/Library/MpInitLib/MpLib.c | 4 +-
UefiCpuPkg/Library/MpInitLib/MpLib.h | 7 +-
3 files changed, 200 insertions(+), 66 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 8f4d4da40c..6da6ebbfd3 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -318,7 +318,7 @@ Done:
}
/**
- Determine if a microcode patch will be loaded into memory.
+ Determine if a microcode patch matchs the specific processor signature and flag.
@param[in] CpuMpData The pointer to CPU MP Data structure.
@param[in] ProcessorSignature The processor signature field value
@@ -330,7 +330,7 @@ Done:
@retval FALSE The specified microcode patch will not be loaded.
**/
BOOLEAN
-IsMicrocodePatchNeedLoad (
+IsProcessorMatchedMicrocodePatch (
IN CPU_MP_DATA *CpuMpData,
IN UINT32 ProcessorSignature,
IN UINT32 ProcessorFlags
@@ -351,7 +351,77 @@ IsMicrocodePatchNeedLoad (
}
/**
- Actual worker function that loads the required microcode patches into memory.
+ Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode
+ patch header with the CPUID and PlatformID of the processors within
+ system to decide if it will be copied into memory.
+
+ @param[in] CpuMpData The pointer to CPU MP Data structure.
+ @param[in] MicrocodeEntryPoint The pointer to the microcode patch header.
+
+ @retval TRUE The specified microcode patch need to be loaded.
+ @retval FALSE The specified microcode patch dosen't need to be loaded.
+**/
+BOOLEAN
+IsMicrocodePatchNeedLoad (
+ IN CPU_MP_DATA *CpuMpData,
+ CPU_MICROCODE_HEADER *MicrocodeEntryPoint
+ )
+{
+ BOOLEAN NeedLoad;
+ UINTN DataSize;
+ UINTN TotalSize;
+ CPU_MICROCODE_EXTENDED_TABLE_HEADER *ExtendedTableHeader;
+ UINT32 ExtendedTableCount;
+ CPU_MICROCODE_EXTENDED_TABLE *ExtendedTable;
+ UINTN Index;
+
+ //
+ // Check the 'ProcessorSignature' and 'ProcessorFlags' in microcode patch header.
+ //
+ NeedLoad = IsProcessorMatchedMicrocodePatch (
+ CpuMpData,
+ MicrocodeEntryPoint->ProcessorSignature.Uint32,
+ MicrocodeEntryPoint->ProcessorFlags
+ );
+
+ //
+ // If the Extended Signature Table exists, check if the processor is in the
+ // support list
+ //
+ DataSize = MicrocodeEntryPoint->DataSize;
+ TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
+ if ((!NeedLoad) && (DataSize != 0) &&
+ (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +
+ sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {
+ ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)
+ + DataSize + sizeof (CPU_MICROCODE_HEADER));
+ ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;
+ ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
+
+ for (Index = 0; Index < ExtendedTableCount; Index ++) {
+ //
+ // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended
+ // Signature Table entry with the CPUID and PlatformID of the processors
+ // within system to decide if it will be copied into memory
+ //
+ NeedLoad = IsProcessorMatchedMicrocodePatch (
+ CpuMpData,
+ ExtendedTable->ProcessorSignature.Uint32,
+ ExtendedTable->ProcessorFlag
+ );
+ if (NeedLoad) {
+ break;
+ }
+ ExtendedTable ++;
+ }
+ }
+
+ return NeedLoad;
+}
+
+
+/**
+ Actual worker function that shadows the required microcode patches into memory.
@param[in, out] CpuMpData The pointer to CPU MP Data structure.
@param[in] Patches The pointer to an array of information on
@@ -363,7 +433,7 @@ IsMicrocodePatchNeedLoad (
to be loaded.
**/
VOID
-LoadMicrocodePatchWorker (
+ShadowMicrocodePatchWorker (
IN OUT CPU_MP_DATA *CpuMpData,
IN MICROCODE_PATCH_INFO *Patches,
IN UINTN PatchCount,
@@ -390,7 +460,6 @@ LoadMicrocodePatchWorker (
(VOID *) Patches[Index].Address,
Patches[Index].Size
);
-
Walker += Patches[Index].Size;
}
@@ -410,12 +479,13 @@ LoadMicrocodePatchWorker (
}
/**
- Load the required microcode patches data into memory.
+ Shadow the required microcode patches data into memory according to PCD
+ PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize.
@param[in, out] CpuMpData The pointer to CPU MP Data structure.
**/
VOID
-LoadMicrocodePatch (
+ShadowMicrocodePatchByPcd (
IN OUT CPU_MP_DATA *CpuMpData
)
{
@@ -423,15 +493,10 @@ LoadMicrocodePatch (
UINTN MicrocodeEnd;
UINTN DataSize;
UINTN TotalSize;
- CPU_MICROCODE_EXTENDED_TABLE_HEADER *ExtendedTableHeader;
- UINT32 ExtendedTableCount;
- CPU_MICROCODE_EXTENDED_TABLE *ExtendedTable;
MICROCODE_PATCH_INFO *PatchInfoBuffer;
UINTN MaxPatchNumber;
UINTN PatchCount;
UINTN TotalLoadSize;
- UINTN Index;
- BOOLEAN NeedLoad;
//
// Initialize the microcode patch related fields in CpuMpData as the values
@@ -487,55 +552,7 @@ LoadMicrocodePatch (
continue;
}
- //
- // Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode
- // patch header with the CPUID and PlatformID of the processors within
- // system to decide if it will be copied into memory
- //
- NeedLoad = IsMicrocodePatchNeedLoad (
- CpuMpData,
- MicrocodeEntryPoint->ProcessorSignature.Uint32,
- MicrocodeEntryPoint->ProcessorFlags
- );
-
- //
- // If the Extended Signature Table exists, check if the processor is in the
- // support list
- //
- if ((!NeedLoad) && (DataSize != 0) &&
- (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +
- sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {
- ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)
- + DataSize + sizeof (CPU_MICROCODE_HEADER));
- ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;
- ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
-
- for (Index = 0; Index < ExtendedTableCount; Index ++) {
- //
- // Avoid access content beyond MicrocodeEnd
- //
- if ((UINTN) ExtendedTable > MicrocodeEnd - sizeof (CPU_MICROCODE_EXTENDED_TABLE)) {
- break;
- }
-
- //
- // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended
- // Signature Table entry with the CPUID and PlatformID of the processors
- // within system to decide if it will be copied into memory
- //
- NeedLoad = IsMicrocodePatchNeedLoad (
- CpuMpData,
- ExtendedTable->ProcessorSignature.Uint32,
- ExtendedTable->ProcessorFlag
- );
- if (NeedLoad) {
- break;
- }
- ExtendedTable ++;
- }
- }
-
- if (NeedLoad) {
+ if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
PatchCount++;
if (PatchCount > MaxPatchNumber) {
//
@@ -581,7 +598,7 @@ LoadMicrocodePatch (
__FUNCTION__, PatchCount, TotalLoadSize
));
- LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
+ ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
}
OnExit:
@@ -590,3 +607,117 @@ OnExit:
}
return;
}
+
+/**
+ Shadow the required microcode patches data into memory according to FIT microcode entry.
+
+ @param[in, out] CpuMpData The pointer to CPU MP Data structure.
+
+ @return EFI_SUCCESS Microcode patch is shadowed into memory.
+ @return EFI_OUT_OF_RESOURCES No enough memory resource.
+ @return EFI_NOT_FOUND There is something wrong in FIT microcode entry.
+
+**/
+EFI_STATUS
+ShadowMicrocodePatchByFit (
+ IN OUT CPU_MP_DATA *CpuMpData
+ )
+{
+ UINT64 FitPointer;
+ FIRMWARE_INTERFACE_TABLE_ENTRY *FitEntry;
+ UINT32 EntryNum;
+ UINT32 Index;
+ MICROCODE_PATCH_INFO *PatchInfoBuffer;
+ UINTN MaxPatchNumber;
+ CPU_MICROCODE_HEADER *MicrocodeEntryPoint;
+ UINTN PatchCount;
+ UINTN TotalSize;
+ UINTN TotalLoadSize;
+
+ FitPointer = *(UINT64 *) (UINTN) FIT_POINTER_ADDRESS;
+ if ((FitPointer == 0) ||
+ (FitPointer == 0xFFFFFFFFFFFFFFFF) ||
+ (FitPointer == 0xEEEEEEEEEEEEEEEE)) {
+ //
+ // No FIT table.
+ //
+ return EFI_NOT_FOUND;
+ }
+ FitEntry = (FIRMWARE_INTERFACE_TABLE_ENTRY *) (UINTN) FitPointer;
+ if ((FitEntry[0].Type != FIT_TYPE_00_HEADER) ||
+ (FitEntry[0].Address != FIT_TYPE_00_SIGNATURE)) {
+ //
+ // Invalid FIT table, treat it as no FIT table.
+ //
+ return EFI_NOT_FOUND;
+ }
+
+ EntryNum = *(UINT32 *)(&FitEntry[0].Size[0]) & 0xFFFFFF;
+
+ //
+ // Calculate microcode entry number
+ //
+ MaxPatchNumber = 0;
+ for (Index = 0; Index < EntryNum; Index++) {
+ if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {
+ MaxPatchNumber++;
+ }
+ }
+ if (MaxPatchNumber == 0) {
+ return EFI_NOT_FOUND;
+ }
+
+ PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
+ if (PatchInfoBuffer == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ //
+ // Fill up microcode patch info buffer according to FIT table.
+ //
+ PatchCount = 0;
+ TotalLoadSize = 0;
+ for (Index = 0; Index < EntryNum; Index++) {
+ if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {
+ MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) FitEntry[Index].Address;
+ TotalSize = (MicrocodeEntryPoint->DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
+ if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
+ PatchInfoBuffer[PatchCount].Address = (UINTN) MicrocodeEntryPoint;
+ PatchInfoBuffer[PatchCount].Size = TotalSize;
+ TotalLoadSize += TotalSize;
+ PatchCount++;
+ }
+ }
+ }
+
+ if (PatchCount != 0) {
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: 0x%x microcode patches will be loaded into memory, with size 0x%x.\n",
+ __FUNCTION__, PatchCount, TotalLoadSize
+ ));
+
+ ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
+ }
+
+ FreePool (PatchInfoBuffer);
+ return EFI_SUCCESS;
+}
+
+/**
+ Shadow the required microcode patches data into memory.
+
+ @param[in, out] CpuMpData The pointer to CPU MP Data structure.
+**/
+VOID
+ShadowMicrocodeUpdatePatch (
+ IN OUT CPU_MP_DATA *CpuMpData
+ )
+{
+ EFI_STATUS Status;
+
+ Status = ShadowMicrocodePatchByFit (CpuMpData);
+ if (EFI_ERROR (Status)) {
+ ShadowMicrocodePatchByPcd (CpuMpData);
+ }
+}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index e611a8ca40..6ec9b172b8 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1,7 +1,7 @@
/** @file
CPU MP Initialize Library common functions.
- Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -1744,7 +1744,7 @@ MpInitLibInitialize (
//
// Load required microcode patches data into memory
//
- LoadMicrocodePatch (CpuMpData);
+ ShadowMicrocodeUpdatePatch (CpuMpData);
} else {
//
// APs have been wakeup before, just get the CPU Information
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b6e5a1afab..7c62d75acc 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -29,6 +29,9 @@
#include <Library/MtrrLib.h>
#include <Library/HobLib.h>
+#include <IndustryStandard/FirmwareInterfaceTable.h>
+
+
#define WAKEUP_AP_SIGNAL SIGNATURE_32 ('S', 'T', 'A', 'P')
#define CPU_INIT_MP_LIB_HOB_GUID \
@@ -587,12 +590,12 @@ MicrocodeDetect (
);
/**
- Load the required microcode patches data into memory.
+ Shadow the required microcode patches data into memory.
@param[in, out] CpuMpData The pointer to CPU MP Data structure.
**/
VOID
-LoadMicrocodePatch (
+ShadowMicrocodeUpdatePatch (
IN OUT CPU_MP_DATA *CpuMpData
);
--
2.19.1.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Patch 0/2] Shadow microcode patch according to FIT microcode table.
2020-01-08 4:25 [Patch 0/2] Shadow microcode patch according to FIT microcode table Siyuan, Fu
2020-01-08 4:25 ` [Patch 1/2] MdePkg: Add header file for Firmware Interface Table specification Siyuan, Fu
2020-01-08 4:25 ` [Patch 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry Siyuan, Fu
@ 2020-01-08 9:42 ` Laszlo Ersek
2020-01-08 10:36 ` [edk2-devel] " Laszlo Ersek
2020-01-08 10:58 ` Siyuan, Fu
2020-01-09 2:04 ` Liming Gao
3 siblings, 2 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-01-08 9:42 UTC (permalink / raw)
To: Siyuan Fu, devel
Cc: michael.d.kinney, liming.gao, eric.dong, ray.ni, Tom Lendacky
(+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@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@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@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@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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Patch 0/2] Shadow microcode patch according to FIT microcode table.
2020-01-08 4:25 [Patch 0/2] Shadow microcode patch according to FIT microcode table Siyuan, Fu
` (2 preceding siblings ...)
2020-01-08 9:42 ` [Patch 0/2] Shadow microcode patch according to FIT microcode table Laszlo Ersek
@ 2020-01-09 2:04 ` Liming Gao
2020-01-09 2:12 ` Siyuan, Fu
3 siblings, 1 reply; 9+ messages in thread
From: Liming Gao @ 2020-01-09 2:04 UTC (permalink / raw)
To: Fu, Siyuan, devel@edk2.groups.io
Cc: Kinney, Michael D, Dong, Eric, Ni, Ray, lersek@redhat.com
Siyuan:
This is new feature, please add it into https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
And, FIT header file is moved from edk2-platform to edk2. After this change, you also need to remove the one in edk2-platform. Right?
Thanks
Liming
-----Original Message-----
From: Fu, Siyuan <siyuan.fu@intel.com>
Sent: 2020年1月8日 12:25
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
Subject: [Patch 0/2] Shadow microcode patch according to FIT microcode table.
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@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@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
--
2.19.1.windows.1
^ permalink raw reply [flat|nested] 9+ messages in thread