> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, December 24, 2019 11:32 AM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when
> loading microcode patches
>
> 6 minor comments.
Hello Ray,
Thanks for the feedbacks.
For 1,2,4,5, I will update the patch to address your comments.
For 3, my concern is that:
ALIGN_VALUE (TotalSize , SIZE_1KB)
might have a chance to overflow, how about changing the logic to:
if (TotalSize > ALIGN_VALUE (TotalSize, SIZE_1KB) ||
ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
goto OnExit;
}
For 6, the 'ApInitReconfig' flag value is still being used in function
ResetProcessorToIdleState().
Also, I found that there are several places in MpInitLib that use:
CpuMpData->InitFlag != ApInitDone
CpuMpData->InitFlag != ApInitConfig
In some if statements.
So I prefer the evaluation of the removal of the 'ApInitReconfig' flag value to
be handled in another separate patch series. Do you agree?
Best Regards,
Hao Wu
>
> > -----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 (#52537): https://edk2.groups.io/g/devel/message/52537
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]
-=-=-=-=-=-=-=-=-=-=-=-
