6 minor comments.
> -----Original Message-----
> From: Wu, Hao A <hao.a...@intel.com>
> Sent: Tuesday, December 24, 2019 9:37 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; Ni,
> Ray <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Zeng, Star
> <star.z...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Kinney, Michael
> D <michael.d.kin...@intel.com>
> Subject: [PATCH v1 2/4] 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 2 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.
>
> Criterion B 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.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Star Zeng <star.z...@intel.com>
> Cc: Siyuan Fu <siyuan...@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Signed-off-by: Hao A Wu <hao.a...@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 ++
> UefiCpuPkg/Library/MpInitLib/Microcode.c | 235 ++++++++++++++++++++
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 82 ++-----
> 3 files changed, 283 insertions(+), 58 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..68088b26a5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -331,3 +331,238 @@ Done:
> MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, ProcessorFlags,
> (UINTN) MicrocodeData, LatestRevision));
> }
> }
> +
> +/**
> + Actual worker function that loads the required microcode patches into
> memory.
> +
> + @param[in, out] CpuMpData The pointer to CPU MP Data structure.
> + @param[in] PatchInfoBuffer The pointer to an array of information
> on
> + the microcode patches that will be
> loaded
> + into memory.
> + @param[in] PatchNumber 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 *PatchInfoBuffer,
> + IN UINTN PatchNumber,
1. "PatchInfoBuffer" -> "Patches"?
"PatchNumber" -> "PatchCount"?
> + //
> + // Load all the required microcode patches into memory
> + //
> + for (Walker = MicrocodePatchInRam, Index = 0; Index < PatchNumber;
> Index++) {
> + CopyMem (
> + Walker,
> + (VOID *) PatchInfoBuffer[Index].Address,
> + PatchInfoBuffer[Index].Size
> + );
> +
> + if (PatchInfoBuffer[Index].AlignedSize > PatchInfoBuffer[Index].Size) {
2. This if-check is not needed because AlignedSize always >= Size.
Below ZeroMem() will directly return due to the 2nd parameter is 0.
> + //
> + // Zero-fill the padding area
> + //
> + ZeroMem (
> + Walker + PatchInfoBuffer[Index].Size,
> + PatchInfoBuffer[Index].AlignedSize - PatchInfoBuffer[Index].Size
> + );
> + }
> +
> + if (TotalSize > MAX_UINTN - TotalLoadSize ||
> + ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
> + goto OnExit;
> + }
3. ALIGN_VALUE (TotalSize , SIZE_1KB) always >= TotalSize.
So can we only check
ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize
?
> + PatchInfoBuffer[PatchNumber - 1].Address = (UINTN)
> MicrocodeEntryPoint;
> + PatchInfoBuffer[PatchNumber - 1].Size = TotalSize;
> + PatchInfoBuffer[PatchNumber - 1].AlignedSize = ALIGN_VALUE
> (TotalSize, SIZE_1KB);
4. "PatchNumber" -> "PatchCount"?
> + TotalLoadSize += PatchInfoBuffer[PatchNumber - 1].AlignedSize;
> + }
> +
> + //
> + // Process the next microcode patch
> + //
> + MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + TotalSize);
> + } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> +
> + if (PatchNumber == 0) {
> + //
> + // No patch needs to be loaded
> + //
> + goto OnExit;
5. This "goto" is not necessary. You can call below two lines when PatchCount
!= 0.
Less "goto" is better.
> + }
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "%a: 0x%x microcode patches will be loaded into memory, with size
> 0x%x.\n",
> + __FUNCTION__, PatchNumber, TotalLoadSize
> + ));
> +
> + LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchNumber,
> TotalLoadSize);
> +
> +OnExit:
> + if (PatchInfoBuffer != NULL) {
> + FreePool (PatchInfoBuffer);
> + }
> + return;
> +}
> @@ -1788,7 +1752,6 @@ MpInitLibInitialize (
> //
> CpuMpData->CpuCount = OldCpuMpData->CpuCount;
> CpuMpData->BspNumber = OldCpuMpData->BspNumber;
> - CpuMpData->InitFlag = ApInitReconfig;
6. Do you think that ApInitReconfig definition can be removed?
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52526): https://edk2.groups.io/g/devel/message/52526
Mute This Topic: https://groups.io/mt/69242654/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
