Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error

["Guo Dong" <guo.dong@intel.com>]

Hi Laszlo,

See my comments below.
Thanks,
Guo

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, December 3, 2020 3:22 AM
> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul1 <rahul1.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
> 
> On 12/02/20 22:38, Guo Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084
> >
> > When DXE drivers are dispatched above 4GB memory and
> > the system is already in 64bit mode, the address
> > setCodeSelectorLongJump in stack will be override
> > by parameter. so change to use 64bit address and
> > jump to qword address.
> >
> > Signed-off-by: Guo Dong <guo.dong@intel.com>
> > ---
> >  UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > index c3489bcc3e..6ad32b49f4 100644
> > --- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > +++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > @@ -23,8 +23,8 @@ ASM_PFX(SetCodeSelector):
> >      sub     rsp, 0x10
> >      lea     rax, [setCodeSelectorLongJump]
> >      mov     [rsp], rax
> 
> I'll let Ray and Eric review this patch, just one question for my
> understanding:
> 
> for clarity, shouldn't the above MOV have referred to "eax" in the first
> place? Because (before this patch) it seems to store 8 bytes (from rax),
> and then to overwrite the high-address DWORD of those 8 bytes, with the
> next operation (which stores 2 bytes from CX).
> 
> IMO, for clarity's sake, the original code should have used EAX. Why
> write 8 bytes when only the low-address 4 bytes matter anyway?
>
These asm code is in X64 folder for long mode, the variable address could 
be below 4GB or above 4GB. If it is below 4GB, using eax or overriding the 
high 4 bytes doesn't matter. If it is in above 4GB, then the high 4 bytes 
matters, it should not be override by others. 
 
> Anyhow, this is no longer relevant, because of the present patch; I just
> wanted to ask about it so I understand better.
> 
> Thanks
> Laszlo
> 
> > -    mov     [rsp+4], cx
> > -    jmp     dword far [rsp]
> > +    mov     [rsp+8], cx
> > +    jmp     qword far [rsp]
> >  setCodeSelectorLongJump:
> >      add     rsp, 0x10
> >      ret
> >