Re: [edk2] [PATCH] UefiCpuPkg/.../X64/ExceptionHandlerAsm.S: 64-bit relocation for variables

Subject: Re: [edk2] [PATCH] UefiCpuPkg/.../X64/ExceptionHandlerAsm.S: 64-bit relocation for variables

From: "Fan, Jeff" <jeff.fan@intel.com>

To: Jordan Justen <jljusten@gmail.com>

Date: 2013-11-29 12:19:14

Jordan,

I agree that sending patch to edk2-devel for review is really necessary for such big changing.:-)

Thanks!
Jeff
-----Original Message-----
From: Jordan Justen [mailto:jljusten@gmail.com] 
Sent: Friday, November 29, 2013 11:45 AM
To: Fan, Jeff
Cc: edk2-devel@lists.sourceforge.net; Andrew J. Fish; Laszlo Ersek
Subject: Re: [edk2] [PATCH] UefiCpuPkg/.../X64/ExceptionHandlerAsm.S: 64-bit relocation for variables

On Thu, Nov 28, 2013 at 5:07 PM, Fan, Jeff  wrote:
> Thanks your great effort to help to investigate this issue and fix it. I will help you to check-in your patch after verification.
>
> Reviewed-by: Jeff Fan 

Jeff,

I think you should check this in after your verification, since it fixes this issue with GCC.

But I would also like to know if this new code works with CLANG. I think CLANG has some additional considerations. (Only allowing IP relative asm code. ??) If Andrew has time, he might be able to verify it and potentially add a follow up patch for CLANG.

Once again, I'd just like to say that r14885 is the kind of change ought to be reviewed on edk2-devel before being committed. (Okay, I've probably mentioned this too many times now. But, I'm still convinced that every edk2 change should be reviewed on edk2-devel before being
committed.)

Of course edk2-devel review is not going to guarantee we avoid issues like this, but it should help lessen the chance.

-Jordan

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, November 29, 2013 7:31 AM
> To: edk2-devel@lists.sourceforge.net
> Subject: [edk2] [PATCH] UefiCpuPkg/.../X64/ExceptionHandlerAsm.S: 
> 64-bit relocation for variables
>
> This assembly file declares the following seven symbols:
>
>   CommonExceptionHandler
>   CommonInterruptEntry
>   HookAfterStubHeaderEnd
>   mErrorCodeFlag
>   mDoFarReturnFlag
>   AsmGetTemplateAddressMap
>   AsmVectorNumFixup
>
> Most of them are either relocated with 64-bit or RIP-relative relocation entries (R_X86_64_64, R_X86_64_PC32), or need no relocations at all.
> However, the following two are incorrectly relocated, with 32-bit 
> entries
> (R_X86_64_32S):
>
>   mErrorCodeFlag -- first use, in HookAfterStubHeaderEnd
>   mDoFarReturnFlag
>
> They are referenced in instructions that can only express 32-bit signed displacement. After the CpuDxe binary is loaded and relocated to a high enough address (above approx. 2GB), these instructions reference invalid addresses and cause page faults.
>
> The second use of mErrorCodeFlag (in CommonInterruptEntry) is correct; it uses RIP-relative addressing, and the relocation used for the leaq instruction (R_X86_64_PC32) doesn't interfere. Yet the patch changes that use too, for uniformity with the first (fixed) use.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  .../CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.S       | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git 
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.S 
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.S
> index 7d5672f..6e80d84 100644
> --- 
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.S
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.
> +++ S
> @@ -273,7 +273,10 @@ ASM_PFX(HookAfterStubHeaderEnd):
>      andl    $0x0fffffff0, %esp
>      pushq   %rcx
>      movq    8(%rax), %rcx
> -    bt      %ecx, ASM_PFX(mErrorCodeFlag)
> +    pushq   %rax
> +    movabsl ASM_PFX(mErrorCodeFlag), %eax
> +    bt      %ecx, %eax
> +    popq    %rax
>      jc      NoErrorData
>      pushq   (%rsp)            # push additional rcx to make stack alignment
>  NoErrorData:
> @@ -301,8 +304,8 @@ ASM_PFX(CommonInterruptEntry):
>      cmp     $32, %ecx          # Intel reserved vector for exceptions?
>      jae     NoErrorCode
>      pushq   %rax
> -    leaq    ASM_PFX(mErrorCodeFlag)(%rip), %rax
> -    bt      %ecx, (%rax)
> +    movabsl ASM_PFX(mErrorCodeFlag), %eax
> +    bt      %ecx, %eax
>      popq    %rax
>      jc      CommonInterruptEntry_al_0000
>
> @@ -549,7 +552,10 @@ ErrorCode:
>      jmp     *-24(%rsp)
>
>  DoReturn:
> -    cmpq    $0, ASM_PFX(mDoFarReturnFlag)   # Check if need to do far return instead of IRET
> +    pushq   %rax
> +    movabsq ASM_PFX(mDoFarReturnFlag), %rax
> +    cmpq    $0, %rax          # Check if need to do far return instead of IRET
> +    popq    %rax
>      jz      DoIret
>      pushq   %rax
>      movq    %rsp, %rax        # save old RSP to rax
> --
> 1.8.3.1
>
>
> ----------------------------------------------------------------------
> -------- Rapidly troubleshoot problems before they affect your 
> business. Most IT organizations don't have a clear picture of how 
> application performance affects their revenue. With AppDynamics, you 
> get 100% visibility into your Java,.NET, & PHP application. Start your 
> 15-day FREE TRIAL of AppDynamics Pro!
> http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.c
> lktrk _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
> ----------------------------------------------------------------------
> -------- Rapidly troubleshoot problems before they affect your 
> business. Most IT organizations don't have a clear picture of how 
> application performance affects their revenue. With AppDynamics, you 
> get 100% visibility into your Java,.NET, & PHP application. Start your 
> 15-day FREE TRIAL of AppDynamics Pro!
> http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.c
> lktrk _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT 
organizations don't have a clear picture of how application performance 
affects their revenue. With AppDynamics, you get 100% visibility into your 
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel