Re: [PATCH v4] IntelFsp2Pkg: LoadMicrocodeDefault() causing unnecessary delay.

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Chiu, Chasel <chasel.chiu@intel.com>
> Sent: Tuesday, April 4, 2023 2:34 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Kuo, Ted <ted.kuo@intel.com>
> Subject: [PATCH v4] IntelFsp2Pkg: LoadMicrocodeDefault() causing
> unnecessary delay.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4391
> 
> FSP should support the scenario that CPU microcode already loaded
> before calling LoadMicrocodeDefault(), in this case it should return
> directly without spending more time.
> Also the LoadMicrocodeDefault() should only attempt to load one version
> of the microcode for current CPU and return directly without parsing
> rest of the microcode in FV.
> 
> This patch also removed unnecessary LoadCheck code after supporting
> CPU microcode already loaded scenario.
> 
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> Reviewed-by: Ted Kuo <ted.kuo@intel.com>
> ---
>  IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 46
> ++++++++++++++++++++++++----------------------
>  IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm  | 45
> ++++++++++++++++++++++++---------------------
>  2 files changed, 48 insertions(+), 43 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> index 2cff8b3643..900126b93b 100644
> --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> @@ -245,6 +245,22 @@ ASM_PFX(LoadMicrocodeDefault):
>     cmp    esp, 0
> 
>     jz     ParamError
> 
> 
> 
> +   ;
> 
> +   ; If microcode already loaded before this function, exit this function with
> SUCCESS.
> 
> +   ;
> 
> +   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> +   xor   eax, eax               ; Clear EAX
> 
> +   xor   edx, edx               ; Clear EDX
> 
> +   wrmsr                        ; Load 0 to MSR at 8Bh
> 
> +
> 
> +   mov   eax, 1
> 
> +   cpuid
> 
> +   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> +   rdmsr                         ; Get current microcode signature
> 
> +   xor   eax, eax
> 
> +   test  edx, edx
> 
> +   jnz   Exit2
> 
> +
> 
>     ; skip loading Microcode if the MicrocodeCodeSize is zero
> 
>     ; and report error if size is less than 2k
> 
>     ; first check UPD header revision
> 
> @@ -330,7 +346,7 @@ CheckMainHeader:
>     cmp   ebx, dword [esi + MicrocodeHdr.MicrocodeHdrProcessor]
> 
>     jne   LoadMicrocodeDefault1
> 
>     test  edx, dword [esi + MicrocodeHdr.MicrocodeHdrFlags ]
> 
> -   jnz   LoadCheck  ; Jif signature and platform ID match
> 
> +   jnz   LoadMicrocode  ; Jif signature and platform ID match
> 
> 
> 
>  LoadMicrocodeDefault1:
> 
>     ; Check if extended header exists
> 
> @@ -363,7 +379,7 @@ CheckExtSig:
>     cmp   dword [edi + ExtSig.ExtSigProcessor], ebx
> 
>     jne   LoadMicrocodeDefault2
> 
>     test  dword [edi + ExtSig.ExtSigFlags], edx
> 
> -   jnz   LoadCheck      ; Jif signature and platform ID match
> 
> +   jnz   LoadMicrocode      ; Jif signature and platform ID match
> 
>  LoadMicrocodeDefault2:
> 
>     ; Check if any more extended signatures exist
> 
>     add   edi, ExtSig.size
> 
> @@ -435,23 +451,7 @@ LoadMicrocodeDefault4:
>     ; Is valid Microcode start point ?
> 
>     cmp   dword [esi + MicrocodeHdr.MicrocodeHdrVersion], 0ffffffffh
> 
>     jz    Done
> 
> -
> 
> -LoadCheck:
> 
> -   ; Get the revision of the current microcode update loaded
> 
> -   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> -   xor   eax, eax               ; Clear EAX
> 
> -   xor   edx, edx               ; Clear EDX
> 
> -   wrmsr                        ; Load 0 to MSR at 8Bh
> 
> -
> 
> -   mov   eax, 1
> 
> -   cpuid
> 
> -   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> -   rdmsr                        ; Get current microcode signature
> 
> -
> 
> -   ; Verify this microcode update is not already loaded
> 
> -   cmp   dword [esi + MicrocodeHdr.MicrocodeHdrRevision], edx
> 
> -   je    Continue
> 
> -
> 
> +   jmp   CheckMainHeader
> 
>  LoadMicrocode:
> 
>     ; EAX contains the linear address of the start of the Update Data
> 
>     ; EDX contains zero
> 
> @@ -465,10 +465,12 @@ LoadMicrocode:
>     mov   eax, 1
> 
>     cpuid
> 
> 
> 
> -Continue:
> 
> -   jmp   NextMicrocode
> 
> -
> 
>  Done:
> 
> +   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> +   xor   eax, eax               ; Clear EAX
> 
> +   xor   edx, edx               ; Clear EDX
> 
> +   wrmsr                        ; Load 0 to MSR at 8Bh
> 
> +
> 
>     mov   eax, 1
> 
>     cpuid
> 
>     mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> index b32fa32a89..698bb063a7 100644
> --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> @@ -141,6 +141,22 @@ ASM_PFX(LoadMicrocodeDefault):
>     jz     ParamError
> 
>     mov    rsp, rcx
> 
> 
> 
> +   ;
> 
> +   ; If microcode already loaded before this function, exit this function with
> SUCCESS.
> 
> +   ;
> 
> +   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> +   xor   eax, eax               ; Clear EAX
> 
> +   xor   edx, edx               ; Clear EDX
> 
> +   wrmsr                        ; Load 0 to MSR at 8Bh
> 
> +
> 
> +   mov   eax, 1
> 
> +   cpuid
> 
> +   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> +   rdmsr                         ; Get current microcode signature
> 
> +   xor   rax, rax
> 
> +   test  edx, edx
> 
> +   jnz   Exit2
> 
> +
> 
>     ; skip loading Microcode if the MicrocodeCodeSize is zero
> 
>     ; and report error if size is less than 2k
> 
>     ; first check UPD header revision
> 
> @@ -198,7 +214,7 @@ CheckMainHeader:
>     cmp   ebx, dword [esi + MicrocodeHdr.MicrocodeHdrProcessor]
> 
>     jne   LoadMicrocodeDefault1
> 
>     test  edx, dword [esi + MicrocodeHdr.MicrocodeHdrFlags ]
> 
> -   jnz   LoadCheck  ; Jif signature and platform ID match
> 
> +   jnz   LoadMicrocode  ; Jif signature and platform ID match
> 
> 
> 
>  LoadMicrocodeDefault1:
> 
>     ; Check if extended header exists
> 
> @@ -231,7 +247,7 @@ CheckExtSig:
>     cmp   dword [edi + ExtSig.ExtSigProcessor], ebx
> 
>     jne   LoadMicrocodeDefault2
> 
>     test  dword [edi + ExtSig.ExtSigFlags], edx
> 
> -   jnz   LoadCheck      ; Jif signature and platform ID match
> 
> +   jnz   LoadMicrocode      ; Jif signature and platform ID match
> 
>  LoadMicrocodeDefault2:
> 
>     ; Check if any more extended signatures exist
> 
>     add   edi, ExtSig.size
> 
> @@ -276,22 +292,7 @@ LoadMicrocodeDefault4:
>     ; Is valid Microcode start point ?
> 
>     cmp   dword [esi + MicrocodeHdr.MicrocodeHdrVersion], 0ffffffffh
> 
>     jz    Done
> 
> -
> 
> -LoadCheck:
> 
> -   ; Get the revision of the current microcode update loaded
> 
> -   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> -   xor   eax, eax               ; Clear EAX
> 
> -   xor   edx, edx               ; Clear EDX
> 
> -   wrmsr                        ; Load 0 to MSR at 8Bh
> 
> -
> 
> -   mov   eax, 1
> 
> -   cpuid
> 
> -   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> -   rdmsr                        ; Get current microcode signature
> 
> -
> 
> -   ; Verify this microcode update is not already loaded
> 
> -   cmp   dword [esi + MicrocodeHdr.MicrocodeHdrRevision], edx
> 
> -   je    Continue
> 
> +   jmp   CheckMainHeader
> 
> 
> 
>  LoadMicrocode:
> 
>     ; EAX contains the linear address of the start of the Update Data
> 
> @@ -306,10 +307,12 @@ LoadMicrocode:
>     mov   eax, 1
> 
>     cpuid
> 
> 
> 
> -Continue:
> 
> -   jmp   NextMicrocode
> 
> -
> 
>  Done:
> 
> +   mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> +   xor   eax, eax               ; Clear EAX
> 
> +   xor   edx, edx               ; Clear EDX
> 
> +   wrmsr                        ; Load 0 to MSR at 8Bh
> 
> +
> 
>     mov   eax, 1
> 
>     cpuid
> 
>     mov   ecx, MSR_IA32_BIOS_SIGN_ID
> 
> --
> 2.35.0.windows.1