Re: [Patch 3/3] UefiCpuPkg/MpInitLib: Load uCode once for one core.

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@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 <lersek@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
>