Re: [Patch V2] UefiCpuPkg/MpInitLib: Fix X64 XCODE5/NASM compatibility issues

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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