* [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
@ 2017-05-22 17:12 Michael Kinney
2017-05-22 17:14 ` Andrew Fish
2017-05-23 2:08 ` Fan, Jeff
0 siblings, 2 replies; 9+ messages in thread
From: Michael Kinney @ 2017-05-22 17:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Andrew Fish, Jeff Fan, Michael D Kinney
https://bugzilla.tianocore.org/show_bug.cgi?id=565
Fix NASM compatibility issues with XCODE5 tool chain.
The XCODE5 tool chain for X64 builds using PIE (Position
Independent Executable). For most assembly sources using
PIE mode does not cause any issues.
However, if assembly code is copied to a different address
(such as AP startup code in the MpInitLib), then the
X64 assembly source must be implemented to be compatible
with PIE mode that uses RIP relative addressing.
The specific changes in this patch are:
* Use LEA instruction instead of MOV instruction to lookup
the addresses of functions.
* The assembly function RendezvousFunnelProc() is copied
below 1MB so it can be executed as part of the MpInitLib
AP startup sequence. RendezvousFunnelProc() calls the
external function InitializeFloatingPointUnits(). The
absolute address of InitializeFloatingPointUnits() is
added to the MP_CPU_EXCHANGE_INFO structure that is passed
to RendezvousFunnelProc().
Cc: Andrew Fish <afish@apple.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 ++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 1 +
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 ++++----
4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 407c44c..735e099 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -751,6 +751,8 @@ FillExchangeInfoData (
ExchangeInfo->EnableExecuteDisable = IsBspExecuteDisableEnabled ();
+ ExchangeInfo->InitializeFloatingPointUnitsAddress = (UINTN)InitializeFloatingPointUnits;
+
//
// Get the BSP's data of GDT and IDT
//
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 989b3f8..ea56412 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -177,6 +177,7 @@ typedef struct {
UINTN InitFlag;
CPU_INFO_IN_HOB *CpuInfo;
CPU_MP_DATA *CpuMpData;
+ UINTN InitializeFloatingPointUnitsAddress;
} MP_CPU_EXCHANGE_INFO;
#pragma pack()
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index a63cd23..852281a 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -40,4 +40,5 @@ EnableExecuteDisableLocation equ LockLocation + 5Ch
Cr3Location equ LockLocation + 64h
InitFlagLocation equ LockLocation + 6Ch
CpuInfoLocation equ LockLocation + 74h
+InitializeFloatingPointUnitsAddress equ LockLocation + 84h
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index fa54d01..0b14a53 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -1,5 +1,5 @@
;------------------------------------------------------------------------------ ;
-; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
; This program and the accompanying materials
; are licensed and made available under the terms and conditions of the BSD License
; which accompanies this distribution. The full text of the license may be found at
@@ -201,7 +201,7 @@ CProcedureInvoke:
push rbp
mov rbp, rsp
- mov rax, ASM_PFX(InitializeFloatingPointUnits)
+ mov rax, qword [esi + InitializeFloatingPointUnitsAddress]
sub rsp, 20h
call rax ; Call assembly function to initialize FPU per UEFI spec
add rsp, 20h
@@ -282,11 +282,11 @@ AsmRelocateApLoopEnd:
;-------------------------------------------------------------------------------------
global ASM_PFX(AsmGetAddressMap)
ASM_PFX(AsmGetAddressMap):
- mov rax, ASM_PFX(RendezvousFunnelProc)
+ lea rax, [ASM_PFX(RendezvousFunnelProc)]
mov qword [rcx], rax
mov qword [rcx + 8h], LongModeStart - RendezvousFunnelProcStart
mov qword [rcx + 10h], RendezvousFunnelProcEnd - RendezvousFunnelProcStart
- mov rax, ASM_PFX(AsmRelocateApLoop)
+ lea rax, [ASM_PFX(AsmRelocateApLoop)]
mov qword [rcx + 18h], rax
mov qword [rcx + 20h], AsmRelocateApLoopEnd - AsmRelocateApLoopStart
ret
--
2.6.3.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-05-22 17:12 [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues Michael Kinney
@ 2017-05-22 17:14 ` Andrew Fish
2017-05-23 2:08 ` Fan, Jeff
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Fish @ 2017-05-22 17:14 UTC (permalink / raw)
To: Mike Kinney; +Cc: edk2-devel, Jeff Fan
Reviewed-by: Andrew Fish <afish@apple.com>
> On May 22, 2017, at 10:12 AM, Michael Kinney <michael.d.kinney@intel.com> wrote:
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=565
>
> Fix NASM compatibility issues with XCODE5 tool chain.
> The XCODE5 tool chain for X64 builds using PIE (Position
> Independent Executable). For most assembly sources using
> PIE mode does not cause any issues.
>
> However, if assembly code is copied to a different address
> (such as AP startup code in the MpInitLib), then the
> X64 assembly source must be implemented to be compatible
> with PIE mode that uses RIP relative addressing.
>
> The specific changes in this patch are:
>
> * Use LEA instruction instead of MOV instruction to lookup
> the addresses of functions.
>
> * The assembly function RendezvousFunnelProc() is copied
> below 1MB so it can be executed as part of the MpInitLib
> AP startup sequence. RendezvousFunnelProc() calls the
> external function InitializeFloatingPointUnits(). The
> absolute address of InitializeFloatingPointUnits() is
> added to the MP_CPU_EXCHANGE_INFO structure that is passed
> to RendezvousFunnelProc().
>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 ++
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 1 +
> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 ++++----
> 4 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 407c44c..735e099 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -751,6 +751,8 @@ FillExchangeInfoData (
>
> ExchangeInfo->EnableExecuteDisable = IsBspExecuteDisableEnabled ();
>
> + ExchangeInfo->InitializeFloatingPointUnitsAddress = (UINTN)InitializeFloatingPointUnits;
> +
> //
> // Get the BSP's data of GDT and IDT
> //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 989b3f8..ea56412 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -177,6 +177,7 @@ typedef struct {
> UINTN InitFlag;
> CPU_INFO_IN_HOB *CpuInfo;
> CPU_MP_DATA *CpuMpData;
> + UINTN InitializeFloatingPointUnitsAddress;
> } MP_CPU_EXCHANGE_INFO;
>
> #pragma pack()
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> index a63cd23..852281a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> @@ -40,4 +40,5 @@ EnableExecuteDisableLocation equ LockLocation + 5Ch
> Cr3Location equ LockLocation + 64h
> InitFlagLocation equ LockLocation + 6Ch
> CpuInfoLocation equ LockLocation + 74h
> +InitializeFloatingPointUnitsAddress equ LockLocation + 84h
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index fa54d01..0b14a53 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -1,5 +1,5 @@
> ;------------------------------------------------------------------------------ ;
> -; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
> ; This program and the accompanying materials
> ; are licensed and made available under the terms and conditions of the BSD License
> ; which accompanies this distribution. The full text of the license may be found at
> @@ -201,7 +201,7 @@ CProcedureInvoke:
> push rbp
> mov rbp, rsp
>
> - mov rax, ASM_PFX(InitializeFloatingPointUnits)
> + mov rax, qword [esi + InitializeFloatingPointUnitsAddress]
> sub rsp, 20h
> call rax ; Call assembly function to initialize FPU per UEFI spec
> add rsp, 20h
> @@ -282,11 +282,11 @@ AsmRelocateApLoopEnd:
> ;-------------------------------------------------------------------------------------
> global ASM_PFX(AsmGetAddressMap)
> ASM_PFX(AsmGetAddressMap):
> - mov rax, ASM_PFX(RendezvousFunnelProc)
> + lea rax, [ASM_PFX(RendezvousFunnelProc)]
> mov qword [rcx], rax
> mov qword [rcx + 8h], LongModeStart - RendezvousFunnelProcStart
> mov qword [rcx + 10h], RendezvousFunnelProcEnd - RendezvousFunnelProcStart
> - mov rax, ASM_PFX(AsmRelocateApLoop)
> + lea rax, [ASM_PFX(AsmRelocateApLoop)]
> mov qword [rcx + 18h], rax
> mov qword [rcx + 20h], AsmRelocateApLoopEnd - AsmRelocateApLoopStart
> ret
> --
> 2.6.3.windows.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-05-22 17:12 [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues Michael Kinney
2017-05-22 17:14 ` Andrew Fish
@ 2017-05-23 2:08 ` Fan, Jeff
2017-06-06 19:41 ` H. Peter Anvin
1 sibling, 1 reply; 9+ messages in thread
From: Fan, Jeff @ 2017-05-23 2:08 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Andrew Fish
Reviewed-by: Jeff Fan <jeff.fan@intel.com>
-----Original Message-----
From: Kinney, Michael D
Sent: Tuesday, May 23, 2017 1:13 AM
To: edk2-devel@lists.01.org
Cc: Andrew Fish; Fan, Jeff; Kinney, Michael D
Subject: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
https://bugzilla.tianocore.org/show_bug.cgi?id=565
Fix NASM compatibility issues with XCODE5 tool chain.
The XCODE5 tool chain for X64 builds using PIE (Position Independent Executable). For most assembly sources using PIE mode does not cause any issues.
However, if assembly code is copied to a different address (such as AP startup code in the MpInitLib), then the
X64 assembly source must be implemented to be compatible with PIE mode that uses RIP relative addressing.
The specific changes in this patch are:
* Use LEA instruction instead of MOV instruction to lookup
the addresses of functions.
* The assembly function RendezvousFunnelProc() is copied
below 1MB so it can be executed as part of the MpInitLib
AP startup sequence. RendezvousFunnelProc() calls the
external function InitializeFloatingPointUnits(). The
absolute address of InitializeFloatingPointUnits() is
added to the MP_CPU_EXCHANGE_INFO structure that is passed
to RendezvousFunnelProc().
Cc: Andrew Fish <afish@apple.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 ++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 1 +
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 ++++----
4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 407c44c..735e099 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -751,6 +751,8 @@ FillExchangeInfoData (
ExchangeInfo->EnableExecuteDisable = IsBspExecuteDisableEnabled ();
+ ExchangeInfo->InitializeFloatingPointUnitsAddress =
+ (UINTN)InitializeFloatingPointUnits;
+
//
// Get the BSP's data of GDT and IDT
//
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 989b3f8..ea56412 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -177,6 +177,7 @@ typedef struct {
UINTN InitFlag;
CPU_INFO_IN_HOB *CpuInfo;
CPU_MP_DATA *CpuMpData;
+ UINTN InitializeFloatingPointUnitsAddress;
} MP_CPU_EXCHANGE_INFO;
#pragma pack()
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index a63cd23..852281a 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -40,4 +40,5 @@ EnableExecuteDisableLocation equ LockLocation + 5Ch
Cr3Location equ LockLocation + 64h
InitFlagLocation equ LockLocation + 6Ch
CpuInfoLocation equ LockLocation + 74h
+InitializeFloatingPointUnitsAddress equ LockLocation + 84h
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index fa54d01..0b14a53 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -1,5 +1,5 @@
;------------------------------------------------------------------------------ ; -; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2015 - 2017, Intel Corporation. All rights
+reserved.<BR>
; This program and the accompanying materials ; are licensed and made available under the terms and conditions of the BSD License ; which accompanies this distribution. The full text of the license may be found at @@ -201,7 +201,7 @@ CProcedureInvoke:
push rbp
mov rbp, rsp
- mov rax, ASM_PFX(InitializeFloatingPointUnits)
+ mov rax, qword [esi + InitializeFloatingPointUnitsAddress]
sub rsp, 20h
call rax ; Call assembly function to initialize FPU per UEFI spec
add rsp, 20h
@@ -282,11 +282,11 @@ AsmRelocateApLoopEnd:
;-------------------------------------------------------------------------------------
global ASM_PFX(AsmGetAddressMap)
ASM_PFX(AsmGetAddressMap):
- mov rax, ASM_PFX(RendezvousFunnelProc)
+ lea rax, [ASM_PFX(RendezvousFunnelProc)]
mov qword [rcx], rax
mov qword [rcx + 8h], LongModeStart - RendezvousFunnelProcStart
mov qword [rcx + 10h], RendezvousFunnelProcEnd - RendezvousFunnelProcStart
- mov rax, ASM_PFX(AsmRelocateApLoop)
+ lea rax, [ASM_PFX(AsmRelocateApLoop)]
mov qword [rcx + 18h], rax
mov qword [rcx + 20h], AsmRelocateApLoopEnd - AsmRelocateApLoopStart
ret
--
2.6.3.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-05-23 2:08 ` Fan, Jeff
@ 2017-06-06 19:41 ` H. Peter Anvin
2017-06-06 20:49 ` Andrew Fish
2017-06-13 23:15 ` Kinney, Michael D
0 siblings, 2 replies; 9+ messages in thread
From: H. Peter Anvin @ 2017-06-06 19:41 UTC (permalink / raw)
To: Fan, Jeff, Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Andrew Fish
On 05/22/17 19:08, Fan, Jeff wrote:
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index fa54d01..0b14a53 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -1,5 +1,5 @@
> ;------------------------------------------------------------------------------ ; -; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2015 - 2017, Intel Corporation. All rights
> +reserved.<BR>
> ; This program and the accompanying materials ; are licensed and made available under the terms and conditions of the BSD License ; which accompanies this distribution. The full text of the license may be found at @@ -201,7 +201,7 @@ CProcedureInvoke:
> push rbp
> mov rbp, rsp
>
> - mov rax, ASM_PFX(InitializeFloatingPointUnits)
> + mov rax, qword [esi + InitializeFloatingPointUnitsAddress]
> sub rsp, 20h
> call rax ; Call assembly function to initialize FPU per UEFI spec
> add rsp, 20h
FYI, the qword specifier is unnecessary since you are already specifying
rax.
However, why not simply drop the use of rax entirely and do:
call [esi + InitializeFloatingPointUnitsAddress]
(Also: is this *really* supposed to be esi and not rsi? The former
means a 32-bit address.)
-hpa
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-06-06 19:41 ` H. Peter Anvin
@ 2017-06-06 20:49 ` Andrew Fish
2017-06-06 21:05 ` hpa
2017-06-13 23:15 ` Kinney, Michael D
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Fish @ 2017-06-06 20:49 UTC (permalink / raw)
To: Fan, Jeff; +Cc: Mike Kinney, edk2-devel@lists.01.org, H. Peter Anvin
> On Jun 6, 2017, at 12:41 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> On 05/22/17 19:08, Fan, Jeff wrote:
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>> index fa54d01..0b14a53 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>> @@ -1,5 +1,5 @@
>> ;------------------------------------------------------------------------------ ; -; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
>> +; Copyright (c) 2015 - 2017, Intel Corporation. All rights
>> +reserved.<BR>
>> ; This program and the accompanying materials ; are licensed and made available under the terms and conditions of the BSD License ; which accompanies this distribution. The full text of the license may be found at @@ -201,7 +201,7 @@ CProcedureInvoke:
>> push rbp
>> mov rbp, rsp
>>
>> - mov rax, ASM_PFX(InitializeFloatingPointUnits)
>> + mov rax, qword [esi + InitializeFloatingPointUnitsAddress]
Does nasm remove the need for the ASM_PFX() macro? That macro hides if C is decorating with a _ prefix.
Also given it is a #define (equ) why do we use camel case vs. all caps?
Thanks,
Andrew Fish
>> sub rsp, 20h
>> call rax ; Call assembly function to initialize FPU per UEFI spec
>> add rsp, 20h
>
> FYI, the qword specifier is unnecessary since you are already specifying
> rax.
>
> However, why not simply drop the use of rax entirely and do:
>
> call [esi + InitializeFloatingPointUnitsAddress]
>
> (Also: is this *really* supposed to be esi and not rsi? The former
> means a 32-bit address.)
>
> -hpa
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-06-06 20:49 ` Andrew Fish
@ 2017-06-06 21:05 ` hpa
2017-06-06 21:15 ` Andrew Fish
0 siblings, 1 reply; 9+ messages in thread
From: hpa @ 2017-06-06 21:05 UTC (permalink / raw)
To: Andrew Fish, Fan, Jeff; +Cc: Mike Kinney, edk2-devel@lists.01.org
On June 6, 2017 1:49:34 PM PDT, Andrew Fish <afish@apple.com> wrote:
>
>> On Jun 6, 2017, at 12:41 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> On 05/22/17 19:08, Fan, Jeff wrote:
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>> index fa54d01..0b14a53 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>> @@ -1,5 +1,5 @@
>>>
>;------------------------------------------------------------------------------
>; -; Copyright (c) 2015 - 2016, Intel Corporation. All rights
>reserved.<BR>
>>> +; Copyright (c) 2015 - 2017, Intel Corporation. All rights
>>> +reserved.<BR>
>>> ; This program and the accompanying materials ; are licensed and
>made available under the terms and conditions of the BSD License ;
>which accompanies this distribution. The full text of the license may
>be found at @@ -201,7 +201,7 @@ CProcedureInvoke:
>>> push rbp
>>> mov rbp, rsp
>>>
>>> - mov rax, ASM_PFX(InitializeFloatingPointUnits)
>>> + mov rax, qword [esi +
>InitializeFloatingPointUnitsAddress]
>
>Does nasm remove the need for the ASM_PFX() macro? That macro hides if
>C is decorating with a _ prefix.
>
>Also given it is a #define (equ) why do we use camel case vs. all caps?
>
>Thanks,
>
>Andrew Fish
>
>>> sub rsp, 20h
>>> call rax ; Call assembly function to
>initialize FPU per UEFI spec
>>> add rsp, 20h
>>
>> FYI, the qword specifier is unnecessary since you are already
>specifying
>> rax.
>>
>> However, why not simply drop the use of rax entirely and do:
>>
>> call [esi + InitializeFloatingPointUnitsAddress]
>>
>> (Also: is this *really* supposed to be esi and not rsi? The former
>> means a 32-bit address.)
>>
>> -hpa
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
The NASM command-line option --prefix _ should do exactly that.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-06-06 21:05 ` hpa
@ 2017-06-06 21:15 ` Andrew Fish
2017-06-06 21:21 ` hpa
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Fish @ 2017-06-06 21:15 UTC (permalink / raw)
To: hpa; +Cc: Fan, Jeff, Mike Kinney, edk2-devel@lists.01.org
> On Jun 6, 2017, at 2:05 PM, hpa@zytor.com wrote:
>
> On June 6, 2017 1:49:34 PM PDT, Andrew Fish <afish@apple.com> wrote:
>>
>>> On Jun 6, 2017, at 12:41 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>>
>>> On 05/22/17 19:08, Fan, Jeff wrote:
>>>>
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>>> index fa54d01..0b14a53 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>>> @@ -1,5 +1,5 @@
>>>>
>> ;------------------------------------------------------------------------------
>> ; -; Copyright (c) 2015 - 2016, Intel Corporation. All rights
>> reserved.<BR>
>>>> +; Copyright (c) 2015 - 2017, Intel Corporation. All rights
>>>> +reserved.<BR>
>>>> ; This program and the accompanying materials ; are licensed and
>> made available under the terms and conditions of the BSD License ;
>> which accompanies this distribution. The full text of the license may
>> be found at @@ -201,7 +201,7 @@ CProcedureInvoke:
>>>> push rbp
>>>> mov rbp, rsp
>>>>
>>>> - mov rax, ASM_PFX(InitializeFloatingPointUnits)
>>>> + mov rax, qword [esi +
>> InitializeFloatingPointUnitsAddress]
>>
>> Does nasm remove the need for the ASM_PFX() macro? That macro hides if
>> C is decorating with a _ prefix.
>>
>> Also given it is a #define (equ) why do we use camel case vs. all caps?
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>>> sub rsp, 20h
>>>> call rax ; Call assembly function to
>> initialize FPU per UEFI spec
>>>> add rsp, 20h
>>>
>>> FYI, the qword specifier is unnecessary since you are already
>> specifying
>>> rax.
>>>
>>> However, why not simply drop the use of rax entirely and do:
>>>
>>> call [esi + InitializeFloatingPointUnitsAddress]
>>>
>>> (Also: is this *really* supposed to be esi and not rsi? The former
>>> means a 32-bit address.)
>>>
>>> -hpa
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
> The NASM command-line option --prefix _ should do exactly that.
How does it know when to prefix the _? That could break dead stripping with the Xcode linker. L is used to imply local symbol, don't dead strip.
Thanks,
Andrew Fish
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-06-06 21:15 ` Andrew Fish
@ 2017-06-06 21:21 ` hpa
0 siblings, 0 replies; 9+ messages in thread
From: hpa @ 2017-06-06 21:21 UTC (permalink / raw)
To: Andrew Fish; +Cc: Fan, Jeff, Mike Kinney, edk2-devel@lists.01.org
On June 6, 2017 2:15:08 PM PDT, Andrew Fish <afish@apple.com> wrote:
>
>> On Jun 6, 2017, at 2:05 PM, hpa@zytor.com wrote:
>>
>> On June 6, 2017 1:49:34 PM PDT, Andrew Fish <afish@apple.com> wrote:
>>>
>>>> On Jun 6, 2017, at 12:41 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>>>
>>>> On 05/22/17 19:08, Fan, Jeff wrote:
>>>>>
>>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>>>> index fa54d01..0b14a53 100644
>>>>> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>>>> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
>>>>> @@ -1,5 +1,5 @@
>>>>>
>>>
>;------------------------------------------------------------------------------
>>> ; -; Copyright (c) 2015 - 2016, Intel Corporation. All rights
>>> reserved.<BR>
>>>>> +; Copyright (c) 2015 - 2017, Intel Corporation. All rights
>>>>> +reserved.<BR>
>>>>> ; This program and the accompanying materials ; are licensed and
>>> made available under the terms and conditions of the BSD License ;
>>> which accompanies this distribution. The full text of the license
>may
>>> be found at @@ -201,7 +201,7 @@ CProcedureInvoke:
>>>>> push rbp
>>>>> mov rbp, rsp
>>>>>
>>>>> - mov rax, ASM_PFX(InitializeFloatingPointUnits)
>>>>> + mov rax, qword [esi +
>>> InitializeFloatingPointUnitsAddress]
>>>
>>> Does nasm remove the need for the ASM_PFX() macro? That macro hides
>if
>>> C is decorating with a _ prefix.
>>>
>>> Also given it is a #define (equ) why do we use camel case vs. all
>caps?
>>>
>>> Thanks,
>>>
>>> Andrew Fish
>>>
>>>>> sub rsp, 20h
>>>>> call rax ; Call assembly function to
>>> initialize FPU per UEFI spec
>>>>> add rsp, 20h
>>>>
>>>> FYI, the qword specifier is unnecessary since you are already
>>> specifying
>>>> rax.
>>>>
>>>> However, why not simply drop the use of rax entirely and do:
>>>>
>>>> call [esi + InitializeFloatingPointUnitsAddress]
>>>>
>>>> (Also: is this *really* supposed to be esi and not rsi? The former
>>>> means a 32-bit address.)
>>>>
>>>> -hpa
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>> The NASM command-line option --prefix _ should do exactly that.
>
>How does it know when to prefix the _? That could break dead stripping
>with the Xcode linker. L is used to imply local symbol, don't dead
>strip.
>
>Thanks,
>
>Andrew Fish
>
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
It appends the underscore to global or external variables.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
2017-06-06 19:41 ` H. Peter Anvin
2017-06-06 20:49 ` Andrew Fish
@ 2017-06-13 23:15 ` Kinney, Michael D
1 sibling, 0 replies; 9+ messages in thread
From: Kinney, Michael D @ 2017-06-13 23:15 UTC (permalink / raw)
To: H. Peter Anvin, Fan, Jeff, edk2-devel@lists.01.org,
Kinney, Michael D
Cc: Andrew Fish
hpa,
Use of esi is on purpose. esi is the base address of a structure and
it is consistently used as a 32-bit value in all 3 execution modes in
this file.
I agree we can remove the qword specifier as a cleaner style.
Also, as we consolidate on NASM sources, we can see if ASM_PFX() can be
dropped completely. That would be another good clean up.
I also agree that the mov and call can be reduced to a single call
instruction. Another good cleanup.
There are many more NASM source files that need to be updated to be
compatible with XCODE5 tool chain. We are continuing to work on those
updates.
Thanks,
Mike
-----Original Message-----
From: H. Peter Anvin [mailto:hpa@zytor.com]
Sent: Tuesday, June 6, 2017 12:42 PM
To: Fan, Jeff <jeff.fan@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org <edk2-devel@ml01.01.org>
Cc: Andrew Fish <afish@apple.com>
Subject: Re: [edk2] [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues
On 05/22/17 19:08, Fan, Jeff wrote:
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index fa54d01..0b14a53 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -1,5 +1,5 @@
> ;------------------------------------------------------------------------------ ; -; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2015 - 2017, Intel Corporation. All rights
> +reserved.<BR>
> ; This program and the accompanying materials ; are licensed and made available under the terms and conditions of the BSD License ; which accompanies this distribution. The full text of the license may be found at @@ -201,7 +201,7 @@ CProcedureInvoke:
> push rbp
> mov rbp, rsp
>
> - mov rax, ASM_PFX(InitializeFloatingPointUnits)
> + mov rax, qword [esi + InitializeFloatingPointUnitsAddress]
> sub rsp, 20h
> call rax ; Call assembly function to initialize FPU per UEFI spec
> add rsp, 20h
FYI, the qword specifier is unnecessary since you are already specifying
rax.
However, why not simply drop the use of rax entirely and do:
call [esi + InitializeFloatingPointUnitsAddress]
(Also: is this *really* supposed to be esi and not rsi? The former
means a 32-bit address.)
-hpa
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-13 23:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-22 17:12 [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues Michael Kinney
2017-05-22 17:14 ` Andrew Fish
2017-05-23 2:08 ` Fan, Jeff
2017-06-06 19:41 ` H. Peter Anvin
2017-06-06 20:49 ` Andrew Fish
2017-06-06 21:05 ` hpa
2017-06-06 21:15 ` Andrew Fish
2017-06-06 21:21 ` hpa
2017-06-13 23:15 ` Kinney, Michael D
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox