1. Patchew
  2. EDK2
  3. View series

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

Chiu, Chasel posted 1 patch 1 year, 1 month ago
Failed in applying to current master (apply log)
There is a newer version of this series
IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 4 ++--
IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm    | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
[edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Chiu, Chasel 1 year, 1 month ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Ni, Ray 1 year, 1 month ago 
>  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?



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101399): https://edk2.groups.io/g/devel/message/101399
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Pedro Falcato 1 year, 1 month ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101400): https://edk2.groups.io/g/devel/message/101400
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Nate DeSimone 1 year, 1 month ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101515): https://edk2.groups.io/g/devel/message/101515
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Chiu, Chasel 1 year, 1 month ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101527): https://edk2.groups.io/g/devel/message/101527
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Ashraf Ali S 1 year, 1 month ago 
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] -=-=-=-=-=-=




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101363): https://edk2.groups.io/g/devel/message/101363
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Marvin Häuser 1 year, 1 month ago 
Hi Ashraf,

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

Best regards,
Marvin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101366): https://edk2.groups.io/g/devel/message/101366
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Ashraf Ali S 1 year, 1 month ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101367): https://edk2.groups.io/g/devel/message/101367
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Marvin Häuser 1 year, 1 month ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101368): https://edk2.groups.io/g/devel/message/101368
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fix NASM X64 build warnings.
Posted by Ashraf Ali S 1 year, 1 month ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101369): https://edk2.groups.io/g/devel/message/101369
Mute This Topic: https://groups.io/mt/97678369/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.