I've got two comments:
On 07/11/18 13:07, Eric Dong wrote:
> SDM requires one core only needs to load uCode once.
(1) This is a very confusing typo ("one core only").
I totally missed the point until I re-read the cover letter of the
patch. In the cover letter, you say:
> 3. Only apply uCode in one thread of a core when hyper threading is
> enabled.
So please fix the commit message to say, "The SDM requires only one
*thread per core* to load the microcode", or something similar.
(You don't need to add the emphasis, I'm only adding it here to make
myself clear.)
> Also load uCode once can save some time.
>
> This change enables this solution.
>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Ruiyu Ni <ruiyu...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.d...@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/Microcode.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 351975e2a2..4586e037d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -61,6 +61,7 @@ MicrocodeDetect (
> VOID *MicrocodeData;
> MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
> UINT32 ProcessorFlags;
> + UINT32 ThreadId;
>
> if (CpuMpData->MicrocodePatchRegionSize == 0) {
> //
> @@ -77,6 +78,14 @@ MicrocodeDetect (
> return;
> }
>
> + GetProcessorLocationByApicId (GetInitialApicId(), NULL, NULL, &ThreadId);
(2) A space character should be inserted after "GetInitialApicId".
With those fixed:
Acked-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
> + if (ThreadId != 0) {
> + //
> + // Skip loading microcode if it is not the first thread in one core.
> + //
> + return;
> + }
> +
> ExtendedTableLength = 0;
> //
> // Here data of CPUID leafs have not been collected into context buffer, so
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
