[PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Chiu, Chasel]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4377

Fix below warnings generated by NASM X64 build:
/X64/FspHelper.iii:26: warning: signed dword value exceeds bounds
/X64/FspHelper.iii:35: warning: signed dword value exceeds bounds
/X64/FspApiEntryT.iii:320: warning: dword data exceeds bounds

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
---
 IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 4 ++--
 IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm    | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
index cdebe90fab..56d6abaea6 100644
--- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
+++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
@@ -317,7 +317,7 @@ Done:
    xor   eax, eax
    cmp   edx, 0
    jnz   Exit2
-   mov   eax, 0800000000000000Eh
+   mov   rax, 0800000000000000Eh
 
 Exit2:
    jmp   rbp
@@ -464,7 +464,7 @@ ParamValid:
   ; Sec Platform Init
   ;
   CALL_YMM  ASM_PFX(SecPlatformInit)
-  cmp       eax, 0
+  cmp       rax, 0
   jnz       TempRamInitExit
 
   ; Load microcode
diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm
index 71624a3aad..ec9140b73c 100644
--- a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm
+++ b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm
@@ -23,7 +23,7 @@ ASM_PFX(AsmGetFspInfoHeader):
 global ASM_PFX(FspInfoHeaderRelativeOff)
 ASM_PFX(FspInfoHeaderRelativeOff):
    DD    0x12345678               ; This value must be patched by the build script
-   and   rax, 0xffffffff
+   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF
    ret
 
 global ASM_PFX(AsmGetFspInfoHeaderNoStack)
@@ -32,5 +32,5 @@ ASM_PFX(AsmGetFspInfoHeaderNoStack):
    lea   rcx, [ASM_PFX(FspInfoHeaderRelativeOff)]
    mov   ecx, [rcx]
    sub   rax, rcx
-   and   rax, 0xffffffff
+   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF
    jmp   rdi
-- 
2.35.0.windows.1

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Ashraf Ali S]

Hi., Chasel

RAX holds the FsptImageBaseAddress, the AND operation is performed to clear the upper 32bits of RAX registers.
Don't we have to clear the upper 32bit of RAX registers?

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu, Chasel
Sent: Friday, March 17, 2023 10:51 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>
Subject: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4377

Fix below warnings generated by NASM X64 build:
/X64/FspHelper.iii:26: warning: signed dword value exceeds bounds
/X64/FspHelper.iii:35: warning: signed dword value exceeds bounds
/X64/FspApiEntryT.iii:320: warning: dword data exceeds bounds

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
---
 IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 4 ++--
 IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm    | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
index cdebe90fab..56d6abaea6 100644
--- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
+++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
@@ -317,7 +317,7 @@ Done:
    xor   eax, eax    cmp   edx, 0    jnz   Exit2-   mov   eax, 0800000000000000Eh+   mov   rax, 0800000000000000Eh  Exit2:    jmp   rbp@@ -464,7 +464,7 @@ ParamValid:
   ; Sec Platform Init   ;   CALL_YMM  ASM_PFX(SecPlatformInit)-  cmp       eax, 0+  cmp       rax, 0   jnz       TempRamInitExit    ; Load microcodediff --git a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm
index 71624a3aad..ec9140b73c 100644
--- a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm
+++ b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm
@@ -23,7 +23,7 @@ ASM_PFX(AsmGetFspInfoHeader):
 global ASM_PFX(FspInfoHeaderRelativeOff) ASM_PFX(FspInfoHeaderRelativeOff):    DD    0x12345678               ; This value must be patched by the build script-   and   rax, 0xffffffff+   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF    ret  global ASM_PFX(AsmGetFspInfoHeaderNoStack)@@ -32,5 +32,5 @@ ASM_PFX(AsmGetFspInfoHeaderNoStack):
    lea   rcx, [ASM_PFX(FspInfoHeaderRelativeOff)]    mov   ecx, [rcx]    sub   rax, rcx-   and   rax, 0xffffffff+   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF    jmp   rdi-- 
2.35.0.windows.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101356): https://edk2.groups.io/g/devel/message/101356
Mute This Topic: https://groups.io/mt/97678369/6226280
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ashraf.ali.s@intel.com] -=-=-=-=-=-=

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 137 bytes --]

Hi Chasel,

If you want to improve the ASM further, maybe opt for “test reg, reg” over “cmp reg, 0”?

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 173 bytes --]

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 95 bytes --]

Hi Ashraf,

”mov eax, eax” does clear the high 32 Bits of rax.

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 181 bytes --]

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Ashraf Ali S]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

Hi.,

Nope, it will not clear the upper 32bit right.


From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Sunday, March 19, 2023 3:38 AM
To: S, Ashraf Ali <ashraf.ali.s@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

Hi Ashraf,

”mov eax, eax” does clear the high 32 Bits of rax.

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 2271 bytes --]

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

Yes - it does. Most (if not all?) operations on 32-bit registers zero-extend the corresponding 64-bit register. This is an AMD64 / Intel 64 design to combat partial register stall. Please consult the SDM (or at least try it out).

What I didn’t realize is that “mov eax, eax” apparently defeats register renaming optimisations: https://stackoverflow.com/a/45660140

Best regards,
Marvin

> On 19. Mar 2023, at 10:07, S, Ashraf Ali <ashraf.ali.s@intel.com> wrote:
> 
> Hi.,
>  
> Nope, it will not clear the upper 32bit right.
>  
>  
> From: Marvin Häuser <mhaeuser@posteo.de> 
> Sent: Sunday, March 19, 2023 3:38 AM
> To: S, Ashraf Ali <ashraf.ali.s@intel.com>; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
>  
> Hi Ashraf,
> 
> ”mov eax, eax” does clear the high 32 Bits of rax.
> 
> Best regards,
> Marvin

[-- Attachment #2: Type: text/html, Size: 2722 bytes --]

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Ashraf Ali S]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

Yes, you are right. Tested on the H/W, It clears the upper 32bits. 😊

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Sunday, March 19, 2023 3:07 PM
To: S, Ashraf Ali <ashraf.ali.s@intel.com>
Cc: devel@edk2.groups.io; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

Yes - it does. Most (if not all?) operations on 32-bit registers zero-extend the corresponding 64-bit register. This is an AMD64 / Intel 64 design to combat partial register stall. Please consult the SDM (or at least try it out).

What I didn’t realize is that “mov eax, eax” apparently defeats register renaming optimisations: https://stackoverflow.com/a/45660140

Best regards,
Marvin


On 19. Mar 2023, at 10:07, S, Ashraf Ali <ashraf.ali.s@intel.com<mailto:ashraf.ali.s@intel.com>> wrote:

Hi.,

Nope, it will not clear the upper 32bit right.


From: Marvin Häuser <mhaeuser@posteo.de<mailto:mhaeuser@posteo.de>>
Sent: Sunday, March 19, 2023 3:38 AM
To: S, Ashraf Ali <ashraf.ali.s@intel.com<mailto:ashraf.ali.s@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

Hi Ashraf,

”mov eax, eax” does clear the high 32 Bits of rax.

Best regards,
Marvin


[-- Attachment #2: Type: text/html, Size: 4868 bytes --]

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Ni, Ray]

>  ASM_PFX(FspInfoHeaderRelativeOff):
> 
>     DD    0x12345678               ; This value must be patched by the build script
> 
> -   and   rax, 0xffffffff
> 
> +   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF

Based on the discussion, we know "mov eax, eax" clears upper 32bits of RAX.
But this code looks very confusing. Is there any other instruction that can do the same thing?

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Pedro Falcato]

On Mon, Mar 20, 2023 at 6:03 AM Ni, Ray <ray.ni@intel.com> wrote:
>
> >  ASM_PFX(FspInfoHeaderRelativeOff):
> >
> >     DD    0x12345678               ; This value must be patched by the build script
> >
> > -   and   rax, 0xffffffff
> >
> > +   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF
>
> Based on the discussion, we know "mov eax, eax" clears upper 32bits of RAX.
> But this code looks very confusing. Is there any other instruction that can do the same thing?

Hi Ray,

Any instruction that writes to the lower 32-bits should zero the upper bits.
(Pardon my AT&T syntax, just reverse the operands for Intel syntax)

and $0xffffffff,%eax gets you a 3 byte opcode (since imm8 is signed,
you only need 0xff as the immediate)
and %eax, %eax gets you 2 bytes
mov %eax, %eax gets you 2 bytes

even something silly like adding 0 to EAX should work. But in a pure
efficiency+size POV, the last 2 instructions should be optimal.

-- 
Pedro

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Nate DeSimone]

Reviewing this code in more detail... I think clearing the upper 32-bits is a bug. These functions are supposed to return pointers, and since this is X64 code those pointers could be anywhere in address space. The fact that the FSP is in XIP NEM, which on current Intel platforms just happens to be mapped <4GB does not mean that this pointer will always be 4GB. Therefore, I believe the correct course of action is to delete the AND/MOV instructions in question.

Thanks,
Nate

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Monday, March 20, 2023 12:31 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

On Mon, Mar 20, 2023 at 6:03 AM Ni, Ray <ray.ni@intel.com> wrote:
>
> >  ASM_PFX(FspInfoHeaderRelativeOff):
> >
> >     DD    0x12345678               ; This value must be patched by the build script
> >
> > -   and   rax, 0xffffffff
> >
> > +   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF
>
> Based on the discussion, we know "mov eax, eax" clears upper 32bits of RAX.
> But this code looks very confusing. Is there any other instruction that can do the same thing?

Hi Ray,

Any instruction that writes to the lower 32-bits should zero the upper bits.
(Pardon my AT&T syntax, just reverse the operands for Intel syntax)

and $0xffffffff,%eax gets you a 3 byte opcode (since imm8 is signed, you only need 0xff as the immediate) and %eax, %eax gets you 2 bytes mov %eax, %eax gets you 2 bytes

even something silly like adding 0 to EAX should work. But in a pure
efficiency+size POV, the last 2 instructions should be optimal.

--
Pedro

Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.

[Chiu, Chasel]

Hello,

Thanks for all the feedbacks and suggestions from everybody! I have sent V3 patch accordingly, please help to review again: https://edk2.groups.io/g/devel/message/101526

Thanks,
Chasel


> -----Original Message-----
> From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> Sent: Tuesday, March 21, 2023 4:05 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
> 
> Reviewing this code in more detail... I think clearing the upper 32-bits is a bug.
> These functions are supposed to return pointers, and since this is X64 code those
> pointers could be anywhere in address space. The fact that the FSP is in XIP NEM,
> which on current Intel platforms just happens to be mapped <4GB does not
> mean that this pointer will always be 4GB. Therefore, I believe the correct
> course of action is to delete the AND/MOV instructions in question.
> 
> Thanks,
> Nate
> 
> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Monday, March 20, 2023 12:31 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
> 
> On Mon, Mar 20, 2023 at 6:03 AM Ni, Ray <ray.ni@intel.com> wrote:
> >
> > >  ASM_PFX(FspInfoHeaderRelativeOff):
> > >
> > >     DD    0x12345678               ; This value must be patched by the build script
> > >
> > > -   and   rax, 0xffffffff
> > >
> > > +   mov   eax, eax                 ; equal to and rax, 0xFFFFFFFF
> >
> > Based on the discussion, we know "mov eax, eax" clears upper 32bits of RAX.
> > But this code looks very confusing. Is there any other instruction that can do
> the same thing?
> 
> Hi Ray,
> 
> Any instruction that writes to the lower 32-bits should zero the upper bits.
> (Pardon my AT&T syntax, just reverse the operands for Intel syntax)
> 
> and $0xffffffff,%eax gets you a 3 byte opcode (since imm8 is signed, you only
> need 0xff as the immediate) and %eax, %eax gets you 2 bytes mov %eax, %eax
> gets you 2 bytes
> 
> even something silly like adding 0 to EAX should work. But in a pure
> efficiency+size POV, the last 2 instructions should be optimal.
> 
> --
> Pedro