public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow X64 Calling Convention
@ 2022-05-16 10:53 cbduggap
  2022-05-16 12:08 ` Chiu, Chasel
  0 siblings, 1 reply; 4+ messages in thread
From: cbduggap @ 2022-05-16 10:53 UTC (permalink / raw)
  To: devel; +Cc: Chasel Chiu, Nate DeSimone, Star Zeng, Ashraf Ali S

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3926
This API accept one parameter using RCX and this is consumed
in mutiple sub functions.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
Signed-off-by: cbduggap <chinni.b.duggapu@intel.com>
---
 IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 26 ++++++++---------
 .../Include/SaveRestoreSseAvxNasm.inc         | 28 +++++++++++++++++++
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
index a9f5f28ed7..9504c96b81 100644
--- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
+++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
@@ -114,7 +114,7 @@ endstruc
 global ASM_PFX(LoadMicrocodeDefault)
 ASM_PFX(LoadMicrocodeDefault):
    ; Inputs:
-   ;   rsp -> LoadMicrocodeParams pointer
+   ;   rcx -> LoadMicrocodeParams pointer
    ; Register Usage:
    ;   rsp  Preserved
    ;   All others destroyed
@@ -130,10 +130,9 @@ ASM_PFX(LoadMicrocodeDefault):
 
    cmp    rsp, 0
    jz     ParamError
-   mov    eax, dword [rsp + 8]    ; Parameter pointer
-   cmp    eax, 0
+   cmp    ecx, 0
    jz     ParamError
-   mov    esp, eax
+   mov    esp, ecx
 
    ; skip loading Microcode if the MicrocodeCodeSize is zero
    ; and report error if size is less than 2k
@@ -321,8 +320,7 @@ ASM_PFX(EstablishStackFsp):
   ;
   ; Save parameter pointer in rdx
   ;
-  mov       rdx, qword [rsp + 8]
-
+  mov       rdx, rcx
   ;
   ; Enable FSP STACK
   ;
@@ -420,7 +418,10 @@ ASM_PFX(TempRamInitApi):
   ;
   ENABLE_SSE
   ENABLE_AVX
-
+  ;
+  ; Save Input Parameter in YMM10
+  ;
+  SAVE_RCX
   ;
   ; Save RBP, RBX, RSI, RDI and RSP in YMM7, YMM8 and YMM6
   ;
@@ -442,9 +443,8 @@ ASM_PFX(TempRamInitApi):
   ;
   ; Check Parameter
   ;
-  mov       rax, qword [rsp + 8]
-  cmp       rax, 0
-  mov       rax, 08000000000000002h
+  cmp       rcx, 0
+  mov       rcx, 08000000000000002h
   jz        TempRamInitExit
 
   ;
@@ -455,18 +455,18 @@ ASM_PFX(TempRamInitApi):
   jnz       TempRamInitExit
 
   ; Load microcode
-  LOAD_RSP
+  LOAD_RCX
   CALL_YMM  ASM_PFX(LoadMicrocodeDefault)
   SAVE_UCODE_STATUS rax             ; Save microcode return status in SLOT 0 in YMM9 (upper 128bits).
   ; @note If return value rax is not 0, microcode did not load, but continue and attempt to boot.
 
   ; Call Sec CAR Init
-  LOAD_RSP
+  LOAD_RCX
   CALL_YMM  ASM_PFX(SecCarInit)
   cmp       rax, 0
   jnz       TempRamInitExit
 
-  LOAD_RSP
+  LOAD_RCX
   CALL_YMM  ASM_PFX(EstablishStackFsp)
   cmp       rax, 0
   jnz       TempRamInitExit
diff --git a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
index e8bd91669d..38c807a311 100644
--- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
+++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
@@ -177,6 +177,30 @@
             LXMMN   xmm5, %1, 1
             %endmacro
 
+;
+; Upper half of YMM10 to save/restore RCX
+;
+;
+; Save RCX to YMM10[128:191]
+; Modified: XMM5 and YMM10
+;
+
+%macro SAVE_RCX     0
+            LYMMN   ymm10, xmm5, 1
+            SXMMN   xmm5, 0, rcx
+            SYMMN   ymm10, 1, xmm5
+            %endmacro
+
+;
+; Restore RCX from YMM10[128:191]
+; Modified: XMM5 and RCX
+;
+
+%macro LOAD_RCX     0
+            LYMMN   ymm10, xmm5, 1
+            movq    rcx,  xmm5
+            %endmacro
+
 ;
 ; YMM7[128:191] for calling stack
 ; arg 1:Entry
@@ -231,6 +255,7 @@ NextAddress:
             ; Use CpuId instruction (CPUID.01H:EDX.SSE[bit 25] = 1) to test
             ; whether the processor supports SSE instruction.
             ;
+            mov     r10, rcx
             mov     rax, 1
             cpuid
             bt      rdx, 25
@@ -241,6 +266,7 @@ NextAddress:
             ;
             bt      ecx, 19
             jnc     SseError
+            mov     rcx,  r10
 
             ;
             ; Set OSFXSR bit (bit #9) & OSXMMEXCPT bit (bit #10)
@@ -258,6 +284,7 @@ NextAddress:
             %endmacro
 
 %macro ENABLE_AVX   0
+            mov     r10, rcx
             mov     eax, 1
             cpuid
             and     ecx, 10000000h
@@ -280,5 +307,6 @@ EnableAvx:
             xgetbv                 ; result in edx:eax
             or      eax, 00000006h ; Set XCR0 bit #1 and bit #2 to enable SSE state and AVX state
             xsetbv
+            mov     rcx, r10
             %endmacro
 
-- 
2.36.0.windows.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow X64 Calling Convention
  2022-05-16 10:53 [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow X64 Calling Convention cbduggap
@ 2022-05-16 12:08 ` Chiu, Chasel
  2022-05-16 16:32   ` cbduggap
  0 siblings, 1 reply; 4+ messages in thread
From: Chiu, Chasel @ 2022-05-16 12:08 UTC (permalink / raw)
  To: Duggapu, Chinni B, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Zeng, Star, S, Ashraf Ali


Thanks for correcting format and updating patch per feedbacks!
Just one more comment below inline and please also help to include patch of IntelFsp2WrapperPkg\Library\SecFspWrapperPlatformSecLibSample\X64\SecEntry.nasm for passing API parameter by RCX.
You might want to create a patch series:
	[1/2] IntelFsp2Pkg patch
	[2/2] IntelFsp2WrapperPkg patch

Thanks,
Chasel

> -----Original Message-----
> From: Duggapu, Chinni B <chinni.b.duggapu@intel.com>
> Sent: Monday, May 16, 2022 6:54 PM
> 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>; S,
> Ashraf Ali <ashraf.ali.s@intel.com>
> Subject: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow
> X64 Calling Convention
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3926
> This API accept one parameter using RCX and this is consumed in mutiple
> sub functions.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> Signed-off-by: cbduggap <chinni.b.duggapu@intel.com>
> ---
>  IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 26 ++++++++---------
>  .../Include/SaveRestoreSseAvxNasm.inc         | 28 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 13 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> index a9f5f28ed7..9504c96b81 100644
> --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> @@ -114,7 +114,7 @@ endstruc
>  global ASM_PFX(LoadMicrocodeDefault)
> ASM_PFX(LoadMicrocodeDefault):    ; Inputs:-   ;   rsp ->
> LoadMicrocodeParams pointer+   ;   rcx -> LoadMicrocodeParams pointer    ;
> Register Usage:    ;   rsp  Preserved    ;   All others destroyed@@ -130,10
> +130,9 @@ ASM_PFX(LoadMicrocodeDefault):
>      cmp    rsp, 0    jz     ParamError-   mov    eax, dword [rsp + 8]    ;
> Parameter pointer-   cmp    eax, 0+   cmp    ecx, 0    jz     ParamError-   mov
> esp, eax+   mov    esp, ecx



I think we do not need to modify esp because now esp/rsp only containing return address initialized by caller.



     ; skip loading Microcode if the
> MicrocodeCodeSize is zero    ; and report error if size is less than 2k@@ -
> 321,8 +320,7 @@ ASM_PFX(EstablishStackFsp):
>    ;   ; Save parameter pointer in rdx   ;-  mov       rdx, qword [rsp + 8]-+  mov
> rdx, rcx   ;   ; Enable FSP STACK   ;@@ -420,7 +418,10 @@
> ASM_PFX(TempRamInitApi):
>    ;   ENABLE_SSE   ENABLE_AVX-+  ;+  ; Save Input Parameter in YMM10+  ;+
> SAVE_RCX   ;   ; Save RBP, RBX, RSI, RDI and RSP in YMM7, YMM8 and
> YMM6   ;@@ -442,9 +443,8 @@ ASM_PFX(TempRamInitApi):
>    ;   ; Check Parameter   ;-  mov       rax, qword [rsp + 8]-  cmp       rax, 0-
> mov       rax, 08000000000000002h+  cmp       rcx, 0+  mov       rcx,
> 08000000000000002h   jz        TempRamInitExit    ;@@ -455,18 +455,18
> @@ ASM_PFX(TempRamInitApi):
>    jnz       TempRamInitExit    ; Load microcode-  LOAD_RSP+  LOAD_RCX
> CALL_YMM  ASM_PFX(LoadMicrocodeDefault)   SAVE_UCODE_STATUS
> rax             ; Save microcode return status in SLOT 0 in YMM9 (upper
> 128bits).   ; @note If return value rax is not 0, microcode did not load, but
> continue and attempt to boot.    ; Call Sec CAR Init-  LOAD_RSP+  LOAD_RCX
> CALL_YMM  ASM_PFX(SecCarInit)   cmp       rax, 0   jnz       TempRamInitExit
> -  LOAD_RSP+  LOAD_RCX   CALL_YMM  ASM_PFX(EstablishStackFsp)   cmp
> rax, 0   jnz       TempRamInitExitdiff --git
> a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> index e8bd91669d..38c807a311 100644
> --- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> +++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> @@ -177,6 +177,30 @@
>              LXMMN   xmm5, %1, 1             %endmacro +;+; Upper half of
> YMM10 to save/restore RCX+;+;+; Save RCX to YMM10[128:191]+;
> Modified: XMM5 and YMM10+;++%macro SAVE_RCX     0+            LYMMN
> ymm10, xmm5, 1+            SXMMN   xmm5, 0, rcx+            SYMMN   ymm10,
> 1, xmm5+            %endmacro++;+; Restore RCX from YMM10[128:191]+;
> Modified: XMM5 and RCX+;++%macro LOAD_RCX     0+            LYMMN
> ymm10, xmm5, 1+            movq    rcx,  xmm5+            %endmacro+ ; ;
> YMM7[128:191] for calling stack ; arg 1:Entry@@ -231,6 +255,7 @@
> NextAddress:
>              ; Use CpuId instruction (CPUID.01H:EDX.SSE[bit 25] = 1) to
> test             ; whether the processor supports SSE instruction.             ;+
> mov     r10, rcx             mov     rax, 1             cpuid             bt      rdx, 25@@ -
> 241,6 +266,7 @@ NextAddress:
>              ;             bt      ecx, 19             jnc     SseError+            mov     rcx,
> r10              ;             ; Set OSFXSR bit (bit #9) & OSXMMEXCPT bit (bit
> #10)@@ -258,6 +284,7 @@ NextAddress:
>              %endmacro  %macro ENABLE_AVX   0+            mov     r10, rcx
> mov     eax, 1             cpuid             and     ecx, 10000000h@@ -280,5 +307,6
> @@ EnableAvx:
>              xgetbv                 ; result in edx:eax             or      eax, 00000006h ; Set
> XCR0 bit #1 and bit #2 to enable SSE state and AVX state             xsetbv+
> mov     rcx, r10             %endmacro --
> 2.36.0.windows.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow X64 Calling Convention
  2022-05-16 12:08 ` Chiu, Chasel
@ 2022-05-16 16:32   ` cbduggap
  2022-05-17  0:27     ` Chiu, Chasel
  0 siblings, 1 reply; 4+ messages in thread
From: cbduggap @ 2022-05-16 16:32 UTC (permalink / raw)
  To: Chiu, Chasel, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Zeng, Star, S, Ashraf Ali

HI Chasel,
Yes, we don't need to modify esp for LoadMicrocodeDefault. However, this function does couple of MSR Accesses in b/w that would lead to modify RCX anyway.
So, if not RSP, we need to use different register to save RCX and consume in the whole function. 

That's why I have not changed the usage of RSP to hold the input parameter.  
 


Thanks,
Chinni.

-----Original Message-----
From: Chiu, Chasel <chasel.chiu@intel.com> 
Sent: Monday, May 16, 2022 5:38 PM
To: Duggapu, Chinni B <chinni.b.duggapu@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
Subject: RE: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow X64 Calling Convention


Thanks for correcting format and updating patch per feedbacks!
Just one more comment below inline and please also help to include patch of IntelFsp2WrapperPkg\Library\SecFspWrapperPlatformSecLibSample\X64\SecEntry.nasm for passing API parameter by RCX.
You might want to create a patch series:
	[1/2] IntelFsp2Pkg patch
	[2/2] IntelFsp2WrapperPkg patch

Thanks,
Chasel

> -----Original Message-----
> From: Duggapu, Chinni B <chinni.b.duggapu@intel.com>
> Sent: Monday, May 16, 2022 6:54 PM
> 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>; S, 
> Ashraf Ali <ashraf.ali.s@intel.com>
> Subject: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow
> X64 Calling Convention
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3926
> This API accept one parameter using RCX and this is consumed in 
> mutiple sub functions.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> Signed-off-by: cbduggap <chinni.b.duggapu@intel.com>
> ---
>  IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 26 ++++++++---------
>  .../Include/SaveRestoreSseAvxNasm.inc         | 28 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 13 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> index a9f5f28ed7..9504c96b81 100644
> --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> @@ -114,7 +114,7 @@ endstruc
>  global ASM_PFX(LoadMicrocodeDefault)
> ASM_PFX(LoadMicrocodeDefault):    ; Inputs:-   ;   rsp ->
> LoadMicrocodeParams pointer+   ;   rcx -> LoadMicrocodeParams pointer    ;
> Register Usage:    ;   rsp  Preserved    ;   All others destroyed@@ -130,10
> +130,9 @@ ASM_PFX(LoadMicrocodeDefault):
>      cmp    rsp, 0    jz     ParamError-   mov    eax, dword [rsp + 8]    ;
> Parameter pointer-   cmp    eax, 0+   cmp    ecx, 0    jz     ParamError-   mov
> esp, eax+   mov    esp, ecx



I think we do not need to modify esp because now esp/rsp only containing return address initialized by caller.



     ; skip loading Microcode if the
> MicrocodeCodeSize is zero    ; and report error if size is less than 2k@@ -
> 321,8 +320,7 @@ ASM_PFX(EstablishStackFsp):
>    ;   ; Save parameter pointer in rdx   ;-  mov       rdx, qword [rsp + 8]-+  mov
> rdx, rcx   ;   ; Enable FSP STACK   ;@@ -420,7 +418,10 @@
> ASM_PFX(TempRamInitApi):
>    ;   ENABLE_SSE   ENABLE_AVX-+  ;+  ; Save Input Parameter in YMM10+  ;+
> SAVE_RCX   ;   ; Save RBP, RBX, RSI, RDI and RSP in YMM7, YMM8 and
> YMM6   ;@@ -442,9 +443,8 @@ ASM_PFX(TempRamInitApi):
>    ;   ; Check Parameter   ;-  mov       rax, qword [rsp + 8]-  cmp       rax, 0-
> mov       rax, 08000000000000002h+  cmp       rcx, 0+  mov       rcx,
> 08000000000000002h   jz        TempRamInitExit    ;@@ -455,18 +455,18
> @@ ASM_PFX(TempRamInitApi):
>    jnz       TempRamInitExit    ; Load microcode-  LOAD_RSP+  LOAD_RCX
> CALL_YMM  ASM_PFX(LoadMicrocodeDefault)   SAVE_UCODE_STATUS
> rax             ; Save microcode return status in SLOT 0 in YMM9 (upper
> 128bits).   ; @note If return value rax is not 0, microcode did not load, but
> continue and attempt to boot.    ; Call Sec CAR Init-  LOAD_RSP+  LOAD_RCX
> CALL_YMM  ASM_PFX(SecCarInit)   cmp       rax, 0   jnz       TempRamInitExit
> -  LOAD_RSP+  LOAD_RCX   CALL_YMM  ASM_PFX(EstablishStackFsp)   cmp
> rax, 0   jnz       TempRamInitExitdiff --git
> a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> index e8bd91669d..38c807a311 100644
> --- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> +++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> @@ -177,6 +177,30 @@
>              LXMMN   xmm5, %1, 1             %endmacro +;+; Upper half of
> YMM10 to save/restore RCX+;+;+; Save RCX to YMM10[128:191]+;
> Modified: XMM5 and YMM10+;++%macro SAVE_RCX     0+            LYMMN
> ymm10, xmm5, 1+            SXMMN   xmm5, 0, rcx+            SYMMN   ymm10,
> 1, xmm5+            %endmacro++;+; Restore RCX from YMM10[128:191]+;
> Modified: XMM5 and RCX+;++%macro LOAD_RCX     0+            LYMMN
> ymm10, xmm5, 1+            movq    rcx,  xmm5+            %endmacro+ ; ;
> YMM7[128:191] for calling stack ; arg 1:Entry@@ -231,6 +255,7 @@
> NextAddress:
>              ; Use CpuId instruction (CPUID.01H:EDX.SSE[bit 25] = 1) to
> test             ; whether the processor supports SSE instruction.             ;+
> mov     r10, rcx             mov     rax, 1             cpuid             bt      rdx, 25@@ -
> 241,6 +266,7 @@ NextAddress:
>              ;             bt      ecx, 19             jnc     SseError+            mov     rcx,
> r10              ;             ; Set OSFXSR bit (bit #9) & OSXMMEXCPT bit (bit
> #10)@@ -258,6 +284,7 @@ NextAddress:
>              %endmacro  %macro ENABLE_AVX   0+            mov     r10, rcx
> mov     eax, 1             cpuid             and     ecx, 10000000h@@ -280,5 +307,6
> @@ EnableAvx:
>              xgetbv                 ; result in edx:eax             or      eax, 00000006h ; Set
> XCR0 bit #1 and bit #2 to enable SSE state and AVX state             xsetbv+
> mov     rcx, r10             %endmacro --
> 2.36.0.windows.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow X64 Calling Convention
  2022-05-16 16:32   ` cbduggap
@ 2022-05-17  0:27     ` Chiu, Chasel
  0 siblings, 0 replies; 4+ messages in thread
From: Chiu, Chasel @ 2022-05-17  0:27 UTC (permalink / raw)
  To: Duggapu, Chinni B, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Zeng, Star, S, Ashraf Ali


Thanks for clarification!
In this case, please use "mov rsp, rcx" to support 64bit addressing.

Thanks,
Chasel


> -----Original Message-----
> From: Duggapu, Chinni B <chinni.b.duggapu@intel.com>
> Sent: Tuesday, May 17, 2022 12:33 AM
> To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star
> <star.zeng@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
> Subject: RE: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must
> follow X64 Calling Convention
> 
> HI Chasel,
> Yes, we don't need to modify esp for LoadMicrocodeDefault. However, this
> function does couple of MSR Accesses in b/w that would lead to modify RCX
> anyway.
> So, if not RSP, we need to use different register to save RCX and consume in
> the whole function.
> 
> That's why I have not changed the usage of RSP to hold the input parameter.
> 
> 
> 
> Thanks,
> Chinni.
> 
> -----Original Message-----
> From: Chiu, Chasel <chasel.chiu@intel.com>
> Sent: Monday, May 16, 2022 5:38 PM
> To: Duggapu, Chinni B <chinni.b.duggapu@intel.com>;
> devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star
> <star.zeng@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
> Subject: RE: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must
> follow X64 Calling Convention
> 
> 
> Thanks for correcting format and updating patch per feedbacks!
> Just one more comment below inline and please also help to include patch
> of
> IntelFsp2WrapperPkg\Library\SecFspWrapperPlatformSecLibSample\X64\S
> ecEntry.nasm for passing API parameter by RCX.
> You might want to create a patch series:
> 	[1/2] IntelFsp2Pkg patch
> 	[2/2] IntelFsp2WrapperPkg patch
> 
> Thanks,
> Chasel
> 
> > -----Original Message-----
> > From: Duggapu, Chinni B <chinni.b.duggapu@intel.com>
> > Sent: Monday, May 16, 2022 6:54 PM
> > 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>; S,
> > Ashraf Ali <ashraf.ali.s@intel.com>
> > Subject: [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow
> > X64 Calling Convention
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3926
> > This API accept one parameter using RCX and this is consumed in
> > mutiple sub functions.
> >
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> > Signed-off-by: cbduggap <chinni.b.duggapu@intel.com>
> > ---
> >  IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 26 ++++++++---------
> >  .../Include/SaveRestoreSseAvxNasm.inc         | 28
> +++++++++++++++++++
> >  2 files changed, 41 insertions(+), 13 deletions(-)
> >
> > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> > b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> > index a9f5f28ed7..9504c96b81 100644
> > --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
> > @@ -114,7 +114,7 @@ endstruc
> >  global ASM_PFX(LoadMicrocodeDefault)
> > ASM_PFX(LoadMicrocodeDefault):    ; Inputs:-   ;   rsp ->
> > LoadMicrocodeParams pointer+   ;   rcx -> LoadMicrocodeParams
> pointer    ;
> > Register Usage:    ;   rsp  Preserved    ;   All others destroyed@@ -130,10
> > +130,9 @@ ASM_PFX(LoadMicrocodeDefault):
> >      cmp    rsp, 0    jz     ParamError-   mov    eax, dword [rsp + 8]    ;
> > Parameter pointer-   cmp    eax, 0+   cmp    ecx, 0    jz     ParamError-   mov
> > esp, eax+   mov    esp, ecx
> 
> 
> 
> I think we do not need to modify esp because now esp/rsp only containing
> return address initialized by caller.
> 
> 
> 
>      ; skip loading Microcode if the
> > MicrocodeCodeSize is zero    ; and report error if size is less than 2k@@ -
> > 321,8 +320,7 @@ ASM_PFX(EstablishStackFsp):
> >    ;   ; Save parameter pointer in rdx   ;-  mov       rdx, qword [rsp + 8]-+
> mov
> > rdx, rcx   ;   ; Enable FSP STACK   ;@@ -420,7 +418,10 @@
> > ASM_PFX(TempRamInitApi):
> >    ;   ENABLE_SSE   ENABLE_AVX-+  ;+  ; Save Input Parameter in
> YMM10+  ;+
> > SAVE_RCX   ;   ; Save RBP, RBX, RSI, RDI and RSP in YMM7, YMM8 and
> > YMM6   ;@@ -442,9 +443,8 @@ ASM_PFX(TempRamInitApi):
> >    ;   ; Check Parameter   ;-  mov       rax, qword [rsp + 8]-  cmp       rax, 0-
> > mov       rax, 08000000000000002h+  cmp       rcx, 0+  mov       rcx,
> > 08000000000000002h   jz        TempRamInitExit    ;@@ -455,18 +455,18
> > @@ ASM_PFX(TempRamInitApi):
> >    jnz       TempRamInitExit    ; Load microcode-  LOAD_RSP+  LOAD_RCX
> > CALL_YMM  ASM_PFX(LoadMicrocodeDefault)   SAVE_UCODE_STATUS
> > rax             ; Save microcode return status in SLOT 0 in YMM9 (upper
> > 128bits).   ; @note If return value rax is not 0, microcode did not load, but
> > continue and attempt to boot.    ; Call Sec CAR Init-  LOAD_RSP+
> LOAD_RCX
> > CALL_YMM  ASM_PFX(SecCarInit)   cmp       rax, 0   jnz
> TempRamInitExit
> > -  LOAD_RSP+  LOAD_RCX   CALL_YMM  ASM_PFX(EstablishStackFsp)
> cmp
> > rax, 0   jnz       TempRamInitExitdiff --git
> > a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> > b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> > index e8bd91669d..38c807a311 100644
> > --- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> > +++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc
> > @@ -177,6 +177,30 @@
> >              LXMMN   xmm5, %1, 1             %endmacro +;+; Upper half of
> > YMM10 to save/restore RCX+;+;+; Save RCX to YMM10[128:191]+;
> > Modified: XMM5 and YMM10+;++%macro SAVE_RCX     0+            LYMMN
> > ymm10, xmm5, 1+            SXMMN   xmm5, 0, rcx+            SYMMN
> ymm10,
> > 1, xmm5+            %endmacro++;+; Restore RCX from YMM10[128:191]+;
> > Modified: XMM5 and RCX+;++%macro LOAD_RCX     0+            LYMMN
> > ymm10, xmm5, 1+            movq    rcx,  xmm5+            %endmacro+ ; ;
> > YMM7[128:191] for calling stack ; arg 1:Entry@@ -231,6 +255,7 @@
> > NextAddress:
> >              ; Use CpuId instruction (CPUID.01H:EDX.SSE[bit 25] = 1) to
> > test             ; whether the processor supports SSE instruction.             ;+
> > mov     r10, rcx             mov     rax, 1             cpuid             bt      rdx, 25@@ -
> > 241,6 +266,7 @@ NextAddress:
> >              ;             bt      ecx, 19             jnc     SseError+            mov     rcx,
> > r10              ;             ; Set OSFXSR bit (bit #9) & OSXMMEXCPT bit (bit
> > #10)@@ -258,6 +284,7 @@ NextAddress:
> >              %endmacro  %macro ENABLE_AVX   0+            mov     r10, rcx
> > mov     eax, 1             cpuid             and     ecx, 10000000h@@ -280,5
> +307,6
> > @@ EnableAvx:
> >              xgetbv                 ; result in edx:eax             or      eax, 00000006h ;
> Set
> > XCR0 bit #1 and bit #2 to enable SSE state and AVX state             xsetbv+
> > mov     rcx, r10             %endmacro --
> > 2.36.0.windows.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-05-17  0:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-16 10:53 [PATCH v3] IntelFsp2Pkg: FSP_TEMP_RAM_INIT call must follow X64 Calling Convention cbduggap
2022-05-16 12:08 ` Chiu, Chasel
2022-05-16 16:32   ` cbduggap
2022-05-17  0:27     ` Chiu, Chasel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox