[edk2-devel] [PATCH v4 2/6] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches
Dong, Eric
eric.dong at intel.com
Mon Dec 30 01:52:27 UTC 2019
Hao,
> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, December 27, 2019 3:32 PM
> To: devel at edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni,
> Ray <ray.ni at intel.com>; Laszlo Ersek <lersek at redhat.com>; Zeng, Star
> <star.zeng at intel.com>; Fu, Siyuan <siyuan.fu at intel.com>; Kinney, Michael
> D <michael.d.kinney at intel.com>
> Subject: [PATCH v4 2/6] UefiCpuPkg/MpInitLib: Reduce the size when
> loading microcode patches
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2429
>
> This commit will attempt to reduce the copy size when loading the
> microcode patches data from flash into memory.
>
> Such optimization is done by a pre-process of the microcode patch headers
> (on flash). A microcode patch will be loaded into memory only when the
> below 3 criteria are met:
>
> A. With a microcode patch header (which means the data is not padding data
> between microcode patches);
> B. The 'ProcessorSignature' & 'ProcessorFlags' fields in the header match
> at least one processor within system;
> C. If the Extended Signature Table exists in a microcode patch, the
> 'ProcessorSignature' & 'ProcessorFlag' fields in the table entries
> match at least one processor within system.
>
> Criterion B and C will require all the processors to be woken up once to
> collect their CPUID and Platform ID information. Hence, this commit will
> move the copy, detect and apply of microcode patch on BSP and APs after
> all the processors have been woken up.
>
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Star Zeng <star.zeng at intel.com>
> Cc: Siyuan Fu <siyuan.fu at intel.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu at intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 ++
> UefiCpuPkg/Library/MpInitLib/Microcode.c | 288 ++++++++++++++++++++
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 90 ++----
> 3 files changed, 340 insertions(+), 62 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 4440dc2701..56b0df664a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -44,6 +44,20 @@
> #define CPU_SWITCH_STATE_LOADED 2
>
> //
> +// Default maximum number of entries to store the microcode patches
> information
> +//
> +#define DEFAULT_MAX_MICROCODE_PATCH_NUM 8
> +
> +//
> +// Data structure for microcode patch information
> +//
> +typedef struct {
> + UINTN Address;
> + UINTN Size;
> + UINTN AlignedSize;
> +} MICROCODE_PATCH_INFO;
> +
> +//
> // CPU exchange information for switch BSP
> //
> typedef struct {
> @@ -576,6 +590,16 @@ MicrocodeDetect (
> );
>
> /**
> + Load the required microcode patches data into memory.
> +
> + @param[in, out] CpuMpData The pointer to CPU MP Data structure.
> +**/
> +VOID
> +LoadMicrocodePatch (
> + IN OUT CPU_MP_DATA *CpuMpData
> + );
> +
> +/**
> Detect whether Mwait-monitor feature is supported.
>
> @retval TRUE Mwait-monitor feature is supported.
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 199b1f23ce..62a104742a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -331,3 +331,291 @@ Done:
> MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, ProcessorFlags,
> (UINTN) MicrocodeData, LatestRevision));
> }
> }
> +
> +/**
> + Determine if a microcode patch will be loaded into memory.
> +
> + @param[in] CpuMpData The pointer to CPU MP Data structure.
> + @param[in] ProcessorSignature The processor signature field value
> + supported by a microcode patch.
> + @param[in] ProcessorFlags The prcessor flags field value supported by
> + a microcode patch.
> +
> + @retval TRUE The specified microcode patch will be loaded.
> + @retval FALSE The specified microcode patch will not be loaded.
> +**/
> +BOOLEAN
> +IsMicrocodePatchNeedLoad (
> + IN CPU_MP_DATA *CpuMpData,
> + IN UINT32 ProcessorSignature,
> + IN UINT32 ProcessorFlags
> + )
> +{
> + UINTN Index;
> + CPU_AP_DATA *CpuData;
> +
> + for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> + CpuData = &CpuMpData->CpuData[Index];
> + if ((ProcessorSignature == CpuData->ProcessorSignature) &&
> + (ProcessorFlags & (1 << CpuData->PlatformId)) != 0) {
> + return TRUE;
> + }
> + }
> +
> + return FALSE;
> +}
> +
> +/**
> + Actual worker function that loads 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
> + the microcode patches that will be loaded
> + into memory.
> + @param[in] PatchCount The number of microcode patches that will
> + be loaded into memory.
> + @param[in] TotalLoadSize The total size of all the microcode patches
> + to be loaded.
> +**/
> +VOID
> +LoadMicrocodePatchWorker (
> + IN OUT CPU_MP_DATA *CpuMpData,
> + IN MICROCODE_PATCH_INFO *Patches,
> + IN UINTN PatchCount,
> + IN UINTN TotalLoadSize
> + )
> +{
> + UINTN Index;
> + VOID *MicrocodePatchInRam;
> + UINT8 *Walker;
> +
> + ASSERT ((Patches != NULL) && (PatchCount != 0));
> +
> + MicrocodePatchInRam = AllocatePages (EFI_SIZE_TO_PAGES
> (TotalLoadSize));
> + if (MicrocodePatchInRam == NULL) {
> + return;
> + }
> +
> + //
> + // Load all the required microcode patches into memory
> + //
> + for (Walker = MicrocodePatchInRam, Index = 0; Index < PatchCount;
> Index++) {
> + CopyMem (
> + Walker,
> + (VOID *) Patches[Index].Address,
> + Patches[Index].Size
> + );
> +
> + //
> + // Zero-fill the padding area
> + // Please note that AlignedSize will be no less than Size
> + //
> + ZeroMem (
> + Walker + Patches[Index].Size,
> + Patches[Index].AlignedSize - Patches[Index].Size
> + );
> +
> + Walker += Patches[Index].AlignedSize;
> + }
> +
> + //
> + // Update the microcode patch related fields in CpuMpData
> + //
> + CpuMpData->MicrocodePatchAddress = (UINTN) MicrocodePatchInRam;
> + CpuMpData->MicrocodePatchRegionSize = TotalLoadSize;
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "%a: Required microcode patches have been loaded at 0x%lx, with size
> 0x%lx.\n",
> + __FUNCTION__, CpuMpData->MicrocodePatchAddress, CpuMpData-
> >MicrocodePatchRegionSize
> + ));
> +
> + return;
> +}
> +
> +/**
> + Load the required microcode patches data into memory.
> +
> + @param[in, out] CpuMpData The pointer to CPU MP Data structure.
> +**/
> +VOID
> +LoadMicrocodePatch (
> + IN OUT CPU_MP_DATA *CpuMpData
> + )
> +{
> + CPU_MICROCODE_HEADER *MicrocodeEntryPoint;
> + 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
> + // specified by the PCD pair. If the microcode patches are loaded into
> memory,
> + // these fields will be updated.
> + //
> + CpuMpData->MicrocodePatchAddress = PcdGet64
> (PcdCpuMicrocodePatchAddress);
> + CpuMpData->MicrocodePatchRegionSize = PcdGet64
> (PcdCpuMicrocodePatchRegionSize);
> +
> + MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN)
> CpuMpData->MicrocodePatchAddress;
> + MicrocodeEnd = (UINTN) MicrocodeEntryPoint +
> + (UINTN) CpuMpData->MicrocodePatchRegionSize;
> + if ((MicrocodeEntryPoint == NULL) || ((UINTN) MicrocodeEntryPoint ==
> MicrocodeEnd)) {
> + //
> + // There is no microcode patches
> + //
> + return;
> + }
> +
> + PatchCount = 0;
> + MaxPatchNumber = DEFAULT_MAX_MICROCODE_PATCH_NUM;
> + TotalLoadSize = 0;
> + PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof
> (MICROCODE_PATCH_INFO));
> + if (PatchInfoBuffer == NULL) {
> + return;
> + }
> +
> + //
> + // Process the header of each microcode patch within the region.
> + // The purpose is to decide which microcode patch(es) will be loaded into
> memory.
> + //
> + do {
> + if (MicrocodeEntryPoint->HeaderVersion != 0x1) {
> + //
> + // Padding data between the microcode patches, skip 1KB to check next
> entry.
> + //
> + MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + SIZE_1KB);
> + continue;
> + }
> +
> + DataSize = MicrocodeEntryPoint->DataSize;
> + TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
> + if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
> + ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
> + (DataSize & 0x3) != 0 ||
> + (TotalSize & (SIZE_1KB - 1)) != 0 ||
> + TotalSize < DataSize
> + ) {
> + //
> + // Not a valid microcode header, skip 1KB to check next entry.
> + //
> + MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + SIZE_1KB);
> + 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) {
> + PatchCount++;
[[Eric]] This logic seems not good. Image one case when PatchCount == 7 and MaxPatchNumber == 8. In this case, current logic needs to allocate new buffer for PatchInfoBuffer, but actually PatchInfoBuffer should be enough for this case. So I think "PatchCount++;" code should move after " TotalLoadSize += PatchInfoBuffer[PatchCount - 1].AlignedSize;" and later " PatchCount - 1" should be " PatchCount".
Thanks,
Eric
> + if (PatchCount >= MaxPatchNumber) {
> + //
> + // Current 'PatchInfoBuffer' cannot hold the information, double the
> size
> + // and allocate a new buffer.
> + //
> + if (MaxPatchNumber > MAX_UINTN / 2 / sizeof
> (MICROCODE_PATCH_INFO)) {
> + //
> + // Overflow check for MaxPatchNumber
> + //
> + goto OnExit;
> + }
> +
> + PatchInfoBuffer = ReallocatePool (
> + MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
> + 2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
> + PatchInfoBuffer
> + );
> + if (PatchInfoBuffer == NULL) {
> + goto OnExit;
> + }
> + MaxPatchNumber = MaxPatchNumber * 2;
> + }
> +
> + //
> + // Store the information of this microcode patch
> + //
> + if (TotalSize > ALIGN_VALUE (TotalSize, SIZE_1KB) ||
> + ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
> + goto OnExit;
> + }
> + PatchInfoBuffer[PatchCount - 1].Address = (UINTN)
> MicrocodeEntryPoint;
> + PatchInfoBuffer[PatchCount - 1].Size = TotalSize;
> + PatchInfoBuffer[PatchCount - 1].AlignedSize = ALIGN_VALUE (TotalSize,
> SIZE_1KB);
> + TotalLoadSize += PatchInfoBuffer[PatchCount - 1].AlignedSize;
> + }
> +
> + //
> + // Process the next microcode patch
> + //
> + MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + TotalSize);
> + } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> +
> + if (PatchCount != 0) {
> + DEBUG ((
> + DEBUG_INFO,
> + "%a: 0x%x microcode patches will be loaded into memory, with size
> 0x%x.\n",
> + __FUNCTION__, PatchCount, TotalLoadSize
> + ));
> +
> + LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount,
> TotalLoadSize);
> + }
> +
> +OnExit:
> + if (PatchInfoBuffer != NULL) {
> + FreePool (PatchInfoBuffer);
> + }
> + return;
> +}
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d5077e080e..c72bf3c9ee 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -628,10 +628,6 @@ ApWakeupFunction (
> ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) *
> CpuMpData->CpuApStackSize;
> BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
> //
> - // Do some AP initialize sync
> - //
> - ApInitializeSync (CpuMpData);
> - //
> // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP
> environment,
> // to initialize AP in InitConfig path.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters
> points to a different IDT shared by all APs.
> @@ -1615,7 +1611,6 @@ MpInitLibInitialize (
> UINTN ApResetVectorSize;
> UINTN BackupBufferAddr;
> UINTN ApIdtBase;
> - VOID *MicrocodePatchInRam;
>
> OldCpuMpData = GetCpuMpDataFromGuidedHob ();
> if (OldCpuMpData == NULL) {
> @@ -1683,39 +1678,7 @@ MpInitLibInitialize (
> CpuMpData->SwitchBspFlag = FALSE;
> CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
> CpuMpData->CpuInfoInHob = (UINT64) (UINTN) (CpuMpData->CpuData
> + MaxLogicalProcessorNumber);
> - if (OldCpuMpData == NULL) {
> - CpuMpData->MicrocodePatchRegionSize = PcdGet64
> (PcdCpuMicrocodePatchRegionSize);
> - //
> - // If platform has more than one CPU, relocate microcode to memory to
> reduce
> - // loading microcode time.
> - //
> - MicrocodePatchInRam = NULL;
> - if (MaxLogicalProcessorNumber > 1) {
> - MicrocodePatchInRam = AllocatePages (
> - EFI_SIZE_TO_PAGES (
> - (UINTN)CpuMpData->MicrocodePatchRegionSize
> - )
> - );
> - }
> - if (MicrocodePatchInRam == NULL) {
> - //
> - // there is only one processor, or no microcode patch is available, or
> - // memory allocation failed
> - //
> - CpuMpData->MicrocodePatchAddress = PcdGet64
> (PcdCpuMicrocodePatchAddress);
> - } else {
> - //
> - // there are multiple processors, and a microcode patch is available, and
> - // memory allocation succeeded
> - //
> - CopyMem (
> - MicrocodePatchInRam,
> - (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress),
> - (UINTN)CpuMpData->MicrocodePatchRegionSize
> - );
> - CpuMpData->MicrocodePatchAddress = (UINTN)MicrocodePatchInRam;
> - }
> - }else {
> + if (OldCpuMpData != NULL) {
> CpuMpData->MicrocodePatchRegionSize = OldCpuMpData-
> >MicrocodePatchRegionSize;
> CpuMpData->MicrocodePatchAddress = OldCpuMpData-
> >MicrocodePatchAddress;
> }
> @@ -1762,14 +1725,6 @@ MpInitLibInitialize (
> (UINT32 *)(MonitorBuffer + MonitorFilterSize * Index);
> }
> //
> - // Load Microcode on BSP
> - //
> - MicrocodeDetect (CpuMpData, TRUE);
> - //
> - // Store BSP's MTRR setting
> - //
> - MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
> - //
> // Enable the local APIC for Virtual Wire Mode.
> //
> ProgramVirtualWireMode ();
> @@ -1781,6 +1736,11 @@ MpInitLibInitialize (
> //
> CollectProcessorCount (CpuMpData);
> }
> +
> + //
> + // Load required microcode patches data into memory
> + //
> + LoadMicrocodePatch (CpuMpData);
> } else {
> //
> // APs have been wakeup before, just get the CPU Information
> @@ -1788,7 +1748,6 @@ MpInitLibInitialize (
> //
> CpuMpData->CpuCount = OldCpuMpData->CpuCount;
> CpuMpData->BspNumber = OldCpuMpData->BspNumber;
> - CpuMpData->InitFlag = ApInitReconfig;
> CpuMpData->CpuInfoInHob = OldCpuMpData->CpuInfoInHob;
> CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData-
> >CpuInfoInHob;
> for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> @@ -1797,21 +1756,28 @@ MpInitLibInitialize (
> CpuMpData->CpuData[Index].ApFunction = 0;
> CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters,
> &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
> }
> - if (MaxLogicalProcessorNumber > 1) {
> - //
> - // Wakeup APs to do some AP initialize sync
> - //
> - WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> - //
> - // Wait for all APs finished initialization
> - //
> - while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
> - CpuPause ();
> - }
> - CpuMpData->InitFlag = ApInitDone;
> - for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> - SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
> - }
> + }
> +
> + //
> + // Detect and apply Microcode on BSP
> + //
> + MicrocodeDetect (CpuMpData, TRUE);
> + //
> + // Store BSP's MTRR setting
> + //
> + MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
> +
> + //
> + // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
> + //
> + if (CpuMpData->CpuCount > 1) {
> + WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> + while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
> + CpuPause ();
> + }
> +
> + for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> + SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
> }
> }
>
> --
> 2.12.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52601): https://edk2.groups.io/g/devel/message/52601
Mute This Topic: https://groups.io/mt/69283205/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